Remote Signing by shizhMSFT · Pull Request #51 · veraison/go-cose

shizhMSFT · 2022-04-20T11:00:55Z

This PR fills the gap to a mature go-cose implementation with security improvements and remote-signing extensibility.
Details are available at DESIGN.md.

It resolves #37 and the following highlighted issues:

This PR is co-authored by @qmuntal.

shizhMSFT · 2022-04-20T11:24:03Z

@thomas-fossati The DCO check fails on 5 commits with error message:

Author: Quim Muntal, Committer: GitHub; Expected "Quim Muntal quimmuntal@gmail.com", but got "qmuntal qmuntaldiaz@microsoft.com".

How can we resolve it? or should we just set DCO to pass?

/cc @qmuntal

SteveLasker · 2022-04-20T11:45:39Z

unfortunately, bypassing dco bypasses the licensing and introduces the potential for ownership of the code.
Can you resolve in the branch and resubmit?

Co-authored-by: qmuntal <qmuntaldiaz@microsoft.com> Signed-off-by: Shiwei Zhang <shizh@microsoft.com>

shizhMSFT · 2022-04-20T13:00:35Z

Squashed commits with DCO fixed.

thomas-fossati · 2022-04-20T13:05:38Z

LGTM, thanks all for the incredible work.

Should we explicitly tag main before merging? (I think Steve suggested this some time ago.)

SteveLasker · 2022-04-20T13:09:10Z

awesome work!
Yeah, let’s tag main into a branch.
Naming is hard. @thomas-fossati, what tag/branch name would be meaningful for you? If we look 6 months from now, what name would make sense?

README.md

thomas-fossati · 2022-04-20T13:23:14Z

Naming is hard. @thomas-fossati, what tag/branch name would be meaningful for you? If we look 6 months from now, what name would make sense?

I'm after something that evokes legacy / impending deprecation.
Any suggestions? :-)

yogeshbdeshpande · 2022-04-20T13:31:20Z

DESIGN.md

