Skip to content
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

crypto: add signOneShot() and verifyOneShot() #26611

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@mscdex
Copy link
Contributor

mscdex commented Mar 12, 2019

These methods are to allow signing and verifying using Ed25519 and Ed448 keys, which do not support streaming of input data.

Fixes: #26320

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 12, 2019

@mscdex mscdex force-pushed the mscdex:crypto-sign-verify-eddsa-support branch from e1ee2d6 to 43be5b7 Mar 12, 2019

@tniessen
Copy link
Member

tniessen left a comment

Maybe we should consider other names? I think having such "one shot" APIs might be useful, also for hashing, but the names sound a bit awkward to me. Do we need to reserve the names sign, verify, hash etc. for something else?

@@ -2651,6 +2651,18 @@ added: v10.0.0
Enables the FIPS compliant crypto provider in a FIPS-enabled Node.js build.
Throws an error if FIPS mode is not available.

### crypto.signOneShot(data, privateKey)

This comment has been minimized.

@tniessen

tniessen Mar 12, 2019

Member

IIRC all other functions that consume keys take them as the first argument.

This comment has been minimized.

@mscdex

mscdex Mar 12, 2019

Author Contributor

I ordered them according to the ordinary streaming flow (.update(data) followed by .sign(privateKey)), which made sense to me.

This comment has been minimized.

@tniessen

tniessen Mar 12, 2019

Member

I understand that and it makes sense, but it deviates from all other functions.

added: REPLACEME
-->
* `data` {Buffer | TypedArray | DataView}
* `privateKey` {Buffer | KeyObject}

This comment has been minimized.

@tniessen

tniessen Mar 12, 2019

Member

This deviates from other functions that consume keys, where they can also be strings and certain objects.

This comment has been minimized.

@mscdex

mscdex Mar 12, 2019

Author Contributor

Supporting string encoding names complicates things and makes separating required vs. optional function arguments nearly impossible as far as I can tell.

Besides, the only reason we have the string encoding parameters is for backwards compatibility and internally node immediately converts it to a Buffer using Buffer.from(), something userland could easily do themselves these days...

This comment has been minimized.

@tniessen

tniessen Mar 12, 2019

Member

I'm not talking about the data, only about privateKey. See e.g. privateDecrypt:

If privateKey is not a KeyObject, this function behaves as if privateKey had been passed to crypto.createPrivateKey().

I designed that and I still think it is a good idea while maintaining compatibility with older API designs.

This comment has been minimized.

@mscdex

mscdex Mar 12, 2019

Author Contributor

To me it should be all or nothing, it doesn't make sense to have an encoding parameter for just one parameter when we're discussing backwards compatibility (.update() also supports an encoding parameter).

This comment has been minimized.

@tniessen

tniessen Mar 12, 2019

Member

Again, I am not talking about the data parameter and none of the existing APIs accept an encoding option or argument for parsing the key, since PEM is ASCII-only.

As far as I can tell, your code already accepts strings and objects as the privateKey, the documentation does not match that behavior, that's all I am trying to explain.

This comment has been minimized.

@mscdex

mscdex Mar 12, 2019

Author Contributor

Doc updated.

added: REPLACEME
-->
* `data` {Buffer | TypedArray | DataView}
* `key` {Buffer | KeyObject}

This comment has been minimized.

@tniessen

tniessen Mar 12, 2019

Member

Same as above.


Calculates and returns the signature for `data` using the given private key.

This method only supports Ed25519 and Ed448 keys.

This comment has been minimized.

@tniessen

tniessen Mar 12, 2019

Member

Is there a reason for this restriction? I think having these "one shot" implementations could solve problems such as #25857 where JS object creation time is critical.

This comment has been minimized.

@mscdex

mscdex Mar 12, 2019

Author Contributor

Currently the reason is to keep things simple and the fact that EdDSA does not utilize a hash algorithm argument with OpenSSL's API.

This comment has been minimized.

@sam-github

sam-github Mar 15, 2019

Member

The lack of a digest arg and signing algorithm makes this very EdDSA specific, if its going to be that specific, might as well just call it what it is: crypto.eddsaSign(). I'm not opposed to that, its a bit odd, but then, EdDSA made some odd algorithm design choices so it doesn't fit so well into generice asym sign/verify APIs. Using the alg-specific name would allow us to do a full one-shot API later.

My skim of the RFCs gave me the impression that there was PureEdDSA and EdDSA, but this API doesn't allow a choice between them. Not a blocker, but I'm curious, does OpenSSL not support them, or did I misunderstand?

I think that even if the other key types are left unsupported for now and only EdDSA is implemented, if there is a one-shot API, it should be equivalent to the multi-shot API:

createSign(digest)
  .update(input, inputEncoding)
  .sign(privateKey+padding+saltLength, outputEncoding)

Its unfortunate IMO that we are squeezing the signature algorithm (technically, the padding method, but most APIs consider the padding to be part of the signature algorithm) into the privateKey, but that's how it is.

crypto.signOneShot(digest, data, dataEncoding, privateKey, outputEncoding) would be one way (with private Key being all the possibilities of Sign.sign()), but using an options object might be nicer, particularly since all 5 args can be string, so its not easy to just not provide them and have the API provide default values.

The one-shot version would be expected to have a null for the digest with EdDSA.

Same general comment on the verify function below.

This comment has been minimized.

@mscdex

mscdex Mar 16, 2019

Author Contributor

The lack of a digest arg and signing algorithm makes this very EdDSA specific, if its going to be that specific, might as well just call it what it is: crypto.eddsaSign()

There was interest shown earlier about possibly allowing an algorithm in the future somehow, so I made the name more generic. I honestly don't care so much about the name as long as it's fairly accurate and concise and doesn't look like it came from Java.

My skim of the RFCs gave me the impression that there was PureEdDSA and EdDSA, but this API doesn't allow a choice between them. Not a blocker, but I'm curious, does OpenSSL not support them, or did I misunderstand?

As noted in the referenced issue, OpenSSL implements PureEdDSA which does not support streaming data.

but using an options object might be nicer, particularly since all 5 args can be string, so its not easy to just not provide them and have the API provide default values.

Possibly, but the other reason for using an object is that having the encoding parameters be optional like they are in the streaming API would be very hard to detect all of the various possibly-string arguments.

As noted in other comments, I did not bother implementing the encoding parameters because they are part of a legacy API and using Buffer.from() and buffer.toString() are trivial these days and it greatly simplifies the implementation.

This comment has been minimized.

@sam-github

sam-github Mar 18, 2019

Member

OK, so that addresses most of my comments, but the plan for this is unclear:

There was interest shown earlier about possibly allowing an algorithm in the future somehow, so I made the name more generic. I honestly don't care so much about the name as long as it's fairly accurate and concise and doesn't look like it came from Java.

I don't know what Java's API conventions are, so this doesn't illuminate the kinds of names you like, for me at least.

I still don't love xxxOneShot() as a naming convention, but I can live with it.

I don't think the API should have a generic name if it doesn't have enough args to one-shot a generic sign algorithm. digest should be added as the first arg, IMO, if the name stays generic.

assert.strictEqual(sig.length, pair.sigLen);

assert.strictEqual(crypto.verifyOneShot(data, pair.private, sig), true);
assert.strictEqual(crypto.verifyOneShot(data, pair.public, sig), true);

This comment has been minimized.

@tniessen

tniessen Mar 12, 2019

Member

There should also be tests for verification failures (e.g. using the wrong key, the wrong message, the wrong signature).

This comment has been minimized.

@mscdex

mscdex Mar 13, 2019

Author Contributor

Added.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 12, 2019

Maybe we should consider other names?

I had to choose something.

Do we need to reserve the names sign, verify, hash etc. for something else?

I think having a static method that's named the same as an instance method could be confusing.

@panva panva referenced this pull request Mar 12, 2019

Open

OKP Support (EdDSA) #12

@mscdex mscdex force-pushed the mscdex:crypto-sign-verify-eddsa-support branch from 43be5b7 to 000ccf0 Mar 12, 2019

@mscdex mscdex force-pushed the mscdex:crypto-sign-verify-eddsa-support branch from 000ccf0 to e3af7fb Mar 13, 2019

@@ -2651,6 +2651,21 @@ added: v10.0.0
Enables the FIPS compliant crypto provider in a FIPS-enabled Node.js build.
Throws an error if FIPS mode is not available.

