New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add support for retrieving ClientId, ClientSecret and SSO secret from env var as opposed to creating a kube Secret #11777

Closed

ebourgeois wants to merge 1 commit into argoproj:main from RBC:feat-support-sso-via-env

ebourgeois commented Sep 8, 2023 •

edited

Fixes #10395
Fixes #11772

Motivation

This change adds support for retrieving ClientId, ClientSecret and SSO secret from env var as opposed to creating a kube Secret, allowing us to provide our own and not use Kube Secrets at all.

Modifications

The changes made were:

In the SSO auth code and config, to solve the above problem statement, we believe the following
would be required:

Allow setting clientId from environment variable
Allow setting clientSecret from environment variable
Allow setting of PKCS1 SSO secret from environment variable

Verification

We successfully:

backed up the sso secret
deleted the sso resource in our Kube namespace.
created 3 environment variables from our secret store:
1. ARGO_CLIENT_ID: our SSO client ID
2. ARGO_CLIENT_SECRET: our client secret
3. ARGO_PRIVATE_KEY: PKCS1 key, encoded in base64
Added the the new mapping into the sso configmap:

      privateKeyEnvName: ARGO_PRIVATE_KEY
      clientIdEnvName: ARGO_CLIENT_ID
      clientSecretEnvName: ARGO_CLIENT_SECRET

started workflow server successfully
able to login with our SSO and use our custom groups

Regarding fix #10395:

In this PR, we are fixing issue: #10395
- this issue was caused by the creation of a new httpClient and not
re-using the existing one in sso.go, where InsecureSkipVerify
is being set and not propagated tot the groups claim call

agilgur5 added the area/sso-rbac label

agilgur5 self-requested a review

September 26, 2023 23:45

agilgur5 reviewed

View reviewed changes

Member

agilgur5 left a comment •

edited

Just a quick first pass with some small feedback:

Lint and DCO are failing.
- See in-line comment re: lint.
- All commits must have DCO (only one of the three current has it), so you'll need to force push on top with and amendment w/ DCO
Would be good to have some docs around this
- the example configmap in the docs would be a good first start
- I think you could also add a subsection here in the SSO docs describing this

config/sso.go Outdated Show resolved Hide resolved

config/sso.go Outdated Show resolved Hide resolved

Member

agilgur5 commented Sep 27, 2023

Fixes #10395

If these are to be part of the same PR, could you add a description about that to this PR? It's only mentioned on this one line.
I did see that you did some root cause analysis in #10395 (comment)

Member

agilgur5 commented Sep 27, 2023 •

edited

Hmm, the groups change might be better as a separate PR since it's a fix while the secrets are a feature. When together, the fix is not cherry-pickable.

If I'm reading correctly, the groups change is isolated to claims while the secrets feature is in sso, so there's no overlap? EDIT: nvm, I see the groups changes in sso as well

agilgur5 reviewed

View reviewed changes

server/auth/sso/sso.go Show resolved Hide resolved

ebourgeois force-pushed the feat-support-sso-via-env branch from 4035b65 to dcb8b6f Compare

September 27, 2023 20:55

ebourgeois closed this

ebourgeois reopened this

ebourgeois force-pushed the feat-support-sso-via-env branch 2 times, most recently from 8e23376 to fc3f210 Compare

September 28, 2023 02:35

ebourgeois requested a review from agilgur5

September 28, 2023 13:20

ebourgeois changed the title ~~feat: Add support for retrieving ClientId, ClientSecret and SSO secret from env var as opposed to creating a kube Secret, allowing us to provide our own and not use Kube Secrets at all.~~ feat: Add support for retrieving ClientId, ClientSecret and SSO secret from env var as opposed to creating a kube Secret


          feat: Add support for retrieving ClientId, ClientSecret and SSO secre…

1593baa

…t from env var as opposed to creating a kube Secret

This will allow us to provide our own SSO private key and not use Kube Secrets at all.

