Incorporate PR feedback

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 == "" {

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,
 				),

internal/upstreamldap/upstreamldap.go

@@ -41,10 +41,18 @@ const (
 	defaultLDAPPort                         = uint16(389)
 	defaultLDAPSPort                        = uint16(636)
 	sAMAccountNameAttribute                 = "sAMAccountName"
+	// 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
 )
 
@@ -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 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 = 10000000
+	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 {

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)

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)