Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/23920.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
agent/xds: Fixed an issue where cross-cluster and peered requests originating from an API Gateway were rejected with HTTP 403 Forbidden by downstream service RBAC intentions. Trailing semicolon-separated fields in the client certificate component of the `x-forwarded-client-cert` (XFCC) header (such as auto-registered DNS SANs) are now properly accepted by the RBAC principal regular expression.
```
5 changes: 3 additions & 2 deletions agent/xds/rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -1009,8 +1009,9 @@ func xfccPrincipal(src rbacService) *envoy_rbac_v3.Principal {
// Remove the leading ^ and trailing $.
idPattern = idPattern[1 : len(idPattern)-1]

// Anchor to the first XFCC component
pattern := `^[^,]+;URI=` + idPattern + `(?:,.*)?$`
// Anchor to the first XFCC component, allowing subsequent semicolon-separated
// fields (such as ;DNS=... or ;Subject=...) or comma-separated hops.
pattern := `^[^,]+;URI=` + idPattern + `(?:[;,].*)?$`
Comment thread
anandmukul93 marked this conversation as resolved.

// By=spiffe://8c7db6d3-e4ee-aa8c-488c-dbedd3772b78.consul/gateway/mesh/dc/dc2;
// Hash=2a2db78ac351a05854a0abd350631bf98cc0eb827d21f4ed5935ccd287779eb6;
Expand Down
83 changes: 83 additions & 0 deletions agent/xds/rbac_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1389,3 +1389,86 @@ func TestMakeSpiffePattern_EscapesRegexMetacharacters(t *testing.T) {
})
}
}

func TestXFCCPrincipal(t *testing.T) {
src := rbacService{
ServiceName: structs.ServiceName{
Name: "gateway",
},
TrustDomain: "2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul",
Peer: "client",
}

principal := xfccPrincipal(src)
headerMatcher := principal.GetHeader()
require.NotNil(t, headerMatcher)
require.Equal(t, "x-forwarded-client-cert", headerMatcher.Name)

regex := headerMatcher.GetStringMatch().GetSafeRegex().Regex
re, err := regexp.Compile(regex)
require.NoError(t, err)

cases := []struct {
name string
xfcc string
expected bool
}{
{
name: "standard sidecar - single hop without DNS SANs",
xfcc: `By=spiffe://server.consul/gateway/mesh/dc/server;URI=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/gateway`,
expected: true,
},
{
name: "standard sidecar - multi-hop without DNS SANs",
xfcc: `By=spiffe://server.consul/gateway/mesh/dc/server;URI=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/gateway,By=gateway`,
expected: true,
},
{
name: "api gateway - multi-hop with auto-registered DNS SANs",
xfcc: `By=spiffe://server.consul/gateway/mesh/dc/server;Hash=abc;Subject="";URI=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/gateway;DNS=gateway.default.svc;DNS=gateway.default.svc.cluster.local,By=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/gateway`,
expected: true,
},
{
name: "api gateway - single hop with auto-registered DNS SANs",
xfcc: `By=spiffe://server.consul/gateway/mesh/dc/server;Hash=abc;Subject="";URI=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/gateway;DNS=gateway.default.svc;DNS=gateway.default.svc.cluster.local`,
expected: true,
},
{
name: "api gateway - with trailing Subject field",
xfcc: `By=spiffe://server.consul/gateway/mesh/dc/server;Hash=abc;URI=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/gateway;Subject="CN=gateway.default"`,
expected: true,
},
{
name: "api gateway - with trailing Subject and multiple hops",
xfcc: `By=spiffe://server.consul/gateway/mesh/dc/server;Hash=abc;URI=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/gateway;Subject="CN=gateway.default",By=spiffe://server.consul/gateway/mesh/dc/server`,
expected: true,
},
{
name: "negative: service name prefix spoofing (gateway2)",
xfcc: `By=spiffe://server.consul/gateway/mesh/dc/server;URI=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/gateway2`,
expected: false,
},
{
name: "negative: hyphenated prefix spoofing (gateway-evil)",
xfcc: `By=spiffe://server.consul/gateway/mesh/dc/server;URI=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/gateway-evil`,
expected: false,
},
{
name: "negative: different service",
xfcc: `By=spiffe://server.consul/gateway/mesh/dc/server;URI=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/other`,
expected: false,
},
{
name: "negative: matched cert is in second hop not first hop",
xfcc: `By=spiffe://server.consul/gateway/mesh/dc/server;URI=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/attacker,By=spiffe://2f6cbc1e-8ff2-dd37-0d90-12156d7d2795.consul/ns/default/dc/client/svc/gateway`,
expected: false,
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
matched := re.MatchString(tc.xfcc)
require.Equal(t, tc.expected, matched, "expected match=%v for xfcc %q with regex %s", tc.expected, tc.xfcc, regex)
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"name": "x-forwarded-client-cert",
"stringMatch": {
"safeRegex": {
"regex": "^[^,]+;URI=spiffe://peer1\\.domain/ap/part1/ns/default/dc/[^/]+/svc/[^/]+(?:,.*)?$"
"regex": "^[^,]+;URI=spiffe://peer1\\.domain/ap/part1/ns/default/dc/[^/]+/svc/[^/]+(?:[;,].*)?$"
}
}
}
Expand All @@ -51,7 +51,7 @@
"name": "x-forwarded-client-cert",
"stringMatch": {
"safeRegex": {
"regex": "^[^,]+;URI=spiffe://peer1\\.domain/ap/part1/ns/default/dc/[^/]+/svc/web(?:,.*)?$"
"regex": "^[^,]+;URI=spiffe://peer1\\.domain/ap/part1/ns/default/dc/[^/]+/svc/web(?:[;,].*)?$"
}
}
}
Expand Down
Loading