parsing objectGUID as human-readable string version

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

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)

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{})

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
+}

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)
+		})
+	}
+}