fix: broken oauth2_proxy flags; change the behavior of oauth2_proxy t…

…o be used as proxy (#149) Co-authored-by: Vivian Shao <[email protected]>
chanzuckerberg · Oct 25, 2024 · 271aabe · 271aabe
1 parent 710adf5
commit 271aabe
Show file tree

Hide file tree

Showing 6 changed files with 133 additions and 142 deletions.
diff --git a/stack/templates/_helpers.tpl b/stack/templates/_helpers.tpl
@@ -3,11 +3,24 @@ Expand the name of the chart.
 */}}
 {{- define "stack.name" -}}
 {{- default .Chart.Name .nameOverride | trunc 63 | trimSuffix "-" }}
-{{- end }}
+{{- end -}}
 
 {{- define "service.name" -}}
 {{- .Values.name | trunc 63 | trimSuffix "-" }}
-{{- end }}
+{{- end -}}
+
+{{- define "service.backend" -}}
+{{- if .Values.ingress.oidcProtected -}}
+name: {{ include "oidcProxy.name" . }}
+port:
+    number:  {{ include "oidcProxy.port" .}}
+{{- else }}
+name: {{ include "service.fullname" . }}
+port:
+    number: {{  .Values.service.port | int}}
+{{- end -}}
+{{- end -}}
+
 
 {{/*
 Create a default fully qualified app name.
@@ -147,7 +160,7 @@ Container probes cannot have both httpGet and tcpSocket fields, so we use omit t
 
 {{- define "oidcProxy.name" -}}
 {{ include "stack.fullname" . | lower }}-oidc-proxy
-{{- end }}
+{{- end -}}
 
 {{- define "oidcProxy.port" -}}
 {{ .Values.oidcProxy.port | default 4180 | int  }}
@@ -180,46 +193,4 @@ app.kubernetes.io/instance: {{ .Release.Name }}
 
 {{- define "oidcProxy.authDomain" -}}
 {{ .Values.ingress.host }}
-{{- end -}}
-
-{{- define "oidcProxy.skipAuthConfig" -}}
-{{- $letsEncryptVerifySkip := (dict "path" "/.well-known/*" "method" "GET") -}}
-{{- range  $k, $v := append .Values.oidcProxy.skipAuth $letsEncryptVerifySkip -}}
-{{- $id := printf "%s_%s" ($v.method |lower) ($v.path | replace "/" "")}}
-{{- $id := regexReplaceAll "\\W+" $id "_" -}}
-{{- $var_name := printf "%s_%s" "skip_auth" $id }}
-set ${{ $var_name }} 1;
-
-if ( $request_uri !~ "{{$v.path}}" ) {
-    set ${{ $var_name }}  0;
-}
-
-if ( $request_method !~ "{{$v.method}}" ) {
-    set ${{ $var_name }}  0;
-}
-
-if ( ${{ $var_name }} ) {
-    return 200;
-}
-{{- end -}}
-{{- end -}}
-
-{{- define "oidcProxy.nginxAuthAnnotations" -}}
-nginx.ingress.kubernetes.io/auth-url: "http://{{ include "oidcProxy.name" . }}.{{ .Release.Namespace }}.svc.cluster.local:4180/oauth2/auth"
-nginx.ingress.kubernetes.io/auth-signin: "https://$host/oauth2/sign_in?rd=https://$host$escaped_request_uri"
-nginx.ingress.kubernetes.io/auth-response-headers: {{join "," (concat (list "Authorization" "X-Auth-Request-User" "X-Auth-Request-Groups" "X-Auth-Request-Email" "X-Auth-Request-Preferred-Username") .Values.oidcProxy.additionalHeaders) }}
-nginx.ingress.kubernetes.io/auth-snippet: |
-{{- include "oidcProxy.skipAuthConfig" . | nindent 4 }}
-nginx.ingress.kubernetes.io/configuration-snippet: |
-    auth_request_set $email $upstream_http_x_auth_request_email;
-    auth_request_set $user $upstream_http_x_auth_request_user;
-    auth_request_set $groups $upstream_http_x_auth_request_groups;
-    auth_request_set $preferred_username $upstream_http_x_auth_request_preferred_username;
-
-    proxy_set_header X-Forwarded-Email $email;
-    proxy_set_header X-Forwarded-User $user;
-    proxy_set_header X-Forwarded-Groups $groups;
-    proxy_set_header X-Forwarded-Preferred-Username $preferred_username;
-    proxy_set_header Authorization $http_authorization;
-    proxy_pass_header Authorization;
-{{- end -}}
+{{- end -}}
diff --git a/stack/templates/ingress.yaml b/stack/templates/ingress.yaml
@@ -1,4 +1,4 @@
-{{ $global := . }}
+{{- $global := . -}}
 {{ range $serviceName, $serviceValues := .Values.services }}
   {{- $globalValuesDict := $global.Values.global | toYaml -}}
   {{- $values := fromYaml $globalValuesDict -}}
@@ -7,25 +7,18 @@
   {{- $service := dict "Chart" $global.Chart "Release" $global.Release "Capabilities" $global.Capabilities "Values" $values -}}
 {{- with $service }}
 
+{{- if .Values.ingress.enabled -}}
 ---
-{{ if .Values.ingress.enabled }}
-{{- $fullName := include "service.fullname" . -}}
-{{- $svcPort := .Values.service.port -}}
 apiVersion: networking.k8s.io/v1
 kind: Ingress
 metadata:
-  name: {{ $fullName }}
+  name: {{ include "service.fullname" .  }}
   labels:
     {{- include "service.labels" . | nindent 4 }}
-  {{- $nginxAuthAnnotations := dict}}
-    {{- if .Values.ingress.oidcProtected }}
-    {{- $nginxAuthAnnotations = (fromYaml (include "oidcProxy.nginxAuthAnnotations" . )) -}}
-    {{- end}}
     {{- $certManagerAnnotations := (fromYaml (include "certManagerAnnotations" . )) }}
   annotations:
     {{- with (mergeOverwrite 
                 (dict)
-                ($nginxAuthAnnotations)
                 ($certManagerAnnotations)
                 (.Values.annotations) 
                 (.Values.ingress.annotations)
@@ -45,15 +38,13 @@ spec:
             {{- end }}
             backend:
               service:
-                name: {{ $fullName }}
-                port:
-                  number: {{ $svcPort }}
+                {{- include "service.backend" $service | nindent 16}}
           {{- end }}
-    {{ $customHosts := list}}
-    {{ range $i, $rule := .Values.ingress.rules }}
+    {{- $customHosts := list -}}
+    {{- range $i, $rule := .Values.ingress.rules -}}
     {{- $ruleValues := mergeOverwrite (dict "host" $service.Values.ingress.host "paths" $service.Values.ingress.paths) $rule -}}
     {{ $customHosts = append $customHosts $ruleValues.host }}
-    - host: {{ $ruleValues.host | quote }}
+    - host: {{ $ruleValues.host }}
       http:
         paths:
           {{- range $ruleValues.paths }} 
@@ -63,11 +54,9 @@ spec:
             {{- end }}
             backend:
               service:
-                name: {{ $fullName }}
-                port:
-                  number: {{ $svcPort }}
-          {{- end }}
-    {{ end }}
+                {{- (include "service.backend" $service) | nindent 16 -}}
+          {{ end -}}
+    {{- end }}
   tls:
     - hosts:
       - {{ $service.Values.ingress.host  }}
@@ -83,8 +72,7 @@ spec:
       {{- toYaml $customHosts | nindent 6 }}
       {{- $secretName := printf "%s-%s-%s" "custom-hosts" (include "stack.fullname" .) "tls-secret" }}
       secretName: {{  regexReplaceAll "[^a-zA-Z0-9-]" $secretName "-" }}
-    {{- end -}}
-{{- end }}
----
+    {{ end }}
+{{ end }}
 {{- end }}
 {{- end }}
diff --git a/stack/templates/oidc_proxy.yaml b/stack/templates/oidc_proxy.yaml
@@ -1,16 +1,26 @@
 {{ $global := . }}
 {{- $allHosts := list -}}
+{{- $allOIDCProtectedServces := list -}}
 {{ range $serviceName, $serviceValues := .Values.services }}
   {{- $globalValuesDict := $global.Values.global | toYaml -}}
   {{- $values := fromYaml $globalValuesDict -}}
   {{- $values = set $values "name" $serviceName -}}
   {{- $values := mergeOverwrite $values $serviceValues -}}
 
   {{- with $values -}}
+  {{ $serviceScope := dict "Chart" $global.Chart "Release" $global.Release "Capabilities" $global.Capabilities "Values" .}}
   {{- if .ingress.oidcProtected -}}
+    {{ range $i, $path := .ingress.paths }}
+      {{- $allOIDCProtectedServces = append 
+        $allOIDCProtectedServces 
+        (printf "http://%s.%s.svc.cluster.local:%d%s" (include "service.fullname" $serviceScope) $global.Release.Namespace ($values.service.port | int) ($path.path)) 
+      -}}
+    {{- end -}}
+
   {{ range $i, $rule := .ingress.rules }}
   {{- $allHosts = append $allHosts $rule.host }}
   {{- end -}}
+
   {{- end -}}
   {{- end -}}
 {{- end -}}
@@ -51,14 +61,27 @@ spec:
             - --provider=oidc
             - --email-domain=*
             - --cookie-secure=true
-            - --set-authorization-header
             - --set-xauthrequest
             - --cookie-domain={{- include "baseDomain" . }}
             - --whitelist-domain=.{{- include "baseDomain" . }}
             - --skip-provider-button=true
             - --pass-authorization-header=true
             - --reverse-proxy
             - --skip-jwt-bearer-tokens
+
+            {{- range $allOIDCProtectedServces }}
+            - --upstream={{ . }}
+            {{- end -}}
+
+            {{- range .Values.oidcProxy.skipAuth }}
+            # for backwards compatibility, could also just be provided using extraArgs
+            {{ if contains "*" .method }}
+            - --skip-auth-route={{ .path }}
+            {{- else -}}
+            - --skip-auth-route={{ .method }}={{ .path }}
+            {{- end -}}
+            {{- end -}}
+
             {{- range $allHosts -}}
             {{- printf "- --whitelist-domain=%s" . | nindent 12 -}}
             {{- printf "- --cookie-domain=%s" . | nindent 12 -}}
@@ -108,6 +131,7 @@ spec:
     {{- include "oidcProxy.selectorLabels" . | nindent 4 }}
 
 ---
+
 {{- if .Values.ingress.enabled }}
 apiVersion: networking.k8s.io/v1
 kind: Ingress

diff --git a/stack/tests/ingress_test.yaml b/stack/tests/ingress_test.yaml
@@ -17,18 +17,6 @@ tests:
     asserts:
       - isKind:
           of: Ingress
-      - documentIndex: 0
-        equal:
-          path: metadata.annotations["nginx.ingress.kubernetes.io/auth-url"]
-          value: "http://release-name-stack-oidc-proxy.NAMESPACE.svc.cluster.local:4180/oauth2/auth"
-      - documentIndex: 0
-        equal:
-          path: metadata.annotations["nginx.ingress.kubernetes.io/auth-signin"]
-          value: "https://$host/oauth2/sign_in?rd=https://$host$escaped_request_uri"
-      - documentIndex: 0
-        equal:
-          path: metadata.annotations["nginx.ingress.kubernetes.io/auth-response-headers"]
-          value: "Authorization,X-Auth-Request-User,X-Auth-Request-Groups,X-Auth-Request-Email,X-Auth-Request-Preferred-Username"
   - it: adds additional nginx auth headers when using additionalHeaders
     set:
       global:
@@ -46,10 +34,6 @@ tests:
     asserts:
       - isKind:
           of: Ingress
-      - documentIndex: 0
-        equal:
-          path: metadata.annotations["nginx.ingress.kubernetes.io/auth-response-headers"]
-          value: "Authorization,X-Auth-Request-User,X-Auth-Request-Groups,X-Auth-Request-Email,X-Auth-Request-Preferred-Username,X-Forwarded-User,blahblahblah"
   - it: adds auth-snippet with using skipAuth
     set:
       global:
@@ -69,49 +53,6 @@ tests:
     asserts:
       - isKind:
           of: Ingress
-      - documentIndex: 0
-        equal:
-          path: metadata.annotations["nginx.ingress.kubernetes.io/auth-snippet"]
-          value: |
-            set $skip_auth_get_healthz 1;
-
-            if ( $request_uri !~ "/healthz" ) {
-                set $skip_auth_get_healthz  0;
-            }
-
-            if ( $request_method !~ "GET" ) {
-                set $skip_auth_get_healthz  0;
-            }
-
-            if ( $skip_auth_get_healthz ) {
-                return 200;
-            }
-            set $skip_auth___api_ 1;
-
-            if ( $request_uri !~ "/api/*" ) {
-                set $skip_auth___api_  0;
-            }
-
-            if ( $request_method !~ "*" ) {
-                set $skip_auth___api_  0;
-            }
-
-            if ( $skip_auth___api_ ) {
-                return 200;
-            }
-            set $skip_auth_get__well_known_ 1;
-
-            if ( $request_uri !~ "/.well-known/*" ) {
-                set $skip_auth_get__well_known_  0;
-            }
-
-            if ( $request_method !~ "GET" ) {
-                set $skip_auth_get__well_known_  0;
-            }
-
-            if ( $skip_auth_get__well_known_ ) {
-                return 200;
-            }
   - it: adds adds certManager annotations
     set:
       global:
@@ -132,6 +73,9 @@ tests:
             rules:
               - host: cellxgene.cziscience.com
               - host: api.cellxgene.cziscience.com
+        unprotected-service:
+          ingress:
+            oidcProtected: false
     asserts:
       - isKind:
           of: Ingress
@@ -194,3 +138,19 @@ tests:
         lengthEqual:
           path: spec.tls[1].hosts
           count: 2
+      - documentIndex: 0
+        equal:
+          path: spec.rules[0].http.paths[0].backend.service.name
+          value: missing-otter-oidc-proxy
+      - documentIndex: 0
+        equal:
+          path: spec.rules[1].http.paths[0].backend.service.name
+          value: missing-otter-oidc-proxy
+      - documentIndex: 0
+        equal:
+          path: spec.rules[2].http.paths[0].backend.service.name
+          value: missing-otter-oidc-proxy
+      - documentIndex: 1
+        equal:
+          path: spec.rules[0].http.paths[0].backend.service.name
+          value: missing-otter-unprotected-service