Support for client secret file. #355

pasha-r · 2020-01-17T15:59:41Z

We had a brief discussion about this issue here: #341

Description

add client-secret-file option
make ProviderData's ClientSecret a function
if client-secret-file is set ClientSecret() reads the file an returns its context
if client-secret-file is not set ClientSecret() behaves the usual way
I replaced all p.ClientSecret reads with p.ClientSecret() call
if client-secret-file is set and file read fails redeem call will return 500 error

Motivation and Context

Some OAuth2 identity management systems rotate client secrets and upload such secrets to running applications as a file in a mounted volume. I would like to be able to read client secret from an in-memory file every time I need to issue a redeem or redeem refresh request.

How Has This Been Tested?

I checked that all existing tests run correctly. I added tests for Options and DataProvider. And checked that Redeem fails if the client secret file is missing.

Checklist:

My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

pasha-r · 2020-01-21T13:28:40Z

Do I need to create a feature branch for this PR?

JoelSpeed

Hey @pasha-r, thanks for implementing this!

I'm pretty happy with this in general, thought I did leave a couple of comments for potential improvements

docs/configuration/configuration.md

providers/provider_data.go

pasha-r · 2020-02-03T11:08:25Z

Updated the naming: the method is GetClientSecret now and the field is ClientSecret

loshz · 2020-02-05T16:25:05Z

options.go

+		if o.ClientSecret == "" && o.ClientSecretFile != "" {
+			_, err := ioutil.ReadFile(o.ClientSecretFile)
+			if err != nil {
+				msgs = append(msgs, "could not read client secret file: "+o.ClientSecretFile)
+			}
+		}


Why do we read the file and do nothing with it? Just to check it's valid?

JoelSpeed

Added one nit, otherwise looking good

Please add a note to the changelog to explain the change and then we can get this merged

providers/provider_data.go

Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>

pasha-r · 2020-02-07T17:08:01Z

CHANGELOG entry - done.
Thank you!

providers/provider_data.go

JoelSpeed

LGTM, thanks!

col-panic · 2024-01-10T10:12:21Z

I don't understand the documentation If you set up your OAuth2 provider to rotate your client secret, you can use the client-secret-file option to reload the secret when it is updated.

Does this mean I have to manually call this on the oauth2-proxy executable? At the moment I start oauth2-proxy and the secret gets updated while the program is running - oauth2-proxy does not seem to pickup the change automatically?!

tuunit · 2024-01-10T11:36:30Z

Hi @col-panic could you share your configuration?

col-panic · 2024-01-10T12:00:10Z

I have a docker compose configuration with the service like this

services:
  # https://oauth2-proxy.github.io/oauth2-proxy/docs/configuration/overview
  # https://oauth2-proxy.github.io/oauth2-proxy/docs/configuration/oauth_provider#keycloak-oidc-auth-provider
  # https://github.com/bitnami/containers/tree/main/bitnami/oauth2-proxy
  oauth2-proxy:
    image: bitnami/oauth2-proxy:7
    networks:
      - backend
    expose:
      - 4180
    volumes:
      - siteconfig:/mnt/siteconfig/
    depends_on:
      ee-config:
        condition: service_completed_successfully
      web:
        condition: service_started
    environment:
      - OAUTH2_PROXY_COOKIE_SECRET=CRtsmWgUeR5sFMVqk9SdyAySkfYvCMkIWSyKKtgXDYY=
      - OAUTH2_PROXY_PROVIDER=keycloak-oidc
      - OAUTH2_PROXY_CLIENT_ID=oauth2-proxy
      - OAUTH2_PROXY_CLIENT_SECRET_FILE=/mnt/siteconfig/oauth2-proxy.secret
      - OAUTH2_PROXY_REDIRECT_URL=https://$EE_HOSTNAME/oauth2/callback
      - OAUTH2_PROXY_OIDC_ISSUER_URL=https://$EE_HOSTNAME/keycloak/auth/realms/ElexisEnvironment
      - OAUTH2_PROXY_CODE_CHALLENGE_METHOD=S256
      - OAUTH2_PROXY_COOKIE_CSRF_PER_REQUEST=true
      - OAUTH2_PROXY_COOKIE_CSRF_EXPIRE=5m
      - OAUTH2_PROXY_EMAIL_DOMAINS=*

Now some time after startup of the oauth2-proxy, the key in /mnt/siteconfig/oauth2-proxy.secret gets exchanged (the resp. volume is mounted by another service, performing the exchange). It seems like the change in the file does not get automatically picked up by oauth2-proxy. Are you reading the file only once, during startup of oauth2-proxy or is it read every time a login process starts?

This is what confuses me in the doc - switching to OAUTH2_PROXY_CLIENT_SECRET with a fixed key seems to work,
but I loose the regular exchange of the client secret!

col-panic · 2024-01-10T13:56:04Z

client-secret-file seems not to be supported in the keycloak-oidc provider. After some research I found, that
only the oidc provider calls the respective method. I am not too familiar with go, but the oidc provider loads the resp. value here https://github.com/oauth2-proxy/oauth2-proxy/blob/v7.5.1/providers/oidc.go while I can't find code in the keycloak-oidc provider doing this https://github.com/oauth2-proxy/oauth2-proxy/blob/v7.5.1/providers/keycloak_oidc.go

admincasper · 2024-01-10T15:50:37Z

I couldn't add the client_secret_file key in my oauth2_proxy.cfg.
But I assume I can set it as a flag or environment variable? To auto-rotate secrets it's recommended to use volumes and not environment variables for secrets. So I'd like to just set the secret file path within the configmap..

oauth2_proxy.cfg: |
    oidc_issuer_url = "https://login.microsoftonline.com/${tenant_id}/v2.0"
    redirect_url = "https://${hostname}"
    email_domains = [ "*" ]
    upstreams = [ "http://${frontend.name}-service.${namespace}.svc.cluster.local:${frontend.port}" ]
    http_address = "0.0.0.0:${oauth2_port}"
    provider = "oidc"
    client_secret_file: "/mnt/secretstore/client-secret" # This gets an error

admincasper · 2024-01-10T15:59:13Z

Also somehow get this error:
2024/01/10 15:58:09] [main.go:43] ERROR: failed to load config: unable to load config file: open /etc/oauth2_proxy/oauth2_proxy.cfg --client-secret-file=/mnt/secretstore/client-secret: no such file or directory when I am definitely mounting the volume..

tuunit · 2024-01-10T16:32:19Z

Hi @col-panic,

first of all, you aren't using our official docker images from quay.io/oauth2-proxy/oauth2-proxy. I have no idea what exactly bitnami is packaging on docker hub. I know they repackage a lot of open source project.

I just checked the code. We always read the client secret file when the toke redeeming is happening:

Redeem:

oauth2-proxy/providers/provider_default.go

Lines 45 to 52 in bfd667e

    
           func (p *ProviderData) Redeem(ctx context.Context, redirectURL, code, codeVerifier string) (*sessions.SessionState, error) { 
        
           	if code == "" { 
        
           		return nil, ErrMissingCode 
        
           	} 
        
           	clientSecret, err := p.GetClientSecret() 
        
           	if err != nil { 
        
           		return nil, err 
        
           	}

GetClientSecret:

oauth2-proxy/providers/provider_data.go

Lines 64 to 76 in bfd667e

    
           func (p *ProviderData) GetClientSecret() (clientSecret string, err error) { 
        
           	if p.ClientSecret != "" || p.ClientSecretFile == "" { 
        
           		return p.ClientSecret, nil 
        
           	} 
        
           	// Getting ClientSecret can fail in runtime so we need to report it without returning the file name to the user 
        
           	fileClientSecret, err := os.ReadFile(p.ClientSecretFile) 
        
           	if err != nil { 
        
           		logger.Errorf("error reading client secret file %s: %s", p.ClientSecretFile, err) 
        
           		return "", errors.New("could not read client secret file") 
        
           	} 
        
           	return string(fileClientSecret), nil 
        
           }

Unfortunately, the Redeem is only called on the full initial oauth2 flow and not the simple authentication path on subsequent requests.

tuunit · 2024-01-10T16:33:48Z

client-secret-file seems not to be supported in the keycloak-oidc provider. After some research I found, that only the oidc provider calls the respective method. I am not too familiar with go, but the oidc provider loads the resp. value here https://github.com/oauth2-proxy/oauth2-proxy/blob/v7.5.1/providers/oidc.go while I can't find code in the keycloak-oidc provider doing this https://github.com/oauth2-proxy/oauth2-proxy/blob/v7.5.1/providers/keycloak_oidc.go

All providers should support it. The issue seems to be as described above but I haven't done a deep analysis yet.

tuunit · 2024-01-10T16:34:51Z

I couldn't add the client_secret_file key in my oauth2_proxy.cfg. But I assume I can set it as a flag or environment variable? To auto-rotate secrets it's recommended to use volumes and not environment variables for secrets. So I'd like to just set the secret file path within the configmap..
oauth2_proxy.cfg: |
    oidc_issuer_url = "https://login.microsoftonline.com/${tenant_id}/v2.0"
    redirect_url = "https://${hostname}"
    email_domains = [ "*" ]
    upstreams = [ "http://${frontend.name}-service.${namespace}.svc.cluster.local:${frontend.port}" ]
    http_address = "0.0.0.0:${oauth2_port}"
    provider = "oidc"
    client_secret_file: "/mnt/secretstore/client-secret" # This gets an error

Your last line is wrong. You are using ":" instead of "=". It's a toml file not a yaml file.

client_secret_file = "/mnt/secretstore/client-secret" # This gets an error

tuunit · 2024-01-10T16:37:03Z

Also somehow get this error: 2024/01/10 15:58:09] [main.go:43] ERROR: failed to load config: unable to load config file: open /etc/oauth2_proxy/oauth2_proxy.cfg --client-secret-file=/mnt/secretstore/client-secret: no such file or directory when I am definitely mounting the volume..

This looks like you made a mistake in your helm chart values.yaml. Can you share your config?

admincasper · 2024-01-11T08:23:42Z

Yeah, I tried to explicity set the command together with the config file in my oauth2 deployment. But it didn't work. The client_secret_file = command seems to work, but will test further. Thanks a lot for the info @tuunit !

    spec:
      securityContext:
        runAsNonRoot: true
        seccompProfile:
          type: "RuntimeDefault"
        fsGroup: 101
      containers:
        - name: oauth2proxy
          image: <repo>.azurecr.io/oauth2-proxy/oauth2-proxy:stable
          args: ["--config=/etc/oauth2_proxy/oauth2_proxy.cfg", "--client-secret-file='/mnt/secretstore/client-secret'"]

admincasper · 2024-01-11T10:08:09Z

@tuunit Is the client_secret_file parameter by .cfg config supported for all providers?
I can only see it referenced in alpha_config code.

Should I try changing from oidc provider to azure, or will it not matter? I see that in commit
42475c2#diff-12eb03d2e4162320c9a848db9eb444ca2bbf9fc19e11d5756f85aaf1a9e38d85
the client_secret_file argument is removed and added as a legacy_option. Do I have no other options as to mount the secret file than using an environment varable?

tuunit · 2024-01-11T20:47:41Z

Your configuration is wrong:

    spec:
      securityContext:
        runAsNonRoot: true
        seccompProfile:
          type: "RuntimeDefault"
        fsGroup: 101
      containers:
        - name: oauth2proxy
          image: <repo>.azurecr.io/oauth2-proxy/oauth2-proxy:stable
          args: ["--config", "/etc/oauth2_proxy/oauth2_proxy.cfg", "--client-secret-file", "/mnt/secretstore/client-secret"]

You have essential tried to use a flag "--config=/etc/oauth2_proxy/oauth2_proxy.cfg" with a parameter of "--client-secret-file=/mnt/secretstore/client-secret".

tuunit · 2024-01-11T20:55:19Z

@tuunit Is the client_secret_file parameter by .cfg config supported for all providers? I can only see it referenced in alpha_config code.

Should I try changing from oidc provider to azure, or will it not matter? I see that in commit 42475c2#diff-12eb03d2e4162320c9a848db9eb444ca2bbf9fc19e11d5756f85aaf1a9e38d85 the client_secret_file argument is removed and added as a legacy_option. Do I have no other options as to mount the secret file than using an environment varable?

The client_secret_file legacy option, --client-secret-file flag and clientSecretFile yaml / alpha option can be used with any provider.

admincasper · 2024-01-16T08:15:10Z

@tuunit Thanks for your patience. I get sporadic 403 CSRF error, sometimes adding client-id, client-secret and cookie through environment variable works. Changing to using client_secret_file option makes the error trigger again.

Could you take a look?
Here is my deployment:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: app-oauth2-proxy
  labels:
    app.kubernetes.io/name: app-oauth2-proxy
    app.kubernetes.io/created-by: app
    app.kubernetes.io/component: frontend-oauth2-proxy
spec:
  selector:
    matchLabels:
      app: app-oauth2-proxy
  template:
    metadata:
      labels:
        app: app-oauth2-proxy
        app.kubernetes.io/name: app-oauth2-proxy
        app.kubernetes.io/created-by: app
        app.kubernetes.io/component: frontend-oauth2-proxy
        azure.workload.identity/use: "true"
    spec:
      serviceAccountName: <keyvault_mi>
      securityContext:
        runAsNonRoot: true
        seccompProfile:
          type: "RuntimeDefault"
        fsGroup: 101
      containers:
        - name: oauth2proxy
          image: <acr>.azurecr.io/oauth2-proxy/oauth2-proxy:v7.5.1
          args: ["--config","/etc/oauth2_proxy/oauth2_proxy.cfg"]
          # Could comment this in instead, get CSRF error.
          # args: ["--config","/etc/oauth2_proxy/oauth2_proxy.cfg","--client-secret-file","/mnt/secretstore/client-secret"]
          livenessProbe:
            httpGet:
              path: /ping
              port: 8080
            periodSeconds: 3
            timeoutSeconds: 1
          resources:
            limits:
              cpu: 500m
              memory: 200Mi
            requests:
              cpu: 200m
              memory: 100Mi
          ports:
            - name: oauth2proxy-pod
              containerPort: 8080
          volumeMounts:
            - name: app-oauth2-proxy-config
              mountPath: /etc/oauth2_proxy/
            - name: oauth2-proxy-secrets-store
              mountPath: /mnt/secretstore
              readOnly: true
          env:
            - name: OAUTH2_PROXY_CLIENT_ID
              valueFrom:
                secretKeyRef:
                  name: oauth2-proxy-secrets-store
                  key: client-id
            - name: OAUTH2_PROXY_CLIENT_SECRET # Could comment this out
              valueFrom:
                secretKeyRef:
                  name: oauth2-proxy-secrets-store
                  key: client-secret
            - name: OAUTH2_PROXY_COOKIE_SECRET
              valueFrom:
                secretKeyRef:
                  name: oauth2-proxy-secrets-store
                  key: cookie-secret
          securityContext:
            privileged: false
            allowPrivilegeEscalation: false
            readOnlyRootFilesystem: true
            capabilities:
              drop: ["ALL"]
      volumes:
        - name: app-oauth2-proxy-config
          configMap:
            name: app-oauth2-proxy
        - name: oauth2-proxy-secrets-store
          csi:
            driver: secrets-store.csi.k8s.io
            readOnly: true
            volumeAttributes:
              secretProviderClass: app-oauth2-proxy

And configmap:

apiVersion: v1
kind: ConfigMap
metadata:
  name: app-oauth2-proxy
data:
  oauth2_proxy.cfg: |
    oidc_issuer_url = "https://login.microsoftonline.com/${tenant_id}/v2.0"
    redirect_url = "https://${hostname}"
    email_domains = [ "*" ]
    upstreams = [ "http://app-service.${namespace}.svc.cluster.local:9898" ]
    http_address = "0.0.0.0:8080"
    provider = "oidc"
    # Could add this, get CSRF error if env variables in deployment are not added
    # client_secret_file = "/mnt/secretstore/client-secret"

    cookie_secure = true
    whitelist_domains = ".<domain>.com"
    cookie_domains = ".<domain>.com"
    cookie_expire = "4h"
    cookie_refresh = "2h"
    cookie_samesite = "lax"
    cookie_name = "__Host-app"

    pass_access_token = true
    pass_authorization_header = true
    skip_provider_button = true
    set_authorization_header = true
    set_xauthrequest = true
    skip_auth_preflight = false

    session_store_type = "redis"
    redis_connection_url = "redis://redis-master.oauth2-proxy.svc.cluster.local:6379"

