Repository navigation
metrics-plugin: add authentication - #12
Conversation
Adds authorization layer for /metrics endpoint which is a bearer token. When a query is performed, under the hood the token gets stripped and compared with the one that is provided to the plugin. Adds --metrics-token-file (path to a file holding the bearer token) and --metrics-disable-auth. The plugin refuses to start unless one of the two is set, so /metrics can't be exposed unauthenticated by accident; the disable flag logs a warning. Reading the token from a file keeps it out of the process list, CLN config, and shell history.
Reject tokens shorter than 16 characters (and empty ones) when reading --metrics-token-file at startup, to catch weak or placeholder values before /metrics is exposed. Updates the README example to generate a random token with openssl instead of a short placeholder.
Switches the token plumbing from Arc<str> to String throughout, and fixes the auth test helper in metrics.rs.
olegfomenko
left a comment
olegfomenko
left a comment
There was a problem hiding this comment.
Overall looks good. Left some comments, most of them are about small refactoring changes. Also, have you thought about using runes for authorization? like, its kind of a native approach for cln, and from the bearer token perspective, we do not care about the nature of the token we are using
| let app = Router::new().route("/metrics", get(metrics_handler)); | ||
| /// Constant-time comparison to avoid leaking the configured token via response-timing side | ||
| /// channels. | ||
| fn tokens_match(provided: &[u8], expected: &[u8]) -> bool { |
There was a problem hiding this comment.
why do we work with bytes? token is already stored as a string, so we can just compare strings
There was a problem hiding this comment.
Ohhh, than question opens a pandora box that I'm really happy to share 😈
The thing is related to so-called remote timing attacks against string comparisons.
Let's consider a case where we compare strings rather than bytes. Rust comrpares string values character by character. This allows an attacker to brute-force a password based on response times. The guard checks the first character, then the second, then the third, and so on. As soon as it encounters an incorrect character, it immediately rejects the input and stops checking.
For example, a guess that matches the first five characters takes slightly longer to reject than one that only matches the first character, simply because the guard has to check more characters before giving up. This creates an opportunity to gradually figure out the correct sequence of characters.
In our case, the comparison function always takes a constant-time approach. It uses a combination of XOR and OR operations to check the result. Even if some characters are incorrect, it processes the entire provided string, making it much harder to determine whether a particular character is invalid based on execution time.
And yes, I know the probability of successfully exploiting this is relatively low, and we also have to account for external noise (e.g. network jitter). However, by performing thousands or even millions of requests and averaging the response times, the actual timing difference (caused by comparing one additional byte) can still emerge statistically.
| == 0 | ||
| } | ||
|
|
||
| fn is_authorized(request: &Request, token: &str) -> bool { |
There was a problem hiding this comment.
also, not sure, but usually i think it may be preferred to fix the token encoding, like URL-compatible Base64. I understand that you may want to leave the decision to the plugin user, but we should be sure that it cant be a potential attack vector
There was a problem hiding this comment.
The encoding choice itself isn't really an attack vector in my opninion - the comparison is already byte-equal and constant-time regardless of what characters are in the token. The HTTP layer (the axum header parser) already rejects control characters like newlines in header values before our code even sees them, so header injection via token should not be possible here
|
Thank you for this review @olegfomenko! You got a lot of really good suggestions over here 🔥 Potentially we can provide rune authentication to Also, applied your suggestions regarding options and token provision (now we use env var). Besides, regarding string comparison for tokens. Normal string comparison stops at the first wrong byte, so response times leak how much of a guess is correct. Our guard instead uses a constant-time XOR/OR comparison over the whole input. The leak is tiny, but averaging many requests can expose it, so this defense matters. So, if you think that we still need to change something - I'm as always open for discussions 🙌 |
Matches the style already used in rpc-log-plugin.
Env vars are easier for devops to inject securely via existing secret tooling (k8s secretKeyRef, systemd EnvironmentFile, Docker secrets) than requiring a file on disk.
It's only used by ServiceExt in tests, so it shouldn't ship in the release binary.
0c0c829 to
b5172f1
Compare
|
LGTM |
Summary
The /metrics endpoint was previously served without any authentication, exposing node liquidity data to anyone who could reach the port. This adds bearer-token auth, gated behind an explicit opt-out for local/dev use.
What's changed
--metrics-token-fileand--metrics-disable-authplugin options. The plugin now refuses to start unless one of them is set, so /metrics can't be exposed unauthenticated by accident./metricsis protected via an axum middleware layer checkingAuthorization: Bearer <token>, using a constant-time comparison to avoid timing side-channels.--metrics-disable-authwarning, curl/Prometheus scrape examples with the bearer token, and guidance to generate the token with openssl rand -hex 32.Related issue: #11