nodejs / webcrypto

This repository has been archived. The WebCrypto API has been implemented in recent versions of Node.js and does not require additional packages.
69 stars 20 forks source link

Add unsafeExportKey #45

Closed tniessen closed 3 years ago

tniessen commented 4 years ago

This adds an exported function unsafeExportKey (which cannot be accessed through any of the standardized interfaces, classes, and functions), which allows to export non-extractable keys.

Since all key types (will eventually) support JWK, all keys can be exported as JWK and then imported as usual.

I would appreciate your opinions @panva @sam-github.

Fixes: #13

sam-github commented 4 years ago

Its hard to tell from just the diff, what's the export format? JSON object?

No strong feelings, other than being loathe to extend a (semi-?)standardized API at the price of maintaining the extension forever.

I would consider allowing conversion to/from nodejs/crypto key objects as a more generally useful feature. It would allow full interop between the WebCrypto API and nodejs crypto API, including to any algorithms supported only by the other. This seems not very controversial, and would allow key material access via node's crypto key methods.

tniessen commented 4 years ago

Its hard to tell from just the diff, what's the export format? JSON object?

Sorry, I'll eventually have to add documentation. It's behavior is the same as crypto.subtle.exportKey(format, key), so the export format depends on the first parameter (jwk, pkcs8, spki, raw). The only difference to exportKey is that unsafeExportKey allows exporting non-extractable keys.

No strong feelings, other than being loathe to extend a (semi-?)standardized API at the price of maintaining the extension forever.

I agree that some refactoring might be necessary. In a non-web context, it's difficult to separate the API from its extensions without making it less intuitive.

To provide some context, this is the relevant part of the spec:

[NoInterfaceObject,Exposed=(Window,Worker)]
interface GlobalCrypto {
  readonly attribute Crypto crypto;
};

Window implements GlobalCrypto;
WorkerGlobalScope implements GlobalCrypto;        

[Exposed=(Window,Worker)]
interface Crypto {
  [SecureContext] readonly attribute SubtleCrypto subtle;
  ArrayBufferView getRandomValues(ArrayBufferView array);
};

Based on these definitions, require('@nodejs/webcrypto') currently implements the GlobalCrypto interface, but extends it with the CryptoKey class. This is not part of the spec (as far as I can tell), but browsers do it, too, and it is required for the web-platform-tests to pass.

This PR would extend the package exports with another property, unsafeExportKey. Maybe we should split the export into { api, extensions } or something similar? Or { api, embed }?

I don't have a strong opinion on this feature either, but I understand the motivation behind #13.

sam-github commented 4 years ago

I wonder if this version of WebCrypto should just preserve the extractable settings on import/export/create. but ignore them.

Pretending that the api can do secure key storage is likely to confuse people into thinking that the api is doing secure key storage, which is a terribly risky place to be.

sam-github commented 4 years ago

And if ignoring the extractable setting isn't the right thing to do, then yes, this seems reasonable.

tniessen commented 4 years ago

I wonder if this version of WebCrypto should just preserve the extractable settings on import/export/create. but ignore them.

I think WebCrypto was designed with "programmer mistakes" in mind. For example, it seems redundant that each operation requires an algorithm name and a key which has already been associated with the exact same algorithm. We also cannot protect against users modifying the usages property and using keys for operations that they were not created for. I think extractable falls into the same category: There are ways to bypass it, but it should be very difficult to accidentally export non-extractable keys.

sam-github commented 4 years ago

I think extractable is more than a guideline when WebCrypto runs in a browser. Browsers run js in a sandbox, and have external mechanisms/GUIs/config etc. where keys can be stored, and they can absolutely guarantee that the key material even of a key who's generation was requested from js can't be accessed by js, and that the key can't be used for a purpose other than that indicated at generation time.

WebCrypto in node can't make such a guarantee, or at least, I can't see how, not unless node had a key persistence system built into it. Hm. That might even be possible with OpenSSL 3.0 with its providers, but that's a ways off. I've no crystal ball, but I predict that the mere presence of extractable in the API will make webdevelopers assume that Node.js implements this as securely as WebBrowsers.The nice thing about the future is we will have the chance to see if my predictions are accurate! :-)

panva commented 4 years ago

key_ops, use, alg (needed for RSA keys) are only exportable using JWK format, that is if you wish to be able to re-import it to the same functioning CryptoKey.

and for oct keys we have no webcrypto-supported format other than JWK anyway.

tniessen commented 4 years ago

@sam-github I tend to agree, but there is no way to implement WebCrypto without conforming to its definition of extractable etc.

Would it help if I moved this to a separate module, so it wouldn't be included in the package's main exports? e.g., if we call the main package @nodejs/webcrypto, I could move unsafeExportKey to @nodejs/webcrypto/embedding.

sam-github commented 4 years ago

I don't have a problem with leaving it as you proposed, seems ok to me.