diff --git a/cmd/admin_auth_oauth.go b/cmd/admin_auth_oauth.go --- a/cmd/admin_auth_oauth.go +++ b/cmd/admin_auth_oauth.go @@ -9,9 +9,12 @@ "net/url" auth_model "forgejo.org/models/auth" + "forgejo.org/models/db" + "forgejo.org/modules/setting" "forgejo.org/services/auth/source/oauth2" "github.com/urfave/cli/v2" + "xorm.io/xorm" ) var ( @@ -35,6 +38,10 @@ Name: "secret", Value: "", Usage: "Client Secret", + }, + &cli.StringFlag{ + Name: "secret-file", + Usage: "Read a 64-byte base64url client secret from an admitted read-only regular file", }, &cli.StringFlag{ Name: "auto-discover-url", @@ -174,14 +181,26 @@ } func runAddOauth(c *cli.Context) error { + secret, selected, err := oauthSecretInput(c) + if err != nil { + return err + } ctx, cancel := installSignals() defer cancel() + ctx = db.WithSensitiveSQL(ctx) if err := initDB(ctx); err != nil { return err } + engine, ok := db.GetEngine(ctx).(*xorm.Engine) + if !ok || setting.Database.LogSQL || engine.Logger().IsShowSQL() { + return errors.New("OAuth configuration requires SQL logging disabled") + } config := parseOAuth2Config(c) + if selected { + config.ClientSecret = secret + } if err := oauth2.ValidateRequiredClaimValues(config.RequiredClaimName, config.RequiredClaimValue, config.RequiredClaimValues); err != nil { return err } @@ -195,24 +214,37 @@ } } - return auth_model.CreateSource(ctx, &auth_model.Source{ + err = auth_model.CreateSource(ctx, &auth_model.Source{ Type: auth_model.OAuth2, Name: c.String("name"), IsActive: true, Cfg: config, }) + if err != nil && c.IsSet("secret-file") { + return errors.New("OAuth source creation failed") + } + return err } func runUpdateOauth(c *cli.Context) error { + secret, selected, err := oauthSecretInput(c) + if err != nil { + return err + } if !c.IsSet("id") { return errors.New("--id flag is missing") } ctx, cancel := installSignals() defer cancel() + ctx = db.WithSensitiveSQL(ctx) if err := initDB(ctx); err != nil { return err + } + engine, ok := db.GetEngine(ctx).(*xorm.Engine) + if !ok || setting.Database.LogSQL || engine.Logger().IsShowSQL() { + return errors.New("OAuth configuration requires SQL logging disabled") } source, err := auth_model.GetSourceByID(ctx, c.Int64("id")) @@ -220,7 +252,10 @@ return err } - oAuth2Config := source.Cfg.(*oauth2.Source) + oAuth2Config, ok := source.Cfg.(*oauth2.Source) + if !ok { + return errors.New("authentication source is not OAuth2") + } if c.IsSet("name") { source.Name = c.String("name") @@ -234,8 +269,8 @@ oAuth2Config.ClientID = c.String("key") } - if c.IsSet("secret") { - oAuth2Config.ClientSecret = c.String("secret") + if selected { + oAuth2Config.ClientSecret = secret } if c.IsSet("auto-discover-url") { @@ -316,5 +351,22 @@ } source.Cfg = oAuth2Config - return auth_model.UpdateSource(ctx, source) + err = auth_model.UpdateSource(ctx, source) + if err != nil && c.IsSet("secret-file") { + return errors.New("OAuth source update failed") + } + return err } + +// Resolve before opening the DB or mutating a loaded source. Never store the +// resolved bytes in the CLI context: they must not become flag/argv diagnostics. +func oauthSecretInput(c *cli.Context) (string, bool, error) { + if c.IsSet("secret-file") { + if c.IsSet("secret") { + return "", false, errors.New("secret and secret-file are mutually exclusive") + } + value, err := readOAuthSecretFile(c.String("secret-file")) + return value, true, err + } + return c.String("secret"), c.IsSet("secret"), nil +} diff --git a/cmd/admin_auth_oauth_secret_linux.go b/cmd/admin_auth_oauth_secret_linux.go --- /dev/null +++ b/cmd/admin_auth_oauth_secret_linux.go @@ -0,0 +1,114 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +//go:build linux + +package cmd + +import ( + "errors" + "io" + "os" + "path/filepath" + "strings" + + "golang.org/x/sys/unix" +) + +var errOAuthSecretFile = errors.New("secret-file input refused") + +// This checks DAC and mount properties, not namespace, Secret provenance or +// operation authority. Admission must bind the exact workload/mount/reader set. +// Kubernetes Secret subPath leaves are root-owned; fsGroup can provide 0440 +// access. Requiring euid ownership would incorrectly refuse that supported case. +func readOAuthSecretFile(path string) (string, error) { + if len(path) > 4096 || !filepath.IsAbs(path) || filepath.Clean(path) != path || strings.ContainsRune(path, 0) { + return "", errOAuthSecretFile + } + parts := strings.Split(strings.TrimPrefix(path, "/"), "/") + if len(parts) < 2 { + return "", errOAuthSecretFile + } + dir, err := unix.Open("/", unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return "", errOAuthSecretFile + } + defer func() { unix.Close(dir) }() + for _, part := range parts[:len(parts)-1] { + var st unix.Stat_t + if unix.Fstat(dir, &st) != nil || !safeOAuthSecretParent(st) { + return "", errOAuthSecretFile + } + next, err := unix.Openat(dir, part, unix.O_PATH|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + return "", errOAuthSecretFile + } + unix.Close(dir) + dir = next + } + var parent unix.Stat_t + if unix.Fstat(dir, &parent) != nil || !safeOAuthSecretParent(parent) { + return "", errOAuthSecretFile + } + // Nonblocking is necessary: reject a FIFO without waiting for a writer. + fd, err := unix.Openat(dir, parts[len(parts)-1], unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK|unix.O_CLOEXEC, 0) + if err != nil { + return "", errOAuthSecretFile + } + f := os.NewFile(uintptr(fd), "private OAuth input") + defer f.Close() + return readOAuthSecretFD(f, dir, parts[len(parts)-1]) +} + +func readOAuthSecretFD(f *os.File, dir int, leaf string) (string, error) { + fd := int(f.Fd()) + var before, after, named unix.Stat_t + var mount unix.Statfs_t + if unix.Fstat(fd, &before) != nil || !safeOAuthSecretLeaf(before) || unix.Fstatfs(fd, &mount) != nil || mount.Flags&unix.ST_RDONLY == 0 { + return "", errOAuthSecretFile + } + // mint-secret uses secrets.token_urlsafe(48): exactly 64 ASCII characters, + // no padding/newline. Bounded read still detects a concurrent size increase. + data, err := io.ReadAll(io.LimitReader(f, 65)) + defer clear(data) + if err != nil || len(data) != 64 { + return "", errOAuthSecretFile + } + for _, b := range data { + if !(b >= 'A' && b <= 'Z' || b >= 'a' && b <= 'z' || b >= '0' && b <= '9' || b == '-' || b == '_') { + return "", errOAuthSecretFile + } + } + if unix.Fstat(fd, &after) != nil || !sameOAuthSecretFile(before, after) || unix.Fstatat(dir, leaf, &named, unix.AT_SYMLINK_NOFOLLOW) != nil || !sameOAuthSecretFile(before, named) { + return "", errOAuthSecretFile + } + return string(data), nil +} + +func safeOAuthSecretParent(st unix.Stat_t) bool { + return st.Mode&unix.S_IFMT == unix.S_IFDIR && (st.Uid == 0 || st.Uid == uint32(os.Geteuid())) && st.Mode&0022 == 0 +} + +func safeOAuthSecretLeaf(st unix.Stat_t) bool { + if st.Mode&unix.S_IFMT != unix.S_IFREG || st.Nlink != 1 || st.Size != 64 || st.Mode&07337 != 0 || (st.Uid != 0 && st.Uid != uint32(os.Geteuid())) { + return false + } + if st.Mode&0040 != 0 { + groups, err := os.Getgroups() + if err != nil { + return false + } + member := st.Gid == uint32(os.Getegid()) + for _, gid := range groups { + member = member || st.Gid == uint32(gid) + } + if !member { + return false + } + } + return true +} + +func sameOAuthSecretFile(a, b unix.Stat_t) bool { + return a.Dev == b.Dev && a.Ino == b.Ino && a.Mode == b.Mode && a.Nlink == b.Nlink && a.Uid == b.Uid && a.Gid == b.Gid && a.Size == b.Size && a.Mtim == b.Mtim && a.Ctim == b.Ctim +} diff --git a/cmd/admin_auth_oauth_secret_other.go b/cmd/admin_auth_oauth_secret_other.go --- /dev/null +++ b/cmd/admin_auth_oauth_secret_other.go @@ -0,0 +1,12 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +//go:build !linux + +package cmd + +import "errors" + +func readOAuthSecretFile(string) (string, error) { + return "", errors.New("secret-file input refused") +} diff --git a/cmd/admin_auth_oauth_secret_linux_test.go b/cmd/admin_auth_oauth_secret_linux_test.go --- /dev/null +++ b/cmd/admin_auth_oauth_secret_linux_test.go @@ -0,0 +1,79 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +//go:build linux + +package cmd + +import ( + "golang.org/x/sys/unix" + "os" + "testing" +) + +func TestPrivateSecretMetadata(t *testing.T) { + st := unix.Stat_t{Mode: unix.S_IFREG | 0440, Uid: 0, Gid: uint32(os.Getegid()), Nlink: 1, Size: 64} + if !safeOAuthSecretLeaf(st) { + t.Fatal("root-owned fsGroup-readable Secret subPath refused") + } + for _, mode := range []uint32{0444, 0640, 0540, 0460, 0441, 0440 | unix.S_ISUID, 0440 | unix.S_ISGID, 0440 | unix.S_ISVTX} { + x := st + x.Mode = unix.S_IFREG | mode + if safeOAuthSecretLeaf(x) { + t.Fatalf("unsafe mode accepted: %o", mode) + } + } + for _, mutate := range []func(*unix.Stat_t){func(x *unix.Stat_t) { x.Uid = 123456 }, func(x *unix.Stat_t) { x.Nlink = 2 }, func(x *unix.Stat_t) { x.Size = 63 }, func(x *unix.Stat_t) { x.Mode = unix.S_IFIFO | 0400 }, func(x *unix.Stat_t) { x.Gid = 123456 }} { + x := st + mutate(&x) + if safeOAuthSecretLeaf(x) { + t.Fatal("unsafe metadata accepted") + } + } + if !safeOAuthSecretParent(unix.Stat_t{Mode: unix.S_IFDIR | 0755, Uid: 0}) { + t.Fatal("root parent refused") + } + if safeOAuthSecretParent(unix.Stat_t{Mode: unix.S_IFDIR | 0775, Uid: 0}) { + t.Fatal("group writable parent accepted") + } + for _, mutate := range []func(*unix.Stat_t){func(x *unix.Stat_t) { x.Dev++ }, func(x *unix.Stat_t) { x.Ino++ }, func(x *unix.Stat_t) { x.Ctim.Nsec++ }, func(x *unix.Stat_t) { x.Mtim.Nsec++ }, func(x *unix.Stat_t) { x.Size++ }, func(x *unix.Stat_t) { x.Uid++ }, func(x *unix.Stat_t) { x.Gid++ }, func(x *unix.Stat_t) { x.Nlink++ }, func(x *unix.Stat_t) { x.Mode++ }} { + x := st + mutate(&x) + if sameOAuthSecretFile(st, x) { + t.Fatal("changed file considered stable") + } + } + x := st + x.Atim.Nsec++ + if !sameOAuthSecretFile(st, x) { + t.Fatal("read-induced atime change refused") + } +} + +// Actual Linux descriptors and namespace read-only bind mounts, not mocked stat +// metadata. A writable alias lets the fixture simulate replacement after open. +func TestPrivateSecretOpenedFileReplacement(t *testing.T) { + if _, err := os.Stat("/run/forgejo-oidc/race"); os.IsNotExist(err) { + t.Skip("requires isolated read-only bind fixture") + } + dir, err := unix.Open("/run/forgejo-oidc", unix.O_PATH|unix.O_DIRECTORY, 0) + if err != nil { + t.Fatal(err) + } + defer unix.Close(dir) + f, err := os.Open("/run/forgejo-oidc/race") + if err != nil { + t.Fatal(err) + } + defer f.Close() + // Preserve the opened inode with one link; replace only the named leaf. + if err := os.Rename("/run/writable-input/race", "/run/writable-input/race-backup"); err != nil { + t.Fatal(err) + } + if err := os.Rename("/run/writable-input/race-replacement", "/run/writable-input/race"); err != nil { + t.Fatal(err) + } + if value, err := readOAuthSecretFD(f, dir, "race"); err != errOAuthSecretFile || value != "" { + t.Fatal("replaced leaf accepted") + } +}