Document all members of EcdhPrivateKey Class #114
Conversation
There was a problem hiding this comment.
Mostly just small things. I'll probably have to spend a bit more time on the review.
I like that we have documentation which explains a lot of things, we just have to careful we don't make mistakes. And I'll freely admit that I have to lookup things before I'm sure what is correct and what is not.
| static Future<EcdhPrivateKey> importPkcs8Key( | ||
| List<int> keyData, | ||
| EllipticCurve curve, | ||
| ) { | ||
| // Note. unsupported on Firefox, see EcdsaPrivateKey.importPkcs8Key |
There was a problem hiding this comment.
This should be noted in a remark similar to what we have at the bottom of:
https://pub.dev/documentation/webcrypto/latest/webcrypto/AesCtrSecretKey/encryptBytes.html
I think the "see EcdsaPrivateKey.importPkcs8Key" is just a note to us, because we probably wrote more about it there. Hopefully, the source contains a bug we can reference.
There was a problem hiding this comment.
Moved the Note above the function definition. Removed "see EcdsaPrivateKey.importPkcs8Key"
There was a problem hiding this comment.
What I meant was that we should probably note in the documentation comment that it won't work for Firefox.
If we actually lookup the code for what the comment says, we'll see:
https://github.com/google/webcrypto.dart/blob/master/lib/src/webcrypto/webcrypto.ecdsa.dart#L21-L29
That comment sends us to:
https://bugzilla.mozilla.org/show_bug.cgi?id=1133698
Which appears to be fixed 3 years ago 🎉, so maybe importPkcs8Key works for EC keys on Firefox. We should update our tests to see if this is the case.
If importPkcs8Key works, then we won't need to document that it doesn't work on Firefox.
Filed: #117 to remove this comment.
As it now appears to work.
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
| static Future<EcdhPrivateKey> importPkcs8Key( | ||
| List<int> keyData, | ||
| EllipticCurve curve, | ||
| ) { | ||
| // Note. unsupported on Firefox, see EcdsaPrivateKey.importPkcs8Key |
There was a problem hiding this comment.
What I meant was that we should probably note in the documentation comment that it won't work for Firefox.
If we actually lookup the code for what the comment says, we'll see:
https://github.com/google/webcrypto.dart/blob/master/lib/src/webcrypto/webcrypto.ecdsa.dart#L21-L29
That comment sends us to:
https://bugzilla.mozilla.org/show_bug.cgi?id=1133698
Which appears to be fixed 3 years ago 🎉, so maybe importPkcs8Key works for EC keys on Firefox. We should update our tests to see if this is the case.
If importPkcs8Key works, then we won't need to document that it doesn't work on Firefox.
Filed: #117 to remove this comment.
As it now appears to work.
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
| // Note. unsupported on Firefox, see EcdsaPrivateKey.importPkcs8Key | ||
| /// Export the [EcdhPrivateKey] as a PKCS8 key. | ||
| /// | ||
| /// This returns the private key as an octet string. |
There was a problem hiding this comment.
I would just say: returns private key as a list of bytes.
Unless, we're really fancy. Look at RsassaPkcs1V15PrivateKey.exportPkcs8Key it says:
Returns the DER encoding of the PrivateKeyInfo structure specified in RFC 5208 as a list of bytes.
I assume I really did look up the specification to find out exactly what is returned and in what format.
I'm guessing it might be a different thing for EC keys.
I actually think I did this, because normally when someone says "PKCS #8" format, you're thinking about a base64 encoded string inside the PEM ---- PRIVATE KEY--- ASCII armor thing..
Which is not what this returns.
Look at: https://www.w3.org/TR/WebCryptoAPI/#ecdh-operations
So it might be worth saying what structure it returns in what encoding as bytes.
I suspect this is also where we have to at-least for our own sanity read about BER vs DER (I forgot all about this).
See: https://en.wikipedia.org/wiki/X.690
But I think webcrypto might only support DER encoding (this might be something we should test across platforms, probably something we should file an issue for testing 🤣 )
So, yeah, I'm not exactly sure what this should say. We should probably read a bit more in the RFCs and webcrypto spec to make sure what references we want to give.
|
|
||
| // Note. unsupported on Firefox, see EcdsaPrivateKey.importPkcs8Key | ||
| /// Export the [EcdhPrivateKey] as a PKCS8 key. |
There was a problem hiding this comment.
nit: Consistent spelling
We write PKCS #8 everywhere else.
| /// The [length] parameter specifies the number of bits to derive and | ||
| /// should be multiples of 8. |
There was a problem hiding this comment.
What happens if it's not a multiple of 8? Is it an error? What does the webcrypto specification say?
Must it be a multiple of 8, then we should say must :D
| /// The [length] parameter specifies the number of bits to derive and | |
| /// should be multiples of 8. | |
| /// The [length] parameter specifies the number of bits to derive and | |
| /// should be multiples of 8. |
| /// // Derive 256 bits from the private key using the public key. | ||
| /// final derivedBits = await keyPair.privateKey.deriveBits(256, keyPair.publicKey); |
There was a problem hiding this comment.
When is usage such as this legitimate?
Is it wrong to use the same public / private key pair together?
I don't know, but I thought the idea was that you generated two key-pairs, exchanged public keys and then generated the same shared secret.
Using a single key-pair may work, but what is the point? If you're sending the private key over a secure channel, then you have a secure channel and might as well send a randomly generated shared secret instead.
At best this is an example of a fairly complex and expensive RNG, at worst this is a flared RNG.
I don't have to expertise to say whether this is a reasonable way to generate random numbers. But I don't think this is how ECDH is typically intended to be used.
| static Future<KeyPair<EcdhPrivateKey, EcdhPublicKey>> generateKey( | ||
| EllipticCurve curve, | ||
| ) { | ||
| return impl.ecdhPrivateKey_generateKey(curve); | ||
| } | ||
|
|
||
| /// Derive an array of bits from the [EcdhPrivateKey]. |
There was a problem hiding this comment.
"an array of bits" is just type information. It's more accurate to write UInt8List which is the return type. So repeating doesn't really help much.
Look at:
https://pub.dev/documentation/webcrypto/latest/webcrypto/HkdfSecretKey/deriveBits.html
We could say Derive shared secret from ... or Derive key from ....
Also we don't derive from private key, you derive from private and public key.
Or derive from two key-pairs might be more accurate, right?
You have the private key from one pair and the private key from another, hence, you derive from two key-pairs.
| /// | ||
| /// The [length] parameter specifies the number of bits to derive and | ||
| /// should be multiples of 8. | ||
| /// The [publicKey] parameter is the [EcdhPublicKey]. |
There was a problem hiding this comment.
This just states the type.
I do think we need to clearly state what the algorithm does.
Maybe, we should concisely explain that it derives a shared secret from two key-pairs.
Using the private key from one and public key from the other. And that using the private key from the other and public key from the one, will result in the same shared secret.
Maybe, that could get confusing, so maybe it's sensible to say: "Derives a shared-secret from two ECDH key-pairs A and B, using the private key from A and public key from B. Using the public key from A and the private key from B will result in the same shared secret. Allowing two parties to obtain a shared secret by exchanging public keys.
|
Splitting this into smaller PRs |
|
Closing as all Docs are split into smaller PRs |
No description provided.