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.

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

Add oauth2_metadata config option #320

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

DrDaveD
Copy link
Contributor

@DrDaveD DrDaveD commented May 3, 2024

This PR adds a configuration option oauth2_metadata which is a list of token types that can be returned in metadata. The list can be any of access_token, id_token, and refresh_token, and when selected they are returned in metadata names oauth2_access_token, oauth2_id_token, and oauth2_refresh_token, respectively.

This option makes it possible for a user to authenticate with bao once and also use the tokens directly from the oauth2 issuer for another purpose. For example, with some help from a client application to store the refresh token back into a vault secrets plugin like the Puppet labs oauthapp plugin, from then on access tokens can be read from the secrets plugin and renewed when they expire.

The new option simply puts the selected tokens into the returned metadata in a similarly to the way that the claims_mapping option returns things in metadata.

This PR was proposed for vault several years ago in hashicorp/vault-plugin-auth-jwt#119 but has been sitting unmerged since then. It is an essential component of the https://github.com/fermitools/htvault-config package and has been in production use for a couple of years.

@DrDaveD
Copy link
Contributor Author

DrDaveD commented Jul 10, 2024

@cipherboy If this can be merged along with #318 and #319 that would be awesome.

@cipherboy
Copy link
Member

@DrDaveD Are these two (#319) independent?

@DrDaveD
Copy link
Contributor Author

DrDaveD commented Jul 18, 2024

Yes they are independent. It's just that my application needs both.

Copy link
Member

@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.

Two minor comments.

I think I'm happy with this change, but I'd like to see it called out better in the docs as something to be mindful of: previously an OpenBao OIDC-bound token could not be used to pivot to other systems natively (if a token was compromised), but with this in the metadata, it could potentially be used to pivot now (depending on claims?).

(That is, assuming I'm correct about the former, which I believe is correct, but might be wrong about the existing client-bound OIDC workflow that existed in upstream -- I don't believe it currently sees either of these tokens and they're internal to upstream/OpenBao even in that workflow, but my understanding could be wrong.)

changelog/320.txt Outdated Show resolved Hide resolved
builtin/credential/jwt/path_role.go Show resolved Hide resolved
@DrDaveD
Copy link
Contributor Author

DrDaveD commented Aug 13, 2024

I added a caution to the web documentation.

Signed-off-by: Dave Dykstra <2129743+DrDaveD@users.noreply.github.com>
Copy link
Member

@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.

Looks great, thanks @DrDaveD for the changes!

@cipherboy
Copy link
Member

Looks like there's a flake in a different test with the data race detector:

2024-08-13T22:35:33.2621062Z === �[31mFailed�[0m
2024-08-13T22:35:33.2622304Z === �[31mFAIL�[0m: vault TestOIDC_PeriodicFunc/test-key-nil-next-signing-key (4.62s)
...
2024-08-13T22:35:33.3387975Z     identity_store_oidc_test.go:1165: For key: test-key-nil-next-signing-key at cycle: 2 expected namedKey's KeyRing to be at least of length 3 but was: 2
2024-08-13T22:35:33.3389873Z     identity_store_oidc_test.go:1176: For key: test-key-nil-next-signing-key at cycle: 2 expected public keys to be at least of length 3 but was: 2

@cipherboy cipherboy merged commit 7757b5b into openbao:main Aug 14, 2024
53 of 54 checks passed
@DrDaveD DrDaveD deleted the add-oauth2-metadata branch August 14, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants