Skip to content
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.

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

fix(security): play framework upgrade #6626

Merged
Merged
Show file tree
Hide file tree
Changes from 5 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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,6 @@ temp/**

# frontend assets
datahub-frontend/public/**
datahub-frontend/test/resources/public/**

.remote*
32 changes: 17 additions & 15 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ buildscript {
ext.testContainersVersion = '1.17.4'
ext.jacksonVersion = '2.13.4'
ext.jettyVersion = '9.4.46.v20220331'
ext.playVersion = '2.8.18'
ext.log4jVersion = '2.19.0'
ext.slf4jVersion = '1.7.32'
ext.logbackClassic = '1.2.11'
Expand Down Expand Up @@ -50,7 +51,7 @@ project.ext.spec = [
]

project.ext.externalDependency = [
'akkaHttp': 'com.typesafe.akka:akka-http-core_2.12:10.1.15',
'akkaHttp': 'com.typesafe.akka:akka-http-core_2.12:10.2.10',
'antlr4Runtime': 'org.antlr:antlr4-runtime:4.7.2',
'antlr4': 'org.antlr:antlr4:4.7.2',
'assertJ': 'org.assertj:assertj-core:3.11.1',
Expand Down Expand Up @@ -80,7 +81,7 @@ project.ext.externalDependency = [
'graphqlJava': 'com.graphql-java:graphql-java:' + graphQLJavaVersion,
'graphqlJavaScalars': 'com.graphql-java:graphql-java-extended-scalars:' + graphQLJavaVersion,
'gson': 'com.google.code.gson:gson:2.8.9',
'guice': 'com.google.inject:guice:4.2.2',
'guice': 'com.google.inject:guice:4.2.3',
'guava': 'com.google.guava:guava:27.0.1-jre',
'h2': 'com.h2database:h2:2.1.214',
'hadoopClient': 'org.apache.hadoop:hadoop-client:3.2.1',
Expand Down Expand Up @@ -121,6 +122,7 @@ project.ext.externalDependency = [
'log4jCore': "org.apache.logging.log4j:log4j-core:$log4jVersion",
'log4jApi': "org.apache.logging.log4j:log4j-api:$log4jVersion",
'log4j12Api': "org.slf4j:log4j-over-slf4j:$slf4jVersion",
'log4j2Api': "org.apache.logging.log4j:log4j-to-slf4j:$log4jVersion",
'lombok': 'org.projectlombok:lombok:1.18.12',
'mariadbConnector': 'org.mariadb.jdbc:mariadb-java-client:2.6.0',
'mavenArtifact': "org.apache.maven:maven-artifact:$mavenVersion",
Expand All @@ -137,17 +139,18 @@ project.ext.externalDependency = [
'opentracingJdbc':'io.opentracing.contrib:opentracing-jdbc:0.2.15',
'parquet': 'org.apache.parquet:parquet-avro:1.12.3',
'picocli': 'info.picocli:picocli:4.5.0',
'playCache': 'com.typesafe.play:play-cache_2.12:2.7.6',
'playEhcache': 'com.typesafe.play:play-ehcache_2.12:2.7.6',
'playWs': 'com.typesafe.play:play-ahc-ws-standalone_2.12:2.0.8',
'playDocs': 'com.typesafe.play:play-docs_2.12:2.7.6',
'playGuice': 'com.typesafe.play:play-guice_2.12:2.7.6',
'playJavaJdbc': 'com.typesafe.play:play-java-jdbc_2.12:2.7.6',
'playAkkaHttpServer': 'com.typesafe.play:play-akka-http-server_2.12:2.7.6',
'playServer': 'com.typesafe.play:play-server_2.12:2.7.6',
'playTest': 'com.typesafe.play:play-test_2.12:2.7.6',
'pac4j': 'org.pac4j:pac4j-oidc:3.6.0',
'playPac4j': 'org.pac4j:play-pac4j_2.12:8.0.2',
'playCache': "com.typesafe.play:play-cache_2.12:$playVersion",
'playEhcache': "com.typesafe.play:play-ehcache_2.12:$playVersion",
'playWs': 'com.typesafe.play:play-ahc-ws-standalone_2.12:2.1.10',
'playDocs': "com.typesafe.play:play-docs_2.12:$playVersion",
'playGuice': "com.typesafe.play:play-guice_2.12:$playVersion",
'playJavaJdbc': "com.typesafe.play:play-java-jdbc_2.12:$playVersion",
'playAkkaHttpServer': "com.typesafe.play:play-akka-http-server_2.12:$playVersion",
'playServer': "com.typesafe.play:play-server_2.12:$playVersion",
'playTest': "com.typesafe.play:play-test_2.12:$playVersion",
'playFilters': "com.typesafe.play:filters-helpers_2.12:$playVersion",
'pac4j': 'org.pac4j:pac4j-oidc:4.5.7',
'playPac4j': 'org.pac4j:play-pac4j_2.12:9.0.2',
'postgresql': 'org.postgresql:postgresql:42.3.8',
'protobuf': 'com.google.protobuf:protobuf-java:3.19.6',
'rangerCommons': 'org.apache.ranger:ranger-plugins-common:2.3.0',
Expand Down Expand Up @@ -199,7 +202,6 @@ configure(subprojects.findAll {! it.name.startsWith('spark-lineage') }) {
exclude group: "io.netty", module: "netty"
exclude group: "log4j", module: "log4j"
exclude group: "org.springframework.boot", module: "spring-boot-starter-logging"
exclude group: "org.apache.logging.log4j", module: "log4j-to-slf4j"
exclude group: "com.vaadin.external.google", module: "android-json"
exclude group: "org.slf4j", module: "slf4j-reload4j"
exclude group: "org.slf4j", module: "slf4j-log4j12"
Expand All @@ -226,7 +228,7 @@ subprojects {
dependencies {
testCompile externalDependency.testng
constraints {
implementation('io.netty:netty-all:4.1.68.Final')
implementation('io.netty:netty-all:4.1.85.Final')
implementation('org.apache.commons:commons-compress:1.21')
implementation('org.apache.velocity:velocity-engine-core:2.3')
implementation('org.hibernate:hibernate-validator:6.0.20.Final')
Expand Down
2 changes: 1 addition & 1 deletion datahub-frontend/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ DataHub frontend is a [Play](https://www.playframework.com/) service written in
between [DataHub GMS](../metadata-service) which is the backend service and [DataHub Web](../datahub-web-react/README.md).

## Pre-requisites
* You need to have [JDK8](https://www.oracle.com/java/technologies/jdk8-downloads.html)
* You need to have [JDK11](https://openjdk.org/projects/jdk/11/)
installed on your machine to be able to build `DataHub Frontend`.
* You need to have [Chrome](https://www.google.com/chrome/) web browser
installed to be able to build because UI tests have a dependency on `Google Chrome`.
Expand Down
2 changes: 1 addition & 1 deletion datahub-frontend/app/auth/AuthModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ protected void configure() {
final String aesKeyHash = DigestUtils.sha1Hex(aesKeyBase.getBytes(StandardCharsets.UTF_8));
final String aesEncryptionKey = aesKeyHash.substring(0, 16);
playCacheCookieStore = new PlayCookieSessionStore(
new ShiroAesDataEncrypter(aesEncryptionKey));
new ShiroAesDataEncrypter(aesEncryptionKey.getBytes()));
} catch (Exception e) {
throw new RuntimeException("Failed to instantiate Pac4j cookie session store!", e);
}
Expand Down
16 changes: 8 additions & 8 deletions datahub-frontend/app/auth/AuthUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ public class AuthUtils {
*
* Returns true if the request is eligible to be forwarded to GMS, false otherwise.
*/
public static boolean isEligibleForForwarding(Http.Context ctx) {
return hasValidSessionCookie(ctx) || hasAuthHeader(ctx);
public static boolean isEligibleForForwarding(Http.Request req) {
return hasValidSessionCookie(req) || hasAuthHeader(req);
}

/**
Expand All @@ -75,17 +75,17 @@ public static boolean isEligibleForForwarding(Http.Context ctx) {
* Note that we depend on the presence of 2 cookies, one accessible to the browser and one not,
* as well as their agreement to determine authentication status.
*/
public static boolean hasValidSessionCookie(final Http.Context ctx) {
return ctx.session().containsKey(ACTOR)
&& ctx.request().cookie(ACTOR) != null
&& ctx.session().get(ACTOR).equals(ctx.request().cookie(ACTOR).value());
public static boolean hasValidSessionCookie(final Http.Request req) {
return req.session().data().containsKey(ACTOR)
&& req.cookie(ACTOR) != null
&& req.session().data().get(ACTOR).equals(req.cookie(ACTOR).value());
}

/**
* Returns true if a request includes the Authorization header, false otherwise
*/
public static boolean hasAuthHeader(final Http.Context ctx) {
return ctx.request().getHeaders().contains(Http.HeaderNames.AUTHORIZATION);
public static boolean hasAuthHeader(final Http.Request req) {
return req.getHeaders().contains(Http.HeaderNames.AUTHORIZATION);
}

/**
Expand Down
27 changes: 4 additions & 23 deletions datahub-frontend/app/auth/Authenticator.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,40 +28,21 @@ public Authenticator(@Nonnull Config config) {
}

@Override
public String getUsername(@Nonnull Http.Context ctx) {
public Optional<String> getUsername(@Nonnull Http.Request req) {
if (this.metadataServiceAuthEnabled) {
// If Metadata Service auth is enabled, we only want to verify presence of the
// "Authorization" header OR the presence of a frontend generated session cookie.
// At this time, the actor is still considered to be unauthenicated.
return AuthUtils.isEligibleForForwarding(ctx) ? "urn:li:corpuser:UNKNOWN" : null;
return Optional.ofNullable(AuthUtils.isEligibleForForwarding(req) ? "urn:li:corpuser:UNKNOWN" : null);
} else {
// If Metadata Service auth is not enabled, verify the presence of a valid session cookie.
return AuthUtils.hasValidSessionCookie(ctx) ? ctx.session().get(ACTOR) : null;
}
}

@Override
public Optional<String> getUsername(@Nonnull Http.Request request) {
Http.Context ctx = Http.Context.current();
if (this.metadataServiceAuthEnabled) {
// If Metadata Service auth is enabled, we only want to verify presence of the
// "Authorization" header OR the presence of a frontend generated session cookie.
// At this time, the actor is still considered to be unauthenicated.
return Optional.ofNullable(AuthUtils.isEligibleForForwarding(ctx) ? "urn:li:corpuser:UNKNOWN" : null);
} else {
// If Metadata Service auth is not enabled, verify the presence of a valid session cookie.
return Optional.ofNullable(AuthUtils.hasValidSessionCookie(ctx) ? ctx.session().get(ACTOR) : null);
return Optional.ofNullable(AuthUtils.hasValidSessionCookie(req) ? req.session().data().get(ACTOR) : null);
}
}

@Override
@Nonnull
public Result onUnauthorized(@Nullable Http.Context ctx) {
return unauthorized();
}

@Override
public Result onUnauthorized(Http.Request req) {
public Result onUnauthorized(@Nullable Http.Request req) {
return unauthorized();
}
}
3 changes: 2 additions & 1 deletion datahub-frontend/app/auth/sso/SsoProvider.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package auth.sso;

import org.pac4j.core.client.Client;
import org.pac4j.core.credentials.Credentials;

/**
* A thin interface over a Pac4j {@link Client} object and its
Expand Down Expand Up @@ -40,6 +41,6 @@ public String getCommonName() {
/**
* Retrieves an initialized Pac4j {@link Client}.
*/
Client<?, ?> client();
Client<? extends Credentials> client();

}
17 changes: 12 additions & 5 deletions datahub-frontend/app/auth/sso/oidc/OidcAuthorizationGenerator.java
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package auth.sso.oidc;

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

import org.pac4j.core.authorization.generator.AuthorizationGenerator;
import org.pac4j.core.context.WebContext;
import org.pac4j.core.profile.AttributeLocation;
import org.pac4j.core.profile.CommonProfile;
import org.pac4j.core.profile.UserProfile;
import org.pac4j.core.profile.definition.ProfileDefinition;
import org.pac4j.oidc.profile.OidcProfile;
import org.slf4j.Logger;
Expand All @@ -18,33 +20,38 @@ public class OidcAuthorizationGenerator implements AuthorizationGenerator {

private static final Logger logger = LoggerFactory.getLogger(OidcAuthorizationGenerator.class);

private final ProfileDefinition profileDef;
private final ProfileDefinition<?> profileDef;

private final OidcConfigs oidcConfigs;

public OidcAuthorizationGenerator(final ProfileDefinition profileDef, final OidcConfigs oidcConfigs) {
public OidcAuthorizationGenerator(final ProfileDefinition<?> profileDef, final OidcConfigs oidcConfigs) {
this.profileDef = profileDef;
this.oidcConfigs = oidcConfigs;
}

@Override
public CommonProfile generate(WebContext context, CommonProfile profile) {
public Optional<UserProfile> generate(WebContext context, UserProfile profile) {
if (oidcConfigs.getExtractJwtAccessTokenClaims().orElse(false)) {
try {
final JWT jwt = JWTParser.parse(((OidcProfile) profile).getAccessToken().getValue());

CommonProfile commonProfile = new CommonProfile();

for (final Entry<String, Object> entry : jwt.getJWTClaimsSet().getClaims().entrySet()) {
final String claimName = entry.getKey();

if (profile.getAttribute(claimName) == null) {
profileDef.convertAndAdd(profile, AttributeLocation.PROFILE_ATTRIBUTE, claimName, entry.getValue());
profileDef.convertAndAdd(commonProfile, AttributeLocation.PROFILE_ATTRIBUTE, claimName, entry.getValue());
}
}

return Optional.of(commonProfile);
} catch (Exception e) {
logger.warn("Cannot parse access token claims", e);
}
}

return profile;
return Optional.ofNullable(profile);
}

}
17 changes: 10 additions & 7 deletions datahub-frontend/app/auth/sso/oidc/OidcCallbackLogic.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,16 @@
import org.pac4j.core.http.adapter.HttpActionAdapter;
import org.pac4j.core.profile.CommonProfile;
import org.pac4j.core.profile.ProfileManager;
import org.pac4j.core.profile.UserProfile;
import org.pac4j.play.PlayWebContext;
import play.mvc.Result;
import auth.sso.SsoManager;

import static auth.AuthUtils.ACCESS_TOKEN;
import static auth.AuthUtils.ACTOR;
import static auth.AuthUtils.createActorCookie;
import static com.linkedin.metadata.Constants.*;
import static play.mvc.Results.*;
import static auth.AuthUtils.*;
import static play.mvc.Results.internalServerError;


/**
Expand Down Expand Up @@ -101,17 +104,17 @@ public Result perform(PlayWebContext context, Config config,

// By this point, we know that OIDC is the enabled provider.
final OidcConfigs oidcConfigs = (OidcConfigs) _ssoManager.getSsoProvider().configs();
return handleOidcCallback(oidcConfigs, result, context, getProfileManager(context, config));
return handleOidcCallback(oidcConfigs, result, context, getProfileManager(context));
}

private Result handleOidcCallback(final OidcConfigs oidcConfigs, final Result result, final PlayWebContext context,
final ProfileManager<CommonProfile> profileManager) {
final ProfileManager<UserProfile> profileManager) {

log.debug("Beginning OIDC Callback Handling...");

if (profileManager.isAuthenticated()) {
// If authenticated, the user should have a profile.
final CommonProfile profile = profileManager.get(true).get();
final CommonProfile profile = (CommonProfile) profileManager.get(true).get();
log.debug(String.format("Found authenticated user with profile %s", profile.getAttributes().toString()));

// Extract the User name required to log into DataHub.
Expand Down Expand Up @@ -149,8 +152,8 @@ private Result handleOidcCallback(final OidcConfigs oidcConfigs, final Result re

// Successfully logged in - Generate GMS login token
final String accessToken = _authClient.generateSessionTokenForUser(corpUserUrn.getId());
context.getJavaSession().put(ACCESS_TOKEN, accessToken);
context.getJavaSession().put(ACTOR, corpUserUrn.toString());
context.getNativeSession().adding(ACCESS_TOKEN, accessToken);
Copy link
Collaborator

Choose a reason for hiding this comment

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

fancy!

context.getNativeSession().adding(ACTOR, corpUserUrn.toString());
return result.withCookies(createActorCookie(corpUserUrn.toString(), oidcConfigs.getSessionTtlInHours()));
}
return internalServerError(
Expand Down
7 changes: 3 additions & 4 deletions datahub-frontend/app/auth/sso/oidc/OidcProvider.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import org.pac4j.core.http.callback.PathParameterCallbackUrlResolver;
import org.pac4j.oidc.config.OidcConfiguration;
import org.pac4j.oidc.credentials.OidcCredentials;
import org.pac4j.oidc.profile.OidcProfile;
import org.pac4j.oidc.profile.OidcProfileDefinition;


Expand All @@ -27,15 +26,15 @@ public class OidcProvider implements SsoProvider<OidcConfigs> {
private static final String OIDC_CLIENT_NAME = "oidc";

private final OidcConfigs _oidcConfigs;
private final Client<OidcCredentials, OidcProfile> _oidcClient; // Used primarily for redirecting to IdP.
private final Client<OidcCredentials> _oidcClient; // Used primarily for redirecting to IdP.

public OidcProvider(final OidcConfigs configs) {
_oidcConfigs = configs;
_oidcClient = createPac4jClient();
}

@Override
public Client<OidcCredentials, OidcProfile> client() {
public Client<OidcCredentials> client() {
return _oidcClient;
}

Expand All @@ -49,7 +48,7 @@ public SsoProtocol protocol() {
return SsoProtocol.OIDC;
}

private Client<OidcCredentials, OidcProfile> createPac4jClient() {
private Client<OidcCredentials> createPac4jClient() {
final OidcConfiguration oidcConfiguration = new OidcConfiguration();
oidcConfiguration.setClientId(_oidcConfigs.getClientId());
oidcConfiguration.setSecret(_oidcConfigs.getClientSecret());
Expand Down
12 changes: 7 additions & 5 deletions datahub-frontend/app/auth/sso/oidc/OidcResponseErrorHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import org.slf4j.LoggerFactory;
import play.mvc.Result;

import java.util.Optional;

import static play.mvc.Results.internalServerError;
import static play.mvc.Results.unauthorized;

Expand All @@ -26,7 +28,7 @@ public static Result handleError(final PlayWebContext context) {
getError(context),
getErrorDescription(context));

if (getError(context).equals("access_denied")) {
if (getError(context).isPresent() && getError(context).get().equals("access_denied")) {
return unauthorized(String.format("Access denied. "
+ "The OIDC service responded with 'Access denied'. "
+ "It seems that you don't have access to this application yet. Please apply for access. \n\n"
Expand All @@ -38,18 +40,18 @@ public static Result handleError(final PlayWebContext context) {

return internalServerError(
String.format("Internal server error. The OIDC service responded with an error: '%s'.\n"
+ "Error description: '%s'", getError(context), getErrorDescription(context)));
+ "Error description: '%s'", getError(context).orElse(""), getErrorDescription(context).orElse("")));
}

public static boolean isError(final PlayWebContext context) {
return getError(context) != null && !getError(context).isEmpty();
return getError(context).isPresent() && !getError(context).get().isEmpty();
}

public static String getError(final PlayWebContext context) {
public static Optional<String> getError(final PlayWebContext context) {
return context.getRequestParameter(ERROR_FIELD_NAME);
}

public static String getErrorDescription(final PlayWebContext context) {
public static Optional<String> getErrorDescription(final PlayWebContext context) {
return context.getRequestParameter(ERROR_DESCRIPTION_FIELD_NAME);
}
}
Loading