Allow the oidc secret to be named other than 'oidc-auth' and improve logging #2379

SamLR · 2022-06-29T15:25:05Z

What changed?

Added --oidc-secret-name flag to allow alternate secrets to be provided to the server
Enabled creation of the oidc-auth secret as part of the helm chart
Improved logging around OIDC start up
Fixed some typos in the docs

Why was this change made?

Because I ran into problems relating to these issues several times whilst setting up Dex (particularly around the logging not providing feedback on why OIDC wasn't working). The additional flag was mostly a "while I'm here".

This should also mean that providing OIDC configuration via CLI actually works now (I believe it didn't previously because featureflag.go would fail to find a secret and then mark OIDC_AUTH as disabled).

How was this change implemented?

Mostly through adding a new flag. The only debatable change was adding a global variable to the pkg/server/auth/server.go (isOidcEnabled) which replaces the featureflag.go secret lookup. I think this is a preferable method as the state of this flag is exposed by a function in the server.go file and should be the concern of it (rather than featureflag). Alternates involved threading a lot more state through the system as featureflag and the authserver are fairly disjoint. I think in theory the information could have been passed by context but that felt even messier.

How did you validate the change?

I added some tests and ran it locally with dex in a variety of configurations (default oidc-auth secret, oidc passed by CLI and oidc from a different secret). All worked as expected.

The helm chart change was tested by inspection of the output of helm template

Release notes

No

Documentation Changes

Fixed some types which gave incorrectly capitalised oidc parameters in the table and listed the namespace to use as the legacy wego-system

bigkevmcd

Thanks for fixing the docs.

bigkevmcd · 2022-06-29T15:31:00Z

pkg/server/auth/server.go

 	}

 	return &AuthServer{cfg, provider}, nil
 }

+// SetOidcEnabled is intended for test use only
+func SetOidcEnabled(newVal bool) {
+	isOidcEnabled = newVal


https://github.com/golang/go/wiki/CodeReviewComments#initialisms

This is not how we give feedback.

We give a suggestion, and explain to the author why we prefer our way to theirs. We do not slam a "RTFM" in their heads.

I'm happy to change to isOIDCEnabled but IMO it's less readable and doesn't match the existing method names

A grep of the code finds that OIDC is the prevailing usage in the code, except for this...

$ rg Oidc pkg/server/auth/server.go 29:var isOidcEnabled bool = false 115: // Set isOidcEnabled now we have a valid provider 116: isOidcEnabled = true 122:// SetOidcEnabled is intended for test use only 123:func SetOidcEnabled(newVal bool) { 124: isOidcEnabled = newVal 127:func OidcEnabled() bool { 128: return isOidcEnabled 138: return OidcEnabled() 172: if !OidcEnabled() { 361: if !OidcEnabled() { core/server/featureflags.go 40: flags["OIDC_AUTH"] = strconv.FormatBool(serverauth.OidcEnabled()) core/server/featureflags_test.go 84: auth.SetOidcEnabled(tt.oidcEnabled)

bigkevmcd · 2022-06-29T15:34:48Z

pkg/server/auth/server.go

@@ -109,19 +112,30 @@ func NewAuthServer(ctx context.Context, cfg AuthConfig) (*AuthServer, error) {
 		if err != nil {
 			return nil, fmt.Errorf("could not create provider: %w", err)
 		}
+		// Set isOidcEnabled now we have a valid provider
+		isOidcEnabled = true


Why not call SetOidcEnabled() here?

because SetOidcEnabled is intended for testing only and frankly shouldn't exist. I'd like to minimise it's usage to test files only so if we can remove it it's as easy to remove as possible

bigkevmcd · 2022-06-29T15:39:55Z

cmd/gitops-server/cmd/cmd.go

 		oidcConfig = auth.NewOIDCConfigFromSecret(secret)
+	} else if err != nil {
+		log.V(logger.LogLevelDebug).Info("Could not read OIDC secret", "secretName", options.OIDCSecret, "error", err)


Maybe use logger.LogLevelError ?

We should also try and standardise on the log string format.

Some log messages https://github.com/weaveworks/weave-gitops/blob/main/pkg/server/auth/jwt.go#L38 and https://github.com/weaveworks/weave-gitops/blob/main/pkg/helm/watcher/controller/helm_watcher.go#L77 for example are "initial lower-case" and others, like this https://github.com/weaveworks/weave-gitops/blob/main/pkg/server/auth/server.go#L291 are "initial upper-case" (that file is a bit of a mess if I'm honest).

We should also try and standardise on the log string format.

Sure. Out of scope for this PR.

Maybe use logger.LogLevelError ?

It's not an error that you haven't set up OIDC login.

Maybe use logger.LogLevelError ?

It's not an error that you haven't set up OIDC login.

This depends on the nature of the error, yes, if the secret is not found, it's "Info", but if it's not, it's perhaps "Error"?

bigkevmcd · 2022-06-29T15:43:24Z

cmd/gitops-server/cmd/cmd.go

+		log.V(logger.LogLevelDebug).Info("Could not read OIDC secret", "secretName", options.OIDCSecret, "error", err)
+	}
+
+	if len(oidcConfig.ClientSecret) > 0 {


It's usually easier to read oidcConfig.ClientSecret != ""

It's easier to understand what ClientSecret is (and they compile to the same code internally), and this is the style used in Effective Go https://go.dev/doc/effective_go

ozamosi · 2022-06-29T16:10:53Z

cmd/gitops-server/cmd/cmd.go

 		oidcConfig = auth.NewOIDCConfigFromSecret(secret)
+	} else if err != nil {
+		log.V(logger.LogLevelDebug).Info("Could not read OIDC secret", "secretName", options.OIDCSecret, "error", err)


We should also try and standardise on the log string format.

Sure. Out of scope for this PR.

ozamosi · 2022-06-29T16:12:49Z

pkg/server/auth/server.go

 	}

 	return &AuthServer{cfg, provider}, nil
 }

