From 033e1f0399eb8115f7102768f1db8e7a1138c82b Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 26 May 2021 17:04:20 -0700 Subject: [PATCH 1/3] Add user search base to downstream subject for upstream LDAP - Also add some tests about UTF-8 characters in LDAP attributes --- internal/oidc/auth/auth_handler.go | 12 +++++++-- internal/oidc/auth/auth_handler_test.go | 15 ++++++----- .../provider/dynamic_upstream_idp_provider.go | 2 +- .../testutil/oidctestutil/oidctestutil.go | 6 +++-- internal/upstreamldap/upstreamldap.go | 19 ++++++++++---- internal/upstreamldap/upstreamldap_test.go | 15 +++++++++-- test/deploy/tools/ldap.yaml | 2 +- test/integration/ldap_client_test.go | 25 +++++++++++++++++++ test/integration/supervisor_login_test.go | 4 +-- 9 files changed, 79 insertions(+), 21 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 21aad56c..a2f6f566 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -15,6 +15,7 @@ import ( "github.com/ory/fosite/token/jwt" "github.com/pkg/errors" "golang.org/x/oauth2" + "k8s.io/apiserver/pkg/authentication/authenticator" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/httputil/securityheader" @@ -108,11 +109,10 @@ func handleAuthRequestForLDAPUpstream( return nil } - subject := fmt.Sprintf("%s?%s=%s", ldapUpstream.GetURL(), oidc.IDTokenSubjectClaim, authenticateResponse.User.GetUID()) now := time.Now().UTC() openIDSession := &openid.DefaultSession{ Claims: &jwt.IDTokenClaims{ - Subject: subject, + Subject: downstreamSubjectFromUpstreamLDAP(ldapUpstream, authenticateResponse), RequestedAt: now, AuthTime: now, }, @@ -359,3 +359,11 @@ func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken return nil } + +func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentityProviderI, authenticateResponse *authenticator.Response) string { + ldapURL := *ldapUpstream.GetURL() + q := ldapURL.Query() + q.Set(oidc.IDTokenSubjectClaim, authenticateResponse.User.GetUID()) + ldapURL.RawQuery = q.Encode() + return ldapURL.String() +} diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index d65dbf4b..9c92301e 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -44,7 +44,7 @@ func TestAuthorizationEndpoint(t *testing.T) { downstreamPKCEChallengeMethod = "S256" happyState = "8b-state" downstreamClientID = "pinniped-cli" - upstreamLDAPURL = "ldaps://some-ldap-host:123" + upstreamLDAPURL = "ldaps://some-ldap-host:123?base=ou%3Dusers%2Cdc%3Dpinniped%2Cdc%3Ddev" htmlContentType = "text/html; charset=utf-8" ) @@ -158,9 +158,12 @@ func TestAuthorizationEndpoint(t *testing.T) { happyLDAPUID := "some-ldap-uid" happyLDAPGroups := []string{"group1", "group2", "group3"} + parsedUpstreamLDAPURL, err := url.Parse(upstreamLDAPURL) + require.NoError(t, err) + upstreamLDAPIdentityProvider := oidctestutil.TestUpstreamLDAPIdentityProvider{ Name: "some-ldap-idp", - URL: upstreamLDAPURL, + URL: parsedUpstreamLDAPURL, AuthenticateFunc: func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) { if username == "" || password == "" { return nil, false, fmt.Errorf("should not have passed empty username or password to the authenticator") @@ -384,7 +387,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: htmlContentType, wantRedirectLocationRegexp: happyAuthcodeDownstreamRedirectLocationRegexp, wantBodyStringWithLocationInHref: false, - wantDownstreamIDTokenSubject: upstreamLDAPURL + "?sub=" + happyLDAPUID, + wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, wantDownstreamIDTokenGroups: happyLDAPGroups, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -443,7 +446,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: htmlContentType, wantRedirectLocationRegexp: happyAuthcodeDownstreamRedirectLocationRegexp, wantBodyStringWithLocationInHref: false, - wantDownstreamIDTokenSubject: upstreamLDAPURL + "?sub=" + happyLDAPUID, + wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, wantDownstreamIDTokenGroups: happyLDAPGroups, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -525,7 +528,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: htmlContentType, wantRedirectLocationRegexp: downstreamRedirectURIWithDifferentPort + `\?code=([^&]+)&scope=openid&state=` + happyState, wantBodyStringWithLocationInHref: false, - wantDownstreamIDTokenSubject: upstreamLDAPURL + "?sub=" + happyLDAPUID, + wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, wantDownstreamIDTokenGroups: happyLDAPGroups, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -941,7 +944,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: htmlContentType, wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=&state=` + happyState, // no scopes granted wantBodyStringWithLocationInHref: false, - wantDownstreamIDTokenSubject: upstreamLDAPURL + "?sub=" + happyLDAPUID, + wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, wantDownstreamIDTokenGroups: happyLDAPGroups, wantDownstreamRequestedScopes: []string{"email"}, // only email was requested diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 50965abc..af24e724 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -56,7 +56,7 @@ type UpstreamLDAPIdentityProviderI interface { // Return a URL which uniquely identifies this LDAP provider, e.g. "ldaps://host.example.com:1234". // This URL is not used for connecting to the provider, but rather is used for creating a globally unique user // identifier by being combined with the user's UID, since user UIDs are only unique within one provider. - GetURL() string + GetURL() *url.URL // A method for performing user authentication against the upstream LDAP provider. authenticators.UserAuthenticator diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index e4718270..b8e7b0de 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -51,10 +51,12 @@ type ExchangeAuthcodeAndValidateTokenArgs struct { type TestUpstreamLDAPIdentityProvider struct { Name string - URL string + URL *url.URL AuthenticateFunc func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) } +var _ provider.UpstreamLDAPIdentityProviderI = &TestUpstreamLDAPIdentityProvider{} + func (u *TestUpstreamLDAPIdentityProvider) GetName() string { return u.Name } @@ -63,7 +65,7 @@ func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, return u.AuthenticateFunc(ctx, username, password) } -func (u *TestUpstreamLDAPIdentityProvider) GetURL() string { +func (u *TestUpstreamLDAPIdentityProvider) GetURL() *url.URL { return u.URL } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index f857c730..a7732261 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -11,17 +11,19 @@ import ( "errors" "fmt" "net" + "net/url" "sort" "strings" "time" - "k8s.io/utils/trace" - "github.com/go-ldap/ldap/v3" "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/utils/trace" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/endpointaddr" + "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" ) @@ -138,6 +140,9 @@ type Provider struct { c ProviderConfig } +var _ provider.UpstreamLDAPIdentityProviderI = &Provider{} +var _ authenticators.UserAuthenticator = &Provider{} + // Create a Provider. The config is not a pointer to ensure that a copy of the config is created, // making the resulting Provider use an effectively read-only configuration. func New(config ProviderConfig) *Provider { @@ -249,11 +254,15 @@ func (p *Provider) GetName() string { return p.c.Name } -// Return a URL which uniquely identifies this LDAP provider, e.g. "ldaps://host.example.com:1234". +// Return a URL which uniquely identifies this LDAP provider, e.g. "ldaps://host.example.com:1234?base=user-search-base". // This URL is not used for connecting to the provider, but rather is used for creating a globally unique user // identifier by being combined with the user's UID, since user UIDs are only unique within one provider. -func (p *Provider) GetURL() string { - return fmt.Sprintf("%s://%s", ldapsScheme, p.c.Host) +func (p *Provider) GetURL() *url.URL { + u := &url.URL{Scheme: ldapsScheme, Host: p.c.Host} + q := u.Query() + q.Set("base", p.c.UserSearch.Base) + u.RawQuery = q.Encode() + return u } // TestConnection provides a method for testing the connection and bind settings. It performs a dial and bind diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index fd8b9658..c9e88be3 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -1115,8 +1115,19 @@ func TestGetConfig(t *testing.T) { } func TestGetURL(t *testing.T) { - require.Equal(t, "ldaps://ldap.example.com:1234", New(ProviderConfig{Host: "ldap.example.com:1234"}).GetURL()) - require.Equal(t, "ldaps://ldap.example.com", New(ProviderConfig{Host: "ldap.example.com"}).GetURL()) + require.Equal(t, + "ldaps://ldap.example.com:1234?base=ou%3Dusers%2Cdc%3Dpinniped%2Cdc%3Ddev", + New(ProviderConfig{ + Host: "ldap.example.com:1234", + UserSearch: UserSearchConfig{Base: "ou=users,dc=pinniped,dc=dev"}, + }).GetURL().String()) + + require.Equal(t, + "ldaps://ldap.example.com?base=ou%3Dusers%2Cdc%3Dpinniped%2Cdc%3Ddev", + New(ProviderConfig{ + Host: "ldap.example.com", + UserSearch: UserSearchConfig{Base: "ou=users,dc=pinniped,dc=dev"}, + }).GetURL().String()) } // Testing of host parsing, TLS negotiation, and CA bundle, etc. for the production code's dialer. diff --git a/test/deploy/tools/ldap.yaml b/test/deploy/tools/ldap.yaml index 25410fcb..ab771b10 100644 --- a/test/deploy/tools/ldap.yaml +++ b/test/deploy/tools/ldap.yaml @@ -41,7 +41,7 @@ ldap.ldif: | objectClass: shadowAccount cn: pinny sn: Seal - givenName: Pinny + givenName: Pinny the 🦭 mail: pinny.ldap@example.com userPassword: (@= data.values.pinny_ldap_password @) uid: pinny diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 765b1f45..e95173f1 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -167,6 +167,31 @@ func TestLDAPSearch(t *testing.T) { User: &user.DefaultInfo{Name: "Seal", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, // note that the final answer has case preserved from the entry }, }, + { + name: "when the UsernameAttribute or UIDAttribute are attributes whose value contains UTF-8 data", + username: "pinny", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { + p.UserSearch.Filter = "cn={}" + p.UserSearch.UsernameAttribute = "givenName" + p.UserSearch.UIDAttribute = "givenName" + })), + wantAuthResponse: &authenticator.Response{ + User: &user.DefaultInfo{Name: "Pinny the 🦭", UID: "Pinny the 🦭", Groups: []string{"ball-game-players", "seals"}}, + }, + }, + { + name: "when the search filter is searching on an attribute whose value contains UTF-8 data", + username: "Pinny the 🦭", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { + p.UserSearch.Filter = "givenName={}" + p.UserSearch.UsernameAttribute = "cn" + })), + wantAuthResponse: &authenticator.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + }, + }, { name: "when the UsernameAttribute is dn and there is no user search filter provided", username: "cn=pinny,ou=users,dc=pinniped,dc=dev", diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 1690c109..12e4553e 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -119,7 +119,7 @@ func TestSupervisorLogin(t *testing.T) { }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: regexp.QuoteMeta( - "ldaps://" + env.SupervisorUpstreamLDAP.Host + "?sub=" + env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue, + "ldaps://" + env.SupervisorUpstreamLDAP.Host + "?base=" + url.QueryEscape(env.SupervisorUpstreamLDAP.UserSearchBase) + "&sub=" + env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue, ), // the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute wantDownstreamIDTokenUsernameToMatch: regexp.QuoteMeta(env.SupervisorUpstreamLDAP.TestUserMailAttributeValue), @@ -176,7 +176,7 @@ func TestSupervisorLogin(t *testing.T) { }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: regexp.QuoteMeta( - "ldaps://" + env.SupervisorUpstreamLDAP.StartTLSOnlyHost + "?sub=" + env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue, + "ldaps://" + env.SupervisorUpstreamLDAP.StartTLSOnlyHost + "?base=" + url.QueryEscape(env.SupervisorUpstreamLDAP.UserSearchBase) + "&sub=" + env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue, ), // the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute wantDownstreamIDTokenUsernameToMatch: regexp.QuoteMeta(env.SupervisorUpstreamLDAP.TestUserDN), From 81148866e0ef0a8e09341823243222491a89ec69 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 27 May 2021 09:25:48 -0700 Subject: [PATCH 2/3] URL query escape the upstream OIDC subject in the downstream subject URL --- internal/oidc/callback/callback_handler.go | 6 +++- .../oidc/callback/callback_handler_test.go | 31 ++++++++++--------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 5bece1d9..1b14c788 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -228,7 +228,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( return "", "", httperr.New(http.StatusUnprocessableEntity, "subject claim in upstream ID token has invalid format") } - subject := fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, oidc.IDTokenSubjectClaim, upstreamSubject) + subject := downstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString, upstreamSubject) usernameClaimName := upstreamIDPConfig.GetUsernameClaim() if usernameClaimName == "" { @@ -282,6 +282,10 @@ func getSubjectAndUsernameFromUpstreamIDToken( return subject, username, nil } +func downstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString string, upstreamSubject string) string { + return fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, oidc.IDTokenSubjectClaim, url.QueryEscape(upstreamSubject)) +} + func getGroupsFromUpstreamIDToken( upstreamIDPConfig provider.UpstreamOIDCIdentityProviderI, idTokenClaims map[string]interface{}, diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index c999cba4..583ee943 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -28,9 +28,10 @@ import ( const ( happyUpstreamIDPName = "upstream-idp-name" - upstreamIssuer = "https://my-upstream-issuer.com" - upstreamSubject = "abc123-some-guid" - upstreamUsername = "test-pinniped-username" + upstreamIssuer = "https://my-upstream-issuer.com" + upstreamSubject = "abc123-some guid" // has a space character which should get escaped in URL + queryEscapedUpstreamSubject = "abc123-some+guid" + upstreamUsername = "test-pinniped-username" upstreamUsernameClaim = "the-user-claim" upstreamGroupsClaim = "the-groups-claim" @@ -141,7 +142,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: upstreamUsername, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -160,8 +161,8 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, - wantDownstreamIDTokenUsername: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, + wantDownstreamIDTokenUsername: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenGroups: []string{}, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, @@ -180,7 +181,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: "joe@whitehouse.gov", wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -201,7 +202,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: "joe@whitehouse.gov", wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -223,7 +224,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, // succeed despite `email_verified=false` because we're not using the email claim for anything wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: "joe", wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -268,7 +269,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: upstreamSubject, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -287,7 +288,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: upstreamUsername, wantDownstreamIDTokenGroups: []string{"notAnArrayGroup1 notAnArrayGroup2"}, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -306,7 +307,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: upstreamUsername, wantDownstreamIDTokenGroups: []string{"group1", "group2"}, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -445,7 +446,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=&state=` + happyDownstreamState, wantDownstreamIDTokenUsername: upstreamUsername, - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamRequestedScopes: []string{"profile", "email"}, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamNonce: downstreamNonce, @@ -467,7 +468,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=openid\+offline_access&state=` + happyDownstreamState, wantDownstreamIDTokenUsername: upstreamUsername, - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamRequestedScopes: []string{"openid", "offline_access"}, wantDownstreamGrantedScopes: []string{"openid", "offline_access"}, wantDownstreamIDTokenGroups: upstreamGroupMembership, @@ -548,7 +549,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: upstreamUsername, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, From d2251d2ea7a3ed4404482300ac80bade0d88ea67 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 27 May 2021 13:47:10 -0700 Subject: [PATCH 3/3] Use base64 binary-encoded value as UID for LDAP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is to allow the use of binary LDAP entry attributes as the UID. For example, a user might like to configure AD’s objectGUID or maybe objectSid attributes as the UID attribute. This negatively impacts the readability of the UID when it did not come from a binary value, but we're considering this an okay trade-off to keep things simple for now. In the future, we may offer more customizable encoding options for binary attributes. These UIDs are currently only used in the downstream OIDC `sub` claim. They do not effect the user's identity on the Kubernetes cluster, which is only based on their mapped username and group memberships from the upstream identity provider. We are not currently supporting any special encoding for those username and group name LDAP attributes, so their values in the LDAP entry must be ASCII or UTF-8 in order for them to be interpreted correctly. --- internal/upstreamldap/upstreamldap.go | 29 ++++++++++++- internal/upstreamldap/upstreamldap_test.go | 7 ++-- test/integration/ldap_client_test.go | 49 +++++++++++++--------- test/integration/supervisor_login_test.go | 8 +++- 4 files changed, 67 insertions(+), 26 deletions(-) diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index a7732261..11f800eb 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -8,6 +8,7 @@ import ( "context" "crypto/tls" "crypto/x509" + "encoding/base64" "errors" "fmt" "net" @@ -417,7 +418,9 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c return "", "", nil, err } - mappedUID, err := p.getSearchResultAttributeValue(p.c.UserSearch.UIDAttribute, userEntry, username) + // We would like to support binary typed attributes for UIDs, so always read them as binary and encode them, + // even when the attribute may not be binary. + mappedUID, err := p.getSearchResultAttributeRawValueEncoded(p.c.UserSearch.UIDAttribute, userEntry, username) if err != nil { return "", "", nil, err } @@ -526,6 +529,30 @@ func (p *Provider) escapeUsernameForSearchFilter(username string) string { return ldap.EscapeFilter(username) } +// Returns the (potentially) binary data of the attribute's value, base64 URL encoded. +func (p *Provider) getSearchResultAttributeRawValueEncoded(attributeName string, entry *ldap.Entry, username string) (string, error) { + if attributeName == distinguishedNameAttributeName { + return base64.RawURLEncoding.EncodeToString([]byte(entry.DN)), nil + } + + attributeValues := entry.GetRawAttributeValues(attributeName) + + if len(attributeValues) != 1 { + return "", fmt.Errorf(`found %d values for attribute "%s" while searching for user "%s", but expected 1 result`, + len(attributeValues), attributeName, username, + ) + } + + attributeValue := attributeValues[0] + if len(attributeValue) == 0 { + return "", fmt.Errorf(`found empty value for attribute "%s" while searching for user "%s", but expected value to be non-empty`, + attributeName, username, + ) + } + + return base64.RawURLEncoding.EncodeToString(attributeValue), nil +} + func (p *Provider) getSearchResultAttributeValue(attributeName string, entry *ldap.Entry, username string) (string, error) { if attributeName == distinguishedNameAttributeName { return entry.DN, nil diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index c9e88be3..35609bcf 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -6,6 +6,7 @@ package upstreamldap import ( "context" "crypto/tls" + "encoding/base64" "errors" "fmt" "net" @@ -153,7 +154,7 @@ func TestEndUserAuthentication(t *testing.T) { expectedAuthResponse := func(editFunc func(r *user.DefaultInfo)) *authenticator.Response { u := &user.DefaultInfo{ Name: testUserSearchResultUsernameAttributeValue, - UID: testUserSearchResultUIDAttributeValue, + UID: base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultUIDAttributeValue)), Groups: []string{testGroupSearchResultGroupNameAttributeValue1, testGroupSearchResultGroupNameAttributeValue2}, } if editFunc != nil { @@ -311,7 +312,7 @@ func TestEndUserAuthentication(t *testing.T) { conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1) }, wantAuthResponse: expectedAuthResponse(func(r *user.DefaultInfo) { - r.UID = testUserSearchResultDNValue + r.UID = base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultDNValue)) }), }, { @@ -477,7 +478,7 @@ func TestEndUserAuthentication(t *testing.T) { wantAuthResponse: &authenticator.Response{ User: &user.DefaultInfo{ Name: testUserSearchResultUsernameAttributeValue, - UID: testUserSearchResultUIDAttributeValue, + UID: base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultUIDAttributeValue)), Groups: []string{"a", "b", "c"}, }, }, diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index e95173f1..5e4735c3 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -5,6 +5,7 @@ package integration import ( "context" + "encoding/base64" "fmt" "io" "net" @@ -58,6 +59,10 @@ func TestLDAPSearch(t *testing.T) { pinnyPassword := env.SupervisorUpstreamLDAP.TestUserPassword + b64 := func(s string) string { + return base64.RawURLEncoding.EncodeToString([]byte(s)) + } + tests := []struct { name string username string @@ -73,7 +78,7 @@ func TestLDAPSearch(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(nil)), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -85,7 +90,7 @@ func TestLDAPSearch(t *testing.T) { p.ConnectionProtocol = upstreamldap.StartTLS })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -94,7 +99,7 @@ func TestLDAPSearch(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.Base = "dc=pinniped,dc=dev" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -103,7 +108,7 @@ func TestLDAPSearch(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.Filter = "(cn={})" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -115,7 +120,7 @@ func TestLDAPSearch(t *testing.T) { p.UserSearch.Filter = "cn={}" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "cn=pinny,ou=users,dc=pinniped,dc=dev", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "cn=pinny,ou=users,dc=pinniped,dc=dev", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -126,7 +131,7 @@ func TestLDAPSearch(t *testing.T) { p.UserSearch.Filter = "(|(cn={})(mail={}))" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -137,7 +142,7 @@ func TestLDAPSearch(t *testing.T) { p.UserSearch.Filter = "(|(cn={})(mail={}))" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -146,7 +151,7 @@ func TestLDAPSearch(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.UIDAttribute = "dn" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "cn=pinny,ou=users,dc=pinniped,dc=dev", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("cn=pinny,ou=users,dc=pinniped,dc=dev"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -155,7 +160,7 @@ func TestLDAPSearch(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.UIDAttribute = "sn" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "Seal", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("Seal"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -164,7 +169,7 @@ func TestLDAPSearch(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.UsernameAttribute = "sn" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "Seal", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, // note that the final answer has case preserved from the entry + User: &user.DefaultInfo{Name: "Seal", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, // note that the final answer has case preserved from the entry }, }, { @@ -177,7 +182,7 @@ func TestLDAPSearch(t *testing.T) { p.UserSearch.UIDAttribute = "givenName" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "Pinny the 🦭", UID: "Pinny the 🦭", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "Pinny the 🦭", UID: b64("Pinny the 🦭"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -189,7 +194,7 @@ func TestLDAPSearch(t *testing.T) { p.UserSearch.UsernameAttribute = "cn" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, }, }, { @@ -210,7 +215,7 @@ func TestLDAPSearch(t *testing.T) { p.GroupSearch.Base = "" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, }, }, { @@ -221,7 +226,7 @@ func TestLDAPSearch(t *testing.T) { p.GroupSearch.Base = "ou=users,dc=pinniped,dc=dev" // there are no groups under this part of the tree })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, }, }, { @@ -232,7 +237,7 @@ func TestLDAPSearch(t *testing.T) { p.GroupSearch.GroupNameAttribute = "dn" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{ "cn=ball-game-players,ou=beach-groups,ou=groups,dc=pinniped,dc=dev", "cn=seals,ou=groups,dc=pinniped,dc=dev", }}, @@ -246,7 +251,7 @@ func TestLDAPSearch(t *testing.T) { p.GroupSearch.GroupNameAttribute = "objectClass" // silly example, but still a meaningful test })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"groupOfNames", "groupOfNames"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"groupOfNames", "groupOfNames"}}, }, }, { @@ -257,7 +262,7 @@ func TestLDAPSearch(t *testing.T) { p.GroupSearch.Filter = "(&(&(objectClass=groupOfNames)(member={}))(cn=seals))" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"seals"}}, }, }, { @@ -268,7 +273,7 @@ func TestLDAPSearch(t *testing.T) { p.GroupSearch.Filter = "foobar={}" // foobar is not a valid attribute name for this LDAP server's schema })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, }, }, { @@ -593,7 +598,7 @@ func TestLDAPSearch(t *testing.T) { } } -func TestSimultaneousRequestsOnSingleProvider(t *testing.T) { +func TestSimultaneousLDAPRequestsOnSingleProvider(t *testing.T) { env := library.IntegrationEnv(t) // Note that these tests depend on the values hard-coded in the LDIF file in test/deploy/tools/ldap.yaml. @@ -614,6 +619,10 @@ func TestSimultaneousRequestsOnSingleProvider(t *testing.T) { provider := upstreamldap.New(*defaultProviderConfig(env, ldapHostPort)) + b64 := func(s string) string { + return base64.RawURLEncoding.EncodeToString([]byte(s)) + } + // Making multiple simultaneous requests on the same upstreamldap.Provider instance should all succeed // without triggering the race detector. iterations := 150 @@ -639,7 +648,7 @@ func TestSimultaneousRequestsOnSingleProvider(t *testing.T) { assert.NoError(t, result.err) assert.True(t, result.authenticated, "expected the user to be authenticated, but they were not") assert.Equal(t, &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: "1000", Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, }, result.response) } } diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 12e4553e..5e550580 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -119,7 +119,9 @@ func TestSupervisorLogin(t *testing.T) { }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: regexp.QuoteMeta( - "ldaps://" + env.SupervisorUpstreamLDAP.Host + "?base=" + url.QueryEscape(env.SupervisorUpstreamLDAP.UserSearchBase) + "&sub=" + env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue, + "ldaps://" + env.SupervisorUpstreamLDAP.Host + + "?base=" + url.QueryEscape(env.SupervisorUpstreamLDAP.UserSearchBase) + + "&sub=" + base64.RawURLEncoding.EncodeToString([]byte(env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue)), ), // the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute wantDownstreamIDTokenUsernameToMatch: regexp.QuoteMeta(env.SupervisorUpstreamLDAP.TestUserMailAttributeValue), @@ -176,7 +178,9 @@ func TestSupervisorLogin(t *testing.T) { }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: regexp.QuoteMeta( - "ldaps://" + env.SupervisorUpstreamLDAP.StartTLSOnlyHost + "?base=" + url.QueryEscape(env.SupervisorUpstreamLDAP.UserSearchBase) + "&sub=" + env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue, + "ldaps://" + env.SupervisorUpstreamLDAP.StartTLSOnlyHost + + "?base=" + url.QueryEscape(env.SupervisorUpstreamLDAP.UserSearchBase) + + "&sub=" + base64.RawURLEncoding.EncodeToString([]byte(env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue)), ), // the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute wantDownstreamIDTokenUsernameToMatch: regexp.QuoteMeta(env.SupervisorUpstreamLDAP.TestUserDN),