djpbessems/ContainerImage.Pinniped
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Be extra defensive and don't lookup dynamic client ID's lacking prefix

Browse Source
This commit is contained in:
Ryan Richard 2022-07-22 15:19:19 -07:00
parent 2f1966dbc8
commit 88f611d31a
3 changed files with 40 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
internal/oidc/clientregistry/clientregistry.go
View File

@ -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) {

25
internal/oidc/clientregistry/clientregistry_test.go
View File

@ -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) {

2
internal/supervisor/server/server.go
View File

@ -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