diff --git a/python/python/tests/test_first_class_function_slice2.py b/python/python/tests/test_first_class_function_slice2.py index 56598779e..6db462ae2 100644 --- a/python/python/tests/test_first_class_function_slice2.py +++ b/python/python/tests/test_first_class_function_slice2.py @@ -11,6 +11,7 @@ import types from datetime import date import http.server import json +import os from pathlib import Path import subprocess import sys @@ -178,6 +179,67 @@ def test_a_function_binds_at_most_sixteen_secrets(): assert state["requests"] == [] +_SECRET_DEBUG_LOG_SOURCE = """ +import http.server +import json +import threading + +import lancedb + + +class Handler(http.server.BaseHTTPRequestHandler): + def log_message(self, *args): + pass + + def do_POST(self): + self.rfile.read(int(self.headers.get("Content-Length", "0"))) + payload = json.dumps({}).encode() + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(payload))) + self.end_headers() + self.wfile.write(payload) + + +server = http.server.ThreadingHTTPServer(("localhost", 0), Handler) +threading.Thread(target=server.serve_forever, daemon=True).start() +try: + db = lancedb.connect( + "db://dev", + api_key="API_KEY_SENTINEL", + host_override="http://localhost:%d" % server.server_address[1], + client_config={"retry_config": {"retries": 0}}, + ) + db.create_secret("openai-prod", "SECRET_VALUE_SENTINEL") +finally: + server.shutdown() +""" + + +def test_a_credential_never_reaches_a_debug_log(tmp_path): + """The logger sees the serialized body, so no value-side redaction reaches it. + + Runs in a subprocess because the Rust logger reads ``LANCEDB_LOG`` once, at + import. + """ + script = tmp_path / "write_secret.py" + script.write_text(_SECRET_DEBUG_LOG_SOURCE) + + result = subprocess.run( + [sys.executable, str(script)], + check=True, + capture_output=True, + text=True, + env={**os.environ, "LANCEDB_LOG": "debug"}, + ) + output = result.stdout + result.stderr + + # Without this the test passes when debug logging is simply off. + assert "Sending request_id=" in output, output + assert "SECRET_VALUE_SENTINEL" not in output + assert "API_KEY_SENTINEL" not in output + + def test_a_credential_value_is_rejected_in_the_binding_position(): """The one mistake the typed binding exists to stop.""" with pytest.raises(TypeError, match="EnvVarSecret"): diff --git a/rust/lancedb/src/remote/client.rs b/rust/lancedb/src/remote/client.rs index 57dd89890..6eaa8fcb1 100644 --- a/rust/lancedb/src/remote/client.rs +++ b/rust/lancedb/src/remote/client.rs @@ -404,6 +404,18 @@ fn validate_dns_hostname(hostname: &str) -> Result<()> { Ok(()) } +/// Whether a route's request body is a credential rather than a description of +/// one. +/// +/// Matched on the path segment rather than a versioned prefix, so a `/v2/` bump +/// or a route added under the namespace later is covered without anyone +/// remembering to extend this. Every secrets route is denied, not only the two +/// that carry a value: their bodies hold names and page tokens, which are worth +/// nothing in a debug log next to the risk of a new verb landing here unnoticed. +pub(crate) fn route_carries_credential(path: &str) -> bool { + path.split('/').any(|segment| segment == "secrets") +} + impl RestfulLanceDbClient { fn get_timeout(passed: Option, env_var: &str) -> Result> { if let Some(passed) = passed { @@ -610,12 +622,14 @@ impl RestfulLanceDbClient { ) -> Result { let mut headers = HeaderMap::new(); if !api_key.is_empty() { - headers.insert( - HeaderName::from_static("x-api-key"), - HeaderValue::from_str(api_key).map_err(|_| Error::InvalidInput { - message: "non-ascii api key provided".to_string(), - })?, - ); + // `log_request` prints the request's Debug, which prints headers. + // Marking the value sensitive is what makes that print `Sensitive` + // instead of the key itself. + let mut key = HeaderValue::from_str(api_key).map_err(|_| Error::InvalidInput { + message: "non-ascii api key provided".to_string(), + })?; + key.set_sensitive(true); + headers.insert(HeaderName::from_static("x-api-key"), key); } if region == "local" { let host = format!("{}.local.api.lancedb.com", db_name); @@ -845,7 +859,12 @@ impl RestfulLanceDbClient { .headers() .get("content-type") .map(|v| v.to_str().unwrap()); - if content_type == Some("application/json") { + if route_carries_credential(request.url().path()) { + debug!( + "Sending request_id={}: {:?} with body suppressed", + request_id, request + ); + } else if content_type == Some("application/json") { let body = request.body().as_ref().unwrap().as_bytes().unwrap(); let body = String::from_utf8_lossy(body); debug!( @@ -1192,6 +1211,56 @@ mod tests { assert_eq!(headers.get("x-api-key").unwrap(), "api-key"); } + /// `log_request` prints the request's Debug, and Debug for a request prints + /// its headers. Marking the value sensitive is the only thing standing + /// between the API key and every debug line; assert on the header map's own + /// Debug, which is what that printing reduces to. + #[test] + fn test_api_key_is_redacted_in_debug_output() { + let headers = RestfulLanceDbClient::::default_headers( + "sk-live-sentinel", + "us-east-1", + "db-name", + false, + &RemoteOptions::default(), + None, + &ClientConfig::default(), + ) + .unwrap(); + + assert_eq!(headers.get("x-api-key").unwrap(), "sk-live-sentinel"); + assert!( + !format!("{:?}", headers).contains("sk-live-sentinel"), + "the API key must not survive Debug formatting" + ); + } + + /// Denial follows the path segment, so a verb that does not exist yet and a + /// future API version are both covered without an edit here. + #[test] + fn test_secrets_routes_never_log_a_body() { + for route in [ + "/v1/secrets/create", + "/v1/secrets/alter", + "/v1/secrets/list", + "/v1/secrets/drop", + "/v1/secrets/describe", + "/v2/secrets/rotate", + ] { + assert!( + route_carries_credential(route), + "{route} must never log a body" + ); + } + for route in [ + "/v1/functions/create", + "/v1/table/foo/query", + "/v1/jobs/list", + ] { + assert!(!route_carries_credential(route), "{route} is not a secret"); + } + } + #[test] fn test_rejects_invalid_cloud_dns_hostname() { let invalid_database_names = ["a".repeat(64), "invalid..database".to_string()];