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
15 changes: 10 additions & 5 deletions docs/security.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,16 @@ for more details.

The authentication would happen on https protocol only. Add the
`authentication:` section in the config file. The default authentication type is
set using `defaultType: "form"` Following types of the authentications are
supported.
set using `defaultTypes: ["form"]`. The first authentication type in `defaultTypes` is prioritized and then falls back to following ones.
Copy link

Choose a reason for hiding this comment

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

suggestion (typo): Consider revising 'falls back to following ones' to 'falls back to the following ones' for grammatical correctness.

Adding 'the' before 'following ones' improves clarity and grammatical accuracy.

Suggested change
set using `defaultTypes: ["form"]`. The first authentication type in `defaultTypes` is prioritized and then falls back to following ones.
set using `defaultTypes: ["form"]`. The first authentication type in `defaultTypes` is prioritized and then falls back to the following ones.

Following types of the authentications are supported.

### OAuth/OpenIDConnect

It can be configured as below

```yaml
authentication:
defaultType: "oauth"
defaultTypes: ["oauth"]
oauth:
issuer:
clientId:
Expand Down Expand Up @@ -81,6 +81,11 @@ Set the `privilegesField` to retrieve privileges from an OAuth claim.
```
- That also means you need to have a cluster with that routing group.
- It's ok to replicate an existing Trino cluster record with a different name for that purpose.
- If you want to have all users who are authenticated via SSO and are not in the `presetUsers` list be able to view the dashboard and query history, you can set `defaultPrivilege` in the config file:
```yaml
authorization:
defaultPrivilege: "USER"
```

### Form/Basic authentication

Expand All @@ -102,7 +107,7 @@ Also provide a random key pair in RSA format.

```yaml
authentication:
defaultType: "form"
defaultTypes: ["form"]
form:
selfSignKeyPair:
privateKeyRsa: <private_key_path>
Expand All @@ -115,7 +120,7 @@ LDAP requires both random key pair and config path for LDAP

```yaml
authentication:
defaultType: "form"
defaultTypes: ["form"]
form:
ldapConfigPath: <ldap_config_path>
selfSignKeyPair:
Expand Down
7 changes: 7 additions & 0 deletions gateway-ha/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,13 @@
<scope>runtime</scope>
</dependency>

<dependency>
<groupId>com.github.docker-java</groupId>
<artifactId>docker-java-api</artifactId>
<version>3.4.2</version>
<scope>test</scope>
</dependency>

<!-- Test deps -->
<dependency>
<groupId>com.h2database</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,29 +13,31 @@
*/
package io.trino.gateway.ha.config;

import java.util.List;

public class AuthenticationConfiguration
{
private String defaultType;
private List<String> defaultTypes;
private OAuthConfiguration oauth;
private FormAuthConfiguration form;

public AuthenticationConfiguration(String defaultType, OAuthConfiguration oauth, FormAuthConfiguration form)
public AuthenticationConfiguration(List<String> defaultTypes, OAuthConfiguration oauth, FormAuthConfiguration form)
{
this.defaultType = defaultType;
this.defaultTypes = defaultTypes;
this.oauth = oauth;
this.form = form;
}

public AuthenticationConfiguration() {}

public String getDefaultType()
public List<String> getDefaultTypes()
{
return this.defaultType;
return this.defaultTypes;
}

public void setDefaultType(String defaultType)
public void setDefaultTypes(List<String> defaultTypes)
{
this.defaultType = defaultType;
this.defaultTypes = defaultTypes;
}

public OAuthConfiguration getOauth()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@ public class AuthorizationConfiguration
private String user;
private String api;
private String ldapConfigPath;
private String defaultPrivilege;

public AuthorizationConfiguration(String admin, String user, String api, String ldapConfigPath)
public AuthorizationConfiguration(String admin, String user, String api, String ldapConfigPath, String defaultPrivilege)
{
this.admin = admin;
this.user = user;
this.api = api;
this.ldapConfigPath = ldapConfigPath;
this.defaultPrivilege = defaultPrivilege;
}

public AuthorizationConfiguration() {}
Expand Down Expand Up @@ -69,4 +71,14 @@ public void setLdapConfigPath(String ldapConfigPath)
{
this.ldapConfigPath = ldapConfigPath;
}

public String getDefaultPrivilege()
{
return this.defaultPrivilege;
}

public void setDefaultPrivilege(String defaultPrivilege)
{
this.defaultPrivilege = defaultPrivilege;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -145,28 +145,39 @@ private LbFormAuthManager getFormAuthManager(HaGatewayConfiguration configuratio
private ChainedAuthFilter getAuthenticationFilters(AuthenticationConfiguration config, Authorizer authorizer)
{
ImmutableList.Builder<ContainerRequestFilter> authFilters = ImmutableList.builder();
String defaultType = config.getDefaultType();
if (oauthManager != null) {
authFilters.add(new LbFilter(
new LbAuthenticator(oauthManager, authorizationManager),
authorizer,
"Bearer",
new LbUnauthorizedHandler(defaultType)));
List<String> authMethods = config.getDefaultTypes();
if (authMethods == null || authMethods.isEmpty()) {
return new ChainedAuthFilter(authFilters.build());
}

if (formAuthManager != null) {
authFilters.add(new LbFilter(
new FormAuthenticator(formAuthManager, authorizationManager),
authorizer,
"Bearer",
new LbUnauthorizedHandler(defaultType)));
for (String authMethod : authMethods) {
switch (authMethod) {
case "oauth" -> {
if (oauthManager != null) {
authFilters.add(new LbFilter(
new LbAuthenticator(oauthManager, authorizationManager),
authorizer,
"Bearer",
new LbUnauthorizedHandler("oauth")));
}
}
case "form" -> {
if (formAuthManager != null) {
authFilters.add(new LbFilter(
new FormAuthenticator(formAuthManager, authorizationManager),
authorizer,
"Bearer",
new LbUnauthorizedHandler("form")));

authFilters.add(new BasicAuthFilter(
new ApiAuthenticator(formAuthManager, authorizationManager),
authorizer,
new LbUnauthorizedHandler(defaultType)));
authFilters.add(new BasicAuthFilter(
new ApiAuthenticator(formAuthManager, authorizationManager),
authorizer,
new LbUnauthorizedHandler("form")));
}
}
default -> {}
Copy link

Choose a reason for hiding this comment

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

suggestion (bug_risk): Default case in switch silently ignores unknown auth methods.

Logging a warning or error in the default case would help identify misconfigurations involving unsupported authentication methods.

Suggested implementation:

        for (String authMethod : authMethods) {
            switch (authMethod) {
                case "oauth" -> {
                default -> {
                    logger.warn("Unknown authentication method configured: {}", authMethod);
                }

If the logger variable is not already defined in this file, you should add it at the top of the class:

private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(HaGatewayProviderModule.class);

Place this line with other field declarations in the class.

}
}

return new ChainedAuthFilter(authFilters.build());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.google.common.collect.ImmutableList;
import com.google.inject.Inject;
import io.airlift.log.Logger;
import io.trino.gateway.ha.config.HaGatewayConfiguration;
import io.trino.gateway.ha.domain.Result;
import io.trino.gateway.ha.domain.request.RestLoginRequest;
import io.trino.gateway.ha.security.LbFormAuthManager;
Expand Down Expand Up @@ -56,10 +57,12 @@ public class LoginResource

private final LbOAuthManager oauthManager;
private final LbFormAuthManager formAuthManager;
private final HaGatewayConfiguration haGatewayConfiguration;

@Inject
public LoginResource(@Nullable LbOAuthManager oauthManager, @Nullable LbFormAuthManager formAuthManager)
public LoginResource(HaGatewayConfiguration haGatewayConfiguration, @Nullable LbOAuthManager oauthManager, @Nullable LbFormAuthManager formAuthManager)
{
this.haGatewayConfiguration = haGatewayConfiguration;
this.oauthManager = oauthManager;
this.formAuthManager = formAuthManager;
}
Expand Down Expand Up @@ -173,16 +176,13 @@ else if (oauthManager != null) {
@Produces(MediaType.APPLICATION_JSON)
public Response loginType()
{
String loginType;
if (formAuthManager != null) {
loginType = "form";
}
else if (oauthManager != null) {
loginType = "oauth";
List<String> loginTypes;
if (haGatewayConfiguration.getAuthentication() != null) {
loginTypes = haGatewayConfiguration.getAuthentication().getDefaultTypes();
}
else {
loginType = "none";
loginTypes = List.of("none");
}
return Response.ok(Result.ok("Ok", loginType)).build();
return Response.ok(Result.ok("Ok", loginTypes)).build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ public class AuthorizationManager
{
private final Map<String, UserConfiguration> presetUsers;
private final LbLdapClient lbLdapClient;
private final AuthorizationConfiguration authorizationConfiguration;

public AuthorizationManager(AuthorizationConfiguration configuration,
Map<String, UserConfiguration> presetUsers)
{
this.presetUsers = presetUsers;
this.authorizationConfiguration = configuration;
if (configuration != null && configuration.getLdapConfigPath() != null) {
lbLdapClient = new LbLdapClient(LdapConfiguration.load(configuration.getLdapConfigPath()));
}
Expand All @@ -49,6 +51,13 @@ public Optional<String> getPrivileges(String username)
else if (lbLdapClient != null) {
privs = lbLdapClient.getMemberOf(username);
}
return Optional.ofNullable(privs);

if (privs == null || privs.trim().isEmpty()) {
if (authorizationConfiguration != null && authorizationConfiguration.getDefaultPrivilege() != null) {
Comment on lines +55 to +56
Copy link

Choose a reason for hiding this comment

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

🚨 suggestion (security): Default privilege fallback may mask misconfigurations.

Log fallback events to alert administrators to missing privilege assignments.

Suggested implementation:

        this.presetUsers = presetUsers;
        this.authorizationConfiguration = configuration;
        if (configuration != null && configuration.getLdapConfigPath() != null) {
            lbLdapClient = new LbLdapClient(LdapConfiguration.load(configuration.getLdapConfigPath()));
        }
        else if (lbLdapClient != null) {
            privs = lbLdapClient.getMemberOf(username);
        }

        // Add logger for fallback events
        private static final org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger(AuthorizationManager.class);
        if (privs == null || privs.trim().isEmpty()) {
            if (authorizationConfiguration != null && authorizationConfiguration.getDefaultPrivilege() != null) {
                log.warn("Privilege for user '{}' is missing or empty. Falling back to default privilege: '{}'. Please check privilege assignments.", username, authorizationConfiguration.getDefaultPrivilege());
                return Optional.of(authorizationConfiguration.getDefaultPrivilege());
            }
            return Optional.empty(); // No default privilege if not configured

If the logger is already defined elsewhere in the class, do not redefine it. Place the logger definition at the top of the class if needed.

return Optional.of(authorizationConfiguration.getDefaultPrivilege());
}
return Optional.empty(); // No default privilege if not configured
}
return Optional.of(privs);
}
}
Loading