Customizing HTTP headers in the config file by hghaf099 · Pull Request #12485 · hashicorp/vault

hghaf099 · 2021-09-02T23:38:40Z

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.

swayne275 · 2021-09-09T16:47:48Z

command/agent.go

-			logical.RespondError(w,
-				http.StatusPreconditionFailed,
-				errors.New(fmt.Sprintf("missing '%s' header", consts.RequestHeaderName)))
+			err := errors.New(fmt.Sprintf("missing '%s' header", consts.RequestHeaderName))


could be fmt.Errorf() since you're doing a format string

command/agent.go

finnigja

Nice to see this feature under development! Added a question re. the Cache-Control header that is currently set separately...

http/logical.go

mickael-hc

thanks for working on this @hghaf099 , this definitely be helpful to many organizations with stricter requirements. I've left some comments inline

internalshared/configutil/http_response_headers.go

http/logical.go

vault/custom_response_headers.go

mickael-hc · 2021-09-10T17:34:29Z

internalshared/configutil/http_response_headers.go

+    "5xx",
+}
+
+const (


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?

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.

internalshared/configutil/http_response_headers.go

changelog/12485.txt

ccapurso · 2021-10-04T18:20:18Z

http/sys_metrics.go

+		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)


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?

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.

internalshared/configutil/http_response_headers.go

raskchanky · 2021-10-05T19:23:06Z

internalshared/configutil/http_response_headers.go

+	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")


Same comment here as above re: type assertion and error message.

raskchanky · 2021-10-05T19:28:36Z

internalshared/configutil/http_response_headers.go

+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")


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.

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.

vault/core.go

ccapurso · 2021-10-06T20:45:52Z

http/handler.go

+	// Checking the validity of the status code
+	if status >= 600 || status < 100 {
+		return
+	}


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.

Sounds good! Thanks

vault/core.go

internalshared/configutil/http_response_headers.go

http/handler.go

command/server/config_custom_response_headers_test.go

http/custom_header_test.go

vault/core.go

vault/custom_response_headers.go

vault/custom_response_headers_test.go

vault/core.go

hghaf099 added 2 commits September 2, 2021 16:37

Customizing HTTP headers in the config file

5bf301c

Add changelog, fix bad imports

9dfa689

vercel bot temporarily deployed to Preview – vault-storybook September 3, 2021 00:04 Inactive

vercel bot temporarily deployed to Preview – vault September 3, 2021 00:04 Inactive

fixing some bugs

65f3d3a

vercel bot temporarily deployed to Preview – vault September 3, 2021 17:55 Inactive

vercel bot temporarily deployed to Preview – vault-storybook September 3, 2021 17:55 Inactive

fixing interaction of custom headers and /ui

61c12eb

vercel bot temporarily deployed to Preview – vault-storybook September 3, 2021 20:59 Inactive

vercel bot temporarily deployed to Preview – vault September 3, 2021 20:59 Inactive

Defining a member in core to set custom response headers

f4232cd

vercel bot temporarily deployed to Preview – vault-storybook September 5, 2021 00:31 Inactive

vercel bot temporarily deployed to Preview – vault September 5, 2021 00:31 Inactive

missing additional file

5ec2510

vercel bot temporarily deployed to Preview – vault-storybook September 5, 2021 00:34 Inactive

vercel bot temporarily deployed to Preview – vault September 5, 2021 00:34 Inactive

Some refactoring

6982b8a

vercel bot temporarily deployed to Preview – vault September 7, 2021 21:12 Inactive

vercel bot temporarily deployed to Preview – vault-storybook September 7, 2021 21:12 Inactive

hghaf099 requested a review from a team September 7, 2021 22:29

Adding automated tests for the feature

f804b26

vercel bot temporarily deployed to Preview – vault September 8, 2021 23:18 Inactive

vercel bot temporarily deployed to Preview – vault-storybook September 8, 2021 23:18 Inactive

Changing some error messages based on some recommendations

e52db45

vercel bot temporarily deployed to Preview – vault September 9, 2021 15:12 Inactive

vercel bot temporarily deployed to Preview – vault-storybook September 9, 2021 15:12 Inactive

swayne275 reviewed Sep 9, 2021

View reviewed changes

command/agent.go Outdated Show resolved Hide resolved

finnigja reviewed Sep 10, 2021

View reviewed changes

http/logical.go Show resolved Hide resolved

mickael-hc reviewed Sep 10, 2021

View reviewed changes

vercel bot temporarily deployed to Preview – vault-storybook September 22, 2021 00:14 Inactive

hghaf099 requested a review from ncabatoff September 22, 2021 04:45

ccapurso reviewed Oct 4, 2021

View reviewed changes

raskchanky reviewed Oct 5, 2021

View reviewed changes

Removing default custom headers, and renaming some function varibles

0964ef8

vercel bot temporarily deployed to Preview – vault October 5, 2021 23:19 Inactive

vercel bot temporarily deployed to Preview – vault-storybook October 5, 2021 23:19 Inactive

Merge branch 'main' into hghaf099-VAULT-3190-Parsing-Custom-HTTP-Headers

96cb6df

vercel bot deployed to Preview – vault-storybook October 5, 2021 23:20 View deployment

vercel bot deployed to Preview – vault October 5, 2021 23:20 View deployment

hghaf099 requested review from ccapurso and raskchanky October 6, 2021 13:39

ccapurso reviewed Oct 6, 2021

View reviewed changes

some refacotring

b6eedd1

vercel bot temporarily deployed to Preview – vault October 7, 2021 01:29 Inactive

vercel bot temporarily deployed to Preview – vault-storybook October 7, 2021 01:29 Inactive

hghaf099 requested review from ccapurso and mladlow October 7, 2021 01:29

ncabatoff reviewed Oct 8, 2021

View reviewed changes

hghaf099 requested a review from ncabatoff October 8, 2021 17:59

Refactoring and addressing comments

08d0157

vercel bot temporarily deployed to Preview – vault October 8, 2021 20:09 Inactive

vercel bot temporarily deployed to Preview – vault-storybook October 8, 2021 20:09 Inactive

ncabatoff reviewed Oct 12, 2021

View reviewed changes

vault/core.go Outdated Show resolved Hide resolved

removing a function and fixing comments

5734ab1

vercel bot temporarily deployed to Preview – vault-storybook October 12, 2021 17:30 Inactive

vercel bot temporarily deployed to Preview – vault October 12, 2021 17:30 Inactive

hghaf099 requested a review from ncabatoff October 12, 2021 17:45

ncabatoff approved these changes Oct 13, 2021

View reviewed changes

hghaf099 merged commit e0bfb73 into main Oct 13, 2021

+                  "5xx",
+              }
+              const (

Conversation

hghaf099 commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

finnigja left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mickael-hc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hghaf099 commented Sep 2, 2021 •

edited

Loading