Skip to content

flesh out webcrypto #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 17, 2016
Merged

flesh out webcrypto #60

merged 1 commit into from
May 17, 2016

Conversation

rapropos
Copy link
Contributor

This should, among other things, sort of address TS issue #3984. Fully making the return type a Promise instead of a PromiseLike is dependent on some sort of resolution of the ability to target the ES6 library separately. Reference document used to create these typings is the 2014.12 W3C WebCrypto API CR.

Generally speaking, this should make code work that used to not compile (such as importing JSON web keys). The only more restrictive change, I believe, is making the name property of Algorithm no longer optional. It is explicitly documented as required in the W3C document.

@msftclas
Copy link

Hi @rapropos, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

unwrapKey(format: string, wrappedKey: ArrayBufferView, unwrappingKey: CryptoKey, unwrapAlgorithm: string | Algorithm, unwrappedKeyAlgorithm: string | Algorithm, extractable: boolean, keyUsages: string[]): any;
verify(algorithm: string | Algorithm, key: CryptoKey, signature: ArrayBufferView, data: ArrayBufferView): any;
wrapKey(format: string, key: CryptoKey, wrappingKey: CryptoKey, wrapAlgorithm: string | Algorithm): any;
decrypt(algorithm: string | RsaOaepParams | AesCtrParams | AesCbcParams | AesCmacParams | AesGcmParams | AesCfbParams, key: CryptoKey, data: ArrayBuffer | ArrayBufferView): PromiseLike<ArrayBuffer>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the algorithm parameter is of type AlgorithmIdentifier, which can be defined as string|Algorithm. not sure i see the point of enumerating all possible Algorithm types here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that also applies to all the other ones. i would suggest adding a new type alias:

type AlgorithmIdentifier = string | Algorithm;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure i see the point of enumerating all possible Algorithm types here

I'm not sure I fully understood the point of this comment until looking at it again now that we have the ability to write type aliases. Assuming you are asking "why are you listing all those xxxParams types instead of just using AlgorithmIdentifier?", the reason is that different algorithms have different allowed purposes, and I thought having them all enumerated would help users see at a glance which algorithms are usable for (e.g.) encrypting/decrypting and which are usable for signing/verifying.

@rapropos
Copy link
Contributor Author

rapropos commented Feb 1, 2016

Thank you for taking the time to comment. The documentation lists available types for adding as "property, method, interface, constructor, or indexer". None of those looked like they would be appropriate for defining the AlgorithmIdentifier and BufferSource aliases that you suggest, so I am unclear as to the proper procedure for defining them.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 1, 2016

@zhengbli, how can we define a type alias?

}

interface JsonWebKey {
kty: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why kty is required while others are not? I didn't see any differences among these properties from the spec here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhengbli: kty is required because of the comment in RFC 7517 section 4.1 that says:

This member MUST be present in a JWK.

@zhengbli
Copy link
Contributor

Sorry for the delay. With the latest updates in master, you can define a type alias like the follows:

