make redirect uri validation case-insensitve #282
Conversation
|
thanks so much for tackling this so quickly! |
path_oidc.go
Outdated
| @@ -547,7 +547,7 @@ func validRedirect(uri string, allowed []string) bool { | |||
|
|
|||
| // if uri isn't a loopback, just string search the allowed list | |||
| if !strutil.StrListContains([]string{"localhost", "127.0.0.1", "::1"}, inputURI.Hostname()) { | |||
There was a problem hiding this comment.
I believe that loop back interface has a CIDR of /8 . I think you might want to remove the last octet from 127.0.0.1. In addition, localhost is not guaranteed to map back to a loopback interface address. You may want to add some comments to that effect.
path_oidc_test.go
Outdated
| `scope=openid`, | ||
| } | ||
|
|
||
| for _, test := range expected { |
There was a problem hiding this comment.
test used a variable name is a bit confusing. I wonder if we could rename it to testPat or something like that. Also, is there a specific reason why we couldn't have a single regex pattern for the full URL - like, is order of the params not deterministic?
There was a problem hiding this comment.
I agree about the name test. I will update that!
This was mostly copied from an existing test case. I tried reworking it to use a single regex pattern but it isn't obvious what match is failing and I couldn't get it to work. This way gives us a better error when the match fails.
path_oidc.go
Outdated
| @@ -537,6 +538,17 @@ func (b *jwtAuthBackend) verifyOIDCRequest(stateID string) *oidcRequest { | |||
| return nil | |||
| } | |||
|
|
|||
| func isLoopbackAddress(hostname string) bool { | |||
There was a problem hiding this comment.
Nice! One minor nit, I would maybe name this function isLocalAddr() or isLocalhost() since it also compares against a hostname.
| @@ -538,7 +538,7 @@ func (b *jwtAuthBackend) verifyOIDCRequest(stateID string) *oidcRequest { | |||
| return nil | |||
| } | |||
|
|
|||
| func isLoopbackAddress(hostname string) bool { | |||
| func isLocalAddr(hostname string) bool { | |||
Overview
This PR proposes to make the redirecut URI matching validation case-insensitive.
Background
An HVD user recently reported an issue where they could not log into the Vault UI using the OIDC auth method even when the URL was correct.
This was discovered when a customer had named their HVD cluster with capital letters (e.g. ABC-cluster).
When setting allowed_redirect_uris as a part of the OIDC setup process and then attempting to login to the Vault UI, the browser automatically transforms the login URL to all-lowercase, causing the URL to not match the configured allowed_redirect_uris.
Impact
This issue affects users whose clusters IDs have capital letters in them and are trying to login via OIDC in the Vault UI.
Tests
Confirmed that the new tests fail before the change was implemented