-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore: update jetty to address CVE #8639
Changes from 4 commits
9b8eb00
8432d87
a3e8c16
4141bcb
f6bf45b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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, () -> new JAASLoginService()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IntelliJ pointed out that this can be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah yes! I had that at first, and then added params, and then removed params and the auto-refactoring got me 😉 I'll make that change |
||
} | ||
|
||
@VisibleForTesting | ||
|
@@ -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 | ||
|
@@ -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(); | ||
|
There was a problem hiding this comment.
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 theorg.eclipse.jetty.jaas.callback.DefaultCallbackHandler
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefaultCallbackHandler
is an implementation ofjavax.security.auth.callback.CallbackHandler
that "knows" how to handle username/password combos when used in conjunction withJAASLoginService
:https://github.com/eclipse/jetty.project/blob/4166f8502635b6f3350d4644cb05c9f8419b4d5e/jetty-jaas/src/main/java/org/eclipse/jetty/jaas/JAASLoginService.java#L189-L205
Using
LoginContext
directly no longer directly works with thePropertyFileLoginModule
(see jetty/jetty.project#5518 - requires you to use theJAASLoginService
) and theJAASLoginService
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.