-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Upgrade api package go-jose to v4 #26527
Conversation
CI Results: |
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, though I'd really prefer we don't bump go.mod
from 1.19 if we don't have to.
Yeah me too :-( unfortunately go-jose bumped their min version way up starting from v4: https://github.com/go-jose/go-jose/blob/v4.0.0/go.mod It's tempting to try to remove the dependency entirely given how little it's used in the package, but I didn't feel as confident about the risk involved for that route. |
Build Results: |
Thanks! |
See https://github.com/go-jose/go-jose/blob/main/CHANGELOG.md#changed for notes on the changes that necessitated v4 of
github.com/go-jose/go-jose
. This upgrades to v4 so that other tools depending on theapi
package can avoid versions of the package with a CVE attached.The
api
package only uses the dependency once (perhaps we should look into trimming that), but it does use a function whose signature has changed. We now need to provide the allowed signing algorithms when parsing a JWT.AFAICT, ES512 has been in use for over 5 years for signing JWTs associated with this
unwrapToken
, so it should be the only algorithm we realistically need to support: https://github.com/hashicorp/vault/blame/1e36019f1c0f82e425661723f865fd7333225d6b/vault/wrapping.go#L221To help me gain confidence that the tests really were exercising this code path, I've initially committed support for the wrong algorithm. Locally, I've at least seen
TestPlugin_lifecycle/v4
withinbuiltin/logical/database
fail with ES256 and pass with ES512 which seems to make sense. Hopefully that result is repeated for the full test suite in CI as well.