access control by ErmiasG · Pull Request #3 · logicalclocks/trino

ErmiasG · 2026-02-24T04:11:59Z

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

ErmiasG · 2026-02-24T04:18:16Z

Jenkins pipeline https://jenkins.hops.works/view/Kubernetes%20imgs/job/k8s-trino-image/

Copilot

Pull request overview

This PR updates file-based access control rule matching to support pattern replacement via capturing groups (through a new ReplacePatternMatcher helper), and adds Jenkins/Docker build artifacts for building/pushing a Trino image.

Changes:

Refactor multiple access-control rule matchers to delegate to the new ReplacePatternMatcher.
Introduce ReplacePatternMatcher to perform replacement-based matching for catalog/schema/table/query-owner patterns.
Add a root Jenkinsfile and build-manifest.json to build artifacts and build/push a Docker image.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/TableAccessControlRule.java	Switch table rule matching to `ReplacePatternMatcher`
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/SchemaAccessControlRule.java	Switch schema rule matching to `ReplacePatternMatcher`
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/CatalogAccessControlRule.java	Switch catalog rule matching to `ReplacePatternMatcher`
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/QueryAccessRule.java	Switch query rule matching to `ReplacePatternMatcher`
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AnySchemaPermissionsRule.java	Switch “any schema permissions” matching to `ReplacePatternMatcher`
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AnyCatalogSchemaPermissionsRule.java	Switch “any catalog+schema permissions” matching to `ReplacePatternMatcher`
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ReplacePatternMatcher.java	New helper implementing replacement-based matching logic
build-manifest.json	New manifest for Docker image build configuration
Jenkinsfile	New Jenkins pipeline to build artifacts and build/push Docker image

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-24T10:05:30Z

+//        if (userRegex.map(regex -> regex.matcher(user).matches()).orElse(true) &&
+//                roleRegex.map(regex -> roles.stream().anyMatch(role -> regex.matcher(role).matches())).orElse(true) &&
+//                groupRegex.map(regex -> groups.stream().anyMatch(group -> regex.matcher(group).matches())).orElse(true) &&
+//                schemaRegex.map(regex -> regex.matcher(schema).matches()).orElse(true)) {
+//            return Optional.of(owner);
+//        }
+


The old match condition is kept as commented-out code. Please remove the commented block to keep the rule implementation clear and avoid style/checkstyle issues.

Suggested change

// if (userRegex.map(regex -> regex.matcher(user).matches()).orElse(true) &&

// roleRegex.map(regex -> roles.stream().anyMatch(role -> regex.matcher(role).matches())).orElse(true) &&

// groupRegex.map(regex -> groups.stream().anyMatch(group -> regex.matcher(group).matches())).orElse(true) &&

// schemaRegex.map(regex -> regex.matcher(schema).matches()).orElse(true)) {

// return Optional.of(owner);

// }

Copilot · 2026-02-24T10:05:31Z

+//        if (userRegex.map(regex -> regex.matcher(user).matches()).orElse(true) &&
+//                roleRegex.map(regex -> roles.stream().anyMatch(role -> regex.matcher(role).matches())).orElse(true) &&
+//                groupRegex.map(regex -> groups.stream().anyMatch(group -> regex.matcher(group).matches())).orElse(true) &&
+//                catalogRegex.map(regex -> regex.matcher(catalog).matches()).orElse(true)) {
+//            return Optional.of(accessMode);
+//        }
+


The old match condition is left as commented-out code. Since ReplacePatternMatcher is now the implementation, the commented block should be deleted to avoid clutter and potential style violations.

Suggested change

// if (userRegex.map(regex -> regex.matcher(user).matches()).orElse(true) &&

// roleRegex.map(regex -> roles.stream().anyMatch(role -> regex.matcher(role).matches())).orElse(true) &&

// groupRegex.map(regex -> groups.stream().anyMatch(group -> regex.matcher(group).matches())).orElse(true) &&