tuunit · 2024-01-16T09:23:52Z

@admincasper you might have actually found a bug then 🤔 I'll have to check the code for the differences in the flow.

admincasper · 2024-01-16T10:02:14Z

@tuunit The CSRF issue was fixed by removing cookie_domains = ".<domain>.com" in configmap.

pasha-r added 3 commits January 16, 2020 19:01

added ClientSecretFile in ProviderData

6346d26

Merge branch 'master' of github.com:pasha-r/oauth2_proxy

3bd0dae

add documentation notes on client secret file

6006eae

pasha-r requested review from steakunderscore and loshz as code owners January 17, 2020 15:59

pasha-r requested a review from a team January 17, 2020 15:59

added Changelog entry for Client Secret File PR

285c730

pasha-r mentioned this pull request Jan 17, 2020

Support for client secret file. #341

Closed

Merge branch 'master' into master

b25878c

JoelSpeed added the enhancement label Jan 26, 2020

JoelSpeed reviewed Jan 26, 2020

View reviewed changes

docs/configuration/configuration.md Outdated Show resolved Hide resolved

providers/provider_data.go Outdated Show resolved Hide resolved

pasha-r added 4 commits January 27, 2020 12:38

fixing configuration.md

5ab3f4e

Merge branch 'master' of github.com:pasha-r/oauth2_proxy

b94ce32

addressing PR issue of ClientSecret property naming

641d308

resolved conflicts with upstream

96bcba2

loshz reviewed Feb 5, 2020

View reviewed changes

JoelSpeed reviewed Feb 6, 2020

View reviewed changes

providers/provider_data.go Outdated Show resolved Hide resolved

pasha-r and others added 3 commits February 7, 2020 15:08

Update providers/provider_data.go

3b6a1b7

Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>

merged with upstream

e440562

corrected changelog entry

81f5dfd

JoelSpeed reviewed Feb 7, 2020

View reviewed changes

providers/provider_data.go Outdated Show resolved Hide resolved

pasha-r and others added 2 commits February 8, 2020 16:54

fixed typo in GetClientSecret

b7560b9

Merge branch 'master' into master

c9fa0fa

JoelSpeed approved these changes Feb 15, 2020

View reviewed changes

JoelSpeed merged commit f2661c4 into oauth2-proxy:master Feb 15, 2020

MisterWil mentioned this pull request Feb 15, 2020

Fails to start with error flag "client-secret-file" does not exist #396

Closed

col-panic mentioned this pull request Jan 10, 2024

Keycloak OIDC provider config fails: invalid keys #2341

Closed

Uh oh!

Support for client secret file. #355

Support for client secret file. #355

Uh oh!

Conversation

pasha-r commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Checklist:

Uh oh!

pasha-r commented Jan 21, 2020

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pasha-r commented Feb 3, 2020

Uh oh!

loshz Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

pasha-r Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pasha-r commented Feb 7, 2020

Uh oh!

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

col-panic commented Jan 10, 2024

Uh oh!

tuunit commented Jan 10, 2024

Uh oh!

col-panic commented Jan 10, 2024

Uh oh!

col-panic commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

admincasper commented Jan 10, 2024

Uh oh!

admincasper commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tuunit commented Jan 10, 2024

Uh oh!

tuunit commented Jan 10, 2024

Uh oh!

tuunit commented Jan 10, 2024

Uh oh!

tuunit commented Jan 10, 2024

Uh oh!

admincasper commented Jan 11, 2024

Uh oh!

admincasper commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tuunit commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tuunit commented Jan 11, 2024

Uh oh!

admincasper commented Jan 16, 2024

Uh oh!

tuunit commented Jan 16, 2024

Uh oh!

admincasper commented Jan 16, 2024

Uh oh!

Uh oh!

pasha-r commented Jan 17, 2020 •

edited

Loading

col-panic commented Jan 10, 2024 •

edited

Loading

admincasper commented Jan 10, 2024 •

edited

Loading

admincasper commented Jan 11, 2024 •

edited

Loading

tuunit commented Jan 11, 2024 •

edited

Loading