tests: Harden OAuth device, PKCE, and refresh-token flows#9732
tests: Harden OAuth device, PKCE, and refresh-token flows#9732nishantmonu51 wants to merge 2 commits into
Conversation
nishantmonu51
changed the title
| scope := strings.TrimSpace(values.Get("scope")) | ||
| if scope == "" { | ||
| http.Error(w, "scope is required", http.StatusBadRequest) | ||
| return | ||
| } | ||
| scopes := strings.Fields(scope) |
There was a problem hiding this comment.
nit: I believe Fields trims leading/trailing whitespace, so no need for the separate TrimSpace
| "go.uber.org/zap" | ||
| ) | ||
|
|
||
| const maxOAuthRegistrationBodyBytes = 1 << 20 |
There was a problem hiding this comment.
Consider adding // 1 MB comment
| // Bound this public endpoint before decoding so a registration request cannot | ||
| // consume an arbitrary amount of server memory. | ||
| body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, maxOAuthRegistrationBodyBytes)) | ||
| if err != nil { | ||
| var maxBytesErr *http.MaxBytesError | ||
| if errors.As(err, &maxBytesErr) { | ||
| http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) | ||
| return | ||
| } |
There was a problem hiding this comment.
Any particular reason to enforce the body limit for this particular endpoint? Consider adding a middleware on all auth HTTP endpoints instead.
Also, isn't 1 MB a really large body for this endpoint? Would just have expected a few KB max.
| if !hasGrantType(authClient.GrantTypes, authorizationCodeGrantType) { | ||
| http.Error(w, "client is not permitted to use authorization codes", http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
Worth double checking that all clients (including old ones) send this to ensure backwards compatible logins
| // ValidateAuthToken has a short cache, so enforce expiry again at the | ||
| // rotation boundary in case the token expired after it was cached. | ||
| if userToken.ExpiresOn != nil && !userToken.ExpiresOn.After(time.Now()) { | ||
| http.Error(w, "invalid refresh token", http.StatusUnauthorized) | ||
| return | ||
| } |
There was a problem hiding this comment.
Shouldn't this be done inside ValidateAuthToken instead of here so it's applied for all callers?
| internalServerError(w, fmt.Errorf("failed to commit refresh token rotation, %w", err)) | ||
| return | ||
| } | ||
| a.admin.PurgeAuthTokenCache() |
There was a problem hiding this comment.
I think this will purge all cached tokens, not just the one that was refreshed.
Might be better to only remove the refreshed one? Or perhaps just keep it simple and let the old token stay valid until the 10 second cache TTL expires? (Since it's an in-memory cache, other nodes may anyway still have it cached, so there's no security win by purging early.)
| // Deleting inside the transaction atomically claims this token. Concurrent | ||
| // rotations can both validate it, but only one can delete and commit it. | ||
| err = a.admin.DB.DeleteUserAuthToken(txCtx, userToken.ID) | ||
| if err != nil { | ||
| if errors.Is(err, database.ErrNotFound) { | ||
| http.Error(w, "invalid refresh token", http.StatusUnauthorized) |
There was a problem hiding this comment.
Not sure, but seems like the new logic has more error cases to handle than the previous logic. Have you checked the frontend and CLI handle these cases?
| "client_id": []string{d.ClientID}, | ||
| "scope": []string{"full_account"}, | ||
| "redirect": []string{url.QueryEscape(redirectURL)}, | ||
| "redirect": []string{redirectURL}, |
There was a problem hiding this comment.
Not sure why we had url.QueryEscape on this previously, but did you confirm it's no longer needed in any cases?
| // Poll only after the server-provided interval. The wait is capped at the | ||
| // device-code expiry so a long interval cannot keep login alive past it. |
There was a problem hiding this comment.
The new retry logic seems overly complex to me (now requiring both a separate enum type and a helper function for a simple poll loop). Consider if the complexity is necessary or if it can be simplified
| func (a *Authenticator) ExchangeCodeForToken(code string) (string, error) { | ||
| return a.ExchangeCodeForTokenContext(context.Background(), code) | ||
| } | ||
|
|
||
| // ExchangeCodeForTokenContext lets a caller tie the token request to its callback | ||
| // lifecycle, so cancelling the callback also cancels an in-flight exchange. | ||
| func (a *Authenticator) ExchangeCodeForTokenContext(ctx context.Context, code string) (string, error) { |
There was a problem hiding this comment.
Consider making ExchangeCodeForToken accept a ctx instead of creating a separate function (ExchangeCodeForToken is anyway only used one place, so easy to refactor).
2 participants
Harden OAuth protocol boundaries across the admin server and CLI clients.
Validation:
go test -short -race -count=1 -timeout=15m ./admin ./admin/server/auth ./cli/pkg/deviceauth ./cli/pkg/pkceChecklist: