diff --git a/internal/authenticators/authenticators.go b/internal/authenticators/authenticators.go index bd24ff0b..f7a59e33 100644 --- a/internal/authenticators/authenticators.go +++ b/internal/authenticators/authenticators.go @@ -7,7 +7,7 @@ package authenticators import ( "context" - "k8s.io/apiserver/pkg/authentication/authenticator" + "k8s.io/apiserver/pkg/authentication/user" ) // This interface is similar to the k8s token authenticator, but works with username/passwords instead @@ -31,5 +31,10 @@ import ( // See k8s.io/apiserver/pkg/authentication/authenticator/interfaces.go for the token authenticator // interface, as well as the Response type. type UserAuthenticator interface { - AuthenticateUser(ctx context.Context, username, password string) (*authenticator.Response, bool, error) + AuthenticateUser(ctx context.Context, username, password string) (*Response, bool, error) +} + +type Response struct { + User user.Info + DN string } diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index aaac4926..bbfdba75 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -15,9 +15,9 @@ import ( "github.com/ory/fosite/token/jwt" "github.com/pkg/errors" "golang.org/x/oauth2" - "k8s.io/apiserver/pkg/authentication/authenticator" supervisoroidc "go.pinniped.dev/generated/latest/apis/supervisor/oidc" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/oidc" @@ -112,7 +112,7 @@ func handleAuthRequestForLDAPUpstream( subject := downstreamSubjectFromUpstreamLDAP(ldapUpstream, authenticateResponse) username = authenticateResponse.User.GetName() groups := authenticateResponse.User.GetGroups() - dn := userDNFromAuthenticatedResponse(authenticateResponse) + dn := authenticateResponse.DN customSessionData := &psession.CustomSessionData{ ProviderUID: ldapUpstream.GetResourceUID(), @@ -482,16 +482,7 @@ func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken return nil } -func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentityProviderI, authenticateResponse *authenticator.Response) string { +func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentityProviderI, authenticateResponse *authenticators.Response) string { ldapURL := *ldapUpstream.GetURL() return downstreamsession.DownstreamLDAPSubject(authenticateResponse.User.GetUID(), ldapURL) } - -func userDNFromAuthenticatedResponse(authenticatedResponse *authenticator.Response) string { - // We should always have something here, but do some error checking anyway so it doesn't panic - dnSlice := authenticatedResponse.User.GetExtra()["userDN"] - if len(dnSlice) != 1 { - return "" - } - return dnSlice[0] -} diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 39264037..564ef380 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -19,12 +19,12 @@ import ( "github.com/ory/fosite" "github.com/stretchr/testify/require" "golang.org/x/oauth2" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/utils/pointer" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/csrftoken" @@ -273,18 +273,18 @@ func TestAuthorizationEndpoint(t *testing.T) { parsedUpstreamLDAPURL, err := url.Parse(upstreamLDAPURL) require.NoError(t, err) - ldapAuthenticateFunc := func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) { + ldapAuthenticateFunc := func(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { if username == "" || password == "" { return nil, false, fmt.Errorf("should not have passed empty username or password to the authenticator") } if username == happyLDAPUsername && password == happyLDAPPassword { - return &authenticator.Response{ + return &authenticators.Response{ User: &user.DefaultInfo{ Name: happyLDAPUsernameFromAuthenticator, UID: happyLDAPUID, Groups: happyLDAPGroups, - Extra: map[string][]string{"userDN": {happyLDAPUserDN}}, }, + DN: happyLDAPUserDN, }, true, nil } return nil, false, nil @@ -307,7 +307,7 @@ func TestAuthorizationEndpoint(t *testing.T) { erroringUpstreamLDAPIdentityProvider := oidctestutil.TestUpstreamLDAPIdentityProvider{ Name: ldapUpstreamName, ResourceUID: ldapUpstreamResourceUID, - AuthenticateFunc: func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) { + AuthenticateFunc: func(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { return nil, false, fmt.Errorf("some ldap upstream auth error") }, } diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index d7aa02c0..b5f2c215 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -90,7 +90,7 @@ type UpstreamLDAPIdentityProviderI interface { authenticators.UserAuthenticator // PerformRefresh performs a refresh against the upstream LDAP identity provider - PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error + PerformRefresh(ctx context.Context, userDN, expectedUsername, expectedSubject string) error } type DynamicUpstreamIDPProvider interface { diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 302321b3..c2eebc16 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -172,7 +172,9 @@ func findOIDCProviderByNameAndValidateUID( func upstreamLDAPRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister, username string, subject string) error { // if you have neither a valid ldap session config nor a valid active directory session config - if (s.LDAP == nil || s.LDAP.UserDN == "") && (s.ActiveDirectory == nil || s.ActiveDirectory.UserDN == "") { + validLDAP := s.ProviderType == psession.ProviderTypeLDAP && s.LDAP != nil && s.LDAP.UserDN != "" + validAD := s.ProviderType == psession.ProviderTypeActiveDirectory && s.ActiveDirectory != nil && s.ActiveDirectory.UserDN != "" + if !(validLDAP || validAD) { return errorsx.WithStack(errMissingUpstreamSessionInternalError) } @@ -230,6 +232,9 @@ func getDownstreamUsernameFromPinnipedSession(session *psession.PinnipedSession) if downstreamUsernameInterface == nil { return "", errorsx.WithStack(errMissingUpstreamSessionInternalError) } - downstreamUsername := downstreamUsernameInterface.(string) + downstreamUsername, ok := downstreamUsernameInterface.(string) + if !ok || len(downstreamUsername) == 0 { + return "", errorsx.WithStack(errMissingUpstreamSessionInternalError) + } return downstreamUsername, nil } diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index f8a7ccd4..773ff5a6 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1922,6 +1922,90 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "username in extra is not a string", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + //fositeSessionData: &openid.DefaultSession{}, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyLDAPCustomSessionData, + ), + }, + modifyRefreshTokenStorage: func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) { + refreshTokenSignature := getFositeDataSignature(t, refreshToken) + firstRequester, err := oauthStore.GetRefreshTokenSession(context.Background(), refreshTokenSignature, nil) + require.NoError(t, err) + session := firstRequester.GetSession().(*psession.PinnipedSession) + session.Fosite = &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + Extra: map[string]interface{}{"username": 123}, + }, + } + err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) + require.NoError(t, err) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + require.NoError(t, err) + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, + { + name: "username in extra is an empty string", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + //fositeSessionData: &openid.DefaultSession{}, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyLDAPCustomSessionData, + ), + }, + modifyRefreshTokenStorage: func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) { + refreshTokenSignature := getFositeDataSignature(t, refreshToken) + firstRequester, err := oauthStore.GetRefreshTokenSession(context.Background(), refreshTokenSignature, nil) + require.NoError(t, err) + session := firstRequester.GetSession().(*psession.PinnipedSession) + session.Fosite = &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + Extra: map[string]interface{}{"username": ""}, + }, + } + err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) + require.NoError(t, err) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + require.NoError(t, err) + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, { name: "when the ldap provider in the session storage is found but has the wrong resource UID during the refresh request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 23b193f0..a1513011 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -21,10 +21,10 @@ import ( "gopkg.in/square/go-jose.v2" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/crud" "go.pinniped.dev/internal/fositestorage/authorizationcode" "go.pinniped.dev/internal/fositestorage/openidconnect" @@ -80,7 +80,7 @@ type TestUpstreamLDAPIdentityProvider struct { Name string ResourceUID types.UID URL *url.URL - AuthenticateFunc func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) + AuthenticateFunc func(ctx context.Context, username, password string) (*authenticators.Response, bool, error) performRefreshCallCount int performRefreshArgs []*PerformRefreshArgs PerformRefreshErr error @@ -96,7 +96,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetName() string { return u.Name } -func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string) (*authenticator.Response, bool, error) { +func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { return u.AuthenticateFunc(ctx, username, password) } @@ -104,7 +104,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetURL() *url.URL { return u.URL } -func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error { +func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, userDN, expectedUsername, expectedSubject string) error { if u.performRefreshArgs == nil { u.performRefreshArgs = make([]*PerformRefreshArgs, 0) } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 976b96b8..4df3fde9 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -21,7 +21,6 @@ import ( "github.com/go-ldap/ldap/v3" "github.com/google/uuid" "k8s.io/apimachinery/pkg/types" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/utils/trace" @@ -170,7 +169,7 @@ func (p *Provider) GetConfig() ProviderConfig { return p.c } -func (p *Provider) PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error { +func (p *Provider) PerformRefresh(ctx context.Context, userDN, expectedUsername, expectedSubject string) error { t := trace.FromContext(ctx).Nest("slow ldap refresh attempt", trace.Field{Key: "providerName", Value: p.GetName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches search := p.refreshUserSearchRequest(userDN) @@ -372,7 +371,7 @@ func (p *Provider) TestConnection(ctx context.Context) error { // authentication for a given end user's username. It runs the same logic as AuthenticateUser except it does // not bind as that user, so it does not test their password. It returns the same values that a real call to // AuthenticateUser with the correct password would return. -func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string) (*authenticator.Response, bool, error) { +func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string) (*authenticators.Response, bool, error) { endUserBindFunc := func(conn Conn, foundUserDN string) error { // Act as if the end user bind always succeeds. return nil @@ -381,14 +380,14 @@ func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string) } // Authenticate an end user and return their mapped username, groups, and UID. Implements authenticators.UserAuthenticator. -func (p *Provider) AuthenticateUser(ctx context.Context, username, password string) (*authenticator.Response, bool, error) { +func (p *Provider) AuthenticateUser(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { endUserBindFunc := func(conn Conn, foundUserDN string) error { return conn.Bind(foundUserDN, password) } return p.authenticateUserImpl(ctx, username, endUserBindFunc) } -func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticator.Response, bool, error) { +func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, bool, error) { t := trace.FromContext(ctx).Nest("slow ldap authenticate user attempt", trace.Field{Key: "providerName", Value: p.GetName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches @@ -428,13 +427,13 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bi return nil, false, nil } - response := &authenticator.Response{ + response := &authenticators.Response{ User: &user.DefaultInfo{ Name: mappedUsername, UID: mappedUID, Groups: mappedGroupNames, - Extra: map[string][]string{"userDN": {userDN}}, }, + DN: userDN, } p.traceAuthSuccess(t) return response, true, nil diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index c9295b8f..d04cc3dd 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -18,9 +18,9 @@ import ( "github.com/go-ldap/ldap/v3" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/endpointaddr" "go.pinniped.dev/internal/mocks/mockldapconn" @@ -151,17 +151,16 @@ func TestEndUserAuthentication(t *testing.T) { } // The auth response which matches the exampleUserSearchResult and exampleGroupSearchResult. - expectedAuthResponse := func(editFunc func(r *user.DefaultInfo)) *authenticator.Response { + expectedAuthResponse := func(editFunc func(r *user.DefaultInfo)) *authenticators.Response { u := &user.DefaultInfo{ Name: testUserSearchResultUsernameAttributeValue, UID: base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultUIDAttributeValue)), Groups: []string{testGroupSearchResultGroupNameAttributeValue1, testGroupSearchResultGroupNameAttributeValue2}, - Extra: map[string][]string{"userDN": {testUserSearchResultDNValue}}, } if editFunc != nil { editFunc(u) } - return &authenticator.Response{User: u} + return &authenticators.Response{User: u, DN: testUserSearchResultDNValue} } tests := []struct { @@ -174,7 +173,7 @@ func TestEndUserAuthentication(t *testing.T) { dialError error wantError string wantToSkipDial bool - wantAuthResponse *authenticator.Response + wantAuthResponse *authenticators.Response wantUnauthenticated bool skipDryRunAuthenticateUser bool // tests about when the end user bind fails don't make sense for DryRunAuthenticateUser() }{ @@ -499,13 +498,13 @@ func TestEndUserAuthentication(t *testing.T) { bindEndUserMocks: func(conn *mockldapconn.MockConn) { conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1) }, - wantAuthResponse: &authenticator.Response{ + wantAuthResponse: &authenticators.Response{ User: &user.DefaultInfo{ Name: testUserSearchResultUsernameAttributeValue, UID: base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultUIDAttributeValue)), Groups: []string{"a", "b", "c"}, - Extra: map[string][]string{"userDN": {testUserSearchResultDNValue}}, }, + DN: testUserSearchResultDNValue, }, }, { diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 8cafa810..5531a978 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -20,6 +20,7 @@ import ( "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/upstreamldap" "go.pinniped.dev/test/testlib" ) @@ -677,7 +678,7 @@ func TestSimultaneousLDAPRequestsOnSingleProvider(t *testing.T) { } type authUserResult struct { - response *authenticator.Response + response *authenticators.Response authenticated bool err error }