From aaf4ccd8c982b8113b1a92b93278167e943b7b27 Mon Sep 17 00:00:00 2001 From: LuoChen Date: Sat, 3 Oct 2026 18:37:56 +0800 Subject: [PATCH] util, types: parse unix_socket_permission strictly Default "0o770" failed the base-8 parse and silently became 0700. Invalid, unquoted or above-0777 values now fail config load. Fixes #3529 --- CHANGELOG.md | 2 + cmd/headscale/headscale_test.go | 8 ++- config-example.yaml | 1 + hscontrol/types/config.go | 18 ++++++- hscontrol/types/config_test.go | 77 ++++++++++++++++++++++++++++ hscontrol/util/file.go | 18 ++++--- hscontrol/util/util_test.go | 91 +++++++++++++++++++++++++++++++++ 7 files changed, 205 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 30be662df..91811c952 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,7 @@ clients, and how to run the same setup without Nix. - `derp.paths` files must end in `.yaml`, `.yml`, `.json` or `.hujson`; the extension picks the format - `dns.extra_records_path` must end in `.json`, `.hujson`, `.yaml` or `.yml`; the extension picks the format - A `derp.paths` file that decodes to no regions now stops headscale from starting instead of being silently ignored +- `unix_socket_permission` must be a quoted octal string no higher than `"0777"`, e.g. `"0770"`; an unquoted number or an invalid value now stops headscale from starting instead of silently using `0700` [#3540](https://github.com/juanfont/headscale/pull/3540) #### CLI @@ -138,6 +139,7 @@ clients, and how to run the same setup without Nix. - Fix SSH check periods coming from the first check rule for a node pair instead of the rule for the login user [#3517](https://github.com/juanfont/headscale/pull/3517) - Policy changes resend a node's SSH policy only when it changed, sparing clients a full netmap rebuild [#3517](https://github.com/juanfont/headscale/pull/3517) - Fix every grant being fully resolved as if it had `via` when the policy has no `via` grants, slowing map generation on large tailnets [#3538](https://github.com/juanfont/headscale/pull/3538) +- Fix the unix socket being created `0700` instead of the documented `0770` when `unix_socket_permission` is unset. The socket grants full admin access without authentication, so members of its group gain that access; check who is in it before upgrading [#3540](https://github.com/juanfont/headscale/pull/3540) ## 0.29.5 (202x-xx-xx) diff --git a/cmd/headscale/headscale_test.go b/cmd/headscale/headscale_test.go index 08532aee2..45adf1a17 100644 --- a/cmd/headscale/headscale_test.go +++ b/cmd/headscale/headscale_test.go @@ -41,7 +41,9 @@ func TestConfigFileLoading(t *testing.T) { assert.Empty(t, viper.GetString("tls_letsencrypt_hostname")) assert.Equal(t, ":http", viper.GetString("tls_letsencrypt_listen")) assert.Equal(t, "HTTP-01", viper.GetString("tls_letsencrypt_challenge_type")) - assert.Equal(t, fs.FileMode(0o770), util.GetFileMode("unix_socket_permission")) + mode, err := util.ParseFileMode(viper.GetString("unix_socket_permission")) + require.NoError(t, err) + assert.Equal(t, fs.FileMode(0o770), mode) assert.False(t, viper.GetBool("logtail.enabled")) } @@ -71,6 +73,8 @@ func TestConfigLoading(t *testing.T) { assert.Empty(t, viper.GetString("tls_letsencrypt_hostname")) assert.Equal(t, ":http", viper.GetString("tls_letsencrypt_listen")) assert.Equal(t, "HTTP-01", viper.GetString("tls_letsencrypt_challenge_type")) - assert.Equal(t, fs.FileMode(0o770), util.GetFileMode("unix_socket_permission")) + mode, err := util.ParseFileMode(viper.GetString("unix_socket_permission")) + require.NoError(t, err) + assert.Equal(t, fs.FileMode(0o770), mode) assert.False(t, viper.GetBool("logtail.enabled")) } diff --git a/config-example.yaml b/config-example.yaml index 685e22dd8..0210de3f7 100644 --- a/config-example.yaml +++ b/config-example.yaml @@ -361,6 +361,7 @@ dns: # Unix socket used for the CLI to connect without authentication # Note: for production you will want to set this to something like: unix_socket: /var/run/headscale/headscale.sock +# Octal, quoted: YAML reads an unquoted 0770 as the number 504. unix_socket_permission: "0770" # OpenID Connect diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index f15fe688b..50476aae7 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -1314,6 +1314,22 @@ func LoadServerConfig() (*Config, error) { }) } + // YAML reads an unquoted 0770 as the integer 504, which cannot be told + // apart from a literal 504, so only a string is accepted. + socketPermRaw := viper.Get("unix_socket_permission") + socketPermStr, _ := socketPermRaw.(string) + + socketPerm, err := util.ParseFileMode(socketPermStr) + if err != nil { + v.Add(&ConfigError{ + Reason: "unix_socket_permission is not a quoted octal file mode", + Current: []KV{{"unix_socket_permission", socketPermRaw}}, + Maximum: `"0777"`, + Hint: `quote the value, e.g. "0770"; unquoted, YAML reads 0770 as the number 504`, + Cause: err, + }) + } + dnsConfig, err := dns() if err != nil { v.Add(&ConfigError{ @@ -1426,7 +1442,7 @@ func LoadServerConfig() (*Config, error) { ACMEURL: viper.GetString("acme_url"), UnixSocket: viper.GetString("unix_socket"), - UnixSocketPermission: util.GetFileMode("unix_socket_permission"), + UnixSocketPermission: socketPerm, OIDC: OIDCConfig{ OnlyStartIfOIDCIsAvailable: viper.GetBool( diff --git a/hscontrol/types/config_test.go b/hscontrol/types/config_test.go index 9723335e7..10f808134 100644 --- a/hscontrol/types/config_test.go +++ b/hscontrol/types/config_test.go @@ -3,6 +3,7 @@ package types import ( "encoding/json" "fmt" + "io/fs" "net/netip" "os" "path/filepath" @@ -11,6 +12,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/juanfont/headscale/hscontrol/util" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -681,6 +683,81 @@ dns: } } +// The unset case is the regression: the default is spelled "0o770", which a +// base-8 parse rejects, so the socket silently became 0700. +// https://github.com/juanfont/headscale/issues/3529 +func TestUnixSocketPermission(t *testing.T) { + tests := []struct { + name string + yaml string + env string + want fs.FileMode + wantErr bool + }{ + {name: "unset-uses-default", want: 0o770}, + {name: "null-uses-default", yaml: `unix_socket_permission: null`, want: 0o770}, + {name: "quoted-leading-zero", yaml: `unix_socket_permission: "0770"`, want: 0o770}, + {name: "quoted-plain", yaml: `unix_socket_permission: "770"`, want: 0o770}, + {name: "quoted-0o-prefix", yaml: `unix_socket_permission: "0o770"`, want: 0o770}, + {name: "quoted-0o-prefix-owner-only", yaml: `unix_socket_permission: "0o700"`, want: 0o700}, + {name: "quoted-zero", yaml: `unix_socket_permission: "0000"`, want: 0}, + {name: "env", env: "0660", want: 0o660}, + {name: "env-0o-prefix", env: "0o660", want: 0o660}, + {name: "env-overrides-file", yaml: `unix_socket_permission: "0770"`, env: "0700", want: 0o700}, + {name: "env-invalid-is-error", env: "bogus", wantErr: true}, + {name: "empty-is-error", yaml: `unix_socket_permission: ""`, wantErr: true}, + {name: "symbolic-is-error", yaml: `unix_socket_permission: "rwxrwx---"`, wantErr: true}, + {name: "special-bits-are-error", yaml: `unix_socket_permission: "1770"`, wantErr: true}, + {name: "out-of-range-is-error", yaml: `unix_socket_permission: "17777"`, wantErr: true}, + // YAML reads unquoted 0770 as int 504, indistinguishable from a literal 504. + {name: "unquoted-leading-zero-is-error", yaml: `unix_socket_permission: 0770`, wantErr: true}, + {name: "unquoted-0o-prefix-is-error", yaml: `unix_socket_permission: 0o770`, wantErr: true}, + {name: "unquoted-plain-is-error", yaml: `unix_socket_permission: 770`, wantErr: true}, + {name: "float-is-error", yaml: `unix_socket_permission: 770.0`, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + viper.Reset() + + if tt.env != "" { + t.Setenv("HEADSCALE_UNIX_SOCKET_PERMISSION", tt.env) + } + + tmpDir := t.TempDir() + cfg := `--- +server_url: https://example.com +listen_addr: 0.0.0.0:8080 +prefixes: + v4: 100.64.0.0/10 +noise: + private_key_path: noise_private.key +database: + type: sqlite3 +dns: + magic_dns: false + override_local_dns: false +` + tt.yaml + "\n" + require.NoError(t, os.WriteFile( + filepath.Join(tmpDir, "config.yaml"), []byte(cfg), 0o600)) + require.NoError(t, LoadConfig(tmpDir, false)) + + got, err := LoadServerConfig() + if tt.wantErr { + require.ErrorIs(t, err, util.ErrInvalidFileMode, + "invalid unix_socket_permission must not silently fall back") + assert.Contains(t, err.Error(), "unix_socket_permission") + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.want, got.UnixSocketPermission, + "got %#o, want %#o", got.UnixSocketPermission, tt.want) + }) + } +} + // OK // server_url: headscale.com, base: clients.headscale.com // server_url: headscale.com, base: headscale.net diff --git a/hscontrol/util/file.go b/hscontrol/util/file.go index a9bc2efeb..8b9091896 100644 --- a/hscontrol/util/file.go +++ b/hscontrol/util/file.go @@ -27,6 +27,9 @@ const ( // ErrDirectoryPermission is returned when creating a directory fails due to permission issues. var ErrDirectoryPermission = errors.New("creating directory failed with permission error") +// ErrInvalidFileMode is returned by [ParseFileMode]. +var ErrInvalidFileMode = errors.New("not an octal file mode between 0 and 0777") + // ErrUnknownFileFormat is returned for a file whose extension names no format // [UnmarshalByExt] reads. var ErrUnknownFileFormat = errors.New("unknown file format, want .json, .hujson, .yaml or .yml") @@ -87,15 +90,16 @@ func AbsolutePathFromConfigPath(path string) string { return path } -func GetFileMode(key string) fs.FileMode { - modeStr := viper.GetString(key) - - mode, err := strconv.ParseUint(modeStr, Base8, BitSize64) - if err != nil { - return PermissionFallback +// ParseFileMode parses an octal permission such as "0770", "770" or "0o770". +// Bits above [fs.ModePerm] are rejected: [os.Chmod] drops them, so "17777" +// would silently become 0777. +func ParseFileMode(s string) (fs.FileMode, error) { + mode, err := strconv.ParseUint(strings.TrimPrefix(strings.ToLower(s), "0o"), Base8, BitSize64) + if err != nil || mode > uint64(fs.ModePerm) { + return 0, fmt.Errorf("%w: %q", ErrInvalidFileMode, s) } - return fs.FileMode(mode) //nolint:gosec // file mode is bounded by ParseUint + return fs.FileMode(mode), nil } func EnsureDir(dir string) error { diff --git a/hscontrol/util/util_test.go b/hscontrol/util/util_test.go index 80824d5e9..df2d28b28 100644 --- a/hscontrol/util/util_test.go +++ b/hscontrol/util/util_test.go @@ -2,6 +2,8 @@ package util import ( "errors" + "fmt" + "io/fs" "net/netip" "strings" "testing" @@ -948,3 +950,92 @@ func TestUnmarshalByExt(t *testing.T) { }) } } + +func TestParseFileMode(t *testing.T) { + tests := []struct { + in string + want fs.FileMode + wantErr bool + }{ + {in: "770", want: 0o770}, + {in: "0770", want: 0o770}, + {in: "0o770", want: 0o770}, + {in: "0O770", want: 0o770}, + {in: "0o700", want: 0o700}, + {in: "0o0770", want: 0o770}, + {in: "00770", want: 0o770}, + {in: "0640", want: 0o640}, + {in: "600", want: 0o600}, + {in: "0", want: 0}, + {in: "000", want: 0}, + {in: "0o0", want: 0}, + {in: "7", want: 0o7}, + {in: "0777", want: 0o777}, + {in: "0o777", want: 0o777}, + {in: "", wantErr: true}, + {in: "0o", wantErr: true}, + {in: "not-a-mode", wantErr: true}, + {in: "-770", wantErr: true}, + {in: "0x1f8", wantErr: true}, + {in: "0b111111000", wantErr: true}, + {in: "0o0o770", wantErr: true}, + {in: "0o-770", wantErr: true}, + {in: "+770", wantErr: true}, + {in: "0_770", wantErr: true}, + {in: "778", wantErr: true}, + {in: "0779", wantErr: true}, + {in: " 0770", wantErr: true}, + {in: "0770 ", wantErr: true}, + {in: "0770\n", wantErr: true}, + {in: "rwxrwx---", wantErr: true}, + {in: "u=rwx,g=rwx", wantErr: true}, + {in: "99999999999999999999999", wantErr: true}, + // Bits above 0777 would be dropped by os.Chmod, widening to 0777. + {in: "1770", wantErr: true}, + {in: "01770", wantErr: true}, + {in: "0o1770", wantErr: true}, + {in: "1777", wantErr: true}, + {in: "07777", wantErr: true}, + {in: "17777", wantErr: true}, + {in: "0o1000", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + got, err := ParseFileMode(tt.in) + if tt.wantErr { + if !errors.Is(err, ErrInvalidFileMode) { + t.Fatalf("ParseFileMode(%q) error = %v, want %v", tt.in, err, ErrInvalidFileMode) + } + + return + } + + if err != nil { + t.Fatalf("ParseFileMode(%q) error = %v", tt.in, err) + } + + if got != tt.want { + t.Errorf("ParseFileMode(%q) = %#o, want %#o", tt.in, got, tt.want) + } + }) + } +} + +// Every permission must parse back from each spelling it is commonly written in. +func TestParseFileModeAllPermissions(t *testing.T) { + for m := range fs.ModePerm + 1 { + for _, s := range []string{ + fmt.Sprintf("%o", m), + fmt.Sprintf("%03o", m), + fmt.Sprintf("%04o", m), + fmt.Sprintf("0o%o", m), + fmt.Sprintf("0O%03o", m), + } { + got, err := ParseFileMode(s) + if err != nil || got != m { + t.Errorf("ParseFileMode(%q) = %#o, %v; want %#o", s, got, err, m) + } + } + } +}