search

veraison / go-cose

go library for CBOR Object Signing and Encryption (COSE)
Mozilla Public License 2.0
49 stars 26 forks source link

Make Key more extensible #151

Closed qmuntal closed 1 year ago

qmuntal commented 1 year ago

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 https://github.com/veraison/go-cose/pull/146#pullrequestreview-1492955742:

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 commented 1 year ago

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 commented 1 year ago

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).

qmuntal commented 1 year ago

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 commented 1 year ago

Done!