From 684e7adec2982f90441c4541e04aea4f3326aa64 Mon Sep 17 00:00:00 2001 From: Mara Nikola Kiefer <8320933+mnkiefer@users.noreply.github.com> Date: Sat, 7 Nov 2026 09:00:31 +0100 Subject: [PATCH] Extract token broker policy parsing with diagnostic logging Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- server/internal/tokenbroker/policy.go | 60 ++++++++-- .../internal/tokenbroker/policy_rules_test.go | 103 ++++++++++++++++++ 2 files changed, 154 insertions(+), 9 deletions(-) create mode 100644 server/internal/tokenbroker/policy_rules_test.go diff --git a/server/internal/tokenbroker/policy.go b/server/internal/tokenbroker/policy.go index ba088e5c29..956eee8363 100644 --- a/server/internal/tokenbroker/policy.go +++ b/server/internal/tokenbroker/policy.go @@ -2,14 +2,18 @@ package tokenbroker import ( "encoding/json" + "errors" "io" "os" "regexp" "strings" "github.com/githubnext/gh-cao/server/internal/admission" + "github.com/githubnext/gh-cao/server/internal/logger" ) +var brokerLog = logger.New("cao:tokenbroker") + var ( stableID = regexp.MustCompile(`^[1-9][0-9]{0,19}$`) revision = regexp.MustCompile(`^[a-f0-9]{40}$`) @@ -53,14 +57,44 @@ func (s *Service) authorized(i admission.Identity) bool { } // Read on every operation. A changed or removed mounted policy immediately // denies authority instead of retaining a stale authorization snapshot. - f, err := os.Open(s.config.PolicyFile) + data, err := readPolicyFile(s.config.PolicyFile) + if err != nil { + brokerLog.Printf("policy denied: reason=unreadable") + return false + } + rules, err := parseWorkflowRules(data) if err != nil { + brokerLog.Printf("policy denied: reason=invalid_policy") + return false + } + if !ruleAllows(rules, i) { + brokerLog.Printf("policy denied: reason=no_matching_rule rules=%d", len(rules)) return false } + return true +} + +var errPolicy = errors.New("invalid token broker policy") + +// readPolicyFile reads the mounted policy, rejecting files above the 1 MiB bound. +func readPolicyFile(path string) ([]byte, error) { + f, err := os.Open(path) + if err != nil { + return nil, err + } defer func() { _ = f.Close() }() data, err := io.ReadAll(io.LimitReader(f, (1<<20)+1)) - if err != nil || len(data) > 1<<20 || uniqueJSON(data) != nil { - return false + if err != nil || len(data) > 1<<20 { + return nil, errPolicy + } + return data, nil +} + +// parseWorkflowRules strictly decodes the copilot-requests workflow rules and +// fails closed on duplicate members, unknown shapes, duplicates, or unsafe values. +func parseWorkflowRules(data []byte) ([]workflowRule, error) { + if uniqueJSON(data) != nil { + return nil, errPolicy } var root, control, broker, requests map[string]json.RawMessage var workflows []json.RawMessage @@ -69,9 +103,9 @@ func (s *Service) authorized(i admission.Identity) bool { json.Unmarshal(control["token-broker"], &broker) != nil || len(broker) != 1 || json.Unmarshal(broker["copilot-requests"], &requests) != nil || len(requests) != 1 || json.Unmarshal(requests["workflows"], &workflows) != nil || len(workflows) > 128 { - return false + return nil, errPolicy } - allowed := false + rules := make([]workflowRule, 0, len(workflows)) seen := make(map[workflowRule]bool, len(workflows)) for _, raw := range workflows { var rule workflowRule @@ -80,19 +114,27 @@ func (s *Service) authorized(i admission.Identity) bool { fields["repository-id"] == nil || fields["owner-id"] == nil || fields["workflow-ref"] == nil || fields["workflow-sha"] == nil || json.Unmarshal(raw, &rule) != nil || seen[rule] { - return false + return nil, errPolicy } seen[rule] = true if !stableID.MatchString(rule.RepositoryID) || !stableID.MatchString(rule.OwnerID) || !validWorkflowRef(rule.WorkflowRef) || !revision.MatchString(rule.WorkflowSHA) { - return false + return nil, errPolicy } + rules = append(rules, rule) + } + return rules, nil +} + +// ruleAllows reports whether an exact rule matches the verified identity. +func ruleAllows(rules []workflowRule, i admission.Identity) bool { + for _, rule := range rules { if rule.RepositoryID == i.RepositoryID && rule.OwnerID == i.OwnerID && rule.WorkflowRef == i.WorkflowRef && rule.WorkflowSHA == i.WorkflowSHA { - allowed = true + return true } } - return allowed + return false } func validWorkflowRef(value string) bool { diff --git a/server/internal/tokenbroker/policy_rules_test.go b/server/internal/tokenbroker/policy_rules_test.go new file mode 100644 index 0000000000..1c12dc62d5 --- /dev/null +++ b/server/internal/tokenbroker/policy_rules_test.go @@ -0,0 +1,103 @@ +package tokenbroker + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/githubnext/gh-cao/server/internal/admission" +) + +const ( + testRuleRef = "acme/project/.github/workflows/triage.lock.yml@refs/heads/main" + testRuleSHA = "0123456789abcdef0123456789abcdef01234567" +) + +func rulesJSON(rule string) []byte { + return []byte(`{"control-plane":{"token-broker":{"copilot-requests":{"workflows":[` + rule + `]}}}}`) +} + +func validRule(repo string) string { + return `{"repository-id":"` + repo + `","owner-id":"7","workflow-ref":"` + testRuleRef + `","workflow-sha":"` + testRuleSHA + `"}` +} + +func TestParseWorkflowRulesAcceptsValidPolicy(t *testing.T) { + rules, err := parseWorkflowRules(rulesJSON(validRule("1") + "," + validRule("2"))) + if err != nil || len(rules) != 2 || rules[1].RepositoryID != "2" { + t.Fatalf("rules=%v err=%v", rules, err) + } + rules, err = parseWorkflowRules(rulesJSON("")) + if err != nil || len(rules) != 0 { + t.Fatalf("empty rules=%v err=%v", rules, err) + } +} + +func TestParseWorkflowRulesRejectsUnsafePolicy(t *testing.T) { + many := strings.TrimSuffix(strings.Repeat(validRule("1")+",", 129), ",") + for name, data := range map[string][]byte{ + "duplicate rule": rulesJSON(validRule("1") + "," + validRule("1")), + "too many": rulesJSON(many), + "wildcard repo": rulesJSON(validRule("*")), + "missing field": rulesJSON(`{"repository-id":"1","owner-id":"7","workflow-ref":"` + testRuleRef + `"}`), + "not an object": []byte(`[]`), + "duplicate field": []byte(`{"control-plane":{},"control-plane":{}}`), + "invalid UTF-8": {0xff}, + } { + if rules, err := parseWorkflowRules(data); err == nil || rules != nil { + t.Fatalf("%s: accepted rules=%v", name, rules) + } + } +} + +func TestRuleAllowsRequiresExactMatch(t *testing.T) { + rules, err := parseWorkflowRules(rulesJSON(validRule("1"))) + if err != nil { + t.Fatal(err) + } + id := admission.Identity{RepositoryID: "1", OwnerID: "7", WorkflowRef: testRuleRef, WorkflowSHA: testRuleSHA} + if !ruleAllows(rules, id) { + t.Fatal("exact identity denied") + } + for name, edit := range map[string]func(*admission.Identity){ + "repository": func(i *admission.Identity) { i.RepositoryID = "2" }, + "owner": func(i *admission.Identity) { i.OwnerID = "8" }, + "ref": func(i *admission.Identity) { i.WorkflowRef += "x" }, + "sha": func(i *admission.Identity) { i.WorkflowSHA = strings.Repeat("a", 40) }, + } { + changed := id + edit(&changed) + if ruleAllows(rules, changed) { + t.Fatalf("%s mismatch authorized", name) + } + } + if ruleAllows(nil, id) { + t.Fatal("empty rule set authorized") + } +} + +func TestReadPolicyFileBounds(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "policy.json") + if _, err := readPolicyFile(path); err == nil { + t.Fatal("missing file accepted") + } + if err := os.WriteFile(path, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + if data, err := readPolicyFile(path); err != nil || string(data) != "{}" { + t.Fatalf("data=%q err=%v", data, err) + } + if err := os.WriteFile(path, make([]byte, (1<<20)+1), 0o600); err != nil { + t.Fatal(err) + } + if _, err := readPolicyFile(path); err == nil { + t.Fatal("oversized policy accepted") + } + if err := os.WriteFile(path, make([]byte, 1<<20), 0o600); err != nil { + t.Fatal(err) + } + if _, err := readPolicyFile(path); err != nil { + t.Fatalf("policy at the bound rejected: %v", err) + } +}