+
+Here are the gaps between the current implementation and the objectives:
+
+- The current implementation implements `COSE_Sign` and `COSE_Sign1`. However, the verification process does not follow [RFC8152](https://datatracker.ietf.org/doc/html/rfc8152) and causes [issue #7](https://github.com/veraison/go-cose/issues/7). There are also implementation errors in signing and verification (see [issue #8](https://github.com/veraison/go-cose/issues/8)).


I think the tense of the sentence should now reflect past tense with the implementation, example like earlier implementation. It should also say, had caused issues like #7 and #8 which is been fixed in the current implementation.

@shizhMSFT, can you make this minor wording change?
Suggested change:

Prior versions of go-cose implemented COSE_Sign and COSE_Sign1. However, the verification process did not follow RFC8152 which had caused issues like #7 and #8 which have been fixed in the current implementation.

updated in 6ef3dd4

yogeshbdeshpande · 2022-04-20T13:35:00Z

DESIGN.md

+  - All conventional signature schemes are _signature with appendix_.
+  - `RS256`, `RS384`, `RS512`, `ES256K` are marked **NOT RECOMMENDED** by [RFC8812](https://datatracker.ietf.org/doc/html/rfc8812).
+- The current implementation has solid `Signer` and `Verify` structures and can only sign or verify against certain algorithms (see [issue #9](https://github.com/veraison/go-cose/issues/9)). The implementation cannot be extended to have new algorithms or new implementation for existing algorithms.
+- The current implementation is not golang native.


Is Line 28 referring to veraison/go-cose or some other repo? when it refers to golang nativity ?

As far as I reckon that was with older notary implementation ? If so needs to be specified clearly.

@shizhMSFT, what do you think?

updated in 6ef3dd4

SteveLasker · 2022-04-20T13:37:10Z

Agreed on the fork info atop. We can update the readme a bit later when we’ve decoupled. For now, just getting the code merged is a great start so we can start the dependencies and flush out the next round of issues

yogeshbdeshpande · 2022-04-20T14:03:20Z

anch name would be meaningful for

I suggest a simple strategy, example(Feel free to shoot it down):
go-cose-v.1.0.0

and then when Notary major release comes go-cose-v.2.0.0
2.0.1 for notary patches and 2.1.1 for notary enhancements?

qmuntal · 2022-04-20T14:24:29Z

anch name would be meaningful for

I suggest a simple strategy, example(Feel free to shoot it down): go-cose-v.1.0.0

and then when Notary major release comes go-cose-v.2.0.0 2.0.1 for notary patches and 2.1.1 for notary enhancements?

Module tags should strictly follow semantic versioning with v prefix, else Go toolchain won't like our releases. This is what I would do:

Tag main as-is now with v0.1.0.
Merge this PR and create a new tag named v0.2.0.
Once we pass all security reviews and we are confident we have a stable API, we can aim for the v1.0.0.

yogeshbdeshpande · 2022-04-20T14:26:36Z

anch name would be meaningful for

I suggest a simple strategy, example(Feel free to shoot it down): go-cose-v.1.0.0
and then when Notary major release comes go-cose-v.2.0.0 2.0.1 for notary patches and 2.1.1 for notary enhancements?

Module tags should strictly follow semantic versioning with v prefix, else Go toolchain won't like our releases. This is what I would do:

Tag main as-is now with v0.1.0.

Merge this PR and create a new tag named v0.2.0.

Once we pass all security reviews and we are confident we have a stable API, we can aim for the v1.0.0.

Works for me!

shizhMSFT · 2022-04-20T14:27:24Z

Golang projects follows SemVer2.

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes,

MINOR version when you add functionality in a backwards compatible manner, and

PATCH version when you make backwards compatible bug fixes.

Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

The previous go-cose implementation is not SemVer2 versioned. To golang, it is still at v0.0.0. Then we can start with v1.0.0.

If the previous implementation is required to be v1.0.0, then we can have this implementation as v2.0.0. However, if we do want v2.0.0, we need to update the go.mod file to github.com/veraison/go-cose/v2. Meanwhile, we have to fix all vulnerabilities in v1.0.0 or deprecate v1.0.0 immediately after tagging it for security concerns.

SteveLasker · 2022-04-20T14:33:07Z

what about v0.0.0-sign1 , with comments that this is NOT a supported release with 1.0 as the supported codebase

qmuntal · 2022-04-20T14:36:09Z

what about v0.0.0-sign1 , with comments that this is NOT a supported release with 1.0 as the supported codebase

Like it and +1 one what @shizhMSFT said. I would just not start with the v1.0.0 but with v0.1.0 so we can still iterate on the API a few weeks more without breaking any compatibility promise.

SteveLasker · 2022-04-20T20:12:42Z

I've created tag v0.0.0-sign1-alpha.0 and created the branch v0.0.0-sign1-alpha.0

I've also updated issue #37 to reflect the steps

@shizhMSFT, if you could make the minor changes noted above

I believe we're ready to merge

Signed-off-by: Shiwei Zhang <shizh@microsoft.com>

thomas-fossati

🚢 it!

yogeshbdeshpande · 2022-04-21T08:25:41Z

Great Job Shiwei! Thank you for all the Hard Work.

SteveLasker

LGTM

shizhMSFT requested review from a team, SteveLasker and thomas-fossati and removed request for SteveLasker and thomas-fossati April 20, 2022 11:20

shizhMSFT added this to the v1-alpha.1 milestone Apr 20, 2022

remote signing

2d91442

Co-authored-by: qmuntal <qmuntaldiaz@microsoft.com> Signed-off-by: Shiwei Zhang <shizh@microsoft.com>

shizhMSFT force-pushed the remote-signing branch from 3b0a1f6 to 2d91442 Compare April 20, 2022 12:53

yogeshbdeshpande reviewed Apr 20, 2022

View reviewed changes

README.md Show resolved Hide resolved

yogeshbdeshpande reviewed Apr 20, 2022

View reviewed changes

SteveLasker mentioned this pull request Apr 20, 2022

Merge Remote Signing Branch into Main #37

Closed

5 tasks

revise design doc

6ef3dd4

Signed-off-by: Shiwei Zhang <shizh@microsoft.com>

thomas-fossati approved these changes Apr 21, 2022

View reviewed changes

yogeshbdeshpande approved these changes Apr 21, 2022

View reviewed changes

SteveLasker approved these changes Apr 21, 2022

View reviewed changes

SteveLasker merged commit 52f40e3 into main Apr 21, 2022

SteveLasker deleted the remote-signing branch April 21, 2022 12:47

SteveLasker mentioned this pull request Apr 22, 2022

Remove design.md as remote signing has merged #57

Merged


		Here are the gaps between the current implementation and the objectives:

		- The current implementation implements `COSE_Sign` and `COSE_Sign1`. However, the verification process does not follow [RFC8152](https://datatracker.ietf.org/doc/html/rfc8152) and causes [issue #7](https://github.com/veraison/go-cose/issues/7). There are also implementation errors in signing and verification (see [issue #8](https://github.com/veraison/go-cose/issues/8)).

Conversation

shizhMSFT commented Apr 20, 2022

Uh oh!

shizhMSFT commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveLasker commented Apr 20, 2022

Uh oh!

shizhMSFT commented Apr 20, 2022

Uh oh!

thomas-fossati commented Apr 20, 2022

Uh oh!

SteveLasker commented Apr 20, 2022

Uh oh!

Uh oh!

thomas-fossati commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yogeshbdeshpande Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

SteveLasker Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

shizhMSFT Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

yogeshbdeshpande Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

SteveLasker Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

shizhMSFT Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

SteveLasker commented Apr 20, 2022

Uh oh!

yogeshbdeshpande commented Apr 20, 2022

Uh oh!

qmuntal commented Apr 20, 2022

Uh oh!

yogeshbdeshpande commented Apr 20, 2022

Uh oh!

shizhMSFT commented Apr 20, 2022

Uh oh!

SteveLasker commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qmuntal commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveLasker commented Apr 20, 2022

Uh oh!

thomas-fossati left a comment

Choose a reason for hiding this comment

Uh oh!

yogeshbdeshpande commented Apr 21, 2022

Uh oh!

SteveLasker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shizhMSFT commented Apr 20, 2022 •

edited

Loading

thomas-fossati commented Apr 20, 2022 •

edited

Loading

SteveLasker commented Apr 20, 2022 •

edited

Loading

qmuntal commented Apr 20, 2022 •

edited

Loading