Repository navigation
Conversation
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
EclipseSourceAI
left a comment
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
This PR fixes the gateway MITM proxy serving cached leaf certificates past their expiry. On a cache hit the leaf is now checked against the CA, the host name, and a validity window of 24h or more, and is re-minted if any check fails. The leaf is loaded while the cache lock is still held, and pruning never evicts the entry being returned. CA setup now refuses incomplete or mismatched pairs instead of regenerating them, and a .ca.lock file lock serializes CA creation across processes.
The renewal logic looks correct. I ran the tlsstore tests locally with -race and they pass. Points for maintainers:
- Leaf cache flock: the extra file lock on
hosts/.cache.lockruns on every handshake. Only one process uses that container-private cache, so the in-process mutex is enough. The CA lock is justified. - CA loading: the CA is still loaded in two places (
ensureCAand the oldloadCAfallback), because the branch that generates a new CA never stores it on the store. - Duplicate work per handshake: on a cache hit the leaf pair is loaded twice, and there are extra
x509.ParseCertificatecalls thatpair.Leafmakes unnecessary. - Behavior change: session start now fails on an incomplete CA pair. This isn't documented in
docs/networking.mdyet.
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| return s.ensureCA() | ||
| return util.WithFileLock(filepath.Join(s.HostsDir, ".cache.lock"), fn) |
There was a problem hiding this comment.
The leaf cache is container-private and only one proxy process with one Store ever touches it (entrypoint, proxy), so s.mu already covers the load-before-prune race. This flock adds an open/lock/close on every TLS handshake just to make the multi-Store test pass; I'd drop it and keep the file lock only for the CA, where concurrent host enclave runs really can race.
| certPath := filepath.Join(s.HostsDir, hostKey+".crt") | ||
| keyPath := filepath.Join(s.HostsDir, hostKey+".key") | ||
| if util.FileExists(certPath) && util.FileExists(keyPath) { | ||
| if s.caCert == nil || s.caKey == nil { |
There was a problem hiding this comment.
This block only runs right after ensureCA generated a new CA, because the generate branch never sets s.caCert/s.caKey. Assign them there (the parsed DER and priv) and this fallback plus loadCA can go, leaving one CA loader instead of two with different key-format support, and no CA read outside .ca.lock.
|
|
||
| // The CA never changes after it is loaded, so cache hits skip the file lock and | ||
| // the RSA key parse that would otherwise run on every TLS handshake. | ||
| func (s *Store) withCALock() error { |
There was a problem hiding this comment.
Nit: this mostly doesn't lock, it ensures the CA is loaded. Something like loadCA (once the old helper is gone) or ensureCALoaded reads better at the call sites.
| return false | ||
| } | ||
| cert, err := tls.LoadX509KeyPair(certPath, keyPath) | ||
| leaf, err := x509.ParseCertificate(pair.Certificate[0]) |
There was a problem hiding this comment.
With go 1.24 tls.LoadX509KeyPair already populates pair.Leaf, so this second parse (and the one in ensureCA) is redundant. Use pair.Leaf.
| if err != nil { | ||
| return err | ||
| } | ||
| pair, err := tls.LoadX509KeyPair(certPath, keyPath) |
There was a problem hiding this comment.
On a cache hit the pair is now read and parsed twice per handshake: once in validLeaf and again here. Have ensureLeaf/validLeaf return the loaded *tls.Certificate (and keep the root CertPool on the store next to caCert instead of rebuilding it each call).
| if !util.FileExists(present) { | ||
| present, missing = missing, present | ||
| } | ||
| return fmt.Errorf("incomplete trusted CA pair: %s is missing; restore it or remove %s to generate a new CA", missing, present) |
There was a problem hiding this comment.
This changes user-visible behavior (an incomplete pair used to be silently regenerated, now session start fails). A line in the CA section of docs/networking.md about this and the recovery step would help, since that's where users look for the TLS dir layout.
…ificates Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
2 participants
What it does
The gateway proxy could serve a cached leaf certificate past its expiry. Cache hits only refreshed file timestamps, so a frequently used host kept its three-month leaf for as long as the gateway container lived.
Cache hits now validate the certificate/key pair, the host name, the signing CA, and validity at least 24 hours ahead, and renew invalid or expiring pairs. Certificate loading stays under the cache lock so pruning cannot remove a pair before it is loaded, and pruning never evicts the certificate being returned, even when other entries have newer timestamps.
Shared CA access is serialized separately from the leaf cache. The CA pair is loaded and checked once per store, so cache hits only re-read and verify the leaf. Incomplete or mismatched CA pairs are rejected instead of being replaced or used, and the error names the files involved. Host-side CA setup does not access the proxy-owned leaf cache, preserving the layout introduced by PR #81.
How to test
Unit tests cover expired and near-expiry leaves, a malformed key, incomplete CA pairs, concurrent stores with a one-entry cache, and a requested leaf whose timestamp is older than another cache entry. Four of the five new tests fail on
main; the fifth guards host-side CA setup against touching the leaf cache.To reproduce, start a restricted session with
--network-log=requestsso HTTPS goes through the MITM proxy, then replace the gateway's cached leaf forapi.github.comwith one that expires within a day:On
maincurl reports the injected one-day expiry, and an already expired leaf fails withcertificate has expired. On this branch the proxy mints a fresh three-month leaf.Verified on Linux (Go 1.27.2; gateway packages also with the
go.modtoolchain, Go 1.24.4):End to end on Docker 29.1.3 and rootless Podman 4.9.3 (claude tool,
--network-log=requests):main, the expired leaf was served and curl failed withcertificate has expired.fetch, and Pythonurllibverified through the proxy.initialize gateway CA: incomplete trusted CA pair: ~/.local/state/enclave/tls/ca.key is missing; restore it or remove ~/.local/state/enclave/tls/ca.crt to generate a new CA. Removing the named file lets the next session generate a new CA. A mismatched pair fails withload trusted CA pair from ~/.local/state/enclave/tls: tls: private key does not match public key.~/.local/state/enclave/tls/hostsdoes not affect session start.QEMU has no restricted-egress gateway and is not affected.
Follow-ups
None.
Breaking changes
Review checklist