Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor: JWS for signature package #76

Merged
merged 8 commits into from
Sep 21, 2022
Merged

refactor: JWS for signature package #76

merged 8 commits into from
Sep 21, 2022

Conversation

JeyJeyGao
Copy link
Contributor

@JeyJeyGao JeyJeyGao commented Sep 14, 2022 •
edited
Loading

This PR refactors the JWS implementation to support the new signature package.

  1. the JWS Sign implementation was replaced by JWT Sign
  2. the jwsEnvelope was refactored to support Envelope interface
type Envelope interface {
	Sign(req *SignRequest) ([]byte, error)

	Verify() (*EnvelopeContent, error)

	Content() (*EnvelopeContent, error)
}
  1. JWS verify used JWT package built-in steps to select SigningMethod

Signed-off-by: Junjie Gao [email protected]

chloeyin
chloeyin reviewed Sep 15, 2022
View reviewed changes
signature/jws/jwt_test.go Outdated Show resolved Hide resolved
patrickzheng200
patrickzheng200 reviewed Sep 15, 2022
View reviewed changes
signature/jws/jws.go Outdated Show resolved Hide resolved
signature/jws/envelope.go Outdated Show resolved Hide resolved
signature/jws/envelope.go Show resolved Hide resolved
signature/jws/envelope.go Outdated Show resolved Hide resolved
signature/jws/envelope.go Outdated Show resolved Hide resolved
signature/jws/jws.go Outdated Show resolved Hide resolved
signature/jws/jws.go Outdated Show resolved Hide resolved
signature/jws/jws.go Outdated Show resolved Hide resolved
signature/jws/jwt.go Outdated Show resolved Hide resolved
signature/jws/jwt.go Show resolved Hide resolved
binbin-li
binbin-li reviewed Sep 15, 2022
View reviewed changes
signature/jws/envelope.go Outdated Show resolved Hide resolved
signature/jws/jwt.go Outdated Show resolved Hide resolved
@JeyJeyGao JeyJeyGao force-pushed the cose-dev branch 6 times, most recently from 197e83e to cf01889 Compare September 15, 2022 06:15
shizhMSFT
shizhMSFT approved these changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JeyJeyGao
fix: resolve comments
0c64ed4 
Signed-off-by: Junjie Gao <[email protected]>
shizhMSFT
shizhMSFT reviewed Sep 16, 2022
View reviewed changes
signature/types.go Outdated Show resolved Hide resolved
signature/types.go Outdated Show resolved Hide resolved
@JeyJeyGao
fix: resolve comments
8687d0d 
Signed-off-by: Junjie Gao <[email protected]>
priteshbandi
priteshbandi reviewed Sep 19, 2022
View reviewed changes
signature/jws/jwt.go Show resolved Hide resolved
signature/jws/jwt.go Show resolved Hide resolved
signature/jws/jwt.go Show resolved Hide resolved
signature/jws/types.go Show resolved Hide resolved
signature/signaturetest/algorithm.go Show resolved Hide resolved
@JeyJeyGao
fix: resolve comments
cd5cadf 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update comment
0af93b2 
Signed-off-by: Junjie Gao <[email protected]>
shizhMSFT
shizhMSFT approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

patrickzheng200
patrickzheng200 approved these changes Sep 20, 2022
View reviewed changes
Copy link

@patrickzheng200 patrickzheng200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

priteshbandi
priteshbandi reviewed Sep 20, 2022
View reviewed changes
Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few nit picks otherwise LGTM

signature/jws/conformance_test.go Outdated Show resolved Hide resolved
signature/jws/envelope.go Outdated Show resolved Hide resolved
signature/jws/envelope.go Outdated Show resolved Hide resolved
signature/jws/jwt.go Show resolved Hide resolved
@JeyJeyGao
fix: resolve comments
b6eae16 
Signed-off-by: Junjie Gao <[email protected]>
shizhMSFT
shizhMSFT approved these changes Sep 21, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

