-
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
Remote Signing #51
Remote Signing #51
Conversation
@thomas-fossati The DCO check fails on 5 commits with error message:
How can we resolve it? or should we just set DCO to pass? /cc @qmuntal |
unfortunately, bypassing dco bypasses the licensing and introduces the potential for ownership of the code. |
Co-authored-by: qmuntal <qmuntaldiaz@microsoft.com> Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
3b0a1f6
to
2d91442
Compare
Squashed commits with DCO fixed. |
LGTM, thanks all for the incredible work. Should we explicitly tag main before merging? (I think Steve suggested this some time ago.) |
awesome work! |
I'm after something that evokes legacy / impending deprecation. |
DESIGN.md
Outdated
|
||
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)). |
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.
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.
@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.
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.
updated in 6ef3dd4
DESIGN.md
Outdated
- 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. |
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 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.
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.
@shizhMSFT, what do you think?
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.
updated in 6ef3dd4
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 |
I suggest a simple strategy, example(Feel free to shoot it down): and then when Notary major release comes go-cose-v.2.0.0 |
Module tags should strictly follow semantic versioning with
|
Works for me! |
Golang projects follows SemVer2.
The previous If the previous implementation is required to be |
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. |
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>
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.
🚢 it!
Great Job Shiwei! Thank you for all the Hard Work. |
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
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:
nil
external data should imply empty slice, not nil #8crit
in protected header #23alg
protected header when Sign and Verify if external data not supplied #32This PR is co-authored by @qmuntal.