+// SetOidcEnabled is intended for test use only
+func SetOidcEnabled(newVal bool) {
+	isOidcEnabled = newVal


This is not how we give feedback.

We give a suggestion, and explain to the author why we prefer our way to theirs. We do not slam a "RTFM" in their heads.

ozamosi · 2022-06-29T16:17:30Z

cmd/gitops-server/cmd/cmd.go

 		oidcConfig = auth.NewOIDCConfigFromSecret(secret)
+	} else if err != nil {
+		log.V(logger.LogLevelDebug).Info("Could not read OIDC secret", "secretName", options.OIDCSecret, "error", err)


Maybe use logger.LogLevelError ?

It's not an error that you haven't set up OIDC login.

ozamosi · 2022-06-29T16:18:56Z

core/server/featureflags.go

-	} else {
-		flags["OIDC_AUTH"] = "true"
-	}
+	flags["OIDC_AUTH"] = strconv.FormatBool(serverauth.OidcEnabled())


This makes me so happy to see 🎉

When setting up with OIDC there's not much feedback that you've actually got the config right or on where it's being read from. This gives a bit more information and some visibility as to what's been read. The `ClientSecretLength` is logged rather than the secret itself to avoid leaking secrets. The other parameters are not considered sensitive.

Make the secret name configurable. It is not unreasonable to assume that the user may have other secrets with the nane 'oidc-auth', or may want to name their secret something else (for example if their secrets are generated via some automation process they may not be able to set it). To enable this the `auth/server.go` package has been made the source of truth for whether OIDC is enabled. This means the featureflag package no longer checks for the `oidc-auth` secret, decoupling it from that system somewhat. Long term we should make the public `OidcEnabled` function the preferred way of testing this rather than ther server method (this wasn't done here as it wasn't obvious how to do this in `pkg/server/auth/auth.go`)

This is kind of ugly (expose the package-wide `isOidcEnabled` flag via a function) but I can't think of a cleaner way to do it that doesn't involve mocking most of the `pkg/server/auth` and really feature flags should be about flags so I'll stick with it.

