Merge pull request #1117 from vmware-tanzu/prefix_tokens

Add custom prefix to downstream access and refresh tokens and authcodes

internal/oidc/dynamic_oauth2_hmac_strategy.go

@@ -1,14 +1,22 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package oidc
 
 import (
 	"context"
+	"strings"
 
 	"github.com/ory/fosite"
 	"github.com/ory/fosite/compose"
 	"github.com/ory/fosite/handler/oauth2"
+	errorsx "github.com/pkg/errors"
+)
+
+const (
+	accessTokenPrefix  = "pin_at_" // "Pinniped access token" abbreviated.
+	refreshTokenPrefix = "pin_rt_" // "Pinniped refresh token" abbreviated.
+	authcodePrefix     = "pin_ac_" // "Pinniped authorization code" abbreviated.
 )
 
 // dynamicOauth2HMACStrategy is an oauth2.CoreStrategy that can dynamically load an HMAC key to sign
@@ -19,6 +27,9 @@ import (
 // If we ever update FederationDomain's to hold their signing key, we might not need this type, since we
 // could have an invariant that routes to an FederationDomain's endpoints are only wired up if an
 // FederationDomain has a valid signing key.
+//
+// Tokens start with a custom prefix to make them identifiable as tokens when seen by a user
+// out of context, such as when accidentally committed to a GitHub repo.
 type dynamicOauth2HMACStrategy struct {
 	fositeConfig *compose.Config
 	keyFunc      func() []byte
@@ -44,7 +55,11 @@ func (s *dynamicOauth2HMACStrategy) GenerateAccessToken(
 	ctx context.Context,
 	requester fosite.Requester,
 ) (token string, signature string, err error) {
-	return s.delegate().GenerateAccessToken(ctx, requester)
+	token, sig, err := s.delegate().GenerateAccessToken(ctx, requester)
+	if err == nil {
+		token = accessTokenPrefix + token
+	}
+	return token, sig, err
 }
 
 func (s *dynamicOauth2HMACStrategy) ValidateAccessToken(
@@ -52,7 +67,11 @@ func (s *dynamicOauth2HMACStrategy) ValidateAccessToken(
 	requester fosite.Requester,
 	token string,
 ) (err error) {
-	return s.delegate().ValidateAccessToken(ctx, requester, token)
+	if !strings.HasPrefix(token, accessTokenPrefix) {
+		return errorsx.WithStack(fosite.ErrInvalidTokenFormat.
+			WithDebugf("Access token did not have prefix %q", accessTokenPrefix))
+	}
+	return s.delegate().ValidateAccessToken(ctx, requester, token[len(accessTokenPrefix):])
 }
 
 func (s *dynamicOauth2HMACStrategy) RefreshTokenSignature(token string) string {
@@ -63,7 +82,11 @@ func (s *dynamicOauth2HMACStrategy) GenerateRefreshToken(
 	ctx context.Context,
 	requester fosite.Requester,
 ) (token string, signature string, err error) {
-	return s.delegate().GenerateRefreshToken(ctx, requester)
+	token, sig, err := s.delegate().GenerateRefreshToken(ctx, requester)
+	if err == nil {
+		token = refreshTokenPrefix + token
+	}
+	return token, sig, err
 }
 
 func (s *dynamicOauth2HMACStrategy) ValidateRefreshToken(
@@ -71,7 +94,11 @@ func (s *dynamicOauth2HMACStrategy) ValidateRefreshToken(
 	requester fosite.Requester,
 	token string,
 ) (err error) {
-	return s.delegate().ValidateRefreshToken(ctx, requester, token)
+	if !strings.HasPrefix(token, refreshTokenPrefix) {
+		return errorsx.WithStack(fosite.ErrInvalidTokenFormat.
+			WithDebugf("Refresh token did not have prefix %q", refreshTokenPrefix))
+	}
+	return s.delegate().ValidateRefreshToken(ctx, requester, token[len(refreshTokenPrefix):])
 }
 
 func (s *dynamicOauth2HMACStrategy) AuthorizeCodeSignature(token string) string {
@@ -82,7 +109,11 @@ func (s *dynamicOauth2HMACStrategy) GenerateAuthorizeCode(
 	ctx context.Context,
 	requester fosite.Requester,
 ) (token string, signature string, err error) {
-	return s.delegate().GenerateAuthorizeCode(ctx, requester)
+	authcode, sig, err := s.delegate().GenerateAuthorizeCode(ctx, requester)
+	if err == nil {
+		authcode = authcodePrefix + authcode
+	}
+	return authcode, sig, err
 }
 
 func (s *dynamicOauth2HMACStrategy) ValidateAuthorizeCode(
@@ -90,7 +121,11 @@ func (s *dynamicOauth2HMACStrategy) ValidateAuthorizeCode(
 	requester fosite.Requester,
 	token string,
 ) (err error) {
-	return s.delegate().ValidateAuthorizeCode(ctx, requester, token)
+	if !strings.HasPrefix(token, authcodePrefix) {
+		return errorsx.WithStack(fosite.ErrInvalidTokenFormat.
+			WithDebugf("Authorization code did not have prefix %q", authcodePrefix))
+	}
+	return s.delegate().ValidateAuthorizeCode(ctx, requester, token[len(authcodePrefix):])
 }
 
 func (s *dynamicOauth2HMACStrategy) delegate() *oauth2.HMACSHAStrategy {

internal/oidc/dynamic_oauth2_hmac_strategy_test.go

@@ -0,0 +1,218 @@
+// Copyright 2022 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package oidc
+
+import (
+	"context"
+	"encoding/base64"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/ory/fosite"
+	"github.com/ory/fosite/compose"
+	"github.com/stretchr/testify/require"
+)
+
+func TestDynamicOauth2HMACStrategy_Signatures(t *testing.T) {
+	s := &dynamicOauth2HMACStrategy{
+		fositeConfig: &compose.Config{}, // defaults are good enough for this unit test
+		keyFunc:      func() []byte { return []byte("12345678901234567890123456789012") },
+	}
+
+	tests := []struct {
+		name          string
+		token         string
+		signatureFunc func(token string) (signature string)
+		wantSignature string
+	}{
+		{
+			name:          "access token signature is the part after the dot in the default HMAC strategy",
+			token:         "token.signature",
+			signatureFunc: s.AccessTokenSignature,
+			wantSignature: "signature",
+		},
+		{
+			name:          "refresh token signature is the part after the dot in the default HMAC strategy",
+			token:         "token.signature",
+			signatureFunc: s.RefreshTokenSignature,
+			wantSignature: "signature",
+		},
+		{
+			name:          "authcode signature is the part after the dot in the default HMAC strategy",
+			token:         "token.signature",
+			signatureFunc: s.AuthorizeCodeSignature,
+			wantSignature: "signature",
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			require.Equal(t, tt.wantSignature, tt.signatureFunc(tt.token))
+		})
+	}
+}
+
+func TestDynamicOauth2HMACStrategy_Generate(t *testing.T) {
+	s := &dynamicOauth2HMACStrategy{
+		fositeConfig: &compose.Config{},                                                   // defaults are good enough for this unit test
+		keyFunc:      func() []byte { return []byte("12345678901234567890123456789012") }, // 32 character secret key
+	}
+
+	generateTokenErrorCausingStrategy := &dynamicOauth2HMACStrategy{
+		fositeConfig: &compose.Config{},
+		keyFunc:      func() []byte { return []byte("too_short_causes_error") }, // secret key is below required 32 characters
+	}
+
+	tests := []struct {
+		name            string
+		generateFunc    func(ctx context.Context, requester fosite.Requester) (token string, signature string, err error)
+		errGenerateFunc func(ctx context.Context, requester fosite.Requester) (token string, signature string, err error)
+		wantPrefix      string
+	}{
+		{
+			name:            "access tokens are base64 random bytes followed by dot followed by base64 signature of the random bytes in the default HMAC strategy",
+			generateFunc:    s.GenerateAccessToken,
+			errGenerateFunc: generateTokenErrorCausingStrategy.GenerateAccessToken,
+			wantPrefix:      "pin_at_",
+		},
+		{
+			name:            "refresh tokens are base64 random bytes followed by dot followed by base64 signature of the random bytes in the default HMAC strategy",
+			generateFunc:    s.GenerateRefreshToken,
+			errGenerateFunc: generateTokenErrorCausingStrategy.GenerateRefreshToken,
+			wantPrefix:      "pin_rt_",
+		},
+		{
+			name:            "authcodes are base64 random bytes followed by dot followed by base64 signature of the random bytes in the default HMAC strategy",
+			generateFunc:    s.GenerateAuthorizeCode,
+			errGenerateFunc: generateTokenErrorCausingStrategy.GenerateAuthorizeCode,
+			wantPrefix:      "pin_ac_",
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			requireRandomTokenInExpectedFormat := func(token, signature string) {
+				// Tokens should start with a custom prefix to make them identifiable as tokens when seen by a user
+				// out of context, such as when accidentally committed to a GitHub repo.
+				require.True(t, strings.HasPrefix(token, tt.wantPrefix), "token %q did not have expected prefix %q", token, tt.wantPrefix)
+				require.Equal(t, 1, strings.Count(token, "."))
+				require.Len(t, signature, 43)
+				require.True(t, strings.HasSuffix(token, "."+signature), "token %q did not end with dot followed by signature", token)
+				// The part before the dot is the prefix plus 43 characters of base64 encoded random bytes.
+				require.Len(t, strings.Split(token, ".")[0], len(tt.wantPrefix)+43)
+			}
+
+			var ctxIsIgnored context.Context
+			var requesterIsIgnored fosite.Requester
+
+			generatedToken1, signature1, err := tt.generateFunc(ctxIsIgnored, requesterIsIgnored)
+			require.NoError(t, err)
+			requireRandomTokenInExpectedFormat(generatedToken1, signature1)
+
+			generatedToken2, signature2, err := tt.generateFunc(ctxIsIgnored, requesterIsIgnored)
+			require.NoError(t, err)
+			requireRandomTokenInExpectedFormat(generatedToken2, signature2)
+
+			// Each generated token is random/different.
+			require.NotEqual(t, generatedToken1, generatedToken2)
+			require.NotEqual(t, signature1, signature2)
+
+			// Test the return values when an error is encountered during generation.
+			generatedToken3, signature3, err := tt.errGenerateFunc(ctxIsIgnored, requesterIsIgnored)
+			require.EqualError(t, err, "secret for signing HMAC-SHA512/256 is expected to be 32 byte long, got 22 byte")
+			require.Empty(t, generatedToken3)
+			require.Empty(t, signature3)
+		})
+	}
+}
+
+func TestDynamicOauth2HMACStrategy_Validate(t *testing.T) {
+	s := &dynamicOauth2HMACStrategy{
+		fositeConfig: &compose.Config{}, // defaults are good enough for this unit test
+		keyFunc:      func() []byte { return []byte("12345678901234567890123456789012") },
+	}
+
+	tests := []struct {
+		name         string
+		generateFunc func(ctx context.Context, requester fosite.Requester) (token string, signature string, err error)
+		validateFunc func(ctx context.Context, requester fosite.Requester, token string) error
+		wantPrefix   string
+	}{
+		{
+			name:         "access tokens",
+			generateFunc: s.GenerateAccessToken,
+			validateFunc: s.ValidateAccessToken,
+			wantPrefix:   "pin_at_",
+		},
+		{
+			name:         "refresh tokens",
+			generateFunc: s.GenerateRefreshToken,
+			validateFunc: s.ValidateRefreshToken,
+			wantPrefix:   "pin_rt_",
+		},
+		{
+			name:         "authcodes",
+			generateFunc: s.GenerateAuthorizeCode,
+			validateFunc: s.ValidateAuthorizeCode,
+			wantPrefix:   "pin_ac_",
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			var ctxIsIgnored context.Context
+			var requesterIsIgnored fosite.Requester
+
+			unexpiredSession := &fosite.DefaultSession{}
+			unexpiredSession.SetExpiresAt(fosite.RefreshToken, time.Now().Add(time.Hour))
+			unexpiredSession.SetExpiresAt(fosite.AccessToken, time.Now().Add(time.Hour))
+			unexpiredSession.SetExpiresAt(fosite.AuthorizeCode, time.Now().Add(time.Hour))
+			requesterWithUnexpiredTokens := &fosite.Request{Session: unexpiredSession}
+
+			expiredSession := &fosite.DefaultSession{}
+			expiredSession.SetExpiresAt(fosite.RefreshToken, time.Now().Add(-time.Hour))
+			expiredSession.SetExpiresAt(fosite.AccessToken, time.Now().Add(-time.Hour))
+			expiredSession.SetExpiresAt(fosite.AuthorizeCode, time.Now().Add(-time.Hour))
+			requesterWithExpiredTokens := &fosite.Request{Session: expiredSession}
+
+			generatedToken, _, err := tt.generateFunc(ctxIsIgnored, requesterIsIgnored)
+			require.NoError(t, err)
+			require.NoError(t, tt.validateFunc(ctxIsIgnored, requesterWithUnexpiredTokens, generatedToken))
+
+			generatedToken, _, err = tt.generateFunc(ctxIsIgnored, requesterIsIgnored)
+			require.NoError(t, err)
+			require.NoError(t, tt.validateFunc(ctxIsIgnored, requesterWithUnexpiredTokens, generatedToken))
+
+			// Generated token has prefix.
+			require.True(t, strings.HasPrefix(generatedToken, tt.wantPrefix), "token %q did not have expected prefix %q", generatedToken, tt.wantPrefix)
+
+			// Validate when expired according to session.
+			require.EqualError(t, tt.validateFunc(ctxIsIgnored, requesterWithExpiredTokens, generatedToken), "invalid_token")
+
+			// Validate when missing prefix.
+			require.EqualError(t, tt.validateFunc(ctxIsIgnored, requesterWithUnexpiredTokens, strings.TrimPrefix(generatedToken, tt.wantPrefix)), "invalid_token")
+
+			// Validate when wrong prefix.
+			require.EqualError(t, tt.validateFunc(ctxIsIgnored, requesterWithUnexpiredTokens, "pin_wrong_"+strings.TrimPrefix(generatedToken, tt.wantPrefix)), "invalid_token")
+
+			// Validate when correct prefix but otherwise invalid format.
+			require.EqualError(t, tt.validateFunc(ctxIsIgnored, requesterWithUnexpiredTokens, tt.wantPrefix+"illegal token"), "invalid_token")
+
+			// Validate when correct prefix but bad signature.
+			var b64 = base64.URLEncoding.WithPadding(base64.NoPadding)
+			tokenWithBadSig := tt.wantPrefix + b64.EncodeToString([]byte("some-token")) + "." + b64.EncodeToString([]byte("bad-signature"))
+			require.EqualError(t, tt.validateFunc(ctxIsIgnored, requesterWithUnexpiredTokens, tokenWithBadSig), "token_signature_mismatch")
+		})
+	}
+}

internal/oidc/token/token_handler_test.go

@@ -3251,6 +3251,9 @@ func requireValidRefreshTokenStorage(
 	storedRequest, err := storage.GetRefreshTokenSession(context.Background(), getFositeDataSignature(t, refreshTokenString), nil)
 	require.NoError(t, err)
 
+	// Refresh tokens should start with the custom prefix "pin_rt_" to make them identifiable as refresh tokens when seen by a user out of context.
+	require.True(t, strings.HasPrefix(refreshTokenString, "pin_rt_"), "token %q did not have expected prefix 'pin_rt_'", refreshTokenString)
+
 	// Fosite stores refresh tokens without any of the original request form parameters.
 	requireValidStoredRequest(
 		t,
@@ -3287,6 +3290,9 @@ func requireValidAccessTokenStorage(
 	storedRequest, err := storage.GetAccessTokenSession(context.Background(), getFositeDataSignature(t, accessTokenString), nil)
 	require.NoError(t, err)
 
+	// Access tokens should start with the custom prefix "pin_at_" to make them identifiable as access tokens when seen by a user out of context.
+	require.True(t, strings.HasPrefix(accessTokenString, "pin_at_"), "token %q did not have expected prefix 'pin_at_'", accessTokenString)
+
 	// Make sure the other body fields are valid.
 	tokenType, ok := body["token_type"]
 	require.True(t, ok)

internal/testutil/oidctestutil/oidctestutil.go

@@ -901,6 +901,9 @@ func RequireAuthCodeRegexpMatch(
 	require.Lenf(t, submatches, 2, "no regexp match in actualContent: %", actualContent)
 	capturedAuthCode := submatches[1]
 
+	// Authcodes should start with the custom prefix "pin_ac_" to make them identifiable as authcodes when seen by a user out of context.
+	require.True(t, strings.HasPrefix(capturedAuthCode, "pin_ac_"), "token %q did not have expected prefix 'pin_ac_'", capturedAuthCode)
+
 	// fosite authcodes are in the format `data.signature`, so grab the signature part, which is the lookup key in the storage interface
 	authcodeDataAndSignature := strings.Split(capturedAuthCode, ".")
 	require.Len(t, authcodeDataAndSignature, 2)

test/integration/supervisor_login_test.go

@@ -1818,6 +1818,9 @@ func testSupervisorLogin(
 		authcode := callback.URL.Query().Get("code")
 		require.NotEmpty(t, authcode)
 
+		// Authcodes should start with the custom prefix "pin_ac_" to make them identifiable as authcodes when seen by a user out of context.
+		require.True(t, strings.HasPrefix(authcode, "pin_ac_"), "token %q did not have expected prefix 'pin_ac_'", authcode)
+
 		// Call the token endpoint to get tokens.
 		tokenResponse, err := downstreamOAuth2Config.Exchange(oidcHTTPClientContext, authcode, pkceParam.Verifier())
 		require.NoError(t, err)
@@ -1973,8 +1976,12 @@ func verifyTokenResponse(
 	require.NotZero(t, tokenResponse.Expiry)
 	expectedAccessTokenLifetime := oidc.DefaultOIDCTimeoutsConfiguration().AccessTokenLifespan
 	testutil.RequireTimeInDelta(t, time.Now().UTC().Add(expectedAccessTokenLifetime), tokenResponse.Expiry, time.Second*30)
+	// Access tokens should start with the custom prefix "pin_at_" to make them identifiable as access tokens when seen by a user out of context.
+	require.True(t, strings.HasPrefix(tokenResponse.AccessToken, "pin_at_"), "token %q did not have expected prefix 'pin_at_'", tokenResponse.AccessToken)
 
 	require.NotEmpty(t, tokenResponse.RefreshToken)
+	// Refresh tokens should start with the custom prefix "pin_rt_" to make them identifiable as refresh tokens when seen by a user out of context.
+	require.True(t, strings.HasPrefix(tokenResponse.RefreshToken, "pin_rt_"), "token %q did not have expected prefix 'pin_rt_'", tokenResponse.RefreshToken)
 }
 
 func requestAuthorizationUsingBrowserAuthcodeFlow(t *testing.T, downstreamAuthorizeURL, downstreamCallbackURL, _, _ string, httpClient *http.Client) {