Also, in this PR, we are fixing issue: argoproj#10395
    - this issue was caused by the creation of a new httpClient and not
      re-using the existing one in `sso.go`, where `InsecureSkipVerify`
      is being set and not propagated tot the groups claim call

Signed-off-by: Erick Bourgeois <erick.bourgeois@rbccm.com>

ebourgeois force-pushed the feat-support-sso-via-env branch from fc3f210 to 1593baa Compare

September 28, 2023 18:02

terrytangyuan self-assigned this

agilgur5 requested changes

View reviewed changes

Member

agilgur5 left a comment

Several more docs changes below. Will leave a separate review for the code

docs/workflow-controller-configmap.yaml

Comment on lines +398 to +400

+                  # This is the environment variable name that will contain the Client ID; this is equivalent to
+                  # what is in the secret `argo-workflows-sso.client-id`. If this is not provided, then we
+                  # default to using kube Secret above.

Member

agilgur5 Oct 12, 2023

Suggested change

      
                # This is the environment variable name that will contain the Client ID; this is equivalent to
          
                # what is in the secret `argo-workflows-sso.client-id`. If this is not provided, then we
          
                # default to using kube Secret above.
          
                # Environment variable that contains the OIDC client ID. Alternative to the Secret clientId above.

use simple and direct language, per k8s style guide
use present tense, per k8s style guide
be specific that this is an alternative
less duplication and consistent references -- "secret", "Secret above", "kube Secret" are all referring to the same thing, use "Secret" instead, per k8s style guide
- consistently use "OIDC client id" to match above text
there is no argo-workflows-sso.client-id. sso.clientId is valid, but we can shorten that to just clientId as it's in the same section and is just a few lines above
- use clientId without backticks to match existing text

docs/workflow-controller-configmap.yaml

@@ @@ -395,6 +395,20 @@ data: @@
                   # be in the form <argo-server-root-url>/oauth2/callback. It must be
                   # browser-accessible. If omitted, will be automatically generated.
                   redirectUrl: https://argo-server/oauth2/callback

Member

agilgur5 Oct 12, 2023

let's move redirectURL to be below these, so that all related configurations are next to each other

docs/workflow-controller-configmap.yaml

Comment on lines +403 to +406

+                  # This is the environment variable name that contains OIDC client
+                  # secret issued to the application by the provider; this is equivalent to
+                  # what is in the secret `argo-workflows-sso.client-secret`. If this is not provided, then we
+                  # default to using kube Secret above.

Member

agilgur5 Oct 12, 2023 •

edited

Suggested change

      
                # This is the environment variable name that contains OIDC client
          
                # secret issued to the application by the provider; this is equivalent to
          
                # what is in the secret `argo-workflows-sso.client-secret`. If this is not provided, then we
          
                # default to using kube Secret above.
          
                # Environment variable that contains the OIDC client secret. Alternative to the Secret clientSecret above.

same as above

docs/workflow-controller-configmap.yaml

Comment on lines +409 to +410

		# This is the environment variable name that contains PKCS1 private key, if this is not
		# provided, then Argo will create one dynamically and store in Kube Secret.

Member

agilgur5 Oct 12, 2023 •

edited

Suggested change

      
                # This is the environment variable name that contains PKCS1 private key, if this is not
          
                # provided, then Argo will create one dynamically and store in Kube Secret.
          
                # Environment variable that contains the PKCS1 private key. If not provided,
          
                # Argo will create one dynamically and store it in a Secret.

similar to above
fix run-on sentence

docs/workflow-controller-configmap.yaml

		@@ -395,6 +395,20 @@ data:
		# be in the form <argo-server-root-url>/oauth2/callback. It must be

Member

agilgur5 Oct 12, 2023

GH won't let me comment on unchanged lines above, but clientId and clientSecret should be changed to "(required if clientIdEnvName is not provided)" etc instead of just "(required)"

docs/argo-server-sso-argocd.md

