From 23504db7708a5829ae918166d08dce8b25c881a3 Mon Sep 17 00:00:00 2001 From: Moritz Johner Date: Sun, 7 Jul 2019 18:34:56 +0200 Subject: [PATCH] feat: auth-req caching add a way to configure the `proxy_cache_*` [1] directive for external-auth. The user-defined cache_key may contain sensitive information (e.g. Authorization header). We want to store *only* a hash of that key, not the key itself on disk. [1] http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_cache_key Signed-off-by: Moritz Johner --- .../nginx-configuration/annotations.md | 38 ++-- .../nginx-configuration/configmap.md | 20 +- internal/ingress/annotations/authreq/main.go | 109 ++++++++-- .../ingress/annotations/authreq/main_test.go | 104 ++++++++- internal/ingress/controller/config/config.go | 16 +- .../ingress/controller/template/configmap.go | 19 ++ .../controller/template/configmap_test.go | 23 ++ rootfs/etc/nginx/template/nginx.tmpl | 24 +++ test/e2e/annotations/auth.go | 199 ++++++++++++++++++ test/e2e/framework/deployment.go | 13 ++ test/e2e/framework/k8s.go | 15 ++ test/e2e/framework/util.go | 8 + test/e2e/settings/global_external_auth.go | 47 +++++ 13 files changed, 583 insertions(+), 52 deletions(-) diff --git a/docs/user-guide/nginx-configuration/annotations.md b/docs/user-guide/nginx-configuration/annotations.md index 824b0512a5..e7b104610e 100755 --- a/docs/user-guide/nginx-configuration/annotations.md +++ b/docs/user-guide/nginx-configuration/annotations.md @@ -26,6 +26,8 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz |[nginx.ingress.kubernetes.io/auth-tls-error-page](#client-certificate-authentication)|string| |[nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream](#client-certificate-authentication)|"true" or "false"| |[nginx.ingress.kubernetes.io/auth-url](#external-authentication)|string| +|[nginx.ingress.kubernetes.io/auth-cache-key](#external-authentication)|string| +|[nginx.ingress.kubernetes.io/auth-cache-duration](#external-authentication)|string| |[nginx.ingress.kubernetes.io/auth-snippet](#external-authentication)|string| |[nginx.ingress.kubernetes.io/enable-global-auth](#external-authentication)|"true" or "false"| |[nginx.ingress.kubernetes.io/backend-protocol](#backend-protocol)|string|HTTP,HTTPS,GRPC,GRPCS,AJP| @@ -113,18 +115,18 @@ In some cases, you may want to "canary" a new set of changes by sending a small * `nginx.ingress.kubernetes.io/canary-by-header-value`: The header value to match for notifying the Ingress to route the request to the service specified in the Canary Ingress. When the request header is set to this value, it will be routed to the canary. For any other header value, the header will be ignored and the request compared against the other canary rules by precedence. This annotation has to be used together with . The annotation is an extension of the `nginx.ingress.kubernetes.io/canary-by-header` to allow customizing the header value instead of using hardcoded values. It doesn't have any effect if the `nginx.ingress.kubernetes.io/canary-by-header` annotation is not defined. -* `nginx.ingress.kubernetes.io/canary-by-cookie`: The cookie to use for notifying the Ingress to route the request to the service specified in the Canary Ingress. When the cookie value is set to `always`, it will be routed to the canary. When the cookie is set to `never`, it will never be routed to the canary. For any other value, the cookie will be ignored and the request compared against the other canary rules by precedence. +* `nginx.ingress.kubernetes.io/canary-by-cookie`: The cookie to use for notifying the Ingress to route the request to the service specified in the Canary Ingress. When the cookie value is set to `always`, it will be routed to the canary. When the cookie is set to `never`, it will never be routed to the canary. For any other value, the cookie will be ignored and the request compared against the other canary rules by precedence. -* `nginx.ingress.kubernetes.io/canary-weight`: The integer based (0 - 100) percent of random requests that should be routed to the service specified in the canary Ingress. A weight of 0 implies that no requests will be sent to the service in the Canary ingress by this canary rule. A weight of 100 means implies all requests will be sent to the alternative service specified in the Ingress. +* `nginx.ingress.kubernetes.io/canary-weight`: The integer based (0 - 100) percent of random requests that should be routed to the service specified in the canary Ingress. A weight of 0 implies that no requests will be sent to the service in the Canary ingress by this canary rule. A weight of 100 means implies all requests will be sent to the alternative service specified in the Ingress. -Canary rules are evaluated in order of precedence. Precedence is as follows: -`canary-by-header -> canary-by-cookie -> canary-weight` +Canary rules are evaluated in order of precedence. Precedence is as follows: +`canary-by-header -> canary-by-cookie -> canary-weight` **Note** that when you mark an ingress as canary, then all the other non-canary annotations will be ignored (inherited from the corresponding main ingress) except `nginx.ingress.kubernetes.io/load-balance` and `nginx.ingress.kubernetes.io/upstream-hash-by`. **Known Limitations** -Currently a maximum of one canary ingress can be applied per Ingress rule. +Currently a maximum of one canary ingress can be applied per Ingress rule. ### Rewrite @@ -142,7 +144,7 @@ The annotation `nginx.ingress.kubernetes.io/affinity` enables and sets the affin The only affinity type available for NGINX is `cookie`. !!! attention - If more than one Ingress is defined for a host and at least one Ingress uses `nginx.ingress.kubernetes.io/affinity: cookie`, then only paths on the Ingress using `nginx.ingress.kubernetes.io/affinity` will use session cookie affinity. All paths defined on other Ingresses for the host will be load balanced through the random selection of a backend server. + If more than one Ingress is defined for a host and at least one Ingress uses `nginx.ingress.kubernetes.io/affinity: cookie`, then only paths on the Ingress using `nginx.ingress.kubernetes.io/affinity` will use session cookie affinity. All paths defined on other Ingresses for the host will be load balanced through the random selection of a backend server. !!! example Please check the [affinity](../../examples/affinity/cookie/README.md) example. @@ -151,7 +153,7 @@ The only affinity type available for NGINX is `cookie`. If you use the ``cookie`` affinity type you can also specify the name of the cookie that will be used to route the requests with the annotation `nginx.ingress.kubernetes.io/session-cookie-name`. The default is to create a cookie named 'INGRESSCOOKIE'. -The NGINX annotation `nginx.ingress.kubernetes.io/session-cookie-path` defines the path that will be set on the cookie. This is optional unless the annotation `nginx.ingress.kubernetes.io/use-regex` is set to true; Session cookie paths do not support regex. +The NGINX annotation `nginx.ingress.kubernetes.io/session-cookie-path` defines the path that will be set on the cookie. This is optional unless the annotation `nginx.ingress.kubernetes.io/use-regex` is set to true; Session cookie paths do not support regex. ### Authentication @@ -294,7 +296,7 @@ CORS can be controlled with the following annotations: Example: `nginx.ingress.kubernetes.io/cors-max-age: 600` !!! note - For more information please see [https://enable-cors.org](https://enable-cors.org/server_nginx.html) + For more information please see [https://enable-cors.org](https://enable-cors.org/server_nginx.html) ### HTTP2 Push Preload. @@ -327,11 +329,11 @@ metadata: annotations: nginx.ingress.kubernetes.io/server-snippet: | set $agentflag 0; - + if ($http_user_agent ~* "(Mobile)" ){ set $agentflag 1; } - + if ( $agentflag = 1 ) { return 301 https://m.example.com; } @@ -378,6 +380,10 @@ Additionally it is possible to set: `` to specify headers to pass to backend once authentication request completes. * `nginx.ingress.kubernetes.io/auth-request-redirect`: `` to specify the X-Auth-Request-Redirect header value. +* `nginx.ingress.kubernetes.io/auth-cache-key`: + `` this enables caching for auth requests. specify a lookup key for auth responses. e.g. `$remote_user$http_authorization`. Each server and location has it's own keyspace. Hence a cached response is only valid on a per-server and per-location basis. +* `nginx.ingress.kubernetes.io/auth-cache-duration`: + `` to specify a caching time for auth responses based on their response codes, e.g. `200 202 30m`. See [proxy_cache_valid](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_cache_valid) for details. You may specify multiple, comma-separated values: `200 202 10m, 401 5m`. defaults to `200 202 401 5m`. * `nginx.ingress.kubernetes.io/auth-snippet`: `` to specify a custom snippet to use with external authentication, e.g. @@ -681,7 +687,7 @@ of ingress locations. The ModSecurity module must first be enabled by enabling M [ConfigMap](./configmap.md#enable-modsecurity). Note this will enable ModSecurity for all paths, and each path must be disabled manually. -It can be enabled using the following annotation: +It can be enabled using the following annotation: ```yaml nginx.ingress.kubernetes.io/enable-modsecurity: "true" ``` @@ -706,7 +712,7 @@ SecDebugLog /tmp/modsec_debug.log ``` Note: If you use both `enable-owasp-core-rules` and `modsecurity-snippet` annotations together, only the -`modsecurity-snippet` will take effect. If you wish to include the [OWASP Core Rule Set](https://www.modsecurity.org/CRS/Documentation/) or +`modsecurity-snippet` will take effect. If you wish to include the [OWASP Core Rule Set](https://www.modsecurity.org/CRS/Documentation/) or [recommended configuration](https://github.com/SpiderLabs/ModSecurity/blob/v3/master/modsecurity.conf-recommended) simply use the include statement: ```yaml @@ -730,7 +736,7 @@ nginx.ingress.kubernetes.io/influxdb-server-name: "nginx-ingress" For the `influxdb-host` parameter you have two options: -- Use an InfluxDB server configured with the [UDP protocol](https://docs.influxdata.com/influxdb/v1.5/supported_protocols/udp/) enabled. +- Use an InfluxDB server configured with the [UDP protocol](https://docs.influxdata.com/influxdb/v1.5/supported_protocols/udp/) enabled. - Deploy Telegraf as a sidecar proxy to the Ingress controller configured to listen UDP with the [socket listener input](https://github.com/influxdata/telegraf/tree/release-1.6/plugins/inputs/socket_listener) and to write using anyone of the [outputs plugins](https://github.com/influxdata/telegraf/tree/release-1.7/plugins/outputs) like InfluxDB, Apache Kafka, Prometheus, etc.. (recommended) @@ -754,7 +760,7 @@ nginx.ingress.kubernetes.io/backend-protocol: "HTTPS" ### Use Regex !!! attention -When using this annotation with the NGINX annotation `nginx.ingress.kubernetes.io/affinity` of type `cookie`, `nginx.ingress.kubernetes.io/session-cookie-path` must be also set; Session cookie paths do not support regex. +When using this annotation with the NGINX annotation `nginx.ingress.kubernetes.io/affinity` of type `cookie`, `nginx.ingress.kubernetes.io/session-cookie-path` must be also set; Session cookie paths do not support regex. Using the `nginx.ingress.kubernetes.io/use-regex` annotation will indicate whether or not the paths defined on an Ingress use regular expressions. The default value is `false`. @@ -770,9 +776,9 @@ nginx.ingress.kubernetes.io/use-regex: "false" When this annotation is set to `true`, the case insensitive regular expression [location modifier](https://nginx.org/en/docs/http/ngx_http_core_module.html#location) will be enforced on ALL paths for a given host regardless of what Ingress they are defined on. -Additionally, if the [`rewrite-target` annotation](#rewrite) is used on any Ingress for a given host, then the case insensitive regular expression [location modifier](https://nginx.org/en/docs/http/ngx_http_core_module.html#location) will be enforced on ALL paths for a given host regardless of what Ingress they are defined on. +Additionally, if the [`rewrite-target` annotation](#rewrite) is used on any Ingress for a given host, then the case insensitive regular expression [location modifier](https://nginx.org/en/docs/http/ngx_http_core_module.html#location) will be enforced on ALL paths for a given host regardless of what Ingress they are defined on. -Please read about [ingress path matching](../ingress-path-matching.md) before using this modifier. +Please read about [ingress path matching](../ingress-path-matching.md) before using this modifier. ### Satisfy diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index cb3b6798cf..1a8b7dbad0 100755 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -160,6 +160,8 @@ The following table shows a configuration option's name, type, and the default v |[global-auth-response-headers](#global-auth-response-headers)|string|""| |[global-auth-request-redirect](#global-auth-request-redirect)|string|""| |[global-auth-snippet](#global-auth-snippet)|string|""| +|[global-auth-cache-key](#global-auth-cache-key)|string|""| +|[global-auth-cache-duration](#global-auth-cache-duration)|string|"200 202 401 5m"| |[no-auth-locations](#no-auth-locations)|string|"/.well-known/acme-challenge"| |[block-cidrs](#block-cidrs)|[]string|""| |[block-user-agents](#block-user-agents)|[]string|""| @@ -196,7 +198,7 @@ __Note:__ the file `/var/log/nginx/access.log` is a symlink to `/dev/stdout` ## enable-access-log-for-default-backend -Enables logging access to default backend. _**default:**_ is disabled. +Enables logging access to default backend. _**default:**_ is disabled. ## error-log-path @@ -439,7 +441,7 @@ _References:_ Instructs NGINX to create an individual listening socket for each worker process (using the SO_REUSEPORT socket option), allowing a kernel to distribute incoming connections between worker processes _**default:**_ true -## proxy-headers-hash-bucket-size +## proxy-headers-hash-bucket-size Sets the size of the bucket for the proxy headers hash tables. @@ -503,7 +505,7 @@ Enables or disables session resumption through [TLS session tickets](http://ngin Sets the secret key used to encrypt and decrypt TLS session tickets. The value must be a valid base64 string. To create a ticket: `openssl rand 80 | openssl enc -A -base64` -[TLS session ticket-key](http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_session_tickets), by default, a randomly generated key is used. +[TLS session ticket-key](http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_session_tickets), by default, a randomly generated key is used. ## ssl-session-timeout @@ -622,7 +624,7 @@ _References:_ Activates the cache for connections to upstream servers. The connections parameter sets the maximum number of idle keepalive connections to upstream servers that are preserved in the cache of each worker process. When this number is -exceeded, the least recently used connections are closed. +exceeded, the least recently used connections are closed. _**default:**_ 32 _References:_ @@ -643,7 +645,7 @@ _References:_ Sets the maximum number of requests that can be served through one keepalive connection. After the maximum number of requests is made, the connection is closed. _**default:**_ 100 - + _References:_ [http://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_requests](http://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_requests) @@ -922,6 +924,14 @@ Sets a custom snippet to use with external authentication. Applied to all the lo Similar to the Ingress rule annotation `nginx.ingress.kubernetes.io/auth-request-redirect`. _**default:**_ "" +## global-auth-cache-key + +Enables caching for global auth requests. Specify a lookup key for auth responses, e.g. `$remote_user$http_authorization`. + +## global-auth-cache-duration + +Set a caching time for auth responses based on their response codes, e.g. `200 202 30m`. See [proxy_cache_valid](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_cache_valid) for details. You may specify multiple, comma-separated values: `200 202 10m, 401 5m`. defaults to `200 202 401 5m`. + ## no-auth-locations A comma-separated list of locations that should not get authenticated. diff --git a/internal/ingress/annotations/authreq/main.go b/internal/ingress/annotations/authreq/main.go index 18258b9f05..f4dfed88c2 100644 --- a/internal/ingress/annotations/authreq/main.go +++ b/internal/ingress/annotations/authreq/main.go @@ -36,14 +36,19 @@ import ( type Config struct { URL string `json:"url"` // Host contains the hostname defined in the URL - Host string `json:"host"` - SigninURL string `json:"signinUrl"` - Method string `json:"method"` - ResponseHeaders []string `json:"responseHeaders,omitempty"` - RequestRedirect string `json:"requestRedirect"` - AuthSnippet string `json:"authSnippet"` + Host string `json:"host"` + SigninURL string `json:"signinUrl"` + Method string `json:"method"` + ResponseHeaders []string `json:"responseHeaders,omitempty"` + RequestRedirect string `json:"requestRedirect"` + AuthSnippet string `json:"authSnippet"` + AuthCacheKey string `json:"authCacheKey"` + AuthCacheDuration []string `json:"authCacheDuration"` } +// DefaultCacheDuration is the fallback value if no cache duration is provided +const DefaultCacheDuration = "200 202 401 5m" + // Equal tests for equality between two Config types func (e1 *Config) Equal(e2 *Config) bool { if e1 == e2 { @@ -77,12 +82,23 @@ func (e1 *Config) Equal(e2 *Config) bool { return false } + if e1.AuthCacheKey != e2.AuthCacheKey { + return false + } + + match = sets.StringElementsMatch(e1.AuthCacheDuration, e2.AuthCacheDuration) + if !match { + return false + } + return true } var ( - methods = []string{"GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "CONNECT", "OPTIONS", "TRACE"} - headerRegexp = regexp.MustCompile(`^[a-zA-Z\d\-_]+$`) + methods = []string{"GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "CONNECT", "OPTIONS", "TRACE"} + headerRegexp = regexp.MustCompile(`^[a-zA-Z\d\-_]+$`) + statusCodeRegex = regexp.MustCompile(`^[\d]{3}$`) + durationRegex = regexp.MustCompile(`^[\d]+(ms|s|m|h|d|w|M|y)$`) // see http://nginx.org/en/docs/syntax.html ) // ValidMethod checks is the provided string a valid HTTP method @@ -104,6 +120,31 @@ func ValidHeader(header string) bool { return headerRegexp.Match([]byte(header)) } +// ValidCacheDuration checks if the provided string is a valid cache duration +// spec: [code ...] [time ...]; +// with: code is an http status code +// time must match the time regex and may appear multiple times, e.g. `1h 30m` +func ValidCacheDuration(duration string) bool { + elements := strings.Split(duration, " ") + seenDuration := false + + for _, element := range elements { + if len(element) == 0 { + continue + } + if statusCodeRegex.Match([]byte(element)) { + if seenDuration { + return false // code after duration + } + continue + } + if durationRegex.Match([]byte(element)) { + seenDuration = true + } + } + return seenDuration +} + type authReq struct { r resolver.Resolver } @@ -143,6 +184,17 @@ func (a authReq) Parse(ing *networking.Ingress) (interface{}, error) { klog.V(3).Infof("auth-snippet annotation is undefined and will not be set") } + authCacheKey, err := parser.GetStringAnnotation("auth-cache-key", ing) + if err != nil { + klog.V(3).Infof("auth-cache-key annotation is undefined and will not be set") + } + + durstr, _ := parser.GetStringAnnotation("auth-cache-duration", ing) + authCacheDuration, err := ParseStringToCacheDurations(durstr) + if err != nil { + return nil, err + } + responseHeaders := []string{} hstr, _ := parser.GetStringAnnotation("auth-response-headers", ing) if len(hstr) != 0 { @@ -161,13 +213,15 @@ func (a authReq) Parse(ing *networking.Ingress) (interface{}, error) { requestRedirect, _ := parser.GetStringAnnotation("auth-request-redirect", ing) return &Config{ - URL: urlString, - Host: authURL.Hostname(), - SigninURL: signIn, - Method: authMethod, - ResponseHeaders: responseHeaders, - RequestRedirect: requestRedirect, - AuthSnippet: authSnippet, + URL: urlString, + Host: authURL.Hostname(), + SigninURL: signIn, + Method: authMethod, + ResponseHeaders: responseHeaders, + RequestRedirect: requestRedirect, + AuthSnippet: authSnippet, + AuthCacheKey: authCacheKey, + AuthCacheDuration: authCacheDuration, }, nil } @@ -189,3 +243,28 @@ func ParseStringToURL(input string) (*url.URL, string) { return parsedURL, "" } + +// ParseStringToCacheDurations parses and validates the provided string +// into a list of cache durations. +// It will always return at least one duration (the default duration) +func ParseStringToCacheDurations(input string) ([]string, error) { + authCacheDuration := []string{} + if len(input) != 0 { + arr := strings.Split(input, ",") + for _, duration := range arr { + duration = strings.TrimSpace(duration) + if len(duration) > 0 { + if !ValidCacheDuration(duration) { + authCacheDuration = []string{DefaultCacheDuration} + return authCacheDuration, ing_errors.NewLocationDenied(fmt.Sprintf("invalid cache duration: %s", duration)) + } + authCacheDuration = append(authCacheDuration, duration) + } + } + } + + if len(authCacheDuration) == 0 { + authCacheDuration = append(authCacheDuration, DefaultCacheDuration) + } + return authCacheDuration, nil +} diff --git a/internal/ingress/annotations/authreq/main_test.go b/internal/ingress/annotations/authreq/main_test.go index a1a5d0f538..cbd8c18b19 100644 --- a/internal/ingress/annotations/authreq/main_test.go +++ b/internal/ingress/annotations/authreq/main_test.go @@ -79,17 +79,19 @@ func TestAnnotations(t *testing.T) { method string requestRedirect string authSnippet string + authCacheKey string expErr bool }{ - {"empty", "", "", "", "", "", true}, - {"no scheme", "bar", "bar", "", "", "", true}, - {"invalid host", "http://", "http://", "", "", "", true}, - {"invalid host (multiple dots)", "http://foo..bar.com", "http://foo..bar.com", "", "", "", true}, - {"valid URL", "http://bar.foo.com/external-auth", "http://bar.foo.com/external-auth", "", "", "", false}, - {"valid URL - send body", "http://foo.com/external-auth", "http://foo.com/external-auth", "POST", "", "", false}, - {"valid URL - send body", "http://foo.com/external-auth", "http://foo.com/external-auth", "GET", "", "", false}, - {"valid URL - request redirect", "http://foo.com/external-auth", "http://foo.com/external-auth", "GET", "http://foo.com/redirect-me", "", false}, - {"auth snippet", "http://foo.com/external-auth", "http://foo.com/external-auth", "", "", "proxy_set_header My-Custom-Header 42;", false}, + {"empty", "", "", "", "", "", "", true}, + {"no scheme", "bar", "bar", "", "", "", "", true}, + {"invalid host", "http://", "http://", "", "", "", "", true}, + {"invalid host (multiple dots)", "http://foo..bar.com", "http://foo..bar.com", "", "", "", "", true}, + {"valid URL", "http://bar.foo.com/external-auth", "http://bar.foo.com/external-auth", "", "", "", "", false}, + {"valid URL - send body", "http://foo.com/external-auth", "http://foo.com/external-auth", "POST", "", "", "", false}, + {"valid URL - send body", "http://foo.com/external-auth", "http://foo.com/external-auth", "GET", "", "", "", false}, + {"valid URL - request redirect", "http://foo.com/external-auth", "http://foo.com/external-auth", "GET", "http://foo.com/redirect-me", "", "", false}, + {"auth snippet", "http://foo.com/external-auth", "http://foo.com/external-auth", "", "", "proxy_set_header My-Custom-Header 42;", "", false}, + {"auth cache ", "http://foo.com/external-auth", "http://foo.com/external-auth", "", "", "", "$foo$bar", false}, } for _, test := range tests { @@ -98,6 +100,7 @@ func TestAnnotations(t *testing.T) { data[parser.GetAnnotationWithPrefix("auth-method")] = fmt.Sprintf("%v", test.method) data[parser.GetAnnotationWithPrefix("auth-request-redirect")] = test.requestRedirect data[parser.GetAnnotationWithPrefix("auth-snippet")] = test.authSnippet + data[parser.GetAnnotationWithPrefix("auth-cache-key")] = test.authCacheKey i, err := NewParser(&resolver.Mock{}).Parse(ing) if test.expErr { @@ -129,6 +132,9 @@ func TestAnnotations(t *testing.T) { if u.AuthSnippet != test.authSnippet { t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.authSnippet, u.AuthSnippet) } + if u.AuthCacheKey != test.authCacheKey { + t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.authCacheKey, u.AuthCacheKey) + } } } @@ -180,6 +186,54 @@ func TestHeaderAnnotations(t *testing.T) { } } +func TestCacheDurationAnnotations(t *testing.T) { + ing := buildIngress() + + data := map[string]string{} + ing.SetAnnotations(data) + + tests := []struct { + title string + url string + duration string + parsedDuration []string + expErr bool + }{ + {"nothing", "http://goog.url", "", []string{DefaultCacheDuration}, false}, + {"spaces", "http://goog.url", " ", []string{DefaultCacheDuration}, false}, + {"one duration", "http://goog.url", "5m", []string{"5m"}, false}, + {"two durations", "http://goog.url", "200 202 10m, 401 5m", []string{"200 202 10m", "401 5m"}, false}, + {"two durations and empty entries", "http://goog.url", ",5m,,401 10m,", []string{"5m", "401 10m"}, false}, + {"only status code provided", "http://goog.url", "200", []string{DefaultCacheDuration}, true}, + {"mixed valid/invalid", "http://goog.url", "5m, xaxax", []string{DefaultCacheDuration}, true}, + {"code after duration", "http://goog.url", "5m 200", []string{DefaultCacheDuration}, true}, + } + + for _, test := range tests { + data[parser.GetAnnotationWithPrefix("auth-url")] = test.url + data[parser.GetAnnotationWithPrefix("auth-cache-duration")] = test.duration + + i, err := NewParser(&resolver.Mock{}).Parse(ing) + if test.expErr { + if err == nil { + t.Errorf("%v: expected error but retuned nil", err.Error()) + } + continue + } + + t.Log(i) + u, ok := i.(*Config) + if !ok { + t.Errorf("%v: expected an External type", test.title) + continue + } + + if !reflect.DeepEqual(u.AuthCacheDuration, test.parsedDuration) { + t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.duration, u.AuthCacheDuration) + } + } +} + func TestParseStringToURL(t *testing.T) { validURL := "http://bar.foo.com/external-auth" validParsedURL, _ := url.Parse(validURL) @@ -214,3 +268,35 @@ func TestParseStringToURL(t *testing.T) { } } + +func TestParseStringToCacheDurations(t *testing.T) { + + tests := []struct { + title string + duration string + expectedDurations []string + expErr bool + }{ + {"empty", "", []string{DefaultCacheDuration}, false}, + {"invalid", ",200,", []string{DefaultCacheDuration}, true}, + {"single", ",200 5m,", []string{"200 5m"}, false}, + {"multiple with duration", ",5m,,401 10m,", []string{"5m", "401 10m"}, false}, + {"multiple durations", "200 202 401 5m, 418 30m", []string{"200 202 401 5m", "418 30m"}, false}, + } + + for _, test := range tests { + + dur, err := ParseStringToCacheDurations(test.duration) + if test.expErr { + if err == nil { + t.Errorf("%v: expected error but nil was returned", test.title) + } + continue + } + + if !reflect.DeepEqual(dur, test.expectedDurations) { + t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.expectedDurations, dur) + } + } + +} diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index ac41ebb272..c48f1c0afe 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -618,7 +618,7 @@ func NewDefault() Configuration { defNginxStatusIpv4Whitelist = append(defNginxStatusIpv4Whitelist, "127.0.0.1") defNginxStatusIpv6Whitelist = append(defNginxStatusIpv6Whitelist, "::1") defProxyDeadlineDuration := time.Duration(5) * time.Second - defGlobalExternalAuth := GlobalExternalAuth{"", "", "", "", append(defResponseHeaders, ""), "", ""} + defGlobalExternalAuth := GlobalExternalAuth{"", "", "", "", append(defResponseHeaders, ""), "", "", "", []string{}} cfg := Configuration{ AllowBackendServerHeader: false, @@ -803,10 +803,12 @@ type ListenPorts struct { type GlobalExternalAuth struct { URL string `json:"url"` // Host contains the hostname defined in the URL - Host string `json:"host"` - SigninURL string `json:"signinUrl"` - Method string `json:"method"` - ResponseHeaders []string `json:"responseHeaders,omitempty"` - RequestRedirect string `json:"requestRedirect"` - AuthSnippet string `json:"authSnippet"` + Host string `json:"host"` + SigninURL string `json:"signinUrl"` + Method string `json:"method"` + ResponseHeaders []string `json:"responseHeaders,omitempty"` + RequestRedirect string `json:"requestRedirect"` + AuthSnippet string `json:"authSnippet"` + AuthCacheKey string `json:"authCacheKey"` + AuthCacheDuration []string `json:"authCacheDuration"` } diff --git a/internal/ingress/controller/template/configmap.go b/internal/ingress/controller/template/configmap.go index 6eaafc42cc..cd493e2f02 100644 --- a/internal/ingress/controller/template/configmap.go +++ b/internal/ingress/controller/template/configmap.go @@ -57,6 +57,8 @@ const ( globalAuthResponseHeaders = "global-auth-response-headers" globalAuthRequestRedirect = "global-auth-request-redirect" globalAuthSnippet = "global-auth-snippet" + globalAuthCacheKey = "global-auth-cache-key" + globalAuthCacheDuration = "global-auth-cache-duration" ) var ( @@ -226,6 +228,23 @@ func ReadConfig(src map[string]string) config.Configuration { to.GlobalExternalAuth.AuthSnippet = val } + if val, ok := conf[globalAuthCacheKey]; ok { + delete(conf, globalAuthCacheKey) + + to.GlobalExternalAuth.AuthCacheKey = val + } + + // Verify that the configured global external authorization cache duration is valid + if val, ok := conf[globalAuthCacheDuration]; ok { + delete(conf, globalAuthCacheDuration) + + cacheDurations, err := authreq.ParseStringToCacheDurations(val) + if err != nil { + klog.Warningf("Global auth location denied - %s", err) + } + to.GlobalExternalAuth.AuthCacheDuration = cacheDurations + } + // Verify that the configured timeout is parsable as a duration. if not, set the default value if val, ok := conf[proxyHeaderTimeout]; ok { delete(conf, proxyHeaderTimeout) diff --git a/internal/ingress/controller/template/configmap_test.go b/internal/ingress/controller/template/configmap_test.go index b0afbd8ce2..765bca2600 100644 --- a/internal/ingress/controller/template/configmap_test.go +++ b/internal/ingress/controller/template/configmap_test.go @@ -25,6 +25,7 @@ import ( "github.com/kylelemons/godebug/pretty" "github.com/mitchellh/hashstructure" + "k8s.io/ingress-nginx/internal/ingress/annotations/authreq" "k8s.io/ingress-nginx/internal/ingress/controller/config" ) @@ -280,3 +281,25 @@ func TestGlobalExternalAuthSnippetParsing(t *testing.T) { } } } + +func TestGlobalExternalAuthCacheDurationParsing(t *testing.T) { + testCases := map[string]struct { + durations string + expect []string + }{ + "nothing": {"", []string{authreq.DefaultCacheDuration}}, + "spaces": {" ", []string{authreq.DefaultCacheDuration}}, + "one duration": {"5m", []string{"5m"}}, + "two durations and empty entries": {",200 5m,,401 30m,", []string{"200 5m", "401 30m"}}, + "only status code provided": {"200", []string{authreq.DefaultCacheDuration}}, + "mixed valid/invalid": {"5m, xaxax", []string{authreq.DefaultCacheDuration}}, + } + + for n, tc := range testCases { + cfg := ReadConfig(map[string]string{"global-auth-cache-duration": tc.durations}) + + if !reflect.DeepEqual(cfg.GlobalExternalAuth.AuthCacheDuration, tc.expect) { + t.Errorf("Testing %v. Expected \"%v\" but \"%v\" was returned", n, tc.expect, cfg.GlobalExternalAuth.AuthCacheDuration) + } + } +} diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 14478afd36..6d2860f950 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -443,6 +443,9 @@ http { {{ $zone }} {{ end }} + # Cache for internal auth checks + proxy_cache_path /tmp/nginx-cache-auth levels=1:2 keys_zone=auth_cache:10m max_size=128m inactive=30m use_temp_path=off; + # Global filters {{ range $ip := $cfg.BlockCIDRs }}deny {{ trimSpace $ip }}; {{ end }} @@ -894,6 +897,23 @@ stream { location = {{ $authPath }} { internal; + {{ if $externalAuth.AuthCacheKey }} + set $tmp_cache_key '{{ $server.Hostname }}{{ $authPath }}{{ $externalAuth.AuthCacheKey }}'; + set $cache_key ''; + + rewrite_by_lua_block { + ngx.var.cache_key = ngx.encode_base64(ngx.sha1_bin(ngx.var.tmp_cache_key)) + } + + proxy_cache auth_cache; + + {{- range $dur := $externalAuth.AuthCacheDuration }} + proxy_cache_valid {{ $dur }}; + {{- end }} + + proxy_cache_key "$cache_key"; + {{ end }} + # ngx_auth_request module overrides variables in the parent request, # therefore we have to explicitly set this variable again so that when the parent request # resumes it has the correct value set for this variable so that Lua can pick backend correctly @@ -926,7 +946,11 @@ stream { proxy_set_header X-Auth-Request-Redirect $request_uri; {{ end }} + {{ if $externalAuth.AuthCacheKey }} + proxy_buffering "on"; + {{ else }} proxy_buffering {{ $location.Proxy.ProxyBuffering }}; + {{ end }} proxy_buffer_size {{ $location.Proxy.BufferSize }}; proxy_buffers {{ $location.Proxy.BuffersNumber }} {{ $location.Proxy.BufferSize }}; proxy_request_buffering {{ $location.Proxy.RequestBuffering }}; diff --git a/test/e2e/annotations/auth.go b/test/e2e/annotations/auth.go index 6a9dec4b43..b0d483bf29 100644 --- a/test/e2e/annotations/auth.go +++ b/test/e2e/annotations/auth.go @@ -260,6 +260,26 @@ var _ = framework.IngressNginxDescribe("Annotations - Auth", func() { }) }) + It(`should set cache_key when external auth cache is configured`, func() { + host := "auth" + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/auth-url": "http://foo.bar/basic-auth/user/password", + "nginx.ingress.kubernetes.io/auth-cache-key": "foo", + "nginx.ingress.kubernetes.io/auth-cache-duration": "200 202 401 30m", + } + + ing := framework.NewSingleIngress(host, "/", host, f.Namespace, "http-svc", 80, &annotations) + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return Expect(server).Should(MatchRegexp(`\$cache_key.*foo`)) && + Expect(server).Should(ContainSubstring(`proxy_cache_valid 200 202 401 30m;`)) + + }) + }) + Context("when external authentication is configured", func() { host := "auth" @@ -322,6 +342,185 @@ var _ = framework.IngressNginxDescribe("Annotations - Auth", func() { Expect(resp.Header.Get("Location")).Should(Equal(fmt.Sprintf("http://%s/auth/start?rd=http://%s%s", host, host, url.QueryEscape("/?a=b&c=d")))) }) }) + + Context("when external authentication with caching is configured", func() { + thisHost := "auth" + thatHost := "different" + + fooPath := "/foo" + barPath := "/bar" + + BeforeEach(func() { + f.NewHttpbinDeployment() + + var httpbinIP string + + err := framework.WaitForEndpoints(f.KubeClientSet, framework.DefaultTimeout, "httpbin", f.Namespace, 1) + Expect(err).NotTo(HaveOccurred()) + + e, err := f.KubeClientSet.CoreV1().Endpoints(f.Namespace).Get("httpbin", metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + + httpbinIP = e.Subsets[0].Addresses[0].IP + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/auth-url": fmt.Sprintf("http://%s/basic-auth/user/password", httpbinIP), + "nginx.ingress.kubernetes.io/auth-signin": "http://$host/auth/start", + "nginx.ingress.kubernetes.io/auth-cache-key": "fixed", + "nginx.ingress.kubernetes.io/auth-cache-duration": "200 201 401 30m", + } + + for _, host := range []string{thisHost, thatHost} { + By("Adding an ingress rule for /foo") + fooIng := framework.NewSingleIngress(fmt.Sprintf("foo-%s-ing", host), fooPath, host, f.Namespace, "http-svc", 80, &annotations) + f.EnsureIngress(fooIng) + f.WaitForNginxServer(host, func(server string) bool { + return Expect(server).Should(ContainSubstring("location /foo")) + }) + + By("Adding an ingress rule for /bar") + barIng := framework.NewSingleIngress(fmt.Sprintf("bar-%s-ing", host), barPath, host, f.Namespace, "http-svc", 80, &annotations) + f.EnsureIngress(barIng) + f.WaitForNginxServer(host, func(server string) bool { + return Expect(server).Should(ContainSubstring("location /bar")) + }) + } + }) + + It("should return status code 200 when signed in after auth backend is deleted ", func() { + resp, _, errs := gorequest.New(). + Get(f.GetURL(framework.HTTP)+fooPath). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", thisHost). + SetBasicAuth("user", "password"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + + err := f.DeleteDeployment("httpbin") + Expect(err).NotTo(HaveOccurred()) + + resp, _, errs = gorequest.New(). + Get(f.GetURL(framework.HTTP)+fooPath). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", thisHost). + SetBasicAuth("user", "password"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + }) + + It("should deny login for different location on same server", func() { + resp, _, errs := gorequest.New(). + Get(f.GetURL(framework.HTTP)+fooPath). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", thisHost). + SetBasicAuth("user", "password"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + + err := f.DeleteDeployment("httpbin") + Expect(err).NotTo(HaveOccurred()) + + resp, _, errs = gorequest.New(). + Get(f.GetURL(framework.HTTP)+fooPath). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", thisHost). + SetBasicAuth("user", "password"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + + resp, _, errs = gorequest.New(). + Get(f.GetURL(framework.HTTP)+barPath). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", thisHost). + SetBasicAuth("user", "password"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + + By("receiving an internal server error without cache on location /bar") + Expect(resp.StatusCode).Should(Equal(http.StatusInternalServerError)) + }) + + It("should deny login for different servers", func() { + By("logging into server thisHost /foo") + resp, _, errs := gorequest.New(). + Get(f.GetURL(framework.HTTP)+fooPath). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", thisHost). + SetBasicAuth("user", "password"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + + err := f.DeleteDeployment("httpbin") + Expect(err).NotTo(HaveOccurred()) + + resp, _, errs = gorequest.New(). + Get(f.GetURL(framework.HTTP)+fooPath). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", thisHost). + SetBasicAuth("user", "password"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + + resp, _, errs = gorequest.New(). + Get(f.GetURL(framework.HTTP)+fooPath). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", thatHost). + SetBasicAuth("user", "password"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + + By("receiving an internal server error without cache on thisHost location /bar") + Expect(resp.StatusCode).Should(Equal(http.StatusInternalServerError)) + }) + + It("should redirect to signin url when not signed in", func() { + resp, _, errs := gorequest.New(). + Get(f.GetURL(framework.HTTP)). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", thisHost). + RedirectPolicy(func(req gorequest.Request, via []gorequest.Request) error { + return http.ErrUseLastResponse + }). + Param("a", "b"). + Param("c", "d"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + Expect(resp.StatusCode).Should(Equal(http.StatusFound)) + Expect(resp.Header.Get("Location")).Should(Equal(fmt.Sprintf("http://%s/auth/start?rd=http://%s%s", thisHost, thisHost, url.QueryEscape("/?a=b&c=d")))) + }) + }) }) // TODO: test Digest Auth diff --git a/test/e2e/framework/deployment.go b/test/e2e/framework/deployment.go index 4c9268fb16..3beb1fe7ea 100644 --- a/test/e2e/framework/deployment.go +++ b/test/e2e/framework/deployment.go @@ -17,6 +17,8 @@ limitations under the License. package framework import ( + "time" + . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" @@ -138,3 +140,14 @@ func (f *Framework) NewDeployment(name, image string, port int32, replicas int32 err = WaitForEndpoints(f.KubeClientSet, DefaultTimeout, name, f.Namespace, int(replicas)) Expect(err).NotTo(HaveOccurred(), "failed to wait for endpoints to become ready") } + +// DeleteDeployment deletes a deployment with a particular name and waits for the pods to be deleted +func (f *Framework) DeleteDeployment(name string) error { + d, err := f.KubeClientSet.AppsV1().Deployments(f.Namespace).Get(name, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred(), "failed to get a deployment") + err = f.KubeClientSet.AppsV1().Deployments(f.Namespace).Delete(name, &metav1.DeleteOptions{}) + Expect(err).NotTo(HaveOccurred(), "failed to delete a deployment") + return WaitForPodsDeleted(f.KubeClientSet, time.Second*60, f.Namespace, metav1.ListOptions{ + LabelSelector: labelSelectorToString(d.Spec.Selector.MatchLabels), + }) +} diff --git a/test/e2e/framework/k8s.go b/test/e2e/framework/k8s.go index c30a958b78..a072b05a93 100644 --- a/test/e2e/framework/k8s.go +++ b/test/e2e/framework/k8s.go @@ -143,6 +143,21 @@ func WaitForPodsReady(kubeClientSet kubernetes.Interface, timeout time.Duration, }) } +// WaitForPodsDeleted waits for a given amount of time until a group of Pods are deleted in the given namespace. +func WaitForPodsDeleted(kubeClientSet kubernetes.Interface, timeout time.Duration, namespace string, opts metav1.ListOptions) error { + return wait.Poll(2*time.Second, timeout, func() (bool, error) { + pl, err := kubeClientSet.CoreV1().Pods(namespace).List(opts) + if err != nil { + return false, nil + } + + if len(pl.Items) == 0 { + return true, nil + } + return false, nil + }) +} + // WaitForEndpoints waits for a given amount of time until an endpoint contains. func WaitForEndpoints(kubeClientSet kubernetes.Interface, timeout time.Duration, name, ns string, expectedEndpoints int) error { if expectedEndpoints == 0 { diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 7ea5c4d27a..cc19d56952 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -288,6 +288,14 @@ func podRunning(c kubernetes.Interface, podName, namespace string) wait.Conditio } } +func labelSelectorToString(labels map[string]string) string { + var str string + for k, v := range labels { + str += fmt.Sprintf("%s=%s,", k, v) + } + return str[:len(str)-1] +} + // NewInt32 converts int32 to a pointer func NewInt32(val int32) *int32 { p := new(int32) diff --git a/test/e2e/settings/global_external_auth.go b/test/e2e/settings/global_external_auth.go index c3916e869c..e1dd3c8326 100755 --- a/test/e2e/settings/global_external_auth.go +++ b/test/e2e/settings/global_external_auth.go @@ -19,6 +19,7 @@ package settings import ( "fmt" "net/http" + "time" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -146,6 +147,52 @@ var _ = framework.IngressNginxDescribe("Global External Auth", func() { Expect(barResp.StatusCode).Should(Equal(http.StatusOK)) }) + It("should still return status code 200 after auth backend is deleted using cache ", func() { + + globalExternalAuthCacheKeySetting := "global-auth-cache-key" + globalExternalAuthCacheKey := "foo" + globalExternalAuthCacheDurationSetting := "global-auth-cache-duration" + globalExternalAuthCacheDuration := "200 201 401 30m" + globalExternalAuthURL := fmt.Sprintf("http://httpbin.%s.svc.cluster.local:80/status/200", f.Namespace) + + By("Adding a global-auth-cache-key to configMap") + f.UpdateNginxConfigMapData(globalExternalAuthCacheKeySetting, globalExternalAuthCacheKey) + f.UpdateNginxConfigMapData(globalExternalAuthCacheDurationSetting, globalExternalAuthCacheDuration) + f.UpdateNginxConfigMapData(globalExternalAuthURLSetting, globalExternalAuthURL) + + f.WaitForNginxServer(host, + func(server string) bool { + return Expect(server).Should(MatchRegexp(`\$cache_key.*foo`)) && + Expect(server).Should(ContainSubstring(`proxy_cache_valid 200 201 401 30m;`)) + }) + + resp, _, errs := gorequest.New(). + Get(f.GetURL(framework.HTTP)+barPath). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", host). + SetBasicAuth("user", "password"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + + err := f.DeleteDeployment("httpbin") + Expect(err).NotTo(HaveOccurred()) + + resp, _, errs = gorequest.New(). + Get(f.GetURL(framework.HTTP)). + Retry(10, 1*time.Second, http.StatusNotFound). + Set("Host", host). + SetBasicAuth("user", "password"). + End() + + for _, err := range errs { + Expect(err).NotTo(HaveOccurred()) + } + }) + It(`should proxy_method method when global-auth-method is configured`, func() { globalExternalAuthMethodSetting := "global-auth-method"