priteshbandi
priteshbandi approved these changes Sep 21, 2022
View reviewed changes
Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shizhMSFT shizhMSFT merged commit 9c1169a into notaryproject:stage/cose Sep 21, 2022
@shizhMSFT shizhMSFT mentioned this pull request Sep 21, 2022
Merged
shizhMSFT pushed a commit that referenced this pull request Sep 21, 2022
@JeyJeyGao @shizhMSFT
refactor: JWS for signature package (#76)
f14510c 
Signed-off-by: Junjie Gao <[email protected]>
patrickzheng200 pushed a commit that referenced this pull request Sep 23, 2022
@Two-Hearts @dependabot
@binbin-li @shizhMSFT @JeyJeyGao @priteshbandi
update cose branch (#80)
70115d5 
* Bump github.com/golang-jwt/jwt/v4 from 4.4.1 to 4.4.2 (#14)

Bumps [github.com/golang-jwt/jwt/v4](https://github.com/golang-jwt/jwt) from 4.4.1 to 4.4.2.
- [Release notes](https://github.com/golang-jwt/jwt/releases)
- [Changelog](https://github.com/golang-jwt/jwt/blob/main/VERSION_HISTORY.md)
- [Commits](golang-jwt/jwt@v4.4.1...v4.4.2)

---
updated-dependencies:
- dependency-name: github.com/golang-jwt/jwt/v4
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* refactor: refactor envelope and signer to support cose (#53)

Signed-off-by: Binbin Li <[email protected]>

* Revert "refactor: refactor envelope and signer to support cose (#53)" (#67)

This reverts commit cbea2cf.

Signed-off-by: Shiwei Zhang <[email protected]>

* update to go 1.18 (#66)

Signed-off-by: Junjie Gao <[email protected]>

* refactor: rename signer to signature submodule (#71)

Signed-off-by: Binbin Li <[email protected]>

* refactor: refactor envelope and signer to support cose (#73)

Signed-off-by: Binbin Li <[email protected]>

* feat: added COSE envelope implementation

Signed-off-by: Patrick Zheng <[email protected]>

* update per code review

Signed-off-by: Patrick Zheng <[email protected]>

* update per code review

Signed-off-by: Patrick Zheng <[email protected]>

* update per code review

Signed-off-by: Patrick Zheng <[email protected]>

* resolve conflicts

Signed-off-by: Patrick Zheng <[email protected]>

* updated conformance test

Signed-off-by: Patrick Zheng <[email protected]>

* refactor: JWS for signature package (#76)

Signed-off-by: Junjie Gao <[email protected]>

* refactor: rename signer to signature submodule (#71)

Signed-off-by: Binbin Li <[email protected]>

* refactor: refactor envelope and signer to support cose (#73)

Signed-off-by: Binbin Li <[email protected]>

* refactor: JWS for signature package (#76)

Signed-off-by: Junjie Gao <[email protected]>

* Allows signing with self-signed certificate (#77)

Signed-off-by: Pritesh Bandi <[email protected]>

* updated conformance_test

Signed-off-by: Patrick Zheng <[email protected]>

* refactor: refactor comments (#78)

Signed-off-by: Binbin Li <[email protected]>

* updated per code review

Signed-off-by: Patrick Zheng <[email protected]>

* updated per code review

Signed-off-by: Patrick Zheng <[email protected]>

* removed CLI related maps, they should be in notation CLI

Signed-off-by: Patrick Zheng <[email protected]>

* resolved conflicts

Signed-off-by: Patrick Zheng <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Binbin Li <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Binbin Li <[email protected]>
Co-authored-by: Shiwei Zhang <[email protected]>
Co-authored-by: Junjie Gao <[email protected]>
Co-authored-by: Binbin Li <[email protected]>
Co-authored-by: Junjie Gao <[email protected]>
Co-authored-by: Pritesh Bandi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chloeyin chloeyin chloeyin left review comments

@binbin-li binbin-li binbin-li left review comments

@patrickzheng200 patrickzheng200 patrickzheng200 approved these changes

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@priteshbandi priteshbandi priteshbandi approved these changes

@gokarnm gokarnm Awaiting requested review from gokarnm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JeyJeyGao @binbin-li @priteshbandi @chloeyin @shizhMSFT @patrickzheng200