bvolpato commented on code in PR #23172:
URL: https://github.com/apache/datafusion/pull/23172#discussion_r3576228252
##########
datafusion/substrait/src/physical_plan/consumer.rs:
##########
@@ -37,19 +38,55 @@ use async_recursion::async_recursion;
use chrono::DateTime;
use datafusion::datasource::memory::DataSourceExec;
use object_store::ObjectMeta;
+use object_store::path::Path as ObjectStorePath;
use substrait::proto::Type;
use substrait::proto::read_rel::local_files::file_or_files::PathType;
use substrait::proto::r#type::{Kind, Nullability};
use substrait::proto::{
Rel, expression::MaskExpression, read_rel::ReadType, rel::RelType,
};
+use url::Url;
+
+/// Options controlling Substrait physical plan import.
+#[derive(Debug, Clone, Default)]
+pub struct PhysicalPlanConsumerOptions {
+ allowed_local_file_roots: Vec<PathBuf>,
+}
+
+impl PhysicalPlanConsumerOptions {
+ /// Create import options that deny access to local files.
+ pub fn new() -> Self {
+ Self::default()
+ }
+
+ /// Allow imported local file paths under `root`.
+ ///
+ /// The root and imported file path are canonicalized before comparison, so
+ /// relative paths and symlinks cannot escape the configured root.
+ pub fn with_allowed_local_file_root(mut self, root: impl Into<PathBuf>) ->
Self {
+ self.allowed_local_file_roots.push(root.into());
+ self
+ }
+}
/// Convert Substrait Rel to DataFusion ExecutionPlan
#[async_recursion]
pub async fn from_substrait_rel(
+ ctx: &SessionContext,
+ rel: &Rel,
+ extensions: &HashMap<u32, &String>,
+) -> Result<Arc<dyn ExecutionPlan>> {
+ let options = PhysicalPlanConsumerOptions::default();
+ from_substrait_rel_with_options(ctx, rel, extensions, &options).await
Review Comment:
`from_substrait_rel` used to accept these plans, and now it denies them by
default. That makes sense for this fix, but it is still a public behavior
change. Can we add the `api change` label and a short 55.0.0 upgrade note
pointing callers to `from_substrait_rel_with_options`?
##########
datafusion/substrait/src/physical_plan/consumer.rs:
##########
@@ -164,6 +206,65 @@ pub async fn from_substrait_rel(
}
}
+fn local_file_path(
+ path: &str,
+ options: &PhysicalPlanConsumerOptions,
+) -> Result<ObjectStorePath> {
+ if options.allowed_local_file_roots.is_empty() {
+ return substrait_err!(
+ "Importing Substrait physical plans with local file paths requires
an allowed local file root"
+ );
+ }
+
+ let path = parse_local_file_path(path)?;
+ let path = canonicalize_path(&path)?;
+
+ let is_allowed = options
+ .allowed_local_file_roots
+ .iter()
+ .map(|root| canonicalize_path(root))
+ .collect::<Result<Vec<_>>>()?
+ .iter()
+ .any(|root| path.starts_with(root));
+
+ if !is_allowed {
+ return substrait_err!(
+ "Substrait physical plan local file path {} is outside the allowed
local file roots",
+ path.display()
+ );
+ }
+
+ ObjectStorePath::from_filesystem_path(path).map_err(|e| {
+ DataFusionError::Substrait(format!(
+ "Invalid local file path in Substrait physical plan: {e}"
+ ))
+ })
+}
+
+fn parse_local_file_path(path: &str) -> Result<PathBuf> {
+ match Url::parse(path) {
Review Comment:
One Windows edge case here: current producer emits bare object-store paths
(#20550). An absolute Windows path comes through as `C:/...`, and `Url::parse`
reads `C` as the scheme, so this rejects a plan produced by DataFusion itself.
Windows CI is disabled right now. Can we check native absolute paths before URL
parsing and cover this with a `#[cfg(windows)]` round-trip test?
##########
datafusion/substrait/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -78,6 +89,47 @@ async fn parquet_exec() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn local_files_require_allowed_root() -> Result<()> {
+ let rel = local_file_rel(testdata_object_path("data.parquet"))?;
+ let ctx = SessionContext::new();
+
+ let err = consumer::from_substrait_rel(&ctx, rel.as_ref(), &HashMap::new())
+ .await
+ .expect_err("local file imports should require an explicit allowed
root");
+
+ assert_contains(
+ &err,
+ "requires an allowed local file root",
+ "unexpected error for missing local file root",
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn local_files_must_stay_under_allowed_root() -> Result<()> {
Review Comment:
Can we add a symlink escape test here? The direct outside-root case proves
the prefix check, but not the canonicalization claim. A Unix-only test with a
symlink under the allowed root pointing outside would pin down the security
property.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
