-
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
Merged
Merged
Cors headers #2021
Changes from 89 commits
Commits
Show all changes
135 commits
Select commit
Hold shift + click to select a range
389434c
Added function to add CORS headers and handle preflight requests.
cdd2ca8
Added missing required headers to the Access-Control-Allow-Headers he…
09282a4
Merge branch 'master' of github.com:naunga/vault into cors-headers
237a9af
Added config options to enable CORS and set a regexp for origins that…
f368d09
Reverted to master revision after moving things into more appropriate…
889ae29
Added wrapper function to apply CORS headers when configured to do so.
8c268e1
Added fields to Core and CoreConfig structs to enable CORS if desired…
5852bb3
Standardized on allowed origins instead of domains.
336ba06
Added CORS configuration options to the tests and associated files.
1fe3605
Merge branch 'master' of github.com:naunga/vault into cors-headers
88da2da
Exported the HandleCORS function and moved the invokation into the se…
d18ed18
This was not the right place to apply the CORS handler. Removed.
0f5a7fa
Added tests for requests that require CORS.
fd5494b
Finalized how and where to apply the CORS headers.
6c54ded
Setting the Origin header in order to test CORS handling.
9b2f66c
Removed AllowCORS() func and replaced it with CORSInfo() func. This m…
6747af0
Added configuration for CORS where required.
23490f1
Merge branch 'master' of github.com:naunga/vault into cors-headers
092b94d
Merge branch 'master' of github.com:naunga/vault into cors-headers
a8cca06
Reverting changes. CORS config to be done via a /sys endpoint.
6d80c03
Reverting changes. CORS config to be done via a /sys endpoint.
7ca440a
Refactored to align with the way things are going to be handled.
71048cf
Refactored to place access to the CORS settings behind the barrier in…
9a4c9f1
Move the logic to add the headers for CORS onto to the cors configura…
b5e45ba
Merge branch 'cors-headers' of github.com:naunga/vault into cors-headers
1171697
Merge branch 'master' of github.com:naunga/vault into cors-headers
71ec898
Added cors command.
4584a87
Moved to logic determine when to the apply the CORS headers to the Ap…
367a04f
Made the error returned by CORSConfig() a const.
ca2487f
Lots of refactoring and clean up.
4f6b25d
Refactoring and DRYing up.
3643a10
Added tests for the CORS functionality.
a98bba2
Initial commit.
e57ac5e
Initial commit.
5cbc9fe
Merge branch 'master' into cors-headers
ceb493e
Added help text.
9957df9
Merge branch 'master' into cors-headers
7aa839c
Fixed a bunch of logic bugs that were causing the server to panic aft…
a974a2b
Removed debug code.
ef2a522
Fixed logic bug.
b22172e
Added code to ensure the server responds correctly when the CORS requ…
dd06686
Skip handling the logical request for preflights.
0ad8ac3
Flesh out tests for CORS handler
796f0ef
Refactored so that the core is created with an empty CORSConfig vs. h…
f1a9225
Refactored ApplyHeaders() to return a 403 if the origin is not valid …
338fdb1
Merge branch 'master' into cors-headers
ec20fbc
Fixed typo in comments.
582189d
Removed (presently) unnecessary test.
121e0ed
Added documentation for CORS functionality.
a247952
Merge branch 'master' into cors-headers
50a0c11
Added a new custom header that tells the Vault server to bypass the C…
e5ff73f
CORS handler now applies to all paths. Added logic to bypass the CORS…
799cc8c
Disable() func now sets the core's CORS config to disabled and clears…
673365a
handleCORSDisable was setting the core's corsConfig property to nil i…
02edf8e
Merge branch 'master' into cors-headers
7122b8c
Fixed failing tests.
7eeba1e
Fixed a typo in a comment.
d3a5e45
Merge branch 'master' into cors-headers
5c05c80
Vary header will now be returned.
d809e4e
Merge branch 'master' into cors-headers
3bba275
Refactored code and updated documents to reflect that the allowed ori…
fd735ba
Merge branch 'master' into cors-headers
7b22c2a
Merged master and resolved conflicts.
3dba363
Merge branch 'master' into cors-headers
ff022b6
Merge branch 'master' into cors-headers
edab784
Merge branch 'master' into cors-headers
919e916
Merge branch 'master' into cors-headers
1e40dc8
Merge branch 'master' of github.com:naunga/vault into cors-headers
0031687
Merge branch 'master' into cors-headers
18bb96b
Merge branch 'master' into cors-headers
13bf4ae
Removed unnecessary header.
543cebd
Remove cors CLI command.
027e546
Merged the handleCORS() func into wrapCORSHandler(). Was not necessar…
d838b8e
Removed unnecessary header.
9d5fb2a
Move check for preflight requests up to the CORS handler where it bel…
75955d5
Removed unneeded struct fields. Changed pointers to slices to just sl…
e27b992
Refactored help text and code to reflect the fact that CORSConfig.all…
1dbb7e1
Merge branch 'master' of github.com:naunga/vault into cors-headers
8c0a7c9
New config store for api-managable configurations.
8ed7fe8
Added Settings field to Config struct to allow for many types of conf…
580ca52
Refactored to use new Config structure.
0ab733d
Added configStore to Core. Wired up unseal and seal tasks.
7645af4
Simplified things a bit.
1b48377
Added code to update the ConfigStore.
cb2ccd7
Merge branch 'master' into cors-headers
5f16cf7
Fixed a typo that was making the test fail.
70b9a9b
Added missing response headers. Renamed allowedHeaders to responseHea…
f70c6dc
Refactored newCORSConfig() out.
47f52cb
Merge branch 'master' into cors-headers
b7be8c5
Merge branch 'master' into cors-headers
0859a85
Moved logic of when to apply the CORS headers here. This makes the lo…
0968686
Refactored CORS handler test to reflect the current state of things.
26af710
It is not necessary to set the isEnabled flag on the CORSConfig. This…
4ac6a0b
Creating an instance of a sync.RWMutex for the CORSConfig when the co…
e1afa97
Renamed responseHeaders to preflightHeaders. Added 'LIST' method to l…
4b5a32a
Renamed handleCORSEnable, Disable, and Status to handleCORSUpdate, De…
3127933
Changed docs to reflect the fact that the CORS Update and Delete func…
2bfa2e7
Merge branch 'master' of github.com:naunga/vault into cors-headers
c3447f4
Refactor test to reflect the fact that the CORS Update and Delete met…
014ab2e
Merge branch 'master' into cors-headers
10a5976
Merge branch 'master' into cors-headers
54ce27b
Merged handleCORSRead() and corsStatusResponse() into a single func.
6bc49a5
Moved logic to determine when to apply the CORS headers to the http h…
bb5d81d
Removed the config store.
9afc279
Gave the test a URL that is actually valid.
3b0a32d
Refactored after removing the ConfigStore.
d8a689a
Exported the Enabled and AllowedOrigins fields of the CORSConfig stru…
077dfad
Updated field names.
7f67976
Merge branch 'master' into cors-headers
3d94364
Merge branch 'master' of github.com:naunga/vault into cors-headers
46d451b
Merge branch 'master' into cors-headers
fca8f36
Moved the code to put the headers on the response here. Instead of ma…
468fda3
CORSConfig.Get() was removed. Updated to reflect that. Fixed a typo i…
2a5a5ef
Removed ApplyHeaders and Get(). Applied correct locks where needed. F…
f6f18b8
Was missing a return after writing the headers for a preflight reques…
1adc2d7
Merge branch 'master' into cors-headers
e7f4662
Merge branch 'master' into cors-headers
48dd63f
Merge branch 'master' into cors-headers
d26b641
Merge branch 'master' into cors-headers
e55caae
Merge branch 'master' into cors-headers
7da6630
Merge branch 'master' into cors-headers
54314e0
Merge branch 'master' into cors-headers
ec019db
Merge branch 'master' of github.com:naunga/vault into cors-headers
117326a
Merge branch 'master' of github.com:naunga/vault into cors-headers
607d2d5
Updated imports.
ed209aa
Made parameter to Enable a string slice.
bd48453
Fixed a typo.
1bcdf0f
Merge branch 'master' of github.com:naunga/vault into cors-headers
b5e9b84
Corrected parameter to func Enable.
4fbeadb
Added Access-Control-Max-Age header to list of expected headers.
8c9b4e2
Enable func returns an error if the list of URLs is empty. Reworded e…
79a87ea
Merge branch 'master' of github.com:naunga/vault into cors-headers
370585b
Merge branch 'master' of github.com:naunga/vault into cors-headers
3403c2d
Merge branch 'master' of github.com:naunga/vault into cors-headers
b7be115
Merge branch 'master' of github.com:naunga/vault into cors-headers
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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"` | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
package http | ||
|
||
import ( | ||
"net/http" | ||
|
||
"github.com/hashicorp/vault/logical" | ||
"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() | ||
statusCode := corsConf.ApplyHeaders(w, req) | ||
if statusCode != http.StatusOK { | ||
respondRaw(w, req, &logical.Response{ | ||
Data: map[string]interface{}{ | ||
logical.HTTPStatusCode: statusCode, | ||
logical.HTTPRawBody: []byte(""), | ||
}, | ||
}) | ||
return | ||
} | ||
|
||
// For pre-flight requests just send back the headers and return. | ||
if req.Method == http.MethodOptions { | ||
return | ||
} | ||
|
||
h.ServeHTTP(w, req) | ||
return | ||
}) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 :-)