egress: MITM tunneled TLS with per-SNI certificate minting - #871
egress: MITM tunneled TLS with per-SNI certificate minting#871haiyanmeng wants to merge 2 commits into
Conversation
195d6c4 to
7976e5a
Compare
6080f7e to
7b74711
Compare
There was a problem hiding this comment.
- Remove the intermediate CA code
- Some of the options seem like we don't need to expose. If so, then the logic inside should be simplified.
- Move the CA generating logic into its own package (suggest
sdsmint/certauth) that exports a very narrow interface out to the server. Seem like the only things that need to be exported are a Get() and Forget() - I don't know if we need an optional minter.Forgetter() -- this comment applies to other places in the code that seem to have more options than we need.
- Go through the comments and scrub for the agent-based self-conversation. The comments are pretty verbose in some places and look more like agent thinking tokens than code comments.
I mostly looked at the server code. Once you get the simplifications in, I will take a more detailed look at the rest of the change.
Done
Done
Done
Done |
|
|
||
| cmd.Flags().StringVar(&cfg.UDSPath, "uds-path", "", "unix socket to listen on; required, and the only transport offered, because leaf private keys transit this channel") | ||
| cmd.Flags().StringVar(&cfg.CAPoolPath, "ca-pool-path", "", "path to a localca pool JSON holding the MITM CA, the format substrate mounts its other CAs in") | ||
| cmd.Flags().StringVar(&cfg.CAID, "ca-id", "", "which CA in the pool to sign with; empty takes the first") |
There was a problem hiding this comment.
Let's leave the behavior to just take the first --- localca.Pool should be tracking which localca.CA is active for signing. I will send a PR to do this (and add rotation commands for the CA secrets).
There was a problem hiding this comment.
Taahir Ahmed (@ahmedtd) , are you suggesting removing this flag and take the first CA from the pool by default?
There was a problem hiding this comment.
Does your PR need to block this PR?
There was a problem hiding this comment.
Can we default to first for now or it doesn't work without this flag.
There was a problem hiding this comment.
It works as is. Here is how the sdsmint container in the egress gateway Pod sets it
args:
- "sdsmint"
- "--uds-path=/var/run/sdsmint/sdsmint.sock"
- "--ca-pool-path=/run/ca-state/mitm-pool.json"
- "--ca-id=mitm"
There was a problem hiding this comment.
Yeah, we can just default to the first CA for now.
Agreed
These can be done. |
This can be done too.
This is to prevent Envoy from being OOM.
If we remove the rotation logic without adding an alternative like per-resource ttl, the UX may be bad. The secret Envoy holds expires, and Envoy keeps presenting it. Every client handshake to that host then fails on an expired cert. Let me test out this behavior. |
And would it retry and then fetch the new cert/secret again? |
This isn't true for ECDSA as I understand it --- ECDSA keygen just reading random bytes from crypto/rand, whereas the signatures for creating the certificates require bigint math. It's more performant than RSA, but I don't think we can get away without caching for a production system. |
|
We should also be careful about exhausting randomness sources. |
|
Bowei Du (@bowei) , Taahir Ahmed (@ahmedtd) , Lior Lieberman (@LiorLieberman) , I updated the PR:
PTAL. |
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
Thanks for all the hard work, sds server code mostly looks good to me.
Left some comments. Plus we need to think how to gurad this with a feature flag or something.
Also - any resolution on https://github.com/agent-substrate/substrate/pull/871/changes#r3771056979?
| st := &deltaStream{ | ||
| srv: s, | ||
| stream: stream, | ||
| sendCh: make(chan *discovery.DeltaDiscoveryResponse, 8), |
There was a problem hiding this comment.
nit: why 8? whats 8?
There was a problem hiding this comment.
concurrency limit?
There was a problem hiding this comment.
8 was picked by Claude. I added a comment generated by Claude. We can tune it in the future if needed.
There was a problem hiding this comment.
what is it for though? limiting concurrency?
There was a problem hiding this comment.
sendDepth is just the capacity of sendCh. If we set it to 0, every response becomes a synchronous rendezvous with sendLoop, so the select loop stalls on each write. That's the thing the buffer exists to avoid.
There was a problem hiding this comment.
dont we already have some egress testing go code machinery (if not, how do we test egress now?) maybe this can be integrated there vs creating a new package?
There was a problem hiding this comment.
The only thing I am aware of https://github.com/agent-substrate/substrate/blob/main/internal/e2e/suites/networking/networking_test.go.
Let us look into how to converge these egress tests after actors trust egress-mitm-ca-pool.
There was a problem hiding this comment.
are we going to have an internal listener now for all envoy deployments? we should likely guard it with a feature flag.
Also does this code/config right now only pass to the envoy internal listener when MITM is needed? (e.g actor dialed HTTPS) or every time? the impl should be the former.
There was a problem hiding this comment.
Also does this code/config right now only pass to the envoy internal listener when MITM is needed? (e.g actor dialed HTTPS) or every time? the impl should be the former.
mitm_listener has two filter chains. One for tls traffic (transport_protocol: tls), the other for plaintext traffic (transport_protocol: raw_buffer).
There was a problem hiding this comment.
not sure I understand the response. if actor sends http traffic (meaning no need for mitm) - do we go to mitm listener?
There was a problem hiding this comment.
it goes to mitm_listener. But it has two filter chain. Only the filter chain for https traffic talks to sdsmint.
For http traffic, it does not talk to sdsmint.
Not yet. Will follow with Bowei and Taahir on that. |
yanavlasov
left a comment
There was a problem hiding this comment.
LGTM for Envoy config and minting server code.
The egress gateway terminated the actor's CONNECT tunnel and forwarded
the bytes inside it opaquely, so nothing could be said about the TLS
session an actor established through it. This intercepts that session:
Envoy terminates the tunnelled TLS with a leaf minted for the SNI the
client asked for, which puts the plaintext request on a filter chain
where policy can be applied later.
The data path, all inside the atenet-egress pod:
atunnel client -> gateway front door (mTLS, socket listener)
-> CONNECT to an IP:port
-> mitm_internal cluster (envoy_internal_address)
-> mitm_listener (internal, no socket anywhere)
-> tls_inspector reads the SNI
-> DownstreamTlsContext, on_demand_secret selector
-> DELTA_GRPC SDS over a unix socket to sdsmint
-> leaf minted for that SNI, handshake resumes
-> dynamic forward proxy to the real destination
Supporting changes:
internal/localca: CA.SigningKey narrows from crypto.PrivateKey to
crypto.Signer, so a key that parses but cannot sign (X25519 out of
PKCS#8) is refused at load rather than at the first handshake. Adds
CA.Validate, and GenerateCA/GenerateOptions with a key type, common name
and lifetime. GenerateED25519CA stays as a wrapper -- every existing
caller wants exactly what it produced.
kubectl-ate admin make-ca-pool: --key-type and --common-name.
hack/install-ate.sh: creates the egress-mitm-ca-pool secret, ecdsa-p256
rather than the ed25519 default. These leaves are validated by arbitrary
clients inside actor sandboxes, where Ed25519 support cannot be assumed.
internal/atunnel: ErrGatewayHandshake and ConnectRejectedError replace
formatted strings, so a caller can tell a front-door TLS rejection from
an authorization denial without matching on message text. A gateway that
resets instead of alerting is folded into the former, since which of the
two happens is a race.
e2e: internal/e2e/suites/sdsmint covers the tunnelled handshake, the
leaf Envoy serves, and actor identity across the MITM, against the new
egressprobe fixture.
|
I rebased after #959 is merged. We should expect |
|
https://github.com/agent-substrate/substrate/actions/runs/31837847684/job/94888039437?pr=871 The test Bowei Du (@bowei) , let us merge #926 first. Then I will rebase and update this PR. |
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
Keith Mattix II (@keithmattix) has done a nice thing on #715 where he builds the listener in go (see xds.go in his PR) based on whether someone enabled connect (using a flag).
we should do something similar for MITM listener and other config changes.
Go's random sources cannot be exhausted --- they are a CSPRNG keyed with initial randomness read from /dev/urandom. /dev/urandom itself is a CSPRNG seeded with a few bytes of random derived from startup or the RDRAND instruction. Linux does a bunch of accounting of entropy sources, and tries to reseed /dev/random and /dev/urandom once many values are read from them. This is widely considered to be superstitious hokum. |
|
|
||
| cmd.Flags().StringVar(&cfg.UDSPath, "uds-path", "", "unix socket to listen on; required, and the only transport offered, because leaf private keys transit this channel") | ||
| cmd.Flags().StringVar(&cfg.CAPoolPath, "ca-pool-path", "", "path to a localca pool JSON holding the MITM CA, the format substrate mounts its other CAs in") | ||
| cmd.Flags().StringVar(&cfg.CAID, "ca-id", "", "which CA in the pool to sign with; empty takes the first") |
There was a problem hiding this comment.
Yeah, we can just default to the first CA for now.
| return | ||
| } | ||
| if err := d.stream.Send(resp); err != nil { | ||
| select { |
There was a problem hiding this comment.
Does this select do anything?
| // derived from the leaf lifetime rather than configured separately, so the | ||
| // two cannot be set into an order that does not work. See pack, which | ||
| // stamps it, for what Envoy does with it. | ||
| resourceTTL time.Duration |
There was a problem hiding this comment.
It seems like the resourceTTL we hand back should be derived from the NotAfter timestamp of the cert we minted (for example, cert.NotAfter.Add(-10*time.Minute). Then we don't have a server-wide setting that can be misconfigured.
| // toSecret packs a minted cert into the Secret proto Envoy expects back. The | ||
| // secret's name MUST equal the requested resource name (the SNI), or Envoy | ||
| // will not match the response to its subscription. | ||
| func toSecret(name string, c *certauth.MintedCert) *tlsv3.Secret { |
There was a problem hiding this comment.
This probably belongs in deltastream.go, since that's where it's used
|
LGTM with some nits |
New changes:
picked by tls_inspector: tls terminates using Envoy 1.37's on_demand_secret selector + sni cert mapper (hence the
1.34 → 1.37 bump), raw_buffer proxies plaintext http:// and enforces the allowlist as an :authority match, since
there's no mint on that path to refuse.
--common-name; install-ate.sh creates the egress-mitm-ca-pool secret before the Deployment.
Not included:
egress-mitm-ca-poolThis is to address #823