From 4687b2e5975a4038f66424ac59d3217fb895eb16 Mon Sep 17 00:00:00 2001 From: Arseny Sher Date: Tue, 15 Aug 2023 17:58:42 +0300 Subject: [PATCH] Test that auth on pg/http services can be enabled separately in sks. To this end add 1) -e option to 'neon_local safekeeper start' command appending extra options to safekeeper invocation; 2) Allow multiple occurrences of the same option in safekeepers, the last value is taken. 3) Allow to specify empty string for *-auth-public-key-path opts, it disables auth for the service. --- control_plane/src/bin/neon_local.rs | 29 +++++++++++-- control_plane/src/safekeeper.rs | 4 +- safekeeper/src/bin/safekeeper.rs | 53 ++++++++++++++++++++---- test_runner/fixtures/neon_fixtures.py | 16 +++++-- test_runner/regress/test_wal_acceptor.py | 31 +++++++++++++- 5 files changed, 116 insertions(+), 17 deletions(-) diff --git a/control_plane/src/bin/neon_local.rs b/control_plane/src/bin/neon_local.rs index 8f71cb65e2..ef308cb2d2 100644 --- a/control_plane/src/bin/neon_local.rs +++ b/control_plane/src/bin/neon_local.rs @@ -825,6 +825,16 @@ fn get_safekeeper(env: &local_env::LocalEnv, id: NodeId) -> Result Vec { + init_match + .get_many::("safekeeper-extra-opt") + .into_iter() + .flatten() + .map(|s| s.to_owned()) + .collect() +} + fn handle_safekeeper(sub_match: &ArgMatches, env: &local_env::LocalEnv) -> Result<()> { let (sub_name, sub_args) = match sub_match.subcommand() { Some(safekeeper_command_data) => safekeeper_command_data, @@ -841,7 +851,9 @@ fn handle_safekeeper(sub_match: &ArgMatches, env: &local_env::LocalEnv) -> Resul match sub_name { "start" => { - if let Err(e) = safekeeper.start() { + let extra_opts = safekeeper_extra_opts(sub_args); + + if let Err(e) = safekeeper.start(extra_opts) { eprintln!("safekeeper start failed: {}", e); exit(1); } @@ -866,7 +878,8 @@ fn handle_safekeeper(sub_match: &ArgMatches, env: &local_env::LocalEnv) -> Resul exit(1); } - if let Err(e) = safekeeper.start() { + let extra_opts = safekeeper_extra_opts(sub_args); + if let Err(e) = safekeeper.start(extra_opts) { eprintln!("safekeeper start failed: {}", e); exit(1); } @@ -893,7 +906,7 @@ fn handle_start_all(sub_match: &ArgMatches, env: &local_env::LocalEnv) -> anyhow for node in env.safekeepers.iter() { let safekeeper = SafekeeperNode::from_env(env, node); - if let Err(e) = safekeeper.start() { + if let Err(e) = safekeeper.start(vec![]) { eprintln!("safekeeper {} start failed: {:#}", safekeeper.id, e); try_stop_all(env, false); exit(1); @@ -956,6 +969,14 @@ fn cli() -> Command { let safekeeper_id_arg = Arg::new("id").help("safekeeper id").required(false); + let safekeeper_extra_opt_arg = Arg::new("safekeeper-extra-opt") + .short('e') + .long("safekeeper-extra-opt") + .num_args(1) + .action(ArgAction::Append) + .help("Additional safekeeper invocation options, e.g. -e=--http-auth-public-key-path=foo") + .required(false); + let tenant_id_arg = Arg::new("tenant-id") .long("tenant-id") .help("Tenant id. Represented as a hexadecimal string 32 symbols length") @@ -1124,6 +1145,7 @@ fn cli() -> Command { .subcommand(Command::new("start") .about("Start local safekeeper") .arg(safekeeper_id_arg.clone()) + .arg(safekeeper_extra_opt_arg.clone()) ) .subcommand(Command::new("stop") .about("Stop local safekeeper") @@ -1134,6 +1156,7 @@ fn cli() -> Command { .about("Restart local safekeeper") .arg(safekeeper_id_arg) .arg(stop_mode_arg.clone()) + .arg(safekeeper_extra_opt_arg) ) ) .subcommand( diff --git a/control_plane/src/safekeeper.rs b/control_plane/src/safekeeper.rs index 961df38ea2..eb8fe1af17 100644 --- a/control_plane/src/safekeeper.rs +++ b/control_plane/src/safekeeper.rs @@ -101,7 +101,7 @@ impl SafekeeperNode { self.datadir_path().join("safekeeper.pid") } - pub fn start(&self) -> anyhow::Result { + pub fn start(&self, extra_opts: Vec) -> anyhow::Result { print!( "Starting safekeeper at '{}' in '{}'", self.pg_connection_config.raw_address(), @@ -181,6 +181,8 @@ impl SafekeeperNode { ]); } + args.extend(extra_opts); + background_process::start_process( &format!("safekeeper-{id}"), &datadir, diff --git a/safekeeper/src/bin/safekeeper.rs b/safekeeper/src/bin/safekeeper.rs index b04076ab59..8d2201b0eb 100644 --- a/safekeeper/src/bin/safekeeper.rs +++ b/safekeeper/src/bin/safekeeper.rs @@ -15,6 +15,7 @@ use toml_edit::Document; use std::fs::{self, File}; use std::io::{ErrorKind, Write}; use std::path::{Path, PathBuf}; +use std::str::FromStr; use std::sync::Arc; use std::time::Duration; use storage_broker::Uri; @@ -124,18 +125,21 @@ struct Args { disable_wal_backup: bool, /// If given, enables auth on incoming connections to WAL service endpoint /// (--listen-pg). Value specifies path to a .pem public key used for - /// validations of JWT tokens. - #[arg(long, verbatim_doc_comment)] + /// validations of JWT tokens. Empty string is allowed and means disabling + /// auth. + #[arg(long, verbatim_doc_comment, value_parser = opt_pathbuf_parser)] pg_auth_public_key_path: Option, /// If given, enables auth on incoming connections to tenant only WAL /// service endpoint (--listen-pg-tenant-only). Value specifies path to a - /// .pem public key used for validations of JWT tokens. - #[arg(long, verbatim_doc_comment)] + /// .pem public key used for validations of JWT tokens. Empty string is + /// allowed and means disabling auth. + #[arg(long, verbatim_doc_comment, value_parser = opt_pathbuf_parser)] pg_tenant_only_auth_public_key_path: Option, /// If given, enables auth on incoming connections to http management /// service endpoint (--listen-http). Value specifies path to a .pem public - /// key used for validations of JWT tokens. - #[arg(long, verbatim_doc_comment)] + /// key used for validations of JWT tokens. Empty string is allowed and + /// means disabling auth. + #[arg(long, verbatim_doc_comment, value_parser = opt_pathbuf_parser)] http_auth_public_key_path: Option, /// Format for logging, either 'plain' or 'json'. #[arg(long, default_value = "plain")] @@ -146,9 +150,39 @@ struct Args { current_thread_runtime: bool, } +// Like PathBufValueParser, but allows empty string. +fn opt_pathbuf_parser(s: &str) -> Result { + Ok(PathBuf::from_str(s).unwrap()) +} + #[tokio::main(flavor = "current_thread")] async fn main() -> anyhow::Result<()> { - let args = Args::parse(); + // We want to allow multiple occurences of the same arg (taking the last) so + // that neon_local could generate command with defaults + overrides without + // getting 'argument cannot be used multiple times' error. This seems to be + // impossible with pure Derive API, so convert struct to Command, modify it, + // parse arguments, and then fill the struct back. + let cmd = ::command().args_override_self(true); + let mut matches = cmd.get_matches(); + let mut args = ::from_arg_matches_mut(&mut matches)?; + + // I failed to modify opt_pathbuf_parser to return Option in + // reasonable time, so turn empty string into option post factum. + if let Some(pb) = &args.pg_auth_public_key_path { + if pb.as_os_str().is_empty() { + args.pg_auth_public_key_path = None; + } + } + if let Some(pb) = &args.pg_tenant_only_auth_public_key_path { + if pb.as_os_str().is_empty() { + args.pg_tenant_only_auth_public_key_path = None; + } + } + if let Some(pb) = &args.http_auth_public_key_path { + if pb.as_os_str().is_empty() { + args.http_auth_public_key_path = None; + } + } if let Some(addr) = args.dump_control_file { let state = control_file::FileStorage::load_control_file(addr)?; @@ -200,7 +234,10 @@ async fn main() -> anyhow::Result<()> { None } Some(path) => { - info!("loading pg tenant only auth JWT key from {}", path.display()); + info!( + "loading pg tenant only auth JWT key from {}", + path.display() + ); Some(Arc::new( JwtAuth::from_key_path(path).context("failed to load the auth key")?, )) diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 7c7ca94163..62c5bd9ba9 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -1313,12 +1313,20 @@ class NeonCli(AbstractNeonCli): log.info(f"Stopping pageserver with {cmd}") return self.raw_cli(cmd) - def safekeeper_start(self, id: int) -> "subprocess.CompletedProcess[str]": + def safekeeper_start( + self, id: int, extra_opts: Optional[List[str]] = None + ) -> "subprocess.CompletedProcess[str]": s3_env_vars = None if self.env.remote_storage is not None and isinstance(self.env.remote_storage, S3Storage): s3_env_vars = self.env.remote_storage.access_env_vars() - return self.raw_cli(["safekeeper", "start", str(id)], extra_env_vars=s3_env_vars) + if extra_opts is not None: + extra_opts = [f"-e={opt}" for opt in extra_opts] + else: + extra_opts = [] + return self.raw_cli( + ["safekeeper", "start", str(id), *extra_opts], extra_env_vars=s3_env_vars + ) def safekeeper_stop( self, id: Optional[int] = None, immediate=False @@ -2507,9 +2515,9 @@ class Safekeeper: id: int running: bool = False - def start(self) -> "Safekeeper": + def start(self, extra_opts: Optional[List[str]] = None) -> "Safekeeper": assert self.running is False - self.env.neon_cli.safekeeper_start(self.id) + self.env.neon_cli.safekeeper_start(self.id, extra_opts=extra_opts) self.running = True # wait for wal acceptor start by checking its status started_at = time.time() diff --git a/test_runner/regress/test_wal_acceptor.py b/test_runner/regress/test_wal_acceptor.py index 9132efe79f..3b6a6c5ceb 100644 --- a/test_runner/regress/test_wal_acceptor.py +++ b/test_runner/regress/test_wal_acceptor.py @@ -912,7 +912,7 @@ def test_start_replication_term(neon_env_builder: NeonEnvBuilder): assert "failed to acquire term 3" in str(excinfo.value) -# Test auth on WAL service (postgres protocol) ports. +# Test auth on all ports: WAL service (postgres protocol), WAL service tenant only and http. def test_sk_auth(neon_env_builder: NeonEnvBuilder): neon_env_builder.auth_enabled = True env = neon_env_builder.init_start() @@ -946,6 +946,35 @@ def test_sk_auth(neon_env_builder: NeonEnvBuilder): with pytest.raises(psycopg2.OperationalError): connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg_tenant_only, password=full_token) + # Now test that auth on http/pg can be enabled separately. + + # By default, neon_local enables auth on all services if auth is configured, + # so http must require the token. + sk_http_cli_noauth = sk.http_client() + sk_http_cli_auth = sk.http_client(auth_token=env.auth_keys.generate_tenant_token(tenant_id)) + with pytest.raises(sk_http_cli_noauth.HTTPError, match="Forbidden|Unauthorized"): + sk_http_cli_noauth.timeline_status(tenant_id, timeline_id) + sk_http_cli_auth.timeline_status(tenant_id, timeline_id) + + # now, disable auth on http + sk.stop() + sk.start(extra_opts=["--http-auth-public-key-path="]) + sk_http_cli_noauth.timeline_status(tenant_id, timeline_id) # must work without token + # but pg should still require the token + with pytest.raises(psycopg2.OperationalError): + connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg) + connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg, password=tenant_token) + + # now also disable auth on pg, but leave on pg tenant only + sk.stop() + sk.start(extra_opts=["--http-auth-public-key-path=", "--pg-auth-public-key-path="]) + sk_http_cli_noauth.timeline_status(tenant_id, timeline_id) # must work without token + connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg) # must work without token + # but pg tenant only should still require the token + with pytest.raises(psycopg2.OperationalError): + connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg_tenant_only) + connector.safe_psql("IDENTIFY_SYSTEM", port=sk.port.pg_tenant_only, password=tenant_token) + class SafekeeperEnv: def __init__(