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

Make Key more extensible #151

Closed
qmuntal opened this issue Jun 29, 2023 · 4 comments
Closed

Make Key more extensible #151

qmuntal opened this issue Jun 29, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@qmuntal
Copy link
Contributor

qmuntal commented Jun 29, 2023

I'm opening this issue to discuss the Key design before cutting v1.2.0 @setrofim (I've removed the tag and release you just created).

Extracted from #146 (review):

The Key struct, implemented in #146, contains all known key parameters, that is, from EC2, OKP and Symmetric Keys. This doesn't scale well, as in the future we might want to support new keys that, for example, defines D as a tstr instead of bstr, in which case it will be hard to retrofit the new key. In fact, this already seems to happen with EC2 y-coordinate, we only support bstr but the spec also supports bool.

IMO it would have been better to define key parameters in separate structs, one for each key type, and have Key just contain a property like Params any, which could contain the specialized parameters struct.

@shizhMSFT

@qmuntal qmuntal added the bug Something isn't working label Jun 29, 2023
@qmuntal qmuntal changed the title Improve how Key is defined Make Key more extensible Jun 29, 2023
@qmuntal qmuntal added enhancement New feature or request and removed bug Something isn't working labels Jun 29, 2023
@qmuntal
Copy link
Contributor Author

qmuntal commented Jun 29, 2023

List of things (for myself) that I'm finding while working on the proposal:

  1. Key.Validate errors if Key.Curve is not one of the curves we support, but RFC 8152 Section 13.1.1 says this: Other curves may be registered in the future, and private curves can be used as well., therefore a key with an unsupported curve is still valid.
  2. Key.D is used to store the symmetric key private key, but RFC 8152 Section 13.3 named is k. This will confuse users.
  3. When Key.KeyType is OKP and the Key.Curve is either P-256, P-384, or P-521, then Key.Public and Key.Private returns and ecdsa key. RFC 8152 Section 13.1 says thatthose curves can only be used when the key type is EC2, so we should error out.
  4. RFC 8152 Section 13.1.1 says that a private EC2 key can omit the x and y parameters, but ecdsa.PrivateKey requires them to be present. We should reconstruct them or fail as unsupported.
  5. NewKeyFromPublic and NewKeyFromPrivate accepts an algorithm and a Go key as parameters. It shouldn't be necessary to pass the algorithm, we can derive it from the key.

@setrofim
Copy link
Contributor

setrofim commented Jul 3, 2023

Key.D is used to store the symmetric key private key, but RFC 8152 Section 13.3 named is k. This will confuse users.

Please could you clarify this? As far as I can see, Key.K is used when marshalling Symmetric keys (see https://github.com/veraison/go-cose/blob/main/key.go#L590 and https://github.com/veraison/go-cose/blob/main/key.go#L533). Key.PrivateKey() will error with unexpeted key type "Symmetric" (as we currently do not support any symmetric algorithms).

@setrofim setrofim mentioned this issue Jul 3, 2023
@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 3, 2023

Please could you clarify this? As far as I can see, Key.K is used when marshalling Symmetric keys (see https://github.com/veraison/go-cose/blob/main/key.go#L590 and https://github.com/veraison/go-cose/blob/main/key.go#L533). Key.PrivateKey() will error with unexpeted key type "Symmetric" (as we currently do not support any symmetric algorithms).

Hug, I misunderstood the Key docs. I now see a symmetric private key is stored in K, not D.

@qmuntal
Copy link
Contributor Author

qmuntal commented Aug 1, 2023

Done!

@qmuntal qmuntal closed this as completed Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants