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

chore: update jetty to address CVE #8639

Merged
merged 5 commits into from Feb 3, 2022
Merged

chore: update jetty to address CVE #8639

merged 5 commits into from Feb 3, 2022

Conversation

agavra
Copy link
Contributor

@agavra agavra commented Jan 21, 2022

Description

Fixes PRISMA-2021-0182

As part of this we need to change the code to address jetty/jetty.project#5518

Testing done

Will let the tests run, jetty is only used in testing

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@agavra agavra requested a review from a team as a code owner January 21, 2022 19:23
Copy link
Member

@jnh5y jnh5y left a comment

Choose a reason for hiding this comment

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

LGTM

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.

@@ -51,7 +50,7 @@
private final String contextName;

public JaasAuthProvider(final Server server, final KsqlRestConfig config) {
this(server, config, LoginContext::new);
this(server, config, () -> new JAASLoginService());
Copy link
Member

Choose a reason for hiding this comment

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

IntelliJ pointed out that this can be JAASLoginService::new instead if you like. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

@agavra agavra merged commit 2b177b3 into 6.0.x Feb 3, 2022
@agavra agavra deleted the jetty-cve-60x branch February 3, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants