Add COSE_Key support
#146
Add COSE_Key support
#146
Conversation
Codecov Report
@@ Coverage Diff @@
## main #146 /- ##
==========================================
- Coverage 92.76% 90.87% -1.90%
==========================================
Files 10 12 2
Lines 1065 1611 546
==========================================
Hits 988 1464 476
- Misses 51 109 58
- Partials 26 38 12
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
setrofim
force-pushed
the
setrofim
branch
2 times, most recently
from
931818c
to
3d9a56f
Compare
setrofim
requested review from
shizhMSFT,
yogeshbdeshpande and
thomas-fossati
and removed request for
shizhMSFT
setrofim
force-pushed
the
setrofim
branch
2 times, most recently
from
ea7debe
to
ea37670
Compare
setrofim
force-pushed
the
setrofim
branch
7 times, most recently
from
029dda4
to
47d4b1a
Compare
There was a problem hiding this comment.
Approve with the following concerns:
- The
Keystruct 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 atstrinstead ofbstr, 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 supportbstrbut the spec also supportsbool. IMO it would have been better to define key parameters in separate structs, one for each key type, and haveKeyjust contain a property likeParams any, which could contain the specialized parameters struct. - Don't like to depend on
github.com/stretchr/testify, as it indirectly depends ongopkg.in/yaml, which is prone to security vulnerabilities that will be reported in go-cose even though we only use it for testing.
Having said this, this PR already contains alot of valuable work and we have gone though many iterations to put it in a good shape. We can work on my concerns in future PRs before cutting a new version.
|
@shizhMSFT: I believe all outstanding have now been addressed. Please let me know if you are happy with the code as-is or would like to see additioinal changes. |
|
note: I've added a commit to disable the |
| case KeyOpMACCreate: | ||
| return "MAC create" | ||
| case KeyOpMACVerify: | ||
| return "MAC verify" |
There was a problem hiding this comment.
The strings for MAC create and verify seem not defined in RFC docs.
There was a problem hiding this comment.
Given that no string value is defined these, I'm using the Name here instead. String() is not used during serialization to CBOR, and does not represent the encoded value, it only provides a string reprsentation (for use in error messages, debug output, etc).
There was a problem hiding this comment.
To prevent misuse, can you document the special cases of "MAC create/verify"?
|
@shizhMSFT please could you re-review? |
|
looking forward to JWK support :) |
| case KeyOpMACCreate: | ||
| return "MAC create" | ||
| case KeyOpMACVerify: | ||
| return "MAC verify" |
There was a problem hiding this comment.
To prevent misuse, can you document the special cases of "MAC create/verify"?
| // https://datatracker.ietf.org/doc/html/rfc8152#section-13.1 | ||
| type Curve int64 | ||
|
|
||
| func (c Curve) String() string { |
There was a problem hiding this comment.
Can you document this method noticing that it is for human reading only to prevent any misuse?
| KeyTypeSymmetric KeyType = 4 | ||
| ) | ||
|
|
||
| func (kt KeyType) String() string { |
There was a problem hiding this comment.
Can you document this method noticing that it is for human reading only to prevent any misuse?
|
As @qmuntal mentioned, it is better to drop the |
I tried to build a small app using the code changes in this PR: package main
import (
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rand"
_ "crypto/sha256"
"fmt"
"github.com/veraison/go-cose"
)
func SignP256(data []byte) ([]byte, error) {
// create a signer
privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
if err != nil {
return nil, err
}
signer, err := cose.NewSigner(cose.AlgorithmES256, privateKey)
if err != nil {
return nil, err
}
// create message header
headers := cose.Headers{
Protected: cose.ProtectedHeader{
cose.HeaderLabelAlgorithm: cose.AlgorithmES256,
},
}
// sign and marshal message
return cose.Sign1(rand.Reader, signer, headers, data, nil)
}
func main() {
fmt.Println(SignP256([]byte("hello world")))
}By running and As you can observe in the |
A number of elements in RFC8152 can be represented as either integer values or string names. This type will allow uniform handling of such elements when (un)marshalling to/from CBOR. Signed-off-by: Sergei Trofimov <[email protected]>
RFC8152 specifies that an algorithm may be identified either by its integer value, or by its string name. This adds support for latter. (note: There are currently no IANA-registered string values for COSE algorithms, so while this support is in place, any provided algorithm value is reported as invalid.) Signed-off-by: Sergei Trofimov <[email protected]>
Add support for COSE_Key structure, and associated enums for key type, ECC curve, and key ops, as defined in RFC8152. Signed-off-by: Sergei Trofimov <[email protected]>
This seems to vary with respect to the commit size, and for some commits, it is not possible to meet its requirement. Signed-off-by: Sergei Trofimov <[email protected]>
|
I've removed the |
Add support for marshalling/unmarshalling
COSE_Keystructures, and using them for signing and verification. At the moment, only RFC8152 is implemented (OKP and EC2 signatures); RFC8230 (RSASSA-PSS signatures) is NOT implemented.