From ee4f725209b49df7ff800be3fe3a911834620d7f Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Mon, 6 Dec 2021 16:24:31 -0800 Subject: [PATCH] Incorporate PR feedback --- .../active_directory_upstream_watcher.go | 6 +- internal/oidc/token/token_handler_test.go | 1 - internal/upstreamldap/upstreamldap.go | 30 ++++++---- internal/upstreamldap/upstreamldap_test.go | 60 +++++++++++++++---- test/integration/supervisor_login_test.go | 6 +- 5 files changed, 76 insertions(+), 27 deletions(-) diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index 7a22b816..970c895e 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -317,7 +317,11 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context, }, Dialer: c.ldapDialer, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{upstreamldap.PwdLastSetAttribute: upstreamldap.PwdUnchangedSinceLogin, upstreamldap.UserAccountControlAttribute: upstreamldap.ValidUserAccountControl, upstreamldap.UserAccountControlComputedAttribute: upstreamldap.ValidComputedUserAccountControl}, + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + upstreamldap.PwdLastSetAttribute: upstreamldap.PwdUnchangedSinceLogin, + upstreamldap.UserAccountControlAttribute: upstreamldap.ValidUserAccountControl, + upstreamldap.UserAccountControlComputedAttribute: upstreamldap.ValidComputedUserAccountControl, + }, } if spec.GroupSearch.Attributes.GroupName == "" { diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index fa799dbe..81207357 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -2149,7 +2149,6 @@ func TestRefreshGrant(t *testing.T) { authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, customSessionData: happyLDAPCustomSessionData, - //fositeSessionData: &openid.DefaultSession{}, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, ), diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index f20fb8a1..9030de81 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -41,11 +41,19 @@ const ( defaultLDAPPort = uint16(389) defaultLDAPSPort = uint16(636) sAMAccountNameAttribute = "sAMAccountName" - PwdLastSetAttribute = "pwdLastSet" - UserAccountControlAttribute = "userAccountControl" - UserAccountControlComputedAttribute = "msDS-User-Account-Control-Computed" - accountDisabledBitmapValue = 2 - accountLockedBitmapValue = 16 + // 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. + // https://docs.microsoft.com/en-us/troubleshoot/windows-server/identity/useraccountcontrol-manipulate-account-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" + // 0x0002 ACCOUNTDISABLE in userAccountControl bitmap. + accountDisabledBitmapValue = 2 + // 0x0010 UF_LOCKOUT in msDS-User-Account-Control-Computed bitmap. + accountLockedBitmapValue = 16 ) // Conn abstracts the upstream LDAP communication protocol (mostly for testing). @@ -881,23 +889,23 @@ func PwdUnchangedSinceLogin(entry *ldap.Entry, attributes provider.StoredRefresh return nil } -func win32timestampToTime(win32timestamp string) (*time.Time, error) { +func win32timestampToTime(win32timestamp string) (time.Time, error) { // take a win32 timestamp (represented as the number of 100 ns intervals since // January 1, 1601) and make a time.Time - const unixTimeBaseAsWin = 116444736000000000 // The unix base time (January 1, 1970 UTC) as 100 ns since Win32 epoch (1601-01-01) - const hundredNsToSecFactor = 10000000 + const unixTimeBaseAsWin = 116_444_736_000_000_000 // The unix base time (January 1, 1970 UTC) as 100 ns since Win32 epoch (1601-01-01) + const hundredNsToSecFactor = 10_000_000 win32Time, err := strconv.ParseUint(win32timestamp, 10, 64) if err != nil { - return nil, fmt.Errorf("couldn't parse as timestamp") + return time.Time{}, fmt.Errorf("couldn't parse as timestamp") } unixsec := int64(win32Time-unixTimeBaseAsWin) / hundredNsToSecFactor - unixns := int64(win32Time % hundredNsToSecFactor) + unixns := int64(win32Time%hundredNsToSecFactor) * 100 convertedTime := time.Unix(unixsec, unixns).UTC() - return &convertedTime, nil + return convertedTime, nil } func ValidUserAccountControl(entry *ldap.Entry, _ provider.StoredRefreshAttributes) error { diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index fc056cc8..b2619baf 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -26,7 +26,7 @@ import ( "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/endpointaddr" "go.pinniped.dev/internal/mocks/mockldapconn" - provider2 "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/tlsserver" ) @@ -1163,9 +1163,9 @@ func TestEndUserAuthentication(t *testing.T) { return conn, nil }) - provider := New(*tt.providerConfig) + ldapProvider := New(*tt.providerConfig) - authResponse, authenticated, err := provider.AuthenticateUser(context.Background(), tt.username, tt.password) + authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password) require.Equal(t, !tt.wantToSkipDial, dialWasAttempted) switch { case tt.wantError != "": @@ -1197,7 +1197,7 @@ func TestEndUserAuthentication(t *testing.T) { } // Skip tt.bindEndUserMocks since DryRunAuthenticateUser() never binds as the end user. - authResponse, authenticated, err = provider.DryRunAuthenticateUser(context.Background(), tt.username) + authResponse, authenticated, err = ldapProvider.DryRunAuthenticateUser(context.Background(), tt.username) require.Equal(t, !tt.wantToSkipDial, dialWasAttempted) switch { case tt.wantError != "": @@ -1265,7 +1265,7 @@ func TestUpstreamRefresh(t *testing.T) { UIDAttribute: testUserSearchUIDAttribute, UsernameAttribute: testUserSearchUsernameAttribute, }, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider2.StoredRefreshAttributes) error{pwdLastSetAttribute: PwdUnchangedSinceLogin}, + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{pwdLastSetAttribute: PwdUnchangedSinceLogin}, } tests := []struct { @@ -1573,10 +1573,10 @@ func TestUpstreamRefresh(t *testing.T) { return conn, nil }) - provider := New(*providerConfig) + ldapProvider := New(*providerConfig) subject := "ldaps://ldap.example.com:8443?base=some-upstream-user-base-dn&sub=c29tZS11cHN0cmVhbS11aWQtdmFsdWU" authTime := time.Date(2021, time.November, 1, 23, 43, 19, 0, time.UTC) - err := provider.PerformRefresh(context.Background(), provider2.StoredRefreshAttributes{ + err := ldapProvider.PerformRefresh(context.Background(), provider.StoredRefreshAttributes{ Username: testUserSearchResultUsernameAttributeValue, Subject: subject, DN: testUserSearchResultDNValue, @@ -2041,7 +2041,7 @@ func TestPwdUnchangedSinceLogin(t *testing.T) { for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { - err := PwdUnchangedSinceLogin(tt.entry, provider2.StoredRefreshAttributes{AuthTime: *tt.authTime}) + err := PwdUnchangedSinceLogin(tt.entry, provider.StoredRefreshAttributes{AuthTime: *tt.authTime}) if tt.wantErr != "" { require.Error(t, err) require.Equal(t, tt.wantErr, err.Error()) @@ -2057,13 +2057,13 @@ func TestWin32TimestampToTime(t *testing.T) { tests := []struct { name string timestampString string - wantTime *time.Time + wantTime time.Time wantErr string }{ { name: "happy case with a valid timestamp", timestampString: "132540199410000000", - wantTime: &happyPasswordChangeTime, + wantTime: happyPasswordChangeTime, }, { name: "handles error with a string thats not a timestamp", @@ -2075,6 +2075,16 @@ func TestWin32TimestampToTime(t *testing.T) { timestampString: "132540199410000000000", wantErr: "couldn't parse as timestamp", }, + { + name: "timestamp of zero", + timestampString: "0", + wantTime: time.Date(1601, time.January, 1, 0, 0, 0, 0, time.UTC).UTC(), + }, + { + name: "fractional seconds", + timestampString: "132540199410000001", + wantTime: time.Date(2021, time.January, 2, 0, 12, 21, 100, time.UTC).UTC(), + }, } for _, test := range tests { @@ -2135,12 +2145,25 @@ func TestValidUserAccountControl(t *testing.T) { }, wantErr: "user has been deactivated", }, + { + name: "non-integer result", + entry: &ldap.Entry{ + DN: "some-dn", + Attributes: []*ldap.EntryAttribute{ + { + Name: "userAccountControl", + Values: []string{"not-an-int"}, + }, + }, + }, + wantErr: "strconv.Atoi: parsing \"not-an-int\": invalid syntax", + }, } for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { - err := ValidUserAccountControl(tt.entry, provider2.StoredRefreshAttributes{}) + err := ValidUserAccountControl(tt.entry, provider.StoredRefreshAttributes{}) if tt.wantErr != "" { require.Error(t, err) @@ -2183,12 +2206,25 @@ func TestValidComputedUserAccountControl(t *testing.T) { }, wantErr: "user has been locked", }, + { + name: "non-integer result", + entry: &ldap.Entry{ + DN: "some-dn", + Attributes: []*ldap.EntryAttribute{ + { + Name: "msDS-User-Account-Control-Computed", + Values: []string{"not-an-int"}, + }, + }, + }, + wantErr: "strconv.Atoi: parsing \"not-an-int\": invalid syntax", + }, } for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { - err := ValidComputedUserAccountControl(tt.entry, provider2.StoredRefreshAttributes{}) + err := ValidComputedUserAccountControl(tt.entry, provider.StoredRefreshAttributes{}) if tt.wantErr != "" { require.Error(t, err) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 66497052..9f54d543 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -23,6 +23,8 @@ import ( "testing" "time" + "go.pinniped.dev/internal/crypto/ptls" + coreosoidc "github.com/coreos/go-oidc/v3/oidc" "github.com/go-ldap/ldap/v3" "github.com/stretchr/testify/assert" @@ -1897,7 +1899,7 @@ func changeADTestUserPassword(t *testing.T, env *testlib.TestEnv, testUserName s newTestUserPassword := createRandomASCIIString(t, 20) enc := unicode.UTF16(unicode.LittleEndian, unicode.IgnoreBOM).NewEncoder() - encodedTestUserPassword, err := enc.String("\"" + newTestUserPassword + "\"") + encodedTestUserPassword, err := enc.String(`"` + newTestUserPassword + `"`) require.NoError(t, err) userDN := fmt.Sprintf("CN=%s,OU=test-users,%s", testUserName, env.SupervisorUpstreamActiveDirectory.UserSearchBase) @@ -1930,7 +1932,7 @@ func dialTLS(t *testing.T, env *testlib.TestEnv) *ldap.Conn { rootCAs := x509.NewCertPool() success := rootCAs.AppendCertsFromPEM([]byte(env.SupervisorUpstreamActiveDirectory.CABundle)) require.True(t, success) - tlsConfig := &tls.Config{MinVersion: tls.VersionTLS12, RootCAs: rootCAs} + tlsConfig := ptls.DefaultLDAP(rootCAs) dialer := &tls.Dialer{NetDialer: &net.Dialer{Timeout: time.Minute}, Config: tlsConfig} c, err := dialer.DialContext(context.Background(), "tcp", env.SupervisorUpstreamActiveDirectory.Host) require.NoError(t, err)