@@ @@ -107,6 +107,44 @@ data: @@
                   redirectUrl: https://argo-workflows.mydomain.com/oauth2/callback
               ```
+              ## Example SSO config with Secrets from environment variable

Member

agilgur5 Oct 12, 2023

this is the ArgoCD Dex integration page, not the general SSO page. this configuration is general to all SSO, so it should be in the general SSO page.

In my previous review comment, I suggested putting it on that page in a specific subsection that has instructions for creating Secrets.

docs/argo-server-sso-argocd.md

Comment on lines +122 to +124

+                # SSO Configuration for the Argo server.
+                # You must also start argo server with `--auth-mode sso`.
+                # https://argoproj.github.io/argo-workflows/argo-server-auth-mode/

Member

agilgur5 Oct 12, 2023 •

edited

Instead of specifying how to start it, you can add a sample manifest for the Argo Server Deployment with this flag. That is what is used in other examples on this page as well (and is why there is a --- in them to split the YAML manifests).
The Deployment would also be useful to specify the environment variables that are provided in the ConfigMap.

I thought I suggested adding a Deployment previously, but couldn't find my comment

docs/argo-server-sso-argocd.md

Comment on lines +112 to +113

		The following example shows how you can use environment variables to get your secrets. This is
		especially useful if you use CSI driver to get your secrets from, say, Azure Key Vault.

Member

agilgur5 Oct 12, 2023

Are you referencing the Secrets Store CSI Driver? That does require that secrets are created in k8s first before you can use env vars. It's a CSI Driver, so the default is to mount the secret, not set an env variable

Also in general, should be specific with what is being referenced in docs and avoid ambiguity

agilgur5 reviewed

View reviewed changes

Member

agilgur5 left a comment •

edited

I mentioned before, but I do think it would be good to split up the fix from the feature. That way we can cherry-pick the fix into 3.4.12 (#11851). I think that bug may have been hit many times in different issues in particular

agilgur5 requested changes

View reviewed changes

Member

agilgur5 left a comment

Some comments on the tests below

server/auth/types/claims_test.go

+              	})
+              }
+              func TestGetUserInfoGroup(t *testing.T) {

Member

agilgur5 Oct 12, 2023

Am I missing something or this test identical to the one above?

Member

agilgur5 Apr 26, 2024 •

edited

Looks like this one passes "group" in, I assume as part of the third fix/feature. Could probably be done within the same suite and should be named differently etc

server/auth/sso/sso_test.go

Comment on lines +165 to +166

		os.Setenv("ARGO_CLIENT_ID", "sso-client-id-value")
		os.Setenv("ARGO_CLIENT_SECRET", "sso-client-secret-value")

Member

agilgur5 Oct 12, 2023

you'll want to unset these at the end of the test, which is done in most other tests

Member

agilgur5 Apr 26, 2024 •

edited

Suggested change

      
            	os.Setenv("ARGO_CLIENT_ID", "sso-client-id-value")
          
            	os.Setenv("ARGO_CLIENT_SECRET", "sso-client-secret-value")
          
            	t.Setenv("ARGO_CLIENT_ID", "sso-client-id-value")
          
            	t.Setenv("ARGO_CLIENT_SECRET", "sso-client-secret-value")

there's a helper for that, t.Setenv

server/auth/sso/sso_test.go

+              	privateKeyBlobEncoded := base64.StdEncoding.EncodeToString(privateKeyBlob)
+              	assert.NotEmpty(t, privateKeyBlobEncoded)
+              	os.Setenv("ARGO_PRIVATE_KEY", privateKeyBlobEncoded)

Member

agilgur5 Oct 12, 2023 •

edited

same as above, unset this at the end

server/auth/sso/sso_test.go

+              	privateKeyBlob := x509.MarshalPKCS1PrivateKey(generatedKey)
+              	assert.NotEmpty(t, privateKeyBlob)
+              	privateKeyBlobEncoded := base64.StdEncoding.EncodeToString(privateKeyBlob)

Member

agilgur5 Oct 12, 2023

the encoding should be specified in the docs as well

server/auth/sso/sso_test.go

Comment on lines +201 to +206

+              	ctx := context.Background()
+              	_, err = fakeClient.Create(ctx, &apiv1.Secret{
+              		ObjectMeta: metav1.ObjectMeta{Name: secretName},
+              		Data:       map[string][]byte{},
+              	}, metav1.CreateOptions{})
+              	assert.NoError(t, err)

Member

agilgur5 Oct 12, 2023 •

edited

hmm, creating an empty PKCS1 secret that would fail normally relies on a good bit of indirection. that's confusing and is also not specified in a comment here either.

instead, we should be direct that no PKCS1 secret is created by asserting that a get finds no secret at the end

agilgur5 requested changes

View reviewed changes

Member

agilgur5 left a comment

Several comments on the source below.

The core env variable retrieval is mostly good, though there is one performance regression in the code (see below).

The GetUserInfoGroups changes require some iteration, there are some anti-patterns being used there. And I also did not expect that there was a third feature/fix to be included there/in this PR, a custom group claim on top of a custom user info path.

server/auth/sso/sso.go

+              func getSsoPrivateKey(ctx context.Context, c Config, secretsIf corev1.SecretInterface) (*rsa.PrivateKey, error) {
+              	privateKeyBlobEncoded := os.Getenv(c.PrivateKeyEnvName)
+              	log.Debug("privateKeyBlobEncoded: ", privateKeyBlobEncoded)

Member

agilgur5 Oct 12, 2023

Suggested change

log.Debug("privateKeyBlobEncoded: ", privateKeyBlobEncoded)

this is a secret, so we should never log it out, even in encoded form

server/auth/sso/sso.go

+              	privateKey, err := getSsoPrivateKey(ctx, c, secretsIf)
+              	if err != nil {
+              		return nil, fmt.Errorf("failed to parse private key. If you have already defined a Secret named %s, delete it and retry: %w", secretName, err)

Member

agilgur5 Oct 12, 2023

Suggested change

      
            		return nil, fmt.Errorf("failed to parse private key. If you have already defined a Secret named %s, delete it and retry: %w", secretName, err)
          
            		return nil, err

the formatted error already exists in the getSsoPrivateKey function. we shouldn't override it, especially as the other errors are more specific

server/auth/sso/sso.go

-              	ctx := context.Background()
-              	clientSecretObj, err := secretsIf.Get(ctx, c.ClientSecret.Name, metav1.GetOptions{})
+              	// Fallback to getting values from Kube Secrets

Member

agilgur5 Oct 12, 2023

Suggested change

      
            	// Fallback to getting values from Kube Secrets
          
            	// Fallback to getting value from Secret

since there is no integration with any specific providers, k8s Secrets are the default (and are native), no need to specify that.
it's a singular value and singular secret in this case

server/auth/sso/sso.go

-              	clientSecretObj, err := secretsIf.Get(ctx, c.ClientSecret.Name, metav1.GetOptions{})
+              	// Fallback to getting values from Kube Secrets
+              	secretObj, err := secretsIf.Get(ctx, c.ClientID.Name, metav1.GetOptions{})

Member

agilgur5 Oct 12, 2023

it looks like you're missing the logic that was previously on lines 126-127 -- if the clientId Secret is the same name as the clientSecret secret, it just uses that existing Secret.

that logic would only make a single network request

server/auth/sso/sso.go

+              		return "", fmt.Errorf("clientSecret empty")
+              	}
+              	// Fallback to getting values from Kube Secrets

Member

agilgur5 Oct 12, 2023 •

edited

Suggested change

      
            	// Fallback to getting values from Kube Secrets
          
            	// Fallback to getting value from Secret

same as above

server/auth/types/claims.go Show resolved Hide resolved

server/auth/types/claims.go

Comment on lines +128 to +129

		// If a custom group key was given, use that
		if customKeyName != "" {

Member

agilgur5 Oct 12, 2023

wait a minute, this is a third fix/feature entirely? this is not mentioned in the PR either...
this PR is definitely addressing a little too many concerns simultaneously... single-purpose PRs are best; they are easy to review, easy to revert, easy to trace. 3 purposes in one PR is a bit much...

as a leading indicator, 3 purposes do not fit in a PR title (not even 2 fit)

Member

agilgur5 Oct 13, 2023

Also your provider returns the groups with a different key even when it has a separate URL to retrieve them?

server/auth/types/claims.go

+              		err = json.Unmarshal(responseBody, &userInfoRawMap)
+              		if err != nil {
+              			return nil, fmt.Errorf("Could not marshall userInfo payload")

Member

agilgur5 Oct 12, 2023

Suggested change

      
            			return nil, fmt.Errorf("Could not marshall userInfo payload")
          
            			return nil, fmt.Errorf("Could not unmarshal userInfo payload")

typo?

server/auth/types/claims.go


		userInfo := UserInfo{}
		err = json.Unmarshal(responseBody, &userInfo)

Member

agilgur5 Oct 12, 2023

correct me if I'm wrong, but if the Groups field is empty, I believe this would fail. there's no omitempty in the struct.

server/auth/types/claims.go

Comment on lines +134 to +138

+              		var userInfoRawMap map[string]json.RawMessage
+              		err = json.Unmarshal(responseBody, &userInfoRawMap)
+              		if err != nil {
+              			return nil, fmt.Errorf("Could not marshall userInfo payload")

Member

agilgur5 Oct 13, 2023

if we change the typings a bit, we can consolidate this by combining it with the top block, which I think would be a lot neater than doing 2 separate unmarshals.

then the conditional logic is just grabbing the custom claim

agilgur5 unassigned terrytangyuan

agilgur5 mentioned this pull request

REQUEST: Promotion to Reviewer for @agilgur5 argoproj/argoproj#242

Closed

8 tasks

agilgur5 added the problem/more information needed label

Contributor

github-actions bot commented Jan 11, 2024

This PR has been automatically marked as stale because it has not had recent activity and needs further changes. It will be closed if no further activity occurs.

github-actions bot added the problem/stale label

agilgur5 self-assigned this

github-actions bot removed problem/stale problem/more information needed labels

agilgur5 mentioned this pull request

fix: insecureSkipVerify for GetUserInfoGroups #12982

Merged

agilgur5 reviewed

View reviewed changes

Member

agilgur5 left a comment •

edited

The fix for #10395, UserInfoGroups not having the httpClient, was properly completed in #12982

server/auth/sso/sso.go Show resolved Hide resolved

server/auth/types/claims.go Show resolved Hide resolved

server/auth/types/claims.go Show resolved Hide resolved

server/auth/types/claims_test.go

+              	})
+              }
+              func TestGetUserInfoGroup(t *testing.T) {

Member

agilgur5 Apr 26, 2024 •

edited

Looks like this one passes "group" in, I assume as part of the third fix/feature. Could probably be done within the same suite and should be named differently etc

server/auth/sso/sso_test.go

Comment on lines +165 to +166

		os.Setenv("ARGO_CLIENT_ID", "sso-client-id-value")
		os.Setenv("ARGO_CLIENT_SECRET", "sso-client-secret-value")

Member

agilgur5 Apr 26, 2024 •

edited

Suggested change

      
            	os.Setenv("ARGO_CLIENT_ID", "sso-client-id-value")
          
            	os.Setenv("ARGO_CLIENT_SECRET", "sso-client-secret-value")
          
            	t.Setenv("ARGO_CLIENT_ID", "sso-client-id-value")
          
            	t.Setenv("ARGO_CLIENT_SECRET", "sso-client-secret-value")

there's a helper for that, t.Setenv

agilgur5 added the problem/more information needed label

Contributor

github-actions bot commented May 16, 2024

This PR has been automatically marked as stale because it has not had recent activity and needs further changes. It will be closed if no further activity occurs.

github-actions bot added the problem/stale label

Contributor

github-actions bot commented May 30, 2024

This PR has been closed due to inactivity and lack of changes. If you would like to still work on this PR, please address the review comments and re-open.

github-actions bot closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment