Customizing HTTP headers in the config file #12485
Conversation
hghaf099
commented
Sep 2, 2021
•
hghaf099
commented
- Setting default security headers: Content-Security-Policy, X-XSS-Protection, X-Frame-Options, X-Content-Type-Options, Strict-Transport-Security, Content-Type
- Parsing from config file: Parsing the configured custom HTTP response headers from the configuration file defined under "custom_response_headers" sub stanza. The structure of the sub-stanza is a map of status codes to a map of headers and headers valuse.
- Response headers for all endpoints: Returning custom HTTP headers for all endpoints
- Status code based response headers: Depending on a response status code, a set of headers is set. The most common headers are defined under the "default" status code. These are set regardless of the status code. Then, the headers defined under a collection status code are set (for example, headers defined under "2xx" are set for all 2 hundred status codes. Finally, the headers defined under the most specific status code are set.
- Reloading configuration: Making sure Reloading config would not hault the server and changes to the "custom_response_headers" sub stanza are reflected in the response headers.
- correctly interact with /ui endpoint. As we don’t want to allow setting headers in multiple places, if a custom header is configured in the /ui endpoint that is equal to the one configured in the configuration file, then, an error is returned stating the header already exists in the HCL configuration file. Suppose it happens that a user configures a header X using the /ui endpoint, where X is not present in the configuration file, then Vault is restarted with a modified configuration file where X is present. Then, an error is logged at startup stating that the header is configured both in the /ui endpoint and in the configuration file. However, the response header is set with the value of X set in the configuration file.
- Restricting some patterns: headers names with "X-Vault-" prefix are not allowed.
command/agent.go
Outdated
| logical.RespondError(w, | ||
| http.StatusPreconditionFailed, | ||
| errors.New(fmt.Sprintf("missing '%s' header", consts.RequestHeaderName))) | ||
| err := errors.New(fmt.Sprintf("missing '%s' header", consts.RequestHeaderName)) |
There was a problem hiding this comment.
could be fmt.Errorf() since you're doing a format string
There was a problem hiding this comment.
thanks for working on this @hghaf099 , this definitely be helpful to many organizations with stricter requirements. I've left some comments inline
| "5xx", | ||
| } | ||
|
|
||
| const ( |
There was a problem hiding this comment.
This is great - these defaults will allow us to update these over time. Do we offer the ability to unset a header value? Since we are specifying (sensible but arbitrary) defaults, perhaps end-users will want to strip headers?
There was a problem hiding this comment.
These headers cannot be unset for now, however, if an operator is not happy with the default values, they can simply configure their desired value in the configuration file. I will discuss the idea of providing a way to unset the defaults or even not set any defaults with our team to see what they think about it! Thanks for raising this up.
| status := resp.Data[logical.HTTPStatusCode].(int) | ||
| w.Header().Set("Content-Type", resp.Data[logical.HTTPContentType].(string)) | ||
| switch v := resp.Data[logical.HTTPRawBody].(type) { | ||
| case string: | ||
| w.WriteHeader(status) | ||
| w.Write([]byte(v)) | ||
| case []byte: | ||
| w.WriteHeader(status) |
There was a problem hiding this comment.
I might be missing something subtle but the proposed changes look functionally equivalent to the prior logic. A call to w.WriteHeader is being done in both the string and []byte cases. Is there a reason that the outer call to w.WriteHeader on line 38 cannot be kept as is?
There was a problem hiding this comment.
So, once the WriteHeader is called, setting other headers would be ignored. So, in this case, the "Content-Type" will be ignored which it seems like an old bug. Another pointer is that the responseError function also sets some headers and calls the WriteHeader. So, I made the change to accommodate the two cases here.
| for _, crh := range customResponseHeader { | ||
| for statusCode, responseHeader := range crh { | ||
| if _, ok := responseHeader.([]map[string]interface{}); !ok { | ||
| return nil, fmt.Errorf("response headers were not configured correctly. please make sure they're in a map") |
There was a problem hiding this comment.
Same comment here as above re: type assertion and error message.
| func parseHeaderValues(h interface{}) (string, error) { | ||
| var sl []string | ||
| if _, ok := h.([]interface{}); !ok { | ||
| return "", fmt.Errorf("headers must be given in a list of strings") |
There was a problem hiding this comment.
If this function should always be called with a list of strings, why not specify the type of the argument h as []string instead of interface{}? You'd be able to avoid a lot of type assertions in this function.
There was a problem hiding this comment.
This function is called from ParseCustomResponseHeaders which its input is an interface parsed using HCL config parser. So, the assertion should happen anyways either in this function or the functions down in the stack. This was the most relevant place for those.
http/handler.go
Outdated
| // Checking the validity of the status code | ||
| if status >= 600 || status < 100 { | ||
| return | ||
| } |
There was a problem hiding this comment.
We might up above the setter assignment. It will keep all the early return logic based on input validation close together making the function easier to read.
There was a problem hiding this comment.
Sounds good! Thanks
