diff --git a/AGENTS.md b/AGENTS.md index 97dbf65bd..e15d63218 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -212,6 +212,8 @@ export EXTENDDB_TEST_ENDPOINT=https://127.0.0.1:18443 export EXTENDDB_ADMIN_USER=admin export EXTENDDB_ADMIN_PASSWORD= eval $(python3 devtools/provision-test-credentials) +# The access key import tests need import enabled. devtools/run-tests sets it. +./target/release/extenddb settings --config extenddb.toml set allow_credential_import true pytest tests/ -v ``` diff --git a/crates/server/src/management/iam_user_self.rs b/crates/server/src/management/iam_user_self.rs index 3bac56a0e..249a2680d 100755 --- a/crates/server/src/management/iam_user_self.rs +++ b/crates/server/src/management/iam_user_self.rs @@ -8,6 +8,7 @@ use std::sync::Arc; use axum::extract::{ConnectInfo, Path, State}; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; +use extenddb_storage::management_store::SettingsStore; use serde::{Deserialize, Serialize}; use super::ManagementState; @@ -223,6 +224,27 @@ pub async fn change_user_password( } } +/// Checks the runtime setting `allow_credential_import` before +/// [`import_access_key`] stores a key. Only the stored value `true` allows the +/// import. No row, `false`, and any other value refuse it with 403, because an +/// import stores an access key ID and secret that the caller chooses. A store +/// error is answered with 500. +#[allow(clippy::result_large_err)] // axum Response as rejection is this module's idiom +async fn credential_import_gate(settings: &dyn SettingsStore) -> Result<(), Response> { + match settings.get_setting("allow_credential_import").await { + Ok(Some(value)) if value == "true" => Ok(()), + Ok(_) => Err(( + StatusCode::FORBIDDEN, + "Credential import is disabled (allow_credential_import = false)", + ) + .into_response()), + Err(e) => { + tracing::error!("Management API: check allow_credential_import failed: {e:?}"); + Err(StatusCode::INTERNAL_SERVER_ERROR.into_response()) + } + } +} + /// `POST /management/accounts/{id}/users/{name}/access-keys/import` — import an external access key. pub async fn import_access_key( State(state): State>, @@ -240,26 +262,8 @@ pub async fn import_access_key( return e; } - // Check runtime setting gate. - let allowed: bool = match state - .catalog_store - .get_setting("allow_credential_import") - .await - { - Ok(Some(v)) => v == "true", - Ok(None) => true, // Default: allowed. - Err(e) => { - tracing::error!("Management API: check allow_credential_import failed: {e:?}"); - return StatusCode::INTERNAL_SERVER_ERROR.into_response(); - } - }; - - if !allowed { - return ( - StatusCode::FORBIDDEN, - "Credential import is disabled (allow_credential_import = false)", - ) - .into_response(); + if let Err(refused) = credential_import_gate(&*state.catalog_store).await { + return refused; } // Validate access key ID format. @@ -302,3 +306,102 @@ pub async fn import_access_key( Err(e) => op_err_to_response(OpError::from_storage(e)), } } + +#[cfg(test)] +mod tests { + use extenddb_storage::management_store::{OpError, OpResult}; + use futures::future::BoxFuture; + + use super::*; + use crate::rate_limit::tests::capture_logs; + + /// What the settings store answers for `allow_credential_import`. + enum Setting { + /// The catalog has no row for the key. + NoRow, + /// The catalog holds this value. + Value(&'static str), + /// The read fails. + StoreError, + } + + impl SettingsStore for Setting { + fn get_setting(&self, key: &str) -> BoxFuture<'_, OpResult>> { + assert_eq!(key, "allow_credential_import"); + let answer = match self { + Self::NoRow => Ok(None), + Self::Value(value) => Ok(Some((*value).to_owned())), + Self::StoreError => Err(OpError::Internal("store is down".to_owned())), + }; + Box::pin(async move { answer }) + } + + fn set_setting(&self, _: &str, _: &str) -> BoxFuture<'_, OpResult<()>> { + unreachable!("the import gate does not write settings") + } + + fn list_settings(&self) -> BoxFuture<'_, OpResult>> { + unreachable!("the import gate does not list settings") + } + } + + /// The gate's answer as a status code, with the body of a refusal. + async fn gate(setting: Setting) -> Result<(), (StatusCode, String)> { + match credential_import_gate(&setting).await { + Ok(()) => Ok(()), + Err(response) => { + let status = response.status(); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .expect("the refusal body is in memory"); + Err((status, String::from_utf8_lossy(&body).into_owned())) + } + } + } + + fn refused() -> Result<(), (StatusCode, String)> { + Err(( + StatusCode::FORBIDDEN, + "Credential import is disabled (allow_credential_import = false)".to_owned(), + )) + } + + #[tokio::test] + async fn no_row_refuses_the_import() { + assert_eq!(gate(Setting::NoRow).await, refused()); + } + + #[tokio::test] + async fn false_refuses_the_import() { + assert_eq!(gate(Setting::Value("false")).await, refused()); + } + + #[tokio::test] + async fn true_allows_the_import() { + assert_eq!(gate(Setting::Value("true")).await, Ok(())); + } + + #[tokio::test] + async fn a_value_other_than_true_refuses_the_import() { + // The settings API writes only `true` or `false`. A row edited in the + // catalog by hand can hold any other value. + for value in ["TRUE", "True", "1", "yes", "", " true"] { + assert_eq!(gate(Setting::Value(value)).await, refused(), "{value:?}"); + } + } + + #[tokio::test] + async fn a_store_error_answers_500_and_is_logged() { + let (logs, _guard) = capture_logs(); + assert_eq!( + gate(Setting::StoreError).await, + Err((StatusCode::INTERNAL_SERVER_ERROR, String::new())) + ); + let text = logs.text(); + assert!( + text.contains("ERROR") + && text.contains("Management API: check allow_credential_import failed"), + "{text}" + ); + } +} diff --git a/devtools/run-tests b/devtools/run-tests index ae7984896..8846c80eb 100755 --- a/devtools/run-tests +++ b/devtools/run-tests @@ -334,6 +334,11 @@ if $NEEDS_INTEGRATION && [[ "$TARGET" != "real-dynamodb" ]]; then if "$BINARY" settings --config "$CONFIG_FOR_SETTINGS" set throttling_enabled true 2>/dev/null; then echo " ✓ throttling_enabled set to true" fi + # Access key import is refused unless this is true. Set once here, before + # any suite starts, so no test changes it while parallel workers run. + if "$BINARY" settings --config "$CONFIG_FOR_SETTINGS" set allow_credential_import true 2>/dev/null; then + echo " ✓ allow_credential_import set to true" + fi fi # Import/export paths. The Python tests write fixtures via tempfile, which diff --git a/docs/differences-from-dynamodb.md b/docs/differences-from-dynamodb.md index 0686d8445..b719eedbc 100755 --- a/docs/differences-from-dynamodb.md +++ b/docs/differences-from-dynamodb.md @@ -209,7 +209,7 @@ ExtendDB exposes runtime settings that have no DynamoDB equivalent: | `enable_multipart_keys` | `false` | Enable multi-part base table key extension | | `log_level` | `info` | Runtime log level (trace, debug, info, warn, error) | | `sqlx_log_level` | `warn` | Separate log level for sqlx query traces | -| `allow_credential_import` | `true` | Allow importing credentials via the management API | +| `allow_credential_import` | `false` | Allow importing credentials via the management API. Only `true` enables it | ## Web Console diff --git a/docs/getting-started.md b/docs/getting-started.md index 712f34cfe..7bd76a44b 100755 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -355,16 +355,16 @@ By default, control plane operations (CreateTable, DeleteTable) emulate real Dyn ### Credential Import -Controls whether `extenddb manage import-access-key` is allowed (default: `true`). +Controls whether `extenddb manage import-access-key` is allowed (default: `false`). An import stores an access key ID and secret that the caller chooses, so only a deployment that sets this to `true` accepts imported keys. ```bash -# Disable credential import +# Enable credential import ./target/release/extenddb settings --config extenddb.toml set \ - allow_credential_import false + allow_credential_import true -# Re-enable +# Disable it again ./target/release/extenddb settings --config extenddb.toml set \ - allow_credential_import true + allow_credential_import false ``` ### GSI Propagation Delay @@ -918,6 +918,13 @@ Admins can also create access keys on behalf of any user: Import real AWS credentials (or any AKIA* key) into extenddb. This enables the "just change the endpoint URL" workflow — use the same credentials against both extenddb and real DynamoDB. +The `allow_credential_import` runtime setting gates credential import (default: `false`). Enable it before the first import: + +```bash +./target/release/extenddb settings --config extenddb.toml set \ + allow_credential_import true +``` + Both `--secret-access-key` and `--yes` are required: ```bash @@ -930,12 +937,7 @@ Both `--secret-access-key` and `--yes` are required: > **Security note:** `--secret-access-key` and `--password` are visible in `ps` output and shell history. For sensitive environments, use the `EXTENDDB_ADMIN_PASSWORD` environment variable for the password. -Credential import is gated by the `allow_credential_import` runtime setting (default: `true`). To disable: - -```bash -./target/release/extenddb settings --config extenddb.toml set \ - allow_credential_import false -``` +Set `allow_credential_import` back to `false` to refuse further imports. Keys already imported keep working. Every import is logged to syslog (access key ID, account, user — never the secret). diff --git a/docs/manuals/05-admin-guide.md b/docs/manuals/05-admin-guide.md index e51da206c..b76c86efb 100755 --- a/docs/manuals/05-admin-guide.md +++ b/docs/manuals/05-admin-guide.md @@ -186,7 +186,7 @@ Managed via `extenddb settings set`. Changes take effect within 30 seconds witho |---------|---------|-------------| | `log_level` | `info` | Log level: trace, debug, info, warn, error | | `control_plane_delay_seconds` | `0.25` | Delay for table status transitions, in seconds (0 = instant). PostgreSQL and SQLite write 0.25 into the catalog when it is created; MongoDB and Cassandra use 0.25 when the setting is absent. On PostgreSQL and SQLite a value below 1 makes DeleteTable remove the table in the request; on PostgreSQL, if a session holds a lock on the table's data (a reader, an open write transaction, an index build), DeleteTable returns `DELETING` instead (after at most about a second), the drop finishes in the background within about a second of that session's release, and a CreateTable of the same name is refused until then. | -| `allow_credential_import` | `true` | Whether `import-access-key` is allowed | +| `allow_credential_import` | `false` | Whether `import-access-key` is allowed. Only `true` enables it | | `vector_backfill_batch_delay_ms` | `0` | **Test-oriented.** Milliseconds to pause between batches while a vector index backfills. Zero in production. A test sets it so a write is guaranteed to land while the index is still building. The pause is outside the batch transaction, so writes are still accepted throughout, but it does extend the per-table propagation hold: no index on that table advances while the build runs, GSIs included, and the accepted range goes up to 60 s per batch. | | `vector_allocation_phase_delay_ms` | `0` | **Test-oriented.** Milliseconds to hold a new vector index in the resource-allocation phase (`CREATING` with `Backfilling: false`) before the scan starts. Zero in production. Without it the phase lasts only from the `UpdateTable` transaction, which inserts the row as `CREATING` with `Backfilling: false`, until the detached build task flips the flag, which is a window no client can time reliably rather than one that cannot exist. | @@ -290,7 +290,9 @@ Account IDs must be 12-digit numeric strings (matching AWS format). If `--accoun delete-access-key --account-id 123456789012 \ --user-name alice --access-key-id AKIAEXTENDDB... -# Import existing credentials +# Import existing credentials (refused unless allow_credential_import is true) +./target/release/extenddb settings --config extenddb.toml set \ + allow_credential_import true ./target/release/extenddb manage --user admin --password \ import-access-key --account-id 123456789012 --user-name alice \ --access-key-id AKIAIOSFODNN7EXAMPLE \ diff --git a/docs/manuals/06-developer-test-guide.md b/docs/manuals/06-developer-test-guide.md index 4e97f24b0..9067acaba 100755 --- a/docs/manuals/06-developer-test-guide.md +++ b/docs/manuals/06-developer-test-guide.md @@ -158,6 +158,7 @@ The `run-tests` script automatically: - Sets `control_plane_delay_seconds` to 0.05 for fast test cycles - Sets `index_propagation_delay_ms` to 0 for immediate GSI updates - Enables throttling for production-like behavior +- Sets `allow_credential_import` to `true` for the access key import tests - Configures import/export paths for file operation tests - Extracts and exports `EXTENDDB_TEST_PG_CONNECTION_STRING` for CLI lifecycle tests @@ -195,6 +196,8 @@ devtools/run-tests --extenddb --pytest --parallel devtools/run-tests --extenddb --pytest --parallel=4 # Direct execution (for quick iteration during development) +# The access key import tests need import enabled. devtools/run-tests sets it. +./target/debug/extenddb settings --config extenddb.toml set allow_credential_import true python3 -m pytest tests/ -v python3 -m pytest tests/test_put_item.py -v python3 -m pytest tests/test_put_item.py::test_put_item_basic -v diff --git a/docs/manuals/07-upgrade-manual.md b/docs/manuals/07-upgrade-manual.md index b1e3295e3..deb7e9c0d 100755 --- a/docs/manuals/07-upgrade-manual.md +++ b/docs/manuals/07-upgrade-manual.md @@ -411,6 +411,38 @@ migration is required. --- +### v0.1.15: access key import is refused unless `allow_credential_import` is `true` + +**What changed.** `extenddb manage import-access-key`, and the management API +route it calls, `POST /management/accounts/{id}/users/{name}/access-keys/import`, +import a key only when the runtime setting `allow_credential_import` is `true`. +When the catalog has no row for the setting, the server refuses the import with +HTTP 403 and `Credential import is disabled (allow_credential_import = false)`. +Before this release the server allowed the import when the row was missing. No +release writes the row, so every deployment that never set it accepted imports. +An import stores an access key ID and secret that the caller chooses. With the +feature off by default, only deployments that turn it on accept caller-chosen +keys. The console settings page already showed `false` as the default. The +setting does not affect keys imported before the upgrade, or the credentials a +dev-mode server seeds at startup. + +**Who is affected.** Deployments that import access keys and never set +`allow_credential_import`. A deployment that set it to `true` or `false` keeps +its behavior. + +**Migration.** Before upgrading, enable the setting on each deployment that +imports keys: + +```bash +extenddb settings --config extenddb.toml set allow_credential_import true +``` + +The command writes the row to the catalog. The server reads the row on each +import, so the command works with the earlier release and needs no restart. No +catalog migration is required. + +--- + ### v0.1.14 LeadingKeys now enforced on Query, batch and transaction operations **What changed.** `dynamodb:LeadingKeys` is now evaluated on `Query`, diff --git a/docs/manuals/11-deployment-guide.md b/docs/manuals/11-deployment-guide.md index 6950552fe..891109cda 100755 --- a/docs/manuals/11-deployment-guide.md +++ b/docs/manuals/11-deployment-guide.md @@ -204,7 +204,7 @@ extenddb init \ - [ ] Set `auth.provider = "builtin"` in `extenddb.toml` - [ ] Change the admin password from the auto-generated one - [ ] Create named accounts and IAM users with least-privilege policies -- [ ] Disable credential import if not needed: `extenddb settings set allow_credential_import false` +- [ ] Leave credential import off unless you need it. The server refuses imports unless `allow_credential_import` is `true`. If you enabled it to import keys, turn it off again: `extenddb settings set allow_credential_import false` ### Transport diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index f22a2c4d1..b4c9ba6fd 100755 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -614,6 +614,16 @@ The error log line names the exact relations for one leaked table and gives this **Fix:** Check PostgreSQL connectivity and logs. +### `Credential import is disabled (allow_credential_import = false)` + +**Cause:** The server refused an access key import (`extenddb manage import-access-key`, or `POST /management/accounts/{id}/users/{name}/access-keys/import`) with HTTP 403 because the `allow_credential_import` runtime setting is not `true`. The message is the same when the catalog has no row for the setting. From v0.1.15 the server refuses the import when the row is missing. A deployment that imported keys before the upgrade without setting it gets this error after the upgrade. + +**Fix:** Enable import, then retry. The next import reads the new value, so you do not need to restart the server: +```bash +extenddb settings --config extenddb.toml set allow_credential_import true +``` +Keys imported earlier keep working while import is disabled. + ### `Management API: check allow_credential_import failed: ` **Cause:** The query to check the `allow_credential_import` runtime setting failed during an access key import. Likely a database connectivity issue. diff --git a/tests/cli/test-cli-comprehensive.sh b/tests/cli/test-cli-comprehensive.sh index 9621e9ba0..e07404568 100755 --- a/tests/cli/test-cli-comprehensive.sh +++ b/tests/cli/test-cli-comprehensive.sh @@ -549,7 +549,9 @@ $EXTENDDB manage --config "$CONFIG" --user "${ACCOUNT_ID1}/user1" --password 'Us list-access-keys --account-id "$ACCOUNT_ID1" --user-name user1 >/dev/null 2>&1; RC=$? assert_ok $RC "self-service list-access-keys" -# import-access-key +# import-access-key (refused unless allow_credential_import is true) +$EXTENDDB settings --config "$CONFIG" set allow_credential_import true >/dev/null 2>&1; RC=$? +assert_ok $RC "settings set allow_credential_import true" manage import-access-key --account-id "$ACCOUNT_ID1" --user-name user1 \ --access-key-id AKIAIOSFODNN7EXAMPLE --secret-access-key wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY --yes >/dev/null 2>&1; RC=$? assert_ok $RC "import-access-key" diff --git a/tests/test_allow_credential_import_sqlite.py b/tests/test_allow_credential_import_sqlite.py new file mode 100644 index 000000000..31cdd7aed --- /dev/null +++ b/tests/test_allow_credential_import_sqlite.py @@ -0,0 +1,252 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Access key import on a SQLite deployment with no ``allow_credential_import`` row. + +``devtools/run-tests`` stores ``allow_credential_import = true`` on the shared +server before any suite starts, so no other integration test reaches the +import route on a catalog without the row. This file runs +``init --backend sqlite`` in a temporary directory, serves that deployment on a +free port on 127.0.0.1, and drives the route through the management API: + + * with no row, the import answers 403 and the key does not list; + * after ``extenddb settings`` stores ``true`` while the server runs, the same + import answers 201 and the key lists; + * after the setting changes to ``false``, a second import answers 403 and the + first key still lists. + +The deployment has its own catalog, so the test does not change the shared +server's settings while other xdist workers import keys. The module skips +unless the ``extenddb`` binary is a SQLite build. +""" + +from __future__ import annotations + +import os +import re +import secrets +import shutil +import socket +import sqlite3 +import ssl +import subprocess +import tempfile +import time +import urllib.request +import uuid + +import pytest + +from management_helpers import ManagementClient + +EXTENDDB_BINARY = os.environ.get( + "EXTENDDB_BINARY", + os.path.join(os.path.dirname(__file__), "..", "target", "release", "extenddb"), +) + +REFUSED = "Credential import is disabled (allow_credential_import = false)" + + +def _free_port() -> int: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.bind(("127.0.0.1", 0)) + return s.getsockname()[1] + + +def _wait_for_health(port: int, timeout: float = 20.0) -> bool: + ctx = ssl.create_default_context() + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + try: + with urllib.request.urlopen( + f"https://127.0.0.1:{port}/health", context=ctx, timeout=2 + ) as resp: + if resp.status == 200: + return True + except Exception: + time.sleep(0.3) + return False + + +def _patch_port(config_path: str, port: int) -> None: + with open(config_path) as f: + content = f.read() + new, n = re.subn( + r"(?m)^[ \t]*#?[ \t]*port[ \t]*=[ \t]*\d+.*$", f"port = {port}", content, count=1 + ) + if n == 0: + new = content.replace( + 'bind_addr = "127.0.0.1"', f'bind_addr = "127.0.0.1"\nport = {port}', 1 + ) + with open(config_path, "w") as f: + f.write(new) + + +class _Deployment: + """A file-backed SQLite deployment in its own directory, served on 127.0.0.1.""" + + def __init__(self, root: str) -> None: + self.dir = root + self.config = os.path.join(root, "extenddb.toml") + self.log_path = os.path.join(root, "serve.log") + self.port = _free_port() + self.admin_password = secrets.token_urlsafe(24) + # `init` writes the TLS certificate and key under ~/.extenddb. A home + # directory inside the deployment keeps them out of the user's home. + self.env = {**os.environ, "HOME": os.path.join(root, "home")} + self.proc: subprocess.Popen | None = None + + def run(self, *args: str, env: dict[str, str] | None = None) -> subprocess.CompletedProcess: + # `--config` goes right after the subcommand: `settings` requires it + # before its `set` action. + return subprocess.run( + [EXTENDDB_BINARY, args[0], "--config", self.config, *args[1:]], + cwd=self.dir, + env=env or self.env, + capture_output=True, + text=True, + stdin=subprocess.DEVNULL, + timeout=60, + check=False, + ) + + def init(self) -> None: + # The admin password comes from the environment, so `init` does not + # print it. + res = self.run( + "init", + "--backend", + "sqlite", + "--bind-addr", + "127.0.0.1", + env={ + **self.env, + "EXTENDDB_ADMIN_USER": "admin", + "EXTENDDB_ADMIN_PASSWORD": self.admin_password, + }, + ) + assert res.returncode == 0, f"init failed: {res.stderr}" + _patch_port(self.config, self.port) + + def start(self) -> None: + with open(self.log_path, "wb") as log: + self.proc = subprocess.Popen( + [EXTENDDB_BINARY, "serve", "--config", self.config, "--foreground"], + cwd=self.dir, + env=self.env, + stdin=subprocess.DEVNULL, + stdout=log, + stderr=subprocess.STDOUT, + ) + if not _wait_for_health(self.port): + with open(self.log_path, errors="replace") as f: + tail = f.read()[-2000:] + pytest.fail(f"server did not become healthy on 127.0.0.1:{self.port}:\n{tail}") + + def stop(self) -> None: + if self.proc is None: + return + # `serve --foreground` shuts down gracefully on SIGTERM. + self.proc.terminate() + try: + self.proc.wait(timeout=30) + except subprocess.TimeoutExpired: + self.proc.kill() + self.proc.wait(timeout=10) + self.proc = None + + def stored_setting(self, key: str) -> str | None: + # `init --backend sqlite` creates the catalog in the working directory. + conn = sqlite3.connect(os.path.join(self.dir, "extenddb.sqlite"), timeout=10) + try: + row = conn.execute("SELECT value FROM settings WHERE key = ?", (key,)).fetchone() + finally: + conn.close() + return None if row is None else row[0] + + def set_setting(self, key: str, value: str) -> None: + res = self.run("settings", "set", key, value) + assert res.returncode == 0, f"settings set {key} {value} failed: {res.stderr}" + + +@pytest.fixture() +def deployment(request) -> _Deployment: + """A started deployment; stopped and deleted on teardown.""" + if not os.path.isfile(EXTENDDB_BINARY): + pytest.skip(f"extenddb binary not found at {EXTENDDB_BINARY}") + version = subprocess.run( + [EXTENDDB_BINARY, "version"], + capture_output=True, + text=True, + stdin=subprocess.DEVNULL, + timeout=30, + check=False, + ) + # `version` prints `catalog