Skip to content

pac4j: fix incompatible dependencies + authorization regression#15753

Merged
xvrl merged 7 commits intoapache:masterfrom
Pankaj260100:pankaj/FixPac4jIssue
Feb 1, 2024
Merged

pac4j: fix incompatible dependencies + authorization regression#15753
xvrl merged 7 commits intoapache:masterfrom
Pankaj260100:pankaj/FixPac4jIssue

Conversation

@Pankaj260100
Copy link
Copy Markdown
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
Copy Markdown
Member

xvrl commented Jan 24, 2024

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

xvrl
xvrl reviewed Jan 24, 2024
View reviewed changes
Comment thread extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java Outdated
xvrl
xvrl reviewed Jan 24, 2024
View reviewed changes
Comment thread extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java Outdated
@abhishekagarwal87 abhishekagarwal87 added this to the 29.0.0 milestone Jan 24, 2024
xvrl
xvrl reviewed Jan 24, 2024
View reviewed changes
Comment thread extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java Outdated
Pankaj260100
Pankaj260100 commented Jan 25, 2024
View reviewed changes
Comment thread extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java
}
},
NOOP_HTTP_ACTION_ADAPTER,
null, null, null, null);
Copy link
Copy Markdown
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
Comment thread extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java Outdated

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
Copy Markdown
Contributor Author

@Pankaj260100 Pankaj260100 Jan 25, 2024

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
Comment thread extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java Outdated
xvrl
xvrl reviewed Jan 31, 2024
View reviewed changes
Comment thread extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java Outdated
xvrl
xvrl reviewed Jan 31, 2024
View reviewed changes
Comment thread extensions-core/druid-pac4j/pom.xml
xvrl
xvrl reviewed Jan 31, 2024
View reviewed changes
Comment thread extensions-core/druid-pac4j/pom.xml Outdated
xvrl
xvrl reviewed Jan 31, 2024
View reviewed changes
Comment thread extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java Outdated
xvrl
xvrl reviewed Jan 31, 2024
View reviewed changes
Copy link
Copy Markdown
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
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