diff --git a/python/python/tests/test_first_class_function_slice2.py b/python/python/tests/test_first_class_function_slice2.py index 24179d867..2e789eea0 100644 --- a/python/python/tests/test_first_class_function_slice2.py +++ b/python/python/tests/test_first_class_function_slice2.py @@ -145,45 +145,13 @@ def test_bindings_may_not_collide_with_plain_configuration(): ) -def test_a_function_binds_at_most_sixteen_secrets(): - """The cap lives in Rust, so no language surface can be talked past it. +def test_a_binding_envelope_reaches_the_service_for_it_to_judge(): + """Binding rules are the service's: it owns the runtime the names land in. - Registering through the typed API and hand-rolling the request envelope - reach the same boundary, and neither reaches the wire. - """ - bindings = [ - EnvVarSecret(secret=f"secret-{index}", env_variable=f"TOKEN_{index}") - for index in range(17) - ] - with _mock_remote_function_catalog() as (host, state): - db = lancedb.connect( - "db://dev", - api_key="fake", - host_override=host, - client_config={"retry_config": {"retries": 0}}, - ) - with pytest.raises(ValueError, match="at most 16 secrets"): - db.create_function(normalize_score, secrets=bindings) - - envelope = json.loads(normalize_score.registration_request.to_canonical_json()) - envelope["secret_env_bindings"] = { - f"TOKEN_{index}": f"secret-{index}" for index in range(17) - } - - async def submit_envelope(): - return await db._conn._inner.create_function_async(json.dumps(envelope)) - - with pytest.raises(ValueError, match="at most 16 secrets"): - LOOP.run(submit_envelope()) - - assert state["requests"] == [] - - -def test_binding_names_are_validated_below_the_python_api(): - """The low-level entry point reaches the same validator the typed API does. - - Registration envelopes can be hand-rolled past ``bind_secrets``, so the - grammar and the disjointness rule live in Rust, above the backend. + The client sends what it was given, so a rule it duplicated could disagree + with the service's without either side noticing. What is checked here is + that the envelope arrives intact -- the shape the service judges is the + shape the caller wrote. """ with _mock_remote_function_catalog() as (host, state): db = lancedb.connect( @@ -192,20 +160,15 @@ def test_binding_names_are_validated_below_the_python_api(): host_override=host, client_config={"retry_config": {"retries": 0}}, ) - envelope = json.loads(analyze_caption.registration_request.to_canonical_json()) - envelope["secret_env_bindings"] = { - "BAD=NAME": "openai-prod", - "TOKEN_0": "secret-0", - } - envelope["runtime"]["env"]["TOKEN_0"] = "public" + bindings = [ + EnvVarSecret(secret=f"secret-{index}", env_variable=f"TOKEN_{index}") + for index in range(17) + ] + db.create_function(normalize_score, secrets=bindings) - async def submit_envelope(): - return await db._conn._inner.create_function_async(json.dumps(envelope)) - - with pytest.raises(ValueError, match="portable"): - LOOP.run(submit_envelope()) - - assert state["requests"] == [] + sent = state["requests"][0][1] + assert len(sent["secret_env_bindings"]) == 17 + assert sent["secret_env_bindings"]["TOKEN_0"] == "secret-0" _SECRET_DEBUG_LOG_SOURCE = """ diff --git a/rust/lancedb/src/connection.rs b/rust/lancedb/src/connection.rs index 6f901fa8d..e4464c187 100644 --- a/rust/lancedb/src/connection.rs +++ b/rust/lancedb/src/connection.rs @@ -24,7 +24,7 @@ use crate::data::scannable::Scannable; use crate::database::listing::ListingDatabase; use crate::database::{ CloneTableRequest, Database, DatabaseOptions, JobInfo, OpenTableRequest, ReadConsistency, - SecretInfo, TableNamesRequest, + TableNamesRequest, }; use crate::embeddings::{EmbeddingRegistry, MemoryRegistry}; use crate::error::{Error, Result}; @@ -36,6 +36,7 @@ use crate::remote::{ OPT_REMOTE_SQL_HOST_OVERRIDE, }, }; +use crate::secrets::SecretInfo; use lance::io::ObjectStoreParams; pub use lance_file::version::LanceFileVersion; #[cfg(feature = "remote")] @@ -587,14 +588,10 @@ impl Connection { /// returned typed job yields the durable [`crate::function::FunctionVersion`]. /// Local databases return [`Error::NotSupported`]. /// - /// The request's binding shape is validated here rather than in any one - /// language binding, so every client surface rejects the same envelopes - /// before one reaches the wire. pub async fn create_function_async( &self, request: crate::function::FunctionRegistrationRequest, ) -> Result> { - request.validate()?; self.internal.create_function_async(request).await } @@ -657,9 +654,9 @@ impl Connection { /// consumer is a Function that binds the Secret by name. Local databases /// return [`Error::NotSupported`]. pub async fn create_secret(&self, name: impl AsRef, value: impl AsRef) -> Result<()> { - let value = value.as_ref(); - crate::function::validate_secret_value(value)?; - self.internal.create_secret(name.as_ref(), value).await + self.internal + .create_secret(name.as_ref(), value.as_ref()) + .await } /// Replace the credential behind an existing Secret. @@ -670,9 +667,9 @@ impl Connection { /// version registered before it. Local databases return /// [`Error::NotSupported`]. pub async fn alter_secret(&self, name: impl AsRef, value: impl AsRef) -> Result<()> { - let value = value.as_ref(); - crate::function::validate_secret_value(value)?; - self.internal.alter_secret(name.as_ref(), value).await + self.internal + .alter_secret(name.as_ref(), value.as_ref()) + .await } /// The names of every Secret in this database. diff --git a/rust/lancedb/src/database.rs b/rust/lancedb/src/database.rs index f4988d217..900542ab1 100644 --- a/rust/lancedb/src/database.rs +++ b/rust/lancedb/src/database.rs @@ -28,6 +28,7 @@ use lance_namespace::models::{ use crate::data::scannable::Scannable; use crate::error::Result; +use crate::secrets::SecretInfo; use crate::table::{BaseTable, WriteOptions}; pub mod listing; @@ -258,20 +259,6 @@ fn secret_catalog_not_supported() -> Result { /// The `Database` trait defines the interface for database implementations. /// /// A database is responsible for managing tables and their metadata. -/// What a database records about a Secret. Never its value. -/// -/// Returned by [`crate::connection::Connection::describe_secret`]. There is no -/// field for the credential and no method that could produce one. -#[derive(Debug, Clone, PartialEq, Eq, serde::Deserialize)] -pub struct SecretInfo { - /// The Secret's database-scoped name. - pub name: String, - /// When the Secret was created, as an RFC 3339 timestamp. - pub created_at: String, - /// When the Secret's value was last rotated, as an RFC 3339 timestamp. - pub updated_at: String, -} - #[async_trait::async_trait] pub trait Database: Send + Sync + std::any::Any + std::fmt::Debug + std::fmt::Display + 'static diff --git a/rust/lancedb/src/function.rs b/rust/lancedb/src/function.rs index 51f6509d0..e14aa1fa8 100644 --- a/rust/lancedb/src/function.rs +++ b/rust/lancedb/src/function.rs @@ -493,56 +493,6 @@ pub struct FunctionArtifactRequest { pub adapter: PythonAdapterSpec, } -/// A Function binds at most this many Secrets to environment variables. -/// -/// Each bound Secret is one extra read on the launch path of every fragment, so -/// the count needs a bound for the same reason a credential needs a size limit. -pub const MAX_FUNCTION_SECRET_ENV_BINDINGS: usize = 16; - -/// Largest credential a Secret may hold, matching the limit the service -/// enforces. Bounded because the value is destined for a process environment. -pub const MAX_SECRET_VALUE_BYTES: usize = 64 * 1024; - -/// Whether `name` is a portable POSIX environment variable name. -/// -/// Leading letter or underscore, then letters, digits, or underscores. Names -/// reserved by the execution sandbox are deliberately not checked here: that -/// list belongs to the runtime that owns it, and a copy in the client would -/// drift from it silently. -fn is_portable_env_name(name: &str) -> bool { - let mut bytes = name.bytes(); - bytes - .next() - .is_some_and(|byte| byte == b'_' || byte.is_ascii_alphabetic()) - && bytes.all(|byte| byte == b'_' || byte.is_ascii_alphanumeric()) -} - -/// Reject a credential the service would refuse on size alone. -/// -/// Checked before the request body is built, so an oversized value is never -/// serialized or uploaded. -pub(crate) fn validate_secret_value(value: &str) -> Result<()> { - if value.is_empty() { - return Err(Error::InvalidInput { - message: "a Secret value must not be empty".to_string(), - }); - } - if value.contains('\0') { - return Err(Error::InvalidInput { - message: "a Secret value must not contain NUL".to_string(), - }); - } - if value.len() > MAX_SECRET_VALUE_BYTES { - return Err(Error::InvalidInput { - message: format!( - "a Secret value is at most {MAX_SECRET_VALUE_BYTES} bytes, not {}", - value.len() - ), - }); - } - Ok(()) -} - /// Stable request envelope for remote immutable Function registration. /// /// Credential values deliberately have no field here. The only secret-shaped @@ -561,49 +511,6 @@ pub struct FunctionRegistrationRequest { pub secret_env_bindings: BTreeMap, } -impl FunctionRegistrationRequest { - /// Reject a registration whose bindings exceed what a launch can deliver. - /// - /// Shape only, and deliberately not a check that each bound Secret exists: - /// that is the service's answer, and it is asked for the first time when a - /// column is declared against the registered version. - pub fn validate(&self) -> Result<()> { - if self.secret_env_bindings.len() > MAX_FUNCTION_SECRET_ENV_BINDINGS { - return Err(Error::InvalidInput { - message: format!( - "a Function binds at most {MAX_FUNCTION_SECRET_ENV_BINDINGS} secrets, not {}", - self.secret_env_bindings.len() - ), - }); - } - for variable in self.secret_env_bindings.keys() { - if !is_portable_env_name(variable) { - return Err(Error::InvalidInput { - message: format!( - "secret_env_bindings key '{variable}' is not a portable \ - environment variable name" - ), - }); - } - // `env` travels with the Function and is readable wherever its - // record is; a bound Secret is not. One name carrying both would - // resolve by delivery order, so refuse rather than pick. - if self - .runtime - .env() - .is_some_and(|env| env.contains_key(variable)) - { - return Err(Error::InvalidInput { - message: format!( - "secret_env_bindings key '{variable}' is already set by runtime.env" - ), - }); - } - } - Ok(()) - } -} - impl_json!(FunctionRegistrationRequest); /// Exact FunctionVersion reference embedded in applications and bindings. diff --git a/rust/lancedb/src/lib.rs b/rust/lancedb/src/lib.rs index 44c5dd616..132b24abd 100644 --- a/rust/lancedb/src/lib.rs +++ b/rust/lancedb/src/lib.rs @@ -195,6 +195,7 @@ pub mod query; #[cfg(feature = "remote")] pub mod remote; pub mod rerankers; +pub mod secrets; pub mod sql; pub mod table; #[cfg(test)] diff --git a/rust/lancedb/src/remote/db.rs b/rust/lancedb/src/remote/db.rs index b0d489c73..3a04513a5 100644 --- a/rust/lancedb/src/remote/db.rs +++ b/rust/lancedb/src/remote/db.rs @@ -21,13 +21,14 @@ use lance_namespace::models::{ use crate::Error; use crate::database::{ CloneTableRequest, CreateTableMode, CreateTableRequest, Database, DatabaseOptions, JobInfo, - OpenTableRequest, ReadConsistency, SecretInfo, TableNamesRequest, + OpenTableRequest, ReadConsistency, TableNamesRequest, }; use crate::error::Result; use crate::function::{FunctionRegistrationRequest, FunctionVersion}; use crate::job::Job; use crate::remote::job::{RemoteJob, job_state_to_client}; use crate::remote::util::stream_as_body; +use crate::secrets::SecretInfo; use crate::table::BaseTable; use super::client::{ diff --git a/rust/lancedb/src/secrets.rs b/rust/lancedb/src/secrets.rs new file mode 100644 index 000000000..904260aee --- /dev/null +++ b/rust/lancedb/src/secrets.rs @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The LanceDB Authors + +//! Named Secrets: database-scoped credentials a Function binds by name. +//! +//! Nothing here holds a credential. The verbs live on +//! [`crate::connection::Connection`], and none of them returns a value -- by +//! construction rather than by policy, so there is no code path that could. +//! What a Function records is a binding, in [`crate::function`]. + +/// What a database records about a Secret. Never its value. +/// +/// Returned by [`crate::connection::Connection::describe_secret`]. There is no +/// field for the credential and no method that could produce one. +#[derive(Debug, Clone, PartialEq, Eq, serde::Deserialize)] +pub struct SecretInfo { + /// The Secret's database-scoped name. + pub name: String, + /// When the Secret was created, as an RFC 3339 timestamp. + pub created_at: String, + /// When the Secret's value was last rotated, as an RFC 3339 timestamp. + pub updated_at: String, +} diff --git a/rust/lancedb/tests/first_class_function_slice2.rs b/rust/lancedb/tests/first_class_function_slice2.rs index 1e7ba1611..89d6a9a98 100644 --- a/rust/lancedb/tests/first_class_function_slice2.rs +++ b/rust/lancedb/tests/first_class_function_slice2.rs @@ -5,9 +5,7 @@ use std::fs; use std::path::PathBuf; use lancedb::Error; -use lancedb::function::{ - FunctionRegistrationRequest, MAX_FUNCTION_SECRET_ENV_BINDINGS, MAX_SECRET_VALUE_BYTES, -}; +use lancedb::function::FunctionRegistrationRequest; use serde_json::Value; fn fixture(name: &str) -> String { @@ -115,105 +113,3 @@ async fn local_function_catalog_operations_return_stable_not_supported() { )); } } - -/// The cap is enforced above the backend, so every database and every language -/// surface rejects the same envelope. A local connection would otherwise answer -/// `NotSupported` first, which is what makes it the honest probe here. -#[tokio::test] -async fn a_function_binds_at_most_sixteen_secrets() { - let directory = tempfile::tempdir().unwrap(); - let connection = lancedb::connect(directory.path().to_str().unwrap()) - .execute() - .await - .unwrap(); - let mut request = FunctionRegistrationRequest::from_json(&fixture( - "remote_function_registration_request.json", - )) - .unwrap(); - request.secret_env_bindings = (0..=MAX_FUNCTION_SECRET_ENV_BINDINGS) - .map(|index| (format!("TOKEN_{index}"), format!("secret-{index}"))) - .collect(); - - let error = connection.create_function_async(request).await.unwrap_err(); - assert!(matches!( - error, - Error::InvalidInput { message } if message.contains("at most 16 secrets") - )); -} - -/// The binding contract is enforced above the backend in full, not just its -/// count: a caller that skips a language binding still cannot register a name -/// the runtime could not deliver. -#[tokio::test] -async fn binding_names_are_validated_before_dispatch() { - let directory = tempfile::tempdir().unwrap(); - let connection = lancedb::connect(directory.path().to_str().unwrap()) - .execute() - .await - .unwrap(); - - let mut invalid_name = FunctionRegistrationRequest::from_json(&fixture( - "remote_function_registration_request.json", - )) - .unwrap(); - invalid_name.secret_env_bindings = [("BAD=NAME".to_string(), "openai-prod".to_string())].into(); - let error = connection - .create_function_async(invalid_name) - .await - .unwrap_err(); - assert!(matches!( - error, - Error::InvalidInput { message } if message.contains("portable") - )); - - // `env` is readable wherever the Function's record is; a bound Secret is - // not. The same name cannot mean both. - let mut overlapping = FunctionRegistrationRequest::from_json(&fixture( - "remote_function_registration_request.json", - )) - .unwrap(); - let bound = overlapping - .runtime - .env() - .and_then(|env| env.keys().next().cloned()) - .expect("fixture runtime declares env"); - overlapping.secret_env_bindings = [(bound.clone(), "openai-prod".to_string())].into(); - let error = connection - .create_function_async(overlapping) - .await - .unwrap_err(); - assert!(matches!( - error, - Error::InvalidInput { message } if message.contains("already set by runtime.env") - )); -} - -/// An oversized credential is refused before a body is built, so it is never -/// serialized or uploaded to be refused by the service instead. -#[tokio::test] -async fn an_oversized_secret_value_is_refused_before_the_wire() { - let directory = tempfile::tempdir().unwrap(); - let connection = lancedb::connect(directory.path().to_str().unwrap()) - .execute() - .await - .unwrap(); - - for value in ["", &"x".repeat(MAX_SECRET_VALUE_BYTES + 1)] { - let error = connection - .create_secret("openai-prod", value) - .await - .unwrap_err(); - assert!( - matches!(error, Error::InvalidInput { .. }), - "expected InvalidInput, got {error:?}" - ); - } - - // A local database refuses the verb outright, which is what proves the - // size check ran ahead of the backend rather than instead of it. - let error = connection - .create_secret("openai-prod", "x".repeat(MAX_SECRET_VALUE_BYTES)) - .await - .unwrap_err(); - assert!(matches!(error, Error::NotSupported { .. })); -}