Repository navigation
feat(satellite): run the snapshot stream over mutual TLS - #219
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 54 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bdee268 to
2bc21db
Compare
The satellite served GET /v1/satellite/snapshots/... on every node address with no authentication, and the answer is the raw send stream of a snapshot. Any pod that could reach a node IP could read any volume with a snapshot on that node; in Cozystack, platform pods without an egress policy could. The stream now runs over HTTPS with mutual TLS. Every satellite presents a certificate for the fixed name blockstor-satellite, signed by a stream CA, and verifies the peer's against it in both directions. The files are read on every handshake, so a rotated certificate is picked up without a restart. A satellite started without the three --stream-tls-* flags serves and fetches nothing, and cross-node clone and restore fall back to a DRBD initial sync; a partial set refuses to start. The stand issues the certificate from a CA of its own rather than blockstor-api-ca, since a stream certificate is also a client certificate and the apiserver accepts any from its CA. Fixes #208 Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
2bc21db to
f06cb69
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. The mutual TLS itself holds up, but two behaviours the PR promises have no test behind them, and the new test file has no SPDX header.
The stream served raw volume data to anything that could reach a node address; this puts it behind mutual TLS with a CA of its own.
The parts I tried to break held. Weakening the server's client-certificate check (VerifyClientCertIfGiven, RequireAnyClientCert), skipping server verification on the client, re-enabling keep-alives, or loading the files once on either side each turns a test red. The rolling-upgrade window does not fall back to plain HTTP in either direction: a new client against an old plain server gets "server gave HTTP response to HTTPS client", and an old client against a new server gets a 400. Neither is ErrNotFound, so the restore retries. The stream CA is its own self-signed root, so the apiserver does not accept a stream certificate.
The fallback for an unconfigured satellite is not pinned. It works only because ErrStreamDisabled wraps storage.ErrNotFound. With that changed to a plain errors.New, the stream, controllers and satellite packages still pass. At runtime SnapshotFetcher would then return it as a real error, and crossNodeClone would requeue the restore forever instead of falling back to a resync. One errors.Is(err, storage.ErrNotFound) assertion in TestStreamWithoutTLSIsOff covers it.
"A partial set refuses to start" is not pinned either. Validate has a test, but the call in cmd/satellite does not: replacing it with nil leaves ./cmd/satellite green, and a partial set then quietly turns the stream off.
The rolling-upgrade claim under Testing is tested in one direction only. TestStreamRefusesAPeerWithoutACertificate is the old client against a new server. The direction that matters more is a new client against an old server, because a 404 there would send the restore to a blank volume. An httptest.NewServer answering 404, plus a check that the error is not ErrNotFound, is enough.
pkg/satellite/stream/tls_test.go has no // SPDX-License-Identifier: Apache-2.0 line. AGENTS.md asks for it on every Go file, and this is the only non-generated one in the tree without it.
Non-blocking:
- The stand DaemonSet comment on the stream port still says a closed port means ECONNREFUSED and a DRBD initial sync. Your new comment in
agent.gohas it right: a connection error makes the restore retry. The networking page in the site PR #205 has the same sentence, plus "9100, plain HTTP, no authentication", which this PR and #190 both make stale. Validateonly checks that the three flags are set. A mismatched or unreadable key starts fine, and then every cross-node restore on that node retries on "load the stream certificate". The apiserver'snewReloadableTLSrefuses to boot on bad material; oneload()at startup would do the same here.- The server accepts any client certificate that chains to the stream CA. That is fine while the CA issues exactly one certificate. A
VerifyConnectioncheck forServerNameon the client certificate would keep it true if the Issuer is ever reused. - The PR body still says "Stacked on #190", which is merged.
CI: Integration is red on TestGroupG/SnapCreateListDelete ("Snapshot CRD rd-g-crud.snap-1 did not appear"). #217 fails the same way, and this diff does not touch that path.
Every Go file in the tree carries the SPDX header; the new stream TLS test file was the one without it. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A satellite with no stream TLS falls back to a blank volume and a DRBD resync only because ErrStreamDisabled wraps storage.ErrNotFound. Nothing checked that: with a plain error the fetcher would return it as a real failure and the restore would requeue forever. Assert it on the stream client and on SnapshotFetcher. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The rolling-upgrade claim was tested only for an old client against a new server. The other direction matters more: an old satellite answers 404 for a snapshot it does not hold, and a 404 taken as storage.ErrNotFound would send the restore to a blank volume. Check the fetch fails on the handshake with an error that is not ErrNotFound. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Validate had a test, but its call in cmd/satellite did not: dropping it left the package green, and a partial set of stream TLS flags would then quietly turn the stream off. Move flag parsing into parseFlags, which validates the stream TLS files, and test it on its own FlagSet. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Validate only checked that the three flags were set. A missing file, a CA bundle with no certificate or a key that does not match started fine, and every cross-node restore on the node then retried on the load error. Load the files once in Validate, as the apiserver refuses to boot on bad TLS material. They are still read on every handshake. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The server accepted any client certificate that chains to the stream CA. That holds while the CA issues one certificate; check the client certificate is for ServerName too, so it stays true if the CA is ever used for anything else. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The port comment still said a closed stream port means ECONNREFUSED and a DRBD initial sync. A connection error makes the restore retry; only a satellite with no stream TLS falls back to a resync. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Thanks, head fb9094a.
From the non-blocking ones: the DaemonSet comment says what a closed port does, |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) LGTM. Everything from my last review is fixed, and I reverted each fix once to see a test go red:
ErrStreamDisabledas a plainerrors.NewfailsTestStreamWithoutTLSIsOffandTestSnapshotFetcherWithTheStreamOffFallsBackToAResync.- Dropping the
Validatecall fromparseFlagsfailsTestSatelliteRefusesAPartialStreamTLSConfiguration. - A client back on
httpfailsTestStreamFetchFromAPlainServerIsNotANotFound. Validatewithoutload()failsTestStreamTLSValidateLoadsTheFiles.- Removing
VerifyConnection, or averifyClientNamethat always returns nil, failsTestStreamServerRefusesAClientCertificateForAnotherName.
The rolling upgrade still holds on fb9094a. A new client against an old plain server gets "server gave HTTP response to HTTPS client", and an old client against a new server gets a 400. SnapshotFetcher returns any error that is not a 404, so the restore retries in both directions. Only a satellite without stream TLS gets ErrNotFound and goes to a resync.
On a trial merge with current main, which now has #206, the cmd/satellite and pkg/satellite/... tests pass and golangci-lint is clean on the changed lines. It also merges cleanly with #215 to #218 and #220.
Non-blocking:
- Under Testing the body says a fetch between an old and a new satellite "fails as a connection error". That is true for the new client only. The old client gets an HTTP 400, same outcome.
- The site PR #205 still lists port 3372 as "Plain HTTP, no authentication" and says the stream "has no authentication and no encryption". Both are false once this lands.
Main now binds the unauthenticated port 3370 to the pod's loopback (#215) and runs the snapshot stream on 3372 over mutual TLS with a CA of its own (#219). Say so on the networking, compatibility and installation pages, add the stream certificate and the new bind address to the upgrade note, and list the stream TLS failures that keep a satellite from starting. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The satellite served
GET /v1/satellite/snapshots/...on every node address with no authentication, and the answer is the raw send stream of a snapshot. Any pod that could reach a node IP could read any volume with a snapshot on that node; in Cozystack, platform pods without an egress policy could.The stream now runs over HTTPS with mutual TLS:
blockstor-satellite, and each side verifies the other against the stream CA. Satellites dial each other by node address, so the name is fixed rather than an address.--stream-tls-cert-file,--stream-tls-key-file,--stream-tls-ca-file. With none set the satellite serves and fetches nothing, and cross-node clone and restore fall back to a DRBD initial sync, as they already did when no peer had the snapshot. A partial set refuses to start.blockstor-api-ca. A stream certificate is also a client certificate, and the apiserver accepts any client certificate from its CA, so sharing it would have given every satellite the REST API.Packaging needs the same: a Certificate, the Secret mounted into the DaemonSet, and the three flags. cozystack/cozystack#2942 picks that up with the pin.
Fixes #208
Testing
go test ./...: green apart from the knownpkg/restshutdown flake.