-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Set allowed headers via API instead of defaulting to wildcard. #3023
Changes from 20 commits
728ba85
6c44bcb
bd4d006
1ee92b3
c4ca1e6
31770b6
8aad9e3
aeae188
89ef267
ce6d754
c1d629b
558e955
ea6deba
5d289ed
e8a68e8
704e7a3
158f690
5cd956d
dafb1f8
e29e68a
7006926
0f54b4d
0a05154
88c20e1
094e2a8
5abce39
6aa4591
a0b2fe5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
package http | ||
|
||
import ( | ||
"encoding/json" | ||
"reflect" | ||
"testing" | ||
|
||
"github.com/hashicorp/vault/vault" | ||
) | ||
|
||
func TestSysConfigCors(t *testing.T) { | ||
core, _, token := vault.TestCoreUnsealed(t) | ||
ln, addr := TestServer(t, core) | ||
defer ln.Close() | ||
TestServerAuth(t, addr, token) | ||
|
||
corsConf := core.CORSConfig() | ||
|
||
// Enable CORS | ||
resp := testHttpPut(t, token, addr+"/v1/sys/config/cors", map[string]interface{}{ | ||
"allowed_origins": addr, | ||
"allowed_headers": "X-Custom-Header", | ||
}) | ||
|
||
testResponseStatus(t, resp, 204) | ||
|
||
// Read the CORS configuration | ||
resp = testHttpGet(t, token, addr+"/v1/sys/config/cors") | ||
testResponseStatus(t, resp, 200) | ||
|
||
var actual map[string]interface{} | ||
var expected map[string]interface{} | ||
|
||
lenStdHeaders := len(corsConf.AllowedHeaders) | ||
|
||
expectedHeaders := make([]interface{}, lenStdHeaders) | ||
|
||
for i := range corsConf.AllowedHeaders { | ||
expectedHeaders[i] = corsConf.AllowedHeaders[i] | ||
} | ||
|
||
expected = map[string]interface{}{ | ||
"lease_id": "", | ||
"renewable": false, | ||
"lease_duration": json.Number("0"), | ||
"wrap_info": nil, | ||
"warnings": nil, | ||
"auth": nil, | ||
"data": map[string]interface{}{ | ||
"enabled": true, | ||
"allowed_origins": []interface{}{addr}, | ||
"allowed_headers": expectedHeaders, | ||
}, | ||
"enabled": true, | ||
"allowed_origins": []interface{}{addr}, | ||
"allowed_headers": expectedHeaders, | ||
} | ||
|
||
testResponseStatus(t, resp, 200) | ||
|
||
testResponseBody(t, resp, &actual) | ||
expected["request_id"] = actual["request_id"] | ||
|
||
if !reflect.DeepEqual(actual, expected) { | ||
t.Fatalf("bad: expected: %#v\nactual: %#v", expected, actual) | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,12 +15,23 @@ const ( | |
CORSEnabled | ||
) | ||
|
||
var stdAllowedHeaders = []string{ | ||
"Content-Type", | ||
"X-Requested-With", | ||
"X-Vault-AWS-IAM-Server-ID", | ||
"X-Vault-No-Request-Forwarding", | ||
"X-Vault-Token", | ||
"X-Vault-Wrap-Format", | ||
"X-Vault-Wrap-TTL", | ||
} | ||
|
||
// CORSConfig stores the state of the CORS configuration. | ||
type CORSConfig struct { | ||
sync.RWMutex `json:"-"` | ||
core *Core | ||
Enabled uint32 `json:"enabled"` | ||
AllowedOrigins []string `json:"allowed_origins,omitempty"` | ||
AllowedHeaders []string `json:"allowed_headers,omitempty"` | ||
} | ||
|
||
func (c *Core) saveCORSConfig() error { | ||
|
@@ -31,6 +42,7 @@ func (c *Core) saveCORSConfig() error { | |
} | ||
c.corsConfig.RLock() | ||
localConfig.AllowedOrigins = c.corsConfig.AllowedOrigins | ||
localConfig.AllowedHeaders = c.corsConfig.AllowedHeaders | ||
c.corsConfig.RUnlock() | ||
|
||
entry, err := logical.StorageEntryJSON("cors", localConfig) | ||
|
@@ -72,19 +84,29 @@ func (c *Core) loadCORSConfig() error { | |
|
||
// Enable takes either a '*' or a comma-seprated list of URLs that can make | ||
// cross-origin requests to Vault. | ||
func (c *CORSConfig) Enable(urls []string) error { | ||
func (c *CORSConfig) Enable(urls []string, headers []string) error { | ||
if len(urls) == 0 { | ||
return errors.New("the list of allowed origins cannot be empty") | ||
} | ||
|
||
if strutil.StrListContains(urls, "*") && len(urls) > 1 { | ||
urls = append(urls, "*") | ||
} else if strutil.StrListContains(urls, "*") { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than unlock and lock, just keep the lock here. |
||
if len(c.AllowedHeaders) == 0 { | ||
c.AllowedHeaders = append(c.AllowedHeaders, stdAllowedHeaders...) | ||
} | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Let me know if I'm missing something here... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
c.Unlock() | ||
|
||
atomic.StoreUint32(&c.Enabled, CORSEnabled) | ||
|
||
return c.core.saveCORSConfig() | ||
|
@@ -95,12 +117,17 @@ func (c *CORSConfig) IsEnabled() bool { | |
return atomic.LoadUint32(&c.Enabled) == CORSEnabled | ||
} | ||
|
||
// Disable sets CORS to disabled and clears the allowed origins | ||
// Disable sets CORS to disabled and clears the allowed origins & headers. | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing here, don't give up the lock and relock. |
||
c.Lock() | ||
c.AllowedHeaders = []string(nil) | ||
c.Unlock() | ||
|
||
return c.core.saveCORSConfig() | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.