feat: support detached payloads in COSESign and COSESign1 #197

alex-richards · 2024-07-22T02:42:11Z

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

SteveLasker · 2024-07-22T15:18:20Z

Hi, @alex-richards,
Thanks for the PR. Several of the maintainers/reviewers are at IETF this week. Please give us a bit of time to review.

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.

alex-richards · 2024-07-22T21:46:25Z

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.

SteveLasker · 2024-07-25T02:36:56Z

Hi @alex-richards. Welcome to the verasion/go-cose project and thank you for your contributions.
As go-cose is used in many projects and products, can you provide more info about yourself? Your profile has very limited info, with no additional info in your commits.

Thank you

alex-richards · 2024-07-25T02:47:52Z

Hi @alex-richards. Welcome to the verasion/go-cose project and thank you for your contributions. As go-cose is used in many projects and products, can you provide more info about yourself? Your profile has very limited info, with no additional info in your commits.

Thank you

Hi Steve,

Yeah, sure. Do you have a preferred private way for me to get in touch?

SteveLasker · 2024-07-25T02:51:23Z

You can reach me through my linkedin profile info.

alex-richards · 2024-07-26T22:02:04Z

Hey Steve,

I've signed as discussed.

Cheers, Alex

SteveLasker · 2024-07-26T22:28:39Z

Thanks, @alex-richards,
Several folks were at IETF this week, so maintainers may take a few days to catch up with their "day jobs" and review.

OR13

This seems ok to me, but it would be good to get more reviews, since this touches several interfaces

SteveLasker · 2024-08-02T22:42:33Z

Thanks, @alex-richards, can you please resolve the DCO error: https://github.com/veraison/go-cose/pull/197/checks?check_run_id=27984447393

sign.go

alex-richards · 2024-08-13T07:43:41Z

@qmuntal I've made those changes, yeah, they're much clearer.

Not sure what resolving etiquette is here, tag you? Resolve myself?

qmuntal · 2024-08-13T07:47:19Z

@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 👍

sign.go

sign1.go

sign.go

qmuntal

LGTM, thanks!

alex-richards · 2024-08-14T21:20:25Z

Any merging etiquette I should know about? Or just go for it once it's approved? Is 2 enough?

SteveLasker · 2024-08-14T21:42:52Z

Due to the slightly larger change, it would be good to have an additional golang and cose expert weigh in
@thomas-fossati, @shizhMSFT l, @setrofim can either of you please take a look?

thomas-fossati

Looks very good to me. Thanks a lot for the great contribution!

bench_test.go

sign1.go

sign.go

yogeshbdeshpande

Left some minor points on the comments to Verify, but in general LGTM!

shizhMSFT

I'd like to discuss more about the design in #195 before merging this PR as I have commented at #195 (comment)

bench_test.go

sign.go

shizhMSFT · 2024-08-15T15:07:28Z

sign1.go

+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()
+}


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

Suggested change

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.

Seems fussy to me, would making detached the base case and calling from Sign1 be ok? Saves unsetting the payload on the detached object.

would making detached the base case and calling from Sign1 be ok?

IMO yes. @shizhMSFT wdyt?

@shizhMSFT, I believe this is the blocking issue on this PR. Can we consider refactoring the implementation, either in this PR, or a separate PR? Either way, can we come to some conclusion here to move forward?

@alex-richards Are you suggesting

func sign1(rand io.Reader, signer Signer, headers Headers, payload, external []byte) (*Sign1Message, error) { msg := Sign1Message{ Headers: headers, Payload: payload, } err := msg.Sign(rand, external, signer) if err != nil { return nil, err } return msg, nil } func Sign1(rand io.Reader, signer Signer, headers Headers, payload, external []byte) ([]byte, error) { msg, err := sign1(rand, signer, headers, payload, external) if err != nil { return nil, err } return msg.MarshalCBOR() } func Sign1Detached(rand io.Reader, signer Signer, headers Headers, detached, external []byte) ([]byte, error) { msg, err := sign1(rand, signer, headers, detached, external) if err != nil { return nil, err } msg.Payload = nil return msg.MarshalCBOR() }

That refactoring works for me.

@alex-richards, @shizhMSFT
Can this comment be resoled?

@SteveLasker I think it will be better if we gather more inputs from other maintainers.

Disclaimer: I am very much a random passerby, nudged to look at this. I do spend a lot time with COSE, in languages other than Go.

The refactoring proposed by @shizhMSFT seems quite reasonable to me, and results in a slightly cleaner API in my opinion.

Ping on this change for closure...

sign.go

Signed-off-by: Alex Richards <alex-richards@users.noreply.github.com>

codecov · 2024-08-16T07:20:53Z

Codecov Report

Attention: Patch coverage is 65.78947% with 26 lines in your changes missing coverage. Please review.

Project coverage is 91.06%. Comparing base (2b6f94f) to head (9a459d1).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
sign1.go	40.90%	26 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
- Coverage   92.04%   91.06%   -0.99%     
==========================================
  Files          12       12              
  Lines        1973     1678     -295     
==========================================
- Hits         1816     1528     -288     
+ Misses        108       94      -14     
- Partials       49       56       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

SteveLasker · 2024-08-16T15:53:18Z

Thanks for all the feedback.
To move forward, there are a number of questions, which seem to be change requests on this PR. Yet, there are several approvals.
Can folks please update their comments?
If you have a comment you expect to be addressed before merging, please change to a change request.
If there are no "change requests" by August 23, 2024, and we have 2 or more approvals from maintainers, we'll move to merge.

