Finished tests for pkce storage and added it to kubestorage

- Also fixed some lint errors with v1.33.0 of the linter

Signed-off-by: Margo Crawford <margaretc@vmware.com>

go.mod

@@ -18,6 +18,7 @@ require (
 	github.com/gorilla/securecookie v1.1.1
 	github.com/ory/fosite v0.35.1
 	github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4
+	github.com/pkg/errors v0.9.1
 	github.com/sclevine/agouti v3.0.0+incompatible
 	github.com/sclevine/spec v1.4.0
 	github.com/spf13/cobra v1.0.0

internal/fositestorage/authorizationcode/authorizationcode.go

@@ -16,10 +16,10 @@ import (
 
 	"go.pinniped.dev/internal/constable"
 	"go.pinniped.dev/internal/crud"
+	"go.pinniped.dev/internal/fositestorage"
 )
 
 const (
-	ErrInvalidAuthorizeRequestType    = constable.Error("authorization request must be of type fosite.Request")
 	ErrInvalidAuthorizeRequestData    = constable.Error("authorization request data must not be nil")
 	ErrInvalidAuthorizeRequestVersion = constable.Error("authorization request data has wrong version")
 
@@ -46,7 +46,7 @@ func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, s
 	// This conversion assumes that we do not wrap the default type in any way
 	// i.e. we use the default fosite.OAuth2Provider.NewAuthorizeRequest implementation
 	// note that because this type is serialized and stored in Kube, we cannot easily change the implementation later
-	request, err := validateAndExtractAuthorizeRequest(requester)
+	request, err := fositestorage.ValidateAndExtractAuthorizeRequest(requester)
 	if err != nil {
 		return err
 	}
@@ -140,22 +140,6 @@ func NewValidEmptyAuthorizeCodeSession() *AuthorizeCodeSession {
 	}
 }
 
-func validateAndExtractAuthorizeRequest(requester fosite.Requester) (*fosite.Request, error) {
-	request, ok1 := requester.(*fosite.Request)
-	if !ok1 {
-		return nil, ErrInvalidAuthorizeRequestType
-	}
-	_, ok2 := request.Client.(*fosite.DefaultOpenIDConnectClient)
-	_, ok3 := request.Session.(*openid.DefaultSession)
-
-	valid := ok2 && ok3
-	if !valid {
-		return nil, ErrInvalidAuthorizeRequestType
-	}
-
-	return request, nil
-}
-
 var _ interface {
 	Is(error) bool
 	Unwrap() error

internal/fositestorage/authorizationcode/authorizationcode_test.go

@@ -25,6 +25,8 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes/fake"
 	coretesting "k8s.io/client-go/testing"
+
+	"go.pinniped.dev/internal/fositestorage"
 )
 
 func TestAuthorizeCodeStorage(t *testing.T) {
@@ -188,7 +190,7 @@ func TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession(t *testing.T) {
 	validSession := NewValidEmptyAuthorizeCodeSession()
 
 	// sanity check our valid session
-	extractedRequest, err := validateAndExtractAuthorizeRequest(validSession.Request)
+	extractedRequest, err := fositestorage.ValidateAndExtractAuthorizeRequest(validSession.Request)
 	require.NoError(t, err)
 	require.Equal(t, validSession.Request, extractedRequest)
 

internal/fositestorage/fositestorage.go

@@ -0,0 +1,34 @@
+// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package fositestorage
+
+import (
+	"github.com/ory/fosite"
+	"github.com/ory/fosite/handler/openid"
+
+	"go.pinniped.dev/internal/constable"
+)
+
+const (
+	ErrInvalidRequestType = constable.Error("requester must be of type fosite.Request")
+	ErrInvalidClientType  = constable.Error("requester's client must be of type fosite.DefaultOpenIDConnectClient")
+	ErrInvalidSessionType = constable.Error("requester's session must be of type openid.DefaultSession")
+)
+
+func ValidateAndExtractAuthorizeRequest(requester fosite.Requester) (*fosite.Request, error) {
+	request, ok1 := requester.(*fosite.Request)
+	if !ok1 {
+		return nil, ErrInvalidRequestType
+	}
+	_, ok2 := request.Client.(*fosite.DefaultOpenIDConnectClient)
+	if !ok2 {
+		return nil, ErrInvalidClientType
+	}
+	_, ok3 := request.Session.(*openid.DefaultSession)
+	if !ok3 {
+		return nil, ErrInvalidSessionType
+	}
+
+	return request, nil
+}

internal/fositestorage/pkce/pkce.go

@@ -10,14 +10,17 @@ import (
 	"github.com/ory/fosite"
 	"github.com/ory/fosite/handler/openid"
 	"github.com/ory/fosite/handler/pkce"
+	"k8s.io/apimachinery/pkg/api/errors"
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 
 	"go.pinniped.dev/internal/constable"
 	"go.pinniped.dev/internal/crud"
+	"go.pinniped.dev/internal/fositestorage"
 )
 
 const (
-	ErrInvalidPKCERequestType = constable.Error("requester must be of type fosite.Request")
+	ErrInvalidPKCERequestVersion = constable.Error("pkce request data has wrong version")
+	ErrInvalidPKCERequestData    = constable.Error("pkce request data must be present")
 
 	pkceStorageVersion = "1"
 )
@@ -37,9 +40,8 @@ func New(secrets corev1client.SecretInterface) pkce.PKCERequestStorage {
 	return &pkceStorage{storage: crud.New("pkce", secrets)}
 }
 
-// TODO test what happens when we pass nil as the requester.
 func (a *pkceStorage) CreatePKCERequestSession(ctx context.Context, signature string, requester fosite.Requester) error {
-	request, err := validateAndExtractAuthorizeRequest(requester)
+	request, err := fositestorage.ValidateAndExtractAuthorizeRequest(requester)
 	if err != nil {
 		return err
 	}
@@ -66,25 +68,22 @@ func (a *pkceStorage) getSession(ctx context.Context, signature string) (*sessio
 	session := newValidEmptyPKCESession()
 	rv, err := a.storage.Get(ctx, signature, session)
 
-	// TODO we do want this
+	if errors.IsNotFound(err) {
-	// if errors.IsNotFound(err) {
+		return nil, "", fosite.ErrNotFound.WithCause(err).WithDebug(err.Error())
-	// 	return nil, "", fosite.ErrNotFound.WithCause(err).WithDebug(err.Error())
-	// }
-
-	if err != nil {
-		return nil, "", fmt.Errorf("failed to get authorization code session for %s: %w", signature, err)
 	}
 
-	// TODO we probably want this
+	if err != nil {
-	// if version := session.Version; version != pkceStorageVersion {
+		return nil, "", fmt.Errorf("failed to get pkce session for %s: %w", signature, err)
-	// 	return nil, "", fmt.Errorf("%w: authorization code session for %s has version %s instead of %s",
+	}
-	// 		ErrInvalidAuthorizeRequestVersion, signature, version, pkceStorageVersion)
-	// }
 
-	// TODO maybe we want this. it would only apply when a human has edited the secret.
+	if version := session.Version; version != pkceStorageVersion {
-	// if session.Request == nil {
+		return nil, "", fmt.Errorf("%w: pkce session for %s has version %s instead of %s",
-	// 	return nil, "", fmt.Errorf("malformed authorization code session for %s: %w", signature, ErrInvalidAuthorizeRequestData)
+			ErrInvalidPKCERequestVersion, signature, version, pkceStorageVersion)
-	// }
+	}
+
+	if session.Request.ID == "" {
+		return nil, "", fmt.Errorf("malformed pkce session for %s: %w", signature, ErrInvalidPKCERequestData)
+	}
 
 	return session, rv, nil
 }
@@ -97,19 +96,3 @@ func newValidEmptyPKCESession() *session {
 		},
 	}
 }
-
-func validateAndExtractAuthorizeRequest(requester fosite.Requester) (*fosite.Request, error) {
-	request, ok1 := requester.(*fosite.Request)
-	if !ok1 {
-		return nil, ErrInvalidPKCERequestType
-	}
-	_, ok2 := request.Client.(*fosite.DefaultOpenIDConnectClient)
-	_, ok3 := request.Session.(*openid.DefaultSession)
-
-	valid := ok2 && ok3
-	if !valid {
-		return nil, ErrInvalidPKCERequestType
-	}
-
-	return request, nil
-}

internal/fositestorage/pkce/pkce_test.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/ory/fosite"
 	"github.com/ory/fosite/handler/openid"
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -19,6 +20,8 @@ import (
 	coretesting "k8s.io/client-go/testing"
 )
 
+const namespace = "test-ns"
+
 func TestPKCEStorage(t *testing.T) {
 	ctx := context.Background()
 	secretsGVR := schema.GroupVersionResource{
@@ -27,8 +30,6 @@ func TestPKCEStorage(t *testing.T) {
 		Resource: "secrets",
 	}
 
-	const namespace = "test-ns"
-
 	wantActions := []coretesting.Action{
 		coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
@@ -98,3 +99,101 @@ func TestPKCEStorage(t *testing.T) {
 
 	require.Equal(t, wantActions, client.Actions())
 }
+
+func TestGetNotFound(t *testing.T) {
+	ctx := context.Background()
+	client := fake.NewSimpleClientset()
+	secrets := client.CoreV1().Secrets(namespace)
+	storage := New(secrets)
+
+	_, notFoundErr := storage.GetPKCERequestSession(ctx, "non-existent-signature", nil)
+	require.EqualError(t, notFoundErr, "not_found")
+	require.True(t, errors.Is(notFoundErr, fosite.ErrNotFound))
+}
+
+func TestWrongVersion(t *testing.T) {
+	ctx := context.Background()
+	client := fake.NewSimpleClientset()
+	secrets := client.CoreV1().Secrets(namespace)
+	storage := New(secrets)
+
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            "pinniped-storage-pkce-pwu5zs7lekbhnln2w4",
+			ResourceVersion: "",
+			Labels: map[string]string{
+				"storage.pinniped.dev": "pkce",
+			},
+		},
+		Data: map[string][]byte{
+			"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"not-the-right-version"}`),
+			"pinniped-storage-version": []byte("1"),
+		},
+		Type: "storage.pinniped.dev/pkce",
+	}
+	_, err := secrets.Create(ctx, secret, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	_, err = storage.GetPKCERequestSession(ctx, "fancy-signature", nil)
+
+	require.EqualError(t, err, "pkce request data has wrong version: pkce session for fancy-signature has version not-the-right-version instead of 1")
+}
+
+func TestNilSessionRequest(t *testing.T) {
+	ctx := context.Background()
+	client := fake.NewSimpleClientset()
+	secrets := client.CoreV1().Secrets(namespace)
+	storage := New(secrets)
+
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            "pinniped-storage-pkce-pwu5zs7lekbhnln2w4",
+			ResourceVersion: "",
+			Labels: map[string]string{
+				"storage.pinniped.dev": "pkce",
+			},
+		},
+		Data: map[string][]byte{
+			"pinniped-storage-data":    []byte(`{"nonsense-key": "nonsense-value","version":"1"}`),
+			"pinniped-storage-version": []byte("1"),
+		},
+		Type: "storage.pinniped.dev/pkce",
+	}
+
+	_, err := secrets.Create(ctx, secret, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	_, err = storage.GetPKCERequestSession(ctx, "fancy-signature", nil)
+	require.EqualError(t, err, "malformed pkce session for fancy-signature: pkce request data must be present")
+}
+
+func TestCreateWithNilRequester(t *testing.T) {
+	ctx := context.Background()
+	client := fake.NewSimpleClientset()
+	secrets := client.CoreV1().Secrets(namespace)
+	storage := New(secrets)
+
+	err := storage.CreatePKCERequestSession(ctx, "signature-doesnt-matter", nil)
+	require.EqualError(t, err, "requester must be of type fosite.Request")
+}
+
+func TestCreateWithWrongRequesterDataTypes(t *testing.T) {
+	ctx := context.Background()
+	client := fake.NewSimpleClientset()
+	secrets := client.CoreV1().Secrets(namespace)
+	storage := New(secrets)
+
+	request := &fosite.Request{
+		Session: nil,
+		Client:  &fosite.DefaultOpenIDConnectClient{},
+	}
+	err := storage.CreatePKCERequestSession(ctx, "signature-doesnt-matter", request)
+	require.EqualError(t, err, "requester's session must be of type openid.DefaultSession")
+
+	request = &fosite.Request{
+		Session: &openid.DefaultSession{},
+		Client:  nil,
+	}
+	err = storage.CreatePKCERequestSession(ctx, "signature-doesnt-matter", request)
+	require.EqualError(t, err, "requester's client must be of type fosite.DefaultOpenIDConnectClient")
+}

internal/oidc/kube_storage.go

@@ -7,6 +7,10 @@ import (
 	"context"
 	"time"
 
+	fositepkce "github.com/ory/fosite/handler/pkce"
+
+	"go.pinniped.dev/internal/fositestorage/pkce"
+
 	"github.com/ory/fosite"
 	"github.com/ory/fosite/handler/oauth2"
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
@@ -19,10 +23,14 @@ const errKubeStorageNotImplemented = constable.Error("KubeStorage does not imple
 
 type KubeStorage struct {
 	authorizationCodeStorage oauth2.AuthorizeCodeStorage
+	pkceStorage              fositepkce.PKCERequestStorage
 }
 
 func NewKubeStorage(secrets corev1client.SecretInterface) *KubeStorage {
-	return &KubeStorage{authorizationCodeStorage: authorizationcode.New(secrets)}
+	return &KubeStorage{
+		authorizationCodeStorage: authorizationcode.New(secrets),
+		pkceStorage:              pkce.New(secrets),
+	}
 }
 
 func (KubeStorage) RevokeRefreshToken(_ context.Context, _ string) error {
@@ -69,16 +77,16 @@ func (KubeStorage) DeleteOpenIDConnectSession(_ context.Context, _ string) error
 	return errKubeStorageNotImplemented
 }
 
-func (KubeStorage) GetPKCERequestSession(_ context.Context, _ string, _ fosite.Session) (fosite.Requester, error) {
+func (k KubeStorage) GetPKCERequestSession(ctx context.Context, signature string, session fosite.Session) (fosite.Requester, error) {
-	return nil, errKubeStorageNotImplemented
+	return k.pkceStorage.GetPKCERequestSession(ctx, signature, session)
 }
 
-func (KubeStorage) CreatePKCERequestSession(_ context.Context, _ string, _ fosite.Requester) error {
+func (k KubeStorage) CreatePKCERequestSession(ctx context.Context, signature string, requester fosite.Requester) error {
-	return nil
+	return k.pkceStorage.CreatePKCERequestSession(ctx, signature, requester)
 }
 
-func (KubeStorage) DeletePKCERequestSession(_ context.Context, _ string) error {
+func (k KubeStorage) DeletePKCERequestSession(ctx context.Context, signature string) error {
-	return errKubeStorageNotImplemented
+	return k.pkceStorage.DeletePKCERequestSession(ctx, signature)
 }
 
 func (k KubeStorage) CreateAuthorizeCodeSession(ctx context.Context, signature string, r fosite.Requester) (err error) {

internal/oidc/oidc.go

@@ -35,7 +35,7 @@ const (
 	UpstreamStateParamEncodingName = "s"
 
 	// CSRFCookieName is the name of the browser cookie which shall hold our CSRF value.
-	// The `__Host` prefix has a special meaning. See
+	// The `__Host` prefix has a special meaning. See:
 	// https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Cookie_prefixes.
 	CSRFCookieName = "__Host-pinniped-csrf"
 

pkg/oidcclient/login.go

@@ -89,6 +89,7 @@ func WithContext(ctx context.Context) Option {
 // WithListenPort specifies a TCP listen port on localhost, which will be used for the redirect_uri and to handle the
 // authorization code callback. By default, a random high port will be chosen which requires the authorization server
 // to support wildcard port numbers as described by https://tools.ietf.org/html/rfc8252:
+//
 // The authorization server MUST allow any port to be specified at the
 // time of the request for loopback IP redirect URIs, to accommodate
 // clients that obtain an available ephemeral port from the operating

test/library/iotest.go

@@ -33,7 +33,6 @@ func (l *testlogReader) Read(p []byte) (n int, err error) {
 	return
 }
 
-//nolint: gochecknoglobals
 var tokenLike = regexp.MustCompile(`(?mi)[a-zA-Z0-9._-]{30,}|[a-zA-Z0-9]{20,}`)
 
 func maskTokens(in []byte) string {