feat(auth): add trusted header authentication - #16615
Conversation
Signed-off-by: Sagitra <sagitrapradeep2006@gmail.com>
Signed-off-by: Sagitra <sagitrapradeep2006@gmail.com>
Signed-off-by: Sagitra <sagitrapradeep2006@gmail.com>
Signed-off-by: Sagitra <sagitrapradeep2006@gmail.com>
Signed-off-by: Sagitra <sagitrapradeep2006@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesAdded configurable trusted-header authentication. Claims can come from metadata headers or static values, with optional group splitting. Gatekeeper now evaluates SSO and client credentials before header authentication, then applies shared RBAC handling. Server wiring, CLI documentation, mocks, and tests were updated. Trusted header authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Gatekeeper
participant sso.Interface
participant ClientCredentials
participant header.Interface
participant RBAC
Client->>Gatekeeper: Send authorization metadata and forwarded headers
Gatekeeper->>sso.Interface: Authorize SSO credentials
Gatekeeper->>ClientCredentials: Authorize client credentials
Gatekeeper->>header.Interface: Authorize forwarded headers
header.Interface-->>Gatekeeper: Return claims
Gatekeeper->>RBAC: Resolve claims permissions
RBAC-->>Client: Return authentication result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/apiserver/argoserver.go (1)
155-186: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winLoad the config once and validate the header configuration.
configController.Get(ctx)now runs twice when bothheaderandssomodes are enabled. Hoist a single load. More importantly, nothing verifies thatc.Headeris populated when Header mode is on. An operator who passes--auth-mode=headerbut omits theheaderconfig block starts a server whose provider resolves no claim at all. Fail at startup instead.♻️ Proposed fix
ssoIf := sso.NullSSO headerIf := header.New(config.HeaderConfig{}) + + var c *config.Config + if opts.AuthModes[auth.Header] || opts.AuthModes[auth.SSO] { + var err error + c, err = configController.Get(ctx) + if err != nil { + return nil, err + } + } if opts.AuthModes[auth.Header] { - c, err := configController.Get(ctx) - if err != nil { - return nil, err - } - + if c.Header == (config.HeaderConfig{}) { + return nil, fmt.Errorf("auth mode %q requires the `header` configuration block", auth.Header) + } headerIf = header.New(c.Header) log.Info(ctx, "Trusted Header authentication enabled") } else { log.Info(ctx, "Trusted Header authentication disabled") } if opts.AuthModes[auth.SSO] { - c, err := configController.Get(ctx) - if err != nil { - return nil, err - } - ssoIf, err = sso.New(ctx, c.SSO, ...) + var err error + ssoIf, err = sso.New(ctx, c.SSO, opts.Clients.Kubernetes.CoreV1().Secrets(opts.Namespace), opts.BaseHRef, opts.TLSConfig != nil) if err != nil { return nil, err }Note that
HeaderConfigis comparable only while all its fields stay comparable. If you add a slice or map field later, replace the equality check with an explicit predicate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/apiserver/argoserver.go` around lines 155 - 186, Load the configuration once via configController.Get(ctx) when either Header or SSO authentication is enabled, then reuse it in both branches while preserving error propagation. In the Header branch, validate that c.Header is populated before passing it to header.New; return a startup error when the configuration is missing. Use a comparable empty-config check only while HeaderConfig remains comparable, otherwise use an explicit population predicate.
🧹 Nitpick comments (3)
server/auth/header/header_test.go (2)
87-101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
requirefor the error assertion.
assert.NoErrorcontinues after a failure, so a nilclaimsproduces a panic on the following lines rather than a clear failure message. Userequire.NoError.♻️ Proposed fix
- assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, tt.issuer, claims.Claims.Issuer)Add the import:
"github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/auth/header/header_test.go` around lines 87 - 101, In the Authorize test subcase within the loop, replace assert.NoError with require.NoError and add the testify/require import, so execution stops before dereferencing claims when authorization fails.
23-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for absent headers.
No case exercises a request that lacks the configured headers. That is the path that currently yields empty claims and a successful authentication. Add a case with a header-based config and an empty
metadata.MD, and assert the behavior you decide on inserver/auth/header/header.go. Also consider a case forPreferredUsernameand a case whereValueandHeaderare both set, to pin the documented precedence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/auth/header/header_test.go` around lines 23 - 85, Add table-driven cases to the header authentication tests covering configured header claims with empty metadata and asserting the behavior implemented by the header authentication logic, plus PreferredUsername and ClaimSource configurations with both Value and Header set to verify documented precedence. Anchor the new cases to the existing test table and the header claim extraction/authentication function in header.go, preserving current static, header, and groups coverage.server/auth/gatekeeper_test.go (1)
174-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the header-only and header-error cases.
These tests always inject an
ssomocks.Interfaceto controlIsRBACEnabled, so they never exercise the realistic header-only deployment wheressoIfissso.NullSSO. Add a case that enablesModes{Header: true}withsso.NullSSOand asserts the resulting service account, and a case where the header provider returns an error and the gatekeeper returnscodes.Unauthenticated. Both cases pin the behavior discussed in the comments onserver/auth/gatekeeper.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/auth/gatekeeper_test.go` around lines 174 - 218, Extend the gatekeeper tests around NewGatekeeper with a header-only case using sso.NullSSO instead of ssomocks.Interface, asserting Context populates the expected service-account identity, and add a header-provider error case where Authorize returns an error and g.Context returns a codes.Unauthenticated status. Keep the existing mocked RBAC cases unchanged and follow the behavior documented in gatekeeper.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/header.go`:
- Around line 20-36: Update the authentication documentation to include the
header auth mode, document the sso.header configuration keys matching
HeaderConfig (Issuer, Subject, Email, PreferredUsername, and Groups), and add a
prominent warning that the proxy must strip client-supplied identity headers
before forwarding requests to Argo Server.
- Around line 22-36: Update the JSON tags on the claim-source fields Issuer,
Subject, Email, PreferredUsername, and Groups to use omitzero instead of
omitempty, matching the surrounding configuration and ensuring empty struct
values are omitted during marshaling.
In `@server/auth/gatekeeper.go`:
- Around line 261-268: Decouple the RBAC gate in authenticateClaims from
s.ssoIf.IsRBACEnabled() so header authentication can authorize claims when RBAC
is configured without SSO. Add or reuse a gatekeeper/header-owned RBAC
configuration established during construction, update the condition to use it,
and adjust rbacAuthorization’s SSO-specific delegation/logging so header
requests do not use or describe SSO service-account behavior.
- Around line 169-193: Update the authorization handling around getClients and
clientForAuthorization so ARGO_TOKEN values without Bearer or Basic prefixes are
normalized before gatekeeper checks, or explicitly enforce those prefixes when
constructing the auth string. Ensure raw ARGO_TOKEN values remain a supported
client authentication path and are not rejected by the prefix validation in the
shown gatekeeper flow.
In `@server/auth/header/header.go`:
- Around line 54-65: Make header authentication reject requests with no resolved
subject: update header.Authorize in server/auth/header/header.go (54-65) to
return an error instead of empty claims when the configured subject headers are
absent. In server/auth/gatekeeper.go (197-202), treat that unresolved-header
error as unauthenticated for Header mode and continue to the s.Modes[Server]
fallback rather than returning the header result; preserve normal errors and
successful header authentication.
- Around line 47-51: Update the group parsing logic around the delimiter branch
to trim surrounding whitespace from each parsed entry and omit empty entries,
including those created by leading, trailing, or repeated delimiters. Preserve
the single-value path by applying the same normalization to its returned group.
---
Outside diff comments:
In `@server/apiserver/argoserver.go`:
- Around line 155-186: Load the configuration once via configController.Get(ctx)
when either Header or SSO authentication is enabled, then reuse it in both
branches while preserving error propagation. In the Header branch, validate that
c.Header is populated before passing it to header.New; return a startup error
when the configuration is missing. Use a comparable empty-config check only
while HeaderConfig remains comparable, otherwise use an explicit population
predicate.
---
Nitpick comments:
In `@server/auth/gatekeeper_test.go`:
- Around line 174-218: Extend the gatekeeper tests around NewGatekeeper with a
header-only case using sso.NullSSO instead of ssomocks.Interface, asserting
Context populates the expected service-account identity, and add a
header-provider error case where Authorize returns an error and g.Context
returns a codes.Unauthenticated status. Keep the existing mocked RBAC cases
unchanged and follow the behavior documented in gatekeeper.go.
In `@server/auth/header/header_test.go`:
- Around line 87-101: In the Authorize test subcase within the loop, replace
assert.NoError with require.NoError and add the testify/require import, so
execution stops before dereferencing claims when authorization fails.
- Around line 23-85: Add table-driven cases to the header authentication tests
covering configured header claims with empty metadata and asserting the behavior
implemented by the header authentication logic, plus PreferredUsername and
ClaimSource configurations with both Value and Header set to verify documented
precedence. Anchor the new cases to the existing test table and the header claim
extraction/authentication function in header.go, preserving current static,
header, and groups coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b87fcc1f-09a3-4bd8-a4ec-d02e3abdb344
⛔ Files ignored due to path filters (1)
server/auth/header/mocks/Interface.gois excluded by!**/mocks/**
📒 Files selected for processing (13)
.mockery.yamlcmd/argo/commands/server.goconfig/config.goconfig/header.godocs/cli/argo_server.mdpkg/apiclient/argo-kube-client.goserver/apiserver/argoserver.goserver/auth/gatekeeper.goserver/auth/gatekeeper_test.goserver/auth/header/header.goserver/auth/header/header_test.goserver/auth/mode.goserver/auth/mode_test.go
💤 Files with no reviewable changes (1)
- server/auth/mode_test.go
| // HeaderConfig contains trusted header authentication configuration settings. | ||
| type HeaderConfig struct { | ||
| // Issuer configures the issuer claim. | ||
| Issuer ClaimSource `json:"iss,omitempty"` | ||
|
|
||
| // Subject configures the subject claim. | ||
| Subject ClaimSource `json:"sub,omitempty"` | ||
|
|
||
| // Email configures the email claim. | ||
| Email ClaimSource `json:"email,omitempty"` | ||
|
|
||
| // PreferredUsername configures the preferred_username claim. | ||
| PreferredUsername ClaimSource `json:"preferred_username,omitempty"` | ||
|
|
||
| // Groups configures the groups claim. | ||
| Groups GroupClaimSource `json:"groups,omitempty"` | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find auth-mode and configmap documentation pages and any generated config reference.
fd -t f -e md . docs | rg -i 'auth|configmap|config'
rg -n 'sso:' --glob '*.md' docs | head -20Repository: argoproj/argo-workflows
Length of output: 766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '### config/header.go outline\n'
ast-grep outline config/header.go --view expanded || true
printf '\n### config/header.go\n'
cat -n config/header.go
printf '\n### references to HeaderConfig/header config/auth-mode in docs/config\n'
rg -n -i 'header|auth-mode|argoclient|argo server|trusted|client-supplied|reverse proxy|identity' docs config README.md 2>/dev/null | head -200Repository: argoproj/argo-workflows
Length of output: 32349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '### relevant auth-mode doc sections\n'
cat -n docs/argo-server-auth-mode.md | sed -n '1,220p'
printf '\n### relevant Argo Server doc sections\n'
cat -n docs/argo-server.md | sed -n '1,90p;155,190p'
printf '\n### relevant workflow-controller ConfigMap SSO section\n'
cat -n docs/workflow-controller-configmap.yaml | sed -n '420,490p'
printf '\n### all exact header-block occurrences outside Go implementation\n'
rg -n -i '^\s*header\b|header:\s*\{|\bheader\s*:\s*|trusted header|client-supplied|reverse proxy|Identity|identity' docs config README.md 2>/dev/null || trueRepository: argoproj/argo-workflows
Length of output: 12855
Document the header authentication config block.
docs/argo-server-auth-mode.md still only lists server, client, and sso. Add the new header auth mode, the sso.header keys under workflow-controller-configmap.yaml, and a clear warning that the proxy must remove client-supplied identity headers before forwarding Argo Server. Otherwise operators can expose a directly reachable server that accepts any caller as any identity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/header.go` around lines 20 - 36, Update the authentication
documentation to include the header auth mode, document the sso.header
configuration keys matching HeaderConfig (Issuer, Subject, Email,
PreferredUsername, and Groups), and add a prominent warning that the proxy must
strip client-supplied identity headers before forwarding requests to Argo
Server.
| // Issuer configures the issuer claim. | ||
| Issuer ClaimSource `json:"iss,omitempty"` | ||
|
|
||
| // Subject configures the subject claim. | ||
| Subject ClaimSource `json:"sub,omitempty"` | ||
|
|
||
| // Email configures the email claim. | ||
| Email ClaimSource `json:"email,omitempty"` | ||
|
|
||
| // PreferredUsername configures the preferred_username claim. | ||
| PreferredUsername ClaimSource `json:"preferred_username,omitempty"` | ||
|
|
||
| // Groups configures the groups claim. | ||
| Groups GroupClaimSource `json:"groups,omitempty"` | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check declared Go version and existing omitzero usage in config.
fd -t f 'go.mod' -d 1 --exec sed -n '1,10p'
rg -n 'omitzero' config/ | head -30Repository: argoproj/argo-workflows
Length of output: 1497
🏁 Script executed:
#!/bin/bash
# Inspect the relevant structs and demonstrate JSON behavior for omitted struct values vs omitzero.
set -euo pipefail
fd -t f 'header.go' -d 5 --exec sh -c 'echo "== $1 =="; wc -l "$1"; sed -n "1,120p" "$1"' sh {}
# If the Go toolchain is present in the sandbox, run a read-only behavioral probe without executing repository code.
if command -v go >/dev/null 2>&1; then
tmpdir="$(mktemp -d)"
cat > "$tmpdir/omit_test.go" <<'GO'
package main
import (
"encoding/json"
"fmt"
)
type ClaimSource struct {
Header string `json:"header,omitempty"`
Value string `json:"value,omitempty"`
}
type HeaderConfig struct {
Issuer ClaimSource `json:"iss,omitempty"`
}
type ConfigWithOmitZero struct {
HeaderConfig HeaderConfig `json:"header,omitzero"`
}
type ConfigWithOmitEmpty struct {
HeaderConfig HeaderConfig `json:"header,omitempty"`
}
func main() {
c := ClaimSource{}
b, err := json.Marshal(c)
if err != nil {
panic(err)
}
fmt.Println("ClaimSource zero:", string(b))
var untyped json.RawMessage
if err := json.Unmarshal(json.RawMessage(`{"header":""}`), &untyped); err != nil {
panic(err)
}
fmt.Println("ClaimSource non-zero header-empty:", string(untyped))
zero := ConfigWithOmitZero{}
fmt.Println("Zero omitzero:", string(mustJSON(zero)))
fmt.Println("Zero omitempty:", string(mustJSON(ConfigWithOmitEmpty{HeaderConfig: c})))
fmt.Println("Non-zero omitzero:", string(mustJSON(ConfigWithOmitZero{HeaderConfig: ClaimSource{Value: "x"}})))
fmt.Println("Non-zero omitempty:", string(mustJSON(ConfigWithOmitEmpty{HeaderConfig: ClaimSource{Value: "x"}})))
}
func mustJSON(v interface{}) []byte {
b, err := json.Marshal(v)
if err != nil {
panic(err)
}
return b
}
GO
go run "$tmpdir/omit_test.go"
fiRepository: argoproj/argo-workflows
Length of output: 2784
🏁 Script executed:
#!/bin/bash
# Demonstrate JSON omit behavior without executing repository code and using a non-test filename.
set -euo pipefail
tmpdir="$(mktemp -d)"
cat > "$tmpdir/omitprobe.go" <<'GO'
package main
import (
"encoding/json"
"fmt"
)
type ConfigWithOmitZero struct{}
type ConfigWithOmitEmpty struct{}
func main() {
zero := ConfigWithOmitZero{}
fmt.Println("zero omitzero:", string(mustJSON(zero)))
fmt.Println("zero omitempty:", string(mustJSON(ConfigWithOmitEmpty{})))
}
func mustJSON(v interface{}) []byte {
b, err := json.Marshal(v)
if err != nil {
panic(err)
}
return b
}
GO
go run "$tmpdir/omitprobe.go"Repository: argoproj/argo-workflows
Length of output: 198
Use omitzero for the claim-source fields.
encoding/json ignores omitempty on struct values, so these fields still marshal even when empty. The surrounding config uses omitzero, and the declared toolchain supports it.
♻️ Proposed tag change
type HeaderConfig struct {
// Issuer configures the issuer claim.
- Issuer ClaimSource `json:"iss,omitempty"`
+ Issuer ClaimSource `json:"iss,omitzero"`
// Subject configures the subject claim.
- Subject ClaimSource `json:"sub,omitempty"`
+ Subject ClaimSource `json:"sub,omitzero"`
// Email configures the email claim.
- Email ClaimSource `json:"email,omitempty"`
+ Email ClaimSource `json:"email,omitzero"`
// PreferredUsername configures the preferred_username claim.
- PreferredUsername ClaimSource `json:"preferred_username,omitempty"`
+ PreferredUsername ClaimSource `json:"preferred_username,omitzero"`
// Groups configures the groups claim.
- Groups GroupClaimSource `json:"groups,omitempty"`
+ Groups GroupClaimSource `json:"groups,omitzero"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Issuer configures the issuer claim. | |
| Issuer ClaimSource `json:"iss,omitempty"` | |
| // Subject configures the subject claim. | |
| Subject ClaimSource `json:"sub,omitempty"` | |
| // Email configures the email claim. | |
| Email ClaimSource `json:"email,omitempty"` | |
| // PreferredUsername configures the preferred_username claim. | |
| PreferredUsername ClaimSource `json:"preferred_username,omitempty"` | |
| // Groups configures the groups claim. | |
| Groups GroupClaimSource `json:"groups,omitempty"` | |
| } | |
| // Issuer configures the issuer claim. | |
| Issuer ClaimSource `json:"iss,omitzero"` | |
| // Subject configures the subject claim. | |
| Subject ClaimSource `json:"sub,omitzero"` | |
| // Email configures the email claim. | |
| Email ClaimSource `json:"email,omitzero"` | |
| // PreferredUsername configures the preferred_username claim. | |
| PreferredUsername ClaimSource `json:"preferred_username,omitzero"` | |
| // Groups configures the groups claim. | |
| Groups GroupClaimSource `json:"groups,omitzero"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/header.go` around lines 22 - 36, Update the JSON tags on the
claim-source fields Issuer, Subject, Email, PreferredUsername, and Groups to use
omitzero instead of omitempty, matching the surrounding configuration and
ensuring empty struct values are omitted during marshaling.
| if len(authorizations) > 0 { | ||
| for _, authorization := range authorizations { | ||
| if authorization == "" { | ||
| continue | ||
| } | ||
|
|
||
| if s.Modes[SSO] && strings.HasPrefix(authorization, sso.Prefix) { | ||
| return s.authenticateSSO(ctx, authorization, req) | ||
| } | ||
|
|
||
| if s.Modes[Client] && | ||
| (strings.HasPrefix(authorization, "Bearer ") || | ||
| strings.HasPrefix(authorization, "Basic ")) { | ||
| return s.authenticateClient(authorization) | ||
| } | ||
|
|
||
| return nil, nil, status.Error( | ||
| codes.Unauthenticated, | ||
| "token not valid. see https://argo-workflows.readthedocs.io/en/latest/faq/", | ||
| ) | ||
| } | ||
|
|
||
| // All Authorization headers were empty. | ||
| // Fall through to the next authentication method. | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare against the previous GetMode implementation and find authorization value producers.
git log -1 --format=%H -- server/auth/mode.go
git show HEAD~1:server/auth/mode.go 2>/dev/null || echo "previous revision not available"
rg -nP --type=go -C3 '"authorization"' | rg -v '_test\.go' | head -40Repository: argoproj/argo-workflows
Length of output: 627
🏁 Script executed:
#!/bin/bash
set -u
echo "== files =="
git ls-files | rg '(^|/)gatekeeper|(^|/)mode|auth' | sed -n '1,120p'
echo "== gatekeeper outline =="
ast-grep outline server/auth/gatekeeper.go --view expanded | sed -n '1,220p'
echo "== relevant gatekeeper section =="
sed -n '1,260p' server/auth/gatekeeper.go
echo "== mode git status =="
git diff -- server/auth/mode.go || true
git show --stat HEAD -- server/auth/mode.go || true
echo "== mode refs =="
rg -n "func \\(m Modes\\) GetMode|type Mode|Mode string|GetMode" server/auth -C 4Repository: argoproj/argo-workflows
Length of output: 13995
🏁 Script executed:
#!/bin/bash
set -u
echo "== mode diff from merge base if available =="
base="$(git merge-base --fork-point HEAD 2>/dev/null || true)"
echo "fork-point base=$base"
if [ -n "$base" ]; then
git diff "$base"..HEAD -- server/auth/gatekeeper.go server/auth/mode.go | sed -n '1,260p'
else
echo "fork-point base unavailable"
fi
echo "== authorization token value producers =="
rg -n --type=go 'authorization|Authorization|Bearer |Basic |SetBearer|SetBasic|clientset|rest\.Config|Token|TokenSource|Cookie|SetCookie' -C 3 \
cmd sdks test server/util | head -320
echo "== docs around auth modes and tokens =="
sed -n '1,260p' docs/argo-server-auth-mode.md
echo "== cmd argo auth root/token =="
sed -n '1,260p' cmd/argo/commands/auth/root.go
sed -n '1,220p' cmd/argo/commands/auth/token.goRepository: argoproj/argo-workflows
Length of output: 21495
🏁 Script executed:
#!/bin/bash
set -u
echo "== full fork-point diff =="
git diff c6da9934b7aaf197c2bd74018cab0e6d3bcf85f6..HEAD -- server/auth/gatekeeper.go server/auth/mode.go | sed -n '1,360p'
echo "== auth client string/header implementations =="
fd -a 'getauth|auth|interceptor|client' cmd sdks server | sed -n '1,200p'
rg -n --type=go 'func GetAuthString|GetAuthString|WithTokenSource|SetHeader|Authorization|Bearer|RestClient|grpc.Dial|CallOption|UnaryClientInterceptor|PerRPCCredentials' cmd sdks server -C 3 | sed -n '1,400p'
echo "== gatekeeper tests around authorization classification =="
sed -n '1,340p' server/auth/gatekeeper_test.go
sed -n '1,180p' server/auth/mode_test.goRepository: argoproj/argo-workflows
Length of output: 48094
🏁 Script executed:
#!/bin/bash
set -u
echo "== inspect argo auth clients =="
sed -n '1,260p' cmd/argo/commands/client/auth.go
find cmd/argo/commands client sdks go/server sdks/java -maxdepth 2 -type d | sed -n '1,120p' | while read -r d; do [ -d "$d" ] && [ "$(fd . "$d" | wc -l)" -lt 120 ] && echo "-- $d"; fd . "$d" | sort | sed -n '1,120p'; done
echo "== exact auth string consumers in cmd/argo and sdks/go =="
rg -n --type=go 'GetAuthString|WithHeader\("Authorization"|Authorization.*Bearer|Bearer Token|TokenSource|PerRPCCredentials|CallOption' cmd/argo sdks/go -C 3 | sed -n '1,360p'
echo "== read-only behavioral probe for gatekeeper prefix branch =="
python3 - <<'PY'
PREFIXES = {"Bearer ", "Basic ", "v2:"}
values = ["abc", "Bearer abc", "Basic abc", "v2:abc", "Bearer", "Basic"]
current = lambda v: True if v.startswith(("Bearer ", "Basic ")) else False
for v in values:
print(v, current(v))
PYRepository: argoproj/argo-workflows
Length of output: 49248
Normalize ARGO_TOKEN before gatekeeper prefix checks.
GetAuthString returns ARGO_TOKEN unchanged, but getClients only passes Bearer ... and Basic ... values into clientForAuthorization; any other token makes argo fail with Unauthenticated. Keep the raw token as a supported client auth value, or require clients/config to use one of those prefixes before returning the auth string.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/auth/gatekeeper.go` around lines 169 - 193, Update the authorization
handling around getClients and clientForAuthorization so ARGO_TOKEN values
without Bearer or Basic prefixes are normalized before gatekeeper checks, or
explicitly enforce those prefixes when constructing the auth string. Ensure raw
ARGO_TOKEN values remain a supported client authentication path and are not
rejected by the prefix validation in the shown gatekeeper flow.
| if s.ssoIf.IsRBACEnabled() { | ||
| clients, err := s.rbacAuthorization(ctx, claims, req) | ||
| if err != nil { | ||
| return nil, nil, status.Error(codes.Unauthenticated, err.Error()) | ||
| logger.WithError(err).Error(ctx, "failed to perform RBAC authorization") | ||
| return nil, nil, status.Error(codes.PermissionDenied, "not allowed") | ||
| } | ||
| claims, _ := serviceaccount.ClaimSetFor(restConfig) | ||
| return clients, claims, nil | ||
| case Server: | ||
| claims, _ := serviceaccount.ClaimSetFor(s.restConfig) | ||
| return s.clients, claims, nil | ||
| case SSO: | ||
| logger := logging.RequireLoggerFromContext(ctx) | ||
| claims, err := s.ssoIf.Authorize(authorization) | ||
| if err != nil { | ||
| return nil, nil, status.Error(codes.Unauthenticated, err.Error()) | ||
| } | ||
| if s.ssoIf.IsRBACEnabled() { | ||
| clients, err := s.rbacAuthorization(ctx, claims, req) | ||
| if err != nil { | ||
| logger.WithError(err).Error(ctx, "failed to perform RBAC authorization") | ||
| return nil, nil, status.Error(codes.PermissionDenied, "not allowed") | ||
| } | ||
| return clients, claims, nil | ||
| } | ||
| // important! write an audit entry (i.e. log entry) so we know which user performed an operation | ||
| logger.WithFields(addClaimsLogFields(claims, nil)).Info(ctx, "using the default service account for user") | ||
| return s.clients, claims, nil | ||
| default: | ||
| panic("this should never happen") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Header authentication should not depend on SSO for its RBAC decision.
authenticateClaims gates RBAC on s.ssoIf.IsRBACEnabled(). server/apiserver/argoserver.go assigns sso.NullSSO when SSO mode is disabled, so a deployment that enables only --auth-mode=header gets IsRBACEnabled() == false. RBAC is then skipped and every header-identified user receives the server service account and its full permissions. Header claims map to no service account.
The tests confirm the coupling: Header+RBAC,precedence=1 must inject an ssomocks.Interface to turn RBAC on for a gatekeeper that has no SSO at all.
Resolve the RBAC decision from a source that both modes own, for example a gatekeeper field set at construction, or an IsRBACEnabled method on header.Interface driven by the header configuration. Also note that rbacAuthorization reads SSO_DELEGATE_RBAC_TO_NAMESPACE and logs "selected SSO RBAC service account for user", which is now misleading for header requests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/auth/gatekeeper.go` around lines 261 - 268, Decouple the RBAC gate in
authenticateClaims from s.ssoIf.IsRBACEnabled() so header authentication can
authorize claims when RBAC is configured without SSO. Add or reuse a
gatekeeper/header-owned RBAC configuration established during construction,
update the condition to use it, and adjust rbacAuthorization’s SSO-specific
delegation/logging so header requests do not use or describe SSO service-account
behavior.
| if source.Delimiter == "" { | ||
| return []string{value} | ||
| } | ||
|
|
||
| return strings.Split(value, source.Delimiter) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim whitespace and drop empty group entries.
strings.Split keeps surrounding spaces and empty segments. A proxy that emits admin, developer produces the group " developer", which never matches an RBAC rule, and a trailing delimiter produces an empty group. Both failures are silent.
♻️ Proposed fix
if source.Delimiter == "" {
return []string{value}
}
- return strings.Split(value, source.Delimiter)
+ var groups []string
+ for _, g := range strings.Split(value, source.Delimiter) {
+ if g = strings.TrimSpace(g); g != "" {
+ groups = append(groups, g)
+ }
+ }
+ return groups
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if source.Delimiter == "" { | |
| return []string{value} | |
| } | |
| return strings.Split(value, source.Delimiter) | |
| if source.Delimiter == "" { | |
| return []string{value} | |
| } | |
| var groups []string | |
| for _, g := range strings.Split(value, source.Delimiter) { | |
| if g = strings.TrimSpace(g); g != "" { | |
| groups = append(groups, g) | |
| } | |
| } | |
| return groups |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/auth/header/header.go` around lines 47 - 51, Update the group parsing
logic around the delimiter branch to trim surrounding whitespace from each
parsed entry and omit empty entries, including those created by leading,
trailing, or repeated delimiters. Preserve the single-value path by applying the
same normalization to its returned group.
| func (h *header) Authorize(md metadata.MD) (*types.Claims, error) { | ||
| claims := &types.Claims{} | ||
|
|
||
| claims.Claims.Issuer = resolveClaim(h.config.Issuer, md) | ||
| claims.Claims.Subject = resolveClaim(h.config.Subject, md) | ||
|
|
||
| claims.Email = resolveClaim(h.config.Email, md) | ||
| claims.PreferredUsername = resolveClaim(h.config.PreferredUsername, md) | ||
| claims.Groups = resolveGroups(h.config.Groups, md) | ||
|
|
||
| return claims, nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Trusted header authentication succeeds without an identity. The provider has no way to report "no identity resolved", so the gatekeeper accepts every request when Header mode is enabled. header.Authorize returns empty claims with a nil error when the configured headers are absent, and the gatekeeper treats that as a successful authentication, returns the server's own clients when SSO RBAC is off, and never falls back to Server mode. A caller that reaches the server directly, bypassing the proxy, is authenticated as the empty user.
server/auth/header/header.go#L54-L65: return an error when no subject claim resolves from the configured headers, instead of returning empty claims with a nil error.server/auth/gatekeeper.go#L197-L202: treat an unresolved header identity as "not authenticated by this mode" and continue to thes.Modes[Server]branch, rather than returning the header result unconditionally.
📍 Affects 2 files
server/auth/header/header.go#L54-L65(this comment)server/auth/gatekeeper.go#L197-L202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/auth/header/header.go` around lines 54 - 65, Make header
authentication reject requests with no resolved subject: update header.Authorize
in server/auth/header/header.go (54-65) to return an error instead of empty
claims when the configured subject headers are absent. In
server/auth/gatekeeper.go (197-202), treat that unresolved-header error as
unauthenticated for Header mode and continue to the s.Modes[Server] fallback
rather than returning the header result; preserve normal errors and successful
header authentication.
Signed-off-by: Sagitra <sagitrapradeep2006@gmail.com>
1 participant
Fixes #16478
Motivation
Add support for trusted header authentication in Argo Workflows, allowing deployments behind a trusted authentication proxy to authenticate users through configured HTTP headers.
Modifications
Verification
make testpasses.Documentation
Updated the authentication configuration to include the new
Headerauthentication mode alongside the existingClient,Server, andSSOmodes.AI
AI was used for assistance with implementation and test development.
Summary by CodeRabbit
headeras a supported server authentication mode.