// catalogRegex.map(regex -> regex.matcher(catalog).matches()).orElse(true)) {

// return Optional.of(accessMode);

// }

Copilot · 2026-02-24T10:05:31Z

+//        return userRegex.map(regex -> regex.matcher(user).matches()).orElse(true) &&
+//                roleRegex.map(regex -> roles.stream().anyMatch(role -> regex.matcher(role).matches())).orElse(true) &&
+//                groupRegex.map(regex -> groups.stream().anyMatch(group -> regex.matcher(group).matches())).orElse(true) &&
+//                schemaRegex.map(regex -> regex.matcher(schemaName).matches()).orElse(true);
+


Commented-out legacy matching logic should be removed. Keeping large commented blocks makes future changes harder and can trip style tooling.

Suggested change

// return userRegex.map(regex -> regex.matcher(user).matches()).orElse(true) &&

// roleRegex.map(regex -> roles.stream().anyMatch(role -> regex.matcher(role).matches())).orElse(true) &&

// groupRegex.map(regex -> groups.stream().anyMatch(group -> regex.matcher(group).matches())).orElse(true) &&

// schemaRegex.map(regex -> regex.matcher(schemaName).matches()).orElse(true);

Copilot · 2026-02-24T10:05:31Z

+    private boolean matchCatalogInternal(final Optional<Pattern> catalogRegex, String catalog)
+    {
+        validateMatchers(userMatcher, roleMatcher, groupMatcher);
+
+        Optional<Pattern> userReplacedCatalogPattern = replaceRegex(userMatcher, catalogRegex, "catalog", "user");


validateMatchers(...) is called unconditionally (e.g., from matchCatalogInternal/matchSchemaInternal/matchTableInternal). This can now throw CONFIGURATION_INVALID for configurations that have multiple capturing groups in user/role/group patterns even when the target pattern (catalog/schema/table/queryOwner) does not contain any $N replacement tokens, which is a behavioral change from the previous implementation. Consider only running this validation when the pattern being matched actually contains replacement references (or when replacement is going to be applied).

Copilot · 2026-02-24T10:05:32Z

+    public boolean matchCatalog(final Optional<Pattern> catalogRegex, String catalog)
+    {
+        return match() && matchCatalogInternal(catalogRegex, catalog);
+    }


This introduces non-trivial new matching behavior (replacement via capturing groups + new CONFIGURATION_INVALID failure modes), but there are no accompanying tests. Please add tests that cover replacement for catalog/schema/table/queryOwner rules and the multiple-capturing-group validation behavior.

Copilot · 2026-02-24T10:05:33Z

+        if (patternToReplace.isPresent() && patternToReplace.get().pattern().matches(".*\\$\\d+.*")) {
+            if (matcher.get().matches() && matcher.get().groupCount() > 0) {
+                StringBuilder stringBuilder = new StringBuilder();
+                try {
+                    matcher.get().appendReplacement(stringBuilder, patternToReplace.get().pattern());
+                }
+                catch (IndexOutOfBoundsException e) {
+                    throw new TrinoException(CONFIGURATION_INVALID,
+                            toReplace + " in replace pattern refers to a capturing group that does not exist in " + capturingGroup, e);
+                }
+                return Optional.of(Pattern.compile(stringBuilder.toString()));
+            }


replaceRegex() builds a new regex by injecting captured substrings from user/role/group directly into a Pattern. Since Trino user/role/group names can contain regex metacharacters (e.g., user ".*"), this can unintentionally broaden matches and create privilege-escalation paths when replacement is used. The captured value should be escaped/quoted before being embedded into the regex (or the behavior should be explicitly constrained/documented and validated).

Copilot · 2026-02-24T10:05:33Z

+
+          # Ensure the destination does not exist to avoid 'Directory not empty' errors
+          if [ -d "${env.WORK_DIR}/trino-server" ]; then
+            echo "Removing existing `trino-server` directory"


The echo message uses backticks around trino-server, which triggers shell command substitution. With set -euo pipefail, this will fail because trino-server is not a command. Use plain quotes or escape the backticks so the script doesn't attempt to execute anything.

Suggested change

echo "Removing existing `trino-server` directory"

echo "Removing existing 'trino-server' directory"

Copilot · 2026-02-24T10:05:33Z

+@Library("jenkins-library@main")
+
+import com.logicalclocks.jenkins.k8s.ImageBuilder
+
+node("local") {


The PR title/description focuses on access control, but this change set also adds a Jenkins pipeline + build-manifest for building/pushing Docker images. If this is intentional, the PR metadata should be updated (or the CI/docker changes split out) so reviewers can evaluate the two concerns independently.

Copilot · 2026-02-24T10:05:33Z

+//        return userRegex.map(regex -> regex.matcher(user).matches()).orElse(true) &&
+//                roleRegex.map(regex -> roles.stream().anyMatch(role -> regex.matcher(role).matches())).orElse(true) &&
+//                groupRegex.map(regex -> groups.stream().anyMatch(group -> regex.matcher(group).matches())).orElse(true) &&
+//                schemaRegex.map(regex -> regex.matcher(table.getSchemaName()).matches()).orElse(true) &&
+//                tableRegex.map(regex -> regex.matcher(table.getTableName()).matches()).orElse(true);
+


The previous matching implementation is left in place as commented-out code. This adds noise and may violate style checks; it should be removed now that the logic is replaced by ReplacePatternMatcher.

Suggested change

// return userRegex.map(regex -> regex.matcher(user).matches()).orElse(true) &&

// roleRegex.map(regex -> roles.stream().anyMatch(role -> regex.matcher(role).matches())).orElse(true) &&

// groupRegex.map(regex -> groups.stream().anyMatch(group -> regex.matcher(group).matches())).orElse(true) &&

// schemaRegex.map(regex -> regex.matcher(table.getSchemaName()).matches()).orElse(true) &&

// tableRegex.map(regex -> regex.matcher(table.getTableName()).matches()).orElse(true);

Copilot · 2026-02-24T10:05:34Z

+        sh "wget -O jdk24.tar.gz \"${env.JDK_DOWNLOAD_LINK}\""
+        sh "mkdir -p ${env.WORKSPACE}/jdk"
+        sh "tar -xzf jdk24.tar.gz -C ${env.WORKSPACE}/jdk --strip-components=1"
+        sh "rm jdk24.tar.gz"


This pipeline downloads a JDK tarball via wget from env.JDK_DOWNLOAD_LINK and immediately extracts and uses it in the build without any integrity verification. If the distribution URL or upstream server is compromised (or traffic is intercepted), an attacker could serve a malicious JDK archive and achieve arbitrary code execution on the Jenkins agent when tar and mvnw run against it. To mitigate this supply-chain risk, pin the JDK source to a trusted location and verify the download using a cryptographic checksum or signature (or rely on a pre-installed, vetted JDK on the agent) before extracting and using it.

access control

70c4a14

ErmiasG requested a review from smkniazi February 24, 2026 04:11

remove unused env vars

1143a6a

ErmiasG requested a review from Copilot February 24, 2026 09:57

Copilot started reviewing on behalf of ErmiasG February 24, 2026 09:57 View session

smkniazi approved these changes Feb 24, 2026

View reviewed changes

Copilot AI reviewed Feb 24, 2026

View reviewed changes

ErmiasG merged commit bc52e9e into logicalclocks:branch-477 Feb 24, 2026
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

access control#3

access control#3
ErmiasG merged 2 commits into
logicalclocks:branch-477from
ErmiasG:access-control

ErmiasG commented Feb 24, 2026

Uh oh!

ErmiasG commented Feb 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	echo "Removing existing `trino-server` directory"
	echo "Removing existing 'trino-server' directory"

Conversation

ErmiasG commented Feb 24, 2026

Description

Additional context and related issues

Release notes

Uh oh!

ErmiasG commented Feb 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants