Set allowed headers via API instead of defaulting to wildcard. #3023
Conversation
vault/cors.go
Outdated
|
|
||
| if strutil.StrListContains(urls, "*") && len(urls) > 1 { | ||
| urls = append(urls, "*") | ||
| } else if strutil.StrListContains(urls, "*") { |
There was a problem hiding this comment.
This new logic seems suspect -- the error message says that to allow all origins * must be the only value, but the logic makes the empty set automatically get * and if you were to only have * as your only supplied url you'd trigger the error since the length > 1 check got removed.
vault/cors.go
Outdated
| return errors.New("to allow all origins the '*' must be the only value for allowed_origins") | ||
| } | ||
|
|
||
| c.Lock() | ||
| c.AllowedOrigins = urls | ||
| c.Unlock() | ||
|
|
||
| c.Lock() |
There was a problem hiding this comment.
Rather than unlock and lock, just keep the lock here.
| // Allow the user to add additional headers to the list of | ||
| // headers allowed on cross-origin requests. | ||
| if len(headers) > 0 { | ||
| c.AllowedHeaders = append(c.AllowedHeaders, headers...) |
There was a problem hiding this comment.
Is there ever a chance that Enable will be called multiple times? It seems like the right thing here would be to start fresh every time since this function is (presuambly) being given the canonical set of allowed headers. So rather than the check above and the check here, simply do:
c.AllowedHeaders = stdAllowedHeaders
if len(headers) > 0 {
c.AllowedHeaders = append(c.AllowedHeaders, headers...)
}
Let me know if I'm missing something here...
There was a problem hiding this comment.
Enable certainly could be called multiple times, because this code allows new headers to be appended to the existing set. This workflow, however, clashes with the workflow for setting origins. Your suggestion is correct.
vault/cors.go
Outdated
| func (c *CORSConfig) Disable() error { | ||
| atomic.StoreUint32(&c.Enabled, CORSDisabled) | ||
| c.Lock() | ||
| c.AllowedOrigins = []string(nil) | ||
| c.Unlock() | ||
|
|
There was a problem hiding this comment.
Same thing here, don't give up the lock and relock.
|
Made a couple of minor adjustments. Looks good! Thanks! |
* oss/master: (55 commits) update dr replication docs with the promotion response (#3124) Make travis_wait Travis wait longer_wait changelog++ Set allowed headers via API instead of defaulting to wildcard. (#3023) Fix formatting in mfa docs (#3122) Fix minor typo (#3120) Update go-plugin to include go-hclog support Unlock the statelock on unsuccessful sealInitCommon Remove a couple unneeded cancels Make seal/stepdown functions async internally so they can poke the request context Update mock-plugin (#3107) Fix minor grammatical error (#3110) docs: MFA API (#3109) Cut version 0.8.0-rc1 Update version Migrate physical backends into separate packages (#3106) changeling ++ changelog++ changelog++ credsutil: Include hyphen as part of reqStr (#3037) ...
This addresses issue #2982.
Setting the Access-Control-Allow-Headers to a wildcard, which was the default in Vault's CORS implementation, is an open spec and not merged is all browsers.
Things have been refactored to be a list of standard headers that Vault uses by default and users can append additional headers via the API.