Skip to content

Encode the user’s roles directly into the JWT #722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Encode the user’s roles directly into the JWT #722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 5 additions & 4 deletions gateway-ha/src/main/java/io/trino/gateway/ha/module/HaGatewayProviderModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,10 @@ public HaGatewayProviderModule(HaGatewayConfiguration configuration)
Map<String, UserConfiguration> presetUsers = configuration.getPresetUsers();

oauthManager = getOAuthManager(configuration);
formAuthManager = getFormAuthManager(configuration);
Copy link
Member

Choose a reason for hiding this comment

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

is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inside getFormAuthManager(configuration) i am using authorizationManager variable. that is why i want to initialize authorizationManager first and then call that method


authorizationManager = new AuthorizationManager(configuration.getAuthorization(), presetUsers);
formAuthManager = getFormAuthManager(configuration);

resourceSecurityDynamicFeature = getAuthFilter(configuration);
backendStateConnectionManager = new BackendStateManager();

Expand Down Expand Up @@ -137,7 +138,7 @@ private LbFormAuthManager getFormAuthManager(HaGatewayConfiguration configuratio
AuthenticationConfiguration authenticationConfiguration = configuration.getAuthentication();
if (authenticationConfiguration != null && authenticationConfiguration.getForm() != null) {
return new LbFormAuthManager(authenticationConfiguration.getForm(),
configuration.getPresetUsers(), configuration.getPagePermissions());
configuration.getPresetUsers(), configuration.getPagePermissions(), authorizationManager);
}
return null;
}
Expand All @@ -156,7 +157,7 @@ private ChainedAuthFilter getAuthenticationFilters(AuthenticationConfiguration c

if (formAuthManager != null) {
authFilters.add(new LbFilter(
new FormAuthenticator(formAuthManager, authorizationManager),
new FormAuthenticator(formAuthManager),
authorizer,
"Bearer",
new LbUnauthorizedHandler(defaultType)));
Expand All @@ -174,7 +175,7 @@ private ResourceSecurityDynamicFeature getAuthFilter(HaGatewayConfiguration conf
{
AuthorizationConfiguration authorizationConfig = configuration.getAuthorization();
Authorizer authorizer = (authorizationConfig != null)
? new LbAuthorizer(authorizationConfig) : new NoopAuthorizer();
? new LbAuthorizer() : new NoopAuthorizer();

AuthenticationConfiguration authenticationConfig = configuration.getAuthentication();

Expand Down
39 changes: 38 additions & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/security/AuthorizationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,21 @@
*/
package io.trino.gateway.ha.security;

import com.google.common.annotations.VisibleForTesting;
import io.trino.gateway.ha.config.AuthorizationConfiguration;
import io.trino.gateway.ha.config.LdapConfiguration;
import io.trino.gateway.ha.config.UserConfiguration;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;

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)
Expand All @@ -35,9 +39,42 @@ public AuthorizationManager(AuthorizationConfiguration configuration,
else {
lbLdapClient = null;
}
this.authorizationConfiguration = configuration;
}

public Optional<String> getPrivileges(String username)
@VisibleForTesting
public AuthorizationManager(Map<String, UserConfiguration> presetUsers, LbLdapClient lbLdapClient, AuthorizationConfiguration authorizationConfiguration)
{
this.presetUsers = presetUsers;
this.lbLdapClient = lbLdapClient;
this.authorizationConfiguration = authorizationConfiguration;
}

public String getPrivileges(String username)
{
if (authorizationConfiguration == null) {
return "ADMIN_USER_API";
Copy link
Member

Choose a reason for hiding this comment

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

I’d prefer not to give any privileges when authorizationConfiguration is not set.

}
Optional<String> memberOf = getMemberOf(username);
List<String> privileges = new ArrayList<String>();

if (authorizationConfiguration.getAdmin() != null) {
Copy link
Member

Choose a reason for hiding this comment

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

would it be cleaner to replace the ifs with

Map<String, String> rolePatterns = Map.of(
    "ADMIN", authorizationConfiguration.getAdmin(),
    "USER", authorizationConfiguration.getUser(),
    "API", authorizationConfiguration.getApi()
);

rolePatterns.forEach((role, pattern) -> {
    if (pattern != null) {
        memberOf.filter(m -> m.matches(pattern)).ifPresent(m -> privileges.add(role));
    }
});

memberOf.filter(m -> m.matches(authorizationConfiguration.getAdmin())).ifPresent(m -> privileges.add("ADMIN"));
}
if (authorizationConfiguration.getUser() != null) {
memberOf.filter(m -> m.matches(authorizationConfiguration.getUser())).ifPresent(m -> privileges.add("USER"));
}
if (authorizationConfiguration.getApi() != null) {
memberOf.filter(m -> m.matches(authorizationConfiguration.getApi())).ifPresent(m -> privileges.add("API"));
}

if (privileges.isEmpty()) {
return "";
}
return String.join("_", privileges);
}

public Optional<String> getMemberOf(String username)
{
//check the preset users
String privs = "";
Expand Down
37 changes: 27 additions & 10 deletions gateway-ha/src/main/java/io/trino/gateway/ha/security/FormAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,23 @@
*/
package io.trino.gateway.ha.security;

import com.auth0.jwt.interfaces.Claim;
import io.airlift.log.Logger;
import io.trino.gateway.ha.security.util.AuthenticationException;
import io.trino.gateway.ha.security.util.IdTokenAuthenticator;

import java.util.Map;
import java.util.Optional;

public class FormAuthenticator
implements IdTokenAuthenticator
{
private static final Logger log = Logger.get(FormAuthenticator.class);
private final LbFormAuthManager formAuthManager;
private final AuthorizationManager authorizationManager;

public FormAuthenticator(LbFormAuthManager formAuthManager,
AuthorizationManager authorizationManager)
public FormAuthenticator(LbFormAuthManager formAuthManager)
{
this.formAuthManager = formAuthManager;
this.authorizationManager = authorizationManager;
}

/**
Expand All @@ -43,11 +44,27 @@ public Optional<LbPrincipal> authenticate(String idToken)
throws AuthenticationException
{
String userIdField = formAuthManager.getUserIdField();
return formAuthManager
.getClaimsFromIdToken(idToken)
.map(c -> c.get(userIdField))
.map(Object::toString)
.map(s -> s.replace("\"", ""))
.map(sub -> new LbPrincipal(sub, authorizationManager.getPrivileges(sub)));
String privilegesField = formAuthManager.getPrivilegesField();

Map<String, Claim> claims = null;
try {
claims = formAuthManager.getClaimsFromIdToken(idToken).orElseThrow();
}
catch (Exception e) {
return Optional.empty();
}
String userId = claims.get(userIdField).asString().replace("\"", "");
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to replace \" ?


Claim claim = claims.get(privilegesField);
if (claim == null || claim.asString() == null) {
log.warn("No privileges found for user %s in idToken", userId);
throw new AuthenticationException("No privileges found for user " + userId + " in idToken");
}

String privileges = claim.asString();
Copy link
Member

Choose a reason for hiding this comment

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

Rather than setting the privileges, let's set the membefOf and everything should fall in place.

if (privileges == null) {
privileges = "";
}
return Optional.of(new LbPrincipal(userId, privileges));
}
}
2 changes: 1 addition & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/security/LbAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public Optional<LbPrincipal> authenticate(String idToken)
privileges = Optional.ofNullable(role);
}

return Optional.of(new LbPrincipal(userId, privileges));
return Optional.of(new LbPrincipal(userId, privileges.orElse("")));
}
return oauthManager
.getClaimsFromIdToken(idToken)
Expand Down
38 changes: 19 additions & 19 deletions gateway-ha/src/main/java/io/trino/gateway/ha/security/LbAuthorizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package io.trino.gateway.ha.security;

import io.airlift.log.Logger;
import io.trino.gateway.ha.config.AuthorizationConfiguration;
import io.trino.gateway.ha.security.util.Authorizer;
import jakarta.annotation.Nullable;
import jakarta.ws.rs.container.ContainerRequestContext;
Expand All @@ -23,11 +22,9 @@ public class LbAuthorizer
implements Authorizer
{
private static final Logger log = Logger.get(LbAuthorizer.class);
private final AuthorizationConfiguration configuration;

public LbAuthorizer(AuthorizationConfiguration configuration)
public LbAuthorizer()
{
this.configuration = configuration;
}

@Override
Expand All @@ -37,23 +34,26 @@ public boolean authorize(LbPrincipal principal,
{
switch (role) {
case "ADMIN":
log.info("User '%s' with memberOf(%s) was identified as ADMIN(%s)",
principal.getName(), principal.getMemberOf(), configuration.getAdmin());
return principal.getMemberOf()
.filter(m -> m.matches(configuration.getAdmin()))
.isPresent();
if (principal.getPrivileges().contains("ADMIN")) {
log.info("User '%s' with memberOf(%s) was identified as ADMIN",
principal.getName(), principal.getPrivileges());
return true;
}
return false;
case "USER":
log.info("User '%s' with memberOf(%s) identified as USER(%s)",
principal.getName(), principal.getMemberOf(), configuration.getUser());
return principal.getMemberOf()
.filter(m -> m.matches(configuration.getUser()))
.isPresent();
if (principal.getPrivileges().contains("USER")) {
log.info("User '%s' with memberOf(%s) identified as USER",
principal.getName(), principal.getPrivileges());
return true;
}
return false;
case "API":
log.info("User '%s' with memberOf(%s) identified as API(%s)",
principal.getName(), principal.getMemberOf(), configuration.getApi());
return principal.getMemberOf()
.filter(m -> m.matches(configuration.getApi()))
.isPresent();
if (principal.getPrivileges().contains("API")) {
log.info("User '%s' with memberOf(%s) identified as API",
principal.getName(), principal.getPrivileges());
return true;
}
return false;
default:
log.warn("User '%s' with role %s has no regex match based on ldap search",
principal.getName(), role);
Expand Down
11 changes: 10 additions & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/security/LbFormAuthManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,12 @@ public class LbFormAuthManager
private final Map<String, UserConfiguration> presetUsers;
private final Map<String, String> pagePermissions;
private final LbLdapClient lbLdapClient;
private final AuthorizationManager authorizationManager;

public LbFormAuthManager(FormAuthConfiguration configuration,
Map<String, UserConfiguration> presetUsers,
Map<String, String> pagePermissions)
Map<String, String> pagePermissions,
AuthorizationManager authorizationManager)
{
this.presetUsers = presetUsers;
this.pagePermissions = pagePermissions.entrySet().stream()
Expand All @@ -69,13 +71,19 @@ public LbFormAuthManager(FormAuthConfiguration configuration,
else {
lbLdapClient = null;
}
this.authorizationManager = authorizationManager;
}

public String getUserIdField()
{
return "sub";
}

public String getPrivilegesField()
{
return "privileges";
}

/**
* Login API
*
Expand Down Expand Up @@ -128,6 +136,7 @@ private String getSelfSignedToken(String username)
.withHeader(headers)
.withIssuer(SessionCookie.SELF_ISSUER_ID)
.withSubject(username)
.withClaim(getPrivilegesField(), authorizationManager.getPrivileges(username))
.sign(algorithm);
}
catch (JWTCreationException exception) {
Expand Down
15 changes: 7 additions & 8 deletions gateway-ha/src/main/java/io/trino/gateway/ha/security/LbPrincipal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,17 @@

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

public class LbPrincipal
implements Principal
{
private final String name;
private final Optional<String> memberOf;
private final String privileges;

public LbPrincipal(String name, Optional<String> memberOf)
public LbPrincipal(String name, String privileges)
{
this.name = name;
this.memberOf = memberOf;
this.privileges = privileges;
Copy link
Member

Choose a reason for hiding this comment

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

It's not a good idea to set the privileges directly in the Principal. The privileges should be calculated dynamically. If any privilege for a user changes, we need to invalidate the session which is going to be difficult. This is especially true with the LDAP based privileges.

}

@Override
Expand All @@ -39,13 +38,13 @@ public boolean equals(Object o)
return false;
}
LbPrincipal that = (LbPrincipal) o;
return name.equals(that.name) && memberOf.equals(that.memberOf);
return name.equals(that.name) && privileges.equals(that.privileges);
}

@Override
public int hashCode()
{
return Objects.hash(name, memberOf);
return Objects.hash(name, privileges);
}

@Override
Expand All @@ -54,8 +53,8 @@ public String getName()
return name;
}

public Optional<String> getMemberOf()
public String getPrivileges()
{
return this.memberOf;
return this.privileges;
}
}
3 changes: 1 addition & 2 deletions gateway-ha/src/main/java/io/trino/gateway/ha/security/NoopFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

import java.io.IOException;
import java.security.Principal;
import java.util.Optional;

import static jakarta.ws.rs.Priorities.AUTHENTICATION;

Expand All @@ -37,7 +36,7 @@ public void filter(final ContainerRequestContext requestContext)
@Override
public Principal getUserPrincipal()
{
return new LbPrincipal("user", Optional.of("ADMIN_USER_API"));
return new LbPrincipal("user", "ADMIN_USER_API");
}

@Override
Expand Down
Loading