From 88f611d31a07382565ca78485980023ac8ef2145 Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Fri, 22 Jul 2022 15:19:19 -0700
Subject: [PATCH] Be extra defensive and don't lookup dynamic client ID's
 lacking prefix

---
 .../oidc/clientregistry/clientregistry.go     | 17 ++++++++++---
 .../clientregistry/clientregistry_test.go     | 25 +++++++++++++++++++
 internal/supervisor/server/server.go          |  2 +-
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/internal/oidc/clientregistry/clientregistry.go b/internal/oidc/clientregistry/clientregistry.go
index 0de96cfa..90451784 100644
--- a/internal/oidc/clientregistry/clientregistry.go
+++ b/internal/oidc/clientregistry/clientregistry.go
@@ -7,6 +7,7 @@ package clientregistry
 import (
 	"context"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/coreos/go-oidc/v3/oidc"
@@ -21,8 +22,12 @@ import (
 	"go.pinniped.dev/internal/plog"
 )
 
-// PinnipedCLIClientID is the client ID of the statically defined public OIDC client which is used by the CLI.
-const PinnipedCLIClientID = "pinniped-cli"
+const (
+	// PinnipedCLIClientID is the client ID of the statically defined public OIDC client which is used by the CLI.
+	PinnipedCLIClientID = "pinniped-cli"
+
+	requiredOIDCClientPrefix = "client.oauth.pinniped.dev-"
+)
 
 // Client represents a Pinniped OAuth/OIDC client. It can be the static pinniped-cli client
 // or a dynamic client defined by an OIDCClient CR.
@@ -37,7 +42,7 @@ var (
 	_ fosite.ResponseModeClient  = (*Client)(nil)
 )
 
-func (c Client) GetResponseModes() []fosite.ResponseModeType {
+func (c *Client) GetResponseModes() []fosite.ResponseModeType {
 	if c.ID == PinnipedCLIClientID {
 		// The pinniped-cli client supports "" (unspecified), "query", and "form_post" response modes.
 		return []fosite.ResponseModeType{fosite.ResponseModeDefault, fosite.ResponseModeQuery, fosite.ResponseModeFormPost}
@@ -78,6 +83,12 @@ func (m *ClientManager) GetClient(ctx context.Context, id string) (fosite.Client
 		return PinnipedCLI(), nil
 	}
 
+	if !strings.HasPrefix(id, requiredOIDCClientPrefix) {
+		// It shouldn't really be possible to find this OIDCClient because the OIDCClient CRD validates the name prefix
+		// upon create, but just in case, don't even try to lookup clients which lack the required name prefix.
+		return nil, fosite.ErrNotFound.WithDescription("no such client")
+	}
+
 	// Try to look up an OIDCClient with the given client ID (which will be the Name of the OIDCClient).
 	oidcClient, err := m.oidcClientsClient.Get(ctx, id, v1.GetOptions{})
 	if errors.IsNotFound(err) {
diff --git a/internal/oidc/clientregistry/clientregistry_test.go b/internal/oidc/clientregistry/clientregistry_test.go
index 77ab18b9..b0b2e01e 100644
--- a/internal/oidc/clientregistry/clientregistry_test.go
+++ b/internal/oidc/clientregistry/clientregistry_test.go
@@ -125,6 +125,31 @@ func TestClientManager(t *testing.T) {
 				require.Nil(t, got)
 			},
 		},
+		{
+			name: "find a dynamic client which somehow does not have the required prefix in its name, just in case, although should not be possible since prefix is a validation on the CRD",
+			oidcClients: []*configv1alpha1.OIDCClient{
+				{
+					ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "does-not-have-prefix", Generation: 1234, UID: testUID},
+					Spec: configv1alpha1.OIDCClientSpec{
+						AllowedGrantTypes:   []configv1alpha1.GrantType{"authorization_code", "urn:ietf:params:oauth:grant-type:token-exchange", "refresh_token"},
+						AllowedScopes:       []configv1alpha1.Scope{"openid", "offline_access", "pinniped:request-audience", "username", "groups"},
+						AllowedRedirectURIs: []configv1alpha1.RedirectURI{"http://localhost:80", "https://foobar.com/callback"},
+					},
+				},
+			},
+			secrets: []*corev1.Secret{
+				testutil.OIDCClientSecretStorageSecretForUID(t, testNamespace, testUID, []string{testutil.HashedPassword1AtSupervisorMinCost, testutil.HashedPassword2AtSupervisorMinCost}),
+			},
+			run: func(t *testing.T, subject *ClientManager) {
+				got, err := subject.GetClient(ctx, "does-not-have-prefix")
+				require.Error(t, err)
+				require.Nil(t, got)
+				rfcErr := fosite.ErrorToRFC6749Error(err)
+				require.NotNil(t, rfcErr)
+				require.Equal(t, rfcErr.CodeField, 404)
+				require.Equal(t, rfcErr.GetDescription(), "no such client")
+			},
+		},
 		{
 			name: "when there is an unexpected error getting the OIDCClient",
 			addSupervisorReactions: func(client *supervisorfake.Clientset) {
diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go
index ac71376a..76a034ff 100644
--- a/internal/supervisor/server/server.go
+++ b/internal/supervisor/server/server.go
@@ -439,7 +439,7 @@ func runSupervisor(ctx context.Context, podInfo *downward.PodInfo, cfg *supervis
 		dynamicUpstreamIDPProvider,
 		&secretCache,
 		clientWithoutLeaderElection.Kubernetes.CoreV1().Secrets(serverInstallationNamespace), // writes to kube storage are allowed for non-leaders
-		clientWithoutLeaderElection.PinnipedSupervisor.ConfigV1alpha1().OIDCClients(serverInstallationNamespace),
+		client.PinnipedSupervisor.ConfigV1alpha1().OIDCClients(serverInstallationNamespace),
 	)
 
 	// Get the "real" name of the client secret supervisor API group (i.e., the API group name with the