NE-1141: Adds logic for setting and deleting headers via Ingress Operator CR and Route Object. #438
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -235,6 +235,22 @@ frontend public | |
| acl secure_redirect base,map_reg_int(/var/lib/haproxy/conf/os_route_http_redirect.map) -m bool | ||
| redirect scheme https if secure_redirect | ||
|
|
||
| {{- range $idx, $http_request_header := .HTTPRequestHeaders }} | ||
|
There was a problem hiding this comment. I don't really understand why we need this on There was a problem hiding this comment. Also, why don't we forbid adding "Proxy" header, which we explicitly delete in https://github.com/openshift/router/pull/438/files#diff-21bd64a4ad8a7927e9d59e99e09628efc884fe6d6291e50eb33c5deb3d2a79f0R221 ? We also make sure the "Host" is lower case, and we aren't disallowing the user to change that? There was a problem hiding this comment. or may be we can add this code just before https://github.com/openshift/router/pull/438/files#diff-21bd64a4ad8a7927e9d59e99e09628efc884fe6d6291e50eb33c5deb3d2a79f0R221 ? There was a problem hiding this comment. I believe you made changes elsewhere for this, and it would be helpful if you addressed the comments instead of leaving them open. There was a problem hiding this comment. I have addressed this in the "Risks and Mitigations" section of the EP openshift/enhancements#1296. |
||
| {{- if eq $http_request_header.Action "Set" }} | ||
| http-request set-header {{ $http_request_header.Name }} {{ $http_request_header.Value }} | ||
| {{- else if eq $http_request_header.Action "Delete" }} | ||
| http-request del-header {{ $http_request_header.Name }} | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| {{- range $idx, $http_response_header := .HTTPResponseHeaders }} | ||
| {{- if eq $http_response_header.Action "Set" }} | ||
| http-response set-header {{ $http_response_header.Name }} {{ $http_response_header.Value }} | ||
| {{- else if eq $http_response_header.Action "Delete" }} | ||
| http-response del-header {{ $http_response_header.Name }} | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| use_backend %[base,map_reg(/var/lib/haproxy/conf/os_http_be.map)] | ||
|
|
||
| default_backend openshift_default | ||
|
|
@@ -355,6 +371,22 @@ frontend fe_sni | |
| http-request set-header X-SSL-Client-DER %{+Q}[ssl_c_der,base64] | ||
| {{- end }} | ||
|
|
||
| {{- range $idx, $http_request_header := .HTTPRequestHeaders }} | ||
|
There was a problem hiding this comment. Again, there are so many headers we explicitly set here, why would we allow the user to change those? There was a problem hiding this comment. This section is set by the admin via ingress controller so I think this shall be his responsibility to set the headers correctly. There was a problem hiding this comment. other option is to add this code just before https://github.com/openshift/router/pull/438/files#diff-21bd64a4ad8a7927e9d59e99e09628efc884fe6d6291e50eb33c5deb3d2a79f0R354 |
||
| {{- if eq $http_request_header.Action "Set" }} | ||
| http-request set-header {{ $http_request_header.Name }} {{ $http_request_header.Value }} | ||
| {{- else if eq $http_request_header.Action "Delete" }} | ||
| http-request del-header {{ $http_request_header.Name }} | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| {{- range $idx, $http_response_header := .HTTPResponseHeaders }} | ||
| {{- if eq $http_response_header.Action "Set" }} | ||
| http-response set-header {{ $http_response_header.Name }} {{ $http_response_header.Value }} | ||
| {{- else if eq $http_response_header.Action "Delete" }} | ||
| http-response del-header {{ $http_response_header.Name }} | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| # map to backend | ||
| # Search from most specific to general path (host case). | ||
| # Note: If no match, haproxy uses the default_backend, no other | ||
|
|
@@ -439,6 +471,22 @@ frontend fe_no_sni | |
| http-request set-header X-SSL-Client-DER %{+Q}[ssl_c_der,base64] | ||
| {{- end }} | ||
|
|
||
| {{- range $idx, $http_request_header := .HTTPRequestHeaders }} | ||
| {{- if eq $http_request_header.Action "Set" }} | ||
| http-request set-header {{ $http_request_header.Name }} {{ $http_request_header.Value }} | ||
| {{- else if eq $http_request_header.Action "Delete" }} | ||
| http-request del-header {{ $http_request_header.Name }} | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| {{- range $idx, $http_response_header := .HTTPResponseHeaders }} | ||
| {{- if eq $http_response_header.Action "Set" }} | ||
| http-response set-header {{ $http_response_header.Name }} {{ $http_response_header.Value }} | ||
| {{- else if eq $http_response_header.Action "Delete" }} | ||
| http-response del-header {{ $http_response_header.Name }} | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| # map to backend | ||
| # Search from most specific to general path (host case). | ||
| # Note: If no match, haproxy uses the default_backend, no other | ||
|
|
@@ -624,6 +672,21 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }} | |
| {{- end }}{{/* hsts header */}} | ||
| {{- end }}{{/* is "edge" or "reencrypt" */}} | ||
|
|
||
| {{- range $idx, $http_request_header := $cfg.HTTPRequestHeaders }} | ||
|
There was a problem hiding this comment. There is a block above: There was a problem hiding this comment. @candita line already checks it.There was a problem hiding this comment. Lines 662 and 669 check it again, so it looks like a mistake that it isn't checked here. There was a problem hiding this comment. is the end of the block which allows the non-edge or non-re-encrypt, edge and re-encrypt route except passthrough. There was a problem hiding this comment. Ok, I see lines 662 and 669 are checking for edge, reencrypt, whereas line 528 checks for none, edge, reencrypt. |
||
| {{- if eq $http_request_header.Action "Set" }} | ||
| http-request set-header {{ $http_request_header.Name }} {{ $http_request_header.Value }} | ||
| {{- else if eq $http_request_header.Action "Delete" }} | ||
| http-request del-header {{ $http_request_header.Name }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- range $idx, $http_response_header := $cfg.HTTPResponseHeaders }} | ||
| {{- if eq $http_response_header.Action "Set" }} | ||
| http-response set-header {{ $http_response_header.Name }} {{ $http_response_header.Value }} | ||
| {{- else if eq $http_response_header.Action "Delete" }} | ||
| http-response del-header {{ $http_response_header.Name }} | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| {{- range $serviceUnitName, $weight := $cfg.ServiceUnitNames }} | ||
| {{- if ge $weight 0 }}{{/* weight=0 is reasonable to keep existing connections to backends with cookies as we can see the HTTP headers */}} | ||
| {{- with $serviceUnit := index $.ServiceUnits $serviceUnitName }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,6 +126,10 @@ type TemplateRouter struct { | |
| CaptureHTTPCookie *templateplugin.CaptureHTTPCookie | ||
| HTTPHeaderNameCaseAdjustmentsString string | ||
| HTTPHeaderNameCaseAdjustments []templateplugin.HTTPHeaderNameCaseAdjustment | ||
| HTTPResponseHeadersString string | ||
|
There was a problem hiding this comment. I'd prefer if there were different names for the ingress controller headers vs the route headers, especially when you need to handle these and overrides in the template. Could you rename the variables to have prefixes There was a problem hiding this comment. These fields in |
||
| HTTPResponseHeaders []templateplugin.HTTPHeader | ||
| HTTPRequestHeadersString string | ||
| HTTPRequestHeaders []templateplugin.HTTPHeader | ||
|
|
||
| TemplateRouterConfigManager | ||
| } | ||
|
|
@@ -182,6 +186,8 @@ func (o *TemplateRouter) Bind(flag *pflag.FlagSet) { | |
| flag.StringVar(&o.CaptureHTTPResponseHeadersString, "capture-http-response-headers", env("ROUTER_CAPTURE_HTTP_RESPONSE_HEADERS", ""), "A comma-delimited list of HTTP response header names and maximum header value lengths that should be captured for logging. Each item must have the following form: name:maxLength") | ||
| flag.StringVar(&o.CaptureHTTPCookieString, "capture-http-cookie", env("ROUTER_CAPTURE_HTTP_COOKIE", ""), "Name and maximum length of HTTP cookie that should be captured for logging. The argument must have the following form: name:maxLength. Append '=' to the name to indicate that an exact match should be performed; otherwise a prefix match will be performed. The value of first cookie that matches the name is captured.") | ||
| flag.StringVar(&o.HTTPHeaderNameCaseAdjustmentsString, "http-header-name-case-adjustments", env("ROUTER_H1_CASE_ADJUST", ""), "A comma-delimited list of HTTP header names that should have their case adjusted. Each item must be a valid HTTP header name and should have the desired capitalization.") | ||
| flag.StringVar(&o.HTTPResponseHeadersString, "set-delete-http-response-header", env("ROUTER_HTTP_RESPONSE_HEADERS", ""), "A comma-delimited list of HTTP response header names and values that should be set/deleted.") | ||
| flag.StringVar(&o.HTTPRequestHeadersString, "set-delete-http-request-header", env("ROUTER_HTTP_REQUEST_HEADERS", ""), "A comma-delimited list of HTTP request header names and values that should be set/deleted.") | ||
|
Comment on lines
+189
to
+190
There was a problem hiding this comment. I don't think these flags are discussed in the EP. These flags would only be applicable to the controller headers, right? There was a problem hiding this comment.
|
||
| } | ||
|
|
||
| type RouterStats struct { | ||
|
|
@@ -287,6 +293,106 @@ func parseCaptureHeaders(in string) ([]templateplugin.CaptureHTTPHeader, error) | |
| return captureHeaders, nil | ||
| } | ||
|
|
||
| func parseHeadersToBeSetOrDeleted(in string) ([]templateplugin.HTTPHeader, error) { | ||
| var captureHeaders []templateplugin.HTTPHeader | ||
| var capture templateplugin.HTTPHeader | ||
| var err error | ||
| if len(in) == 0 { | ||
| return captureHeaders, fmt.Errorf("encoded header string not present") | ||
| } | ||
| if len(in) > 0 { | ||
| for _, header := range strings.Split(in, ",") { | ||
| parts := strings.Split(header, ":") | ||
| num := len(parts) | ||
| switch num { | ||
| default: | ||
| return captureHeaders, fmt.Errorf("invalid HTTP header input specification: %v", header) | ||
|
There was a problem hiding this comment. You should have a unit test for anything that could produce an error. I think you need three more unit tests, as there is only one: There was a problem hiding this comment. You don't necessarily need to test all possible paths, but at least test for 1. token errors and 2. format errors that produce len(parts) < 2 and 3. error on decode. |
||
| case 3: | ||
| { | ||
| headerName, err := url.QueryUnescape(parts[0]) | ||
| if err != nil { | ||
| return captureHeaders, fmt.Errorf("failed to decode percent encoding: %v", parts[0]) | ||
| } | ||
| err = checkValidHeaderName(headerName) | ||
| if err != nil { | ||
| return captureHeaders, err | ||
| } | ||
| sanitizedHeaderName := templateplugin.SanitizeHeaderValue(headerName) | ||
| headerValue, err := url.QueryUnescape(parts[1]) | ||
| if err != nil { | ||
| return captureHeaders, fmt.Errorf("failed to decode percent encoding: %v", parts[1]) | ||
| } | ||
| sanitizedHeaderValue := templateplugin.SanitizeHeaderValue(headerValue) | ||
|
There was a problem hiding this comment. Why does the header value have to be sanitized? If it has a valid reason, why isn't the header name or action sanitized? There was a problem hiding this comment. Header name goes through a regex check so a user can't provide an invalid value. There was a problem hiding this comment. The SanitizeHeaderValue is escaping single quotes, which we allow in the header name per the regex. Why don't we allow single quotes in the header value? There was a problem hiding this comment.
|
||
| action, err := url.QueryUnescape(parts[2]) | ||
| if err != nil { | ||
| return captureHeaders, fmt.Errorf("failed to decode percent encoding: %v", parts[2]) | ||
| } | ||
| err = checkValidAction(action) | ||
| if err != nil { | ||
| return captureHeaders, err | ||
| } | ||
| capture = templateplugin.HTTPHeader{ | ||
| Name: sanitizedHeaderName, | ||
| Value: sanitizedHeaderValue, | ||
| Action: routev1.RouteHTTPHeaderActionType(action), | ||
|
There was a problem hiding this comment. Is this going to panic if There was a problem hiding this comment. i have written a function checkValidAction |
||
| } | ||
| captureHeaders = append(captureHeaders, capture) | ||
| } | ||
| case 2: | ||
| { | ||
| headerName, err := url.QueryUnescape(parts[0]) | ||
| if err != nil { | ||
| return captureHeaders, fmt.Errorf("failed to decode percent encoding: %v", parts[0]) | ||
| } | ||
| err = checkValidHeaderName(headerName) | ||
| if err != nil { | ||
| return captureHeaders, err | ||
| } | ||
| sanitizedHeaderName := templateplugin.SanitizeHeaderValue(headerName) | ||
| action, err := url.QueryUnescape(parts[1]) | ||
| if err != nil { | ||
| return captureHeaders, fmt.Errorf("failed to decode percent encoding: %v", parts[1]) | ||
| } | ||
| err = checkValidAction(action) | ||
| if err != nil { | ||
| return captureHeaders, err | ||
| } | ||
| capture = templateplugin.HTTPHeader{ | ||
| Name: sanitizedHeaderName, | ||
| Action: routev1.RouteHTTPHeaderActionType(action), | ||
| } | ||
| captureHeaders = append(captureHeaders, capture) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return captureHeaders, err | ||
| } | ||
|
|
||
| func checkValidAction(action string) error { | ||
| if action != string(routev1.Set) && action != string(routev1.Delete) { | ||
| return fmt.Errorf("invalid action: %s", action) | ||
| } else { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // permittedHeaderNameRE is a compiled regexp to match an HTTP header name | ||
| // as specified in RFC 2616, section 4.2. | ||
| // Any changes made to regex of header name in route type in openshift/api and `validation.go` file in library-go must be reflected here and | ||
| // vice versa. | ||
| var permittedHeaderNameRE = regexp.MustCompile("^[-!#$%&'*+.0-9A-Z^_`a-z|~]+$") | ||
|
|
||
| // checkValidHeaderName verifies that the given HTTP header name is valid. | ||
| func checkValidHeaderName(headerName string) error { | ||
| if !permittedHeaderNameRE.MatchString(headerName) { | ||
| return fmt.Errorf("invalid HTTP header name: %s", headerName) | ||
| } else { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| func parseCaptureCookie(in string) (*templateplugin.CaptureHTTPCookie, error) { | ||
| if len(in) == 0 { | ||
| return nil, nil | ||
|
|
@@ -393,6 +499,22 @@ func (o *TemplateRouterOptions) Complete() error { | |
| } | ||
| o.CaptureHTTPResponseHeaders = captureHTTPResponseHeaders | ||
|
|
||
| if len(o.HTTPResponseHeadersString) != 0 { | ||
| httpResponseHeaders, err := parseHeadersToBeSetOrDeleted(o.HTTPResponseHeadersString) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| o.HTTPResponseHeaders = httpResponseHeaders | ||
| } | ||
|
|
||
| if len(o.HTTPRequestHeadersString) != 0 { | ||
| httpRequestHeaders, err := parseHeadersToBeSetOrDeleted(o.HTTPRequestHeadersString) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| o.HTTPRequestHeaders = httpRequestHeaders | ||
| } | ||
|
|
||
| captureHTTPCookie, err := parseCaptureCookie(o.CaptureHTTPCookieString) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -648,6 +770,8 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error { | |
| CaptureHTTPResponseHeaders: o.CaptureHTTPResponseHeaders, | ||
| CaptureHTTPCookie: o.CaptureHTTPCookie, | ||
| HTTPHeaderNameCaseAdjustments: o.HTTPHeaderNameCaseAdjustments, | ||
| HTTPResponseHeaders: o.HTTPResponseHeaders, | ||
| HTTPRequestHeaders: o.HTTPRequestHeaders, | ||
| } | ||
|
|
||
| svcFetcher := templateplugin.NewListWatchServiceLookup(kc.CoreV1(), o.ResyncInterval, o.Namespace) | ||
|
|
||
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.
Why don't you set/delete the headers on
frontend public_ssl?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.
The connection is still encrypted in
public_ssl, so it is not possible to examine or modify the HTTP session there. Thefe_sniandfe_no_snifrontends terminate TLS.Here's my attempt at a diagram showing the transitions between cleartext HTTP and TLS: