From 429004d97858470a0163298ab88b134dfa6202d0 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Fri, 16 Oct 2026 10:23:41 +0100 Subject: [PATCH 1/2] ate-setup: support bootstrap owners that contain a comma Signed-off-by: Eric Curtin --- cmd/ate-setup/internal/config/config.go | 6 +- cmd/ate-setup/internal/config/config_test.go | 42 +++++--- cmd/ate-setup/internal/config/load.go | 14 ++- cmd/ate-setup/internal/config/setting.go | 3 +- cmd/ate-setup/internal/steps/apienv.go | 23 ++-- cmd/ate-setup/internal/steps/apienv_test.go | 42 ++++++-- docs/authorization.md | 4 + internal/principal/list.go | 60 +++++++++++ internal/principal/list_test.go | 105 +++++++++++++++++++ 9 files changed, 254 insertions(+), 45 deletions(-) create mode 100644 internal/principal/list.go create mode 100644 internal/principal/list_test.go diff --git a/cmd/ate-setup/internal/config/config.go b/cmd/ate-setup/internal/config/config.go index ab39897cf8..76861df4fc 100644 --- a/cmd/ate-setup/internal/config/config.go +++ b/cmd/ate-setup/internal/config/config.go @@ -117,9 +117,9 @@ type Config struct { // follows neither form. ExpectedJWTIssuer string - // APIAuthzBootstrapOwners is the comma-separated principal list granted - // owner on global:root (ATE_API_AUTHZ_BOOTSTRAP_OWNERS). - APIAuthzBootstrapOwners string + // APIAuthzBootstrapOwners are the principals granted owner on global:root + // (ATE_API_AUTHZ_BOOTSTRAP_OWNERS, parsed by principal.ParseList). + APIAuthzBootstrapOwners []string // ActorJWTAlgorithm is the signing algorithm of the key in a new actor JWT // pool (ACTOR_JWT_ALGORITHM): ES256 or RS256. diff --git a/cmd/ate-setup/internal/config/config_test.go b/cmd/ate-setup/internal/config/config_test.go index 99ae080bbd..fcc3724c83 100644 --- a/cmd/ate-setup/internal/config/config_test.go +++ b/cmd/ate-setup/internal/config/config_test.go @@ -698,34 +698,46 @@ func TestLoadAPILogLevel(t *testing.T) { } func TestLoadAPIAuthz(t *testing.T) { - const ( - clientOwner = "system:serviceaccount:ate-system:ate-client" - owners = "alice@example.com,bob@example.com" - ) + const clientOwner = "system:serviceaccount:ate-system:ate-client" for _, tc := range []struct { name string env map[string]string - wantOwners string - wantErr bool + wantOwners []string + wantErr string }{ { name: "owned by the client ServiceAccount by default", - wantOwners: clientOwner, + wantOwners: []string{clientOwner}, }, { name: "default owner follows the namespace", env: map[string]string{"ATE_NAMESPACE": "custom"}, - wantOwners: "system:serviceaccount:custom:ate-client", + wantOwners: []string{"system:serviceaccount:custom:ate-client"}, }, { name: "explicit owners replace the default", - env: map[string]string{"ATE_API_AUTHZ_BOOTSTRAP_OWNERS": owners}, - wantOwners: owners, + env: map[string]string{"ATE_API_AUTHZ_BOOTSTRAP_OWNERS": "alice@example.com,bob@example.com"}, + wantOwners: []string{"alice@example.com", "bob@example.com"}, + }, + { + name: "whitespace around owners is ignored", + env: map[string]string{"ATE_API_AUTHZ_BOOTSTRAP_OWNERS": " alice , bob "}, + wantOwners: []string{"alice", "bob"}, + }, + { + name: "an owner that contains a comma is quoted", + env: map[string]string{"ATE_API_AUTHZ_BOOTSTRAP_OWNERS": `"Doe, Jane", bob`}, + wantOwners: []string{"Doe, Jane", "bob"}, }, { name: "only separators", env: map[string]string{"ATE_API_AUTHZ_BOOTSTRAP_OWNERS": " , "}, - wantErr: true, + wantErr: "bootstrap owner", + }, + { + name: "an unterminated quote", + env: map[string]string{"ATE_API_AUTHZ_BOOTSTRAP_OWNERS": `"alice`}, + wantErr: "ATE_API_AUTHZ_BOOTSTRAP_OWNERS", }, } { t.Run(tc.name, func(t *testing.T) { @@ -734,16 +746,16 @@ func TestLoadAPIAuthz(t *testing.T) { t.Setenv(name, value) } cfg, err := Load(Options{}) - if tc.wantErr { - if err == nil || !strings.Contains(err.Error(), "bootstrap owner") { - t.Fatalf("Load() error = %v, want the missing bootstrap owner error", err) + if tc.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("Load() error = %v, want it to contain %q", err, tc.wantErr) } return } if err != nil { t.Fatalf("Load() error = %v", err) } - if cfg.APIAuthzBootstrapOwners != tc.wantOwners { + if !slices.Equal(cfg.APIAuthzBootstrapOwners, tc.wantOwners) { t.Errorf("APIAuthzBootstrapOwners = %q, want %q", cfg.APIAuthzBootstrapOwners, tc.wantOwners) } }) diff --git a/cmd/ate-setup/internal/config/load.go b/cmd/ate-setup/internal/config/load.go index a85f7d64d5..873e176c96 100644 --- a/cmd/ate-setup/internal/config/load.go +++ b/cmd/ate-setup/internal/config/load.go @@ -28,6 +28,7 @@ import ( "go.ate.dev/substrate/internal/actoridjwt" "go.ate.dev/substrate/internal/installdefaults" "go.ate.dev/substrate/internal/preview" + "go.ate.dev/substrate/internal/principal" ) // ConfigPathEnv names the configuration document when --config is not given. @@ -214,9 +215,14 @@ func buildConfig(root string, env map[string]string, r *Resolved) (*Config, erro // tokens minted from the client ServiceAccount, so making it the default // bootstrap owner keeps them working. The deploy warns whenever it is a // bootstrap owner. - bootstrapOwners := r.String("ateapi.authz.bootstrapOwners") - if bootstrapOwners == "" { - bootstrapOwners = installdefaults.ClientPrincipal(r.String("namespace")) + bootstrapOwners := []string{installdefaults.ClientPrincipal(r.String("namespace"))} + if raw := r.String("ateapi.authz.bootstrapOwners"); raw != "" { + owners, err := principal.ParseList(raw) + if err != nil { + v, _ := r.Value("ateapi.authz.bootstrapOwners") + return nil, fmt.Errorf("%s: %w (from %s)", v.Setting.Key, err, v.From.Describe(v.Setting)) + } + bootstrapOwners = owners } cfg := &Config{ @@ -468,7 +474,7 @@ func validateResolved(cfg *Config, r *Resolved) error { } // ate-api-server exits without a bootstrap owner, so catch it before // deploying. - if strings.Trim(cfg.APIAuthzBootstrapOwners, " ,") == "" { + if len(cfg.APIAuthzBootstrapOwners) == 0 { owners, _ := r.Value("ateapi.authz.bootstrapOwners") return &InvalidError{Value: owners, Want: "a list of at least one bootstrap owner"} } diff --git a/cmd/ate-setup/internal/config/setting.go b/cmd/ate-setup/internal/config/setting.go index 389da4d55c..4ea6dc3552 100644 --- a/cmd/ate-setup/internal/config/setting.go +++ b/cmd/ate-setup/internal/config/setting.go @@ -184,7 +184,8 @@ var Registry = []Setting{ { Key: "ateapi.authz.bootstrapOwners", Env: "ATE_API_AUTHZ_BOOTSTRAP_OWNERS", Flag: "ateapi-authz-bootstrap-owners", Kind: KindString, - Usage: "Comma-separated principals that always hold owner on global:root. " + + Usage: "Comma-separated principals that always hold owner on global:root; " + + `quote a principal that contains a comma, as in '"Doe, Jane",bob'. ` + "Unset means kubectl-ate's ServiceAccount, system:serviceaccount::ate-client", }, { diff --git a/cmd/ate-setup/internal/steps/apienv.go b/cmd/ate-setup/internal/steps/apienv.go index 481f72de1e..802021ea89 100644 --- a/cmd/ate-setup/internal/steps/apienv.go +++ b/cmd/ate-setup/internal/steps/apienv.go @@ -30,6 +30,7 @@ import ( "go.ate.dev/substrate/cmd/ate-setup/internal/log" "go.ate.dev/substrate/internal/installdefaults" "go.ate.dev/substrate/internal/preview" + "go.ate.dev/substrate/internal/principal" "go.ate.dev/substrate/internal/resources" ) @@ -169,17 +170,15 @@ func (e *Env) UpdateAPIServerEnvVars(ctx context.Context) error { // It is the default so demos, benchmarks and e2e work, but everyone who can // mint its token, including every kubectl-ate user without --token-file, is // then a global owner. -func sharedBootstrapOwnerWarning(bootstrapOwners, namespace string) string { +func sharedBootstrapOwnerWarning(bootstrapOwners []string, namespace string) string { shared := installdefaults.ClientPrincipal(namespace) - for owner := range strings.SplitSeq(bootstrapOwners, ",") { - if strings.TrimSpace(owner) == shared { - return fmt.Sprintf("%s is a bootstrap owner, so anyone who can mint a token for it is a global owner of this install. "+ - "It is the default so the demos, benchmarks and e2e tests work; for a shared or production install, "+ - "set --ateapi-authz-bootstrap-owners (ATE_API_AUTHZ_BOOTSTRAP_OWNERS) to your operators' own principals. "+ - "See docs/authorization.md.", shared) - } + if !slices.Contains(bootstrapOwners, shared) { + return "" } - return "" + return fmt.Sprintf("%s is a bootstrap owner, so anyone who can mint a token for it is a global owner of this install. "+ + "It is the default so the demos, benchmarks and e2e tests work; for a shared or production install, "+ + "set --ateapi-authz-bootstrap-owners (ATE_API_AUTHZ_BOOTSTRAP_OWNERS) to your operators' own principals. "+ + "See docs/authorization.md.", shared) } // clusterSnapshotLocation validates location and returns it as ate-api-server's @@ -367,7 +366,7 @@ const apiServerAuthzFlag = "--authz-bootstrap-owners" // setAPIServerAuthzArgs sets ate-api-server's bootstrap owners flag on its // container in objs, replacing any the manifest already carries. A changed // flag changes the pod template, so it rolls the Deployment. -func setAPIServerAuthzArgs(objs []*unstructured.Unstructured, bootstrapOwners string) error { +func setAPIServerAuthzArgs(objs []*unstructured.Unstructured, bootstrapOwners []string) error { var dep *unstructured.Unstructured for _, obj := range objs { if obj.GetKind() == "Deployment" && obj.GetName() == "ate-api-server" { @@ -394,8 +393,8 @@ func setAPIServerAuthzArgs(objs []*unstructured.Unstructured, bootstrapOwners st args = slices.DeleteFunc(args, func(arg string) bool { return arg == apiServerAuthzFlag || strings.HasPrefix(arg, apiServerAuthzFlag+"=") }) - if strings.Trim(bootstrapOwners, " ,") != "" { - args = append(args, apiServerAuthzFlag+"="+bootstrapOwners) + if len(bootstrapOwners) > 0 { + args = append(args, apiServerAuthzFlag+"="+principal.FormatList(bootstrapOwners)) } if err := unstructured.SetNestedStringSlice(container, args, "args"); err != nil { return fmt.Errorf("%s: %w", kube.Describe(dep), err) diff --git a/cmd/ate-setup/internal/steps/apienv_test.go b/cmd/ate-setup/internal/steps/apienv_test.go index 3c97dea3dc..cd7ee05af8 100644 --- a/cmd/ate-setup/internal/steps/apienv_test.go +++ b/cmd/ate-setup/internal/steps/apienv_test.go @@ -19,6 +19,8 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/spf13/pflag" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -648,17 +650,25 @@ spec: ` for _, tc := range []struct { name string - owners string + owners []string want []string }{ { name: "replaces the owners", - owners: "alice,bob", + owners: []string{"alice", "bob"}, want: []string{ "--config-file=/run/apiconfig.ate.dev/apiconfig.textproto", "--authz-bootstrap-owners=alice,bob", }, }, + { + name: "an owner that contains a comma or a quote is quoted", + owners: []string{"Doe, Jane", `say "hi"`, "bob"}, + want: []string{ + "--config-file=/run/apiconfig.ate.dev/apiconfig.textproto", + `--authz-bootstrap-owners="Doe, Jane","say ""hi""",bob`, + }, + }, { name: "no owners drops the flag", want: []string{"--config-file=/run/apiconfig.ate.dev/apiconfig.textproto"}, @@ -680,6 +690,17 @@ spec: if diff := cmp.Diff(tc.want, argsOf(1)); diff != "" { t.Errorf("ate-api-server args (-want +got):\n%s", diff) } + // ate-api-server reads the flag as a StringSlice, so it must come + // back as the owners that were set. + fs := pflag.NewFlagSet("ate-api-server", pflag.ContinueOnError) + fs.ParseErrorsAllowlist.UnknownFlags = true + owners := fs.StringSlice("authz-bootstrap-owners", nil, "") + if err := fs.Parse(argsOf(1)); err != nil { + t.Fatalf("parsing ate-api-server args: %v", err) + } + if diff := cmp.Diff(tc.owners, *owners, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("--authz-bootstrap-owners as ate-api-server parses it (-want +got):\n%s", diff) + } if diff := cmp.Diff([]string{"--keep"}, argsOf(0)); diff != "" { t.Errorf("other container's args changed (-want +got):\n%s", diff) } @@ -690,7 +711,7 @@ spec: if err != nil { t.Fatalf("DecodeManifestBytes() error = %v", err) } - if err := setAPIServerAuthzArgs(objs, "alice"); err == nil { + if err := setAPIServerAuthzArgs(objs, []string{"alice"}); err == nil { t.Error("setAPIServerAuthzArgs() without deployment/ate-api-server error = nil, want an error") } } @@ -700,7 +721,7 @@ func TestCreateAPIServerEnvVarsOmitsAuthz(t *testing.T) { TransferGSAName: testTransferGSAName, ProjectID: testProjectID, PostgresReadWriteRole: config.DefaultPostgresReadWriteRole, PostgresOwnerRole: config.DefaultPostgresOwnerRole, - APIAuthzBootstrapOwners: "system:serviceaccount:ate-system:ate-client", + APIAuthzBootstrapOwners: []string{"system:serviceaccount:ate-system:ate-client"}, SnapshotLocation: "gs://ate-snapshots/", }, Kube: fakeKube(t, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: NamespaceAteSystem}}, @@ -725,14 +746,15 @@ func TestSharedBootstrapOwnerWarning(t *testing.T) { const shared = "system:serviceaccount:ate-system:ate-client" for _, tc := range []struct { name string - owners string + owners []string warn bool }{ - {name: "default owner", owners: shared, warn: true}, - {name: "listed among others", owners: "alice, " + shared + " ,bob", warn: true}, - {name: "own principals only", owners: "alice,bob", warn: false}, - {name: "another namespace's client", owners: "system:serviceaccount:other:ate-client", warn: false}, - {name: "prefix is not a match", owners: shared + "-x", warn: false}, + {name: "default owner", owners: []string{shared}, warn: true}, + {name: "listed among others", owners: []string{"alice", shared, "bob"}, warn: true}, + {name: "own principals only", owners: []string{"alice", "bob"}, warn: false}, + {name: "another namespace's client", owners: []string{"system:serviceaccount:other:ate-client"}, warn: false}, + {name: "prefix is not a match", owners: []string{shared + "-x"}, warn: false}, + {name: "part of a principal that contains a comma", owners: []string{"alice," + shared}, warn: false}, } { t.Run(tc.name, func(t *testing.T) { got := sharedBootstrapOwnerWarning(tc.owners, "ate-system") diff --git a/docs/authorization.md b/docs/authorization.md index 8dbf7b1ffa..3275233131 100644 --- a/docs/authorization.md +++ b/docs/authorization.md @@ -34,6 +34,10 @@ rolls `ate-api-server`. |---|---|---| | `ateapi.authz.bootstrapOwners` | `ATE_API_AUTHZ_BOOTSTRAP_OWNERS` | `alice@example.com,bob@example.com` | +The owners are a comma-separated list. Put a principal that contains a comma +or a double quote in double quotes, and double any quote inside it: +`"Doe, Jane",bob@example.com`. + If you don't set `ateapi.authz.bootstrapOwners`, `ate-setup` uses the shared `ate-client` ServiceAccount (`system:serviceaccount::ate-client`), so the demos, benchmarks and e2e tests work. See the warning at the top of diff --git a/internal/principal/list.go b/internal/principal/list.go new file mode 100644 index 0000000000..97d1feed37 --- /dev/null +++ b/internal/principal/list.go @@ -0,0 +1,60 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package principal + +import ( + "bytes" + "encoding/csv" + "fmt" + "strings" +) + +// ParseList parses a comma-separated list of principals. It reads CSV, as a +// pflag StringSlice does, so a principal that contains a comma or a double +// quote goes in double quotes, with each embedded quote doubled: +// +// alice, "Doe, Jane", "say ""hi""" +// +// Whitespace around a principal is ignored and empty entries are dropped, so +// an empty list or one of only separators is no principals. +func ParseList(s string) ([]string, error) { + r := csv.NewReader(strings.NewReader(s)) + r.TrimLeadingSpace = true + r.FieldsPerRecord = -1 + records, err := r.ReadAll() + if err != nil { + return nil, fmt.Errorf("invalid principal list, quote a principal that contains a comma or a double quote: %w", err) + } + var principals []string + for _, record := range records { + for _, p := range record { + if p = strings.TrimSpace(p); p != "" { + principals = append(principals, p) + } + } + } + return principals, nil +} + +// FormatList renders principals as a list ParseList and a pflag StringSlice +// read back as the same principals. +func FormatList(principals []string) string { + var b bytes.Buffer + w := csv.NewWriter(&b) + // A bytes.Buffer does not fail to write. + _ = w.Write(principals) + w.Flush() + return strings.TrimSuffix(b.String(), "\n") +} diff --git a/internal/principal/list_test.go b/internal/principal/list_test.go new file mode 100644 index 0000000000..cbbce2d32f --- /dev/null +++ b/internal/principal/list_test.go @@ -0,0 +1,105 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package principal + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/spf13/pflag" +) + +func TestParseList(t *testing.T) { + for _, tc := range []struct { + name string + in string + want []string + }{ + {name: "empty", in: ""}, + {name: "only separators", in: " , ,"}, + {name: "one", in: "alice", want: []string{"alice"}}, + {name: "several", in: "alice,bob", want: []string{"alice", "bob"}}, + {name: "whitespace around entries", in: " alice ,\tbob ", want: []string{"alice", "bob"}}, + {name: "empty entries dropped", in: "alice,,bob,", want: []string{"alice", "bob"}}, + {name: "comma in quotes", in: `"Doe, Jane",bob`, want: []string{"Doe, Jane", "bob"}}, + {name: "whitespace before a quoted entry", in: `alice, "Doe, Jane"`, want: []string{"alice", "Doe, Jane"}}, + {name: "doubled quote", in: `"say ""hi""",bob`, want: []string{`say "hi"`, "bob"}}, + {name: "quoted without a comma", in: `"alice",bob`, want: []string{"alice", "bob"}}, + {name: "service account", in: "system:serviceaccount:ate-system:ate-client", want: []string{"system:serviceaccount:ate-system:ate-client"}}, + {name: "newline separates", in: "alice\nbob", want: []string{"alice", "bob"}}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := ParseList(tc.in) + if err != nil { + t.Fatalf("ParseList(%q) error = %v", tc.in, err) + } + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("ParseList(%q) (-want +got):\n%s", tc.in, diff) + } + }) + } +} + +func TestParseListInvalid(t *testing.T) { + for _, in := range []string{ + `"alice`, // unterminated quote + `ali"ce`, // quote inside an unquoted entry + `"alice"x`, // text after the closing quote + } { + if got, err := ParseList(in); err == nil { + t.Errorf("ParseList(%q) = %q, want an error", in, got) + } + } +} + +// A list ate-setup renders must reach ate-api-server's StringSlice flag as the +// principals it was built from. +func TestFormatList(t *testing.T) { + for _, tc := range []struct { + name string + in []string + want string + }{ + {name: "none"}, + {name: "plain", in: []string{"alice", "bob"}, want: "alice,bob"}, + {name: "comma", in: []string{"Doe, Jane", "bob"}, want: `"Doe, Jane",bob`}, + {name: "quote", in: []string{`say "hi"`, "bob"}, want: `"say ""hi""",bob`}, + } { + t.Run(tc.name, func(t *testing.T) { + got := FormatList(tc.in) + if got != tc.want { + t.Errorf("FormatList(%q) = %q, want %q", tc.in, got, tc.want) + } + + parsed, err := ParseList(got) + if err != nil { + t.Fatalf("ParseList(%q) error = %v", got, err) + } + if diff := cmp.Diff(tc.in, parsed); diff != "" { + t.Errorf("ParseList(FormatList(%q)) (-want +got):\n%s", tc.in, diff) + } + + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + owners := fs.StringSlice("owners", nil, "") + if err := fs.Parse([]string{"--owners=" + got}); err != nil { + t.Fatalf("flag parse error = %v", err) + } + if diff := cmp.Diff(tc.in, *owners, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("StringSlice of FormatList(%q) (-want +got):\n%s", tc.in, diff) + } + }) + } +} From 30c185c84cd782d2435d35489575fac5ad7074c5 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Sun, 8 Nov 2026 12:13:13 +0000 Subject: [PATCH 2/2] Use Agent Substrate Authors copyright Signed-off-by: Eric Curtin --- internal/principal/list.go | 2 +- internal/principal/list_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/principal/list.go b/internal/principal/list.go index 97d1feed37..4b2265e9cb 100644 --- a/internal/principal/list.go +++ b/internal/principal/list.go @@ -1,4 +1,4 @@ -// Copyright 2026 Google LLC +// Copyright 2026 The Agent Substrate Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/internal/principal/list_test.go b/internal/principal/list_test.go index cbbce2d32f..ff7cb8d35e 100644 --- a/internal/principal/list_test.go +++ b/internal/principal/list_test.go @@ -1,4 +1,4 @@ -// Copyright 2026 Google LLC +// Copyright 2026 The Agent Substrate Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License.