The docs had the incorrect parameters for the `oidc-auth` secret (the table had the first character capitalised). Docs also listed that the secret should be created in the `wego-system` (rather than flux-system)

`len(foo) > 0` -> `foo != ""` `isOidcEnabled` -> `isOIDCEnabled` https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Apparently `isOIDCEnabled` wasn't being reset between runs. Now any time we use `makeAuthServer` in make sure the auth package is initialised with it set to false (as it should be).

SamLR requested a review from ozamosi June 29, 2022 15:26

bigkevmcd reviewed Jun 29, 2022

View reviewed changes

ozamosi approved these changes Jun 29, 2022

View reviewed changes

Sam Cook added 6 commits June 30, 2022 11:50

Make featureflag_tests pass

4a89e7e

This is kind of ugly (expose the package-wide `isOidcEnabled` flag via a function) but I can't think of a cleaner way to do it that doesn't involve mocking most of the `pkg/server/auth` and really feature flags should be about flags so I'll stick with it.

Add more logging

8fda635

Create the oidc-auth secret via the helm chart

bd947df

Fix docs on oidc parameters

8fbe82d

The docs had the incorrect parameters for the `oidc-auth` secret (the table had the first character capitalised). Docs also listed that the secret should be created in the `wego-system` (rather than flux-system)

SamLR force-pushed the oidc-fixes-and-notes branch from 2fcc257 to d060d89 Compare June 30, 2022 13:08

SamLR added the type/enhancement New feature or request label Jun 30, 2022

SamLR force-pushed the oidc-fixes-and-notes branch from d060d89 to 9eb3bf9 Compare June 30, 2022 15:33

Fixes for Kevin

2de8ec4

`len(foo) > 0` -> `foo != ""` `isOidcEnabled` -> `isOIDCEnabled` https://github.com/golang/go/wiki/CodeReviewComments#initialisms

SamLR force-pushed the oidc-fixes-and-notes branch from 9eb3bf9 to 50608e3 Compare June 30, 2022 16:00

Fix auth/server_tests.go

9513892

Apparently `isOIDCEnabled` wasn't being reset between runs. Now any time we use `makeAuthServer` in make sure the auth package is initialised with it set to false (as it should be).

SamLR force-pushed the oidc-fixes-and-notes branch from 50608e3 to 9513892 Compare June 30, 2022 17:03

Merge branch 'main' into oidc-fixes-and-notes

8b5eef2

SamLR merged commit 213cc50 into main Jul 1, 2022

SamLR deleted the oidc-fixes-and-notes branch July 1, 2022 10:02

This was referenced Jul 26, 2022

Updates for 0.9.1-rc.2 #2491

Merged

Updates for 0.9.1 #2500

Merged

weave-gitops-bot mentioned this pull request Aug 10, 2022

Updates for 0.9.2-rc.1 #2578

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the oidc secret to be named other than 'oidc-auth' and improve logging #2379

Allow the oidc secret to be named other than 'oidc-auth' and improve logging #2379

SamLR commented Jun 29, 2022

bigkevmcd left a comment

bigkevmcd Jun 29, 2022

ozamosi Jun 29, 2022

SamLR Jun 30, 2022

bigkevmcd Jun 30, 2022

bigkevmcd Jun 29, 2022

SamLR Jun 30, 2022

bigkevmcd Jun 29, 2022

bigkevmcd Jun 29, 2022

ozamosi Jun 29, 2022

ozamosi Jun 29, 2022

bigkevmcd Jun 29, 2022 •

edited

bigkevmcd Jun 29, 2022

ozamosi Jun 29, 2022

ozamosi Jun 29, 2022

ozamosi Jun 29, 2022

ozamosi Jun 29, 2022

Allow the oidc secret to be named other than 'oidc-auth' and improve logging #2379

Allow the oidc secret to be named other than 'oidc-auth' and improve logging #2379

Conversation

SamLR commented Jun 29, 2022

bigkevmcd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigkevmcd Jun 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigkevmcd Jun 29, 2022 •

edited