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

pac4j: fix incompatible dependencies + authorization regression #15753

Merged
merged 7 commits into from
Feb 1, 2024
Merged

pac4j: fix incompatible dependencies + authorization regression #15753

merged 7 commits into from
Feb 1, 2024

Conversation

Pankaj260100
Copy link
Contributor

@Pankaj260100 Pankaj260100 commented Jan 24, 2024 •
edited by xvrl
Loading

Description

  • After upgrading the pac4j version here: Upgrade pac4j-oidc to 4.5.7 to address CVE-2021-44878 #15522. We were not able to access the druid ui.
  • Upgraded the Nimbus libraries version to a compatible version to pac4j.
  • In the older pac4j version, when we return RedirectAction there we also update the webcontext Response status code and add the authentication URL to the header. But in the newer pac4j version, we just simply return the RedirectAction. So that's why it was not getting redirected to the generated authentication URL.
  • To fix the above, I have updated the NOOP_HTTP_ACTION_ADAPTER to JEE_HTTP_ACTION_ADAPTER and it updates the HTTP Response in context as per the HTTP Action.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@Pankaj260100 Pankaj260100 changed the title Fix Druid UI classCast Exception error. Fix Pac4j Issue Jan 24, 2024
@xvrl
Copy link
Member

xvrl commented Jan 24, 2024

thanks @Pankaj260100 , could we add a test for this to make sure we don't regress again?

@abhishekagarwal87 abhishekagarwal87 added this to the 29.0.0 milestone Jan 24, 2024
Pankaj260100
Pankaj260100 commented Jan 25, 2024
View reviewed changes
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java
@@ -110,11 +110,11 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo
return profiles.iterator().next().getId();
}
},
NOOP_HTTP_ACTION_ADAPTER,
null, null, null, null);
Copy link
Contributor Author

@Pankaj260100 Pankaj260100 Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Changed the Authorizer from null to "none". In the older version, if it is null, it simply returns authenticated and authorized -> grant access. But in the 4.5.7 pac4j version, it uses CsrfAuthorizer as default, And because of this, I was getting 403 in API calls. So, I have set it to "none".

Pankaj260100
Pankaj260100 commented Jan 25, 2024
View reviewed changes
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java Outdated
@@ -48,11 +48,12 @@ public class Pac4jFilter implements Filter
{
private static final Logger LOGGER = new Logger(Pac4jFilter.class);

private static final HttpActionAdapter<String, JEEContext> NOOP_HTTP_ACTION_ADAPTER = (HttpAction code, JEEContext ctx) -> null;
// JEE_HTTP_ACTION_ADAPTER updates the response in the context according to the HTTPAction.
private static final HttpActionAdapter<Object, JEEContext> JEE_HTTP_ACTION_ADAPTER = new JEEHttpActionAdapter();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Earlier NOOP_HTTP_ACTION_ADAPTER was working fine because the response in the context was getting updated when generating HTTPAction. But now in 4.5.7 pac4j, it just simply returns the HTTPAction, and we need HTTPActionAdapter to update the context.

@Pankaj260100 Pankaj260100 requested a review from xvrl January 25, 2024 10:02
xvrl
xvrl reviewed Jan 31, 2024
View reviewed changes
Copy link
Member

@xvrl xvrl left a comment

Choose a reason for hiding this comment

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

a few minor nits, otherwise LGTM

@Pankaj260100 Pankaj260100 changed the title Fix Pac4j Issue Fix Druid UI class cast exception due to pac4j upgrade Jan 31, 2024
@Pankaj260100 Pankaj260100 requested a review from xvrl January 31, 2024 18:42
@xvrl xvrl changed the title Fix Druid UI class cast exception due to pac4j upgrade pac4j: fix incompatible dependencies + authorization regression Feb 1, 2024
@xvrl xvrl merged commit 65857dc into apache:master Feb 1, 2024
78 of 83 checks passed
Pankaj260100 added a commit to confluentinc/druid that referenced this pull request Feb 2, 2024
@Pankaj260100
pac4j: fix incompatible dependencies + authorization regression (apac…
4f0caed 
…he#15753)

- After upgrading the pac4j version in: apache#15522. We were not able to access the druid ui. 
- Upgraded the Nimbus libraries version to a compatible version to pac4j.
- In the older pac4j version, when we return RedirectAction there we also update the webcontext Response status code and add the authentication URL to the header. But in the newer pac4j version, we just simply return the RedirectAction. So that's why it was not getting redirected to the generated authentication URL.
- To fix the above, I have updated the NOOP_HTTP_ACTION_ADAPTER to JEE_HTTP_ACTION_ADAPTER and it updates the HTTP Response in context as per the HTTP Action.
@Pankaj260100 Pankaj260100 mentioned this pull request Feb 2, 2024
Merged
10 tasks
pagrawal10 pushed a commit to confluentinc/druid that referenced this pull request Feb 6, 2024
@Pankaj260100 @KeerthanaSrikanth
[BackPort]: Upgrade pac4j-oidc to 4.5.7 to address CVE-2021-44878 (#185)
62d8a38 
* Upgrade pac4j-oidc to 4.5.7 to address CVE-2021-44878 (apache#15522)

* Upgrade org.pac4j:pac4j-oidc to 4.5.5 to address CVE-2021-44878
* add CVE suppression and notes, since vulnerability scan still shows this CVE
* Add tests to improve coverage

* pac4j: fix incompatible dependencies + authorization regression (apache#15753)

- After upgrading the pac4j version in: apache#15522. We were not able to access the druid ui. 
- Upgraded the Nimbus libraries version to a compatible version to pac4j.
- In the older pac4j version, when we return RedirectAction there we also update the webcontext Response status code and add the authentication URL to the header. But in the newer pac4j version, we just simply return the RedirectAction. So that's why it was not getting redirected to the generated authentication URL.
- To fix the above, I have updated the NOOP_HTTP_ACTION_ADAPTER to JEE_HTTP_ACTION_ADAPTER and it updates the HTTP Response in context as per the HTTP Action.

---------

Co-authored-by: Keerthana Srikanth <ksrikanth@confluent.io>
LakshSingla pushed a commit to LakshSingla/druid that referenced this pull request Feb 7, 2024
@Pankaj260100 @LakshSingla
pac4j: fix incompatible dependencies + authorization regression (apac…
88407a1 
…he#15753)

- After upgrading the pac4j version in: apache#15522. We were not able to access the druid ui. 
- Upgraded the Nimbus libraries version to a compatible version to pac4j.
- In the older pac4j version, when we return RedirectAction there we also update the webcontext Response status code and add the authentication URL to the header. But in the newer pac4j version, we just simply return the RedirectAction. So that's why it was not getting redirected to the generated authentication URL.
- To fix the above, I have updated the NOOP_HTTP_ACTION_ADAPTER to JEE_HTTP_ACTION_ADAPTER and it updates the HTTP Response in context as per the HTTP Action.
@LakshSingla LakshSingla mentioned this pull request Feb 7, 2024
Merged
cryptoe pushed a commit that referenced this pull request Feb 7, 2024
@LakshSingla @Pankaj260100
pac4j: fix incompatible dependencies + authorization regression (#15753…
8cfaaac 
…) (#15851)

- After upgrading the pac4j version in: #15522. We were not able to access the druid ui. 
- Upgraded the Nimbus libraries version to a compatible version to pac4j.
- In the older pac4j version, when we return RedirectAction there we also update the webcontext Response status code and add the authentication URL to the header. But in the newer pac4j version, we just simply return the RedirectAction. So that's why it was not getting redirected to the generated authentication URL.
- To fix the above, I have updated the NOOP_HTTP_ACTION_ADAPTER to JEE_HTTP_ACTION_ADAPTER and it updates the HTTP Response in context as per the HTTP Action.

Co-authored-by: PANKAJ KUMAR <87029331+Pankaj260100@users.noreply.github.com>
@LakshSingla LakshSingla mentioned this pull request Feb 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xvrl xvrl xvrl approved these changes

Assignees
No one assigned
Labels
Area - Dependencies Bug
Projects
None yet
Milestone
29.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Pankaj260100 @xvrl @abhishekagarwal87