From 856ac05edc4ce4b9c849ea9d125e47562ba65145 Mon Sep 17 00:00:00 2001 From: mptyl Date: Sun, 16 Aug 2026 19:37:12 +0200 Subject: [PATCH] fix(auth): harden tht auth mutations --- tools/tht/internal/authconfig/commands.go | 56 ++++--- .../tht/internal/authconfig/commands_test.go | 141 ++++++++++++++++++ tools/tht/internal/authconfig/users.go | 29 ++-- tools/tht/internal/safeio/files.go | 9 ++ tools/tht/internal/safeio/replace_unix.go | 26 ++++ tools/tht/internal/safeio/replace_windows.go | 15 ++ 6 files changed, 245 insertions(+), 31 deletions(-) diff --git a/tools/tht/internal/authconfig/commands.go b/tools/tht/internal/authconfig/commands.go index 6a021d8c..096016a7 100644 --- a/tools/tht/internal/authconfig/commands.go +++ b/tools/tht/internal/authconfig/commands.go @@ -26,6 +26,9 @@ const maxPasswordFileBytes int64 = 1025 var errCommandRefused = errors.New("authentication command refused") +var writeNewAuthFile = safeio.WriteCanonicalNewFile +var removeAuthFile = safeio.RemoveCanonicalPrivateRegular + // Run implements the host-only authentication operator surface. It accepts password bytes only // from an echo-free terminal or a bounded private file, and never writes them to either stream. func Run(ctx context.Context, installation config.Installation, args []string, stdin io.Reader, stdout, stderr io.Writer) int { @@ -105,10 +108,8 @@ func configure(directory string, args []string, stdin io.Reader, stderr io.Write if _, _, err := Load(directory); err == nil { return errExistingConfiguration } - for _, name := range []string{authFileName, usersFileName} { - if _, err := os.Lstat(filepath.Join(directory, name)); !errors.Is(err, os.ErrNotExist) { - return errCommandRefused - } + if err := recoverOrRefuseBootstrapState(directory); err != nil { + return err } if request.mode == "oidc" { if request.userGroup == request.adminGroup { @@ -145,20 +146,22 @@ func parseConfigure(args []string) (configureRequest, error) { "--issuer": &request.issuer, "--client-id": &request.clientID, "--authentik-base-url": &request.authentikBaseURL, "--user-group": &request.userGroup, "--admin-group": &request.adminGroup, } + seen := make(map[string]bool, len(values)) for len(args) > 0 { option := args[0] args = args[1:] target, exists := values[option] - if !exists || len(args) == 0 || *target != "" { + if !exists || seen[option] || len(args) == 0 || args[0] == "" { return configureRequest{}, errCommandRefused } *target, args = args[0], args[1:] + seen[option] = true } if request.mode != "local" && request.mode != "oidc" || !validPublicURL(request.publicURL) { return configureRequest{}, errCommandRefused } if request.mode == "local" { - if request.issuer != "" || request.clientID != "" || request.authentikBaseURL != "" || request.userGroup != "" || request.adminGroup != "" { + if seen["--issuer"] || seen["--client-id"] || seen["--authentik-base-url"] || seen["--user-group"] || seen["--admin-group"] { return configureRequest{}, errCommandRefused } if request.adminUser != "" && request.passwordFile == "" { // TTY supplies the password interactively. @@ -166,7 +169,7 @@ func parseConfigure(args []string) (configureRequest, error) { } return request, nil } - if request.adminUser != "" || request.adminDisplayName != "" || request.passwordFile != "" || request.issuer == "" || request.clientID == "" || request.authentikBaseURL == "" || request.userGroup == "" || request.adminGroup == "" || !validOIDCURL(request.issuer) || !validHTTPSURL(request.authentikBaseURL) || (!isHTTPS(request.publicURL) && !isLoopbackURL(request.publicURL)) { + if seen["--admin-user"] || seen["--admin-display-name"] || seen["--password-file"] || request.issuer == "" || request.clientID == "" || request.authentikBaseURL == "" || request.userGroup == "" || request.adminGroup == "" || !validOIDCURL(request.issuer) || !validHTTPSURL(request.authentikBaseURL) || (!isHTTPS(request.publicURL) && !isLoopbackURL(request.publicURL)) { return configureRequest{}, errCommandRefused } return request, nil @@ -208,10 +211,13 @@ func writeInitialLocal(directory, publicURL string, user User) error { if err != nil { return errCommandRefused } - if err := safeio.WriteCanonicalNewFile(filepath.Join(directory, usersFileName), append(registryBytes, '\n'), 0o600); err != nil { + if err := writeNewAuthFile(filepath.Join(directory, usersFileName), append(registryBytes, '\n'), 0o600); err != nil { return errCommandRefused } - if err := safeio.WriteCanonicalNewFile(filepath.Join(directory, authFileName), append(configBytes, '\n'), 0o600); err != nil { + if err := writeNewAuthFile(filepath.Join(directory, authFileName), append(configBytes, '\n'), 0o600); err != nil { + if removeErr := removeAuthFile(filepath.Join(directory, usersFileName)); removeErr != nil { + return errCommandRefused + } return errCommandRefused } return nil @@ -228,7 +234,7 @@ func writeInitialOIDC(directory string, request configureRequest) error { if err != nil { return errCommandRefused } - if err := safeio.WriteCanonicalNewFile(filepath.Join(directory, authFileName), append(contents, '\n'), 0o600); err != nil { + if err := writeNewAuthFile(filepath.Join(directory, authFileName), append(contents, '\n'), 0o600); err != nil { return errCommandRefused } return nil @@ -442,33 +448,26 @@ func parseUserRolePassword(args []string) (string, userRequest, error) { username := args[0] args = args[1:] request := userRequest{} + seen := make(map[string]bool, 3) for len(args) > 0 { option := args[0] args = args[1:] - if len(args) == 0 { + if len(args) == 0 || args[0] == "" || seen[option] { return "", userRequest{}, errCommandRefused } value := args[0] args = args[1:] switch option { case "--role": - if request.role != "" { - return "", userRequest{}, errCommandRefused - } request.role = Role(value) case "--display-name": - if request.displayName != "" { - return "", userRequest{}, errCommandRefused - } request.displayName = value case "--password-file": - if request.passwordFile != "" { - return "", userRequest{}, errCommandRefused - } request.passwordFile = value default: return "", userRequest{}, errCommandRefused } + seen[option] = true } if !usernamePattern.MatchString(username) || (request.role != RoleUser && request.role != RoleAdmin) { return "", userRequest{}, errCommandRefused @@ -514,6 +513,23 @@ func parsePasswordFile(args []string) (string, error) { return args[1], nil } +func recoverOrRefuseBootstrapState(directory string) error { + authPath := filepath.Join(directory, authFileName) + usersPath := filepath.Join(directory, usersFileName) + _, authErr := os.Lstat(authPath) + _, usersErr := os.Lstat(usersPath) + if !errors.Is(authErr, os.ErrNotExist) { + return errCommandRefused + } + if errors.Is(usersErr, os.ErrNotExist) { + return nil + } + if usersErr != nil || removeAuthFile(usersPath) != nil { + return errCommandRefused + } + return nil +} + func setEnabled(directory string, enabled bool, args []string) error { if len(args) != 1 || !usernamePattern.MatchString(args[0]) { return errCommandRefused diff --git a/tools/tht/internal/authconfig/commands_test.go b/tools/tht/internal/authconfig/commands_test.go index ccc36014..9593ddd9 100644 --- a/tools/tht/internal/authconfig/commands_test.go +++ b/tools/tht/internal/authconfig/commands_test.go @@ -4,12 +4,14 @@ import ( "bytes" "context" "encoding/json" + "errors" "os" "path/filepath" "strings" "testing" "github.com/aritmolab/thothii/tools/tht/internal/config" + "github.com/aritmolab/thothii/tools/tht/internal/safeio" ) func TestRunConfiguresLocalRegistryAndRedactsStatusJSON(t *testing.T) { @@ -155,6 +157,145 @@ func TestRunRefusesNonTTYPasswordsWithoutPasswordFile(t *testing.T) { } } +func TestRunBumpsRevisionExactlyOnceForEverySecurityMutation(t *testing.T) { + directory := newAuthDirectory(t) + installation := authInstallation(directory) + adminPassword := writePasswordFile(t, "initial administrator password\n") + userPassword := writePasswordFile(t, "ordinary user password one\n") + if code := Run(context.Background(), installation, []string{"configure", "--mode", "local", "--public-url", "http://127.0.0.1:8080", "--admin-user", "admin", "--password-file", adminPassword}, strings.NewReader(""), ioDiscard{}, ioDiscard{}); code != 0 { + t.Fatalf("configure = %d", code) + } + if code := Run(context.Background(), installation, []string{"user", "add", "operator", "--role", "user", "--password-file", userPassword}, strings.NewReader(""), ioDiscard{}, ioDiscard{}); code != 0 { + t.Fatalf("add = %d", code) + } + for _, test := range []struct { + args []string + wantRevision uint64 + wantCode int + }{ + {[]string{"user", "set-password", "operator", "--password-file", adminPassword}, 2, 0}, + {[]string{"user", "disable", "operator"}, 3, 0}, + {[]string{"user", "disable", "operator"}, 3, 0}, // intentional idempotent no-op + {[]string{"user", "enable", "operator"}, 4, 0}, + {[]string{"user", "enable", "operator"}, 4, 0}, // intentional idempotent no-op + {[]string{"user", "grant", "operator", "--role", "admin"}, 5, 0}, + {[]string{"user", "grant", "operator", "--role", "admin"}, 5, 2}, // rejected duplicate + {[]string{"user", "revoke", "operator", "--role", "admin"}, 6, 0}, + {[]string{"user", "revoke", "operator", "--role", "admin"}, 6, 2}, // rejected duplicate + {[]string{"user", "logout-all", "operator", "--yes"}, 7, 0}, + } { + code := Run(context.Background(), installation, test.args, strings.NewReader(""), ioDiscard{}, ioDiscard{}) + if code != test.wantCode { + t.Fatalf("%v = %d, want %d", test.args, code, test.wantCode) + } + if got := authRevisionFor(t, directory, "operator"); got != test.wantRevision { + t.Fatalf("%v revision = %d, want %d", test.args, got, test.wantRevision) + } + } +} + +func TestRunUserListJSONRedactsPasswordHash(t *testing.T) { + directory := newAuthDirectory(t) + installation := authInstallation(directory) + passwordFile := writePasswordFile(t, "this is a local test password\n") + if code := Run(context.Background(), installation, []string{"configure", "--mode", "local", "--public-url", "http://127.0.0.1:8080", "--admin-user", "admin", "--password-file", passwordFile}, strings.NewReader(""), ioDiscard{}, ioDiscard{}); code != 0 { + t.Fatalf("configure = %d", code) + } + var stdout, stderr bytes.Buffer + if code := Run(context.Background(), installation, []string{"user", "list", "--json"}, strings.NewReader(""), &stdout, &stderr); code != 0 { + t.Fatalf("list = %d, stderr=%q", code, stderr.String()) + } + var listed struct { + Users []User `json:"users"` + } + if err := json.Unmarshal(stdout.Bytes(), &listed); err != nil || len(listed.Users) != 1 { + t.Fatalf("list JSON = %q, err=%v", stdout.String(), err) + } + if strings.Contains(stdout.String()+stderr.String(), "this is a local test password") || strings.Contains(stdout.String()+stderr.String(), "$argon2id$") || strings.Contains(stdout.String(), "passwordHash") { + t.Fatalf("list JSON exposed password material: stdout=%q stderr=%q", stdout.String(), stderr.String()) + } +} + +func TestStrictOptionParsersRejectEmptyDuplicateMissingAndWrongModeFlags(t *testing.T) { + for _, args := range [][]string{ + {"--mode", "local", "--public-url", "http://127.0.0.1:8080", "--admin-display-name", "", "--admin-display-name", "Admin"}, + {"--mode", "local", "--public-url", "http://127.0.0.1:8080", "--issuer", "https://auth.example.org/"}, + {"--mode", "oidc", "--public-url", "https://thothii.example.org", "--issuer", "https://auth.example.org/", "--client-id", "client", "--authentik-base-url", "https://auth.example.org", "--user-group", "users", "--admin-group", "admins", "--password-file", "password-file"}, + {"--mode", "local", "--public-url"}, + } { + if _, err := parseConfigure(args); err == nil { + t.Fatalf("parseConfigure(%q) accepted unsafe options", args) + } + } + for _, args := range [][]string{ + {"operator", "--role", "user", "--display-name", "", "--display-name", "Operator"}, + {"operator", "--role", "user", "--password-file", ""}, + {"operator", "--role"}, + } { + if _, _, err := parseUserRolePassword(args); err == nil { + t.Fatalf("parseUserRolePassword(%q) accepted unsafe options", args) + } + } +} + +func TestConfigureRecoversSafeOrphanUsersFileAfterInterruptedBootstrap(t *testing.T) { + directory := writeAuthFiles(t, defaultAuthYAML, registryYAML(adminUserYAML("abandoned", "Abandoned", true, "admin"))) + if err := safeio.RemoveCanonicalPrivateRegular(filepath.Join(directory, authFileName)); err != nil { + t.Fatal(err) + } + passwordFile := writePasswordFile(t, "this is a local test password\n") + if code := Run(context.Background(), authInstallation(directory), []string{"configure", "--mode", "local", "--public-url", "http://127.0.0.1:8080", "--admin-user", "admin", "--password-file", passwordFile}, strings.NewReader(""), ioDiscard{}, ioDiscard{}); code != 0 { + t.Fatalf("recovered configure = %d", code) + } + if got := authRevisionFor(t, directory, "admin"); got != 1 { + t.Fatalf("recovered initial administrator revision = %d, want 1", got) + } +} + +func TestConfigureRollsBackPublishFailureAndRetrySucceeds(t *testing.T) { + for _, failAt := range []int{1, 2} { + t.Run("publish failure", func(t *testing.T) { + directory := newAuthDirectory(t) + passwordFile := writePasswordFile(t, "this is a local test password\n") + previous := writeNewAuthFile + calls := 0 + writeNewAuthFile = func(path string, contents []byte, mode os.FileMode) error { + calls++ + if calls == failAt { + return errors.New("injected publish failure") + } + return previous(path, contents, mode) + } + t.Cleanup(func() { writeNewAuthFile = previous }) + if code := Run(context.Background(), authInstallation(directory), []string{"configure", "--mode", "local", "--public-url", "http://127.0.0.1:8080", "--admin-user", "admin", "--password-file", passwordFile}, strings.NewReader(""), ioDiscard{}, ioDiscard{}); code == 0 { + t.Fatal("configure succeeded despite injected publish failure") + } + for _, name := range []string{authFileName, usersFileName} { + if _, err := os.Lstat(filepath.Join(directory, name)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("%s remains after failed bootstrap: %v", name, err) + } + } + writeNewAuthFile = previous + if code := Run(context.Background(), authInstallation(directory), []string{"configure", "--mode", "local", "--public-url", "http://127.0.0.1:8080", "--admin-user", "admin", "--password-file", passwordFile}, strings.NewReader(""), ioDiscard{}, ioDiscard{}); code != 0 { + t.Fatalf("retry = %d", code) + } + }) + } +} + +func authRevisionFor(t *testing.T, directory, username string) uint64 { + t.Helper() + _, registry, err := Load(directory) + if err != nil { + t.Fatal(err) + } + user := registry.FindByUsername(username) + if user == nil { + t.Fatalf("missing user %q", username) + } + return user.AuthRevision +} + func writePasswordFile(t *testing.T, password string) string { t.Helper() path := filepath.Join(newAuthDirectory(t), "password") diff --git a/tools/tht/internal/authconfig/users.go b/tools/tht/internal/authconfig/users.go index 54b44895..b12c1cd3 100644 --- a/tools/tht/internal/authconfig/users.go +++ b/tools/tht/internal/authconfig/users.go @@ -166,23 +166,30 @@ func applyMutationInvariants(before Registry, after *Registry) error { return errInvalidAuthenticationConfig } securityChanged := user.PasswordHash != old.PasswordHash || user.Enabled != old.Enabled || !sameRoles(user.Roles, old.Roles) - explicitLogoutAll := user.AuthRevision == old.AuthRevision+1 - if user.AuthRevision != old.AuthRevision && !explicitLogoutAll { + revision, err := reconciledAuthRevision(old.AuthRevision, user.AuthRevision, securityChanged) + if err != nil { return errInvalidAuthenticationConfig } - if securityChanged && !explicitLogoutAll { - if old.AuthRevision == ^uint64(0) { - return errInvalidAuthenticationConfig - } - user.AuthRevision = old.AuthRevision + 1 - } - if !securityChanged && !explicitLogoutAll { - user.AuthRevision = old.AuthRevision - } + user.AuthRevision = revision } return validateRegistry(*after) } +// reconciledAuthRevision is the single authority for local-user security revisions. Security +// changes get one bump; a caller may also request exactly one explicit logout-all bump. +func reconciledAuthRevision(previous, requested uint64, securityChanged bool) (uint64, error) { + if requested != previous && (previous == ^uint64(0) || requested != previous+1) { + return 0, errInvalidAuthenticationConfig + } + if securityChanged && requested == previous { + if previous == ^uint64(0) { + return 0, errInvalidAuthenticationConfig + } + return previous + 1, nil + } + return requested, nil +} + func sameRoles(left, right []Role) bool { if len(left) != len(right) { return false diff --git a/tools/tht/internal/safeio/files.go b/tools/tht/internal/safeio/files.go index c41bd88a..4800c24a 100644 --- a/tools/tht/internal/safeio/files.go +++ b/tools/tht/internal/safeio/files.go @@ -118,6 +118,15 @@ func ReplaceCanonicalRegular(path string, contents []byte, mode os.FileMode) err return replaceCanonicalRegular(path, contents) } +// RemoveCanonicalPrivateRegular removes one existing private regular file without following a +// symlinked path component. It is intended only for rolling back a file this process published. +func RemoveCanonicalPrivateRegular(path string) error { + if err := ValidatePrivateRegular(path); err != nil { + return ErrUnsafeFile + } + return removeCanonicalPrivateRegular(path) +} + func randomTemporaryName() (string, error) { bytes := make([]byte, 16) if _, err := rand.Read(bytes); err != nil { diff --git a/tools/tht/internal/safeio/replace_unix.go b/tools/tht/internal/safeio/replace_unix.go index 83888857..456756a6 100644 --- a/tools/tht/internal/safeio/replace_unix.go +++ b/tools/tht/internal/safeio/replace_unix.go @@ -43,6 +43,24 @@ func replaceCanonicalRegular(path string, contents []byte) error { return nil } +func removeCanonicalPrivateRegular(path string) error { + directory, target, err := openCanonicalParentDirectory(path) + if err != nil { + return ErrUnsafeFile + } + defer unix.Close(directory) + if err := requirePrivateRegularAt(directory, target); err != nil { + return ErrUnsafeFile + } + if err := unix.Unlinkat(directory, target, 0); err != nil { + return ErrUnsafeFile + } + if err := unix.Fsync(directory); err != nil { + return ErrUnsafeFile + } + return nil +} + func openCanonicalParentDirectory(path string) (int, string, error) { components := strings.Split(strings.TrimPrefix(path, string(os.PathSeparator)), string(os.PathSeparator)) if len(components) < 2 || components[0] == "" || components[len(components)-1] == "" { @@ -72,6 +90,14 @@ func requireSingleRegularAt(directory int, name string) error { return nil } +func requirePrivateRegularAt(directory int, name string) error { + var stat unix.Stat_t + if err := unix.Fstatat(directory, name, &stat, unix.AT_SYMLINK_NOFOLLOW); err != nil || stat.Mode&unix.S_IFMT != unix.S_IFREG || stat.Nlink != 1 || stat.Mode&0o7777 != 0o600 { + return ErrUnsafeFile + } + return nil +} + func writePrivateTemporaryAt(directory int, contents []byte) (string, error) { for attempt := 0; attempt < 16; attempt++ { name, err := randomTemporaryName() diff --git a/tools/tht/internal/safeio/replace_windows.go b/tools/tht/internal/safeio/replace_windows.go index 98da45c5..838ccca9 100644 --- a/tools/tht/internal/safeio/replace_windows.go +++ b/tools/tht/internal/safeio/replace_windows.go @@ -47,6 +47,21 @@ func replaceCanonicalRegular(path string, contents []byte) error { return nil } +func removeCanonicalPrivateRegular(path string) error { + parents, target, err := openCanonicalWindowsParent(path) + if err != nil { + return ErrUnsafeFile + } + defer parents.Close() + if err := ValidatePrivateRegular(path); err != nil { + return ErrUnsafeFile + } + if err := windows.DeleteFile(windows.StringToUTF16Ptr(filepath.Join(parents.directory, target))); err != nil { + return ErrUnsafeFile + } + return nil +} + func safeExistingRegular(path string) bool { return ValidatePrivateRegular(path) == nil }