Configure upstream TLS context with peer root certs#13321
Conversation
freddygv
requested review from
a team,
acpana,
kisunji and
rboyer
and removed request for
a team
agent/proxycfg/testing_peering.go
Outdated
There was a problem hiding this comment.
I noticed that the trust domain here didn't match the one from L54. It needs to match because both upstreams are in the same peered "cloud" cluster.
freddygv
force-pushed
the
peering/upstream-tls
branch
2 times, most recently
from
d6198cf to
d36b72b
Compare
There was a problem hiding this comment.
Same as above, this needed to be in the same trust domain as the other upstream, since they're in the same peered cluster
There was a problem hiding this comment.
Would be nice to keep auto-generated types/methods in a different file with the commands to regenerate if necessary (I like //go:generate mockery blah... at the top of the file cause Goland can run it)
agent/proxycfg/connect_proxy.go
Outdated
There was a problem hiding this comment.
Do we intentionally not write to map if resp.Bundle is nil?
There was a problem hiding this comment.
Right, because the output isn't usable
agent/proxycfg/snapshot.go
Outdated
There was a problem hiding this comment.
nit: could use strings.Builder here to save memory in case there's a non-trivial number of roots
There was a problem hiding this comment.
There should only ever be two at most, for the rotation case
proto/pbpeering/peering.go
Outdated
There was a problem hiding this comment.
This sounds like a package design issue. Is it this cycle?
pbpeering -> connect/ca -> connect -> pbpeering
OOS for this PR but sounds annoying
There was a problem hiding this comment.
proto/<anything> is structurally in a similar position as agent/structs or api in that it's pretty leaf-y when it comes to dependencies by design.
You could simply move ca.EnsureTrailingNewLine to lib instead since it really just does string manipulation.
There was a problem hiding this comment.
Moved the function to lib.
The cycle was:
pbpeering -> connect/ca -> agent/consul/state
freddygv
force-pushed
the
peering/upstream-tls
branch
from
d36b72b to
a7c7110
Compare
agent/proxycfg/connect_proxy.go
Outdated
There was a problem hiding this comment.
Do you need the garbage collection logic here, or would a TODO to cash-out at tproxy time be sufficient?
There was a problem hiding this comment.
That would be a tproxy concern. Will add it to Asana.
freddygv
added
the
pr/no-changelog
This commit modifies proxycfg for connect proxies so that when an explicit upstream is in a peered cluster, we watch the trust bundle for that cluster. These trust bundles then get stored in a map keyed on peer name. This map is used to only spin up one watch per peer, since there may be multiple upstreams in a peered cluster.
* Skip creating cluster for upstreams in peered clusters that we do
not have a trust bundle for yet.
* If the upstream has a peer name specified, use the trust bundle for
the peered cluster, rather than our local roots.
* Fixup whitespace usage in test bundles for peers.
* Add go:generate command for new cache-type mock * Move ca.EnsureNewLine to lib pkg * Fixup assertion expectation in proxycfg test
freddygv
force-pushed
the
peering/upstream-tls
branch
from
53e9279 to
9732269
Compare
kisunji
added
the
theme/cluster-peering
3 participants
Description
For mTLS to work between two proxies in peered clusters with different root CAs,
proxies need to configure their outbound listener to use different root certificates
for validation.
Up until peering was introduced proxies would only ever use one set of root certificates
to validate all mesh traffic, both inbound and outbound. Now an upstream proxy
may have a leaf certificate signed by a CA that's different from the dialing proxy's.
This PR makes changes to proxycfg and xds so that the upstream TLS validation
uses different root certificates depending on which cluster is being dialed.
Changes by pkg
proxycfg
When an explicit upstream is in a peered cluster, we watch the trust bundle
for that cluster. These trust bundles then get stored in a map keyed on peer name.
xds
When generating upstream clusters for discovery chains we use the RootPEMs
from the appropriate peer trust bundle if the upstream is in another cluster.
Testing & Reproduction steps
These changes have unit tests, but have not been validated against running Envoy proxies yet. That will happen after this PR and the public listener changes have been merged.
PR Checklist
external facing docs updatedchecklist folder consulted