Add untagged COSE_Sign1 support #143

setrofim · 2023-05-12T09:24:15Z

RFC8152 specifies that a single-signer token can be either tagged (COSE_Sign1_Tagged) or untagged (COSE_Sign1). Add support for parsing, and optionally generating, the untagged version.

The default behaviour is to still generated COSE_Sign1_Tagged.

codecov · 2023-05-12T09:25:30Z

Codecov Report

Merging #143 (0d70fc7) into main (bfa9f54) will decrease coverage by 0.01%.
The diff coverage is 95.94%.

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   92.77%   92.76%   -0.01%     
==========================================
  Files          10       10              
  Lines        1024     1065      +41     
==========================================
+ Hits          950      988      +38     
- Misses         49       51       +2     
- Partials       25       26       +1

Impacted Files	Coverage Δ
sign1.go	`90.10% <95.94%> (+0.74%)`	⬆️

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

sign1.go

README.md

yogeshbdeshpande

LGTM! Left some minor nits, if we can tidy up further!

@shizhMSFT, @qmuntal Please review as well..

yogeshbdeshpande · 2023-05-12T14:03:44Z

sign1_test.go

@@ -320,7 +343,7 @@ func TestSign1Message_UnmarshalCBOR(t *testing.T) {
 				0x40, 0xa0, // empty headers
 				0xf6, // payload
 			},
-			wantErr: "cbor: invalid COSE_Sign1_Tagged object",
+			wantErr: "cbor: CBOR tag isn't allowed",


The error print does not align with the description of the test

It's not supposed to -- the descrition of the test says why a particular value is in valid. The returned error merely indicates that the value is invalid. The actual error comes form the underlying 'cbor' parser.

yogeshbdeshpande · 2023-05-12T14:04:01Z

sign1_test.go

@@ -331,7 +354,7 @@ func TestSign1Message_UnmarshalCBOR(t *testing.T) {
 				0x41, 0x00, // signature
 				0x40,
 			},
-			wantErr: "cbor: invalid COSE_Sign1_Tagged object",
+			wantErr: "cbor: CBOR tag isn't allowed",


Same as in line 357

See reply above.

thomas-fossati

LGTM, thanks!

IIUC, verification will swallow both COSE_Sign1 and COSE_Sign1_Tagged transparently?

sign1.go

README.md

yogeshbdeshpande · 2023-05-15T16:42:19Z

@setrofim : For the convenience of the external reviewers, can you please also attach a Feature request github issue, to highlight the need for making this change?

shizhMSFT

I have security concerns in the untagged COSE_Sign1 support.

According to RFC 9052 Section 2,

When a COSE object is carried in a media type of "application/cose", the optional parameter "cose-type" can be used to identify the embedded object. The parameter is OPTIONAL if the tagged version of the structure is used. The parameter is REQUIRED if the untagged version of the structure is used.

With this PR, it is possible for an application to consume a COSE_Sign1 object where its media type is application/cose instead of application/cose; cose-type="cose_sign1", which should fail and fail early.

We can have workaround with discussions like

type UntaggedSign1Message Sign1Message

func (m *UntaggedSign1Message) MarshalCBOR() ([]byte, error) {
    data, err := ((*Sign1Message)m).MarshalCBOR()
    if err != nil {
        return nil, err
    }
    return data[1:], nil
}

func (m *UntaggedSign1Message) UnmarshalCBOR(data []byte) error {
    return ((*Sign1Message)m).UnmarshalCBOR(append([]byte{sign1MessagePrefix[0]}, data...))
}

With above code, application should be able to decode the object according to the media type and fail early if there's anything wrong.

Of course, the above code is for illustration only and has no CPU / memory optimizations.

shizhMSFT · 2023-05-16T08:03:18Z

sign1.go

+	var err error
+	var isUntagged bool
+
+	if bytes.HasPrefix(data, sign1MessagePrefix) {
+		err = decModeWithTagsForbidden.Unmarshal(data[1:], &raw)
+		isUntagged = false
+	} else {
+		err = decModeWithTagsForbidden.Unmarshal(data, &raw)
+		isUntagged = true
+	}
+	if err != nil {
 		return err
 	}


We still can perform the fast message check

Suggested change

var err error

var isUntagged bool

