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.

Sign up for GitHub

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

corim/signedcorim: check for mandatory alg and kid #157

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

corim/signedcorim: check for mandatory alg and kid #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions corim/signedcorim.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,21 @@ func (o *SignedCorim) processHdrs() error {
return errors.New("missing mandatory protected header")
}

v, ok := hdr.Protected[cose.HeaderLabelContentType]
v, ok := hdr.Protected[cose.HeaderLabelAlgorithm]
if !ok {
return errors.New("missing mandatory algorithm")
}

// TODO: make this consistent, either int64 or cose.Algorithm
// cose.Algorithm is an alias to int64 defined in veraison/go-cose
switch v.(type) {
case int64:
case cose.Algorithm:
default:
return fmt.Errorf("expecting integer CoRIM Algorithm, got %T instead", v)
}

v, ok = hdr.Protected[cose.HeaderLabelContentType]
if !ok {
return errors.New("missing mandatory content type")
}
Expand All @@ -68,9 +82,15 @@ func (o *SignedCorim) processHdrs() error {
return fmt.Errorf("expecting content type %q, got %q instead", ContentType, v)
}

// TODO(tho) key id is apparently mandatory, which doesn't look right.
// TODO(tho) Check with the CoRIM design team.
// See https://github.com/veraison/corim/issues/14
v, ok = hdr.Protected[cose.HeaderLabelKeyID]
if !ok {
return errors.New("missing mandatory key id")
}

_, ok = v.([]byte)
if !ok {
return fmt.Errorf("expecting byte string CoRIM Key ID, got %T instead", v)
}

v, ok = hdr.Protected[HeaderLabelCorimMeta]
if !ok {
Expand Down Expand Up @@ -129,7 +149,7 @@ func (o *SignedCorim) FromCOSE(buf []byte) error {
// Sign returns the serialized signed-corim, signed by the supplied cose Signer.
// The target SignedCorim must have its UnsignedCorim field correctly
// populated.
func (o *SignedCorim) Sign(signer cose.Signer) ([]byte, error) {
func (o *SignedCorim) Sign(signer cose.Signer, kid []byte) ([]byte, error) {
if signer == nil {
return nil, errors.New("nil signer")
}
Expand Down Expand Up @@ -159,6 +179,7 @@ func (o *SignedCorim) Sign(signer cose.Signer) ([]byte, error) {

o.message.Headers.Protected.SetAlgorithm(alg)
o.message.Headers.Protected[cose.HeaderLabelContentType] = ContentType
o.message.Headers.Protected[cose.HeaderLabelKeyID] = kid
Copy link
Contributor

Choose a reason for hiding this comment

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

we should we ensure that kid != nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kid is supposed to never be nil. I was thinking we should store kid as part of the cose.Signer while creating it from the JWK (like we do for alg). getKidFromJWK can get a kid from every JWK.

I'll wait for the meeting on kid being mandatory before making any more changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thomas-fossati Please ping me after the final decision on whether kid should be mandatory.

o.message.Headers.Protected[HeaderLabelCorimMeta] = metaCBOR

err = o.message.Sign(rand.Reader, NoExternalData, signer)
Expand Down
38 changes: 23 additions & 15 deletions corim/signedcorim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ func TestSignedCorim_FromCOSE_fail_corim_bad_cbor(t *testing.T) {
/ protected / << {
/ alg / 1: -7, / ECDSA 256 /
/ content-type / 3: "application/rim+cbor",
/ kid / 4: h'1',
/ corim-meta / 8: h'a200a1006941434d45204c74642e01a101c11a5fad2056'
} >>,
/ unprotected / {},
Expand All @@ -295,12 +296,12 @@ func TestSignedCorim_FromCOSE_fail_corim_bad_cbor(t *testing.T) {
)
*/
tv := []byte{
0xd2, 0x84, 0x58, 0x32, 0xa3, 0x01, 0x26, 0x03, 0x74, 0x61, 0x70, 0x70,
0xd2, 0x84, 0x58, 0x35, 0xa4, 0x01, 0x26, 0x03, 0x74, 0x61, 0x70, 0x70,
0x6c, 0x69, 0x63, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x2f, 0x72, 0x69, 0x6d,
0x2b, 0x63, 0x62, 0x6f, 0x72, 0x08, 0x57, 0xa2, 0x00, 0xa1, 0x00, 0x69,
0x41, 0x43, 0x4d, 0x45, 0x20, 0x4c, 0x74, 0x64, 0x2e, 0x01, 0xa1, 0x01,
0xc1, 0x1a, 0x5f, 0xad, 0x20, 0x56, 0xa0, 0x44, 0xba, 0xdc, 0xb0, 0x30,
0x44, 0xde, 0xad, 0xbe, 0xef,
0x2b, 0x63, 0x62, 0x6f, 0x72, 0x04, 0x41, 0x31, 0x08, 0x57, 0xa2, 0x00,
0xa1, 0x00, 0x69, 0x41, 0x43, 0x4d, 0x45, 0x20, 0x4c, 0x74, 0x64, 0x2e,
0x01, 0xa1, 0x01, 0xc1, 0x1a, 0x5f, 0xad, 0x20, 0x56, 0xa0, 0x44, 0xba,
0xdc, 0xb0, 0x30, 0x44, 0xde, 0xad, 0xbe, 0xef,
}

var actual SignedCorim
Expand All @@ -316,6 +317,7 @@ func TestSignedCorim_FromCOSE_fail_invalid_corim(t *testing.T) {
/ protected / << {
/ alg / 1: -7, / ECDSA 256 /
/ content-type / 3: "application/rim+cbor",
/ kid / 4: h'1',
/ corim-meta / 8: h'a200a1006941434d45204c74642e01a101c11a5fad2056'
} >>,
/ unprotected / {},
Expand All @@ -327,13 +329,13 @@ func TestSignedCorim_FromCOSE_fail_invalid_corim(t *testing.T) {
)
*/
tv := []byte{
0xd2, 0x84, 0x58, 0x32, 0xa3, 0x01, 0x26, 0x03, 0x74, 0x61, 0x70, 0x70,
0xd2, 0x84, 0x58, 0x35, 0xa4, 0x01, 0x26, 0x03, 0x74, 0x61, 0x70, 0x70,
0x6c, 0x69, 0x63, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x2f, 0x72, 0x69, 0x6d,
0x2b, 0x63, 0x62, 0x6f, 0x72, 0x08, 0x57, 0xa2, 0x00, 0xa1, 0x00, 0x69,
0x41, 0x43, 0x4d, 0x45, 0x20, 0x4c, 0x74, 0x64, 0x2e, 0x01, 0xa1, 0x01,
0xc1, 0x1a, 0x5f, 0xad, 0x20, 0x56, 0xa0, 0x50, 0xa1, 0x00, 0x6d, 0x69,
0x6e, 0x76, 0x61, 0x6c, 0x69, 0x64, 0x20, 0x63, 0x6f, 0x72, 0x69, 0x6d,
0x44, 0xde, 0xad, 0xbe, 0xef,
0x2b, 0x63, 0x62, 0x6f, 0x72, 0x04, 0x41, 0x31, 0x08, 0x57, 0xa2, 0x00,
0xa1, 0x00, 0x69, 0x41, 0x43, 0x4d, 0x45, 0x20, 0x4c, 0x74, 0x64, 0x2e,
0x01, 0xa1, 0x01, 0xc1, 0x1a, 0x5f, 0xad, 0x20, 0x56, 0xa0, 0x50, 0xa1,
0x00, 0x6d, 0x69, 0x6e, 0x76, 0x61, 0x6c, 0x69, 0x64, 0x20, 0x63, 0x6f,
0x72, 0x69, 0x6d, 0x44, 0xde, 0xad, 0xbe, 0xef,
}

var actual SignedCorim
Expand Down Expand Up @@ -435,13 +437,15 @@ func TestSignedCorim_SignVerify_ok(t *testing.T) {
} {
signer, err := NewSignerFromJWK(key)
require.NoError(t, err)
kid, err := getKidFromJWK(key)
require.NoError(t, err)

var SignedCorimIn SignedCorim

SignedCorimIn.UnsignedCorim = *unsignedCorimFromCBOR(t, testGoodUnsignedCorimCBOR)
SignedCorimIn.Meta = *metaGood(t)

cbor, err := SignedCorimIn.Sign(signer)
cbor, err := SignedCorimIn.Sign(signer, kid)
assert.Nil(t, err)

var SignedCorimOut SignedCorim
Expand All @@ -462,12 +466,14 @@ func TestSignedCorim_SignVerify_ok(t *testing.T) {
func TestSignedCorim_SignVerify_fail_tampered(t *testing.T) {
signer, err := NewSignerFromJWK(testES256Key)
require.NoError(t, err)
kid, err := getKidFromJWK(testES256Key)
require.NoError(t, err)

var SignedCorimIn SignedCorim

SignedCorimIn.UnsignedCorim = *unsignedCorimFromCBOR(t, testGoodUnsignedCorimCBOR)

cbor, err := SignedCorimIn.Sign(signer)
cbor, err := SignedCorimIn.Sign(signer, kid)
assert.Nil(t, err)

var SignedCorimOut SignedCorim
Expand All @@ -493,6 +499,8 @@ func TestSignedCorim_SignVerify_fail_tampered(t *testing.T) {
func TestSignedCorim_Sign_fail_bad_corim(t *testing.T) {
signer, err := NewSignerFromJWK(testES256Key)
require.NoError(t, err)
kid, err := getKidFromJWK(testES256Key)
require.NoError(t, err)

var SignedCorimIn SignedCorim

Expand All @@ -501,7 +509,7 @@ func TestSignedCorim_Sign_fail_bad_corim(t *testing.T) {

SignedCorimIn.UnsignedCorim = *emptyCorim

_, err = SignedCorimIn.Sign(signer)
_, err = SignedCorimIn.Sign(signer, kid)
assert.EqualError(t, err, "failed validation of unsigned CoRIM: empty id")
}

Expand All @@ -513,7 +521,7 @@ func TestSignedCorim_Sign_fail_no_signer(t *testing.T) {

SignedCorimIn.UnsignedCorim = *emptyCorim

_, err := SignedCorimIn.Sign(nil)
_, err := SignedCorimIn.Sign(nil, nil)
assert.EqualError(t, err, "nil signer")
}

Expand Down
19 changes: 19 additions & 0 deletions corim/signer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,25 @@ func getAlgAndKeyFromJWK(j []byte) (cose.Algorithm, crypto.Signer, error) {
return alg, key, nil
}

func getKidFromJWK(j []byte) ([]byte, error) {
k, err := jwk.ParseKey(j)
if err != nil {
return nil, err
}

if k.KeyID() != "" {
return []byte(k.KeyID()), nil
}

// Generate a key ID from the JWK Thumbprint if none exist
// See https://datatracker.ietf.org/doc/html/rfc7638
kid, err := k.Thumbprint(crypto.SHA256)
if err != nil {
return nil, err
}
return kid, nil
}

func ellipticCurveToAlg(c elliptic.Curve) cose.Algorithm {
switch c {
case elliptic.P256():
Expand Down
2 changes: 1 addition & 1 deletion corim/testcases/regen-from-src.sh
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ GEN_TESTCASE=$(go env GOPATH)/bin/gen-testcase

if [[ ! -f ${GEN_TESTCASE} ]]; then
echo "installing gen-testcase"
go install github.com/veraison/[email protected].1
go install github.com/veraison/[email protected].2
fi

testcases=(
Expand Down
Binary file modified corim/testcases/signed-corim-with-extensions.cbor
View file
Open in desktop
Binary file not shown.
Binary file modified corim/testcases/signed-example-corim.cbor
View file
Open in desktop
Binary file not shown.
Binary file modified corim/testcases/signed-good-corim.cbor
View file
Open in desktop
Binary file not shown.
Binary file modified corim/testcases/unsigned-corim-with-extensions.cbor
View file
Open in desktop
Binary file not shown.
Binary file modified corim/testcases/unsigned-example-corim.cbor
View file
Open in desktop
Binary file not shown.
Binary file modified corim/testcases/unsigned-good-corim.cbor
View file
Open in desktop
Binary file not shown.
Loading