Implement OAuth2 PKCE in SSORoleCredentialsProvider #1258
Conversation
Inspired by AWS CLI's recent change this PR adds support for OAuth2 "authorization code" grant flow accompanied by Proof Key for Code Exchange (PKCE) by OAuth Public Clients and makes it the default. In the "device code" flow the user is entirely responsible for matching the one time password displayed by their terminal and browser. This is error prone at best and a one click penalty for every login at worst. In the "authorization code" flow we create an ephemeral HTTP server to host an OAuth2 callback on 127.0.0.1. Users are redirected here after confirming their login attempt and the code comparison step is automated for them. As AWS enforces the use of 127.0.0.1 as a callback destination host a remote attacker who successfully convinces a user to confirm their malicious login prompt would not be able to access the resulting credential as the user would be redirected to their own localhost and not an attacker controlled destination. If we determine that we are not able to open a browser (either from the user specifying --stdout or observing that we are executing in an SSH session) we revert to the previous "device code" flow and print the URLs for copy/pasting into browsers.
| // generate the code challenge: base64(sha256(codeVerifier)) | ||
| codeChallengeBytes := sha256.Sum256([]byte(codeVerifier)) | ||
| codeChallenge := base64.RawURLEncoding.EncodeToString(codeChallengeBytes[:]) | ||
| log.Printf("Generated PKCE code_challenge: %q", codeChallenge) |
There was a problem hiding this comment.
I don't think this necessarily needs to be displayed by default
There was a problem hiding this comment.
I was confused at first also at the use of log.Printf in this project but these log lines are not displayed unless the user passes --debug as they're otherwise sent to io.Discard
Lines 166 to 168 in d4706c8
| // If we're in an SSH session, we can't use the browser for SSO so we print | ||
| // the URLs to stdout instead. | ||
| useStdout := config.SSOUseStdout | ||
| if os.Getenv("SSH_CONNECTION") != "" { |
There was a problem hiding this comment.
this seems like an addition that's independant from the PKCE flow? Overall I wouldn't necessarily expect this "magic" behavior from aws-vault, as a user I prefer a consistent behavior and explicitely passing --stdout if needs be
There was a problem hiding this comment.
Happy to back this out and have it be a separate discussion. My thinking was that given we now have login flows that cater to browser and non-browser setups to direct the user to the one that makes sense in context but I can understand how that would be confusing. A number of us at Tailscale use aws-vault with remote workspaces and run into the no-browser scenario frequently.
| // Code Exchange (PKCE) and open the browser automatically. The latter flow | ||
| // is more user-friendly and secure because the step comparing the challenge | ||
| // code between the CLI and browser is automated entirely. | ||
| if p.UseStdout { |
There was a problem hiding this comment.
I commonly use --stdout even when I'm on the same machine as where aws-vault is running, for various purposes (e.g. wanting to login in a non-default browser). I'm not 100% convinced that UseStdout should be the basis for chosing between the device code and PKCE flow, how about instead following the same behavior as the AWS CLI? It seems to have a flag --use-device-code flag aws/aws-cli@130005a#diff-e07a10a6eb1a677e905b0498651e672137b5dff19d357933bd7bc3e36f845a3bL42 which defaults to false
There was a problem hiding this comment.
Great shout. I hadn't considered the same device non-default browser path that users might wish to take. I was primarily thinking about remote workstations and in that context printing the URL out to be opened locally would not make sense as the eventual redirect destination would be inaccessible to the browser.
| log.Printf("Authorize URL: %s", authorizeURL.String()) | ||
|
|
||
| // redirect user to the authorize URL | ||
| log.Println("Opening SSO authorization page in browser") |
There was a problem hiding this comment.
(related to my first comment of this PR, I think this should rather honor the --stdout parameter to decide what to do)
|
|
||
| // await the authorization code from the callback server once the user has completed the flow. | ||
| var code string | ||
| timeout := time.After(1 * time.Minute) |
There was a problem hiding this comment.
FWIW the device code implementation here waits indefinitely (modulo a timeout on the AWS side), so perhaps it's worth increasing it?
|
|
||
| // send the authorization code to the channel | ||
| code := r.URL.Query().Get("code") | ||
| s.code <- code |
There was a problem hiding this comment.
perhaps it's worth returning early if there's an error rather than letting the consumer timeout? In which case s.code could be a channel of a struct that can hold either an error or a string, and the consumer would do something like
code, err := <- server.code
if err != nil {
// error
}
Inspired by AWS CLI's recent change this PR adds support for the OAuth2
"authorization code" grant flow accompanied by Proof Key for Code
Exchange (PKCE) by OAuth Public Clients and makes it the default.
In the "device code" flow the user is entirely responsible for matching
the one time password displayed by their terminal and browser. This is
error prone at best and a one click penalty for every login at worst.
In the "authorization code" flow we create an ephemeral HTTP server to
host an OAuth2 callback on 127.0.0.1. Users are redirected here after
confirming their login attempt and the code comparison step is automated
for them. As AWS enforces the use of 127.0.0.1 as a callback destination
host a remote attacker who successfully convinces a user to confirm
their malicious login prompt would not be able to access the resulting
credential as the user would be redirected to their own localhost and
not an attacker controlled destination.
If we determine that we are not able to open a browser (either from the
user specifying --stdout or observing that we are executing in an SSH
session) we revert to the previous "device code" flow and print the URLs
for copy/pasting into browsers.