alexandrefimov commented on code in PR #7594: URL: https://github.com/apache/kyuubi/pull/7594#discussion_r3782414628
########## extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/gen/KnownHarmlessNodes.scala: ########## @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.kyuubi.plugin.spark.authz.gen + +import org.apache.kyuubi.plugin.spark.authz.serde.HarmlessNodeSpec + +/** + * The explicit allowlist of plan nodes that are not authorization-relevant, backing + * `known_harmless_spec.json`. Every entry must carry a reason (an allowlist entry is a + * reviewed security decision) and names the exact Spark minor versions the review was + * performed against. On any other Spark version the entry is inert and the node counts as + * unclassified: a class is free to change shape under the same fully qualified name in + * the next release (exactly what CALL did between Spark 3 and 4), so re-reviewing this + * list, entry by entry, is part of every Spark version bump. + */ +object KnownHarmlessNodes { + + // The Spark minors the plugin currently supports and tests per-profile. Every entry + // below was reviewed against the 3.x baseline when the allowlist was introduced and + // re-reviewed (and the exercised ones re-run in deny mode, full module suite per + // profile) for the 4.x port. A version joins an entry's list by being tested or + // reviewed, never by interpolation. + private val spark3xAnd4x = Seq("3.3", "3.4", "3.5", "4.0", "4.1", "4.2") + + // Nodes that only exist on Spark 4. + private val spark4x = Seq("4.0", "4.1", "4.2") + + val specs: Seq[HarmlessNodeSpec] = Seq( + HarmlessNodeSpec( + "org.apache.kyuubi.plugin.spark.authz.rule.rowfilter.FilteredShowColumnsCommand", + "This plugin's own row-filtering replacement for ShowColumnsCommand (installed by" + + " RuleReplaceShowObjectCommands); PrivilegesBuilder.build handles it with a" + + " dedicated dispatch arm and every result row is checked for SHOWCOLUMNS access", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.kyuubi.plugin.spark.authz.rule.rowfilter.FilteredShowFunctionsCommand", + "This plugin's own row-filtering replacement for ShowFunctionsCommand (installed by" + + " RuleReplaceShowObjectCommands); PrivilegesBuilder.build handles it with a" + + " dedicated dispatch arm and every result row is checked for SHOWFUNCTIONS access", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.kyuubi.plugin.spark.authz.rule.rowfilter.FilteredShowTablesCommand", + "This plugin's own row-filtering replacement for ShowTablesCommand (installed by" + + " RuleReplaceShowObjectCommands); PrivilegesBuilder.build handles it with a" + + " dedicated dispatch arm and every result row is checked for SHOWTABLES access", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.catalyst.analysis.ResolvedNamespace", + "Analysis-time resolution artifact naming a namespace; reads no data itself, and the" + + " commands resolved over it are classified in their own right", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.catalyst.plans.logical.CTERelationRef", + "Leaf reference to a CTE definition; the definition's own plan appears under" + + " WithCTE in the same tree and is authorized there", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.catalyst.plans.logical.CommandResult", + "Holds rows already produced by an eagerly executed command; that command was" + + " authorized when it executed", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.catalyst.plans.logical.LocalRelation", + "Holds in-memory literal rows (VALUES lists, createDataFrame); reads no stored data", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.catalyst.plans.logical.NoopCommand", + "Spark's placeholder for commands with nothing to do (e.g. IF EXISTS / IF NOT EXISTS" + + " variants when the object is absent); executes nothing", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.catalyst.plans.logical.OneRowRelation", + "The implicit single-row relation backing SELECT without FROM; reads no stored data", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.catalyst.plans.logical.Range", + "Generates rows from a numeric range (e.g. spark.range); reads no stored data", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.catalyst.plans.logical.ShowNamespaces", + "Enforced elsewhere: results are row-filtered per namespace by ObjectFilterPlaceHolder" + + " + FilterDataSourceV2Strategy; Spark eagerly executes the bare command in a nested" + + " QueryExecution whose unfiltered result the placeholder discards", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.execution.command.ShowNamespacesCommand", + "Enforced elsewhere: Spark 4.x's v1 SHOW DATABASES/NAMESPACES command; results are" + + " row-filtered per namespace by ObjectFilterPlaceHolder + FilterDataSourceV2Strategy" + + " exactly like v2 ShowNamespaces, and the bare command Spark eagerly executes in a" + + " nested QueryExecution has its unfiltered result discarded by the placeholder", + spark4x), + HarmlessNodeSpec( + "org.apache.spark.sql.catalyst.plans.logical.ShowTables", + "Enforced elsewhere: results are row-filtered per table by ObjectFilterPlaceHolder" + + " + FilterDataSourceV2Strategy; Spark eagerly executes the bare command in a nested" + + " QueryExecution whose unfiltered result the placeholder discards", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.execution.ExternalRDD", + "Wraps a session-created RDD/local collection (e.g. spark.createDataset, and Delta's" + + " internal VACUUM plumbing); RDD-level access is outside the plugin's scope and is" + + " an existing, separate concern", + spark3xAnd4x), + HarmlessNodeSpec( + "org.apache.spark.sql.execution.LogicalRDD", + "Wraps a pre-existing RDD; RDD-level access is outside the plugin's scope and is an" + + " existing, separate concern", + spark3xAnd4x), + HarmlessNodeSpec( Review Comment: I do not think `InMemoryRelation` is harmless across users. Spark sessions can share the cache, while authorization must be evaluated for the current user. In that case the optimized plan is an `InMemoryRelation` leaf and no table privilege is produced. The existing cache test confirms that another session gets `InMemoryRelation`, but both sessions run inside the same `doAs(admin, ...)` block and the test only checks the authz tag. Could we add a regression test where an admin populates the cache and an unprivileged user reads the same table from a new session? The read should still be denied. This probably requires authorizing the original relation instead of allowlisting `InMemoryRelation`. ########## extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ParanoidMode.scala: ########## @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.kyuubi.plugin.spark.authz + +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.LongAdder + +import org.apache.spark.sql.SparkSession +import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan +import org.slf4j.LoggerFactory + +/** + * Handling for plan nodes that fall outside the plugin's recognition machinery + * ("paranoid mode"). + * + * The privilege builder classifies plan nodes by pattern-matching against the command, + * scan and function spec files. Historically a node that fell through every match was + * silently treated as not-authz-relevant, i.e. the plugin failed open. This object + * centralizes the policy applied when such a node is encountered: + * + * - `allow`: legacy behavior, log at DEBUG only + * - `warn`: log at WARN once per (class name, violation kind) per JVM (default) + * - `deny`: throw [[AccessControlException]], failing the query closed + * + * configured via `spark.kyuubi.authz.unclassifiedNode.behavior`. + * + * Nodes that are genuinely not authz-relevant are declared in + * `known_harmless_spec.json`, each with a human-reviewed reason. + */ +object ParanoidMode { + + final private val LOG = LoggerFactory.getLogger(getClass) + + final val UNCLASSIFIED_NODE_BEHAVIOR_KEY = "spark.kyuubi.authz.unclassifiedNode.behavior" + + object Behavior extends Enumeration { + type Behavior = Value + val ALLOW: Value = Value("allow") + val WARN: Value = Value("warn") + val DENY: Value = Value("deny") + } + + /** The kinds of classification violations, used in log/error messages and dedup keys. */ + object ViolationKind extends Enumeration { + type ViolationKind = Value + + /** A `Command` with no entry in any command spec file (fail-open layer 1). */ + val UNCLASSIFIED_COMMAND: Value = Value("unclassified command") + + /** A leaf plan node not matched by any scan spec (fail-open layer 2). */ + val UNCLASSIFIED_LEAF: Value = Value("unclassified leaf node") + + /** A node matched by a scan spec but not resolved, so no privileges were extracted. */ + val UNRESOLVED_SCAN: Value = Value("unresolved scan node") + + /** + * A node whose class name has a command spec entry, encountered on the query path: + * the spec exists but the dispatch never reaches it (the CALL-on-Spark-4 shape). + */ + val UNREACHABLE_SPEC: Value = Value("command spec not reachable from dispatch") + + /** + * A node that executes during analysis (e.g. Spark 4's `ExecutableDuringAnalysis`): + * by the time authorization rules run it may already have produced side effects. + */ + val ANALYSIS_TIME_EXECUTION: Value = Value("node executes during analysis") + + /** + * A matched spec whose extractors all failed against the current plan shape + * (fail-open layer 3, typically Spark version drift). + */ + val EXTRACTION_FAILURE: Value = Value("spec matched but extraction failed") + } + + import Behavior._ + import ViolationKind.ViolationKind + + def behavior(spark: SparkSession): Behavior = { + val raw = spark.conf.getOption(UNCLASSIFIED_NODE_BEHAVIOR_KEY).getOrElse(WARN.toString) Review Comment: This makes `deny` controllable by the client because the value is read from the session-level `spark.conf`. A user can set `spark.kyuubi.authz.unclassifiedNode.behavior=allow` before running the query, so an operator cannot use `deny` as an authorization boundary. Could we read this setting only from the application/engine configuration, for example `SparkContext.getConf`, and add negative tests for attempts to change it through SQL `SET` and the Spark Connect config API? -- 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]
