From 9b7629826f45af70ed2668353ff4d28446cd1417 Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Thu, 12 Oct 2023 22:26:53 +0200 Subject: refactor(tvix/castore): factor out node checks Implement `validate()` on `node::Node`, and call it from PathInfo's validate() too. Node-related errors are moved to a ValidateNodeError error type. This additionally adds some more validations for symlink targets (they must not be empty, and not contain null bytes). Change-Id: Ib9b89f1c9c795e868a1533281239bc8a36d97c5d Reviewed-on: https://cl.tvl.fyi/c/depot/+/9715 Autosubmit: flokli Tested-by: BuildkiteCI Reviewed-by: edef --- tvix/store/src/proto/mod.rs | 38 +++++++--------------------------- tvix/store/src/proto/tests/pathinfo.rs | 31 +++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 34 deletions(-) (limited to 'tvix/store') diff --git a/tvix/store/src/proto/mod.rs b/tvix/store/src/proto/mod.rs index 033fb79dbbef..7921adc4062e 100644 --- a/tvix/store/src/proto/mod.rs +++ b/tvix/store/src/proto/mod.rs @@ -3,10 +3,7 @@ use data_encoding::BASE64; // https://github.com/hyperium/tonic/issues/1056 use nix_compat::store_path::{self, StorePath}; use thiserror::Error; -use tvix_castore::{ - proto::{self as castorepb, NamedNode}, - B3Digest, B3_LEN, -}; +use tvix_castore::proto::{self as castorepb, NamedNode, ValidateNodeError}; mod grpc_pathinfoservice_wrapper; @@ -34,14 +31,14 @@ pub enum ValidatePathInfoError { #[error("No node present")] NoNodePresent(), - /// Invalid node name encountered. + /// Node fails validation + #[error("Invalid root node: {:?}", .0.to_string())] + InvalidRootNode(ValidateNodeError), + + /// Invalid node name encountered. Root nodes in PathInfos have more strict name requirements #[error("Failed to parse {0:?} as StorePath: {1}")] InvalidNodeName(Vec, store_path::Error), - /// The digest the (root) node refers to has invalid length. - #[error("Invalid Digest length: expected {}, got {}", B3_LEN, .0)] - InvalidNodeDigestLen(usize), - /// The digest in narinfo.nar_sha256 has an invalid len. #[error("Invalid narinfo.nar_sha256 length: expected {}, got {}", 32, .0)] InvalidNarSha256DigestLen(usize), @@ -146,27 +143,8 @@ impl PathInfo { Err(ValidatePathInfoError::NoNodePresent())? } Some(castorepb::Node { node: Some(node) }) => { - match node { - // for a directory root node, ensure the digest has the appropriate size. - castorepb::node::Node::Directory(directory_node) => { - if TryInto::::try_into(directory_node.digest.clone()).is_err() { - return Err(ValidatePathInfoError::InvalidNodeDigestLen( - directory_node.digest.len(), - )); - } - } - // for a file root node, ensure the digest has the appropriate size. - castorepb::node::Node::File(file_node) => { - // ensure the digest has the appropriate size. - if TryInto::::try_into(file_node.digest.clone()).is_err() { - return Err(ValidatePathInfoError::InvalidNodeDigestLen( - file_node.digest.len(), - )); - } - } - // nothing to do specifically for symlinks - castorepb::node::Node::Symlink(_) => {} - } + node.validate() + .map_err(|e| ValidatePathInfoError::InvalidRootNode(e))?; // parse the name of the node itself and return parse_node_name_root(node.get_name(), ValidatePathInfoError::InvalidNodeName)? } diff --git a/tvix/store/src/proto/tests/pathinfo.rs b/tvix/store/src/proto/tests/pathinfo.rs index cfecbac3b82e..5e1ae9c45b64 100644 --- a/tvix/store/src/proto/tests/pathinfo.rs +++ b/tvix/store/src/proto/tests/pathinfo.rs @@ -43,7 +43,7 @@ fn validate_no_node( digest: Bytes::new(), size: 0, }, - Err(ValidatePathInfoError::InvalidNodeDigestLen(0)); + Err(ValidatePathInfoError::InvalidRootNode(castorepb::ValidateNodeError::InvalidDigestLen(0))); "invalid digest length" )] #[test_case( @@ -88,7 +88,7 @@ fn validate_directory( digest: Bytes::new(), ..Default::default() }, - Err(ValidatePathInfoError::InvalidNodeDigestLen(0)); + Err(ValidatePathInfoError::InvalidRootNode(castorepb::ValidateNodeError::InvalidDigestLen(0))); "invalid digest length" )] #[test_case( @@ -120,7 +120,7 @@ fn validate_file( #[test_case( castorepb::SymlinkNode { name: DUMMY_NAME.into(), - ..Default::default() + target: "foo".into(), }, Ok(StorePath::from_str(DUMMY_NAME).expect("must succeed")); "ok" @@ -128,7 +128,7 @@ fn validate_file( #[test_case( castorepb::SymlinkNode { name: "invalid".into(), - ..Default::default() + target: "foo".into(), }, Err(ValidatePathInfoError::InvalidNodeName( "invalid".into(), @@ -239,3 +239,26 @@ fn validate_inconsistent_narinfo_reference_name_digest() { e => panic!("unexpected error: {:?}", e), } } + +/// Create a node with an empty symlink target, and ensure it fails validation. +#[test] +fn validate_symlink_empty_target_invalid() { + let node = castorepb::node::Node::Symlink(castorepb::SymlinkNode { + name: "foo".into(), + target: "".into(), + }); + + node.validate().expect_err("must fail validation"); +} + +/// Create a node with a symlink target including null bytes, and ensure it +/// fails validation. +#[test] +fn validate_symlink_target_null_byte_invalid() { + let node = castorepb::node::Node::Symlink(castorepb::SymlinkNode { + name: "foo".into(), + target: "foo\0".into(), + }); + + node.validate().expect_err("must fail validation"); +} -- cgit 1.4.1