From bfcd752093237f8592f0619bbfc34ab56dc2f3ad Mon Sep 17 00:00:00 2001 From: Edward Viaene Date: Sat, 10 Oct 2026 13:49:39 -0500 Subject: [PATCH] auth, user management and storage improvements - SAML and OIDC login flow improvements: one-time login codes and state, session lifetime defaults, ID token claim checks, license limits for external users, cleanup of pending login state - OIDC token renewal applies the configured action on repeated failures - Profile password changes require the current password; userType now distinguishes local, oidc and saml users - Users are reloaded from disk on SIGHUP - SCIM paging follows RFC 7644, renames are handled by user id - TOTP verification improvements - Password hashing and login handling improvements - Local storage path handling and atomic file writes - Access log omits query strings - Additional tests across auth, users, storage and SCIM Co-Authored-By: Claude Opus 5.5 --- auth/oidc/store/cleanup.go | 45 ++- auth/oidc/store/cleanup_test.go | 76 +++++ auth/oidc/store/renewal/worker.go | 74 +++-- auth/oidc/store/renewal/worker_test.go | 127 ++++++++ auth/oidc/store/save.go | 5 + auth/oidc/token.go | 48 +++ auth/oidc/token_test.go | 95 ++++++ auth/provisioning/scim/middleware.go | 5 +- auth/provisioning/scim/middleware_test.go | 38 +++ auth/provisioning/scim/paging_test.go | 104 +++++++ auth/provisioning/scim/put_test.go | 128 ++++++++ auth/provisioning/scim/response.go | 16 +- auth/provisioning/scim/users.go | 40 ++- auth/provisioning/scim/users_test.go | 6 +- auth/saml/assertion.go | 76 +++++ auth/saml/handlers.go | 20 +- auth/saml/new.go | 15 +- auth/saml/session.go | 72 ++++- auth/saml/session_security_test.go | 113 +++++++ auth/saml/session_test.go | 2 +- auth/saml/types.go | 13 +- mfa/totp/verify.go | 91 +++++- mfa/totp/verify_test.go | 123 ++++++++ rest/auth.go | 60 ++-- rest/auth_test.go | 16 +- rest/login/auth_test.go | 48 +++ rest/login/jwt.go | 6 +- rest/middleware.go | 3 +- rest/passwordchange_test.go | 2 +- rest/prerelease_test.go | 362 ++++++++++++++++++++++ rest/profile.go | 14 +- rest/security_test.go | 2 +- rest/signals.go | 23 +- rest/types.go | 5 + rest/users.go | 26 +- storage/local/fullpath.go | 37 +++ storage/local/fullpath_test.go | 195 ++++++++++++ storage/local/ownership_other.go | 12 + storage/local/ownership_unix.go | 21 ++ storage/local/path.go | 52 +++- storage/local/read.go | 19 +- storage/local/write.go | 92 +++++- storage/local/write_test.go | 123 ++++++++ users/localusers.go | 91 +++++- users/localusers_test.go | 105 +++++++ 45 files changed, 2497 insertions(+), 149 deletions(-) create mode 100644 auth/oidc/store/cleanup_test.go create mode 100644 auth/oidc/store/renewal/worker_test.go create mode 100644 auth/oidc/token_test.go create mode 100644 auth/provisioning/scim/middleware_test.go create mode 100644 auth/provisioning/scim/paging_test.go create mode 100644 auth/provisioning/scim/put_test.go create mode 100644 auth/saml/assertion.go create mode 100644 auth/saml/session_security_test.go create mode 100644 rest/prerelease_test.go create mode 100644 storage/local/fullpath.go create mode 100644 storage/local/fullpath_test.go create mode 100644 storage/local/ownership_other.go create mode 100644 storage/local/ownership_unix.go create mode 100644 storage/local/write_test.go create mode 100644 users/localusers_test.go diff --git a/auth/oidc/store/cleanup.go b/auth/oidc/store/cleanup.go index 41fbf42..e9b0e68 100644 --- a/auth/oidc/store/cleanup.go +++ b/auth/oidc/store/cleanup.go @@ -7,6 +7,14 @@ import ( "github.com/in4it/go-devops-platform/auth/oidc" ) +// PENDING_STATE_TTL is how long a state without token (a login that was +// started, but not completed yet) is kept +const PENDING_STATE_TTL = 10 * time.Minute + +// MAX_PENDING_STATES is the maximum number of states without token. When a new +// state is saved and there are more, the oldest ones are dropped. +var MAX_PENDING_STATES = 1000 + func (store *Store) CleanupOAuth2DataForAllEntries() int { deleted := 0 for _, oauthData := range store.GetOAuth2DataCopy() { @@ -24,16 +32,17 @@ func (store *Store) CleanupOAuth2Data(oauthData oidc.OAuthData) int { for k := range store.OAuth2Data { if oauthData.CreatedAt.After(store.OAuth2Data[k].CreatedAt) { // cleanup old oauth2 data that might be duplicates (same subject & oidc provider, but older tokens) - if store.OAuth2Data[k].ID != oauthData.ID && store.OAuth2Data[k].OIDCProviderID == oauthData.OIDCProviderID && store.OAuth2Data[k].Subject == oauthData.Subject { + // (only for entries with a subject: pending states without token have none) + if oauthData.Subject != "" && store.OAuth2Data[k].ID != oauthData.ID && store.OAuth2Data[k].OIDCProviderID == oauthData.OIDCProviderID && store.OAuth2Data[k].Subject == oauthData.Subject { keysToDelete1 = append(keysToDelete1, k) } // cleanup oauthdata with the same email address - if store.OAuth2Data[k].ID != oauthData.ID && store.OAuth2Data[k].UserInfo.Email == oauthData.UserInfo.Email { + if oauthData.UserInfo.Email != "" && store.OAuth2Data[k].ID != oauthData.ID && store.OAuth2Data[k].UserInfo.Email == oauthData.UserInfo.Email { keysToDelete3 = append(keysToDelete3, k) } } // cleanup old oauth2 data that doesn't have a token and is stale - if store.OAuth2Data[k].Token.AccessToken == "" && store.OAuth2Data[k].CreatedAt.Add(10*time.Minute).After(time.Now()) { + if store.OAuth2Data[k].Token.AccessToken == "" && store.OAuth2Data[k].CreatedAt.Add(PENDING_STATE_TTL).Before(time.Now()) { keysToDelete2 = append(keysToDelete2, k) } } @@ -48,3 +57,33 @@ func (store *Store) CleanupOAuth2Data(oauthData oidc.OAuthData) int { } return len(keysToDelete) } + +// prunePendingStates removes stale states without a token, and the oldest +// states without a token when there are more than MAX_PENDING_STATES - 1, to +// make room for a new one. Must be called with store.Mu held. +func (store *Store) prunePendingStates(now time.Time) { + type pendingState struct { + key string + createdAt time.Time + } + pending := []pendingState{} + for k, oauthData := range store.OAuth2Data { + if oauthData.Token.AccessToken != "" { + continue + } + if oauthData.CreatedAt.Add(PENDING_STATE_TTL).Before(now) { + delete(store.OAuth2Data, k) + continue + } + pending = append(pending, pendingState{key: k, createdAt: oauthData.CreatedAt}) + } + if len(pending) < MAX_PENDING_STATES { + return + } + slices.SortFunc(pending, func(a, b pendingState) int { + return a.createdAt.Compare(b.createdAt) + }) + for _, p := range pending[:len(pending)-MAX_PENDING_STATES+1] { + delete(store.OAuth2Data, p.key) + } +} diff --git a/auth/oidc/store/cleanup_test.go b/auth/oidc/store/cleanup_test.go new file mode 100644 index 0000000..68d0425 --- /dev/null +++ b/auth/oidc/store/cleanup_test.go @@ -0,0 +1,76 @@ +package oidcstore + +import ( + "fmt" + "testing" + "time" + + "github.com/in4it/go-devops-platform/auth/oidc" + memorystorage "github.com/in4it/go-devops-platform/storage/memory" +) + +// TestCleanupPendingStates: states without token are removed once they're +// older than PENDING_STATE_TTL, younger ones (logins in progress) are kept. +func TestCleanupPendingStates(t *testing.T) { + store, err := NewStore(&memorystorage.MockMemoryStorage{}) + if err != nil { + t.Fatalf("new store error: %s", err) + } + store.OAuth2Data["young"] = oidc.OAuthData{ID: "young", OIDCProviderID: "prov-1", CreatedAt: time.Now().Add(-time.Minute)} + store.OAuth2Data["young2"] = oidc.OAuthData{ID: "young2", OIDCProviderID: "prov-1", CreatedAt: time.Now().Add(-2 * time.Minute)} + store.OAuth2Data["old"] = oidc.OAuthData{ID: "old", CreatedAt: time.Now().Add(-PENDING_STATE_TTL - time.Minute)} + store.OAuth2Data["withtoken"] = oidc.OAuthData{ID: "withtoken", CreatedAt: time.Now().Add(-24 * time.Hour), Token: oidc.Token{AccessToken: "abc"}, Subject: "sub-1", OIDCProviderID: "prov-1", UserInfo: oidc.UserInfo{Email: "john@example.com"}} + + deleted := store.CleanupOAuth2DataForAllEntries() + if deleted != 1 { + t.Fatalf("expected 1 deleted entry, got %d", deleted) + } + if _, ok := store.GetOAuth2DataByKey("young"); !ok { + t.Fatalf("pending login in progress was deleted") + } + if _, ok := store.GetOAuth2DataByKey("young2"); !ok { + t.Fatalf("other pending login in progress was deleted") + } + if _, ok := store.GetOAuth2DataByKey("old"); ok { + t.Fatalf("stale pending state was not deleted") + } + if _, ok := store.GetOAuth2DataByKey("withtoken"); !ok { + t.Fatalf("entry with token was deleted") + } +} + +// TestPendingStatesCapped: unauthenticated users starting logins can't grow +// the store without bound. +func TestPendingStatesCapped(t *testing.T) { + maxPendingStates := MAX_PENDING_STATES + MAX_PENDING_STATES = 10 + defer func() { MAX_PENDING_STATES = maxPendingStates }() + + store, err := NewStore(&memorystorage.MockMemoryStorage{}) + if err != nil { + t.Fatalf("new store error: %s", err) + } + if err := store.SaveOAuth2Data(oidc.OAuthData{ID: "loggedin", CreatedAt: time.Now().Add(-time.Hour), Token: oidc.Token{AccessToken: "abc"}}, "loggedin"); err != nil { + t.Fatalf("save error: %s", err) + } + start := time.Now().Add(-5 * time.Minute) + for i := 0; i < 25; i++ { + err := store.SaveOAuth2Data(oidc.OAuthData{ID: fmt.Sprintf("id-%d", i), CreatedAt: start.Add(time.Duration(i) * time.Second)}, fmt.Sprintf("state-%d", i)) + if err != nil { + t.Fatalf("save error: %s", err) + } + } + all := store.GetOAuth2DataCopy() + if len(all) != MAX_PENDING_STATES+1 { + t.Fatalf("expected %d entries, got %d", MAX_PENDING_STATES+1, len(all)) + } + if _, ok := all["loggedin"]; !ok { + t.Fatalf("entry with token was dropped") + } + if _, ok := all["state-24"]; !ok { + t.Fatalf("newest pending state was dropped") + } + if _, ok := all["state-0"]; ok { + t.Fatalf("oldest pending state was kept") + } +} diff --git a/auth/oidc/store/renewal/worker.go b/auth/oidc/store/renewal/worker.go index cbd05e2..fd5afbe 100644 --- a/auth/oidc/store/renewal/worker.go +++ b/auth/oidc/store/renewal/worker.go @@ -13,6 +13,11 @@ const DEFAULT_RENEWAL_TIME_MINUTES = 60 // every hour we want to refresh the tok const RENEWAL_RETRIES = 3 // 3 retries before we suspend a user const RENEWAL_BACKOFF_SECONDS = 10 // time between requests to the oidc providers +// backoffSleep waits between requests to the oidc providers +var backoffSleep = func() { + time.Sleep(RENEWAL_BACKOFF_SECONDS * time.Second) +} + func (r *Renewal) Worker() { // do renewal if r.enabled { @@ -23,26 +28,59 @@ func (r *Renewal) Worker() { time.Sleep(WAKEUP_TIME_SECONDS * time.Second) continue } - deletedEntries := r.oidcStore.CleanupOAuth2DataForAllEntries() - if deletedEntries > 0 { - err := r.oidcStore.SaveOIDCStore() - if err != nil { - log.Printf("Renewal Worker: [warning] couldn't save oidc store after cleanup: %s", err) - } + r.renewAll() + time.Sleep(WAKEUP_TIME_SECONDS * time.Second) + } +} + +func (r *Renewal) renewAll() { + deletedEntries := r.oidcStore.CleanupOAuth2DataForAllEntries() + if deletedEntries > 0 { + err := r.oidcStore.SaveOIDCStore() + if err != nil { + log.Printf("Renewal Worker: [warning] couldn't save oidc store after cleanup: %s", err) } - for key, oauth2Data := range r.oidcStore.GetOAuth2DataCopy() { - logging.DebugLog(fmt.Errorf("running canRenew of %s", oauth2Data.ID)) - // can we renew? Do we have expiration date and it is expired? - canRenew, oidcProvider, discovery, err := canRenew(r.renewalTime, oauth2Data, r.oidcStore, r.oidcProviders) - if err != nil { - log.Printf("Renewal Worker: [warning] needsRenewal: %s", err) - } - if canRenew { - logging.DebugLog(fmt.Errorf("we can renew %s", oauth2Data.ID)) - r.renew(discovery, key, oauth2Data, oidcProvider) // error logging within function + } + for key, oauth2Data := range r.oidcStore.GetOAuth2DataCopy() { + logging.DebugLog(fmt.Errorf("running canRenew of %s", oauth2Data.ID)) + // can we renew? Do we have expiration date and it is expired? + canRenew, oidcProvider, discovery, err := canRenew(r.renewalTime, oauth2Data, r.oidcStore, r.oidcProviders) + if err != nil { + log.Printf("Renewal Worker: [warning] needsRenewal: %s", err) + } + if canRenew { + logging.DebugLog(fmt.Errorf("we can renew %s", oauth2Data.ID)) + userDisabled := r.renew(discovery, key, oauth2Data, oidcProvider) // error logging within function + if userDisabled { + r.disableUser(oauth2Data.ID) } - time.Sleep(RENEWAL_BACKOFF_SECONDS * time.Second) + backoffSleep() // only wait after a request to the oidc provider } - time.Sleep(WAKEUP_TIME_SECONDS * time.Second) + } +} + +// disableUser disables the connections of the user with the given oidc id +// after token renewal failed +func (r *Renewal) disableUser(oidcID string) { + if r.userStore == nil { + return + } + user, err := r.userStore.GetUserByOIDCIDs([]string{oidcID}) + if err != nil { + logging.ErrorLog(fmt.Errorf("renewal Worker: no user found with oidc id %s", oidcID)) + return + } + logging.DebugLog(fmt.Errorf("disable user with oidc id %s", oidcID)) + if r.userStore.UserHooks.DisableFunc != nil { + err = r.userStore.UserHooks.DisableFunc(r.storage, user) + if err != nil { + logging.ErrorLog(fmt.Errorf("renewal Worker: could not disable connections for userID %s: %s", user.ID, err)) + return + } + } + user.ConnectionsDisabledOnAuthFailure = true + err = r.userStore.UpdateUser(user) + if err != nil { + logging.ErrorLog(fmt.Errorf("renewal Worker: could not update connectionsDisabledOnAuthFailure for userID %s: %s", user.ID, err)) } } diff --git a/auth/oidc/store/renewal/worker_test.go b/auth/oidc/store/renewal/worker_test.go new file mode 100644 index 0000000..336a240 --- /dev/null +++ b/auth/oidc/store/renewal/worker_test.go @@ -0,0 +1,127 @@ +package oidcrenewal + +import ( + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" + + "github.com/in4it/go-devops-platform/auth/oidc" + oidcstore "github.com/in4it/go-devops-platform/auth/oidc/store" + "github.com/in4it/go-devops-platform/storage" + memorystorage "github.com/in4it/go-devops-platform/storage/memory" + "github.com/in4it/go-devops-platform/users" +) + +func fakeAccessToken(t *testing.T, exp time.Time) string { + t.Helper() + payload, err := json.Marshal(jwtExp{Expiration: exp.Unix()}) + if err != nil { + t.Fatalf("marshal error: %s", err) + } + return "eyJhbGciOiJub25lIn0." + base64.RawURLEncoding.EncodeToString(payload) + ".sig" +} + +func newTestRenewal(t *testing.T, tokenEndpointStatus int) (*Renewal, *httptest.Server) { + t.Helper() + var ts *httptest.Server + ts = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/discovery.json": + out, _ := json.Marshal(oidc.Discovery{Issuer: "test-issuer", TokenEndpoint: ts.URL + "/token"}) + w.Write(out) + case "/token": + w.WriteHeader(tokenEndpointStatus) + w.Write([]byte(`{"error": "invalid_grant"}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(ts.Close) + + storage := &memorystorage.MockMemoryStorage{} + store, err := oidcstore.NewStore(storage) + if err != nil { + t.Fatalf("new store error: %s", err) + } + userStore, err := users.NewUserStore(storage, 100) + if err != nil { + t.Fatalf("new user store error: %s", err) + } + return &Renewal{ + oidcStore: store, + enabled: true, + oidcProviders: []oidc.OIDCProvider{{ID: "prov-1", ClientID: "client", ClientSecret: "secret", DiscoveryURI: ts.URL + "/discovery.json"}}, + userStore: userStore, + renewalTime: time.Hour, + storage: storage, + }, ts +} + +// TestWorkerDisablesUserOnRenewalFailure: when token renewal fails for the last +// time, the background worker disables the user's connections. +func TestWorkerDisablesUserOnRenewalFailure(t *testing.T) { + backoffSleep = func() {} + r, _ := newTestRenewal(t, http.StatusBadRequest) + + var mu sync.Mutex + disabled := []string{} + r.userStore.UserHooks.DisableFunc = func(_ storage.Iface, user users.User) error { + mu.Lock() + defer mu.Unlock() + disabled = append(disabled, user.ID) + return nil + } + user, err := r.userStore.AddUser(users.User{Login: "john@example.com", OIDCID: "oidc-1"}) + if err != nil { + t.Fatalf("add user error: %s", err) + } + err = r.oidcStore.SaveOAuth2Data(oidc.OAuthData{ + ID: "oidc-1", + OIDCProviderID: "prov-1", + CreatedAt: time.Now().Add(-2 * time.Hour), + LastTokenRenewal: time.Now().Add(-2 * time.Hour), + RenewalRetries: RENEWAL_RETRIES - 1, + Token: oidc.Token{AccessToken: fakeAccessToken(t, time.Now().Add(-time.Minute)), RefreshToken: "refresh"}, + }, "state-1") + if err != nil { + t.Fatalf("save error: %s", err) + } + + r.renewAll() + + if len(disabled) != 1 || disabled[0] != user.ID { + t.Fatalf("expected disable hook for %s, got %v", user.ID, disabled) + } + updated, err := r.userStore.GetUserByID(user.ID) + if err != nil { + t.Fatalf("get user error: %s", err) + } + if !updated.ConnectionsDisabledOnAuthFailure { + t.Fatalf("expected ConnectionsDisabledOnAuthFailure to be set") + } + oauth2Data, _ := r.oidcStore.GetOAuth2DataByKey("state-1") + if !oauth2Data.RenewalFailed { + t.Fatalf("expected renewal to be marked as failed") + } +} + +// TestWorkerOnlyBacksOffAfterRenewal: entries that aren't renewed (pending +// logins without token) don't make the worker sleep. +func TestWorkerOnlyBacksOffAfterRenewal(t *testing.T) { + sleeps := 0 + backoffSleep = func() { sleeps++ } + r, _ := newTestRenewal(t, http.StatusBadRequest) + for _, state := range []string{"a", "b", "c"} { + if err := r.oidcStore.SaveOAuth2Data(oidc.OAuthData{ID: state, OIDCProviderID: "prov-1", CreatedAt: time.Now()}, state); err != nil { + t.Fatalf("save error: %s", err) + } + } + r.renewAll() + if sleeps != 0 { + t.Fatalf("expected no backoff for entries without renewal, got %d", sleeps) + } +} diff --git a/auth/oidc/store/save.go b/auth/oidc/store/save.go index 4f26ab3..32c1412 100644 --- a/auth/oidc/store/save.go +++ b/auth/oidc/store/save.go @@ -3,6 +3,7 @@ package oidcstore import ( "encoding/json" "fmt" + "time" "github.com/in4it/go-devops-platform/auth/oidc" ) @@ -24,6 +25,10 @@ func (store *Store) SaveOIDCStore() error { func (store *Store) SaveOAuth2Data(oauth2Data oidc.OAuthData, key string) error { store.Mu.Lock() + if _, exists := store.OAuth2Data[key]; !exists && oauth2Data.Token.AccessToken == "" { + // new login started: make sure pending states can't grow without bound + store.prunePendingStates(time.Now()) + } store.OAuth2Data[key] = oauth2Data store.Mu.Unlock() return store.SaveOIDCStore() diff --git a/auth/oidc/token.go b/auth/oidc/token.go index 4294b46..57881c9 100644 --- a/auth/oidc/token.go +++ b/auth/oidc/token.go @@ -7,6 +7,8 @@ import ( "net/http" "net/mail" "net/url" + "slices" + "strings" "time" "github.com/go-jose/go-jose/v4" @@ -101,6 +103,10 @@ func UpdateOAuth2DataWithToken(jwks Jwks, discovery Discovery, clientID, clientS if !ok { return newOAuthData, fmt.Errorf("issuer missing from id token") } + err = validateIDTokenClaims(claims, discovery, clientID) + if err != nil { + return newOAuthData, err + } newOAuthData.Token = token newOAuthData.LastTokenRenewal = renewalTime @@ -133,3 +139,45 @@ func GetPublicKeyForToken(allJwks []Jwks, discoveryProviders []Discovery, token } return nil, fmt.Errorf("no matching kid found for token") } + +// validateIDTokenClaims checks the audience, issuer and email_verified claims of +// an id token +func validateIDTokenClaims(claims jwt.MapClaims, discovery Discovery, clientID string) error { + audience, err := claims.GetAudience() + if err != nil { + return fmt.Errorf("invalid audience in id token: %s", err) + } + if !slices.Contains(audience, clientID) { + return fmt.Errorf("id token audience doesn't contain the client id") + } + issuer, ok := claims["iss"].(string) + if !ok { + return fmt.Errorf("issuer in id token is not a string") + } + if discovery.Issuer != "" { + expectedIssuer := discovery.Issuer + // multi-tenant discovery documents (e.g. Microsoft Entra ID's common endpoint) use a {tenantid} placeholder + if strings.Contains(expectedIssuer, "{tenantid}") { + if tenantID, ok := claims["tid"].(string); ok && tenantID != "" { + expectedIssuer = strings.ReplaceAll(expectedIssuer, "{tenantid}", tenantID) + } + } + if issuer != expectedIssuer { + return fmt.Errorf("id token issuer doesn't match the issuer of the oidc provider") + } + } + // only reject when the IdP explicitly says the email address is not verified + if emailVerified, ok := claims["email_verified"]; ok { + verified := true + switch v := emailVerified.(type) { + case bool: + verified = v + case string: + verified = !strings.EqualFold(v, "false") + } + if !verified { + return fmt.Errorf("email address in id token is not verified") + } + } + return nil +} diff --git a/auth/oidc/token_test.go b/auth/oidc/token_test.go new file mode 100644 index 0000000..682e4d8 --- /dev/null +++ b/auth/oidc/token_test.go @@ -0,0 +1,95 @@ +package oidc + +import ( + "crypto/rand" + "crypto/rsa" + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" +) + +func TestUpdateOAuth2DataWithTokenValidatesClaims(t *testing.T) { + privateKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("can't generate key: %s", err) + } + jwks := Jwks{Keys: []JwksKey{{ + Kid: "kid-1", + Alg: "RS256", + Kty: "RSA", + Use: "sig", + N: base64.RawURLEncoding.EncodeToString(privateKey.PublicKey.N.Bytes()), + E: "AQAB", + }}} + + baseClaims := func() jwt.MapClaims { + return jwt.MapClaims{ + "iss": "https://idp.inv", + "aud": "client-1", + "sub": "john", + "email": "john@example.inv", + "exp": time.Now().Add(time.Hour).Unix(), + "iat": time.Now().Unix(), + } + } + + tests := []struct { + name string + discoveryIssuer string + modify func(jwt.MapClaims) + wantErr bool + }{ + {name: "valid", discoveryIssuer: "https://idp.inv", modify: func(jwt.MapClaims) {}}, + {name: "audience array", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["aud"] = []string{"other", "client-1"} }}, + {name: "wrong audience", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["aud"] = "other-client" }, wantErr: true}, + {name: "missing audience", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { delete(c, "aud") }, wantErr: true}, + {name: "wrong issuer", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["iss"] = "https://evil.inv" }, wantErr: true}, + {name: "email verified", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["email_verified"] = true }}, + {name: "email not verified", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["email_verified"] = false }, wantErr: true}, + {name: "email not verified (string)", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["email_verified"] = "false" }, wantErr: true}, + {name: "tenant issuer", discoveryIssuer: "https://login.inv/{tenantid}/v2.0", modify: func(c jwt.MapClaims) { + c["iss"] = "https://login.inv/tenant-1/v2.0" + c["tid"] = "tenant-1" + }}, + {name: "wrong tenant issuer", discoveryIssuer: "https://login.inv/{tenantid}/v2.0", modify: func(c jwt.MapClaims) { + c["iss"] = "https://login.inv/tenant-2/v2.0" + c["tid"] = "tenant-1" + }, wantErr: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + claims := baseClaims() + tc.modify(claims) + token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) + token.Header["kid"] = "kid-1" + idToken, err := token.SignedString(privateKey) + if err != nil { + t.Fatalf("sign error: %s", err) + } + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + out, _ := json.Marshal(Token{AccessToken: "access", IDToken: idToken}) + w.Write(out) + })) + defer ts.Close() + discovery := Discovery{Issuer: tc.discoveryIssuer, TokenEndpoint: ts.URL + "/token"} + oauthData, err := UpdateOAuth2DataWithToken(jwks, discovery, "client-1", "secret", "https://vpn.inv/callback", "code", "state", OAuthData{ID: "id-1"}) + if tc.wantErr { + if err == nil { + t.Fatalf("expected error") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if oauthData.UserInfo.Email != "john@example.inv" || oauthData.Subject != "john" { + t.Fatalf("unexpected oauth data: %+v", oauthData) + } + }) + } +} diff --git a/auth/provisioning/scim/middleware.go b/auth/provisioning/scim/middleware.go index 4f5c1cb..8ac3340 100644 --- a/auth/provisioning/scim/middleware.go +++ b/auth/provisioning/scim/middleware.go @@ -1,6 +1,7 @@ package scim import ( + "crypto/subtle" "fmt" "net/http" "strings" @@ -12,7 +13,7 @@ func (s *Scim) authMiddleware(next http.Handler) http.Handler { writeWithStatus(w, []byte(`{"error": "token not found"}`), http.StatusUnauthorized) return } - tokenString := strings.Replace(r.Header.Get("Authorization"), "Bearer ", "", -1) + tokenString := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") if len(tokenString) == 0 { returnError(w, fmt.Errorf("empty token"), http.StatusUnauthorized) return @@ -21,7 +22,7 @@ func (s *Scim) authMiddleware(next http.Handler) http.Handler { writeWithStatus(w, []byte(`{"error": "scim not active"}`), http.StatusUnauthorized) return } - if s.Token != tokenString { + if subtle.ConstantTimeCompare([]byte(s.Token), []byte(tokenString)) != 1 { writeWithStatus(w, []byte(`{"error": "authentication failed"}`), http.StatusUnauthorized) return } diff --git a/auth/provisioning/scim/middleware_test.go b/auth/provisioning/scim/middleware_test.go new file mode 100644 index 0000000..f8220c4 --- /dev/null +++ b/auth/provisioning/scim/middleware_test.go @@ -0,0 +1,38 @@ +package scim + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +func TestAuthMiddleware(t *testing.T) { + tests := []struct { + configuredToken string + header string + expectedStatus int + }{ + {configuredToken: "token", header: "Bearer token", expectedStatus: http.StatusOK}, + {configuredToken: "token", header: "Bearer wrong", expectedStatus: http.StatusUnauthorized}, + {configuredToken: "token", header: "Bearer tok", expectedStatus: http.StatusUnauthorized}, + {configuredToken: "token", header: "Bearer tokenx", expectedStatus: http.StatusUnauthorized}, + {configuredToken: "token", header: "Bearer Bearer token", expectedStatus: http.StatusUnauthorized}, + {configuredToken: "token", header: "Bearer ", expectedStatus: http.StatusUnauthorized}, + {configuredToken: "token", header: "token", expectedStatus: http.StatusUnauthorized}, + {configuredToken: "", header: "Bearer ", expectedStatus: http.StatusUnauthorized}, + {configuredToken: "", header: "Bearer x", expectedStatus: http.StatusUnauthorized}, + } + for _, test := range tests { + s := &Scim{Token: test.configuredToken} + handler := s.authMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + req := httptest.NewRequest("GET", "http://example.com/api/scim/v2/Users", nil) + req.Header.Set("Authorization", test.header) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != test.expectedStatus { + t.Fatalf("token %q, header %q: expected status %d, got %d", test.configuredToken, test.header, test.expectedStatus, w.Code) + } + } +} diff --git a/auth/provisioning/scim/paging_test.go b/auth/provisioning/scim/paging_test.go new file mode 100644 index 0000000..8916cb6 --- /dev/null +++ b/auth/provisioning/scim/paging_test.go @@ -0,0 +1,104 @@ +package scim + +import ( + "encoding/json" + "fmt" + "net/http/httptest" + "testing" + + memorystorage "github.com/in4it/go-devops-platform/storage/memory" + "github.com/in4it/go-devops-platform/users" +) + +func newScimWithUsers(t *testing.T, userCount int) (*Scim, []users.User) { + t.Helper() + storage := &memorystorage.MockMemoryStorage{} + userStore, err := users.NewUserStore(storage, USERSTORE_MAX_USERS) + if err != nil { + t.Fatalf("cannot create new user store: %s", err) + } + usersToCreate := make([]users.User, userCount) + for i := 0; i < userCount; i++ { + usersToCreate[i] = users.User{Login: fmt.Sprintf("user-%d@domain.inv", i)} + } + created, err := userStore.AddUsers(usersToCreate) + if err != nil { + t.Fatalf("cannot create users: %s", err) + } + return New(storage, userStore, "token"), created +} + +func getUsersPage(t *testing.T, s *Scim, query string) UserResponse { + t.Helper() + req := httptest.NewRequest("GET", "http://example.com/api/scim/v2/Users?"+query, nil) + w := httptest.NewRecorder() + s.GetUsersHandler(w, req) + resp := w.Result() + if resp.StatusCode != 200 { + t.Fatalf("unexpected status code: %d", resp.StatusCode) + } + var userResponse UserResponse + if err := json.NewDecoder(resp.Body).Decode(&userResponse); err != nil { + t.Fatalf("Could not decode output: %s", err) + } + return userResponse +} + +func TestUsersGetPagingNoSkippedUsers(t *testing.T) { + totalUserCount := 25 + s, created := newScimWithUsers(t, totalUserCount) + seen := []string{} + for start := 1; start <= totalUserCount; start += 10 { + page := getUsersPage(t, s, fmt.Sprintf("count=10&startIndex=%d", start)) + if page.TotalResults != totalUserCount { + t.Fatalf("wrong totalResults: %d", page.TotalResults) + } + if page.StartIndex != start { + t.Fatalf("wrong startIndex: %d", page.StartIndex) + } + for _, resource := range page.Resources { + seen = append(seen, resource.UserName) + } + } + if len(seen) != totalUserCount { + t.Fatalf("expected %d users over all pages, got %d", totalUserCount, len(seen)) + } + for k := range created { + if seen[k] != created[k].Login { + t.Fatalf("user %d mismatch: %s (actual) vs %s (expected)", k, seen[k], created[k].Login) + } + } +} + +func TestUsersGetPagingInvalidValues(t *testing.T) { + s, created := newScimWithUsers(t, 5) + tests := []struct { + query string + expectedCount int + expectedStart int + firstLogin string + }{ + {query: "count=-5&startIndex=1", expectedCount: 0, expectedStart: 1}, + {query: "count=-1&startIndex=1", expectedCount: 0, expectedStart: 1}, + {query: "count=2&startIndex=0", expectedCount: 2, expectedStart: 1, firstLogin: created[0].Login}, + {query: "count=2&startIndex=-10", expectedCount: 2, expectedStart: 1, firstLogin: created[0].Login}, + {query: "count=10&startIndex=6", expectedCount: 0, expectedStart: 6}, + {query: "count=10&startIndex=5", expectedCount: 1, expectedStart: 5, firstLogin: created[4].Login}, + {query: "", expectedCount: 5, expectedStart: 1, firstLogin: created[0].Login}, + } + for _, test := range tests { + page := getUsersPage(t, s, test.query) + if len(page.Resources) != test.expectedCount || page.ItemsPerPage != test.expectedCount { + t.Fatalf("%s: expected %d resources, got %d (itemsPerPage %d)", test.query, test.expectedCount, len(page.Resources), page.ItemsPerPage) + } + if page.StartIndex != test.expectedStart { + t.Fatalf("%s: expected startIndex %d, got %d", test.query, test.expectedStart, page.StartIndex) + } + if page.TotalResults != 5 { + t.Fatalf("%s: expected totalResults 5, got %d", test.query, page.TotalResults) + } + if test.firstLogin != "" && page.Resources[0].UserName != test.firstLogin { + t.Fatalf("%s: wrong first login: %s", test.query, page.Resources[0].UserName) + } + } +} diff --git a/auth/provisioning/scim/put_test.go b/auth/provisioning/scim/put_test.go new file mode 100644 index 0000000..9006119 --- /dev/null +++ b/auth/provisioning/scim/put_test.go @@ -0,0 +1,128 @@ +package scim + +import ( + "bytes" + "encoding/json" + "fmt" + "net/http/httptest" + "testing" + + "github.com/in4it/go-devops-platform/storage" + memorystorage "github.com/in4it/go-devops-platform/storage/memory" + "github.com/in4it/go-devops-platform/users" +) + +func newScimForPut(t *testing.T) (*Scim, *int, *int) { + t.Helper() + disableCalls, reactivateCalls := 0, 0 + storage_ := &memorystorage.MockMemoryStorage{} + userStore, err := users.NewUserStoreWithHooks(storage_, USERSTORE_MAX_USERS, users.UserHooks{ + DisableFunc: func(storage.Iface, users.User) error { + disableCalls++ + return nil + }, + ReactivateFunc: func(storage.Iface, users.User) error { + reactivateCalls++ + return nil + }, + }) + if err != nil { + t.Fatalf("cannot create new user store: %s", err) + } + return New(storage_, userStore, "token"), &disableCalls, &reactivateCalls +} + +func putUser(t *testing.T, s *Scim, id string, payload PostUserRequest) *httptest.ResponseRecorder { + t.Helper() + payloadBytes, err := json.Marshal(payload) + if err != nil { + t.Fatalf("cannot marshal payload: %s", err) + } + req := httptest.NewRequest("PUT", fmt.Sprintf("http://example.com/api/scim/v2/Users/%s", id), bytes.NewBuffer(payloadBytes)) + req.SetPathValue("id", id) + w := httptest.NewRecorder() + s.PutUserHandler(w, req) + return w +} + +func TestPutUserRename(t *testing.T) { + s, disableCalls, _ := newScimForPut(t) + user, err := s.UserStore.AddUser(users.User{Login: "john@domain.inv", Role: "user", Provisioned: true, Password: "secret"}) + if err != nil { + t.Fatalf("add user error: %s", err) + } + w := putUser(t, s, user.ID, PostUserRequest{UserName: "john.doe@domain.inv", Active: false}) + if w.Code != 200 { + t.Fatalf("expected status 200, got %d: %s", w.Code, w.Body.String()) + } + updated, err := s.UserStore.GetUserByID(user.ID) + if err != nil { + t.Fatalf("get user error: %s", err) + } + if updated.Login != "john.doe@domain.inv" { + t.Fatalf("expected user to be renamed, got %s", updated.Login) + } + if !updated.Suspended { + t.Fatalf("expected user to be suspended") + } + if *disableCalls != 1 { + t.Fatalf("expected disable hook to be called once, got %d", *disableCalls) + } + if s.UserStore.LoginExists("john@domain.inv") { + t.Fatalf("old login should not exist anymore") + } + if s.UserStore.UserCount() != 1 { + t.Fatalf("expected 1 user, got %d", s.UserStore.UserCount()) + } + if _, ok := s.UserStore.AuthUser("john.doe@domain.inv", "secret"); !ok { + t.Fatalf("expected password to be kept after rename") + } +} + +func TestPutUserRenameToExistingLogin(t *testing.T) { + s, disableCalls, _ := newScimForPut(t) + user, err := s.UserStore.AddUser(users.User{Login: "john@domain.inv", Role: "user", Provisioned: true}) + if err != nil { + t.Fatalf("add user error: %s", err) + } + _, err = s.UserStore.AddUser(users.User{Login: "jane@domain.inv", Role: "user", Provisioned: true}) + if err != nil { + t.Fatalf("add user error: %s", err) + } + w := putUser(t, s, user.ID, PostUserRequest{UserName: "jane@domain.inv", Active: false}) + if w.Code != 409 { + t.Fatalf("expected status 409, got %d: %s", w.Code, w.Body.String()) + } + if *disableCalls != 0 { + t.Fatalf("disable hook should not run when validation fails") + } + unchanged, err := s.UserStore.GetUserByID(user.ID) + if err != nil { + t.Fatalf("get user error: %s", err) + } + if unchanged.Login != "john@domain.inv" || unchanged.Suspended { + t.Fatalf("user should be unchanged: %+v", unchanged) + } +} + +func TestPutUserReactivate(t *testing.T) { + s, _, reactivateCalls := newScimForPut(t) + user, err := s.UserStore.AddUser(users.User{Login: "john@domain.inv", Role: "user", Provisioned: true, Suspended: true}) + if err != nil { + t.Fatalf("add user error: %s", err) + } + w := putUser(t, s, user.ID, PostUserRequest{UserName: "john@domain.inv", Active: true}) + if w.Code != 200 { + t.Fatalf("expected status 200, got %d: %s", w.Code, w.Body.String()) + } + if *reactivateCalls != 1 { + t.Fatalf("expected reactivate hook to be called once, got %d", *reactivateCalls) + } + updated, err := s.UserStore.GetUserByID(user.ID) + if err != nil { + t.Fatalf("get user error: %s", err) + } + if updated.Suspended { + t.Fatalf("expected user to be active") + } +} diff --git a/auth/provisioning/scim/response.go b/auth/provisioning/scim/response.go index c81456b..36794ed 100644 --- a/auth/provisioning/scim/response.go +++ b/auth/provisioning/scim/response.go @@ -7,12 +7,20 @@ import ( "github.com/in4it/go-devops-platform/users" ) +// listUserResponse returns a SCIM ListResponse. start is the 1-based startIndex (RFC 7644 section 3.4.2.4), +// values < 1 are interpreted as 1. count is the maximum number of resources to return, -1 (or any negative value) +// means no limit; callers must map a negative count from the request to 0. func listUserResponse(users []users.User, attributes string, count, start int) ([]byte, error) { - if start != -1 && start > 1 && start <= len(users) { - users = users[start:] - } totalResults := len(users) - if len(users) > count && count != -1 { + if start < 1 { + start = 1 + } + if start > len(users) { + users = users[:0] + } else { + users = users[start-1:] + } + if count >= 0 && len(users) > count { users = users[0:count] } response := UserResponse{ diff --git a/auth/provisioning/scim/users.go b/auth/provisioning/scim/users.go index 1d31650..ce041a8 100644 --- a/auth/provisioning/scim/users.go +++ b/auth/provisioning/scim/users.go @@ -46,11 +46,13 @@ func (s *Scim) GetUsersHandler(w http.ResponseWriter, r *http.Request) { filter := r.URL.Query().Get("filter") count, err := strconv.Atoi(r.URL.Query().Get("count")) if err != nil { - count = -1 + count = -1 // not specified: no limit + } else if count < 0 { + count = 0 // RFC 7644: a negative value SHALL be interpreted as 0 } start, err := strconv.Atoi(r.URL.Query().Get("startIndex")) - if err != nil { - start = 1 + if err != nil || start < 1 { + start = 1 // RFC 7644: a value less than 1 SHALL be interpreted as 1 } if filter != "" { @@ -101,14 +103,24 @@ func (s *Scim) PutUserHandler(w http.ResponseWriter, r *http.Request) { return } - if !putUserRequest.Active && !user.Suspended { // user is suspended + // validate before running any hooks + username := getUsername(putUserRequest) + if username != "" && user.Login != username { + if s.UserStore.LoginExists(username) { + returnError(w, fmt.Errorf("user with login %s already exists", username), http.StatusConflict) + return + } + user.Login = username + } + + if !putUserRequest.Active && !user.Suspended && s.UserStore.UserHooks.DisableFunc != nil { // user is suspended err = s.UserStore.UserHooks.DisableFunc(s.storage, user) if err != nil { returnError(w, fmt.Errorf("could not delete all clients for user %s: %s", user.ID, err), http.StatusBadRequest) return } } - if putUserRequest.Active && user.Suspended { // user is unsuspended + if putUserRequest.Active && user.Suspended && s.UserStore.UserHooks.ReactivateFunc != nil { // user is unsuspended err := s.UserStore.UserHooks.ReactivateFunc(s.storage, user) if err != nil { returnError(w, fmt.Errorf("could not reactivate all clients for user %s: %s", user.ID, err), http.StatusBadRequest) @@ -117,14 +129,8 @@ func (s *Scim) PutUserHandler(w http.ResponseWriter, r *http.Request) { } user.Suspended = !putUserRequest.Active - username := getUsername(putUserRequest) - if user.Login != username { - if !s.UserStore.LoginExists(username) { - user.Login = username - } - } - err = s.UserStore.UpdateUser(user) + err = s.UserStore.UpdateUserByID(user) if err != nil { returnError(w, fmt.Errorf("user update error: %s", err), http.StatusBadRequest) return @@ -146,10 +152,12 @@ func (s *Scim) DeleteUserHandler(w http.ResponseWriter, r *http.Request) { return } - err = s.UserStore.UserHooks.DeleteFunc(s.storage, user) - if err != nil { - returnError(w, fmt.Errorf("could not delete all clients for user %s: %s", user.ID, err), http.StatusBadRequest) - return + if s.UserStore.UserHooks.DeleteFunc != nil { + err = s.UserStore.UserHooks.DeleteFunc(s.storage, user) + if err != nil { + returnError(w, fmt.Errorf("could not delete all clients for user %s: %s", user.ID, err), http.StatusBadRequest) + return + } } err = s.UserStore.DeleteUserByID(user.ID) diff --git a/auth/provisioning/scim/users_test.go b/auth/provisioning/scim/users_test.go index 5ee1a23..660a152 100644 --- a/auth/provisioning/scim/users_test.go +++ b/auth/provisioning/scim/users_test.go @@ -116,7 +116,7 @@ func TestUsersGetCount10Start5(t *testing.T) { if err != nil { t.Fatalf("Could not decode output: %s", err) } - if userResponse.TotalResults != totalUserCount-start { + if userResponse.TotalResults != totalUserCount { t.Fatalf("Wrong user count: %d", userResponse.TotalResults) } if userResponse.ItemsPerPage != count { @@ -128,8 +128,8 @@ func TestUsersGetCount10Start5(t *testing.T) { if len(userResponse.Resources) != count { t.Fatalf("Wrong response count: %d", len(userResponse.Resources)) } - if userResponse.Resources[0].UserName != users[5].Login { - t.Fatalf("Wrong first login: %s (actual) vs %s (expected)", userResponse.Resources[0].UserName, users[5].Login) + if userResponse.Resources[0].UserName != users[start-1].Login { // startIndex is 1-based + t.Fatalf("Wrong first login: %s (actual) vs %s (expected)", userResponse.Resources[0].UserName, users[start-1].Login) } } diff --git a/auth/saml/assertion.go b/auth/saml/assertion.go new file mode 100644 index 0000000..14a58d3 --- /dev/null +++ b/auth/saml/assertion.go @@ -0,0 +1,76 @@ +package saml + +import ( + "fmt" + "time" + + "github.com/google/uuid" + saml2 "github.com/russellhaering/gosaml2" +) + +// DEFAULT_ASSERTION_TTL is how long a consumed assertion id is remembered when +// the assertion has no (parsable) Conditions NotOnOrAfter. +const DEFAULT_ASSERTION_TTL = 24 * time.Hour + +// MAX_ASSERTION_TTL bounds how long a consumed assertion id is remembered. +const MAX_ASSERTION_TTL = 7 * 24 * time.Hour + +// markAssertionConsumed records the assertion id and returns an error if the +// assertion was already used (replay). +func (s *saml) markAssertionConsumed(providerID string, assertionInfo *saml2.AssertionInfo, now time.Time) error { + if assertionInfo == nil || len(assertionInfo.Assertions) == 0 || assertionInfo.Assertions[0].ID == "" { + return fmt.Errorf("assertion id missing") + } + assertion := assertionInfo.Assertions[0] + expiresAt := now.Add(DEFAULT_ASSERTION_TTL) + if assertion.Conditions != nil && assertion.Conditions.NotOnOrAfter != "" { + if notOnOrAfter, err := time.Parse(time.RFC3339, assertion.Conditions.NotOnOrAfter); err == nil { + expiresAt = notOnOrAfter + } + } + if expiresAt.After(now.Add(MAX_ASSERTION_TTL)) { + expiresAt = now.Add(MAX_ASSERTION_TTL) + } + key := providerID + "/" + assertion.ID + + s.mu.Lock() + defer s.mu.Unlock() + if s.consumedAssertions == nil { + s.consumedAssertions = make(map[string]time.Time) + } + if consumedUntil, ok := s.consumedAssertions[key]; ok && !now.After(consumedUntil) { + return fmt.Errorf("assertion already used") + } + s.consumedAssertions[key] = expiresAt + return nil +} + +// createSessionFromAssertion creates a single-use session for a validated +// assertion and returns the session id (the code passed to the browser). +func (s *saml) createSessionFromAssertion(providerID string, assertionInfo *saml2.AssertionInfo) (string, error) { + now := time.Now() + err := s.markAssertionConsumed(providerID, assertionInfo, now) + if err != nil { + return "", err + } + + notAfter := now.Add(DEFAULT_SESSION_DURATION) + if assertionInfo.SessionNotOnOrAfter != nil { + notAfter = *assertionInfo.SessionNotOnOrAfter + } + + randomString, err := getRandomString(128) + if err != nil { + return "", fmt.Errorf("could not create session: %s", err) + } + sessionKey := SessionKey{ + ProviderID: providerID, + SessionID: randomString, + } + s.CreateSession(sessionKey, AuthenticatedUser{ + ID: uuid.New().String(), + Login: assertionInfo.NameID, + ExpiresAt: notAfter, + }) + return sessionKey.SessionID, nil +} diff --git a/auth/saml/handlers.go b/auth/saml/handlers.go index 59cb73b..7cd5b69 100644 --- a/auth/saml/handlers.go +++ b/auth/saml/handlers.go @@ -3,8 +3,6 @@ package saml import ( "fmt" "net/http" - - "github.com/google/uuid" ) func (s *saml) samlHandler(w http.ResponseWriter, r *http.Request) { @@ -73,24 +71,12 @@ func (s *saml) samlHandler(w http.ResponseWriter, r *http.Request) { return } - login := assertionInfo.NameID - notAfter := *assertionInfo.SessionNotOnOrAfter - - randomString, err := getRandomString(128) + sessionID, err := s.createSessionFromAssertion(providerID, assertionInfo) if err != nil { w.WriteHeader(http.StatusForbidden) - w.Write([]byte(fmt.Sprintf("saml error: could not create session: %s\n", err))) + w.Write([]byte(fmt.Sprintf("saml error: %s\n", err))) return } - sessionKey := SessionKey{ - ProviderID: providerID, - SessionID: randomString, - } - s.CreateSession(sessionKey, AuthenticatedUser{ - ID: uuid.New().String(), - Login: login, - ExpiresAt: notAfter, - }) - w.Header().Add("Location", fmt.Sprintf("/callback/saml/%s?code=%s", providerID, sessionKey.SessionID)) + w.Header().Add("Location", fmt.Sprintf("/callback/saml/%s?code=%s", providerID, sessionID)) w.WriteHeader(http.StatusFound) } diff --git a/auth/saml/new.go b/auth/saml/new.go index 5a06696..8e1904f 100644 --- a/auth/saml/new.go +++ b/auth/saml/new.go @@ -1,18 +1,21 @@ package saml import ( + "time" + "github.com/in4it/go-devops-platform/storage" saml2 "github.com/russellhaering/gosaml2" ) func New(providers *[]Provider, storage storage.Iface, protocol, hostname *string) Iface { s := &saml{ - Providers: providers, - sessions: make(map[SessionKey]AuthenticatedUser), - serviceProvider: make(map[string]*saml2.SAMLServiceProvider), - protocol: protocol, - hostname: hostname, - storage: storage, + Providers: providers, + sessions: make(map[SessionKey]session), + consumedAssertions: make(map[string]time.Time), + serviceProvider: make(map[string]*saml2.SAMLServiceProvider), + protocol: protocol, + hostname: hostname, + storage: storage, } for _, provider := range *providers { s.loadSP(provider) diff --git a/auth/saml/session.go b/auth/saml/session.go index ce473c5..aaf129a 100644 --- a/auth/saml/session.go +++ b/auth/saml/session.go @@ -5,25 +5,83 @@ import ( "time" ) +// SESSION_REDEEM_TTL is how long the code handed to the browser after the ACS +// callback can be exchanged for a token. The code can only be used once. +const SESSION_REDEEM_TTL = 5 * time.Minute + +// DEFAULT_SESSION_DURATION is used when the IdP doesn't send a +// SessionNotOnOrAfter in the AuthnStatement. +const DEFAULT_SESSION_DURATION = 72 * time.Hour + +type session struct { + user AuthenticatedUser + createdAt time.Time +} + +// GetAuthenticatedUser returns the authenticated user of a session without +// consuming it. Use ConsumeAuthenticatedUser to redeem a session. func (s *saml) GetAuthenticatedUser(provider Provider, sessionID string) (AuthenticatedUser, error) { sessionKey := SessionKey{ ProviderID: provider.ID, SessionID: sessionID, } s.mu.Lock() - authenticatedUser, ok := s.sessions[sessionKey] + sess, ok := s.sessions[sessionKey] s.mu.Unlock() + if !ok { + return AuthenticatedUser{}, fmt.Errorf("session not found") + } + return validateSession(sess, time.Now()) +} + +// ConsumeAuthenticatedUser returns the authenticated user of a session and +// deletes the session, so the same code can't be redeemed twice. +func (s *saml) ConsumeAuthenticatedUser(provider Provider, sessionID string) (AuthenticatedUser, error) { + sessionKey := SessionKey{ + ProviderID: provider.ID, + SessionID: sessionID, + } + s.mu.Lock() + sess, ok := s.sessions[sessionKey] if ok { - if authenticatedUser.ExpiresAt.Before(time.Now()) { - return authenticatedUser, fmt.Errorf("session is expired") - } - return authenticatedUser, nil + delete(s.sessions, sessionKey) } - return AuthenticatedUser{}, fmt.Errorf("session not found") + s.mu.Unlock() + if !ok { + return AuthenticatedUser{}, fmt.Errorf("session not found") + } + return validateSession(sess, time.Now()) +} + +func validateSession(sess session, now time.Time) (AuthenticatedUser, error) { + if now.After(sess.createdAt.Add(SESSION_REDEEM_TTL)) { + return sess.user, fmt.Errorf("session is expired") + } + if sess.user.ExpiresAt.Before(now) { + return sess.user, fmt.Errorf("session is expired") + } + return sess.user, nil } func (s *saml) CreateSession(key SessionKey, value AuthenticatedUser) { s.mu.Lock() defer s.mu.Unlock() - s.sessions[key] = value + now := time.Now() + s.pruneExpired(now) + s.sessions[key] = session{user: value, createdAt: now} +} + +// pruneExpired removes sessions that can't be redeemed anymore and consumed +// assertion ids that can't be replayed anymore. Must be called with s.mu held. +func (s *saml) pruneExpired(now time.Time) { + for k, sess := range s.sessions { + if now.After(sess.createdAt.Add(SESSION_REDEEM_TTL)) { + delete(s.sessions, k) + } + } + for k, expiresAt := range s.consumedAssertions { + if now.After(expiresAt) { + delete(s.consumedAssertions, k) + } + } } diff --git a/auth/saml/session_security_test.go b/auth/saml/session_security_test.go new file mode 100644 index 0000000..303ca0c --- /dev/null +++ b/auth/saml/session_security_test.go @@ -0,0 +1,113 @@ +package saml + +import ( + "testing" + "time" + + saml2 "github.com/russellhaering/gosaml2" + "github.com/russellhaering/gosaml2/types" +) + +func newTestSAML() *saml { + return New(&[]Provider{}, nil, nil, nil).(*saml) +} + +func testAssertionInfo(id string, notOnOrAfter string, sessionNotOnOrAfter *time.Time) *saml2.AssertionInfo { + return &saml2.AssertionInfo{ + NameID: "john@example.com", + SessionNotOnOrAfter: sessionNotOnOrAfter, + Assertions: []types.Assertion{ + {ID: id, Conditions: &types.Conditions{NotOnOrAfter: notOnOrAfter}}, + }, + } +} + +// TestSAMLSessionSingleUse: the code handed out by the ACS handler can only be +// redeemed once. +func TestSAMLSessionSingleUse(t *testing.T) { + s := newTestSAML() + provider := Provider{ID: "prov-1"} + s.CreateSession(SessionKey{ProviderID: provider.ID, SessionID: "code-1"}, AuthenticatedUser{Login: "john", ExpiresAt: time.Now().Add(time.Hour)}) + + user, err := s.ConsumeAuthenticatedUser(provider, "code-1") + if err != nil || user.Login != "john" { + t.Fatalf("expected first redeem to succeed, got %+v, %v", user, err) + } + if _, err := s.ConsumeAuthenticatedUser(provider, "code-1"); err == nil { + t.Fatalf("expected second redeem of the same code to fail") + } + if _, err := s.GetAuthenticatedUser(provider, "code-1"); err == nil { + t.Fatalf("expected consumed session to be gone") + } +} + +// TestSAMLSessionRedeemTTL: a code can only be redeemed shortly after it was +// created, even if the user's SAML session is valid for longer. Expired +// sessions are pruned when a new session is created. +func TestSAMLSessionRedeemTTL(t *testing.T) { + s := newTestSAML() + provider := Provider{ID: "prov-1"} + oldKey := SessionKey{ProviderID: provider.ID, SessionID: "old"} + s.sessions[oldKey] = session{ + user: AuthenticatedUser{Login: "john", ExpiresAt: time.Now().Add(24 * time.Hour)}, + createdAt: time.Now().Add(-SESSION_REDEEM_TTL - time.Second), + } + if _, err := s.GetAuthenticatedUser(provider, "old"); err == nil { + t.Fatalf("expected session older than the redeem ttl to be expired") + } + s.CreateSession(SessionKey{ProviderID: provider.ID, SessionID: "new"}, AuthenticatedUser{Login: "jane", ExpiresAt: time.Now().Add(time.Hour)}) + if _, ok := s.sessions[oldKey]; ok { + t.Fatalf("expected expired session to be pruned on create") + } + if _, err := s.ConsumeAuthenticatedUser(provider, "new"); err != nil { + t.Fatalf("expected new session to be valid: %s", err) + } +} + +// TestSAMLAssertionReplay: the same assertion can't be used twice. +func TestSAMLAssertionReplay(t *testing.T) { + s := newTestSAML() + notOnOrAfter := time.Now().Add(5 * time.Minute).UTC().Format(time.RFC3339) + sessionNotOnOrAfter := time.Now().Add(time.Hour) + code, err := s.createSessionFromAssertion("prov-1", testAssertionInfo("assertion-1", notOnOrAfter, &sessionNotOnOrAfter)) + if err != nil || code == "" { + t.Fatalf("expected session to be created, got %q, %v", code, err) + } + if _, err := s.createSessionFromAssertion("prov-1", testAssertionInfo("assertion-1", notOnOrAfter, &sessionNotOnOrAfter)); err == nil { + t.Fatalf("expected replayed assertion to be rejected") + } + if _, err := s.createSessionFromAssertion("prov-1", testAssertionInfo("assertion-2", notOnOrAfter, &sessionNotOnOrAfter)); err != nil { + t.Fatalf("expected other assertion to be accepted: %s", err) + } + if _, err := s.createSessionFromAssertion("prov-1", testAssertionInfo("", notOnOrAfter, &sessionNotOnOrAfter)); err == nil { + t.Fatalf("expected assertion without id to be rejected") + } + // consumed assertion ids are pruned once they can't be replayed anymore + s.mu.Lock() + s.consumedAssertions["prov-1/stale"] = time.Now().Add(-time.Second) + s.mu.Unlock() + s.CreateSession(SessionKey{ProviderID: "prov-1", SessionID: "x"}, AuthenticatedUser{ExpiresAt: time.Now().Add(time.Hour)}) + if _, ok := s.consumedAssertions["prov-1/stale"]; ok { + t.Fatalf("expected stale consumed assertion to be pruned") + } +} + +// TestSAMLMissingSessionNotOnOrAfter: an IdP that omits SessionNotOnOrAfter +// must not crash the ACS handler. +func TestSAMLMissingSessionNotOnOrAfter(t *testing.T) { + s := newTestSAML() + code, err := s.createSessionFromAssertion("prov-1", testAssertionInfo("assertion-1", "", nil)) + if err != nil { + t.Fatalf("expected session to be created: %s", err) + } + user, err := s.ConsumeAuthenticatedUser(Provider{ID: "prov-1"}, code) + if err != nil { + t.Fatalf("expected valid session: %s", err) + } + if user.Login != "john@example.com" { + t.Fatalf("unexpected login: %s", user.Login) + } + if user.ExpiresAt.Before(time.Now().Add(DEFAULT_SESSION_DURATION - time.Minute)) { + t.Fatalf("expected default session duration, got expiry %s", user.ExpiresAt) + } +} diff --git a/auth/saml/session_test.go b/auth/saml/session_test.go index 2c3a1ff..2a40208 100644 --- a/auth/saml/session_test.go +++ b/auth/saml/session_test.go @@ -13,7 +13,7 @@ import ( // sessions while the ACS handler creates them. func TestSessionsConcurrentAccess(t *testing.T) { s := &saml{ - sessions: make(map[SessionKey]AuthenticatedUser), + sessions: make(map[SessionKey]session), serviceProvider: make(map[string]*saml2.SAMLServiceProvider), } provider := Provider{ID: "prov-1"} diff --git a/auth/saml/types.go b/auth/saml/types.go index 05838d4..ca7171d 100644 --- a/auth/saml/types.go +++ b/auth/saml/types.go @@ -23,11 +23,13 @@ type Provider struct { type saml struct { Providers *[]Provider serviceProvider map[string]*saml2.SAMLServiceProvider - sessions map[SessionKey]AuthenticatedUser - hostname *string - protocol *string - mu sync.Mutex - storage storage.Iface + sessions map[SessionKey]session + // consumed assertion ids (providerID/assertionID) and until when they're kept + consumedAssertions map[string]time.Time + hostname *string + protocol *string + mu sync.Mutex + storage storage.Iface } type AuthenticatedUser struct { ID string @@ -43,6 +45,7 @@ type Iface interface { GetAuthURL(provider Provider) (string, error) GetRouter() *http.ServeMux GetAuthenticatedUser(provider Provider, sessionID string) (AuthenticatedUser, error) + ConsumeAuthenticatedUser(provider Provider, sessionID string) (AuthenticatedUser, error) HasValidMetadataURL(metadataURL string) (bool, error) CreateSession(key SessionKey, value AuthenticatedUser) } diff --git a/mfa/totp/verify.go b/mfa/totp/verify.go index 651afb3..23e556d 100644 --- a/mfa/totp/verify.go +++ b/mfa/totp/verify.go @@ -4,15 +4,22 @@ import ( "bytes" "crypto/hmac" "crypto/sha1" + "crypto/sha256" + "crypto/subtle" "encoding/base32" "encoding/binary" "fmt" "strings" + "sync" "time" ) const INTERVAL = 30 +// SKEW is the number of time steps before and after the current one that are +// accepted by Verify, to allow for clock drift between server and client. +const SKEW = 1 + func GetToken(secret string, interval int64) (string, error) { key, err := base32.StdEncoding.DecodeString(strings.ToUpper(secret)) if err != nil { @@ -36,12 +43,12 @@ func GetToken(secret string, interval int64) (string, error) { return fmt.Sprintf("%06d", int((int(header)&0x7fffffff)%1000000)), nil } +// Verify checks a TOTP code for the given secret. The current time step and +// SKEW steps before/after are accepted. A code that has been accepted once +// for a secret can't be used again (replay protection): any code for the same +// or an earlier time step of that secret is rejected afterwards. func Verify(secret, code string) (bool, error) { - token, err := GetToken(secret, time.Now().Unix()/30) - if err != nil { - return false, fmt.Errorf("GetToken error: %s", err) - } - return token == code, nil + return defaultReplayCache.verify(secret, code, time.Now()) } func VerifyMultipleIntervals(secret, code string, count int) (bool, error) { @@ -49,14 +56,82 @@ func VerifyMultipleIntervals(secret, code string, count int) (bool, error) { } func verifyMultipleIntervals(secret, code string, count int, now time.Time) (bool, error) { + matched := false for i := 0; i < count; i++ { token, err := GetToken(secret, now.Add(time.Duration(i)*time.Duration(-30)*time.Second).Unix()/30) if err != nil { return false, fmt.Errorf("GetToken error: %s", err) } - if token == code { - return true, nil + if tokenEqual(token, code) { + matched = true + } + } + return matched, nil +} + +func tokenEqual(token, code string) bool { + return subtle.ConstantTimeCompare([]byte(token), []byte(code)) == 1 +} + +// matchInterval returns the matching interval within now ± SKEW steps (constant +// amount of work regardless of which step matches). +func matchInterval(secret, code string, now time.Time) (int64, bool, error) { + current := now.Unix() / INTERVAL + var matchedInterval int64 + matched := false + for i := current - SKEW; i <= current+SKEW; i++ { + token, err := GetToken(secret, i) + if err != nil { + return 0, false, fmt.Errorf("GetToken error: %s", err) + } + if tokenEqual(token, code) && !matched { + matched = true + matchedInterval = i + } + } + return matchedInterval, matched, nil +} + +// replayCache keeps, per factor (keyed by a hash of the secret), the last +// interval for which a code was accepted. Entries older than the accepted +// window can't be replayed anyway and are pruned. +type replayCache struct { + mu sync.Mutex + used map[[32]byte]int64 +} + +var defaultReplayCache = newReplayCache() + +func newReplayCache() *replayCache { + return &replayCache{used: make(map[[32]byte]int64)} +} + +func (r *replayCache) verify(secret, code string, now time.Time) (bool, error) { + interval, ok, err := matchInterval(secret, code, now) + if err != nil { + return false, err + } + if !ok { + return false, nil + } + key := sha256.Sum256([]byte(strings.ToUpper(secret))) + current := now.Unix() / INTERVAL + + r.mu.Lock() + defer r.mu.Unlock() + r.prune(current) + if last, found := r.used[key]; found && interval <= last { + return false, nil // replay of an already used (or older) code + } + r.used[key] = interval + return true, nil +} + +// prune removes entries that are outside the acceptance window. Must be called with mu held. +func (r *replayCache) prune(current int64) { + for k, last := range r.used { + if last < current-SKEW { + delete(r.used, k) } } - return false, nil } diff --git a/mfa/totp/verify_test.go b/mfa/totp/verify_test.go index 5c52922..8d77811 100644 --- a/mfa/totp/verify_test.go +++ b/mfa/totp/verify_test.go @@ -47,3 +47,126 @@ func TestVerifyMultipleIntervalsWrongToken(t *testing.T) { t.Fatalf("token matched, but shouldn't have") } } + +func TestVerifyClockSkew(t *testing.T) { + secret := "GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ" + now := time.Unix(1718272397, 0) + current := now.Unix() / INTERVAL + for _, offset := range []int64{-1, 0, 1} { + r := newReplayCache() + token, err := GetToken(secret, current+offset) + if err != nil { + t.Fatalf("GetToken error: %s", err) + } + ok, err := r.verify(secret, token, now) + if err != nil { + t.Fatalf("verify error: %s", err) + } + if !ok { + t.Fatalf("expected token with offset %d to be accepted", offset) + } + } + for _, offset := range []int64{-2, 2} { + r := newReplayCache() + token, err := GetToken(secret, current+offset) + if err != nil { + t.Fatalf("GetToken error: %s", err) + } + ok, err := r.verify(secret, token, now) + if err != nil { + t.Fatalf("verify error: %s", err) + } + if ok { + t.Fatalf("expected token with offset %d to be rejected", offset) + } + } +} + +func TestVerifyReplay(t *testing.T) { + secret := "GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ" + now := time.Unix(1718272397, 0) + current := now.Unix() / INTERVAL + r := newReplayCache() + token, err := GetToken(secret, current) + if err != nil { + t.Fatalf("GetToken error: %s", err) + } + ok, err := r.verify(secret, token, now) + if err != nil || !ok { + t.Fatalf("expected first use to succeed (ok=%v, err=%v)", ok, err) + } + ok, err = r.verify(secret, token, now.Add(5*time.Second)) + if err != nil { + t.Fatalf("verify error: %s", err) + } + if ok { + t.Fatalf("expected replayed token to be rejected") + } + // an older code within the skew window can't be used after a newer one + prevToken, err := GetToken(secret, current-1) + if err != nil { + t.Fatalf("GetToken error: %s", err) + } + ok, err = r.verify(secret, prevToken, now) + if err != nil { + t.Fatalf("verify error: %s", err) + } + if ok { + t.Fatalf("expected older token to be rejected after newer token was used") + } + // next interval's code is still accepted + nextToken, err := GetToken(secret, current+1) + if err != nil { + t.Fatalf("GetToken error: %s", err) + } + ok, err = r.verify(secret, nextToken, now.Add(30*time.Second)) + if err != nil || !ok { + t.Fatalf("expected next interval token to be accepted (ok=%v, err=%v)", ok, err) + } + // other factor (different secret) isn't affected + otherSecret := "JBSWY3DPEHPK3PXP" + otherToken, err := GetToken(otherSecret, current) + if err != nil { + t.Fatalf("GetToken error: %s", err) + } + ok, err = r.verify(otherSecret, otherToken, now) + if err != nil || !ok { + t.Fatalf("expected other secret token to be accepted (ok=%v, err=%v)", ok, err) + } +} + +func TestVerifyReplayCachePruned(t *testing.T) { + secret := "GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ" + now := time.Unix(1718272397, 0) + r := newReplayCache() + token, err := GetToken(secret, now.Unix()/INTERVAL) + if err != nil { + t.Fatalf("GetToken error: %s", err) + } + if ok, err := r.verify(secret, token, now); err != nil || !ok { + t.Fatalf("expected token to be accepted (ok=%v, err=%v)", ok, err) + } + later := now.Add(10 * time.Minute) + otherSecret := "JBSWY3DPEHPK3PXP" + otherToken, err := GetToken(otherSecret, later.Unix()/INTERVAL) + if err != nil { + t.Fatalf("GetToken error: %s", err) + } + if ok, err := r.verify(otherSecret, otherToken, later); err != nil || !ok { + t.Fatalf("expected token to be accepted (ok=%v, err=%v)", ok, err) + } + if len(r.used) != 1 { + t.Fatalf("expected stale entries to be pruned, got %d entries", len(r.used)) + } +} + +func TestVerifyWrongCode(t *testing.T) { + secret := "GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ" + ok, err := newReplayCache().verify(secret, "", time.Now()) + if err != nil { + t.Fatalf("verify error: %s", err) + } + if ok { + t.Fatalf("expected empty code to be rejected") + } +} diff --git a/rest/auth.go b/rest/auth.go index afe5db8..56d2312 100644 --- a/rest/auth.go +++ b/rest/auth.go @@ -2,6 +2,7 @@ package rest import ( "encoding/json" + "errors" "fmt" "net/http" "strings" @@ -80,6 +81,7 @@ func (c *Context) oidcProviderHandler(w http.ResponseWriter, r *http.Request) { for k := range oidcProviders { oidcProviders[k].LoginURL = fmt.Sprintf("%s://%s%s", c.Protocol, c.Hostname, strings.Replace(oidcProviders[k].RedirectURI, "/callback/", "/login/", -1)) oidcProviders[k].RedirectURI = fmt.Sprintf("%s://%s%s", c.Protocol, c.Hostname, oidcProviders[k].RedirectURI) + oidcProviders[k].ClientSecret = "" // never return the client secret } out, err := json.Marshal(oidcProviders) if err != nil { @@ -118,7 +120,9 @@ func (c *Context) oidcProviderHandler(w http.ResponseWriter, r *http.Request) { } oidcProvider.RedirectURI = "/callback/oidc/" + oidcProvider.ID c.OIDCProviders = append(c.OIDCProviders, oidcProvider) - out, err := json.Marshal(oidcProvider) + oidcProviderResponse := oidcProvider + oidcProviderResponse.ClientSecret = "" // never return the client secret + out, err := json.Marshal(oidcProviderResponse) if err != nil { c.returnError(w, fmt.Errorf("oidcProvider marshal error: %s", err), http.StatusBadRequest) return @@ -206,14 +210,25 @@ func (c *Context) authMethodsByID(w http.ResponseWriter, r *http.Request) { return } - samlSession, err := c.SAML.Client.GetAuthenticatedUser(samlProvider, samlCallback.Code) + // the code can only be redeemed once + samlSession, err := c.SAML.Client.ConsumeAuthenticatedUser(samlProvider, samlCallback.Code) if err != nil { c.returnError(w, fmt.Errorf("saml session not found"), http.StatusBadRequest) return } // add user to the user database (or modify existing one) - user, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, samlSession.Login, "saml", samlSession.ID) + user, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, samlSession.Login, "saml", samlSession.ID) + if errors.Is(err, errNoLicense) { + loginResponse.NoLicense = true + out, err := json.Marshal(loginResponse) + if err != nil { + c.returnError(w, fmt.Errorf("loginResponse Marshal error: %s", err), http.StatusBadRequest) + return + } + c.write(w, out) + return + } if err != nil { c.returnError(w, fmt.Errorf("couldn't add/modify user in database: %s", err), http.StatusBadRequest) return @@ -230,7 +245,12 @@ func (c *Context) authMethodsByID(w http.ResponseWriter, r *http.Request) { return } - token, err := login.GetJWTTokenWithExpiration(user.Login, user.Role, c.JWTKeys.PrivateKey, c.JWTKeysKID, samlSession.ExpiresAt) + // don't issue tokens that live longer than local tokens + expiresAt := samlSession.ExpiresAt + if maxExpiresAt := time.Now().Add(login.MAX_TOKEN_LIFETIME); expiresAt.After(maxExpiresAt) { + expiresAt = maxExpiresAt + } + token, err := login.GetJWTTokenWithExpiration(user.Login, user.Role, c.JWTKeys.PrivateKey, c.JWTKeysKID, expiresAt) if err != nil { c.returnError(w, fmt.Errorf("token generation failed: %s", err), http.StatusBadRequest) return @@ -263,21 +283,13 @@ func (c *Context) authMethodsByID(w http.ResponseWriter, r *http.Request) { c.returnError(w, fmt.Errorf("cannot find oauth2 data using state provided: %s", err), http.StatusBadRequest) return } + if oauth2data.OIDCProviderID != oidcProvider.ID { + c.returnError(w, fmt.Errorf("state doesn't belong to this oidc provider"), http.StatusBadRequest) + return + } if oauth2data.Token.AccessToken != "" { - if oauth2data.Suspended { - loginResponse.Suspended = true - } else if c.LicenseUserCount >= c.UserStore.UserCount() { - loginResponse.NoLicense = true - } else { - loginResponse.Authenticated = true - loginResponse.Token = oauth2data.Token.AccessToken - } - out, err := json.Marshal(loginResponse) - if err != nil { - c.returnError(w, fmt.Errorf("loginResponse Marshal error: %s", err), http.StatusBadRequest) - return - } - c.write(w, out) + // states are single-use: the code was already exchanged for a token + c.returnError(w, fmt.Errorf("state already used, login again"), http.StatusBadRequest) return } // no token, let's generate a new one @@ -297,7 +309,17 @@ func (c *Context) authMethodsByID(w http.ResponseWriter, r *http.Request) { return } // add user to the user database (or modify existing one) - user, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, updatedOauth2data.UserInfo.Email, "oidc", updatedOauth2data.ID) + user, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, updatedOauth2data.UserInfo.Email, "oidc", updatedOauth2data.ID) + if errors.Is(err, errNoLicense) { + loginResponse.NoLicense = true + out, err := json.Marshal(loginResponse) + if err != nil { + c.returnError(w, fmt.Errorf("loginResponse Marshal error: %s", err), http.StatusBadRequest) + return + } + c.write(w, out) + return + } if err != nil { c.returnError(w, fmt.Errorf("couldn't add/modify user in database: %s", err), http.StatusBadRequest) return diff --git a/rest/auth_test.go b/rest/auth_test.go index 46aad3a..81c4a42 100644 --- a/rest/auth_test.go +++ b/rest/auth_test.go @@ -794,7 +794,8 @@ func TestOIDCFlow(t *testing.T) { return } token := jwt.NewWithClaims(jwt.GetSigningMethod("RS256"), jwt.MapClaims{ - "iss": "test-server", + "iss": "test-issuer", + "aud": oidcProvider.ClientID, "sub": "john", "email": "john@example.inv", "role": "user", @@ -941,6 +942,19 @@ func TestOIDCFlow(t *testing.T) { if loginResponse.Token == "" { t.Fatalf("no token received: %+v", loginResponse) } + + // the state is single-use: a second callback must not return a token + req = httptest.NewRequest("POST", "http://example.inv/api/authmethods/oidc/"+oidcProvider.ID, bytes.NewBuffer(callbackPayload)) + req.SetPathValue("id", oidcProvider.ID) + req.SetPathValue("method", "oidc") + w = httptest.NewRecorder() + c.authMethodsByID(w, req) + if w.Result().StatusCode != http.StatusBadRequest { + t.Fatalf("expected 400 for second callback with the same state, got %d", w.Result().StatusCode) + } + if strings.Contains(w.Body.String(), loginResponse.Token) { + t.Fatalf("token returned for second callback with the same state") + } } func TestOIDCRedirect(t *testing.T) { diff --git a/rest/login/auth_test.go b/rest/login/auth_test.go index 0137e5d..56ad0b9 100644 --- a/rest/login/auth_test.go +++ b/rest/login/auth_test.go @@ -121,3 +121,51 @@ func TestAuthenticateMFAWithToken(t *testing.T) { t.Fatalf("expected to be authenticated") } } + +func TestAuthenticateMFAReplay(t *testing.T) { + secret := base32.StdEncoding.EncodeToString([]byte("replaysecret")) + m := MockAuth{ + AuthUserUser: users.User{ + Login: "john", + Factors: []users.Factor{ + { + Name: "test-factor", + Type: "test", + Secret: secret, + }, + }, + }, + AuthUserResult: true, + } + token, err := totp.GetToken(secret, time.Now().Unix()/30) + if err != nil { + t.Fatalf("GetToken error: %s", err) + } + loginReq := LoginRequest{ + Login: "john", + Password: "mypass", + FactorResponse: FactorResponse{ + Name: "test-factor", + Code: token, + }, + } + privateKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("private key error: %s", err) + } + + loginResp, _, err := Authenticate(loginReq, &m, privateKey, "jwtKeyID") + if err != nil { + t.Fatalf("authentication error: %s", err) + } + if !loginResp.Authenticated { + t.Fatalf("expected to be authenticated") + } + loginResp, _, err = Authenticate(loginReq, &m, privateKey, "jwtKeyID") + if err != nil { + t.Fatalf("authentication error: %s", err) + } + if loginResp.Authenticated { + t.Fatalf("expected replayed MFA code to be rejected") + } +} diff --git a/rest/login/jwt.go b/rest/login/jwt.go index bba4e09..397a927 100644 --- a/rest/login/jwt.go +++ b/rest/login/jwt.go @@ -7,8 +7,12 @@ import ( "github.com/golang-jwt/jwt/v5" ) +// MAX_TOKEN_LIFETIME is the lifetime of local tokens, and the maximum lifetime +// of tokens issued for external (SAML) logins +const MAX_TOKEN_LIFETIME = 72 * time.Hour + func GetJWTToken(login, role string, signKey *rsa.PrivateKey, kid string) (string, error) { - return GetJWTTokenWithExpiration(login, role, signKey, kid, time.Now().Add(time.Hour*72)) + return GetJWTTokenWithExpiration(login, role, signKey, kid, time.Now().Add(MAX_TOKEN_LIFETIME)) } func GetJWTTokenWithExpiration(login, role string, signKey *rsa.PrivateKey, kid string, expiration time.Time) (string, error) { diff --git a/rest/middleware.go b/rest/middleware.go index 7593707..f4d9d63 100644 --- a/rest/middleware.go +++ b/rest/middleware.go @@ -118,7 +118,8 @@ func (c *Context) loggingMiddleware(next http.Handler) http.Handler { start := time.Now() wrappedResponse := &responseWriter{ResponseWriter: w} next.ServeHTTP(wrappedResponse, r) - log.Printf("req=%s res=%d method=%s src=%s duration=%s", r.RequestURI, wrappedResponse.status, r.Method, r.RemoteAddr, time.Since(start)) + // log the path only: query strings can contain secrets (saml code, oidc code & state) + log.Printf("req=%s res=%d method=%s src=%s duration=%s", r.URL.EscapedPath(), wrappedResponse.status, r.Method, r.RemoteAddr, time.Since(start)) }) } diff --git a/rest/passwordchange_test.go b/rest/passwordchange_test.go index 7863f56..3a623f0 100644 --- a/rest/passwordchange_test.go +++ b/rest/passwordchange_test.go @@ -164,7 +164,7 @@ func TestProfilePasswordHandlerStampsPasswordChangedAt(t *testing.T) { t.Fatalf("cannot create user: %s", err) } - payload, err := json.Marshal(users.User{Password: "newpass1!"}) + payload, err := json.Marshal(ProfilePasswordRequest{Password: "newpass1!", CurrentPassword: "mypass"}) if err != nil { t.Fatalf("marshal error: %s", err) } diff --git a/rest/prerelease_test.go b/rest/prerelease_test.go new file mode 100644 index 0000000..1b0a5f4 --- /dev/null +++ b/rest/prerelease_test.go @@ -0,0 +1,362 @@ +package rest + +import ( + "bytes" + "context" + "encoding/json" + "encoding/xml" + "log" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" + "github.com/in4it/go-devops-platform/auth/oidc" + "github.com/in4it/go-devops-platform/auth/provisioning/scim" + "github.com/in4it/go-devops-platform/auth/saml" + "github.com/in4it/go-devops-platform/rest/login" + memorystorage "github.com/in4it/go-devops-platform/storage/memory" + "github.com/in4it/go-devops-platform/users" +) + +// newTestSAMLProvider creates a saml provider in the context, backed by a +// metadata test server +func newTestSAMLProvider(t *testing.T, c *Context) saml.Provider { + t.Helper() + c.Hostname = "example.inv" + c.Protocol = "https" + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.RequestURI == "/metadata" { + out, err := xml.Marshal(getSAMLCert("http://localhost.inv")) + if err != nil { + t.Errorf("marshal error: %s", err) + } + w.Write(out) + return + } + w.WriteHeader(http.StatusBadRequest) + })) + t.Cleanup(ts.Close) + + payload, _ := json.Marshal(saml.Provider{Name: "testProvider", MetadataURL: ts.URL + "/metadata"}) + req := httptest.NewRequest("POST", "http://example.com/api/saml-setup", bytes.NewBuffer(payload)) + w := httptest.NewRecorder() + c.samlSetupHandler(w, req) + var samlProvider saml.Provider + if err := json.NewDecoder(w.Result().Body).Decode(&samlProvider); err != nil || samlProvider.ID == "" { + t.Fatalf("could not create saml provider: %v (status %d)", err, w.Result().StatusCode) + } + return samlProvider +} + +func postSAMLCallback(c *Context, providerID, code string) (int, login.LoginResponse) { + payload, _ := json.Marshal(SAMLCallback{Code: code, RedirectURI: "https://localhost.inv/something"}) + req := httptest.NewRequest("POST", "http://example.com/api/authmethods/saml/"+providerID, bytes.NewBuffer(payload)) + req.SetPathValue("method", "saml") + req.SetPathValue("id", providerID) + w := httptest.NewRecorder() + c.authMethodsByID(w, req) + var loginResponse login.LoginResponse + _ = json.NewDecoder(w.Result().Body).Decode(&loginResponse) + return w.Result().StatusCode, loginResponse +} + +// TestSAMLCallbackCodeSingleUse: the code from the ACS handler can only be +// exchanged for a token once, and the token doesn't outlive local tokens. +func TestSAMLCallbackCodeSingleUse(t *testing.T) { + c := newTestContext(t) + samlProvider := newTestSAMLProvider(t, c) + + c.SAML.Client.CreateSession(saml.SessionKey{ProviderID: samlProvider.ID, SessionID: "abc"}, saml.AuthenticatedUser{ID: "123", Login: "john@example.com", ExpiresAt: time.Now().AddDate(0, 0, 30)}) + status, loginResponse := postSAMLCallback(c, samlProvider.ID, "abc") + if status != http.StatusOK || !loginResponse.Authenticated || loginResponse.Token == "" { + t.Fatalf("expected first callback to authenticate, got %d %+v", status, loginResponse) + } + token, err := jwt.Parse(loginResponse.Token, func(token *jwt.Token) (interface{}, error) { + return c.JWTKeys.PublicKey, nil + }) + if err != nil { + t.Fatalf("token parse error: %s", err) + } + exp, err := token.Claims.GetExpirationTime() + if err != nil || exp == nil { + t.Fatalf("no expiration in token: %v", err) + } + if exp.After(time.Now().Add(login.MAX_TOKEN_LIFETIME + time.Minute)) { + t.Fatalf("saml token lives longer than local tokens: %s", exp) + } + + status, loginResponse = postSAMLCallback(c, samlProvider.ID, "abc") + if status != http.StatusBadRequest || loginResponse.Token != "" { + t.Fatalf("expected second callback with the same code to fail, got %d %+v", status, loginResponse) + } +} + +// TestExternalUserLicenseLimit: SSO logins can't create users beyond the +// license limit. Existing users can still log in. +func TestExternalUserLicenseLimit(t *testing.T) { + storage := &memorystorage.MockMemoryStorage{} + userStore, err := users.NewUserStore(storage, 100) + if err != nil { + t.Fatalf("userstore error: %s", err) + } + // license for 1 user + c, err := newContextWithParams(storage, SERVER_TYPE_VPN, userStore, scim.New(storage, userStore, ""), 1, "", map[string]AppClient{}) + if err != nil { + t.Fatalf("cannot create context: %s", err) + } + c.SetupCompleted = true + samlProvider := newTestSAMLProvider(t, c) + if _, err := c.UserStore.AddUser(users.User{Login: "existing@example.com", SAMLID: "old"}); err != nil { + t.Fatalf("cannot create user: %s", err) + } + + c.SAML.Client.CreateSession(saml.SessionKey{ProviderID: samlProvider.ID, SessionID: "new"}, saml.AuthenticatedUser{ID: "1", Login: "new@example.com", ExpiresAt: time.Now().Add(time.Hour)}) + status, loginResponse := postSAMLCallback(c, samlProvider.ID, "new") + if status != http.StatusOK || !loginResponse.NoLicense || loginResponse.Authenticated || loginResponse.Token != "" { + t.Fatalf("expected noLicense response, got %d %+v", status, loginResponse) + } + if c.UserStore.LoginExists("new@example.com") { + t.Fatalf("user was created beyond the license limit") + } + + c.SAML.Client.CreateSession(saml.SessionKey{ProviderID: samlProvider.ID, SessionID: "existing"}, saml.AuthenticatedUser{ID: "2", Login: "existing@example.com", ExpiresAt: time.Now().Add(time.Hour)}) + status, loginResponse = postSAMLCallback(c, samlProvider.ID, "existing") + if status != http.StatusOK || !loginResponse.Authenticated { + t.Fatalf("expected existing user to log in, got %d %+v", status, loginResponse) + } +} + +// TestExternalUserLinking: an SSO login is not linked to a user that is bound +// to another type of SSO. +func TestExternalUserLinking(t *testing.T) { + c := newTestContext(t) + if _, err := c.UserStore.AddUser(users.User{Login: "saml@example.com", SAMLID: "saml-1"}); err != nil { + t.Fatalf("cannot create user: %s", err) + } + if _, err := c.UserStore.AddUser(users.User{Login: "oidc@example.com", OIDCID: "oidc-1"}); err != nil { + t.Fatalf("cannot create user: %s", err) + } + if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, "saml@example.com", "oidc", "oidc-2"); err == nil { + t.Fatalf("expected oidc login to be refused for saml user") + } + if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, "oidc@example.com", "saml", "saml-2"); err == nil { + t.Fatalf("expected saml login to be refused for oidc user") + } + samlUser, _ := c.UserStore.GetUserByLogin("saml@example.com") + oidcUser, _ := c.UserStore.GetUserByLogin("oidc@example.com") + if samlUser.OIDCID != "" || oidcUser.SAMLID != "" { + t.Fatalf("external ids were overwritten: %+v %+v", samlUser, oidcUser) + } + // same type of login still works + if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, "oidc@example.com", "oidc", "oidc-3"); err != nil { + t.Fatalf("expected oidc login for oidc user: %s", err) + } +} + +// TestOIDCCallbackStateSingleUse: a state that was already exchanged for a +// token can't be used to obtain the token again. +func TestOIDCCallbackStateSingleUse(t *testing.T) { + c := newTestContext(t) + c.OIDCProviders = []oidc.OIDCProvider{{ID: "prov-1", Name: "test", ClientID: "client", ClientSecret: "secret", DiscoveryURI: "http://127.0.0.1:1/discovery.json"}} + if _, err := c.UserStore.AddUser(users.User{Login: "john@example.com", OIDCID: "oidc-1"}); err != nil { + t.Fatalf("cannot create user: %s", err) + } + err := c.OIDCStore.SaveOAuth2Data(oidc.OAuthData{ID: "oidc-1", OIDCProviderID: "prov-1", CreatedAt: time.Now(), Token: oidc.Token{AccessToken: "the-access-token"}}, "used-state") + if err != nil { + t.Fatalf("save error: %s", err) + } + payload, _ := json.Marshal(OIDCCallback{Code: "code", State: "used-state", RedirectURI: "/callback/oidc/prov-1"}) + req := httptest.NewRequest("POST", "http://example.com/api/authmethods/oidc/prov-1", bytes.NewBuffer(payload)) + req.SetPathValue("method", "oidc") + req.SetPathValue("id", "prov-1") + w := httptest.NewRecorder() + c.authMethodsByID(w, req) + body := w.Body.String() + if w.Result().StatusCode != http.StatusBadRequest { + t.Fatalf("expected 400 for reused state, got %d: %s", w.Result().StatusCode, body) + } + if strings.Contains(body, "the-access-token") { + t.Fatalf("access token returned for reused state") + } +} + +// TestLoggingMiddlewareOmitsQuery: codes and states in query strings must not +// end up in the access log. +func TestLoggingMiddlewareOmitsQuery(t *testing.T) { + c := newTestContext(t) + var buf bytes.Buffer + log.SetOutput(&buf) + defer log.SetOutput(os.Stderr) + handler := c.loggingMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + req := httptest.NewRequest("GET", "http://example.com/callback/oidc/prov-1?code=secretcode&state=secretstate", nil) + handler.ServeHTTP(httptest.NewRecorder(), req) + if strings.Contains(buf.String(), "secretcode") || strings.Contains(buf.String(), "secretstate") { + t.Fatalf("query string logged: %s", buf.String()) + } + if !strings.Contains(buf.String(), "req=/callback/oidc/prov-1 ") { + t.Fatalf("path not logged: %s", buf.String()) + } +} + +func userinfoType(t *testing.T, c *Context, user users.User) string { + t.Helper() + req := httptest.NewRequest("GET", "http://example.com/api/userinfo", nil) + req = req.WithContext(context.WithValue(req.Context(), CustomValue("user"), user)) + w := httptest.NewRecorder() + c.userinfoHandler(w, req) + var response UserInfoResponse + if err := json.NewDecoder(w.Result().Body).Decode(&response); err != nil { + t.Fatalf("decode error: %s", err) + } + return response.UserType +} + +func TestUserinfoUserType(t *testing.T) { + c := newTestContext(t) + for expected, user := range map[string]users.User{ + "local": {Login: "local"}, + "oidc": {Login: "oidc", OIDCID: "oidc-1"}, + "saml": {Login: "saml", SAMLID: "saml-1"}, + } { + if userType := userinfoType(t, c, user); userType != expected { + t.Fatalf("expected userType %s, got %s", expected, userType) + } + } +} + +func postProfilePassword(c *Context, user users.User, body string) int { + req := httptest.NewRequest("POST", "http://example.com/api/profile/password", bytes.NewBufferString(body)) + req = req.WithContext(context.WithValue(req.Context(), CustomValue("user"), user)) + w := httptest.NewRecorder() + c.profilePasswordHandler(w, req) + return w.Result().StatusCode +} + +// TestProfilePasswordRequiresCurrentPassword: a local user needs the current +// password to set a new one; SSO users can't set a password. +func TestProfilePasswordRequiresCurrentPassword(t *testing.T) { + c := newTestContext(t) + user, err := c.UserStore.AddUser(users.User{Login: "john", Password: "mypass"}) + if err != nil { + t.Fatalf("cannot create user: %s", err) + } + if status := postProfilePassword(c, user, `{"password": "newpass1!"}`); status != http.StatusBadRequest { + t.Fatalf("expected 400 without current password, got %d", status) + } + if status := postProfilePassword(c, user, `{"password": "newpass1!", "currentPassword": "wrong"}`); status != http.StatusBadRequest { + t.Fatalf("expected 400 with wrong current password, got %d", status) + } + if _, ok := c.UserStore.AuthUser("john", "mypass"); !ok { + t.Fatalf("password changed without valid current password") + } + if status := postProfilePassword(c, user, `{"password": "newpass1!", "currentPassword": "mypass"}`); status != http.StatusOK { + t.Fatalf("expected 200 with current password, got %d", status) + } + if _, ok := c.UserStore.AuthUser("john", "newpass1!"); !ok { + t.Fatalf("password was not changed") + } + + for _, ssoUser := range []users.User{ + {Login: "oidc@example.com", OIDCID: "oidc-1"}, + {Login: "saml@example.com", SAMLID: "saml-1"}, + {Login: "scim@example.com", Provisioned: true}, + } { + added, err := c.UserStore.AddUser(ssoUser) + if err != nil { + t.Fatalf("cannot create user: %s", err) + } + if status := postProfilePassword(c, added, `{"password": "newpass1!", "currentPassword": ""}`); status != http.StatusForbidden { + t.Fatalf("expected 403 for sso user %s, got %d", added.Login, status) + } + } +} + +// TestReloadConfigReloadsUsers: users.json changed by another process (e.g. a +// password reset from the command line) is picked up on SIGHUP. +func TestReloadConfigReloadsUsers(t *testing.T) { + c := newTestContext(t) + hooks := &hookCalls{} + hooks.install(c) + user, err := c.UserStore.AddUser(users.User{Login: "admin", Password: "oldpass", Role: "admin"}) + if err != nil { + t.Fatalf("cannot create user: %s", err) + } + if err := SaveConfig(c); err != nil { + t.Fatalf("save config error: %s", err) + } + // another process resets the password + otherStore, err := users.NewUserStore(c.Storage.Client, 100) + if err != nil { + t.Fatalf("user store error: %s", err) + } + if err := otherStore.UpdatePassword(user.ID, "resetpass"); err != nil { + t.Fatalf("update password error: %s", err) + } + userStore := c.UserStore + + c.ReloadConfig() + + if _, ok := c.UserStore.AuthUser("admin", "resetpass"); !ok { + t.Fatalf("reset password not loaded after reload") + } + if userStore != c.UserStore { + t.Fatalf("user store was replaced instead of reloaded") + } + if c.UserStore.UserHooks.DisableFunc == nil || c.UserStore.UserHooks.ReactivateFunc == nil { + t.Fatalf("user hooks were lost on reload") + } + // the next save must not revert the reset + if err := c.UserStore.SaveUsers(); err != nil { + t.Fatalf("save users error: %s", err) + } + reread, _ := users.NewUserStore(c.Storage.Client, 100) + if _, ok := reread.AuthUser("admin", "resetpass"); !ok { + t.Fatalf("reset password reverted by save") + } +} + +// TestReloadConfigError: a failing reload must not crash the server. +func TestReloadConfigError(t *testing.T) { + c := newTestContext(t) + c.Hostname = "example.inv" + if err := c.Storage.Client.WriteFile(c.Storage.Client.ConfigPath("config.json"), []byte("{invalid")); err != nil { + t.Fatalf("write error: %s", err) + } + c.ReloadConfig() + if c.Hostname != "example.inv" { + t.Fatalf("config changed after failed reload") + } +} + +// TestOIDCProviderClientSecretNotReturned: the client secret is stored, but +// never returned to the browser. +func TestOIDCProviderClientSecretNotReturned(t *testing.T) { + c := newTestContext(t) + payload, _ := json.Marshal(oidc.OIDCProvider{Name: "test", ClientID: "client", ClientSecret: "supersecret", Scope: "openid", DiscoveryURI: "https://idp.inv/.well-known/openid-configuration"}) + req := httptest.NewRequest("POST", "http://example.com/api/oidc", bytes.NewBuffer(payload)) + w := httptest.NewRecorder() + c.oidcProviderHandler(w, req) + if w.Result().StatusCode != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Result().StatusCode, w.Body.String()) + } + if strings.Contains(w.Body.String(), "supersecret") { + t.Fatalf("client secret returned on create: %s", w.Body.String()) + } + if len(c.OIDCProviders) != 1 || c.OIDCProviders[0].ClientSecret != "supersecret" { + t.Fatalf("client secret not stored: %+v", c.OIDCProviders) + } + + req = httptest.NewRequest("GET", "http://example.com/api/oidc", nil) + w = httptest.NewRecorder() + c.oidcProviderHandler(w, req) + if strings.Contains(w.Body.String(), "supersecret") { + t.Fatalf("client secret returned on list: %s", w.Body.String()) + } + if c.OIDCProviders[0].ClientSecret != "supersecret" { + t.Fatalf("listing removed the stored client secret") + } +} diff --git a/rest/profile.go b/rest/profile.go index ff0fa79..735e825 100644 --- a/rest/profile.go +++ b/rest/profile.go @@ -13,7 +13,11 @@ func (c *Context) profilePasswordHandler(w http.ResponseWriter, r *http.Request) user := r.Context().Value(CustomValue("user")).(users.User) switch r.Method { case http.MethodPost: - var userInput users.User + if user.OIDCID != "" || user.SAMLID != "" || user.Provisioned { + c.returnError(w, fmt.Errorf("password can't be changed for users that log in with single sign-on"), http.StatusForbidden) + return + } + var userInput ProfilePasswordRequest decoder := json.NewDecoder(r.Body) err := decoder.Decode(&userInput) if err != nil { @@ -24,6 +28,14 @@ func (c *Context) profilePasswordHandler(w http.ResponseWriter, r *http.Request) c.returnError(w, fmt.Errorf("no password supplied"), http.StatusBadRequest) return } + if userInput.CurrentPassword == "" { + c.returnError(w, fmt.Errorf("current password is required"), http.StatusBadRequest) + return + } + if _, ok := c.UserStore.AuthUser(user.Login, userInput.CurrentPassword); !ok { + c.returnError(w, fmt.Errorf("current password is incorrect"), http.StatusBadRequest) + return + } err = c.UserStore.UpdatePassword(user.ID, userInput.Password) if err != nil { c.returnError(w, fmt.Errorf("update password error: %s", err), http.StatusBadRequest) diff --git a/rest/security_test.go b/rest/security_test.go index efd9c0c..ed9c4be 100644 --- a/rest/security_test.go +++ b/rest/security_test.go @@ -406,7 +406,7 @@ func TestExternalLoginReactivatesConnections(t *testing.T) { if err != nil { t.Fatalf("cannot create user: %s", err) } - if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, "john@example.com", "oidc", "oidc-1"); err != nil { + if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, "john@example.com", "oidc", "oidc-1"); err != nil { t.Fatalf("addOrModifyExternalUser error: %s", err) } updated, err := c.UserStore.GetUserByID(user.ID) diff --git a/rest/signals.go b/rest/signals.go index 625667b..056d95c 100644 --- a/rest/signals.go +++ b/rest/signals.go @@ -7,6 +7,8 @@ import ( "os/signal" "path" "syscall" + + "github.com/in4it/go-devops-platform/users" ) func handleSignals(c *Context) { @@ -29,10 +31,29 @@ func (c *Context) ReloadConfig() { newC, err := NewContext(c.Storage.Client, c.ServerType, c.UserStore, c.SCIM.Client, c.LicenseUserCount, c.CloudType, c.Apps.Clients) if err != nil { log.Printf("ReloadConfig failed: %s\n", err) + return } c.AppDir = newC.AppDir c.Hostname = newC.Hostname c.SetupCompleted = newC.SetupCompleted - c.UserStore = newC.UserStore + err = c.reloadUsers() + if err != nil { + log.Printf("ReloadConfig: could not reload users: %s\n", err) + return + } log.Printf("Config Reloaded!\n") } + +// reloadUsers re-reads the users from storage (e.g. after a password reset from +// the command line). The existing user store is updated in place, so every +// component that has a reference to it (and its hooks) keeps working. +func (c *Context) reloadUsers() error { + reloaded, err := users.NewUserStore(c.Storage.Client, c.UserStore.GetMaxUsers()) + if err != nil { + return fmt.Errorf("user store read error: %s", err) + } + users.UserStoreMu.Lock() + defer users.UserStoreMu.Unlock() + c.UserStore.Users = reloaded.Users + return nil +} diff --git a/rest/types.go b/rest/types.go index 3c3991a..3c757f0 100644 --- a/rest/types.go +++ b/rest/types.go @@ -98,6 +98,11 @@ type SAMLCallback struct { RedirectURI string `json:"redirectURI"` } +type ProfilePasswordRequest struct { + Password string `json:"password"` + CurrentPassword string `json:"currentPassword"` +} + type UserInfoResponse struct { Login string `json:"login"` Role string `json:"role"` diff --git a/rest/users.go b/rest/users.go index 49ee442..532d7db 100644 --- a/rest/users.go +++ b/rest/users.go @@ -2,6 +2,7 @@ package rest import ( "encoding/json" + "errors" "fmt" "net/http" "time" @@ -236,12 +237,23 @@ func (c *Context) userHandler(w http.ResponseWriter, r *http.Request) { } } -func addOrModifyExternalUser(storage storage.Iface, userStore *users.UserStore, login, authType, externalAuthID string) (users.User, error) { +// errNoLicense is returned when a new user can't be added because the user +// license limit has been reached +var errNoLicense = errors.New("no more licenses available") + +func addOrModifyExternalUser(storage storage.Iface, userStore *users.UserStore, licenseUserCount int, login, authType, externalAuthID string) (users.User, error) { if userStore.LoginExists(login) { existingUser, err := userStore.GetUserByLogin(login) if err != nil { return existingUser, fmt.Errorf("couldn't find existing user in database: %s", login) } + // don't link a login to a user that is bound to another type of external auth + if authType == "oidc" && existingUser.SAMLID != "" { + return users.User{}, fmt.Errorf("user %s is linked to a saml login, can't login with oidc", login) + } + if authType == "saml" && existingUser.OIDCID != "" { + return users.User{}, fmt.Errorf("user %s is linked to an oidc login, can't login with saml", login) + } if authType == "oidc" { existingUser.OIDCID = externalAuthID @@ -267,6 +279,9 @@ func addOrModifyExternalUser(storage storage.Iface, userStore *users.UserStore, } return existingUser, nil } else { + if userStore.UserCount() >= licenseUserCount { + return users.User{}, errNoLicense + } newUser := users.User{ Login: login, Role: "user", @@ -295,10 +310,13 @@ func (c *Context) userinfoHandler(w http.ResponseWriter, r *http.Request) { response.Login = user.Login response.Role = user.Role - if user.OIDCID == "" { - response.UserType = "local" - } else { + switch { + case user.SAMLID != "": + response.UserType = "saml" + case user.OIDCID != "": response.UserType = "oidc" + default: + response.UserType = "local" } out, err := json.Marshal(response) diff --git a/storage/local/fullpath.go b/storage/local/fullpath.go new file mode 100644 index 0000000..757ee44 --- /dev/null +++ b/storage/local/fullpath.go @@ -0,0 +1,37 @@ +package localstorage + +import ( + "fmt" + "path/filepath" + "strings" +) + +// fullPath returns the cleaned, absolute (storage path prefixed) path for name, and returns an error +// when the result would be outside of the storage path (e.g. by using ../). +// Names are relative to the storage path. For backwards compatibility, a name that is an absolute path which +// already starts with the storage path is also accepted. +func (l *LocalStorage) fullPath(name string) (string, error) { + base := filepath.Clean(l.path) + var full string + if filepath.IsAbs(name) && isSubPath(base, filepath.Clean(name)) { + full = filepath.Clean(name) + } else { + full = filepath.Join(base, name) + } + if !isSubPath(base, full) { + return "", fmt.Errorf("path %s is outside of the storage path", name) + } + return full, nil +} + +// isSubPath returns true if p equals base or is located under base. Both paths need to be cleaned. +func isSubPath(base, p string) bool { + if p == base { + return true + } + rel, err := filepath.Rel(base, p) + if err != nil { + return false + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) && !filepath.IsAbs(rel) +} diff --git a/storage/local/fullpath_test.go b/storage/local/fullpath_test.go new file mode 100644 index 0000000..6f555f7 --- /dev/null +++ b/storage/local/fullpath_test.go @@ -0,0 +1,195 @@ +package localstorage + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func newTestStorage(t *testing.T) (*LocalStorage, string) { + t.Helper() + root := t.TempDir() + dataDir := filepath.Join(root, "data") + if err := os.Mkdir(dataDir, 0700); err != nil { + t.Fatalf("mkdir error: %s", err) + } + // a file outside of the storage path, that must not be reachable + if err := os.WriteFile(filepath.Join(root, "secret.txt"), []byte("secret"), 0600); err != nil { + t.Fatalf("write error: %s", err) + } + l, err := NewWithPath(dataDir) + if err != nil { + t.Fatalf("NewWithPath error: %s", err) + } + return l, root +} + +func TestFullPath(t *testing.T) { + l := &LocalStorage{path: "/data/app"} + valid := map[string]string{ + "config/users.json": "/data/app/config/users.json", + "./config/../config/users.json": "/data/app/config/users.json", + "": "/data/app", + "/data/app/config/users.json": "/data/app/config/users.json", // already prefixed + "/config/users.json": "/data/app/config/users.json", // absolute names were always relative to the storage path + } + for name, expected := range valid { + res, err := l.fullPath(name) + if err != nil { + t.Fatalf("%s: unexpected error: %s", name, err) + } + if res != expected { + t.Fatalf("%s: expected %s, got %s", name, expected, res) + } + } + invalid := []string{ + "../secret.txt", + "config/../../secret.txt", + "..", + "/data/application/x", + } + for _, name := range invalid { + res, err := l.fullPath(name) + if name == "/data/application/x" { // not prefixed with the storage dir, so it's treated as relative + if err != nil || res != "/data/app/data/application/x" { + t.Fatalf("%s: unexpected result %s (err: %v)", name, res, err) + } + continue + } + if err == nil { + t.Fatalf("%s: expected error, got %s", name, res) + } + } +} + +func TestPathTraversalRejected(t *testing.T) { + l, root := newTestStorage(t) + escape := "../secret.txt" + + if _, err := l.ReadFile(escape); err == nil { + t.Fatalf("ReadFile: expected error") + } + if _, err := l.OpenFile(escape); err == nil { + t.Fatalf("OpenFile: expected error") + } + if _, err := l.OpenFilesFromPos([]string{escape}, 0); err == nil { + t.Fatalf("OpenFilesFromPos: expected error") + } + if l.FileExists(escape) { + t.Fatalf("FileExists: expected false") + } + if _, err := l.FileInfo(escape); err == nil { + t.Fatalf("FileInfo: expected error") + } + if _, err := l.ReadDir(".."); err == nil { + t.Fatalf("ReadDir: expected error") + } + if err := l.WriteFile(escape, []byte("overwritten")); err == nil { + t.Fatalf("WriteFile: expected error") + } + if err := l.AppendFile(escape, []byte("appended")); err == nil { + t.Fatalf("AppendFile: expected error") + } + if _, err := l.OpenFileForWriting(escape); err == nil { + t.Fatalf("OpenFileForWriting: expected error") + } + if _, err := l.OpenFileForAppending(escape); err == nil { + t.Fatalf("OpenFileForAppending: expected error") + } + if err := l.EnsurePermissions(escape, 0644); err == nil { + t.Fatalf("EnsurePermissions: expected error") + } + if err := l.EnsureOwnership(escape, "vpn"); err == nil { + t.Fatalf("EnsureOwnership: expected error") + } + if err := l.EnsurePath("../newdir"); err == nil { + t.Fatalf("EnsurePath: expected error") + } + if err := l.Rename(escape, "stolen.txt"); err == nil { + t.Fatalf("Rename: expected error") + } + if err := l.WriteFile("x.txt", []byte("x")); err != nil { + t.Fatalf("WriteFile error: %s", err) + } + if err := l.Rename("x.txt", "../x.txt"); err == nil { + t.Fatalf("Rename: expected error") + } + if err := l.Remove(escape); err == nil { + t.Fatalf("Remove: expected error") + } + + data, err := os.ReadFile(filepath.Join(root, "secret.txt")) + if err != nil { + t.Fatalf("secret file should still exist: %s", err) + } + if string(data) != "secret" { + t.Fatalf("secret file was modified: %s", data) + } + if _, err := os.Stat(filepath.Join(root, "newdir")); err == nil { + t.Fatalf("directory outside of storage path was created") + } + if _, err := os.Stat(filepath.Join(root, "x.txt")); err == nil { + t.Fatalf("file was moved outside of storage path") + } +} + +func TestRelativeAndPrefixedPaths(t *testing.T) { + l, _ := newTestStorage(t) + if err := l.EnsurePath(CONFIG_PATH); err != nil { + t.Fatalf("EnsurePath error: %s", err) + } + relative := l.ConfigPath("test.json") + prefixed := filepath.Join(l.GetPath(), relative) + + if err := l.WriteFile(relative, []byte("one")); err != nil { + t.Fatalf("WriteFile error: %s", err) + } + for _, name := range []string{relative, prefixed} { + data, err := l.ReadFile(name) + if err != nil { + t.Fatalf("ReadFile(%s) error: %s", name, err) + } + if string(data) != "one" { + t.Fatalf("ReadFile(%s): unexpected data: %s", name, data) + } + if !l.FileExists(name) { + t.Fatalf("FileExists(%s): expected true", name) + } + } + if err := l.AppendFile(prefixed, []byte("two")); err != nil { + t.Fatalf("AppendFile error: %s", err) + } + data, err := os.ReadFile(prefixed) + if err != nil { + t.Fatalf("read error: %s", err) + } + if string(data) != "onetwo" { + t.Fatalf("unexpected data: %s", data) + } + files, err := l.ReadDir(CONFIG_PATH) + if err != nil { + t.Fatalf("ReadDir error: %s", err) + } + if len(files) != 1 || files[0] != "test.json" { + t.Fatalf("unexpected dir contents: %s", strings.Join(files, ",")) + } +} + +func TestFileInfoRelativeToStoragePath(t *testing.T) { + l, _ := newTestStorage(t) + if err := l.WriteFile("info.txt", []byte("12345")); err != nil { + t.Fatalf("WriteFile error: %s", err) + } + // make sure the file doesn't exist relative to the working directory + if _, err := os.Stat("info.txt"); err == nil { + t.Skip("info.txt exists in working directory") + } + info, err := l.FileInfo("info.txt") + if err != nil { + t.Fatalf("FileInfo error: %s", err) + } + if info.Size() != 5 { + t.Fatalf("unexpected size: %d", info.Size()) + } +} diff --git a/storage/local/ownership_other.go b/storage/local/ownership_other.go new file mode 100644 index 0000000..8b6bad7 --- /dev/null +++ b/storage/local/ownership_other.go @@ -0,0 +1,12 @@ +//go:build !unix + +package localstorage + +import ( + "io/fs" + "os" +) + +func copyOwnership(file *os.File, existing fs.FileInfo) error { + return nil +} diff --git a/storage/local/ownership_unix.go b/storage/local/ownership_unix.go new file mode 100644 index 0000000..ff743df --- /dev/null +++ b/storage/local/ownership_unix.go @@ -0,0 +1,21 @@ +//go:build unix + +package localstorage + +import ( + "io/fs" + "os" + "syscall" +) + +// copyOwnership gives file the same owner and group as existing +func copyOwnership(file *os.File, existing fs.FileInfo) error { + stat, ok := existing.Sys().(*syscall.Stat_t) + if !ok { + return nil + } + if int(stat.Uid) == os.Geteuid() && int(stat.Gid) == os.Getegid() { + return nil // newly created file already has this owner/group + } + return file.Chown(int(stat.Uid), int(stat.Gid)) +} diff --git a/storage/local/path.go b/storage/local/path.go index e9b761b..9e6571c 100644 --- a/storage/local/path.go +++ b/storage/local/path.go @@ -13,7 +13,11 @@ import ( ) func (l *LocalStorage) FileExists(filename string) bool { - if _, err := os.Stat(path.Join(l.path, filename)); errors.Is(err, os.ErrNotExist) { + fullPath, err := l.fullPath(filename) + if err != nil { + return false + } + if _, err := os.Stat(fullPath); errors.Is(err, os.ErrNotExist) { return false } return true @@ -28,7 +32,10 @@ func (l *LocalStorage) GetPath() string { } func (l *LocalStorage) EnsurePath(pathname string) error { - fullPathname := path.Join(l.path, pathname) + fullPathname, err := l.fullPath(pathname) + if err != nil { + return err + } if _, err := os.Stat(fullPathname); errors.Is(err, os.ErrNotExist) { err := os.Mkdir(fullPathname, 0700) if err != nil { @@ -39,6 +46,10 @@ func (l *LocalStorage) EnsurePath(pathname string) error { } func (l *LocalStorage) EnsureOwnership(filename, login string) error { + fullPath, err := l.fullPath(filename) + if err != nil { + return err + } currentUser, err := user.Current() if err != nil { return fmt.Errorf("could not get current user: %s", err) @@ -59,8 +70,7 @@ func (l *LocalStorage) EnsureOwnership(filename, login string) error { if err != nil { return fmt.Errorf("user lookup error (gid): %s", err) } - - err = os.Chown(path.Join(l.path, filename), vpnUserUid, vpnUserGid) + err = os.Chown(fullPath, vpnUserUid, vpnUserGid) if err != nil { return fmt.Errorf("vpn chown error: %s", err) } @@ -68,7 +78,11 @@ func (l *LocalStorage) EnsureOwnership(filename, login string) error { } func (l *LocalStorage) ReadDir(pathname string) ([]string, error) { - res, err := os.ReadDir(path.Join(l.path, pathname)) + fullPath, err := l.fullPath(pathname) + if err != nil { + return []string{}, err + } + res, err := os.ReadDir(fullPath) if err != nil { return []string{}, err } @@ -80,17 +94,37 @@ func (l *LocalStorage) ReadDir(pathname string) ([]string, error) { } func (l *LocalStorage) Remove(name string) error { - return os.Remove(path.Join(l.path, name)) + fullPath, err := l.fullPath(name) + if err != nil { + return err + } + return os.Remove(fullPath) } func (l *LocalStorage) Rename(oldName, newName string) error { - return os.Rename(path.Join(l.path, oldName), path.Join(l.path, newName)) + oldPath, err := l.fullPath(oldName) + if err != nil { + return err + } + newPath, err := l.fullPath(newName) + if err != nil { + return err + } + return os.Rename(oldPath, newPath) } func (l *LocalStorage) EnsurePermissions(name string, mode fs.FileMode) error { - return os.Chmod(path.Join(l.path, name), mode) + fullPath, err := l.fullPath(name) + if err != nil { + return err + } + return os.Chmod(fullPath, mode) } func (l *LocalStorage) FileInfo(name string) (fs.FileInfo, error) { - return os.Stat(name) + fullPath, err := l.fullPath(name) + if err != nil { + return nil, err + } + return os.Stat(fullPath) } diff --git a/storage/local/read.go b/storage/local/read.go index 3a2e6ce..fa874cf 100644 --- a/storage/local/read.go +++ b/storage/local/read.go @@ -4,11 +4,14 @@ import ( "fmt" "io" "os" - "path" ) func (l *LocalStorage) ReadFile(name string) ([]byte, error) { - return os.ReadFile(path.Join(l.path, name)) + fullPath, err := l.fullPath(name) + if err != nil { + return nil, err + } + return os.ReadFile(fullPath) } func (l *LocalStorage) OpenFilesFromPos(names []string, pos int64) ([]io.ReadCloser, error) { @@ -17,7 +20,11 @@ func (l *LocalStorage) OpenFilesFromPos(names []string, pos int64) ([]io.ReadClo return readers, nil } for _, name := range names { - file, err := os.Open(path.Join(l.path, name)) + fullPath, err := l.fullPath(name) + if err != nil { + return nil, err + } + file, err := os.Open(fullPath) if err != nil { return nil, fmt.Errorf("cannot open file (%s): %s", name, err) } @@ -40,7 +47,11 @@ func (l *LocalStorage) OpenFilesFromPos(names []string, pos int64) ([]io.ReadClo } func (l *LocalStorage) OpenFile(name string) (io.ReadCloser, error) { - file, err := os.Open(path.Join(l.path, name)) + fullPath, err := l.fullPath(name) + if err != nil { + return nil, err + } + file, err := os.Open(fullPath) if err != nil { return nil, fmt.Errorf("cannot open file (%s): %s", name, err) } diff --git a/storage/local/write.go b/storage/local/write.go index 5eb3d19..79d024a 100644 --- a/storage/local/write.go +++ b/storage/local/write.go @@ -4,28 +4,104 @@ import ( "fmt" "io" "os" - "path" + "path/filepath" ) +// WriteFile writes the data atomically: the data is written to a temporary file in the same directory, +// which is then renamed to name. A crash or a full disk can't leave a truncated file behind. +// New files are created with mode 0600. When the file already exists, its mode and ownership are kept +// (like os.WriteFile does). If that isn't possible (e.g. the directory isn't writable or the ownership +// can't be copied), the file is written in place. func (l *LocalStorage) WriteFile(name string, data []byte) error { - return os.WriteFile(path.Join(l.path, name), data, 0600) + fullPath, err := l.fullPath(name) + if err != nil { + return err + } + return writeFileAtomic(fullPath, data, 0600) +} + +func writeFileAtomic(filename string, data []byte, perm os.FileMode) error { + existing, err := os.Lstat(filename) + if err != nil && !os.IsNotExist(err) { + return err + } + if existing != nil && !existing.Mode().IsRegular() { // e.g. a symlink: write through it, like os.WriteFile + return os.WriteFile(filename, data, perm) + } + tmpFile, err := os.CreateTemp(filepath.Dir(filename), "."+filepath.Base(filename)+".tmp-*") + if err != nil { // directory not writable: fall back to writing in place + return os.WriteFile(filename, data, perm) + } + tmpName := tmpFile.Name() + renamed := false + defer func() { + if !renamed { + tmpFile.Close() + os.Remove(tmpName) + } + }() + + mode := perm + if existing != nil { + mode = existing.Mode() & (os.ModePerm | os.ModeSetuid | os.ModeSetgid | os.ModeSticky) + if err := copyOwnership(tmpFile, existing); err != nil { // can't keep ownership: write in place + return os.WriteFile(filename, data, perm) + } + } + if err := tmpFile.Chmod(mode); err != nil { + return fmt.Errorf("chmod error: %s", err) + } + if _, err := tmpFile.Write(data); err != nil { + return err + } + if err := tmpFile.Sync(); err != nil { + return err + } + if err := tmpFile.Close(); err != nil { + return err + } + if err := os.Rename(tmpName, filename); err != nil { + return err + } + renamed = true + syncDir(filepath.Dir(filename)) + return nil +} + +// syncDir makes sure the rename is persisted (best effort) +func syncDir(dir string) { + d, err := os.Open(dir) + if err != nil { + return + } + defer d.Close() + _ = d.Sync() } func (l *LocalStorage) AppendFile(name string, data []byte) error { - f, err := os.OpenFile(path.Join(l.path, name), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0660) + fullPath, err := l.fullPath(name) + if err != nil { + return err + } + f, err := os.OpenFile(fullPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0660) if err != nil { return err } + defer f.Close() + if _, err := f.Write(data); err != nil { return err } - return nil } func (l *LocalStorage) OpenFileForWriting(name string) (io.WriteCloser, error) { - file, err := os.Create(path.Join(l.path, name)) + fullPath, err := l.fullPath(name) + if err != nil { + return nil, err + } + file, err := os.Create(fullPath) if err != nil { return nil, fmt.Errorf("cannot open file (%s): %s", name, err) } @@ -33,7 +109,11 @@ func (l *LocalStorage) OpenFileForWriting(name string) (io.WriteCloser, error) { } func (l *LocalStorage) OpenFileForAppending(name string) (io.WriteCloser, error) { - file, err := os.OpenFile(path.Join(l.path, name), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0660) + fullPath, err := l.fullPath(name) + if err != nil { + return nil, err + } + file, err := os.OpenFile(fullPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0660) if err != nil { return nil, err } diff --git a/storage/local/write_test.go b/storage/local/write_test.go new file mode 100644 index 0000000..e169cc1 --- /dev/null +++ b/storage/local/write_test.go @@ -0,0 +1,123 @@ +package localstorage + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestWriteFileNewFileMode(t *testing.T) { + l, _ := newTestStorage(t) + if err := l.WriteFile("new.json", []byte(`{"a":1}`)); err != nil { + t.Fatalf("WriteFile error: %s", err) + } + info, err := os.Stat(filepath.Join(l.GetPath(), "new.json")) + if err != nil { + t.Fatalf("stat error: %s", err) + } + if info.Mode().Perm() != 0600 { + t.Fatalf("expected mode 0600, got %o", info.Mode().Perm()) + } +} + +func TestWriteFileIsAtomic(t *testing.T) { + l, _ := newTestStorage(t) + filename := filepath.Join(l.GetPath(), "users.json") + if err := l.WriteFile("users.json", []byte(`["old"]`)); err != nil { + t.Fatalf("WriteFile error: %s", err) + } + // keep a handle to the old file: an in-place (truncating) write would modify its contents, + // an atomic write (rename of a new file) keeps the old inode intact + oldFile, err := os.Open(filename) + if err != nil { + t.Fatalf("open error: %s", err) + } + defer oldFile.Close() + oldInfo, err := oldFile.Stat() + if err != nil { + t.Fatalf("stat error: %s", err) + } + + if err := l.WriteFile("users.json", []byte(`["new"]`)); err != nil { + t.Fatalf("WriteFile error: %s", err) + } + newInfo, err := os.Stat(filename) + if err != nil { + t.Fatalf("stat error: %s", err) + } + if os.SameFile(oldInfo, newInfo) { + t.Fatalf("expected file to be replaced (written to a temp file and renamed), not written in place") + } + oldData := make([]byte, 64) + n, _ := oldFile.ReadAt(oldData, 0) + if string(oldData[:n]) != `["old"]` { + t.Fatalf("old file contents were modified in place: %s", oldData[:n]) + } + data, err := l.ReadFile("users.json") + if err != nil { + t.Fatalf("ReadFile error: %s", err) + } + if string(data) != `["new"]` { + t.Fatalf("unexpected contents: %s", data) + } + // no temp files left behind + files, err := l.ReadDir("") + if err != nil { + t.Fatalf("ReadDir error: %s", err) + } + for _, file := range files { + if strings.Contains(file, ".tmp-") { + t.Fatalf("temp file left behind: %s", file) + } + } +} + +func TestWriteFileKeepsExistingMode(t *testing.T) { + l, _ := newTestStorage(t) + if err := l.WriteFile("client.conf", []byte("a")); err != nil { + t.Fatalf("WriteFile error: %s", err) + } + if err := l.EnsurePermissions("client.conf", 0640); err != nil { + t.Fatalf("EnsurePermissions error: %s", err) + } + if err := l.WriteFile("client.conf", []byte("b")); err != nil { + t.Fatalf("WriteFile error: %s", err) + } + info, err := l.FileInfo("client.conf") + if err != nil { + t.Fatalf("FileInfo error: %s", err) + } + if info.Mode().Perm() != 0640 { + t.Fatalf("expected existing mode 0640 to be kept, got %o", info.Mode().Perm()) + } +} + +func TestWriteFileReadOnlyDirFallback(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("permissions are not enforced for root") + } + l, _ := newTestStorage(t) + if err := l.EnsurePath("ro"); err != nil { + t.Fatalf("EnsurePath error: %s", err) + } + if err := l.WriteFile("ro/file.txt", []byte("a")); err != nil { + t.Fatalf("WriteFile error: %s", err) + } + roDir := filepath.Join(l.GetPath(), "ro") + if err := os.Chmod(roDir, 0500); err != nil { + t.Fatalf("chmod error: %s", err) + } + t.Cleanup(func() { os.Chmod(roDir, 0700) }) + // the directory isn't writable, but the file is: like os.WriteFile, writing should still work + if err := l.WriteFile("ro/file.txt", []byte("b")); err != nil { + t.Fatalf("WriteFile error: %s", err) + } + data, err := l.ReadFile("ro/file.txt") + if err != nil { + t.Fatalf("ReadFile error: %s", err) + } + if string(data) != "b" { + t.Fatalf("unexpected contents: %s", data) + } +} diff --git a/users/localusers.go b/users/localusers.go index b1c6430..3422172 100644 --- a/users/localusers.go +++ b/users/localusers.go @@ -2,6 +2,7 @@ package users import ( "fmt" + "sync" "time" "github.com/google/uuid" @@ -43,11 +44,13 @@ func (u *UserStore) AddUsers(users []User) ([]User, error) { } } users[k].ID = uuid.NewString() - hashedPassword, err := HashPassword(users[k].Password) - if err != nil { - return createdUsers, fmt.Errorf("HashPassword error: %s", err) + if users[k].Password != "" { + hashedPassword, err := HashPassword(users[k].Password) + if err != nil { + return createdUsers, fmt.Errorf("HashPassword error: %s", err) + } + users[k].Password = hashedPassword } - users[k].Password = hashedPassword u.Users = append(u.Users, users[k]) existingUsers = append(existingUsers, users[k]) createdUsers = append(createdUsers, users[k]) @@ -103,16 +106,56 @@ func (u *UserStore) DeleteUserByID(id string) error { return fmt.Errorf("User not found") } +// compareHashAndPassword can be replaced in tests +var compareHashAndPassword = bcrypt.CompareHashAndPassword + +var ( + dummyHashOnce sync.Once + dummyHash []byte +) + +// getDummyHash returns a bcrypt hash (at the current cost) that is used to compare +// against when the login doesn't exist, so that the response time doesn't reveal +// whether a user exists. +func getDummyHash() []byte { + dummyHashOnce.Do(func() { + hash, err := bcrypt.GenerateFromPassword([]byte(uuid.NewString()), passwordHashCost) + if err != nil { + panic(fmt.Sprintf("could not generate dummy bcrypt hash: %s", err)) + } + dummyHash = hash + }) + return dummyHash +} + func (u *UserStore) AuthUser(login, password string) (User, bool) { - for _, user := range u.Users { + userIndex := -1 + for k, user := range u.Users { if user.Login == login { - passwordMatch := bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(password)) - if passwordMatch == nil { - return user, true + userIndex = k + break + } + } + if userIndex == -1 || u.Users[userIndex].Password == "" { + // always run bcrypt, so that timing is similar for unknown users / users without password + compareHashAndPassword(getDummyHash(), []byte(password)) + return User{}, false + } + user := u.Users[userIndex] + if compareHashAndPassword([]byte(user.Password), []byte(password)) != nil { + return User{}, false + } + // upgrade hashes created with a lower cost + if cost, err := bcrypt.Cost([]byte(user.Password)); err == nil && cost < passwordHashCost { + if hashedPassword, err := HashPassword(password); err == nil { + u.Users[userIndex].Password = hashedPassword + user.Password = hashedPassword + if u.autoSave { + _ = u.SaveUsers() // login still succeeds if the upgraded hash can't be saved } } } - return User{}, false + return user, true } func (u *UserStore) LoginExists(login string) bool { @@ -138,6 +181,32 @@ func (u *UserStore) UpdateUser(user User) error { } return fmt.Errorf("user not found in database: %s", user.Login) } + +// UpdateUserByID updates the user with the given ID (the login can be changed). The password is kept. +func (u *UserStore) UpdateUserByID(user User) error { + if user.ID == "" { + return fmt.Errorf("user id cannot be empty") + } + if user.Login == "" { + return fmt.Errorf("login cannot be empty") + } + for _, existingUser := range u.Users { + if existingUser.ID != user.ID && existingUser.Login == user.Login { + return fmt.Errorf("user with login '%s' already exists", user.Login) + } + } + for k, existingUser := range u.Users { + if existingUser.ID == user.ID { + user.Password = existingUser.Password // we keep the password + u.Users[k] = user + if u.autoSave { + return u.SaveUsers() + } + return nil + } + } + return fmt.Errorf("user not found in database: userID %s", user.ID) +} func (u *UserStore) UpdatePassword(userID string, password string) error { for k, existingUser := range u.Users { if existingUser.ID == userID { @@ -159,8 +228,10 @@ func (u *UserStore) UpdatePassword(userID string, password string) error { return fmt.Errorf("user not found in database: userID %s", userID) } +const passwordHashCost = bcrypt.DefaultCost + func HashPassword(password string) (string, error) { - adminPasswordHashed, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.MinCost) + adminPasswordHashed, err := bcrypt.GenerateFromPassword([]byte(password), passwordHashCost) if err != nil { return "", fmt.Errorf("unable to set password: %s", err) } diff --git a/users/localusers_test.go b/users/localusers_test.go new file mode 100644 index 0000000..c6b6e39 --- /dev/null +++ b/users/localusers_test.go @@ -0,0 +1,105 @@ +package users + +import ( + "testing" + + memorystorage "github.com/in4it/go-devops-platform/storage/memory" + "golang.org/x/crypto/bcrypt" +) + +func TestHashPasswordUsesDefaultCost(t *testing.T) { + hash, err := HashPassword("secret") + if err != nil { + t.Fatalf("HashPassword error: %s", err) + } + cost, err := bcrypt.Cost([]byte(hash)) + if err != nil { + t.Fatalf("bcrypt cost error: %s", err) + } + if cost < bcrypt.DefaultCost { + t.Fatalf("expected cost >= %d, got %d", bcrypt.DefaultCost, cost) + } +} + +func TestAuthUserLegacyMinCostHashAndRehash(t *testing.T) { + storage := &memorystorage.MockMemoryStorage{} + store, err := NewUserStore(storage, 99) + if err != nil { + t.Fatalf("new store error: %s", err) + } + legacyHash, err := bcrypt.GenerateFromPassword([]byte("secret"), bcrypt.MinCost) + if err != nil { + t.Fatalf("hash error: %s", err) + } + store.Users = append(store.Users, User{ID: "1", Login: "legacy", Password: string(legacyHash)}) + + if _, ok := store.AuthUser("legacy", "wrong"); ok { + t.Fatalf("expected wrong password to fail") + } + if store.Users[0].Password != string(legacyHash) { + t.Fatalf("hash should not be changed after a failed login") + } + if _, ok := store.AuthUser("legacy", "secret"); !ok { + t.Fatalf("expected legacy MinCost hash to verify") + } + cost, err := bcrypt.Cost([]byte(store.Users[0].Password)) + if err != nil { + t.Fatalf("bcrypt cost error: %s", err) + } + if cost != bcrypt.DefaultCost { + t.Fatalf("expected hash to be upgraded to cost %d, got %d", bcrypt.DefaultCost, cost) + } + // upgraded hash is persisted + reloaded, err := NewUserStore(storage, 99) + if err != nil { + t.Fatalf("new store error: %s", err) + } + if _, ok := reloaded.AuthUser("legacy", "secret"); !ok { + t.Fatalf("expected login to work with persisted upgraded hash") + } + cost, err = bcrypt.Cost([]byte(reloaded.Users[0].Password)) + if err != nil { + t.Fatalf("bcrypt cost error: %s", err) + } + if cost != bcrypt.DefaultCost { + t.Fatalf("expected persisted hash with cost %d, got %d", bcrypt.DefaultCost, cost) + } +} + +func TestAuthUserUnknownUserRunsBcrypt(t *testing.T) { + calls := 0 + orig := compareHashAndPassword + compareHashAndPassword = func(hash, password []byte) error { + calls++ + return orig(hash, password) + } + defer func() { compareHashAndPassword = orig }() + + store, err := NewUserStore(&memorystorage.MockMemoryStorage{}, 99) + if err != nil { + t.Fatalf("new store error: %s", err) + } + if _, err := store.AddUser(User{Login: "john", Password: "secret"}); err != nil { + t.Fatalf("add user error: %s", err) + } + if _, err := store.AddUser(User{Login: "oidcuser"}); err != nil { + t.Fatalf("add user error: %s", err) + } + + for _, login := range []string{"unknown", "oidcuser"} { + calls = 0 + if _, ok := store.AuthUser(login, "secret"); ok { + t.Fatalf("expected auth to fail for %s", login) + } + if calls != 1 { + t.Fatalf("expected bcrypt compare to run once for %s, got %d", login, calls) + } + } + calls = 0 + if _, ok := store.AuthUser("john", "secret"); !ok { + t.Fatalf("expected auth to succeed") + } + if calls != 1 { + t.Fatalf("expected bcrypt compare to run once, got %d", calls) + } +}