fix(auth): harden tht auth mutations

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

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

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

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 {

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

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
 }