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 oidc callback mode that is direct to server #318

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DrDaveD
Copy link
Contributor

@DrDaveD DrDaveD commented May 3, 2024

This adds a new option to the oidc auth method role option called callback_mode. When set to direct it enables the callback from the Authorization Server to be direct to bao instead of to the client. This allows clients from multiple users to share a machine because they do not need to share a port to listen on, and it also makes for easier management of firewalls, etc, because only the bao server needs to be configured to accept connections from the Authorization Server instead of every client.

When callback_mode=direct is set, the oidc/auth_url client API returns additional parameters 'state' and 'poll_interval'. The client is then expected to call a new API oidc/poll (instead of oidc/callback) and try again every poll_interval seconds while the response is an http 400 error authorization_pending. When the Authorization Server instead calls the oidc/callback api, the response is in html because it goes to the user's web browser, and the authorization information is stored in the state entry until the next call to oidc/poll.

The cli also has a new option callbackmode=direct (without an underscore) to apply different defaults for the redirect_uri parameter, based on the $VAULT_ADDR environment variable. That is a convenience and is not strictly necessary in order to make it work. When there is a state in the response to the oidc/auth_url API, instead of starting a listener the client calls back to oidc/poll every poll_interval seconds.

cli_responses.go is renamed to html_responses.go because it's not used exclusively for cli (and in fact it already wasn't).

Essentially the same PR has been pending in hashicorp/vault-plugin-auth-jwt#130 for several years, and although several other people expressed an interest in it, no action has been taken to merge it yet there. It has been in production use for a couple of years through https://github.com/fermitools/htvault-config.

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.

Thanks for this, it is looking fairly comprehensive @DrDaveD! One thing missing that I see off the top of my head are doc updates?

builtin/credential/jwt/path_oidc.go Show resolved Hide resolved
builtin/credential/jwt/path_role_test.go Show resolved Hide resolved
builtin/credential/jwt/path_role.go Show resolved Hide resolved
builtin/credential/jwt/cli.go Outdated Show resolved Hide resolved
builtin/credential/jwt/cli.go Show resolved Hide resolved
}

callbackPort, ok := m[FieldCallbackPort]
if !ok {
callbackPort = port
if serverURL != nil {
callbackPort = serverURL.Port() + "/v1/auth/" + mount
Copy link
Contributor

Choose a reason for hiding this comment

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

Since I'm thinking about this, this precludes namespaces right? I think you'd need path.Join(namespace, mount) in the general case, for this to work with Vault Enterprise fwiw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know anything about namespaces, and it doesn't otherwise occur in cli.go. There is another case in cli.go that already uses "/v1/auth" with mount so I think we should stick with the precedent.

},
},
Operations: map[logical.Operation]framework.OperationHandler{
logical.UpdateOperation: &framework.PathOperation{
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a logical.ReadOperation? I don't really view this poll as having updated the state, logically, to a user. Other read operations can update the state from time to time (PKI's CRL read for instance perhaps?), so I don't think we need to expose API operation == storage operation(s).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose so, but this was inspired by the poll operation in RFC8628 and it used a POST for this. Also, it would break compatibility with the client that has been in production use for several years and is installed in many places. It would be a nightmare to change.

builtin/credential/jwt/path_oidc.go Show resolved Hide resolved
@DrDaveD
Copy link
Contributor Author

DrDaveD commented May 19, 2024

Thanks for all the detailed comments. This is just to tell you that I won't be able to look at them closely this week, but should be able to the following week.

@DrDaveD
Copy link
Contributor Author

DrDaveD commented May 30, 2024

I was waiting to write the docs until I had interest shown that the PR would be accepted. I have updated the docs now but am waiting to commit until I do some more testing.

@DrDaveD DrDaveD marked this pull request as draft May 30, 2024 21:39
@DrDaveD
Copy link
Contributor Author

DrDaveD commented May 30, 2024

I decided to go ahead and push it as-is but mark the PR as draft for now until I am able to do further testing

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.

None yet

2 participants