Merge branch 'main' into reenable-max-inflight-checks

internal/oidc/callback/callback_handler.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package callback provides a handler for the OIDC callback endpoint.
@@ -255,7 +255,7 @@ func getSubjectAndUsernameFromUpstreamIDToken(
 func getGroupsFromUpstreamIDToken(
 	upstreamIDPConfig provider.UpstreamOIDCIdentityProviderI,
 	idTokenClaims map[string]interface{},
-) (interface{}, error) {
+) ([]string, error) {
 	groupsClaim := upstreamIDPConfig.GetGroupsClaim()
 	if groupsClaim == "" {
 		return nil, nil
@@ -272,9 +272,8 @@ func getGroupsFromUpstreamIDToken(
 		return nil, nil // the upstream IDP may have omitted the claim if the user has no groups
 	}
 
-	groupsAsArray, okAsArray := groupsAsInterface.([]string)
+	groupsAsArray, okAsArray := extractGroups(groupsAsInterface)
-	groupsAsString, okAsString := groupsAsInterface.(string)
+	if !okAsArray {
-	if !okAsArray && !okAsString {
 		plog.Warning(
 			"groups claim in upstream ID token has invalid format",
 			"upstreamName", upstreamIDPConfig.GetName(),
@@ -284,13 +283,40 @@ func getGroupsFromUpstreamIDToken(
 		return nil, httperr.New(http.StatusUnprocessableEntity, "groups claim in upstream ID token has invalid format")
 	}
 
-	if okAsArray {
 	return groupsAsArray, nil
 }
-	return groupsAsString, nil
+
+func extractGroups(groupsAsInterface interface{}) ([]string, bool) {
+	groupsAsString, okAsString := groupsAsInterface.(string)
+	if okAsString {
+		return []string{groupsAsString}, true
 	}
 
-func makeDownstreamSession(subject string, username string, groups interface{}) *openid.DefaultSession {
+	groupsAsStringArray, okAsStringArray := groupsAsInterface.([]string)
+	if okAsStringArray {
+		return groupsAsStringArray, true
+	}
+
+	groupsAsInterfaceArray, okAsArray := groupsAsInterface.([]interface{})
+	if !okAsArray {
+		return nil, false
+	}
+
+	var groupsAsStrings []string
+	for _, groupAsInterface := range groupsAsInterfaceArray {
+		groupAsString, okAsString := groupAsInterface.(string)
+		if !okAsString {
+			return nil, false
+		}
+		if groupAsString != "" {
+			groupsAsStrings = append(groupsAsStrings, groupAsString)
+		}
+	}
+
+	return groupsAsStrings, true
+}
+
+func makeDownstreamSession(subject string, username string, groups []string) *openid.DefaultSession {
 	now := time.Now().UTC()
 	openIDSession := &openid.DefaultSession{
 		Claims: &jwt.IDTokenClaims{
@@ -299,11 +325,12 @@ func makeDownstreamSession(subject string, username string, groups interface{})
 			AuthTime:    now,
 		},
 	}
+	if groups == nil {
+		groups = []string{}
+	}
 	openIDSession.Claims.Extra = map[string]interface{}{
 		oidc.DownstreamUsernameClaim: username,
-	}
+		oidc.DownstreamGroupsClaim:   groups,
-	if groups != nil {
-		openIDSession.Claims.Extra[oidc.DownstreamGroupsClaim] = groups
 	}
 	return openIDSession
 }

internal/oidc/callback/callback_handler_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package callback
@@ -134,7 +134,7 @@ func TestCallbackEndpoint(t *testing.T) {
 		wantDownstreamGrantedScopes       []string
 		wantDownstreamIDTokenSubject      string
 		wantDownstreamIDTokenUsername     string
-		wantDownstreamIDTokenGroups       interface{}
+		wantDownstreamIDTokenGroups       []string
 		wantDownstreamRequestedScopes     []string
 		wantDownstreamNonce               string
 		wantDownstreamPKCEChallenge       string
@@ -172,7 +172,7 @@ func TestCallbackEndpoint(t *testing.T) {
 			wantBody:                          "",
 			wantDownstreamIDTokenSubject:      upstreamIssuer + "?sub=" + upstreamSubject,
 			wantDownstreamIDTokenUsername:     upstreamIssuer + "?sub=" + upstreamSubject,
-			wantDownstreamIDTokenGroups:       nil,
+			wantDownstreamIDTokenGroups:       []string{},
 			wantDownstreamRequestedScopes:     happyDownstreamScopesRequested,
 			wantDownstreamGrantedScopes:       happyDownstreamScopesGranted,
 			wantDownstreamNonce:               downstreamNonce,
@@ -210,7 +210,26 @@ func TestCallbackEndpoint(t *testing.T) {
 			wantBody:                          "",
 			wantDownstreamIDTokenSubject:      upstreamIssuer + "?sub=" + upstreamSubject,
 			wantDownstreamIDTokenUsername:     upstreamUsername,
-			wantDownstreamIDTokenGroups:       "notAnArrayGroup1 notAnArrayGroup2",
+			wantDownstreamIDTokenGroups:       []string{"notAnArrayGroup1 notAnArrayGroup2"},
+			wantDownstreamRequestedScopes:     happyDownstreamScopesRequested,
+			wantDownstreamGrantedScopes:       happyDownstreamScopesGranted,
+			wantDownstreamNonce:               downstreamNonce,
+			wantDownstreamPKCEChallenge:       downstreamPKCEChallenge,
+			wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod,
+			wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs,
+		},
+		{
+			name:                              "upstream IDP's configured groups claim in the ID token is a slice of interfaces",
+			idp:                               happyUpstream().WithIDTokenClaim(upstreamGroupsClaim, []interface{}{"group1", "group2"}).Build(),
+			method:                            http.MethodGet,
+			path:                              newRequestPath().WithState(happyState).String(),
+			csrfCookie:                        happyCSRFCookie,
+			wantStatus:                        http.StatusFound,
+			wantRedirectLocationRegexp:        happyDownstreamRedirectLocationRegexp,
+			wantBody:                          "",
+			wantDownstreamIDTokenSubject:      upstreamIssuer + "?sub=" + upstreamSubject,
+			wantDownstreamIDTokenUsername:     upstreamUsername,
+			wantDownstreamIDTokenGroups:       []string{"group1", "group2"},
 			wantDownstreamRequestedScopes:     happyDownstreamScopesRequested,
 			wantDownstreamGrantedScopes:       happyDownstreamScopesGranted,
 			wantDownstreamNonce:               downstreamNonce,
@@ -437,6 +456,7 @@ func TestCallbackEndpoint(t *testing.T) {
 			wantDownstreamIDTokenUsername:     upstreamUsername,
 			wantDownstreamRequestedScopes:     happyDownstreamScopesRequested,
 			wantDownstreamGrantedScopes:       happyDownstreamScopesGranted,
+			wantDownstreamIDTokenGroups:       []string{},
 			wantDownstreamNonce:               downstreamNonce,
 			wantDownstreamPKCEChallenge:       downstreamPKCEChallenge,
 			wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod,
@@ -482,6 +502,26 @@ func TestCallbackEndpoint(t *testing.T) {
 			wantBody:                          "Unprocessable Entity: groups claim in upstream ID token has invalid format\n",
 			wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs,
 		},
+		{
+			name:                              "upstream ID token contains groups claim where one element is invalid",
+			idp:                               happyUpstream().WithIDTokenClaim(upstreamGroupsClaim, []interface{}{"foo", 7}).Build(),
+			method:                            http.MethodGet,
+			path:                              newRequestPath().WithState(happyState).String(),
+			csrfCookie:                        happyCSRFCookie,
+			wantStatus:                        http.StatusUnprocessableEntity,
+			wantBody:                          "Unprocessable Entity: groups claim in upstream ID token has invalid format\n",
+			wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs,
+		},
+		{
+			name:                              "upstream ID token contains groups claim with invalid null type",
+			idp:                               happyUpstream().WithIDTokenClaim(upstreamGroupsClaim, nil).Build(),
+			method:                            http.MethodGet,
+			path:                              newRequestPath().WithState(happyState).String(),
+			csrfCookie:                        happyCSRFCookie,
+			wantStatus:                        http.StatusUnprocessableEntity,
+			wantBody:                          "Unprocessable Entity: groups claim in upstream ID token has invalid format\n",
+			wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs,
+		},
 	}
 	for _, test := range tests {
 		test := test
@@ -779,7 +819,7 @@ func validateAuthcodeStorage(
 	wantDownstreamGrantedScopes []string,
 	wantDownstreamIDTokenSubject string,
 	wantDownstreamIDTokenUsername string,
-	wantDownstreamIDTokenGroups interface{},
+	wantDownstreamIDTokenGroups []string,
 	wantDownstreamRequestedScopes []string,
 ) (*fosite.Request, *openid.DefaultSession) {
 	t.Helper()
@@ -818,23 +858,10 @@ func validateAuthcodeStorage(
 	// Check the user's identity, which are put into the downstream ID token's subject, username and groups claims.
 	require.Equal(t, wantDownstreamIDTokenSubject, actualClaims.Subject)
 	require.Equal(t, wantDownstreamIDTokenUsername, actualClaims.Extra["username"])
-	if wantDownstreamIDTokenGroups != nil { //nolint:nestif // there are some nested if's here but its probably fine for a test
 	require.Len(t, actualClaims.Extra, 2)
-		wantArray, ok := wantDownstreamIDTokenGroups.([]string)
+	actualDownstreamIDTokenGroups := actualClaims.Extra["groups"]
-		if ok {
+	require.NotNil(t, actualDownstreamIDTokenGroups)
-			require.ElementsMatch(t, wantArray, actualClaims.Extra["groups"])
+	require.ElementsMatch(t, wantDownstreamIDTokenGroups, actualDownstreamIDTokenGroups)
-		} else {
-			wantString, ok := wantDownstreamIDTokenGroups.(string)
-			if ok {
-				require.Equal(t, wantString, actualClaims.Extra["groups"])
-			} else {
-				require.Fail(t, "wantDownstreamIDTokenGroups should be of type: either []string or string")
-			}
-		}
-	} else {
-		require.Len(t, actualClaims.Extra, 1)
-		require.NotContains(t, actualClaims.Extra, "groups")
-	}
 
 	// Check the rest of the downstream ID token's claims. Fosite wants us to set these (in UTC time).
 	testutil.RequireTimeInDelta(t, time.Now().UTC(), actualClaims.RequestedAt, timeComparisonFudgeFactor)

internal/upstreamoidc/upstreamoidc.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package upstreamoidc implements an abstraction of upstream OIDC provider interactions.
@@ -16,6 +16,7 @@ import (
 	"go.pinniped.dev/internal/httputil/httperr"
 	"go.pinniped.dev/internal/oidc"
 	"go.pinniped.dev/internal/oidc/provider"
+	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/pkg/oidcclient/nonce"
 	"go.pinniped.dev/pkg/oidcclient/oidctypes"
 	"go.pinniped.dev/pkg/oidcclient/pkce"
@@ -101,10 +102,12 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e
 	if err := validated.Claims(&validatedClaims); err != nil {
 		return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err)
 	}
+	plog.All("claims from ID token", "providerName", p.Name, "claims", validatedClaims)
 
 	if err := p.fetchUserInfo(ctx, tok, validatedClaims); err != nil {
 		return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err)
 	}
+	plog.All("claims from ID token and userinfo", "providerName", p.Name, "claims", validatedClaims)
 
 	return &oidctypes.Token{
 		AccessToken: &oidctypes.AccessToken{

test/integration/e2e_test.go

@@ -292,14 +292,12 @@ func TestE2EFullIntegration(t *testing.T) {
 	require.NotNil(t, token)
 
 	idTokenClaims := token.IDToken.Claims
-	username := idTokenClaims[oidc.DownstreamUsernameClaim].(string)
+	require.Equal(t, env.SupervisorTestUpstream.Username, idTokenClaims[oidc.DownstreamUsernameClaim])
-	groups, _ := idTokenClaims[oidc.DownstreamGroupsClaim].([]string)
 
-	require.Equal(t, env.SupervisorTestUpstream.Username, username)
+	// The groups claim in the file ends up as an []interface{}, so adjust our expectation to match.
-	if len(env.SupervisorTestUpstream.ExpectedGroups) == 0 {
+	expectedGroups := make([]interface{}, 0, len(env.SupervisorTestUpstream.ExpectedGroups))
-		// We only put a groups claim in our downstream ID token if we got groups from the upstream.
+	for _, g := range env.SupervisorTestUpstream.ExpectedGroups {
-		require.Nil(t, groups)
+		expectedGroups = append(expectedGroups, g)
-	} else {
-		require.Equal(t, env.SupervisorTestUpstream.ExpectedGroups, groups)
 	}
+	require.Equal(t, expectedGroups, idTokenClaims[oidc.DownstreamGroupsClaim])
 }

test/integration/kubeclient_test.go

@@ -136,6 +136,19 @@ func TestKubeClientOwnerRef(t *testing.T) {
 		return err
 	})
 
+	// TODO: update middleware code to not set owner references on cluster-scoped objects.
+	//
+	// The Kube 1.20 garbage collector asserts some new behavior in regards to invalid owner
+	// references (i.e., when you have a namespace-scoped owner references for a cluster-scoped
+	// dependent, the cluster-scoped dependent is not removed). We also found a bug in the 1.20
+	// garbage collector where namespace-scoped dependents are not garbage collected if their owner
+	// had been used as an invalid owner reference before - this bug causes our test to fallover
+	// because we are setting a namespace-scoped owner ref on this APIService.
+	//
+	// We believe that the best way to get around this problem is to update our kubeclient code to
+	// never set owner references on cluster-scoped objects. After we do that, we will uncomment this
+	// part of the test.
+	if false {
 		// sanity check API service client
 		apiService, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Create(
 			ctx,
@@ -160,6 +173,7 @@ func TestKubeClientOwnerRef(t *testing.T) {
 			_, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Get(ctx, apiService.Name, metav1.GetOptions{})
 			return err
 		})
+	}
 
 	// sanity check concierge client
 	credentialIssuer, err := ownerRefClient.PinnipedConcierge.ConfigV1alpha1().CredentialIssuers(namespace.Name).Create(
@@ -244,16 +258,15 @@ func hasOwnerRef(t *testing.T, obj metav1.Object, ref metav1.OwnerReference) {
 func isEventuallyDeleted(t *testing.T, f func() error) {
 	t.Helper()
 
-	require.Eventually(t, func() bool {
+	library.RequireEventuallyWithoutError(t, func() (bool, error) {
 		err := f()
 		switch {
 		case err == nil:
-			return false
+			return false, nil
 		case errors.IsNotFound(err):
-			return true
+			return true, nil
 		default:
-			require.NoError(t, err)
+			return false, err
-			return false
 		}
 	}, time.Minute, time.Second)
 }

test/integration/supervisor_login_test.go

@@ -207,7 +207,7 @@ func TestSupervisorLogin(t *testing.T) {
 	tokenResponse, err := downstreamOAuth2Config.Exchange(oidcHTTPClientContext, authcode, pkceParam.Verifier())
 	require.NoError(t, err)
 
-	expectedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "nonce", "rat", "username"}
+	expectedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "nonce", "rat", "username", "groups"}
 	verifyTokenResponse(t, tokenResponse, discovery, downstreamOAuth2Config, env.SupervisorTestUpstream.Issuer, nonceParam, expectedIDTokenClaims)
 
 	// token exchange on the original token

test/library/assertions.go

@@ -0,0 +1,26 @@
+// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package library
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/util/wait"
+)
+
+// RequireEventuallyWithoutError is a wrapper around require.Eventually() that allows the caller to
+// return an error from the condition function. If the condition function returns an error at any
+// point, the assertion will immediately fail.
+func RequireEventuallyWithoutError(
+	t *testing.T,
+	f func() (bool, error),
+	waitFor time.Duration,
+	tick time.Duration,
+	msgAndArgs ...interface{},
+) {
+	t.Helper()
+	require.NoError(t, wait.PollImmediate(tick, waitFor, f), msgAndArgs...)
+}