if bytes.HasPrefix(data, sign1MessagePrefix) {

err = decModeWithTagsForbidden.Unmarshal(data[1:], &raw)

isUntagged = false

} else {

err = decModeWithTagsForbidden.Unmarshal(data, &raw)

isUntagged = true

}

if err != nil {

return err

}

// fast message check

isUntagged := !bytes.HasPrefix(data, sign1MessagePrefix)

if isUntagged {

if !bytes.HasPrefix(data, sign1MessagePrefix[1:]) {

return errors.New("cbor: invalid COSE_Sign1 / COSE_Sign1_Tagged object")

}

data = data[1:]

}

// decode to sign1Message and parse

if err := decModeWithTagsForbidden.Unmarshal(data, &raw); err != nil {

return err

}

sign1.go

setrofim · 2023-05-16T08:27:10Z

@setrofim : For the convenience of the external reviewers, can you please also attach a Feature request github issue, to highlight the need for making this change?

This is more of a bug fix than a feature implementation -- the "request" here is simply "allow parsing of valid COSE structures".

yogeshbdeshpande · 2023-05-16T08:37:15Z

rding to the media type and fail early if there's anything wrong.

ok, then please raise a github issue, explaining which section of specification we are not compliant with and what would be the
implications of non-compliance?

thomas-fossati · 2023-05-16T09:16:52Z

I have security concerns in the untagged COSE_Sign1 support.

According to RFC 9052 Section 2,

When a COSE object is carried in a media type of "application/cose", the optional parameter "cose-type" can be used to identify the embedded object. The parameter is OPTIONAL if the tagged version of the structure is used. The parameter is REQUIRED if the untagged version of the structure is used.

With this PR, it is possible for an application to consume a COSE_Sign1 object where its media type is application/cose instead of application/cose; cose-type="cose_sign1", which should fail and fail early.

While I like the increased robustness in the approach you suggest, I don't think there's a security concern here. I reckon that the bit of spec you quote above is intended to protect against confusion attacks with different top-level formats (e.g., Sign vs Sign1) rather than within the same format (e.g., Sign1 vs Sign1_Tagged).

setrofim · 2023-05-16T10:30:30Z

ok, then please raise a github issue, explaining which section of specification we are not compliant with and what would be the
implications of non-compliance?

This addressed in the commit message. Issues are to-do items there is no point in raising one for something that is already being done. They are not meant to provide context to existing changes -- the place for that is the commit message for the change.

setrofim · 2023-05-16T11:34:05Z

I have security concerns in the untagged COSE_Sign1 support.
According to RFC 9052 Section 2,

When a COSE object is carried in a media type of "application/cose", the optional parameter "cose-type" can be used to identify the embedded object. The parameter is OPTIONAL if the tagged version of the structure is used. The parameter is REQUIRED if the untagged version of the structure is used.

With this PR, it is possible for an application to consume a COSE_Sign1 object where its media type is application/cose instead of application/cose; cose-type="cose_sign1", which should fail and fail early.

While I like the increased robustness in the approach you suggest, I don't think there's a security concern here. I reckon that the bit of spec you quote above is intended to protect against confusion attacks with different top-level formats (e.g., Sign vs Sign1) rather than within the same format (e.g., Sign1 vs Sign1_Tagged).

I agree that @shizhMSFT's suggestion is neater and a lot more robust than my idea of using a field to sindicate tagged status. I've re-implemented untagged COSE_Sign1 support based on that. Please re-review.

yogeshbdeshpande · 2023-05-16T13:03:20Z

LGTM!
Was wondering we should have alteast a one-liner in the main README.md to say, we support Untagged COSE also and refer to examples for how to use it?

yogeshbdeshpande

LGTM!

sign1.go

RFC8152 specifies that a single-signer token can be either tagged (COSE_Sign1_Tagged) or untagged (COSE_Sign1). The existing Sign1Message implementation only handles COSE_Sign1_Tagged objects. Add UntaggedSign1Message to analogously handle COSE_Sign1 objects. Signed-off-by: setrofim <setrofim@gmail.com>

shizhMSFT

LGTM

thomas-fossati

Thanks for spotting and closing the gap!
I will raise an issue to do the same for COSE_Sign.

setrofim requested review from yogeshbdeshpande and thomas-fossati May 12, 2023 09:24

yogeshbdeshpande requested review from shizhMSFT and qmuntal May 12, 2023 13:21

yogeshbdeshpande reviewed May 12, 2023

View reviewed changes

sign1.go Outdated Show resolved Hide resolved

yogeshbdeshpande reviewed May 12, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

yogeshbdeshpande reviewed May 12, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

setrofim force-pushed the setrofim branch from 067b5d6 to b9d189e Compare May 12, 2023 14:01

yogeshbdeshpande approved these changes May 12, 2023

View reviewed changes

thomas-fossati approved these changes May 15, 2023

View reviewed changes

sign1.go Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

setrofim force-pushed the setrofim branch from 922ab72 to d08d663 Compare May 15, 2023 13:35

shizhMSFT reviewed May 16, 2023

View reviewed changes

setrofim force-pushed the setrofim branch from d08d663 to 6c7dbea Compare May 16, 2023 11:30

setrofim force-pushed the setrofim branch from 6c7dbea to 49d349f Compare May 16, 2023 11:41

setrofim requested review from thomas-fossati, shizhMSFT and yogeshbdeshpande May 16, 2023 11:43

yogeshbdeshpande approved these changes May 16, 2023

View reviewed changes

shizhMSFT reviewed May 17, 2023

View reviewed changes

sign1.go Show resolved Hide resolved

setrofim force-pushed the setrofim branch 2 times, most recently from b7e6157 to c33b925 Compare May 17, 2023 09:21

setrofim force-pushed the setrofim branch from c33b925 to 0d70fc7 Compare May 17, 2023 11:50

shizhMSFT approved these changes May 18, 2023

View reviewed changes

thomas-fossati approved these changes May 18, 2023

View reviewed changes

setrofim merged commit 354ac99 into main May 18, 2023

setrofim deleted the setrofim branch May 18, 2023 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add untagged COSE_Sign1 support #143

Add untagged COSE_Sign1 support #143

setrofim commented May 12, 2023

codecov bot commented May 12, 2023 •

edited

Loading

yogeshbdeshpande left a comment

yogeshbdeshpande May 12, 2023

setrofim May 12, 2023

yogeshbdeshpande May 12, 2023

setrofim May 12, 2023

thomas-fossati left a comment

yogeshbdeshpande commented May 15, 2023

shizhMSFT left a comment •

edited

Loading

shizhMSFT May 16, 2023

setrofim commented May 16, 2023 •

edited

Loading

yogeshbdeshpande commented May 16, 2023 •

edited

Loading

thomas-fossati commented May 16, 2023 •

edited

Loading

setrofim commented May 16, 2023

setrofim commented May 16, 2023 •

edited

Loading

yogeshbdeshpande commented May 16, 2023

yogeshbdeshpande left a comment

shizhMSFT left a comment

thomas-fossati left a comment

Add untagged COSE_Sign1 support #143

Add untagged COSE_Sign1 support #143

Conversation

setrofim commented May 12, 2023

codecov bot commented May 12, 2023 • edited Loading

Codecov Report

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

yogeshbdeshpande May 12, 2023

Choose a reason for hiding this comment

setrofim May 12, 2023

Choose a reason for hiding this comment

yogeshbdeshpande May 12, 2023

Choose a reason for hiding this comment

setrofim May 12, 2023

Choose a reason for hiding this comment

thomas-fossati left a comment

Choose a reason for hiding this comment

yogeshbdeshpande commented May 15, 2023

shizhMSFT left a comment • edited Loading

Choose a reason for hiding this comment

shizhMSFT May 16, 2023

Choose a reason for hiding this comment

setrofim commented May 16, 2023 • edited Loading

yogeshbdeshpande commented May 16, 2023 • edited Loading

thomas-fossati commented May 16, 2023 • edited Loading

setrofim commented May 16, 2023

setrofim commented May 16, 2023 • edited Loading

yogeshbdeshpande commented May 16, 2023

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

thomas-fossati left a comment

Choose a reason for hiding this comment

codecov bot commented May 12, 2023 •

edited

Loading

shizhMSFT left a comment •

edited

Loading

setrofim commented May 16, 2023 •

edited

Loading

yogeshbdeshpande commented May 16, 2023 •

edited

Loading

thomas-fossati commented May 16, 2023 •

edited

Loading

setrofim commented May 16, 2023 •

edited

Loading