From b7916dc1232e125931befd75c050d8bd7d3d4c4a Mon Sep 17 00:00:00 2001 From: Claudia Beresford Date: Wed, 16 Mar 2022 16:58:42 +0000 Subject: [PATCH] ref: Tidy up how we process OIDC config It was starting to look a little hairy in cmd/gitops-server/cmd/cmd.go --- cmd/gitops-server/cmd/cmd.go | 66 +++---------- pkg/server/auth/auth_test.go | 165 +++++++++++++++------------------ pkg/server/auth/server.go | 66 +++++++++---- pkg/server/auth/server_test.go | 19 ++-- 4 files changed, 144 insertions(+), 172 deletions(-) diff --git a/cmd/gitops-server/cmd/cmd.go b/cmd/gitops-server/cmd/cmd.go index 48ed6f5d36..679c6a2fc6 100644 --- a/cmd/gitops-server/cmd/cmd.go +++ b/cmd/gitops-server/cmd/cmd.go @@ -11,7 +11,6 @@ import ( "log" "net" "net/http" - "net/url" "os" "os/signal" "path/filepath" @@ -51,7 +50,7 @@ type Options struct { WatcherPort int Path string LoggingEnabled bool - OIDC OIDCAuthenticationOptions + OIDC auth.OIDCConfig NotificationControllerAddress string TLSCertFile string TLSKeyFile string @@ -59,16 +58,6 @@ type Options struct { MTLS bool } -// OIDCAuthenticationOptions contains the OIDC authentication options for the -// gitops-server -type OIDCAuthenticationOptions struct { - IssuerURL string - ClientID string - ClientSecret string - RedirectURL string - TokenDuration time.Duration -} - var options Options func NewCommand() *cobra.Command { @@ -181,62 +170,29 @@ func runCmd(cmd *cobra.Command, args []string) error { var authServer *auth.AuthServer - if server.AuthEnabled() { - var OIDCConfig auth.OIDCConfig + oidcConfig := options.OIDC - // If OIDC auth secret is not found use CLI parameters + if server.AuthEnabled() { + // If OIDC auth secret is found prefer that over CLI parameters var secret corev1.Secret if err := rawClient.Get(cmd.Context(), client.ObjectKey{ Namespace: v1alpha1.DefaultNamespace, Name: auth.OIDCAuthSecretName, - }, &secret); err != nil { - appConfig.Logger.Error(err, "OIDC auth secret not found") - - OIDCConfig.IssuerURL = options.OIDC.IssuerURL - OIDCConfig.ClientID = options.OIDC.ClientID - OIDCConfig.ClientSecret = options.OIDC.ClientSecret - OIDCConfig.RedirectURL = options.OIDC.RedirectURL - OIDCConfig.TokenDuration = options.OIDC.TokenDuration - } else { - OIDCConfig.IssuerURL = string(secret.Data["issuerURL"]) - OIDCConfig.ClientID = string(secret.Data["clientID"]) - OIDCConfig.ClientSecret = string(secret.Data["clientSecret"]) - OIDCConfig.RedirectURL = string(secret.Data["redirectURL"]) - - tokenDuration, err := time.ParseDuration(string(secret.Data["tokenDuration"])) - if err != nil { - appConfig.Logger.Error(err, "Invalid token duration") - tokenDuration = time.Hour - } - OIDCConfig.TokenDuration = tokenDuration + }, &secret); err == nil { + oidcConfig = auth.NewOIDCConfigFromSecret(secret) } - _, err := url.Parse(OIDCConfig.IssuerURL) + tsv, err := auth.NewHMACTokenSignerVerifier(oidcConfig.TokenDuration) if err != nil { - return fmt.Errorf("invalid issuer URL: %w", err) - } - - _, err = url.Parse(OIDCConfig.RedirectURL) - if err != nil { - return fmt.Errorf("invalid redirect URL: %w", err) + return fmt.Errorf("could not create HMAC token signer: %w", err) } - tsv, err := auth.NewHMACTokenSignerVerifier(OIDCConfig.TokenDuration) + authCfg, err := auth.NewAuthServerConfig(appConfig.Logger, oidcConfig, rawClient, tsv) if err != nil { - return fmt.Errorf("could not create HMAC token signer: %w", err) + return err } - srv, err := auth.NewAuthServer(cmd.Context(), appConfig.Logger, http.DefaultClient, - auth.AuthConfig{ - OIDCConfig: auth.OIDCConfig{ - IssuerURL: OIDCConfig.IssuerURL, - ClientID: OIDCConfig.ClientID, - ClientSecret: OIDCConfig.ClientSecret, - RedirectURL: OIDCConfig.RedirectURL, - TokenDuration: OIDCConfig.TokenDuration, - }, - }, rawClient, tsv, - ) + srv, err := auth.NewAuthServer(cmd.Context(), authCfg) if err != nil { return fmt.Errorf("could not create auth server: %w", err) } diff --git a/pkg/server/auth/auth_test.go b/pkg/server/auth/auth_test.go index fd4fd63a9b..411300965b 100644 --- a/pkg/server/auth/auth_test.go +++ b/pkg/server/auth/auth_test.go @@ -14,8 +14,8 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/go-logr/logr" "github.com/oauth2-proxy/mockoidc" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" + "github.com/onsi/gomega" + . "github.com/onsi/gomega" "github.com/weaveworks/weave-gitops/pkg/server/auth" "golang.org/x/crypto/bcrypt" corev1 "k8s.io/api/core/v1" @@ -24,44 +24,41 @@ import ( ) func TestWithAPIAuthReturns401ForUnauthenticatedRequests(t *testing.T) { - ctx := context.Background() + g := gomega.NewGomegaWithT(t) m, err := mockoidc.Run() - if err != nil { - t.Error("failed to create fake OIDC server") - } + g.Expect(err).NotTo(HaveOccurred()) - defer func() { + t.Cleanup(func() { _ = m.Shutdown() - }() + }) fake := m.Config() mux := http.NewServeMux() fakeKubernetesClient := ctrlclient.NewClientBuilder().Build() tokenSignerVerifier, err := auth.NewHMACTokenSignerVerifier(5 * time.Minute) - if err != nil { - t.Errorf("failed to create HMAC signer: %v", err) - } + g.Expect(err).NotTo(HaveOccurred()) - srv, err := auth.NewAuthServer(ctx, logr.Discard(), http.DefaultClient, - auth.AuthConfig{ - auth.OIDCConfig{ - IssuerURL: fake.Issuer, - ClientID: fake.ClientID, - ClientSecret: fake.ClientSecret, - RedirectURL: "", - TokenDuration: 20 * time.Minute, - }, - }, fakeKubernetesClient, tokenSignerVerifier) - if err != nil { - t.Error("failed to create auth config") + oidcCfg := auth.OIDCConfig{ + ClientID: fake.ClientID, + ClientSecret: fake.ClientSecret, + IssuerURL: fake.Issuer, } - _ = auth.RegisterAuthServer(mux, "/oauth2", srv, 1) + authCfg, err := auth.NewAuthServerConfig(logr.Discard(), oidcCfg, fakeKubernetesClient, tokenSignerVerifier) + g.Expect(err).NotTo(HaveOccurred()) + + srv, err := auth.NewAuthServer(context.Background(), authCfg) + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(auth.RegisterAuthServer(mux, "/oauth2", srv, 1)).To(Succeed()) s := httptest.NewServer(mux) - defer s.Close() + + t.Cleanup(func() { + s.Close() + }) // Set the correct redirect URL now that we have a server running srv.SetRedirectURL(s.URL + "/oauth2/callback") @@ -70,59 +67,52 @@ func TestWithAPIAuthReturns401ForUnauthenticatedRequests(t *testing.T) { req := httptest.NewRequest(http.MethodGet, s.URL, nil) auth.WithAPIAuth(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {}), srv, nil).ServeHTTP(res, req) - if res.Result().StatusCode != http.StatusUnauthorized { - t.Errorf("expected status of %d but got %d", http.StatusUnauthorized, res.Result().StatusCode) - } + g.Expect(res).To(HaveHTTPStatus(http.StatusUnauthorized)) // Test out the publicRoutes res = httptest.NewRecorder() req = httptest.NewRequest(http.MethodGet, s.URL+"/v1/featureflags", nil) auth.WithAPIAuth(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {}), srv, []string{"/v1/featureflags"}).ServeHTTP(res, req) - if res.Result().StatusCode != http.StatusOK { - t.Errorf("expected status of %d but got %d", http.StatusUnauthorized, res.Result().StatusCode) - } + g.Expect(res).To(HaveHTTPStatus(http.StatusOK)) } func TestOauth2FlowRedirectsToOIDCIssuerForUnauthenticatedRequests(t *testing.T) { - ctx := context.Background() + g := gomega.NewGomegaWithT(t) m, err := mockoidc.Run() - if err != nil { - t.Error("failed to create fake OIDC server") - } + g.Expect(err).NotTo(HaveOccurred()) - defer func() { + t.Cleanup(func() { _ = m.Shutdown() - }() + }) fake := m.Config() mux := http.NewServeMux() fakeKubernetesClient := ctrlclient.NewClientBuilder().Build() tokenSignerVerifier, err := auth.NewHMACTokenSignerVerifier(5 * time.Minute) - if err != nil { - t.Errorf("failed to create HMAC signer: %v", err) - } + g.Expect(err).NotTo(HaveOccurred()) - srv, err := auth.NewAuthServer(ctx, logr.Discard(), http.DefaultClient, - auth.AuthConfig{ - auth.OIDCConfig{ - IssuerURL: fake.Issuer, - ClientID: fake.ClientID, - ClientSecret: fake.ClientSecret, - RedirectURL: "", - TokenDuration: 20 * time.Minute, - }, - }, fakeKubernetesClient, tokenSignerVerifier) - if err != nil { - t.Error("failed to create auth config") + oidcCfg := auth.OIDCConfig{ + ClientID: fake.ClientID, + ClientSecret: fake.ClientSecret, + IssuerURL: fake.Issuer, } - _ = auth.RegisterAuthServer(mux, "/oauth2", srv, 1) + authCfg, err := auth.NewAuthServerConfig(logr.Discard(), oidcCfg, fakeKubernetesClient, tokenSignerVerifier) + g.Expect(err).NotTo(HaveOccurred()) + + srv, err := auth.NewAuthServer(context.Background(), authCfg) + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(auth.RegisterAuthServer(mux, "/oauth2", srv, 1)).To(Succeed()) s := httptest.NewServer(mux) - defer s.Close() + + t.Cleanup(func() { + s.Close() + }) // Set the correct redirect URL now that we have a server running redirectURL := s.URL + "/oauth2/callback" @@ -132,31 +122,30 @@ func TestOauth2FlowRedirectsToOIDCIssuerForUnauthenticatedRequests(t *testing.T) req := httptest.NewRequest(http.MethodGet, s.URL, nil) srv.OAuth2Flow().ServeHTTP(res, req) - if res.Result().StatusCode != http.StatusSeeOther { - t.Errorf("expected status of %d but got %d", http.StatusSeeOther, res.Result().StatusCode) - } + g.Expect(res).To(HaveHTTPStatus(http.StatusSeeOther)) authCodeURL := fmt.Sprintf("%s?client_id=%s&redirect_uri=%s&response_type=code&scope=%s", m.AuthorizationEndpoint(), fake.ClientID, url.QueryEscape(redirectURL), strings.Join([]string{"profile", oidc.ScopeOpenID, oidc.ScopeOfflineAccess, "email"}, "+")) - if !strings.HasPrefix(res.Result().Header.Get("Location"), authCodeURL) { - t.Errorf("expected Location header URL to include scopes %s but does not: %s", authCodeURL, res.Result().Header.Get("Location")) - } + g.Expect(res.Result().Header.Get("Location")).To(ContainSubstring(authCodeURL)) } func TestIsPublicRoute(t *testing.T) { - assert.True(t, auth.IsPublicRoute(&url.URL{Path: "/foo"}, []string{"/foo"})) - assert.False(t, auth.IsPublicRoute(&url.URL{Path: "foo"}, []string{"/foo"})) - assert.False(t, auth.IsPublicRoute(&url.URL{Path: "/foob"}, []string{"/foo"})) + g := gomega.NewGomegaWithT(t) + + g.Expect(auth.IsPublicRoute(&url.URL{Path: "/foo"}, []string{"/foo"})).To(BeTrue()) + g.Expect(auth.IsPublicRoute(&url.URL{Path: "foo"}, []string{"/foo"})).To(BeFalse()) + g.Expect(auth.IsPublicRoute(&url.URL{Path: "/foob"}, []string{"/foo"})).To(BeFalse()) } func TestRateLimit(t *testing.T) { - ctx := context.Background() + g := gomega.NewGomegaWithT(t) + mux := http.NewServeMux() tokenSignerVerifier, err := auth.NewHMACTokenSignerVerifier(5 * time.Minute) - require.NoError(t, err) + g.Expect(err).NotTo(HaveOccurred()) password := "my-secret-password" hashed, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) - require.NoError(t, err) + g.Expect(err).NotTo(HaveOccurred()) hashedSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -169,39 +158,33 @@ func TestRateLimit(t *testing.T) { } fakeKubernetesClient := ctrlclient.NewClientBuilder().WithObjects(hashedSecret).Build() - srv, err := auth.NewAuthServer(ctx, logr.Discard(), http.DefaultClient, - auth.AuthConfig{ - auth.OIDCConfig{ - TokenDuration: 20 * time.Minute, - }, - }, fakeKubernetesClient, tokenSignerVerifier) - require.NoError(t, err) - err = auth.RegisterAuthServer(mux, "/oauth2", srv, 1) - require.NoError(t, err) + oidcCfg := auth.OIDCConfig{} + + authCfg, err := auth.NewAuthServerConfig(logr.Discard(), oidcCfg, fakeKubernetesClient, tokenSignerVerifier) + g.Expect(err).NotTo(HaveOccurred()) + + srv, err := auth.NewAuthServer(context.Background(), authCfg) + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(auth.RegisterAuthServer(mux, "/oauth2", srv, 1)).To(Succeed()) s := httptest.NewServer(mux) - defer s.Close() - res1, err := http.Post(s.URL+"/oauth2/sign_in", "application/json", bytes.NewReader([]byte(`{"password":"my-secret-password"}`))) - require.NoError(t, err) + t.Cleanup(func() { + s.Close() + }) - if res1.StatusCode != http.StatusOK { - t.Errorf("expected 200 but got %d instead", res1.StatusCode) - } + res1, err := http.Post(s.URL+"/oauth2/sign_in", "application/json", bytes.NewReader([]byte(`{"password":"my-secret-password"}`))) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res1).To(HaveHTTPStatus(http.StatusOK)) res2, err := http.Post(s.URL+"/oauth2/sign_in", "application/json", bytes.NewReader([]byte(`{"password":"my-secret-password"}`))) - require.NoError(t, err) - - if res2.StatusCode != http.StatusTooManyRequests { - t.Errorf("expected 429 but got %d instead", res2.StatusCode) - } + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res2).To(HaveHTTPStatus(http.StatusTooManyRequests)) time.Sleep(time.Second) res3, err := http.Post(s.URL+"/oauth2/sign_in", "application/json", bytes.NewReader([]byte(`{"password":"my-secret-password"}`))) - require.NoError(t, err) - - if res3.StatusCode != http.StatusOK { - t.Errorf("expected 200 but got %d instead", res3.StatusCode) - } + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res3).To(HaveHTTPStatus(http.StatusOK)) } diff --git a/pkg/server/auth/server.go b/pkg/server/auth/server.go index 0ccfbcac10..73fea72015 100644 --- a/pkg/server/auth/server.go +++ b/pkg/server/auth/server.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "net/http" + "net/url" "time" "github.com/coreos/go-oidc/v3/oidc" @@ -36,17 +37,17 @@ type OIDCConfig struct { // AuthConfig is used to configure an AuthServer. type AuthConfig struct { - OIDCConfig -} - -// AuthServer interacts with an OIDC issuer to handle the OAuth2 process flow. -type AuthServer struct { logger logr.Logger client *http.Client - provider *oidc.Provider - config AuthConfig kubernetesClient ctrlclient.Client tokenSignerVerifier TokenSignerVerifier + config OIDCConfig +} + +// AuthServer interacts with an OIDC issuer to handle the OAuth2 process flow. +type AuthServer struct { + AuthConfig + provider *oidc.Provider } // LoginRequest represents the data submitted by client when the auth flow (non-OIDC) is used. @@ -61,27 +62,56 @@ type UserInfo struct { Groups []string `json:"groups"` } +func NewOIDCConfigFromSecret(secret corev1.Secret) OIDCConfig { + cfg := OIDCConfig{ + IssuerURL: string(secret.Data["issuerURL"]), + ClientID: string(secret.Data["clientID"]), + ClientSecret: string(secret.Data["clientSecret"]), + RedirectURL: string(secret.Data["redirectURL"]), + } + + tokenDuration, err := time.ParseDuration(string(secret.Data["tokenDuration"])) + if err != nil { + tokenDuration = time.Hour + } + + cfg.TokenDuration = tokenDuration + + return cfg +} + +func NewAuthServerConfig(logger logr.Logger, oidcCfg OIDCConfig, kubernetesClient ctrlclient.Client, tsv TokenSignerVerifier) (AuthConfig, error) { + if _, err := url.Parse(oidcCfg.IssuerURL); err != nil { + return AuthConfig{}, fmt.Errorf("invalid issuer URL: %w", err) + } + + if _, err := url.Parse(oidcCfg.RedirectURL); err != nil { + return AuthConfig{}, fmt.Errorf("invalid redirect URL: %w", err) + } + + return AuthConfig{ + logger: logger, + client: http.DefaultClient, + kubernetesClient: kubernetesClient, + tokenSignerVerifier: tsv, + config: oidcCfg, + }, nil +} + // NewAuthServer creates a new AuthServer object. -func NewAuthServer(ctx context.Context, logger logr.Logger, client *http.Client, config AuthConfig, kubernetesClient ctrlclient.Client, tokenSignerVerifier TokenSignerVerifier) (*AuthServer, error) { +func NewAuthServer(ctx context.Context, cfg AuthConfig) (*AuthServer, error) { var provider *oidc.Provider - if config.IssuerURL != "" { + if cfg.config.IssuerURL != "" { var err error - provider, err = oidc.NewProvider(ctx, config.IssuerURL) + provider, err = oidc.NewProvider(ctx, cfg.config.IssuerURL) if err != nil { return nil, fmt.Errorf("could not create provider: %w", err) } } - return &AuthServer{ - logger: logger, - client: client, - provider: provider, - config: config, - kubernetesClient: kubernetesClient, - tokenSignerVerifier: tokenSignerVerifier, - }, nil + return &AuthServer{cfg, provider}, nil } // SetRedirectURL is used to set the redirect URL. This is meant to be used diff --git a/pkg/server/auth/server_test.go b/pkg/server/auth/server_test.go index be5088fa7f..73e9aca68a 100644 --- a/pkg/server/auth/server_test.go +++ b/pkg/server/auth/server_test.go @@ -599,7 +599,7 @@ func TestLogoutWithWrongMethod(t *testing.T) { g.Expect(w.Result().StatusCode).To(Equal(http.StatusMethodNotAllowed)) } -func makeAuthServer(t *testing.T, client ctrlclient.Client, tokenSignerVerifier auth.TokenSignerVerifier, disableProvider bool) (*auth.AuthServer, *mockoidc.MockOIDC) { +func makeAuthServer(t *testing.T, client ctrlclient.Client, tsv auth.TokenSignerVerifier, disableProvider bool) (*auth.AuthServer, *mockoidc.MockOIDC) { t.Helper() g := gomega.NewGomegaWithT(t) @@ -616,13 +616,16 @@ func makeAuthServer(t *testing.T, client ctrlclient.Client, tokenSignerVerifier cfg.Issuer = "" } - s, err := auth.NewAuthServer(context.Background(), logr.Discard(), http.DefaultClient, auth.AuthConfig{ - OIDCConfig: auth.OIDCConfig{ - ClientID: cfg.ClientID, - ClientSecret: cfg.ClientSecret, - IssuerURL: cfg.Issuer, - }, - }, client, tokenSignerVerifier) + oidcCfg := auth.OIDCConfig{ + ClientID: cfg.ClientID, + ClientSecret: cfg.ClientSecret, + IssuerURL: cfg.Issuer, + } + + authCfg, err := auth.NewAuthServerConfig(logr.Discard(), oidcCfg, client, tsv) + g.Expect(err).NotTo(HaveOccurred()) + + s, err := auth.NewAuthServer(context.Background(), authCfg) g.Expect(err).NotTo(HaveOccurred()) return s, m