Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/src/main/sphinx/security/opa-access-control.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ The following table lists the configuration properties for the OPA access contro
* - `opa.allow-permission-management-operations`
- Configure if permission management operations are allowed. Find more details in
[](opa-permission-management). Defaults to `false`.
* - `opa.include-user-principal`
- Whether to include the user's principal when sending authorization requests to OPA. Defaults to `false`.
* - `opa.http-client.*`
- Optional HTTP client configurations for the connection from Trino to OPA,
for example `opa.http-client.http-proxy` for configuring the HTTP proxy.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ public sealed class OpaAccessControl
private final LifeCycleManager lifeCycleManager;
private final OpaHighLevelClient opaHighLevelClient;
private final boolean allowPermissionManagementOperations;
private final boolean includeUserPrincipal;
private final OpaPluginContext pluginContext;

@Inject
Expand All @@ -98,6 +99,7 @@ public OpaAccessControl(LifeCycleManager lifeCycleManager, OpaHighLevelClient op
this.lifeCycleManager = requireNonNull(lifeCycleManager, "lifeCycleManager is null");
this.opaHighLevelClient = requireNonNull(opaHighLevelClient, "opaHighLevelClient is null");
this.allowPermissionManagementOperations = config.getAllowPermissionManagementOperations();
this.includeUserPrincipal = config.getIncludeUserPrincipal();
this.pluginContext = requireNonNull(pluginContext, "pluginContext is null");
}

Expand All @@ -124,7 +126,7 @@ public void checkCanExecuteQuery(Identity identity, QueryId queryId)
@Override
public void checkCanViewQueryOwnedBy(Identity identity, Identity queryOwner)
{
opaHighLevelClient.queryAndEnforce(buildQueryContext(identity), "ViewQueryOwnedBy", AccessDeniedException::denyViewQuery, OpaQueryInputResource.builder().user(new TrinoUser(queryOwner)).build());
opaHighLevelClient.queryAndEnforce(buildQueryContext(identity), "ViewQueryOwnedBy", AccessDeniedException::denyViewQuery, OpaQueryInputResource.builder().user(new TrinoUser(queryOwner, this.includeUserPrincipal)).build());
}

@Override
Expand All @@ -135,13 +137,13 @@ public Collection<Identity> filterViewQueryOwnedBy(Identity identity, Collection
queryOwner -> buildQueryInputForSimpleResource(
buildQueryContext(identity),
"FilterViewQueryOwnedBy",
OpaQueryInputResource.builder().user(new TrinoUser(queryOwner)).build()));
OpaQueryInputResource.builder().user(new TrinoUser(queryOwner, this.includeUserPrincipal)).build()));
}

@Override
public void checkCanKillQueryOwnedBy(Identity identity, Identity queryOwner)
{
opaHighLevelClient.queryAndEnforce(buildQueryContext(identity), "KillQueryOwnedBy", AccessDeniedException::denyKillQuery, OpaQueryInputResource.builder().user(new TrinoUser(queryOwner)).build());
opaHighLevelClient.queryAndEnforce(buildQueryContext(identity), "KillQueryOwnedBy", AccessDeniedException::denyKillQuery, OpaQueryInputResource.builder().user(new TrinoUser(queryOwner, this.includeUserPrincipal)).build());
}

@Override
Expand Down Expand Up @@ -802,11 +804,16 @@ private static Map<String, Optional<Object>> convertProperties(Map<String, Objec

OpaQueryContext buildQueryContext(Identity trinoIdentity)
{
return new OpaQueryContext(TrinoIdentity.fromTrinoIdentity(trinoIdentity), pluginContext, Optional.empty());
return new OpaQueryContext(TrinoIdentity.fromTrinoIdentity(trinoIdentity, this.includeUserPrincipal), pluginContext, Optional.empty());
}

OpaQueryContext buildQueryContext(SystemSecurityContext securityContext)
{
return new OpaQueryContext(TrinoIdentity.fromTrinoIdentity(securityContext.getIdentity()), pluginContext, Optional.of(securityContext.getQueryId()));
return new OpaQueryContext(TrinoIdentity.fromTrinoIdentity(securityContext.getIdentity(), this.includeUserPrincipal), pluginContext, Optional.of(securityContext.getQueryId()));
}

protected boolean allowPermissionManagementOperations()
{
return this.allowPermissionManagementOperations;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public Collection<Identity> filterViewQueryOwnedBy(Identity identity, Collection
"FilterViewQueryOwnedBy",
queryOwners,
queryOwner -> OpaQueryInputResource.builder()
.user(new TrinoUser(queryOwner))
.user(new TrinoUser(queryOwner, this.allowPermissionManagementOperations()))
.build());
}

Expand Down
14 changes: 14 additions & 0 deletions plugin/trino-opa/src/main/java/io/trino/plugin/opa/OpaConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public class OpaConfig
private boolean logRequests;
private boolean logResponses;
private boolean allowPermissionManagementOperations;
private boolean includeUserPrincipal;
private Optional<URI> opaRowFiltersUri = Optional.empty();
private Optional<URI> opaColumnMaskingUri = Optional.empty();
private Optional<URI> opaBatchColumnMaskingUri = Optional.empty();
Expand Down Expand Up @@ -99,6 +100,19 @@ public OpaConfig setAllowPermissionManagementOperations(boolean allowPermissionM
return this;
}

public boolean getIncludeUserPrincipal()
{
return this.includeUserPrincipal;
}

@Config("opa.include-user-principal")
@ConfigDescription("Whether to include the user's principal when sending authorization requests to OPA")
public OpaConfig setIncludeUserPrincipal(boolean includeUserPrincipal)
{
this.includeUserPrincipal = includeUserPrincipal;
return this;
}

@NotNull
public Optional<URI> getOpaRowFiltersUri()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,29 @@
import com.google.common.collect.ImmutableSet;
import io.trino.spi.security.Identity;

import java.security.Principal;
import java.util.Optional;
import java.util.Set;

import static java.util.Objects.requireNonNull;

public record TrinoIdentity(
String user,
Set<String> groups)
Set<String> groups,
Optional<Principal> principal)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked how these are sent and it seems the OpaHttpClient serializes everything to JSON. Are we sure that all principals can be serialized properly?

I don't have a really good answer myself for how to test this to be honest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we only have the java.security.Principal interface here. In Rust I would simply add a trait bound on Serialize and the compiler would ensure all passed types can be serialized :P
This is the main reason this is opt-in, so only users that really need access to the principal in OPA need to make sure it's serializable.

It would be great to find out what possible concrete types the Trino source code passes to authorizers.

{
public static TrinoIdentity fromTrinoIdentity(Identity identity)
public static TrinoIdentity fromTrinoIdentity(Identity identity, boolean includeUserPrincipal)
{
return new TrinoIdentity(
identity.getUser(),
identity.getGroups());
identity.getGroups(),
includeUserPrincipal ? identity.getPrincipal() : Optional.empty());
}

public TrinoIdentity
{
requireNonNull(user, "user is null");
groups = ImmutableSet.copyOf(requireNonNull(groups, "groups is null"));
requireNonNull(principal, "principal is null");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ public TrinoUser(String name)
this(name, null);
}

public TrinoUser(Identity identity)
public TrinoUser(Identity identity, boolean includeUserPrincipal)
{
this(null, TrinoIdentity.fromTrinoIdentity(identity));
this(null, TrinoIdentity.fromTrinoIdentity(identity, includeUserPrincipal));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Licensed 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 io.trino.plugin.opa;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;

import java.security.Principal;

// A Principal for tests that can be serialized using Jackson
@JsonSerialize
public class SerializableTestPrincipal
implements Principal
{
private String name;

public SerializableTestPrincipal(String name)
{
this.name = name;
}

@Override
@JsonProperty
public String getName()
{
return name;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import io.trino.spi.connector.CatalogSchemaRoutineName;
import io.trino.spi.connector.CatalogSchemaTableName;
import io.trino.spi.connector.ColumnSchema;
import io.trino.spi.security.BasicPrincipal;
import io.trino.spi.security.Identity;
import io.trino.spi.security.PrincipalType;
import io.trino.spi.security.SystemAccessControlFactory;
Expand Down Expand Up @@ -223,6 +224,7 @@ private void testIdentityResourceActions(
{
Identity dummyIdentity = Identity.forUser("dummy-user")
.withGroups(ImmutableSet.of("some-group"))
.withPrincipal(new BasicPrincipal("basic-principal"))
.build();
ThrowingMethodWrapper wrappedMethod = new ThrowingMethodWrapper(
accessControl -> callable.accept(accessControl, TEST_IDENTITY, dummyIdentity));
Expand Down Expand Up @@ -689,6 +691,39 @@ private void testRequestContextContentsForGivenTrinoVersion(Optional<SystemAcces
assertStringRequestsEqual(ImmutableSet.of(expectedRequest), mockClient.getRequests(), "/input");
}

@Test
void testIncludeUserPrincipal() {
InstrumentedHttpClient mockClient = createMockHttpClient(OPA_SERVER_URI, request -> OK_RESPONSE);
OpaAccessControl authorizer = (OpaAccessControl) OpaAccessControlFactory.create(
ImmutableMap.of("opa.policy.uri", OPA_SERVER_URI.toString(), "opa.include-user-principal", "true"),
Optional.of(mockClient),
Optional.empty());
Identity sampleIdentityWithGroupsAndPrincipal = Identity.forUser("test_user").withGroups(ImmutableSet.of("some_group")).withPrincipal(new SerializableTestPrincipal("test_principal")).build();
authorizer.checkCanExecuteQuery(sampleIdentityWithGroupsAndPrincipal, TEST_QUERY_ID);

String expectedRequest =
"""
{
"action": {
"operation": "ExecuteQuery"
},
"context": {
"identity": {
"user": "test_user",
"groups": ["some_group"],
"principal": {
"name": "test_principal"
}
},
"softwareStack": {
"trinoVersion": "UNKNOWN"
}
}
}\
""";
assertStringRequestsEqual(ImmutableSet.of(expectedRequest), mockClient.getRequests(), "/input");
}

@Test
void testGetRowFiltersThrowsForIllegalResponse()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ void testDefaults()
.setOpaBatchColumnMaskingUri(null)
.setLogRequests(false)
.setLogResponses(false)
.setAllowPermissionManagementOperations(false));
.setAllowPermissionManagementOperations(false)
.setIncludeUserPrincipal(false));
}

@Test
Expand All @@ -51,6 +52,7 @@ void testExplicitPropertyMappings()
.put("opa.log-requests", "true")
.put("opa.log-responses", "true")
.put("opa.allow-permission-management-operations", "true")
.put("opa.include-user-principal", "true")
.buildOrThrow();

OpaConfig expected = new OpaConfig()
Expand All @@ -61,7 +63,8 @@ void testExplicitPropertyMappings()
.setOpaBatchColumnMaskingUri(URI.create("https://opa-column-masking.example.com"))
.setLogRequests(true)
.setLogResponses(true)
.setAllowPermissionManagementOperations(true);
.setAllowPermissionManagementOperations(true)
.setIncludeUserPrincipal(true);

assertFullMapping(properties, expected);
}
Expand Down