From f38c150f6a64a1e6d4597ae4b475c69d08cc70b8 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 1 Dec 2020 14:53:22 -0800 Subject: [PATCH] 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 --- go.mod | 1 + internal/certauthority/certauthority.go | 2 +- .../authorizationcode/authorizationcode.go | 20 +--- .../authorizationcode_test.go | 4 +- internal/fositestorage/fositestorage.go | 34 ++++++ internal/fositestorage/pkce/pkce.go | 53 +++------ internal/fositestorage/pkce/pkce_test.go | 103 +++++++++++++++++- internal/oidc/kube_storage.go | 22 ++-- internal/oidc/oidc.go | 2 +- pkg/oidcclient/login.go | 9 +- test/library/iotest.go | 1 - 11 files changed, 181 insertions(+), 70 deletions(-) create mode 100644 internal/fositestorage/fositestorage.go diff --git a/go.mod b/go.mod index 4277bd9b..84237fd7 100644 --- a/go.mod +++ b/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 diff --git a/internal/certauthority/certauthority.go b/internal/certauthority/certauthority.go index 13636db4..6d3cff84 100644 --- a/internal/certauthority/certauthority.go +++ b/internal/certauthority/certauthority.go @@ -194,7 +194,7 @@ func (c *CA) Issue(subject pkix.Name, dnsNames []string, ips []net.IP, ttl time. } // IssuePEM issues a new server certificate for the given identity and duration, returning it as a pair of -// PEM-formatted byte slices for the certificate and private key. +// PEM-formatted byte slices for the certificate and private key. func (c *CA) IssuePEM(subject pkix.Name, dnsNames []string, ttl time.Duration) ([]byte, []byte, error) { return toPEM(c.Issue(subject, dnsNames, nil, ttl)) } diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index e41059b3..0c522985 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/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 diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index 09e0e374..9d03995a 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/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) diff --git a/internal/fositestorage/fositestorage.go b/internal/fositestorage/fositestorage.go new file mode 100644 index 00000000..d23c9f6a --- /dev/null +++ b/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 +} diff --git a/internal/fositestorage/pkce/pkce.go b/internal/fositestorage/pkce/pkce.go index 153d93a2..9e8ef3d5 100644 --- a/internal/fositestorage/pkce/pkce.go +++ b/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) { - // 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) + if errors.IsNotFound(err) { + return nil, "", fosite.ErrNotFound.WithCause(err).WithDebug(err.Error()) } - // TODO we probably want this - // if version := session.Version; version != pkceStorageVersion { - // return nil, "", fmt.Errorf("%w: authorization code session for %s has version %s instead of %s", - // ErrInvalidAuthorizeRequestVersion, signature, version, pkceStorageVersion) - // } + if err != nil { + return nil, "", fmt.Errorf("failed to get pkce session for %s: %w", signature, err) + } - // TODO maybe we want this. it would only apply when a human has edited the secret. - // if session.Request == nil { - // return nil, "", fmt.Errorf("malformed authorization code session for %s: %w", signature, ErrInvalidAuthorizeRequestData) - // } + if version := session.Version; version != pkceStorageVersion { + return nil, "", fmt.Errorf("%w: pkce session for %s has version %s instead of %s", + 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 -} diff --git a/internal/fositestorage/pkce/pkce_test.go b/internal/fositestorage/pkce/pkce_test.go index 82124a9d..80b2d9dd 100644 --- a/internal/fositestorage/pkce/pkce_test.go +++ b/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") +} diff --git a/internal/oidc/kube_storage.go b/internal/oidc/kube_storage.go index 664e382d..388cd771 100644 --- a/internal/oidc/kube_storage.go +++ b/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) { - return nil, errKubeStorageNotImplemented +func (k KubeStorage) GetPKCERequestSession(ctx context.Context, signature string, session fosite.Session) (fosite.Requester, error) { + return k.pkceStorage.GetPKCERequestSession(ctx, signature, session) } -func (KubeStorage) CreatePKCERequestSession(_ context.Context, _ string, _ fosite.Requester) error { - return nil +func (k KubeStorage) CreatePKCERequestSession(ctx context.Context, signature string, requester fosite.Requester) error { + return k.pkceStorage.CreatePKCERequestSession(ctx, signature, requester) } -func (KubeStorage) DeletePKCERequestSession(_ context.Context, _ string) error { - return errKubeStorageNotImplemented +func (k KubeStorage) DeletePKCERequestSession(ctx context.Context, signature string) error { + return k.pkceStorage.DeletePKCERequestSession(ctx, signature) } func (k KubeStorage) CreateAuthorizeCodeSession(ctx context.Context, signature string, r fosite.Requester) (err error) { diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 1241444f..b129714b 100644 --- a/internal/oidc/oidc.go +++ b/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" diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 2e286efa..fbbe23a9 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -89,10 +89,11 @@ 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 -// system at the time of the request. +// +// 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 +// system at the time of the request. func WithListenPort(port uint16) Option { return func(h *handlerState) error { h.listenAddr = fmt.Sprintf("localhost:%d", port) diff --git a/test/library/iotest.go b/test/library/iotest.go index dcb0e695..daf2ed4e 100644 --- a/test/library/iotest.go +++ b/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 {