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

Refactor Key to support custom parameters #158

Merged
merged 2 commits into from
Jul 13, 2023
Merged

Refactor Key to support custom parameters #158

merged 2 commits into from
Jul 13, 2023

Conversation

qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Jul 11, 2023

This PR redesigns Key so all non-common parameters are stored in a map instead, even the ones from known key types (i.e. EC2, OKP, and Symmetric keys). This allows go-cose to support key types and parameters that are not defined in RFC8152 without having to break the module API. See #151 for more context.

The Key struct now looks like this:

type Key struct {
        // Common Params
	KeyType KeyType
	KeyID []byte
	Algorithm Algorithm
	KeyOps []KeyOp
	BaseIV []byte

        // Custom Params
	Params map[interface{}]interface{}
}

I've also added some helper functions to access the known and unknown key parameters:

// Useful to retrieve all parameters relevant for each known key type
func (*Key) OKP() (crv Curve, x []byte, d []byte)
func (*Key) EC2() (crv Curve, x []byte, y, d []byte)
func (*Key) Symmetric() (k []byte)

// Useful to retrieve individual parameters
func (*Key) ParamBytes(label interface{}) ([]byte, bool)
func (*Key) ParamInt(label interface{}) (int64, bool)
func (*Key) ParamUint(label interface{}) (uint64, bool)
func (*Key) ParamString(label interface{}) (string, bool)
func (*Key) ParamBool(label interface{}) (bool, bool)

A good amount of new test cases have been added.

@setrofim I've remove the intOrStr struct, as it is not necessary with the new way of decoding keys, It is a pity, because I conceptually liked it. If we ever release a new major version, we should consider using something like that to have a better type enforcement in all map labels.

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #158 (0c37dca) into main (c862f87) will increase coverage by 2.14%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
+ Coverage   91.16%   93.30%   +2.14%     
==========================================
  Files          12       11       -1     
  Lines        1550     1658     +108     
==========================================
+ Hits         1413     1547     +134     
+ Misses        101       78      -23     
+ Partials       36       33       -3     
Impacted Files Coverage Δ
key.go 95.76% <96.66%> (+6.46%) ⬆️

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

Copy link
Contributor

@setrofim setrofim left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

good stuff 👍

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 12, 2023

@thomas-fossati @setrofim I've added one last commit to this PR, in case you want to take another look.

@thomas-fossati
Copy link
Contributor

@thomas-fossati @setrofim I've added one last commit to this PR, in case you want to take another look.

neat!

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!

@qmuntal qmuntal merged commit bfe4717 into main Jul 13, 2023
6 checks passed
@qmuntal qmuntal deleted the keyapinew branch July 13, 2023 06:07
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.

4 participants