diff --git a/docs/documentation/configuration/overview.md b/docs/documentation/configuration/overview.md index 717831a..5699e7d 100644 --- a/docs/documentation/configuration/overview.md +++ b/docs/documentation/configuration/overview.md @@ -827,6 +827,14 @@ Below are the properties for each LDAP provider entry inside `auth.ldap`: #### `disable_missing` - **Default:** `false` - **Description:** If `true`, any user **not** found in LDAP (during sync) is disabled in WireGuard Portal. +- **Important**: As a safeguard, a synchronization that returns no usable user identifiers at all disables + nobody. LDAP cannot distinguish a directory that legitimately holds no matching users from a wrong `base_dn`, + a `sync_filter` whose group was renamed, a replica that is reachable but not yet populated, or a bind account + that has lost read access — all of them answer with success and zero entries. Disabling every user on that + signal would lock administrators out of a portal they may only reach over the VPN, so the sync logs an error + and skips the disable step instead. + If you intend to empty a group, keep one account matching `sync_filter`: the remaining users are then disabled + normally, and the account doubles as a canary, since its disappearance means the query itself is broken. #### `auto_re_enable` - **Default:** `false` diff --git a/internal/app/users/ldap_sync.go b/internal/app/users/ldap_sync.go index 56cc299..7e77380 100644 --- a/internal/app/users/ldap_sync.go +++ b/internal/app/users/ldap_sync.go @@ -193,6 +193,42 @@ func (m Manager) disableMissingLdapUsers( rawUsers []internal.RawLdapUser, fields *config.LdapFields, ) error { + // Collect the identifiers the directory actually returned. A hard LDAP + // failure is already handled by the caller, but a search that *succeeds* + // and yields nothing usable is indistinguishable here from "every user was + // removed from the directory". In practice that means a wrong base DN, a + // sync_filter that no longer matches, a replica that is up but not yet + // populated, a field map pointing at an attribute the server does not + // return, or a bind account that has lost read access to the user subtree. + // LDAP offers nothing to tell those apart: a search that matches nobody and + // one the client is not allowed to answer both come back as success with + // zero entries, and a directory server may reply that way for a base DN + // that does not exist rather than disclose it to an unprivileged client. + // + // Acting on it disables every LDAP-sourced user at once, and through + // TopicUserDisabled that removes each of their peers from the WireGuard + // device rather than only flagging them. The search succeeded, so no error + // is logged, and every message on this path is Debug: at the default log + // level the whole thing is silent. It also repeats every sync interval. + // Refuse instead. The cost is that a directory intentionally emptied of all + // users disables nobody; leaving a single account in scope restores the + // normal behaviour for everyone else. + ldapUserIds := make(map[domain.UserIdentifier]struct{}, len(rawUsers)) + for _, rawUser := range rawUsers { + if userId := ldapUserIdentifier(rawUser, fields.UserIdentifier); userId != "" { + ldapUserIds[userId] = struct{}{} + } + } + if len(ldapUserIds) == 0 { + slog.Error("refusing to disable missing LDAP users: directory returned no usable user identifiers", + "provider", providerName, + "raw-entries", len(rawUsers), + "identifier-field", fields.UserIdentifier, + "hint", "check base_dn, sync_filter and the bind account's read permissions; "+ + "if the directory is intentionally empty, keep one account matching sync_filter") + return nil + } + allUsers, err := m.users.GetAllUsers(ctx) if err != nil { return err @@ -215,20 +251,14 @@ func (m Manager) disableMissingLdapUsers( continue // skip sync for this user } - existsInLDAP := false - for _, rawUser := range rawUsers { - userId := ldapUserIdentifier(rawUser, fields.UserIdentifier) - if user.Identifier == userId { - existsInLDAP = true - break - } - } - - if existsInLDAP { + if _, existsInLDAP := ldapUserIds[user.Identifier]; existsInLDAP { continue } - slog.Debug("user is missing in ldap provider, disabling", "user", user.Identifier, "provider", providerName) + // Warn, not Debug: this removes the user's peers from the device, and at + // the default log level a Debug line would make a mass disable silent. + slog.Warn("user is missing in ldap provider, disabling", + "user", user.Identifier, "provider", providerName) now := time.Now() user.Disabled = &now diff --git a/internal/app/users/ldap_sync_disable_missing_test.go b/internal/app/users/ldap_sync_disable_missing_test.go new file mode 100644 index 0000000..adeb61d --- /dev/null +++ b/internal/app/users/ldap_sync_disable_missing_test.go @@ -0,0 +1,159 @@ +package users + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/h44z/wg-portal/internal" + "github.com/h44z/wg-portal/internal/config" + "github.com/h44z/wg-portal/internal/domain" +) + +// fakeUserRepo is a minimal UserDatabaseRepo that records which users were +// saved, so a test can assert whether the disable path ran at all. +type fakeUserRepo struct { + users []domain.User + saved map[domain.UserIdentifier]*domain.User +} + +func (f *fakeUserRepo) GetAllUsers(_ context.Context) ([]domain.User, error) { + return f.users, nil +} + +func (f *fakeUserRepo) SaveUser( + _ context.Context, + id domain.UserIdentifier, + updateFunc func(u *domain.User) (*domain.User, error), +) error { + existing := &domain.User{Identifier: id} + updated, err := updateFunc(existing) + if err != nil { + return err + } + if f.saved == nil { + f.saved = make(map[domain.UserIdentifier]*domain.User) + } + f.saved[id] = updated + return nil +} + +func (f *fakeUserRepo) GetUser(_ context.Context, _ domain.UserIdentifier) (*domain.User, error) { + return nil, domain.ErrNotFound +} +func (f *fakeUserRepo) GetUserByEmail(_ context.Context, _ string) (*domain.User, error) { + return nil, domain.ErrNotFound +} +func (f *fakeUserRepo) GetUserByWebAuthnCredential(_ context.Context, _ string) (*domain.User, error) { + return nil, domain.ErrNotFound +} +func (f *fakeUserRepo) FindUsers(_ context.Context, _ string) ([]domain.User, error) { + return nil, nil +} +func (f *fakeUserRepo) DeleteUser(_ context.Context, _ domain.UserIdentifier) error { return nil } + +type fakeBus struct { + published []string +} + +func (f *fakeBus) Publish(topic string, _ ...any) { + f.published = append(f.published, topic) +} + +// ldapUser builds an existing wg-portal user that is sourced from LDAP and is +// therefore a candidate for being disabled when missing. +func ldapUser(id string) domain.User { + return domain.User{ + Identifier: domain.UserIdentifier(id), + Authentications: []domain.UserAuthentication{ + {Source: domain.UserSourceLdap, ProviderName: "testprovider"}, + }, + } +} + +func newTestManager(repo *fakeUserRepo, bus *fakeBus) Manager { + return Manager{cfg: &config.Config{}, bus: bus, users: repo} +} + +// A directory that answers successfully but returns nothing usable must not be +// read as "every user was removed". Disabling every LDAP-sourced user at once +// takes down the VPN an admin would need in order to fix the directory. +func TestDisableMissingLdapUsers_RefusesOnEmptyDirectoryResult(t *testing.T) { + repo := &fakeUserRepo{users: []domain.User{ldapUser("alice"), ldapUser("bob")}} + bus := &fakeBus{} + m := newTestManager(repo, bus) + + err := m.disableMissingLdapUsers(context.Background(), "testprovider", + []internal.RawLdapUser{}, makeTestLdapFields()) + + require.NoError(t, err) + assert.Empty(t, repo.saved, "no user may be disabled when LDAP returned no entries") + assert.Empty(t, bus.published, "no disable events may be published") +} + +// Same protection when the search returns entries but the configured identifier +// attribute is absent from all of them -- a misconfigured field map yields no +// usable identifiers, which would otherwise mark every user as missing. +func TestDisableMissingLdapUsers_RefusesWhenNoUsableIdentifiers(t *testing.T) { + repo := &fakeUserRepo{users: []domain.User{ldapUser("alice"), ldapUser("bob")}} + bus := &fakeBus{} + m := newTestManager(repo, bus) + + // Entries exist, but none carry the "uid" attribute the field map expects. + raw := []internal.RawLdapUser{ + {"dn": "cn=alice,dc=example,dc=com", "cn": "alice"}, + {"dn": "cn=bob,dc=example,dc=com", "cn": "bob"}, + } + + err := m.disableMissingLdapUsers(context.Background(), "testprovider", raw, makeTestLdapFields()) + + require.NoError(t, err) + assert.Empty(t, repo.saved, "no user may be disabled when no identifiers could be extracted") + assert.Empty(t, bus.published) +} + +// The guard must not suppress the feature: with a genuine directory result, +// users absent from it are still disabled. +func TestDisableMissingLdapUsers_DisablesGenuinelyMissingUsers(t *testing.T) { + repo := &fakeUserRepo{users: []domain.User{ldapUser("alice"), ldapUser("bob")}} + bus := &fakeBus{} + m := newTestManager(repo, bus) + + // Only alice is still present in the directory. + raw := []internal.RawLdapUser{{"dn": "uid=alice,dc=example,dc=com", "uid": "alice"}} + + err := m.disableMissingLdapUsers(context.Background(), "testprovider", raw, makeTestLdapFields()) + + require.NoError(t, err) + assert.NotContains(t, repo.saved, domain.UserIdentifier("alice"), "alice is present in LDAP") + require.Contains(t, repo.saved, domain.UserIdentifier("bob"), "bob is missing and must be disabled") + assert.Equal(t, domain.DisabledReasonLdapMissing, repo.saved["bob"].DisabledReason) + assert.NotNil(t, repo.saved["bob"].Disabled) + assert.Len(t, bus.published, 1) +} + +// Users the operator has pinned with PersistLocalChanges, and users already +// disabled, must be left alone even when genuinely missing. +func TestDisableMissingLdapUsers_SkipsPinnedAndAlreadyDisabled(t *testing.T) { + pinned := ldapUser("pinned") + pinned.PersistLocalChanges = true + + alreadyDisabled := ldapUser("gone") + disabledAt := time.Now() + alreadyDisabled.Disabled = &disabledAt + + repo := &fakeUserRepo{users: []domain.User{pinned, alreadyDisabled}} + bus := &fakeBus{} + m := newTestManager(repo, bus) + + raw := []internal.RawLdapUser{{"dn": "uid=other,dc=example,dc=com", "uid": "other"}} + + err := m.disableMissingLdapUsers(context.Background(), "testprovider", raw, makeTestLdapFields()) + + require.NoError(t, err) + assert.Empty(t, repo.saved) + assert.Empty(t, bus.published) +}