metrics-plugin: add authentication - #12
Nazarevsky wants to merge 3 commits into
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
| "How often (seconds) to refresh CLN node data for gauge metrics", | ||
| ); | ||
|
|
||
| const OPT_METRICS_TOKEN_FILE: ConfigOption<'static, cln_plugin::options::config_type::String> = |
There was a problem hiding this comment.
There are already StringConfigOption and DefaultStringConfigOption types declared in cln_plugin::options
| "Path to a file containing the bearer token required to access the /metrics endpoint", | ||
| ); | ||
|
|
||
| const OPT_METRICS_DISABLE_AUTH: ConfigOption< |
There was a problem hiding this comment.
There are already BoolConfigOption and DefaultBoolConfigOption declared in cln_plugin::options
|
|
||
| /// Reads the bearer token from `path`, trimming surrounding whitespace so a trailing newline | ||
| /// left by `echo` or an editor doesn't become part of the expected token. | ||
| async fn read_token_file(path: &str) -> Result<String> { |
There was a problem hiding this comment.
Not sure about storing token in file... i think devopses may prefer envs cause it may be easier for them to setup secrets securely. With env approach, we can just leave the disable auth option, and if it wasnt set require the existence of env. What fo you think?
| 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
| == 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
| serde_json = { version = "1" } | ||
| anyhow = { version = "1" } | ||
| tracing = { version = "0.1" } | ||
| tower = { version = "0.5", features = ["util"] } |
There was a problem hiding this comment.
looks like you're using it only in tests, then I'd rather move it into [dev-dependencies]
2 participants
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