-
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
Cors headers #2021
Cors headers #2021
Changes from 109 commits
389434c
cdd2ca8
09282a4
237a9af
f368d09
889ae29
8c268e1
5852bb3
336ba06
1fe3605
88da2da
d18ed18
0f5a7fa
fd5494b
6c54ded
9b2f66c
6747af0
23490f1
092b94d
a8cca06
6d80c03
7ca440a
71048cf
9a4c9f1
b5e45ba
1171697
71ec898
4584a87
367a04f
ca2487f
4f6b25d
3643a10
a98bba2
e57ac5e
5cbc9fe
ceb493e
9957df9
7aa839c
a974a2b
ef2a522
b22172e
dd06686
0ad8ac3
796f0ef
f1a9225
338fdb1
ec20fbc
582189d
121e0ed
a247952
50a0c11
e5ff73f
799cc8c
673365a
02edf8e
7122b8c
7eeba1e
d3a5e45
5c05c80
d809e4e
3bba275
fd735ba
7b22c2a
3dba363
ff022b6
edab784
919e916
1e40dc8
0031687
18bb96b
13bf4ae
543cebd
027e546
d838b8e
9d5fb2a
75955d5
e27b992
1dbb7e1
8c0a7c9
8ed7fe8
580ca52
0ab733d
7645af4
1b48377
cb2ccd7
5f16cf7
70b9a9b
f70c6dc
47f52cb
b7be8c5
0859a85
0968686
26af710
4ac6a0b
e1afa97
4b5a32a
3127933
2bfa2e7
c3447f4
014ab2e
10a5976
54ce27b
6bc49a5
bb5d81d
9afc279
3b0a32d
d8a689a
077dfad
7f67976
3d94364
46d451b
fca8f36
468fda3
2a5a5ef
f6f18b8
1adc2d7
e7f4662
48dd63f
d26b641
e55caae
7da6630
54314e0
ec019db
117326a
607d2d5
ed209aa
bd48453
1bcdf0f
b5e9b84
4fbeadb
8c9b4e2
79a87ea
370585b
3403c2d
b7be115
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,56 @@ | ||
package api | ||
|
||
func (c *Sys) CORSStatus() (*CORSResponse, error) { | ||
r := c.c.NewRequest("GET", "/v1/sys/config/cors") | ||
resp, err := c.c.RawRequest(r) | ||
if err != nil { | ||
return nil, err | ||
} | ||
defer resp.Body.Close() | ||
|
||
var result CORSResponse | ||
err = resp.DecodeJSON(&result) | ||
return &result, err | ||
} | ||
|
||
func (c *Sys) ConfigureCORS(req *CORSRequest) (*CORSResponse, error) { | ||
r := c.c.NewRequest("PUT", "/v1/sys/config/cors") | ||
if err := r.SetJSONBody(req); err != nil { | ||
return nil, err | ||
} | ||
|
||
resp, err := c.c.RawRequest(r) | ||
if err != nil { | ||
return nil, err | ||
} | ||
defer resp.Body.Close() | ||
|
||
var result CORSResponse | ||
err = resp.DecodeJSON(&result) | ||
return &result, err | ||
} | ||
|
||
func (c *Sys) DisableCORS() (*CORSResponse, error) { | ||
r := c.c.NewRequest("DELETE", "/v1/sys/config/cors") | ||
|
||
resp, err := c.c.RawRequest(r) | ||
if err != nil { | ||
return nil, err | ||
} | ||
defer resp.Body.Close() | ||
|
||
var result CORSResponse | ||
err = resp.DecodeJSON(&result) | ||
return &result, err | ||
|
||
} | ||
|
||
type CORSRequest struct { | ||
AllowedOrigins string `json:"allowed_origins"` | ||
Enabled bool `json:"enabled"` | ||
} | ||
|
||
type CORSResponse struct { | ||
AllowedOrigins string `json:"allowed_origins"` | ||
Enabled bool `json:"enabled"` | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
package http | ||
|
||
import ( | ||
"net/http" | ||
|
||
"github.com/hashicorp/vault/vault" | ||
) | ||
|
||
func wrapCORSHandler(h http.Handler, core *vault.Core) http.Handler { | ||
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { | ||
corsConf := core.CORSConfig() | ||
|
||
origin := req.Header.Get("Origin") | ||
requestMethod := req.Header.Get("Access-Control-Request-Method") | ||
|
||
// If CORS is not enabled or if no Origin header is present (i.e. the request | ||
// is from the Vault CLI. A browser will always send an Origin header), then | ||
// just return a 204. | ||
if !corsConf.IsEnabled() || origin == "" { | ||
h.ServeHTTP(w, req) | ||
return | ||
} | ||
|
||
// Return a 403 if the origin is not | ||
// allowed to make cross-origin requests. | ||
if !corsConf.IsValidOrigin(origin) { | ||
w.WriteHeader(http.StatusForbidden) | ||
return | ||
} | ||
|
||
if req.Method == http.MethodOptions && !corsConf.IsValidMethod(requestMethod) { | ||
w.WriteHeader(http.StatusMethodNotAllowed) | ||
return | ||
} | ||
|
||
corsConf.ApplyHeaders(w, req) | ||
|
||
h.ServeHTTP(w, req) | ||
return | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,86 @@ import ( | |
"github.com/hashicorp/vault/vault" | ||
) | ||
|
||
func TestHandler_cors(t *testing.T) { | ||
core, _, _ := vault.TestCoreUnsealed(t) | ||
ln, addr := TestServer(t, core) | ||
defer ln.Close() | ||
|
||
// Enable CORS and allow from any origin for testing. | ||
corsConfig := core.CORSConfig() | ||
err := corsConfig.Enable(addr) | ||
if err != nil { | ||
t.Fatalf("Error enabling CORS: %s", err) | ||
} | ||
|
||
req, err := http.NewRequest(http.MethodOptions, addr+"/v1/sys/seal-status", nil) | ||
if err != nil { | ||
t.Fatalf("err: %s", err) | ||
} | ||
req.Header.Set("Origin", "BAD ORIGIN") | ||
|
||
// Requests from unacceptable origins will be rejected with a 403. | ||
client := cleanhttp.DefaultClient() | ||
resp, err := client.Do(req) | ||
if err != nil { | ||
t.Fatalf("err: %s", err) | ||
} | ||
|
||
if resp.StatusCode != http.StatusForbidden { | ||
t.Fatalf("Bad status:\nexpected: 403 Forbidden\nactual: %s", resp.Status) | ||
} | ||
|
||
// | ||
// Test preflight requests | ||
// | ||
|
||
// Set a valid origin | ||
req.Header.Set("Origin", addr) | ||
|
||
// Server should NOT accept arbitrary methods. | ||
req.Header.Set("Access-Control-Request-Method", "FOO") | ||
|
||
client = cleanhttp.DefaultClient() | ||
resp, err = client.Do(req) | ||
if err != nil { | ||
t.Fatalf("err: %s", err) | ||
} | ||
|
||
// Fail if an arbitrary method is accepted. | ||
if resp.StatusCode != http.StatusMethodNotAllowed { | ||
t.Fatalf("Bad status:\nexpected: 405 Method Not Allowed\nactual: %s", resp.Status) | ||
} | ||
|
||
// Server SHOULD accept acceptable methods. | ||
req.Header.Set("Access-Control-Request-Method", http.MethodPost) | ||
|
||
client = cleanhttp.DefaultClient() | ||
resp, err = client.Do(req) | ||
if err != nil { | ||
t.Fatalf("err: %s", err) | ||
} | ||
|
||
// | ||
// Test that the CORS headers are applied correctly. | ||
// | ||
expHeaders := map[string]string{ | ||
"Access-Control-Allow-Origin": addr, | ||
"Access-Control-Allow-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. Can we also add 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. We can indeed. |
||
"Vary": "Origin", | ||
} | ||
|
||
for expHeader, expected := range expHeaders { | ||
actual := resp.Header.Get(expHeader) | ||
if actual == "" { | ||
t.Fatalf("bad:\nHeader: %#v was not on response.", expHeader) | ||
} | ||
|
||
if actual != expected { | ||
t.Fatalf("bad:\nExpected: %#v\nActual: %#v\n", expected, actual) | ||
} | ||
} | ||
} | ||
|
||
func TestHandler_CacheControlNoStore(t *testing.T) { | ||
core, _, token := vault.TestCoreUnsealed(t) | ||
ln, addr := TestServer(t, core) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -308,6 +308,9 @@ type Core struct { | |
// The grpc forwarding client | ||
rpcForwardingClient RequestForwardingClient | ||
|
||
// CORS Information | ||
corsConfig *CORSConfig | ||
|
||
// replicationState keeps the current replication state cached for quick | ||
// lookup | ||
replicationState logical.ReplicationState | ||
|
@@ -430,6 +433,7 @@ func NewCore(conf *CoreConfig) (*Core, error) { | |
clusterCertPool: x509.NewCertPool(), | ||
clusterListenerShutdownCh: make(chan struct{}), | ||
clusterListenerShutdownSuccessCh: make(chan struct{}), | ||
corsConfig: &CORSConfig{}, | ||
} | ||
|
||
// Wrap the backend in a cache unless disabled | ||
|
@@ -506,6 +510,11 @@ func (c *Core) Shutdown() error { | |
return c.sealInternal() | ||
} | ||
|
||
// CORSConfig returns the current CORS configuration | ||
func (c *Core) CORSConfig() *CORSConfig { | ||
return c.corsConfig.Get() | ||
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. Can we change it to |
||
} | ||
|
||
// LookupToken returns the properties of the token from the token store. This | ||
// is particularly useful to fetch the accessor of the client token and get it | ||
// populated in the logical request along with the client token. The accessor | ||
|
@@ -1205,6 +1214,9 @@ func (c *Core) postUnseal() (retErr error) { | |
if err := c.setupPolicyStore(); err != nil { | ||
return err | ||
} | ||
if err := c.loadCORSConfig(); err != nil { | ||
return err | ||
} | ||
if err := c.loadCredentials(); err != nil { | ||
return err | ||
} | ||
|
@@ -1267,6 +1279,9 @@ func (c *Core) preSeal() error { | |
if err := c.teardownPolicyStore(); err != nil { | ||
result = multierror.Append(result, errwrap.Wrapf("error tearing down policy store: {{err}}", err)) | ||
} | ||
if err := c.saveCORSConfig(); err != nil { | ||
result = multierror.Append(result, errwrap.Wrapf("error tearing down config store: {{err}}", err)) | ||
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. I would have the log say |
||
} | ||
if err := c.stopRollback(); err != nil { | ||
result = multierror.Append(result, errwrap.Wrapf("error stopping rollback: {{err}}", err)) | ||
} | ||
|
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 is in the hot path since it's called on every request, so we should first check whether cors is even enabled, and immediately chain to the next handler if not. We don't really need to acquire a lock here because if cors is being toggled and requests are flying in, we don't have guaranteed ordering/timing anyways.
If cors is enabled, do we actually need to send those headers back on every request, or just with OPTIONS?
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.
I'll take a look at the code. It should be sending headers appropriate for the request that was made.
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.
I don't really know which ones are appropriate for which requests, I just had the impression that you used OPTIONS to get the CORS headers, so if they're not needed for other requests we should just skip all of the code to reduce slowdown.
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.
If you look at
cors.ApplyHeaders()
that is exactly what we do. If CORS isn't enable, then we just return and go on to the next handler in the chain. I do see that it might be helpful to move the logic to determine if CORS in enabled up towrapCORSHandler
just to make the code more readable.At the very least
Access-Control-Allow-Origin
needs to be returned with every cross-origin request.Access-Control-Allow-Origin
,Access-Control-Allow-Methods
,Access-Control-Allow-Headers
, andAccess-Control-Max-Age
need to be sent back on the response for OPTIONS (preflight) requests, and that's what I'm doing.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.
@jefferai is response to your previous comment: OPTIONS (or preflights) requests are made by the browser when a request is made with a method other than
GET
,HEAD
, orPOST
and the content type istext/plain
,application/x-www-form-urlencoded
ormultipart/form-data
.The preflight request is sent to the API by the browser to determine if the actual request is acceptable to the API. For example when a browser wants to make a
LIST
request the browser will make a preflight request to see if Vault will accept theLIST
method.The preflight response will include the
Access-Control-Max-Age
header, which sets the length of time that the preflight response should be valid so a preflight won't sent before every request.Every browser does it a bit differently: Firefox will allow you to set the max age to 24 hours, but Chrome will only allow you to set it to a max of 10 minutes.
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.
But only for those three content types you listed, none of which we use?
As you can tell, I'm rather confused. :-)
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.
@jefferai CORS is a confusing subject. I've learned more about it than I ever wanted, since I started working on this PR. Let's see if I can help alleviate your confusion.
The browser will not send a preflight request to Vault (and any cross-origin resource) for the following requests:
GET
HEAD
POST
if the content-type of thePOST
request istext/plain
,application/x-www-form-urlencoded
, ormultipart/form-data
.For anything else the browser will send a preflight request to Vault (and any cross-origin resource).
Regardless of the browser's need for a preflight request the browser will expect an
Access-Control-Allow-Origin
header on the response from Vault and any cross-origin resource.This is a pretty good explanation for CORS: http://restlet.com/blog/2015/12/15/understanding-and-using-cors/
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.
Just want to comment as an interested party on the work being done here.
All of this talk of which verb or what content type is a moot point.
Because Vault requires all but the authentication requests to have the
X-Vault-Token
header, every request will be pre-flighted, no matter what.This section of the Mozilla CORS page explains it in detail, but basically if you have any non-standard headers (among other things), the request is preflighted.
Note that the
Authorization
header does not appear in any of the whitelisted headers lists, either (in case you were thinking you could circumvent the whole CORS issue ;) ). So, unless there's a plan to make Vault stateful using theCookie
header - a bad idea - supporting CORS will be required for any useful HTTP access.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.
@rockerest yes, exactly this! Wish I'd recalled that bit about the non-standard headers. Probably would've saved a lot of confusion.
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.
That doesn't really change the thread here: I was worried about the CORS check being done first to get it out of the hot path, and @naunga indicated that it's being done early in the function but could be moved out of it to make it more clear/explicit. I think that conversation is still valid :-)