From c155c6e629554bbe6f18a798c1375a9758d828da Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Wed, 15 Dec 2021 10:30:36 -0500 Subject: [PATCH] Clean up nits in AD code - Make everything private - Drop unused AuthTime field - Use %q format string instead of "%s" - Only rely on GetRawAttributeValues in AttributeUnchangedSinceLogin Signed-off-by: Monis Khan --- .../active_directory_upstream_watcher.go | 44 +++++---- .../active_directory_upstream_watcher_test.go | 92 +++++++++---------- .../provider/dynamic_upstream_idp_provider.go | 2 - internal/upstreamldap/upstreamldap.go | 56 +++++------ internal/upstreamldap/upstreamldap_test.go | 10 +- test/integration/ldap_client_test.go | 6 +- 6 files changed, 106 insertions(+), 104 deletions(-) diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index 633fb438..c96d20a4 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -58,15 +58,15 @@ const ( defaultActiveDirectoryGroupSearchFilter = "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={}))" sAMAccountNameAttribute = "sAMAccountName" - // PwdLastSetAttribute is the date and time that the password for this account was last changed. + // pwdLastSetAttribute is the date and time that the password for this account was last changed. // https://docs.microsoft.com/en-us/windows/win32/adschema/a-pwdlastset - PwdLastSetAttribute = "pwdLastSet" - // UserAccountControlAttribute represents a bitmap of user properties. + pwdLastSetAttribute = "pwdLastSet" + // userAccountControlAttribute represents a bitmap of user properties. // https://docs.microsoft.com/en-us/troubleshoot/windows-server/identity/useraccountcontrol-manipulate-account-properties - UserAccountControlAttribute = "userAccountControl" - // UserAccountControlComputedAttribute represents a bitmap of user properties. + userAccountControlAttribute = "userAccountControl" + // userAccountControlComputedAttribute represents a bitmap of user properties. // https://docs.microsoft.com/en-us/windows/win32/adschema/a-msds-user-account-control-computed - UserAccountControlComputedAttribute = "msDS-User-Account-Control-Computed" + userAccountControlComputedAttribute = "msDS-User-Account-Control-Computed" // 0x0002 ACCOUNTDISABLE in userAccountControl bitmap. accountDisabledBitmapValue = 2 // 0x0010 UF_LOCKOUT in msDS-User-Account-Control-Computed bitmap. @@ -334,17 +334,21 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context, Filter: adUpstreamImpl.Spec().GroupSearch().Filter(), GroupNameAttribute: adUpstreamImpl.Spec().GroupSearch().GroupNameAttribute(), }, - Dialer: c.ldapDialer, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + Dialer: c.ldapDialer, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){ + "objectGUID": microsoftUUIDFromBinaryAttr("objectGUID"), + }, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ - PwdLastSetAttribute: upstreamldap.AttributeUnchangedSinceLogin(PwdLastSetAttribute), - UserAccountControlAttribute: ValidUserAccountControl, - UserAccountControlComputedAttribute: ValidComputedUserAccountControl, + pwdLastSetAttribute: upstreamldap.AttributeUnchangedSinceLogin(pwdLastSetAttribute), + userAccountControlAttribute: validUserAccountControl, + userAccountControlComputedAttribute: validComputedUserAccountControl, }, } if spec.GroupSearch.Attributes.GroupName == "" { - config.GroupAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){defaultActiveDirectoryGroupNameAttributeName: GroupSAMAccountNameWithDomainSuffix} + config.GroupAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){ + defaultActiveDirectoryGroupNameAttributeName: groupSAMAccountNameWithDomainSuffix, + } } conditions := upstreamwatchers.ValidateGenericLDAP(ctx, adUpstreamImpl, c.secretInformer, c.validatedSecretVersionsCache, config) @@ -378,7 +382,7 @@ func (c *activeDirectoryWatcherController) updateStatus(ctx context.Context, ups } } -func MicrosoftUUIDFromBinary(attributeName string) func(entry *ldap.Entry) (string, error) { +func microsoftUUIDFromBinaryAttr(attributeName string) func(entry *ldap.Entry) (string, error) { // validation has already been done so we can just get the attribute... return func(entry *ldap.Entry) (string, error) { binaryUUID := entry.GetRawAttributeValue(attributeName) @@ -399,18 +403,18 @@ func microsoftUUIDFromBinary(binaryUUID []byte) (string, error) { return uuidVal.String(), nil } -func GroupSAMAccountNameWithDomainSuffix(entry *ldap.Entry) (string, error) { +func groupSAMAccountNameWithDomainSuffix(entry *ldap.Entry) (string, error) { sAMAccountNameAttributeValues := entry.GetAttributeValues(sAMAccountNameAttribute) if len(sAMAccountNameAttributeValues) != 1 { - return "", fmt.Errorf(`found %d values for attribute "%s", but expected 1 result`, + return "", fmt.Errorf(`found %d values for attribute %q, but expected 1 result`, len(sAMAccountNameAttributeValues), sAMAccountNameAttribute, ) } sAMAccountName := sAMAccountNameAttributeValues[0] if len(sAMAccountName) == 0 { - return "", fmt.Errorf(`found empty value for attribute "%s", but expected value to be non-empty`, + return "", fmt.Errorf(`found empty value for attribute %q, but expected value to be non-empty`, sAMAccountNameAttribute, ) } @@ -433,8 +437,8 @@ func getDomainFromDistinguishedName(distinguishedName string) (string, error) { return strings.Join(domainComponents[1:], "."), nil } -func ValidUserAccountControl(entry *ldap.Entry, _ provider.StoredRefreshAttributes) error { - userAccountControl, err := strconv.Atoi(entry.GetAttributeValue(UserAccountControlAttribute)) +func validUserAccountControl(entry *ldap.Entry, _ provider.StoredRefreshAttributes) error { + userAccountControl, err := strconv.Atoi(entry.GetAttributeValue(userAccountControlAttribute)) if err != nil { return err } @@ -446,8 +450,8 @@ func ValidUserAccountControl(entry *ldap.Entry, _ provider.StoredRefreshAttribut return nil } -func ValidComputedUserAccountControl(entry *ldap.Entry, _ provider.StoredRefreshAttributes) error { - userAccountControl, err := strconv.Atoi(entry.GetAttributeValue(UserAccountControlComputedAttribute)) +func validComputedUserAccountControl(entry *ldap.Entry, _ provider.StoredRefreshAttributes) error { + userAccountControl, err := strconv.Atoi(entry.GetAttributeValue(userAccountControlComputedAttribute)) if err != nil { return err } 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 5c47a5b8..76a35ca2 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go @@ -220,11 +220,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, } @@ -541,11 +541,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -602,11 +602,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: "sAMAccountName", }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -666,11 +666,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -730,11 +730,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -793,11 +793,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -927,11 +927,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -1056,11 +1056,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }}, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1111,11 +1111,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -1315,12 +1315,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={}))", GroupNameAttribute: "sAMAccountName", }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, - GroupAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"sAMAccountName": GroupSAMAccountNameWithDomainSuffix}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, + GroupAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"sAMAccountName": groupSAMAccountNameWithDomainSuffix}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -1373,11 +1373,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -1434,11 +1434,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -1489,11 +1489,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -1690,11 +1690,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": MicrosoftUUIDFromBinary("objectGUID")}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), - "userAccountControl": ValidUserAccountControl, - "msDS-User-Account-Control-Computed": ValidComputedUserAccountControl, + "userAccountControl": validUserAccountControl, + "msDS-User-Account-Control-Computed": validComputedUserAccountControl, }, }, }, @@ -1931,7 +1931,7 @@ func TestGroupSAMAccountNameWithDomainSuffix(t *testing.T) { for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { - suffixedSAMAccountName, err := GroupSAMAccountNameWithDomainSuffix(tt.entry) + suffixedSAMAccountName, err := groupSAMAccountNameWithDomainSuffix(tt.entry) if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) } else { @@ -2074,7 +2074,7 @@ func TestValidUserAccountControl(t *testing.T) { for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { - err := ValidUserAccountControl(tt.entry, provider.StoredRefreshAttributes{}) + err := validUserAccountControl(tt.entry, provider.StoredRefreshAttributes{}) if tt.wantErr != "" { require.Error(t, err) @@ -2135,7 +2135,7 @@ func TestValidComputedUserAccountControl(t *testing.T) { for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { - err := ValidComputedUserAccountControl(tt.entry, provider.StoredRefreshAttributes{}) + err := validComputedUserAccountControl(tt.entry, provider.StoredRefreshAttributes{}) if tt.wantErr != "" { require.Error(t, err) diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 0770e2c3..2c34dcbe 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -7,7 +7,6 @@ import ( "context" "net/url" "sync" - "time" "golang.org/x/oauth2" "k8s.io/apimachinery/pkg/types" @@ -101,7 +100,6 @@ type StoredRefreshAttributes struct { Username string Subject string DN string - AuthTime time.Time AdditionalAttributes map[string]string } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 594aac1f..0a7e37a2 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -184,14 +184,14 @@ func (p *Provider) PerformRefresh(ctx context.Context, storedRefreshAttributes p // if any more or less than one entry, error. // we don't need to worry about logging this because we know it's a dn. if len(searchResult.Entries) != 1 { - return fmt.Errorf(`searching for user "%s" resulted in %d search results, but expected 1 result`, + return fmt.Errorf(`searching for user %q resulted in %d search results, but expected 1 result`, userDN, len(searchResult.Entries), ) } userEntry := searchResult.Entries[0] if len(userEntry.DN) == 0 { - return fmt.Errorf(`searching for user with original DN "%s" resulted in search result without DN`, userDN) + return fmt.Errorf(`searching for user with original DN %q resulted in search result without DN`, userDN) } newUsername, err := p.getSearchResultAttributeValue(p.c.UserSearch.UsernameAttribute, userEntry, userDN) @@ -199,7 +199,7 @@ func (p *Provider) PerformRefresh(ctx context.Context, storedRefreshAttributes p return err } if newUsername != storedRefreshAttributes.Username { - return fmt.Errorf(`searching for user "%s" returned a different username than the previous value. expected: "%s", actual: "%s"`, + return fmt.Errorf(`searching for user %q returned a different username than the previous value. expected: %q, actual: %q`, userDN, storedRefreshAttributes.Username, newUsername, ) } @@ -210,12 +210,12 @@ func (p *Provider) PerformRefresh(ctx context.Context, storedRefreshAttributes p } newSubject := downstreamsession.DownstreamLDAPSubject(newUID, *p.GetURL()) if newSubject != storedRefreshAttributes.Subject { - return fmt.Errorf(`searching for user "%s" produced a different subject than the previous value. expected: "%s", actual: "%s"`, userDN, storedRefreshAttributes.Subject, newSubject) + return fmt.Errorf(`searching for user %q produced a different subject than the previous value. expected: %q, actual: %q`, userDN, storedRefreshAttributes.Subject, newSubject) } for attribute, validateFunc := range p.c.RefreshAttributeChecks { err = validateFunc(userEntry, storedRefreshAttributes) if err != nil { - return fmt.Errorf(`validation for attribute "%s" failed during upstream refresh: %w`, attribute, err) + return fmt.Errorf(`validation for attribute %q failed during upstream refresh: %w`, attribute, err) } } // we checked that the user still exists and their information is the same, so just return. @@ -227,19 +227,19 @@ func (p *Provider) performRefresh(ctx context.Context, userDN string) (*ldap.Sea conn, err := p.dial(ctx) if err != nil { - return nil, fmt.Errorf(`error dialing host "%s": %w`, p.c.Host, err) + return nil, fmt.Errorf(`error dialing host %q: %w`, p.c.Host, err) } defer conn.Close() err = conn.Bind(p.c.BindUsername, p.c.BindPassword) if err != nil { - return nil, fmt.Errorf(`error binding as "%s" before user search: %w`, p.c.BindUsername, err) + return nil, fmt.Errorf(`error binding as %q before user search: %w`, p.c.BindUsername, err) } searchResult, err := conn.Search(search) if err != nil { - return nil, fmt.Errorf(`error searching for user "%s": %w`, userDN, err) + return nil, fmt.Errorf(`error searching for user %q: %w`, userDN, err) } return searchResult, nil } @@ -369,13 +369,13 @@ func (p *Provider) TestConnection(ctx context.Context) error { conn, err := p.dial(ctx) if err != nil { - return fmt.Errorf(`error dialing host "%s": %w`, p.c.Host, err) + return fmt.Errorf(`error dialing host %q: %w`, p.c.Host, err) } defer conn.Close() err = conn.Bind(p.c.BindUsername, p.c.BindPassword) if err != nil { - return fmt.Errorf(`error binding as "%s": %w`, p.c.BindUsername, err) + return fmt.Errorf(`error binding as %q: %w`, p.c.BindUsername, err) } return nil @@ -420,14 +420,14 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bi conn, err := p.dial(ctx) if err != nil { p.traceAuthFailure(t, err) - return nil, false, fmt.Errorf(`error dialing host "%s": %w`, p.c.Host, err) + return nil, false, fmt.Errorf(`error dialing host %q: %w`, p.c.Host, err) } defer conn.Close() err = conn.Bind(p.c.BindUsername, p.c.BindPassword) if err != nil { p.traceAuthFailure(t, err) - return nil, false, fmt.Errorf(`error binding as "%s" before user search: %w`, p.c.BindUsername, err) + return nil, false, fmt.Errorf(`error binding as %q before user search: %w`, p.c.BindUsername, err) } response, err := p.searchAndBindUser(conn, username, bindFunc) @@ -455,7 +455,7 @@ func (p *Provider) searchGroupsForUserDN(conn Conn, userDN string) ([]string, er groupAttributeName = distinguishedNameAttributeName } - groups := []string{} + var groups []string entries: for _, groupEntry := range searchResult.Entries { if len(groupEntry.DN) == 0 { @@ -495,14 +495,14 @@ func (p *Provider) SearchForDefaultNamingContext(ctx context.Context) (string, e conn, err := p.dial(ctx) if err != nil { p.traceSearchBaseDiscoveryFailure(t, err) - return "", fmt.Errorf(`error dialing host "%s": %w`, p.c.Host, err) + return "", fmt.Errorf(`error dialing host %q: %w`, p.c.Host, err) } defer conn.Close() err = conn.Bind(p.c.BindUsername, p.c.BindPassword) if err != nil { p.traceSearchBaseDiscoveryFailure(t, err) - return "", fmt.Errorf(`error binding as "%s" before querying for defaultNamingContext: %w`, p.c.BindUsername, err) + return "", fmt.Errorf(`error binding as %q before querying for defaultNamingContext: %w`, p.c.BindUsername, err) } searchResult, err := conn.Search(p.defaultNamingContextRequest()) @@ -546,13 +546,13 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c // At this point, we have matched at least one entry, so we can be confident that the username is not actually // someone's password mistakenly entered into the username field, so we can log it without concern. if len(searchResult.Entries) > 1 { - return nil, fmt.Errorf(`searching for user "%s" resulted in %d search results, but expected 1 result`, + return nil, fmt.Errorf(`searching for user %q resulted in %d search results, but expected 1 result`, username, len(searchResult.Entries), ) } userEntry := searchResult.Entries[0] if len(userEntry.DN) == 0 { - return nil, fmt.Errorf(`searching for user "%s" resulted in search result without DN`, username) + return nil, fmt.Errorf(`searching for user %q resulted in search result without DN`, username) } mappedUsername, err := p.getSearchResultAttributeValue(p.c.UserSearch.UsernameAttribute, userEntry, username) @@ -567,7 +567,7 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c return nil, err } - mappedGroupNames := []string{} + var mappedGroupNames []string if len(p.c.GroupSearch.Base) > 0 { mappedGroupNames, err = p.searchGroupsForUserDN(conn, userEntry.DN) if err != nil { @@ -594,7 +594,7 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c if errors.As(err, &ldapErr) && ldapErr.ResultCode == ldap.LDAPResultInvalidCredentials { return nil, nil } - return nil, fmt.Errorf(`error binding for user "%s" using provided password against DN "%s": %w`, username, userEntry.DN, err) + return nil, fmt.Errorf(`error binding for user %q using provided password against DN %q: %w`, username, userEntry.DN, err) } if len(mappedUsername) == 0 || len(mappedUID) == 0 { @@ -675,7 +675,7 @@ func (p *Provider) refreshUserSearchRequest(dn string) *ldap.SearchRequest { } func (p *Provider) userSearchRequestedAttributes() []string { - attributes := []string{} + attributes := make([]string, 0, len(p.c.RefreshAttributeChecks)+2) if p.c.UserSearch.UsernameAttribute != distinguishedNameAttributeName { attributes = append(attributes, p.c.UserSearch.UsernameAttribute) } @@ -736,14 +736,14 @@ func (p *Provider) getSearchResultAttributeRawValueEncoded(attributeName string, 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`, + return "", fmt.Errorf(`found %d values for attribute %q while searching for user %q, 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`, + return "", fmt.Errorf(`found empty value for attribute %q while searching for user %q, but expected value to be non-empty`, attributeName, username, ) } @@ -763,14 +763,14 @@ func (p *Provider) getSearchResultAttributeValue(attributeName string, entry *ld attributeValues := entry.GetAttributeValues(attributeName) if len(attributeValues) != 1 { - return "", fmt.Errorf(`found %d values for attribute "%s" while searching for user "%s", but expected 1 result`, + return "", fmt.Errorf(`found %d values for attribute %q while searching for user %q, 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`, + return "", fmt.Errorf(`found empty value for attribute %q while searching for user %q, but expected value to be non-empty`, attributeName, username, ) } @@ -805,14 +805,14 @@ func (p *Provider) traceRefreshFailure(t *trace.Trace, err error) { func AttributeUnchangedSinceLogin(attribute string) func(*ldap.Entry, provider.StoredRefreshAttributes) error { return func(entry *ldap.Entry, storedAttributes provider.StoredRefreshAttributes) error { prevAttributeValue := storedAttributes.AdditionalAttributes[attribute] - newValues := entry.GetAttributeValues(attribute) + newValues := entry.GetRawAttributeValues(attribute) if len(newValues) != 1 { - return fmt.Errorf(`expected to find 1 value for "%s" attribute, but found %d`, attribute, len(newValues)) + return fmt.Errorf(`expected to find 1 value for %q attribute, but found %d`, attribute, len(newValues)) } - encodedNewValue := base64.RawURLEncoding.EncodeToString(entry.GetRawAttributeValue(attribute)) + encodedNewValue := base64.RawURLEncoding.EncodeToString(newValues[0]) if prevAttributeValue != encodedNewValue { - return fmt.Errorf(`value for attribute "%s" has changed since initial value at login`, attribute) + return fmt.Errorf(`value for attribute %q has changed since initial value at login`, attribute) } return nil } diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index f7c884fb..65a7629f 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -254,7 +254,7 @@ func TestEndUserAuthentication(t *testing.T) { }, wantAuthResponse: expectedAuthResponse(func(r *authenticators.Response) { info := r.User.(*user.DefaultInfo) - info.Groups = []string{} + info.Groups = nil }), }, { @@ -1410,8 +1410,8 @@ func TestUpstreamRefresh(t *testing.T) { ByteValues: [][]byte{[]byte(testUserSearchResultUIDAttributeValue)}, }, { - Name: pwdLastSetAttribute, - Values: []string{"132801740800000001"}, + Name: pwdLastSetAttribute, + ByteValues: [][]byte{[]byte("132801740800000001")}, }, }, }, @@ -1812,8 +1812,8 @@ func TestAttributeUnchangedSinceLogin(t *testing.T) { DN: "some-dn", Attributes: []*ldap.EntryAttribute{ { - Name: attributeName, - Values: []string{"val1", "val2"}, + Name: attributeName, + ByteValues: [][]byte{[]byte("val1"), []byte("val2")}, }, }, }, diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 1897d924..47fa6684 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -244,7 +244,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.GroupSearch.Base = "" })), wantAuthResponse: &authenticators.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: nil}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", ExtraRefreshAttributes: map[string]string{}, }, @@ -257,7 +257,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.GroupSearch.Base = "ou=users,dc=pinniped,dc=dev" // there are no groups under this part of the tree })), wantAuthResponse: &authenticators.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: nil}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", ExtraRefreshAttributes: map[string]string{}, }, @@ -328,7 +328,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.GroupSearch.Filter = "foobar={}" // foobar is not a valid attribute name for this LDAP server's schema })), wantAuthResponse: &authenticators.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: nil}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", ExtraRefreshAttributes: map[string]string{}, },