From 079dca89242e3a5c85ef19da09c6e98f1ffbe8c3 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 13 May 2026 10:11:55 +0000 Subject: [PATCH] policy/v2: reject non-ASCII tag names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SaaS rejects tag names whose first character after `tag:` is not an ASCII letter (`[a-zA-Z]`) with `tagOwners["tag:X"]: tag names must start with a letter, after 'tag:'`. headscale was accepting any UTF-8. Tighten Tag.Validate to enforce the first-character rule and reshape the surfaced error in unmarshalPolicy so the tagOwners key appears in the SaaS-style prefix. Subsequent characters remain unconstrained — only the leading byte is checked. --- .../v2/tailscale_ssh_data_compat_test.go | 8 -- hscontrol/policy/v2/types.go | 32 +++++++- hscontrol/policy/v2/types_test.go | 76 +++++++++++++++++++ 3 files changed, 105 insertions(+), 11 deletions(-) diff --git a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go index 54d908bd..6fb5153e 100644 --- a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go +++ b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go @@ -107,14 +107,6 @@ var sshRejectSkipReasons = map[string]string{ "ssh-e2": "domain validation: headscale has no 'associated tailnet domains' concept", "ssh-malformed-user-localpart-multi-glob": "domain validation: headscale has no 'associated tailnet domains' concept (same gap as ssh-b4/d1/e1/e2)", - // NON_ASCII_TAG_NAME (1 test) - // - // SaaS rejects non-ASCII characters in tag names with - // `tag names must start with a letter, after 'tag:'`. headscale's - // tag parser accepts arbitrary UTF-8. Tighten the tag-name - // validator to match SaaS's letter-only rule. - "ssh-unicode-cyrillic-tag": "SaaS rejects non-ASCII tag names, headscale accepts", - // GROUP_NESTING_ERROR_BODY (3 tests) // // SaaS rejects any group-in-group reference (cycle, chain, diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index b43415c9..0e0a78f7 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -53,6 +53,7 @@ var ( ErrSSHActionMustBeSpecified = errors.New("action must be specified") ErrSSHActionInvalid = errors.New("is not a valid action") ErrSSHDestinationHostAlias = errors.New("invalid dst") + ErrTagNameMustStartWithLetter = errors.New("tag names must start with a letter, after 'tag:'") ) // SSH check period constants per Tailscale docs: @@ -545,12 +546,27 @@ func (g *Group) resolve(p *Policy, users types.Users, nodes views.Slice[types.No // Tag is a special string which is always prefixed with `tag:`. type Tag string +// Validate enforces the `tag:` prefix and the SaaS rule that the +// first character after the prefix is an ASCII letter ([A-Za-z]). +// Subsequent characters may be ASCII letters, digits, hyphens, or +// dots — those are checked by the existing alias parser elsewhere. func (t *Tag) Validate() error { - if isTag(string(*t)) { - return nil + s := string(*t) + if !isTag(s) { + return fmt.Errorf("%w, got: %q", ErrInvalidTagFormat, *t) } - return fmt.Errorf("%w, got: %q", ErrInvalidTagFormat, *t) + rest := strings.TrimPrefix(s, "tag:") + if rest == "" { + return ErrTagNameMustStartWithLetter + } + + first := rest[0] + if !((first >= 'a' && first <= 'z') || (first >= 'A' && first <= 'Z')) { + return ErrTagNameMustStartWithLetter + } + + return nil } func (t *Tag) UnmarshalJSON(b []byte) error { @@ -3268,6 +3284,16 @@ func unmarshalPolicy(b []byte) (*Policy, error) { if strings.Contains(string(serr.JSONPointer), "/via/") { return nil, ErrGrantViaNotATag } + + // Non-ASCII tag-name failures surface from Tag.Validate + // at unmarshal time. Reshape the error to mirror SaaS + // (`tagOwners["tag:X"]: …`). + if errors.Is(serr.Err, ErrTagNameMustStartWithLetter) { + ptr := serr.JSONPointer + name := ptr.LastToken() + + return nil, fmt.Errorf("tagOwners[%q]: %w", name, ErrTagNameMustStartWithLetter) + } } return nil, fmt.Errorf("parsing policy from bytes: %w", err) diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index 46a75131..a46738b7 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -4893,6 +4893,82 @@ func TestAliasEncUnmarshalTrim(t *testing.T) { } } +// TestTagValidateFirstCharLetter exercises the SaaS rule that the +// first character after `tag:` must be an ASCII letter. Digits, +// punctuation, and non-ASCII Unicode letters are rejected with the +// same body SaaS produces. Subsequent characters are unconstrained. +func TestTagValidateFirstCharLetter(t *testing.T) { + tests := []struct { + name string + tag Tag + wantErr error + }{ + { + name: "ascii lowercase letter", + tag: Tag("tag:server"), + }, + { + name: "ascii uppercase letter", + tag: Tag("tag:Server"), + }, + { + name: "ascii letter then digit", + tag: Tag("tag:a1"), + }, + { + name: "leading digit rejected", + tag: Tag("tag:1server"), + wantErr: ErrTagNameMustStartWithLetter, + }, + { + name: "leading hyphen rejected", + tag: Tag("tag:-server"), + wantErr: ErrTagNameMustStartWithLetter, + }, + { + name: "cyrillic letter rejected", + tag: Tag("tag:сервер"), + wantErr: ErrTagNameMustStartWithLetter, + }, + { + name: "empty name rejected", + tag: Tag("tag:"), + wantErr: ErrTagNameMustStartWithLetter, + }, + { + name: "missing prefix rejected", + tag: Tag("server"), + wantErr: ErrInvalidTagFormat, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.tag.Validate() + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + + return + } + + require.NoError(t, err) + }) + } +} + +// TestUnmarshalPolicyCyrillicTagOwner verifies the full SaaS body +// (`tagOwners["tag:сервер"]: …`) surfaces when a non-ASCII tag +// appears as a tagOwners key. +func TestUnmarshalPolicyCyrillicTagOwner(t *testing.T) { + policy := []byte(`{"tagOwners": {"tag:сервер": ["odin@example.com"]}}`) + + _, err := unmarshalPolicy(policy) + require.Error(t, err) + require.ErrorIs(t, err, ErrTagNameMustStartWithLetter) + require.Contains(t, err.Error(), + `tagOwners["tag:сервер"]: tag names must start with a letter, after 'tag:'`) +} + // TestSSHCheckPeriodInvalidDuration verifies the SaaS body for the // malformed-duration case (`time: invalid duration "abc"`). func TestSSHCheckPeriodInvalidDuration(t *testing.T) {