-
Notifications
You must be signed in to change notification settings - Fork 26
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
Validate key sizes and allow signing with empty public keys #159
Conversation
Codecov Report
@@ Coverage Diff @@
## main #159 +/- ##
==========================================
- Coverage 93.48% 93.38% -0.11%
==========================================
Files 11 11
Lines 1658 1678 +20
==========================================
+ Hits 1550 1567 +17
- Misses 75 78 +3
Partials 33 33
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
I looked at this, I agree its important to do, seems to be done correctly, but I am not a reliable CR for golang. |
@yogeshbdeshpande, @shizhMSFT can you PTAL |
if len(x) == 0 { | ||
return nil, ErrOKPNoPub | ||
return ed25519.NewKeyFromSeed(d), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a general question here: What does d
mean for Ed25519 in terms of OKP? Is it private key or seed?
RFC 8152 13.2 says that
d: This contains the private key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d is a private key component of a crv, but my understanding of the specs is that it can be anything sufficiently random... so in the case that you derive a seed from a mnemonic or other details, you might use the seed as the private key... thats all super dangerous stuff... and I don't think any of it is relevant to COSE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name is a little misleading. Its documentation is more interesting:
NewKeyFromSeed calculates a private key from a seed. It will panic if len(seed) is not SeedSize. This function is provided for interoperability with RFC 8032. RFC 8032's private keys correspond to seeds in this package.
RFC8152 8.2 says that EdDSA curves should be implemented following RFC8032, so my understanding is that it is safe to use NewKeyFromSeed
here.
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR adds the following features:
x
,y
andd
parameters for OKP and EC2 keys have the right size.x
andy
for OKP and EC in case they are omitted, which is allowed by the COSE spec.