Closed setrofim closed 1 year ago
Merging #143 (0d70fc7) into main (bfa9f54) will decrease coverage by
0.01%
. The diff coverage is95.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%) |
:arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
@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?
@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".
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?
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 isapplication/cose
instead ofapplication/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).
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.
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 isapplication/cose
instead ofapplication/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.
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?
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.