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

Upgrade go-jose library to v3 #20559

Merged
merged 4 commits into from
May 23, 2023
Merged

Upgrade go-jose library to v3 #20559

merged 4 commits into from
May 23, 2023

Conversation

sagikazarmark
Copy link
Contributor

go-jose v2 is EOL as of 27 February: https://github.com/square/go-jose

This PR upgrades the API to use v3 of the library: https://github.com/go-jose/go-jose

@hashicorp-cla
Copy link

hashicorp-cla commented May 10, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@sagikazarmark sagikazarmark force-pushed the upgrade-go-jose branch 2 times, most recently from 46752ae to 599d68b Compare May 10, 2023 21:41
@cipherboy cipherboy requested review from swenson and a team May 11, 2023 11:44
@cipherboy
Copy link
Contributor

Many thanks for this, @sagikazarmark! Are you interested in another PR for the rest of Vault (the core server implementation)?

@cipherboy
Copy link
Contributor

cipherboy commented May 11, 2023 •
edited
Loading

Ah yes, we can't update just the API's version without bumping it in the main /go.mod version as well:

Error: api/plugin_helpers.go:17:2: github.com/hashicorp/vault/api@v1.9.1 requires
	github.com/go-jose/go-jose/v3@v3.0.0: missing go.sum entry; to add it:
	go mod download github.com/go-jose/go-jose/v3
make: *** [Makefile:292: ci-build] Error 1
Error: Process completed with exit code 2.

I think go mod tidy is sufficient to fix this without having to update the main Vault version as well.

@sagikazarmark
Copy link
Contributor Author

Sure, let me take a look.

@sagikazarmark
Copy link
Contributor Author

@cipherboy ran go mod tidy.

maxb
maxb reviewed May 11, 2023
View reviewed changes
api/plugin_helpers.go Outdated Show resolved Hide resolved
@maxb
Copy link
Contributor

maxb commented May 11, 2023

Are you interested in another PR for the rest of Vault (the core server implementation)?

Maybe just do it all in this PR? It would be a bit weird to prioritize updating the single use-case in api, which is in code which is only used in Vault plugin development, and is disabled and skipped anyway, when running with a modern Vault version with modern go-plugin auto-mTLS support.

@sagikazarmark
Copy link
Contributor Author

@maxb updated all occurrences in Vault code as well. The remaining imports are due to references to older versions of the api:

❯ grep -r square/go-jose .
./go.mod:       gopkg.in/square/go-jose.v2 v2.6.0 // indirect
./go.sum:gopkg.in/square/go-jose.v2 v2.2.2/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./go.sum:gopkg.in/square/go-jose.v2 v2.3.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./go.sum:gopkg.in/square/go-jose.v2 v2.5.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./go.sum:gopkg.in/square/go-jose.v2 v2.6.0 h1:NGk74WTnPKBNUhNzQX7PYcTLUjoq7mzKk2OKbvwk2iI=
./go.sum:gopkg.in/square/go-jose.v2 v2.6.0/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./sdk/go.mod:   gopkg.in/square/go-jose.v2 v2.6.0 // indirect
./sdk/go.sum:gopkg.in/square/go-jose.v2 v2.6.0 h1:NGk74WTnPKBNUhNzQX7PYcTLUjoq7mzKk2OKbvwk2iI=
./sdk/go.sum:gopkg.in/square/go-jose.v2 v2.6.0/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./api/auth/userpass/go.sum:gopkg.in/square/go-jose.v2 v2.5.1 h1:7odma5RETjNHWJnR32wx8t+Io4djHE1PqxCFx3iiZ2w=
./api/auth/userpass/go.sum:gopkg.in/square/go-jose.v2 v2.5.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./api/auth/azure/go.sum:gopkg.in/square/go-jose.v2 v2.5.1 h1:7odma5RETjNHWJnR32wx8t+Io4djHE1PqxCFx3iiZ2w=
./api/auth/azure/go.sum:gopkg.in/square/go-jose.v2 v2.5.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./api/auth/ldap/go.sum:gopkg.in/square/go-jose.v2 v2.5.1 h1:7odma5RETjNHWJnR32wx8t+Io4djHE1PqxCFx3iiZ2w=
./api/auth/ldap/go.sum:gopkg.in/square/go-jose.v2 v2.5.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./api/auth/approle/go.sum:gopkg.in/square/go-jose.v2 v2.5.1 h1:7odma5RETjNHWJnR32wx8t+Io4djHE1PqxCFx3iiZ2w=
./api/auth/approle/go.sum:gopkg.in/square/go-jose.v2 v2.5.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./api/auth/gcp/go.sum:gopkg.in/square/go-jose.v2 v2.5.1 h1:7odma5RETjNHWJnR32wx8t+Io4djHE1PqxCFx3iiZ2w=
./api/auth/gcp/go.sum:gopkg.in/square/go-jose.v2 v2.5.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./api/auth/aws/go.sum:gopkg.in/square/go-jose.v2 v2.5.1 h1:7odma5RETjNHWJnR32wx8t+Io4djHE1PqxCFx3iiZ2w=
./api/auth/aws/go.sum:gopkg.in/square/go-jose.v2 v2.5.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=
./api/auth/kubernetes/go.sum:gopkg.in/square/go-jose.v2 v2.5.1 h1:7odma5RETjNHWJnR32wx8t+Io4djHE1PqxCFx3iiZ2w=
./api/auth/kubernetes/go.sum:gopkg.in/square/go-jose.v2 v2.5.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=

