ambient: fix auto-allow waypoint #50710
Conversation
Probably we need to increase our test coverage here
stevenctl
added
the
release-notes-none
istio-testing
added
the
size/M
stevenctl
force-pushed
the
fix-auto-allow
branch
2 times, most recently
from
47740eb
to
3df654d
Compare
istio-testing
added
size/L
stevenctl
force-pushed
the
fix-auto-allow
branch
2 times, most recently
from
2807dc2
to
5151551
Compare
stevenctl
force-pushed
the
fix-auto-allow
branch
from
5151551
to
8c3042c
Compare
| @@ -124,7 +126,7 @@ func implicitWaypointPolicy(ctx krt.HandlerContext, waypoint Waypoint) *model.Wo | |||
| { | |||
| Principals: slices.Map(waypoint.ServiceAccounts, func(sa string) *security.StringMatch { | |||
| return &security.StringMatch{MatchType: &security.StringMatch_Exact{ | |||
| Exact: spiffe.MustGenSpiffeURI(waypoint.Namespace, sa), | |||
| Exact: strings.TrimPrefix(spiffe.MustGenSpiffeURI(waypoint.Namespace, sa), spiffe.URIPrefix), | |||
There was a problem hiding this comment.
Do the SANs not have a spiffe:// prefix? Do SPIRE generated SANs have a spiffe prefix?
There was a problem hiding this comment.
Also, can't we just use StringMatch_Prefix directly here, rather than trimming the prefix?
There was a problem hiding this comment.
// Istio matches all assumes spiffe:// prefix. This includes prefix matches.
// A prefix match for "*foo" means "spiffe://*foo".
// So we strip it, and fail if it isn't present.
let Some(check) = check.strip_prefix("spiffe://") else {
return false;
};
This is what zTunnel does
There was a problem hiding this comment.
Istio matches all assumes spiffe:// prefix. This includes prefix matches.
why do we assume that, I wonder. It's not simpler than just matching on spiffe://ns/sa prefixes consistently everywhere, since, as @keithmattix pointed out, the spiffe:// prefix is non-optional.
Oh well.
There was a problem hiding this comment.
Property of the AuthorizationPolicy API fwiw, not a recent decision in ztunnel
stevenctl
added
the
cherrypick/release-1.22
|
In response to a cherrypick label: new pull request created: #50752 |
Taking over #50617