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

Key fixes #154

Merged
merged 6 commits into from
Jul 5, 2023
Merged

Key fixes #154

merged 6 commits into from
Jul 5, 2023

Conversation

setrofim
Copy link
Contributor

@setrofim setrofim commented Jul 3, 2023

A number of spec compliance fixes and interface adjustements for COSE_Key.

This address points 1, 3, 4, and 5 in #151 (comment)

@setrofim
Copy link
Contributor Author

setrofim commented Jul 3, 2023

This sits on top of #153

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #154 (f83d377) into main (a579021) will increase coverage by 0.42%.
The diff coverage is 100.00%.

❗ Current head f83d377 differs from pull request most recent head 7c18bbe. Consider uploading reports for the commit 7c18bbe to get more accurate results

@@            Coverage Diff             @@
##             main     #154      +/-   ##
==========================================
+ Coverage   90.87%   91.30%   +0.42%     
==========================================
  Files          12       12              
  Lines        1611     1633      +22     
==========================================
+ Hits         1464     1491      +27     
+ Misses        109      104       -5     
  Partials       38       38              
Impacted Files Coverage Δ
key.go 89.71% <100.00%> (+2.21%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@setrofim setrofim force-pushed the setrofim branch 3 times, most recently from 2de9dd1 to efc9cd7 Compare July 3, 2023 12:19
key.go Show resolved Hide resolved
key.go Show resolved Hide resolved
key_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@qmuntal qmuntal left a comment

Choose a reason for hiding this comment

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

I don't see that this PR fixes my first point from #151 (comment).

@setrofim
Copy link
Contributor Author

setrofim commented Jul 3, 2023

I don't see that this PR fixes my first point from #151 (comment).

This is addressed by d591f36

Validate() is changed to only check that a know curve is used with an appropriate key type (.e.g that CurveEd25519 with KeyTypeEC2 results in an error). However, unknown curves are no longer reported as errors.

@setrofim setrofim requested a review from qmuntal July 3, 2023 14:31
Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

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

LGTM!

The alg parameter is unnecessary, and makes these functions more awkward
to use. The input PublicKey/PrivateKey is now identified via a type
switch, and the algorithm is derived from it.

Signed-off-by: setrofim <setrofim@gmail.com>
RFC8152 allows for unregistered curves, therefore we should not fail key
validation if a curve is not recognised when marshalling. We should only
fail when a known curve is used with an incorrect key type.

Signed-off-by: setrofim <setrofim@gmail.com>
Ensure that the KeyType is the expected one for the Curve when creating
crypto.PublicKey or crypto.PrivateKey.

Signed-off-by: setrofim <setrofim@gmail.com>
RFC8152 allows public parts to be omitted from private keys, as they
could be derived (though it recommends that they are present).
crypto.PrivateKey implementations require for them to be present.

Signed-off-by: setrofim <setrofim@gmail.com>
Add test for String() calls not exercised by other test cases to placate
Codecov.

Signed-off-by: setrofim <setrofim@gmail.com>
Signed-off-by: setrofim <setrofim@gmail.com>
@qmuntal qmuntal merged commit cebef14 into main Jul 5, 2023
5 checks passed
@qmuntal qmuntal deleted the setrofim branch July 5, 2023 11:50
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.

None yet

4 participants