### crypto.signOneShot(data, key)

This comment has been minimized.

@jasnell

jasnell Mar 13, 2019

Member

Why not just crypto.sign() and crypto.verify()

This comment has been minimized.

@tniessen

This comment has been minimized.

@sam-github

sam-github Mar 15, 2019

Member

mscdex:

I think having a static method that's named the same as an instance method could be confusing.

I think its the opposite of confusing, it makes it clear it does the same thing, but with no saved state (because there is no object to hold state), its clearly one-shot.

case EVP_PKEY_ED448:
break;
default:
return env->ThrowError("Unsupported key type");

This comment has been minimized.

@jasnell

jasnell Mar 13, 2019

Member

Is it possible to do this check at the JavaScript layer? Either way, it would be great to have an ERR_* code for this.

This comment has been minimized.

@mscdex

mscdex Mar 13, 2019

Author Contributor

Probably not reliably and not without crossing the JS<->C++ boundary an extra time. Is there a way to throw errors with codes set from C++?

EDIT: I see that there is. I could add a new error code I suppose, although nothing else in this source file utilizes error codes.

This comment has been minimized.

@mscdex

mscdex Mar 14, 2019

Author Contributor

Added, but I noticed that the name for error objects produced from C++ are not formatted the same as those generated from JS. JS includes the code in brackets after the error type. C++ does not.

This comment has been minimized.

@sam-github

sam-github Mar 15, 2019

Member

I would like an error code, if its not too much trouble. I have a PR almost done that adds a lot more error codes, but it didn't work everywhere, and I had to put it aside while doing other work. Every new throw of a string is something I'll have to convert to setting .code sometime in the future.

crypto: add signOneShot() and verifyOneShot()
These methods are to allow signing and verifying using Ed25519
and Ed448 keys, which do not support streaming of input data.

Fixes: #26320

@mscdex mscdex force-pushed the mscdex:crypto-sign-verify-eddsa-support branch from e3af7fb to 61ca0f5 Mar 14, 2019

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 14, 2019

Marking this for not landing on v10.x/v11.x unless it's decided at some point that we don't support OpenSSL v1.1.0 or older for those branches.

@@ -2651,6 +2651,21 @@ added: v10.0.0
Enables the FIPS compliant crypto provider in a FIPS-enabled Node.js build.
Throws an error if FIPS mode is not available.

### crypto.signOneShot(data, key)

This comment has been minimized.

@sam-github

sam-github Mar 15, 2019

Member

mscdex:

I think having a static method that's named the same as an instance method could be confusing.

I think its the opposite of confusing, it makes it clear it does the same thing, but with no saved state (because there is no object to hold state), its clearly one-shot.


Calculates and returns the signature for `data` using the given private key.

This method only supports Ed25519 and Ed448 keys.

This comment has been minimized.

@sam-github

sam-github Mar 15, 2019

Member

The lack of a digest arg and signing algorithm makes this very EdDSA specific, if its going to be that specific, might as well just call it what it is: crypto.eddsaSign(). I'm not opposed to that, its a bit odd, but then, EdDSA made some odd algorithm design choices so it doesn't fit so well into generice asym sign/verify APIs. Using the alg-specific name would allow us to do a full one-shot API later.

My skim of the RFCs gave me the impression that there was PureEdDSA and EdDSA, but this API doesn't allow a choice between them. Not a blocker, but I'm curious, does OpenSSL not support them, or did I misunderstand?

I think that even if the other key types are left unsupported for now and only EdDSA is implemented, if there is a one-shot API, it should be equivalent to the multi-shot API:

createSign(digest)
  .update(input, inputEncoding)
  .sign(privateKey+padding+saltLength, outputEncoding)

Its unfortunate IMO that we are squeezing the signature algorithm (technically, the padding method, but most APIs consider the padding to be part of the signature algorithm) into the privateKey, but that's how it is.

crypto.signOneShot(digest, data, dataEncoding, privateKey, outputEncoding) would be one way (with private Key being all the possibilities of Sign.sign()), but using an options object might be nicer, particularly since all 5 args can be string, so its not easy to just not provide them and have the API provide default values.

The one-shot version would be expected to have a null for the digest with EdDSA.

Same general comment on the verify function below.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.