Make remote work over http(s) + cbor - #8283
Conversation
Fill in the open-source remote-mode stub so `below record --port N` serves store frames over a small HTTP+CBOR protocol and remote clients (live, replay, dump, snapshot) can read them. Authentication is a shared bearer token sourced from a Kubernetes Secret via BELOW_REMOTE_TOKEN or BELOW_REMOTE_TOKEN_FILE; the server checks it with a constant-time comparison. Transport security (TLS) is intentionally delegated to the platform (service mesh / ingress), matching typical k8s deployment. - Client (store crate): ureq-based RemoteStore, signature unchanged so no call sites are affected. - Server (below crate): blocking tiny_http server started from the record path under #[cfg(not(fbcode_build))]; the fbcode serving path is untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Serve remote viewing over HTTPS when `record` is given a cert and key: below record --port N --tls-cert server.pem --tls-key server.key Uses tiny_http's rustls-backed TLS. The client opts into TLS via environment (consistent with the env-based token), so no per-command flags are needed: BELOW_REMOTE_TLS=1 connect over HTTPS, verify against system roots BELOW_REMOTE_CA_FILE=<pem> verify against a specific CA bundle BELOW_REMOTE_TLS_INSECURE=1 skip server verification (testing only) The OSS server is now started from the record path (so it can receive the cert/key) rather than from run(); the fbcode serving path is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hi @alexgartrell! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
dschatzberg
left a comment
There was a problem hiding this comment.
Hey Alex, hope you're doing well. Sorry for the long delay in getting to this - it has been busy here.
Have you considered using (apache) thrift for all of this? Kidding - I think this is a sensible approach. I have a bunch of comments inline but overwhelming just want to make sure this isn't a security headache for me to maintain.
| // Start the open-source remote-viewing server. In the fbcode build, serving | ||
| // is handled by `facebook::init` in `run()` instead. | ||
| #[cfg(not(fbcode_build))] | ||
| remote_server::start( |
There was a problem hiding this comment.
This unconditionally starts an unauthenticated remote server by default. Probably we want this to be opt-in?
| if let Ok(path) = std::env::var("BELOW_REMOTE_TOKEN_FILE") { | ||
| let token = std::fs::read_to_string(&path) | ||
| .with_context(|| format!("Failed to read BELOW_REMOTE_TOKEN_FILE {path:?}"))?; | ||
| return Ok(Some(token.trim().to_owned())); |
There was a problem hiding this comment.
We're not validating that the file contents are non-empty here - so if we get an empty file we assume auth is enabled and then later we allow an unauthenticated request to pass through because empty string == empty string
| .find(|h| h.field.equiv("Authorization")) | ||
| .map(|h| h.value.as_str()) | ||
| .and_then(|v| v.strip_prefix("Bearer ")) | ||
| .unwrap_or(""); |
There was a problem hiding this comment.
I don't think this is right - if we're missing an auth header we should just 401 right away, no?
| let timestamp = timestamp.ok_or((400, "missing timestamp".to_owned()))?; | ||
| let direction = direction.ok_or((400, "missing direction".to_owned()))?; | ||
|
|
||
| let target = UNIX_EPOCH + Duration::from_secs(timestamp); |
There was a problem hiding this comment.
timestamp is user-defined input - if this sum exceeds u64::MAX then we'll panic here. Need to do a checked_add for this.
| let frame: Option<(u64, DataFrame)> = | ||
| serde_cbor::from_slice(&buf).context("Failed to deserialize remote frame")?; | ||
|
|
||
| Ok(frame.map(|(ts, df)| (UNIX_EPOCH + Duration::from_secs(ts), df))) |
There was a problem hiding this comment.
This is the same as the previous user-defined-timestamp can cause panic. I'd really want to avoid doing this calculation twice since it's pretty error prone. We should try and turn user-defined input into something valid ASAP
| for _ in 0..NUM_WORKERS { | ||
| let server = server.clone(); | ||
| let state = state.clone(); | ||
| thread::Builder::new() |
There was a problem hiding this comment.
This thread pool looks like it's handling requests only but how does tiny_http handle accepting connections? I briefly looked and it seems like it just spawns more threads as long as its connection threads are busy? That's not great. I think we need to make this opt-in and be clear about the threat model of the remote server we protect against
| if let Some(ca) = std::env::var_os("BELOW_REMOTE_CA_FILE") { | ||
| return Some(TlsMode::Ca(PathBuf::from(ca))); | ||
| } | ||
| if env_truthy("BELOW_REMOTE_TLS") { |
There was a problem hiding this comment.
I think a present but unrecognized value here should be an error? Not silently not use TLS? E.g. if someone sets BELOW_REMOTE_TLS=treu (with that typo) then we won't use TLS
| //! A small blocking HTTP server: GET `/get_frame?timestamp=<unix>&direction=<forward|reverse>` | ||
| //! returns a CBOR-encoded `Option<(u64, DataFrame)>` read from the local store. | ||
| //! | ||
| //! Authentication is a shared bearer token sourced from a Kubernetes Secret via |
There was a problem hiding this comment.
Let's remove Kubernetes specifics here - the interface to below is just through the env
| rustls-pemfile = "2" | ||
| slog = { version = "2.8.2", features = ["max_level_trace", "nested-values"] } | ||
| static_assertions = "1.1.0" | ||
| ureq = { version = "2.12", default-features = false, features = ["tls"] } |
There was a problem hiding this comment.
Why use tls and not the default?
| Some(TlsMode::Insecure) => { | ||
| builder = builder.tls_config(Arc::new(build_insecure_config())); | ||
| } | ||
| // Default TLS uses ureq's built-in rustls config (system roots); |
There was a problem hiding this comment.
Where is this actually happening? I read through the ureq docs and it says:
The rustls-platform-verifier crate provides access to natively checking the certificate via your OS. To use this verifier, you need to enable it using feature flag platform-verifier as well as configure an agent to use it.
Which I didn't see enabled
This PR adds support for remote viewing of below, supporting the use of an auth token as well as TLS (but not mTLS). Care has been taken to minimize impact to the rest of the below code base.
To save time, a quick overview of controversial choices:
First, this uses HTTP + CBOR rather than grpc or open source thrift. The reason for this is to avoid keeping another IDL spec and to keep changes to the rest of the binary pretty minimal.
Second, the configuration is happening through env vars instead of additional CLI arguments. This can change as well but requires more plumbing.
Third, it just uses token auth. mTLS is the most correct way to deal with this IMO but it's not easily done outside of a Meta context and you can bind to localhost and proxy it if you're super worried about it.