diff --git a/go.mod b/go.mod index 584c25a5..702d5e2e 100644 --- a/go.mod +++ b/go.mod @@ -15,6 +15,7 @@ require ( github.com/golang/mock v1.6.0 github.com/google/go-cmp v0.5.6 github.com/google/gofuzz v1.2.0 + github.com/google/uuid v1.1.2 // indirect github.com/gorilla/securecookie v1.1.1 github.com/gorilla/websocket v1.4.2 github.com/onsi/ginkgo v1.13.0 // indirect diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index d3041180..677ea2b7 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -306,7 +306,8 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context, Filter: adUpstreamImpl.Spec().GroupSearch().Filter(), GroupNameAttribute: adUpstreamImpl.Spec().GroupSearch().GroupNameAttribute(), }, - Dialer: c.ldapDialer, + Dialer: c.ldapDialer, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, } conditions := upstreamwatchers.ValidateGenericLDAP(ctx, &adUpstreamImpl, c.secretInformer, c.validatedSecretVersionsCache, config) diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go index 932c4816..d3657099 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go @@ -8,6 +8,7 @@ import ( "encoding/base64" "errors" "fmt" + "reflect" "sort" "testing" "time" @@ -217,6 +218,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, } // Make a copy with targeted changes. @@ -531,6 +533,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -588,6 +591,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -645,6 +649,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantErr: controllerlib.ErrSyntheticRequeue.Error(), @@ -701,6 +706,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -824,6 +830,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -943,6 +950,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -992,6 +1000,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1181,6 +1190,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={}))", GroupNameAttribute: "sAMAccountName", }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1230,6 +1240,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1278,6 +1289,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1326,6 +1338,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1561,7 +1574,20 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { // The dialer that was passed in to the controller's constructor should always have been // passed through to the provider. copyOfExpectedValueForResultingCache.Dialer = dialer - require.Equal(t, copyOfExpectedValueForResultingCache, actualIDP.GetConfig()) + + // function equality is awkward. Do the check for equality separately from the rest of the config. + expectedUIDAttributeParsingOverrides := copyOfExpectedValueForResultingCache.UIDAttributeParsingOverrides + actualConfig := actualIDP.GetConfig() + actualUIDAttributeParsingOverrides := actualConfig.UIDAttributeParsingOverrides + copyOfExpectedValueForResultingCache.UIDAttributeParsingOverrides = []upstreamldap.AttributeParsingOverride{} + actualConfig.UIDAttributeParsingOverrides = []upstreamldap.AttributeParsingOverride{} + + require.Len(t, actualUIDAttributeParsingOverrides, 1) + require.Len(t, expectedUIDAttributeParsingOverrides, 1) + require.Equal(t, expectedUIDAttributeParsingOverrides[0].AttributeName, actualUIDAttributeParsingOverrides[0].AttributeName) + require.Equal(t, reflect.ValueOf(expectedUIDAttributeParsingOverrides[0].OverrideFunc).Pointer(), reflect.ValueOf(actualUIDAttributeParsingOverrides[0].OverrideFunc).Pointer()) + + require.Equal(t, copyOfExpectedValueForResultingCache, actualConfig) } actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().ActiveDirectoryIdentityProviders(testNamespace).List(ctx, metav1.ListOptions{}) diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 06d19ac8..89bd3338 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -17,6 +17,8 @@ import ( "strings" "time" + "github.com/google/uuid" + "github.com/go-ldap/ldap/v3" "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" @@ -103,6 +105,15 @@ type ProviderConfig struct { // Dialer exists to enable testing. When nil, will use a default appropriate for production use. Dialer LDAPDialer + + // UIDAttributeParsingOverrides are mappings between an attribute name and a way to parse it when + // it comes out of LDAP. + UIDAttributeParsingOverrides []AttributeParsingOverride +} + +type AttributeParsingOverride struct { + AttributeName string + OverrideFunc func([]byte) (string, error) } // UserSearchConfig contains information about how to search for users in the upstream LDAP IDP. @@ -610,6 +621,12 @@ func (p *Provider) getSearchResultAttributeRawValueEncoded(attributeName string, ) } + for _, override := range p.c.UIDAttributeParsingOverrides { + if attributeName == override.AttributeName { + return override.OverrideFunc(attributeValue) + } + } + return base64.RawURLEncoding.EncodeToString(attributeValue), nil } @@ -653,3 +670,16 @@ func (p *Provider) traceSearchBaseDiscoveryFailure(t *trace.Trace, err error) { t.Step("search base discovery failed", trace.Field{Key: "reason", Value: err.Error()}) } + +func MicrosoftUUIDFromBinary(binaryUUID []byte) (string, error) { + uuidVal, err := uuid.FromBytes(binaryUUID) // start out with the RFC4122 version + if err != nil { + return "", err + } + // then swap it because AD stores the first 3 fields little-endian rather than the expected + // big-endian. + uuidVal[0], uuidVal[1], uuidVal[2], uuidVal[3] = uuidVal[3], uuidVal[2], uuidVal[1], uuidVal[0] + uuidVal[4], uuidVal[5] = uuidVal[5], uuidVal[4] + uuidVal[6], uuidVal[7] = uuidVal[7], uuidVal[6] + return uuidVal.String(), nil +} diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index c4482fdf..12e7f4eb 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -506,6 +506,64 @@ func TestEndUserAuthentication(t *testing.T) { }, }, }, + { + name: "override UID parsing to work with microsoft style objectGUIDs", + username: testUpstreamUsername, + password: testUpstreamPassword, + providerConfig: providerConfig(func(p *ProviderConfig) { + p.UIDAttributeParsingOverrides = []AttributeParsingOverride{{ + AttributeName: "objectGUID", + OverrideFunc: MicrosoftUUIDFromBinary, + }} + p.UserSearch.UIDAttribute = "objectGUID" + }), + searchMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch(func(r *ldap.SearchRequest) { + r.Attributes = []string{testUserSearchUsernameAttribute, "objectGUID"} + })).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testUserSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute(testUserSearchUsernameAttribute, []string{testUserSearchResultUsernameAttributeValue}), + ldap.NewEntryAttribute("objectGUID", []string{"\x01\x02\x03\x04\x05\x06\x07\x08\x09\x10\x11\x12\x13\x14\x15\x16"}), + }, + }, + }}, nil).Times(1) + conn.EXPECT().SearchWithPaging(expectedGroupSearch(nil), expectedGroupSearchPageSize). + Return(exampleGroupSearchResult, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + bindEndUserMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1) + }, + wantAuthResponse: expectedAuthResponse(func(r *user.DefaultInfo) { + r.UID = "04030201-0605-0807-0910-111213141516" + }), + }, + { + name: "override UID parsing when the attribute name doesn't match what's returned does default parsing", + username: testUpstreamUsername, + password: testUpstreamPassword, + providerConfig: providerConfig(func(p *ProviderConfig) { + p.UIDAttributeParsingOverrides = []AttributeParsingOverride{{ + AttributeName: "objectGUID", + OverrideFunc: MicrosoftUUIDFromBinary, + }} + }), + searchMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch(nil)).Return(exampleUserSearchResult, nil).Times(1) + conn.EXPECT().SearchWithPaging(expectedGroupSearch(nil), expectedGroupSearchPageSize). + Return(exampleGroupSearchResult, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + bindEndUserMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1) + }, + wantAuthResponse: expectedAuthResponse(nil), + }, { name: "when dial fails", username: testUpstreamUsername, @@ -1297,3 +1355,36 @@ func TestRealTLSDialing(t *testing.T) { }) } } + +func TestGetMicrosoftFormattedUUID(t *testing.T) { + tests := []struct { + name string + binaryUUID []byte + wantString string + wantErr string + }{ + { + name: "happy path", + binaryUUID: []byte("\x01\x02\x03\x04\x05\x06\x07\x08\x09\x10\x11\x12\x13\x14\x15\x16"), + wantString: "04030201-0605-0807-0910-111213141516", + }, + { + name: "not the right length", + binaryUUID: []byte("2\xf8\xb0\xaa\xb6V\xb1D\x8b(\xee"), + wantErr: "invalid UUID (got 11 bytes)", + }, + } + + for _, test := range tests { + tt := test + t.Run(tt.name, func(t *testing.T) { + actualUUIDString, err := MicrosoftUUIDFromBinary(tt.binaryUUID) + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + } else { + require.NoError(t, err) + } + require.Equal(t, tt.wantString, actualUUIDString) + }) + } +}