-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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
feat: support detached payloads in COSESign and COSESign1 #197
base: main
Are you sure you want to change the base?
Conversation
Hi, @alex-richards, Without knowing more about the specific scenario you want to implement, you might want to check this new draft for HASH Envelopes. It addresses many of the challenges detached payloads solve and create. Here's a set of slides we're presenting this week at the COSE Working group. While we're still generally supportive for go-cose supporting detached payloads, one of the advantages of HASH Envelope is it's "just a payload" and you won't need to detach and re-attach the payload. Just FYI, we should still support detached payloads in go-cose. |
Thanks Steve, I'll have a read, though I'm interested in ISO 18013-5, which specifies detached payloads. I've removed the breaking changes. |
4e80ef5
to
bc11d1c
Compare
Hi @alex-richards. Welcome to the verasion/go-cose project and thank you for your contributions. Thank you |
Hi Steve, Yeah, sure. Do you have a preferred private way for me to get in touch? |
You can reach me through my linkedin profile info. |
bc11d1c
to
1736a40
Compare
Hey Steve, I've signed as discussed. Cheers, Alex |
Thanks, @alex-richards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems ok to me, but it would be good to get more reviews, since this touches several interfaces
Thanks, @alex-richards, can you please resolve the DCO error: https://github.com/veraison/go-cose/pull/197/checks?check_run_id=27984447393 |
1736a40
to
a636db9
Compare
a636db9
to
94007e7
Compare
@qmuntal I've made those changes, yeah, they're much clearer. Not sure what resolving etiquette is here, tag you? Resolve myself? |
You can resolve them yourself 👍 |
94007e7
to
40060e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Any merging etiquette I should know about? Or just go for it once it's approved? Is 2 enough? |
Due to the slightly larger change, it would be good to have an additional golang and cose expert weigh in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good to me. Thanks a lot for the great contribution!
return m.sign(rand, nil, external, signer) | ||
} | ||
|
||
// SignDetached signs a Sign1Message using the provided Signer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments here are very identical to the comments for normal Sign Method.
At the least you should refer to the
// COSE defined detached payloads in Section 2 of [RFC9052]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments here are very identical to the comments for normal Sign Method.
I'd be surprised if they weren't ;-)
At the least you should refer to the // COSE defined detached payloads in Section 2 of [RFC9052]
I reckon the reference to S4.4 of RFC8152 is precise enough. A standards nerd would probably reference S4.4 of RFC9052 :-), but for the detached payload case, there is no material difference between 9052 and 8152.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomas-fossati I beg to differ. Section 4.4 RFC8152 talks about general signing. There is no reference to Detached Signing in 4.4
My intention was also to reference to specification which clarifies aspects of DetachedSigning, which is lacking in the reference!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomas-fossati I beg to differ. Section 4.4 RFC8152 talks about general signing. There is no reference to Detached Signing in 4.4
And rightly so, because there's nothing special about detached/non-detached in terms of signing and verification.
S4.4, Bullet point 5, second sentence:
[T]he full payload is used here, independent of how it is transported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any decision here?
// Notice: The COSE Sign API is EXPERIMENTAL and may be changed or removed in a | ||
// later release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Line 487 and 488 relevant here ?, given that we are commenting on Verify here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied these lines from the existing methods. I took it to mean the whole of the Sign/Verify API together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should change this to sign/verify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm waiting to see if anyone else has strong opinions on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor points on the comments to Verify, but in general LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to discuss more about the design in #195 before merging this PR as I have commented at #195 (comment)
func Sign1Detached(rand io.Reader, signer Signer, headers Headers, detached, external []byte) ([]byte, error) { | ||
msg := Sign1Message{ | ||
Headers: headers, | ||
} | ||
err := msg.SignDetached(rand, detached, external, signer) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return msg.MarshalCBOR() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need
func (m *SignMessage) SignDetached(rand io.Reader, detached, external []byte, signers ...Signer) error
func (m *Sign1Message) SignDetached(rand io.Reader, detached, external []byte, signer Signer) error
?
Can Sign1Detached
be implemented as
func Sign1Detached(rand io.Reader, signer Signer, headers Headers, detached, external []byte) ([]byte, error) { | |
msg := Sign1Message{ | |
Headers: headers, | |
} | |
err := msg.SignDetached(rand, detached, external, signer) | |
if err != nil { | |
return nil, err | |
} | |
return msg.MarshalCBOR() | |
} | |
func Sign1Detached(rand io.Reader, signer Signer, headers Headers, detached, external []byte) ([]byte, error) { | |
msg := Sign1Message{ | |
Headers: headers, | |
Payload: detached, | |
} | |
err := msg.Sign(rand, external, signer) | |
if err != nil { | |
return nil, err | |
} | |
msg.Payload = nil | |
return msg.MarshalCBOR() | |
} |
?
Same question for SignDetached
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fussy to me, would making detached the base case and calling from Sign1
be ok? Saves unsetting the payload on the detached object.
if detached == nil { | ||
return ErrMissingPayload | ||
} | ||
return m.verify(detached, external, verifiers...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can it be simplified as
return m.verify(detached, external, verifiers...) | |
msg := *m | |
msg.Payload = detached | |
return msg.Verify(external, verifiers...) |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clones the SignMessage
object, correct?
Signed-off-by: Alex Richards <alex-richards@users.noreply.github.com>
40060e0
to
fa0344c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #197 +/- ##
==========================================
- Coverage 92.04% 91.16% -0.88%
==========================================
Files 12 12
Lines 1973 1676 -297
==========================================
- Hits 1816 1528 -288
+ Misses 108 92 -16
- Partials 49 56 +7 ☔ View full report in Codecov by Sentry. |
Thanks for all the feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing to "Request changes"
Hey,
I've extended COSESign and COSESign1 to support detached payloads, adds a parameter to the public API, not sure if that's desirable.
I'm also pretty new to Go, I've tried to copy the existing style but I'm sure there's plenty that could be improved.
Cheers, Alex
#195