cipherboy
cipherboy approved these changes May 12, 2023
View reviewed changes
Copy link
Contributor

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

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

This looks good from my side. It appears the remaining usage of go-jose v2 is in the Okta plugin:

[cipherboy@xps15 okta]$ go mod why gopkg.in/square/go-jose.v2
# gopkg.in/square/go-jose.v2
github.com/hashicorp/vault/builtin/credential/okta
github.com/okta/okta-sdk-golang/v2/okta
gopkg.in/square/go-jose.v2

@miagilepner / @hghaf099, do either of you two know if there's a later version of okta-sdk-golang that uses go-jose v3?

@cipherboy cipherboy added the dependencies Pull requests that update a dependency file label May 12, 2023
@sagikazarmark
Copy link
Contributor Author

@cipherboy doesn't look like it. I'm gonna send a PR to them as well, but I don't think that should be a blocker (not at least for merging the PR/an API release).

Auth libraries will also have the old jose library as a reference, until the API is updated: https://github.com/hashicorp/vault/tree/main/api/auth

@sagikazarmark
Copy link
Contributor Author

okta/okta-sdk-golang#377

swenson
swenson approved these changes May 15, 2023
View reviewed changes
Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

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

LGTM

@sagikazarmark
upgrade go-jose library to v3
e75f38e 
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
@sagikazarmark
chore: fix unnecessary import alias
e30bc99 
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
@sagikazarmark
upgrade go-jose library to v2 in vault
a94d942 
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
@sagikazarmark
Copy link
Contributor Author

@cipherboy @maxb anything else I need to do to get this merged? Thanks!

@cipherboy cipherboy enabled auto-merge (squash) May 23, 2023 11:59
@cipherboy
Add changelog entry
2723412 
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy
Copy link
Contributor

@sagikazarmark Many thanks for the ping :-) I've pushed a changelog entry for this one and set this to auto-merge.

@cipherboy cipherboy merged commit 200f0c0 into hashicorp:main May 23, 2023
@cipherboy
Copy link
Contributor

@sagikazarmark Thank you for the PR! I've subscribed to the Okta PR so I'll get a reminder to update when that one merges.

@sagikazarmark sagikazarmark deleted the upgrade-go-jose branch May 23, 2023 12:45
@sagikazarmark
Copy link
Contributor Author

Perfect! Thanks!

@sagikazarmark
Copy link
Contributor Author

@cipherboy any ETA for the next API release?

@cipherboy
Copy link
Contributor

@sagikazarmark I'm not sure the exact date, but within the coming weeks I'd expect: #20808

@sagikazarmark
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxb maxb maxb left review comments

@swenson swenson swenson approved these changes

@cipherboy cipherboy cipherboy approved these changes

Assignees
No one assigned
Labels
cryptosec dependencies Pull requests that update a dependency file ecosystem
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sagikazarmark @hashicorp-cla @cipherboy @maxb @swenson