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.

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

Jump to bottom

chore: update jetty to address CVE #8639

Merged
merged 5 commits into from
Feb 3, 2022
Merged

chore: update jetty to address CVE #8639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b8eb00
chore: update jetty to address CVE
agavra Jan 21, 2022
8432d87
chore: fix the JaasAuthProvider post jetty upgrade
agavra Feb 3, 2022
a3e8c16
chore: fix the JaasAuthProviderTest
agavra Feb 3, 2022
4141bcb
chore: checkstyle
agavra Feb 3, 2022
f6bf45b
chore: change lambda reference
agavra Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 5 additions & 32 deletions ksqldb-rest-app/src/main/java/io/confluent/ksql/api/auth/BasicCallbackHandler.java
View file
Open in desktop
Expand Up @@ -16,42 +16,33 @@
package io.confluent.ksql.api.auth;

import java.io.IOException;
import java.util.Objects;
import javax.security.auth.callback.Callback;
import javax.security.auth.callback.CallbackHandler;
import javax.security.auth.callback.NameCallback;
import javax.security.auth.callback.PasswordCallback;
import javax.security.auth.callback.TextOutputCallback;
import javax.security.auth.callback.UnsupportedCallbackException;
import org.eclipse.jetty.jaas.callback.DefaultCallbackHandler;
import org.eclipse.jetty.jaas.callback.ObjectCallback;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

class BasicCallbackHandler implements CallbackHandler {
public class BasicCallbackHandler extends DefaultCallbackHandler {
Copy link
Member

Choose a reason for hiding this comment

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

Ok, can you say more about why this PR changes from the javax.security.auth.callback.CallbackHandler to the org.eclipse.jetty.jaas.callback.DefaultCallbackHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DefaultCallbackHandler is an implementation of javax.security.auth.callback.CallbackHandler that "knows" how to handle username/password combos when used in conjunction with JAASLoginService:

https://github.com/eclipse/jetty.project/blob/4166f8502635b6f3350d4644cb05c9f8419b4d5e/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/JAASLoginService.java#L189-L205

            CallbackHandler callbackHandler = null;
            if (_callbackHandlerClass == null)
                callbackHandler = new DefaultCallbackHandler();
            else
            {
                Class<?> clazz = Loader.loadClass(_callbackHandlerClass);
                callbackHandler = (CallbackHandler)clazz.getDeclaredConstructor().newInstance();
            }
            
            if (callbackHandler instanceof DefaultCallbackHandler)
            {
                DefaultCallbackHandler dch = (DefaultCallbackHandler)callbackHandler;
                if (request instanceof Request)
                    dch.setRequest((Request)request);
                dch.setCredential(credentials);
                dch.setUserName(username);
            }

Using LoginContext directly no longer directly works with the PropertyFileLoginModule (see jetty/jetty.project#5518 - requires you to use the JAASLoginService) and the JAASLoginService doesn't let you pass in instantiated callbacks, it requires passing in just the callback class. That means that I had to take advantage of the code snippet above to get the code properly wired through.

Honestly documentation here is close to impossible to find, so I had to inspect the jetty source code to figure out how to wire this through properly and I very well could be doing something wrong.


private static final Logger log = LoggerFactory.getLogger(BasicCallbackHandler.class);

private final String username;
private final String password;

BasicCallbackHandler(final String username, final String password) {
this.username = Objects.requireNonNull(username, "username");
this.password = Objects.requireNonNull(password, "password");
}

@Override
public void handle(final Callback[] callbacks)
throws IOException, UnsupportedCallbackException {
for (final Callback callback : callbacks) {
if (callback instanceof NameCallback) {
final NameCallback nc = (NameCallback) callback;
nc.setName(username);
nc.setName(getUserName());
} else if (callback instanceof ObjectCallback) {
final ObjectCallback oc = (ObjectCallback)callback;
oc.setObject(password);
oc.setObject(getCredential());
} else if (callback instanceof PasswordCallback) {
final PasswordCallback pc = (PasswordCallback) callback;
pc.setPassword(password.toCharArray());
pc.setPassword(((String) getCredential()).toCharArray());
} else if (callback instanceof TextOutputCallback) {
final TextOutputCallback toc = (TextOutputCallback) callback;
switch (toc.getMessageType()) {
Expand All @@ -73,22 +64,4 @@ public void handle(final Callback[] callbacks)
}
}
}

@Override
public boolean equals(final Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
final BasicCallbackHandler that = (BasicCallbackHandler) o;
return username.equals(that.username)
&& password.equals(that.password);
}

@Override
public int hashCode() {
return Objects.hash(username, password);
}
}
37 changes: 19 additions & 18 deletions ksqldb-rest-app/src/main/java/io/confluent/ksql/api/auth/JaasAuthProvider.java
View file
Open in desktop
Expand Up @@ -30,10 +30,9 @@
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import javax.security.auth.callback.CallbackHandler;
import javax.security.auth.login.LoginContext;
import javax.security.auth.login.LoginException;
import org.apache.commons.collections4.CollectionUtils;
import org.eclipse.jetty.jaas.JAASLoginService;
import org.eclipse.jetty.server.UserIdentity;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -51,7 +50,7 @@ public class JaasAuthProvider implements AuthProvider {
private final String contextName;

public JaasAuthProvider(final Server server, final KsqlRestConfig config) {
this(server, config, LoginContext::new);
this(server, config, JAASLoginService::new);
}

@VisibleForTesting
Expand All @@ -75,7 +74,7 @@ public JaasAuthProvider(final Server server, final KsqlRestConfig config) {
@VisibleForTesting
@FunctionalInterface
interface LoginContextSupplier {
LoginContext get(String name, CallbackHandler callbackHandler) throws LoginException;
JAASLoginService get();
}

@Override
Expand Down Expand Up @@ -107,36 +106,38 @@ private void getUser(
final List<String> allowedRoles,
final Promise<User> promise
) {
final LoginContext lc;
final JAASLoginService login = loginContextSupplier.get();
login.setCallbackHandlerClass(BasicCallbackHandler.class.getName());
login.setLoginModuleName(contextName);

try {
lc = loginContextSupplier.get(contextName, new BasicCallbackHandler(username, password));
} catch (LoginException | SecurityException e) {
log.error("Failed to create LoginContext. " + e.getMessage());
promise.fail("Failed to create LoginContext.");
return;
login.start();
} catch (final Exception e) {
log.error("Could not start login service.", e);
promise.fail("Could not start login service.");
}

try {
lc.login();
} catch (LoginException le) {
log.error("Failed to log in. " + le.getMessage());
final UserIdentity user = login.login(username, password, null);

if (user == null) {
log.error("Failed to log in. ");
promise.fail("Failed to log in: Invalid username/password.");
return;
}

// We do the actual authorization here not in the User class
final boolean authorized = validateRoles(lc, allowedRoles);
final boolean authorized = validateRoles(user, allowedRoles);

promise.complete(new JaasUser(username, authorized));
}

private static boolean validateRoles(final LoginContext lc, final List<String> allowedRoles) {
private static boolean validateRoles(final UserIdentity ui, final List<String> allowedRoles) {
if (allowedRoles.contains("*")) {
// all users allowed
return true;
}

final Set<String> userRoles = lc.getSubject().getPrincipals().stream()
final Set<String> userRoles = ui.getSubject().getPrincipals().stream()
.map(Principal::getName)
.collect(Collectors.toSet());
return !CollectionUtils.intersection(userRoles, allowedRoles).isEmpty();
Expand Down
4 changes: 3 additions & 1 deletion ksqldb-rest-app/src/test/java/io/confluent/ksql/api/auth/BasicCallbackHandlerTest.java
View file
Open in desktop
Expand Up @@ -46,7 +46,9 @@ public class BasicCallbackHandlerTest {

@Before
public void setUp() {
callbackHandler = new BasicCallbackHandler(USERNAME, PASSWORD);
callbackHandler = new BasicCallbackHandler();
callbackHandler.setUserName(USERNAME);
callbackHandler.setCredential(PASSWORD);
}

@Test
Expand Down
18 changes: 13 additions & 5 deletions ksqldb-rest-app/src/test/java/io/confluent/ksql/api/auth/JaasAuthProviderTest.java
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
Expand All @@ -40,7 +41,8 @@
import java.util.Set;
import java.util.stream.Stream;
import javax.security.auth.Subject;
import javax.security.auth.login.LoginContext;
import org.eclipse.jetty.jaas.JAASLoginService;
import org.eclipse.jetty.server.UserIdentity;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -73,21 +75,27 @@ public class JaasAuthProviderTest {
@Mock
private LoginContextSupplier loginContextSupplier;
@Mock
private LoginContext loginContext;
private JAASLoginService loginService;
@Mock
private UserIdentity userIdentity;
@Mock
private Subject subject;

private JaasAuthProvider authProvider;

@Before
public void setUp() throws Exception {
final BasicCallbackHandler callbackHandler = new BasicCallbackHandler();
callbackHandler.setUserName(USERNAME);
callbackHandler.setCredential(PASSWORD);

handleAsyncExecution();
when(config.getString(KsqlRestConfig.AUTHENTICATION_REALM_CONFIG)).thenReturn(REALM);
when(authInfo.getString("username")).thenReturn(USERNAME);
when(authInfo.getString("password")).thenReturn(PASSWORD);
when(loginContextSupplier.get(REALM, new BasicCallbackHandler(USERNAME, PASSWORD)))
.thenReturn(loginContext);
when(loginContext.getSubject()).thenReturn(subject);
when(loginContextSupplier.get()).thenReturn(loginService);
when(loginService.login(eq(USERNAME), eq(PASSWORD), any())).thenReturn(userIdentity);
when(userIdentity.getSubject()).thenReturn(subject);

authProvider = new JaasAuthProvider(server, config, loginContextSupplier);
}
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Expand Up @@ -129,7 +129,7 @@
<skip.docker.test>true</skip.docker.test>
<compile.warnings-flag>-Werror</compile.warnings-flag>
<!-- Only used to provide login module implementation for tests -->
<jetty.version>9.4.28.v20200408</jetty.version>
<jetty.version>9.4.44.v20210927</jetty.version>
<git-commit-id-plugin.version>2.2.6</git-commit-id-plugin.version>
<scala.version>2.13.2</scala.version>

Expand Down