shizhMSFT

Changing to "Request changes"

SteveLasker · 2024-09-13T14:55:02Z

A note we'll be reviewing today in the fortnightly go-cose meeting if folks would like to attend or comment prior to the meeting.

Co-authored-by: Shiwei Zhang <shizh@microsoft.com> Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>

OR13

Did a full review, @shizhMSFT 's comments need to be applied or dismissed.

SteveLasker · 2024-09-13T15:27:59Z

@shizhMSFT, @alex-richards, this feedback appears to be the blocking topic. Can we focus on unblocking that element? I see the discussion on #195 and #205.
With the summer over, can we target resolving by Sept 20th?

SteveLasker · 2024-09-27T14:47:34Z

@alex-richards, @shizhMSFT, gentle ping on Issue #195 and PR #197

OR13 · 2024-12-06T16:20:12Z

The discussion continues, I will approve the PR when the comments are applied or dismissed, and the PR is mergable.

OR13 · 2024-12-06T16:21:46Z

If we think this is a dead end, we should close the PR and await a PR that is mergable

alex-richards requested review from henkbirkholz, qmuntal, roywill, setrofim, shizhMSFT, SimonFrost-Arm, SteveLasker, thomas-fossati and yogeshbdeshpande as code owners July 22, 2024 02:42

alex-richards force-pushed the detached-payloads branch 2 times, most recently from 4e80ef5 to bc11d1c Compare July 22, 2024 22:59

alex-richards force-pushed the detached-payloads branch from bc11d1c to 1736a40 Compare July 26, 2024 22:01

OR13 approved these changes Aug 2, 2024

View reviewed changes

alex-richards force-pushed the detached-payloads branch from 1736a40 to a636db9 Compare August 2, 2024 23:07

alex-richards mentioned this pull request Aug 13, 2024

feat: COSEMac #198

Draft

qmuntal requested changes Aug 13, 2024

View reviewed changes

sign.go Outdated Show resolved Hide resolved

sign.go Outdated Show resolved Hide resolved

alex-richards force-pushed the detached-payloads branch from a636db9 to 94007e7 Compare August 13, 2024 07:41

qmuntal requested changes Aug 13, 2024

View reviewed changes

alex-richards force-pushed the detached-payloads branch from 94007e7 to 40060e0 Compare August 14, 2024 00:25

qmuntal approved these changes Aug 14, 2024

View reviewed changes

thomas-fossati approved these changes Aug 15, 2024

View reviewed changes

bench_test.go Outdated Show resolved Hide resolved

yogeshbdeshpande reviewed Aug 15, 2024

View reviewed changes

sign1.go Show resolved Hide resolved

yogeshbdeshpande reviewed Aug 15, 2024

View reviewed changes

sign.go Show resolved Hide resolved

yogeshbdeshpande approved these changes Aug 15, 2024

View reviewed changes

shizhMSFT reviewed Aug 15, 2024

View reviewed changes

shizhMSFT linked an issue Aug 15, 2024 that may be closed by this pull request

External Payload Support for CoseSign1 #195

Open

feat: support detached payloads in COSESign and COSESign1

fa0344c

Signed-off-by: Alex Richards <alex-richards@users.noreply.github.com>

alex-richards force-pushed the detached-payloads branch from 40060e0 to fa0344c Compare August 16, 2024 07:14

SteveLasker added this to the v1.4.0 milestone Aug 16, 2024

shizhMSFT requested changes Aug 24, 2024

View reviewed changes

shizhMSFT mentioned this pull request Aug 24, 2024

External Payload Support for CoseSign1 #195

Open

Update sign.go

9a459d1

Co-authored-by: Shiwei Zhang <shizh@microsoft.com> Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>

OR13 requested changes Sep 13, 2024

View reviewed changes

SteveLasker mentioned this pull request Sep 13, 2024

test: add examples for detached payload #205

Merged

feat: support detached payloads in COSESign and COSESign1 #197

Are you sure you want to change the base?

feat: support detached payloads in COSESign and COSESign1 #197

Conversation

alex-richards commented Jul 22, 2024

SteveLasker commented Jul 22, 2024

alex-richards commented Jul 22, 2024

SteveLasker commented Jul 25, 2024

alex-richards commented Jul 25, 2024

SteveLasker commented Jul 25, 2024

alex-richards commented Jul 26, 2024

SteveLasker commented Jul 26, 2024

OR13 left a comment

Choose a reason for hiding this comment

SteveLasker commented Aug 2, 2024

alex-richards commented Aug 13, 2024

qmuntal commented Aug 13, 2024

qmuntal left a comment

Choose a reason for hiding this comment

alex-richards commented Aug 14, 2024

SteveLasker commented Aug 14, 2024

thomas-fossati left a comment

Choose a reason for hiding this comment

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-richards Aug 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2024 • edited Loading

Codecov Report

SteveLasker commented Aug 16, 2024

shizhMSFT left a comment

Choose a reason for hiding this comment

SteveLasker commented Sep 13, 2024

OR13 left a comment

Choose a reason for hiding this comment

SteveLasker commented Sep 13, 2024

SteveLasker commented Sep 27, 2024

OR13 commented Dec 6, 2024

OR13 commented Dec 6, 2024

alex-richards Aug 18, 2024 •

edited

Loading

codecov bot commented Aug 16, 2024 •

edited

Loading