    {
        "kind": "typedef",
        "name": "IDBValidKey",
        "type": "number | string | Date | IDBArrayKey"
    }

@rapropos
Copy link
Contributor Author

Having the algorithm parameter to importKey be strictly an AlgorithmIdentifier instead of enumerating the possible types causes problems in my client code:

this.subtle.importKey("jwk", pubjwk,
                                {name: "RSASSA-PKCS1-v1_5", hash: {name: "SHA-256"}},
                                true, ["verify"])
                                .then((pubkey:CryptoKey) => {
                                    this.accessKeys.publicKey = pubkey;
                                })
Argument of type '{ name: string; hash: { name: string; }; }' is not assignable to parameter of type 'string | Algorithm'.
  Object literal may only specify known properties, and 'hash' does not exist in type 'string | Algorithm'.

@rapropos
Copy link
Contributor Author

CI seems to have failed due to some sort of network error. Is there a way I can try to rerun it?

@zhengbli
Copy link
Contributor

Seems like npm can't install tsd for now for some reason, tested on my local machine with the same result. I'll rerun the test later when it recovers.

@rapropos
Copy link
Contributor Author

Squashed and rebased.

digest(algorithm: AlgorithmIdentifier, data: BufferSource): PromiseLike<ArrayBuffer>;
encrypt(algorithm: string | RsaOaepParams | AesCtrParams | AesCbcParams | AesCmacParams | AesGcmParams | AesCfbParams, key: CryptoKey, data: BufferSource): PromiseLike<ArrayBuffer>;
exportKey(format: "jwk", key: CryptoKey): PromiseLike<JsonWebKey>;
exportKey(format: "raw" | "pkcs8" | "spki", key: CryptoKey): PromiseLike<ArrayBuffer>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs a string overload as well. otherwise somehing like:

var format = "jwk";
c.exportKey(format); /// Error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Added.

@samuelhorwitz
Copy link

Thank you for this! I've been trying it locally and it fixes all my issues with WebCrypto breaking due to bad type definitions. However, I am now wrestling with Promise versus PromiseLike sadly. Anyway, hopefully this gets merged into TS soon.

@rapropos
Copy link
Contributor Author

rapropos commented Apr 1, 2016

@samuelhorwitz: Depending on your framework, the PromiseLike/Promise business may turn out to be a blessing in disguise. I ran into some subtle problems with Angular2, whereby it inserts some zone magic into its Promises that isn't sprinkled on the things coming back from WebCrypto. Having the return value be a PromiseLike reminds me that I need to wrap everything coming back from WebCrypto in Promise.resolve() in order for the change detection to fire correctly. In any event, whatever your Promise library, that resolve wrap should be sufficient to convert a PromiseLike to a Promise.

@samuelhorwitz
Copy link

Yeah, I suppose so. I was just thrown through a loop because I guess there is no catch on promise likes and I just walked away from the code for a bit, frustrated about how just trying to upgrade Typescript led to so much issues. But anyway I found something else that might be wrong with the PR but I don't know enough about the specs to say:

interface EcKeyImportParams {
    typedCurve: string;
}

This should probably be namedCurve. That's what Chrome expects, at least. Not sure if this is some discrepancy between spec and implementation (haven't looked at the spec).

@rapropos
Copy link
Contributor Author

rapropos commented Apr 3, 2016

Incorporated @samuelhorwitz's catch on EcKeyImportParams and resquashed.

importKey(format: "raw" | "pkcs8" | "spki", keyData: BufferSource, algorithm: string | RsaHashedImportParams | EcKeyImportParams | HmacImportParams | DhImportKeyParams, extractable:boolean, keyUsages: string[]): PromiseLike<CryptoKey>;
importKey(format: string, keyData: JsonWebKey | BufferSource, algorithm: string | RsaHashedImportParams | EcKeyImportParams | HmacImportParams | DhImportKeyParams, extractable:boolean, keyUsages: string[]): PromiseLike<CryptoKey>;
sign(algorithm: string | RsaPssParams | EcdsaParams | AesCmacParams, key: CryptoKey, data: BufferSource): PromiseLike<ArrayBuffer>;
unwrapKey(format: string, wrappedKey: BufferSource, unwrappingKey: CryptoKey, unwrapAlgorithm: AlgorithmIdentifier, unwrappedKeyAlgorithm: AlgorithmIdentifier, extractable: boolean, keyUsages: string[]): PromiseLike<ArrayBuffer>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says the unwrapKey method returns PromiseLike<CryptoKey>

@zhengbli
Copy link
Contributor

zhengbli commented Apr 18, 2016

👍 @mhegazy further comments?

@rapropos rapropos force-pushed the webcrypto branch 2 times, most recently from 1cddfa9 to 52fbb4c Compare April 19, 2016 22:53
@rapropos
Copy link
Contributor Author

Rebased again. It sure would be convenient if the parser could process a bunch of separate files instead of having two monolithic ones. Would cut down on the merge conflicts a ton.

@mhegazy
Copy link
Contributor

mhegazy commented May 17, 2016

👍

@zhengbli
Copy link
Contributor

@rapropos Thank you for your contribution!

@zhengbli zhengbli merged commit e577388 into microsoft:master May 17, 2016
sandersn pushed a commit that referenced this pull request Jun 12, 2020
Previously, these methods returned PromiseLike, even though the spec has
always required them to return real Promises and browsers have always
done so. According to #60, this was done in order to work around #47;
now that #47 has been fixed, this workaround is obsolete.

This generally should not be a breaking change since it is making the
SubtleCrypto type contract stricter and that API is generally
implemented by the platform, not by users.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants