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

KNOX-3037 - Client credentials flow accepts essential parameters in the request body only #906

Merged
merged 1 commit into from
May 9, 2024
Merged

KNOX-3037 - Client credentials flow accepts essential parameters in the request body only #906

merged 1 commit into from
May 9, 2024

Conversation

smolnar82
Copy link
Contributor

@smolnar82 smolnar82 commented May 8, 2024 •
edited
Loading

What changes were proposed in this pull request?

As described in the corresponding JIRA, Knox no longer accepts the grant_type and client_secret as query parameters. Instead, they should be passed in the request body.

For reviewers: I'm not satisfied with the WARN message I added in case the client secret is passed as a query param. I'm hoping for a better sentence from someone :)

How was this patch tested?

Added JUnit tests and conducted manual testing:

  1. Using the request body:
$ curl -ik -X GET -H "Content-Type: application/x-www-form-urlencoded" --data "grant_type=client_credentials" --data "client_id=$CLIENT_ID" --data-urlencode "client_secret=$CLIENT_SECRET" https://localhost:8443/gateway/tokenbased/oauth/v1/token
HTTP/1.1 200 OK
Date: Wed, 08 May 2024 11:19:06 GMT
Content-Type: application/json
Content-Length: 1061

{"access_token":"eyJqa3UiOiJodHRwczpcL1wvbG9jYWxob3N0Ojg0NDNcL2dhdGV3YXlcL3Rva2VuYmFzZWRcL29hdXRoXC92MVwvandrcy5qc29uIiwia2lkIjoiQ2t3dkZFY1VBbkpUbGtyWWhjazk1RHdIYU5GeGkzdER4S2JxQ2VTWE8yOCIsInR5cCI6IkpXVCIsImFsZyI6IlJTMjU2In0.eyJzdWIiOiJhZG1pbiIsImprdSI6Imh0dHBzOlwvXC9sb2NhbGhvc3Q6ODQ0M1wvZ2F0ZXdheVwvdG9rZW5iYXNlZFwvb2F1dGhcL3YxXC9qd2tzLmpzb24iLCJraWQiOiJDa3d2RkVjVUFuSlRsa3JZaGNrOTVEd0hhTkZ4aTN0RHhLYnFDZVNYTzI4IiwiaXNzIjoiS05PWFNTTyIsImV4cCI6MTAzNjgsIm1hbmFnZWQudG9rZW4iOiJ0cnVlIiwia25veC5pZCI6ImNlNzA2MDZlLTk1OWQtNDQ5NC1hOWFiLWU3OGEyY2IwYzQ4YyJ9.fVa9Uy2UwXzl-kTJcVCaSBpXpUbs9pK5GZyU3BjyZPkkqAruo-eGJIHnkP8TP5bbiGQh2eNjKiaIUd32apXt-164IcwU3QSg7f95pYCC6XCY8g5Lcsk3rFfs-o55oaV8Uo21CJ4N762taC2sC8xKUXDWaOxttcc2uIufy_VBebCR1S2itlCgGREqL-amxeUtKs3_UQ-7ZcBTxDLO_iC5T1tlFSqdQuvW0puWTsjc8iUQ19WgvDiMqOO2MBt9aE96fUNZN41h9Vy3Y6eNWuQVv_qptRgi_Ib8G76ktjMB_PGcft4M9FuUwLyofbAKWqVVsDKUlcWoBakJ5cfRjzpK3Q","refresh_token":"b8987378-b016-4a78-99d8-5b73fcf59449","issued_token_type":"urn:ietf:params:oauth:token-type:access_token","token_type":"Bearer","expires_in":10368000}
  1. Using query params (achieved by adding the -G option in the curl command:
$ curl -G -ik -X POST -H "Content-Type: application/x-www-form-urlencoded" --data "grant_type=client_credentials" --data "client_id=$CLIENT_ID" --data-urlencode "client_secret=$CLIENT_SECRET" https://localhost:8443/gateway/tokenbased/oauth/v1/token
HTTP/1.1 400 Bad Request
Cache-Control: must-revalidate,no-cache,no-store
Content-Type: text/html;charset=iso-8859-1
Content-Length: 426
Connection: close

<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
<title>Error 400 Bad Request</title>
</head>
<body><h2>HTTP ERROR 400 Bad Request</h2>
<table>
<tr><th>URI:</th><td>/gateway/tokenbased/oauth/v1/token</td></tr>
<tr><th>STATUS:</th><td>400</td></tr>
<tr><th>MESSAGE:</th><td>Bad Request</td></tr>
<tr><th>SERVLET:</th><td>tokenbased-knox-gateway-servlet</td></tr>
</table>

</body>
</html>

Relevant gateway.log:

2024-05-09 09:28:59,694 778970b4-7973-4e3d-ab8d-b37980f082c1 ERROR knox.gateway (AbstractGatewayFilter.java:doFilter(64)) - Failed to execute filter: java.lang.SecurityException
2024-05-09 09:28:59,695 778970b4-7973-4e3d-ab8d-b37980f082c1 ERROR knox.gateway (GatewayFilter.java:doFilter(193)) - Gateway processing failed: javax.servlet.ServletException: java.lang.SecurityException
javax.servlet.ServletException: java.lang.SecurityException
	at org.apache.knox.gateway.filter.AbstractGatewayFilter.doFilter(AbstractGatewayFilter.java:65) ~[gateway-spi-2.1.0-SNAPSHOT.jar:2.1.0-SNAPSHOT]
...
Caused by: java.lang.SecurityException
	at org.apache.knox.gateway.provider.federation.jwt.filter.JWTFederationFilter.parseFromClientCredentialsFlow(JWTFederationFilter.java:282) ~[gateway-provider-security-jwt-2.1.0-SNAPSHOT.jar:2.1.0-SNAPSHOT]

@smolnar82
Copy link
Contributor Author

Cc. @kardolus

@smolnar82 smolnar82 force-pushed the KNOX-3037 branch 3 times, most recently from f407740 to 1c28262 Compare May 8, 2024 14:37
lmccay
lmccay requested changes May 8, 2024
View reviewed changes
Copy link
Contributor

@lmccay lmccay left a comment

Choose a reason for hiding this comment

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

Already made comments on this.

@smolnar82 smolnar82 requested a review from lmccay May 9, 2024 07:32
@smolnar82
Copy link
Contributor Author

@lmccay - please review the new patchset.

@smolnar82 smolnar82 self-assigned this May 9, 2024
lmccay
lmccay approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@lmccay lmccay left a comment

Choose a reason for hiding this comment

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

LGTM
+1

@smolnar82 smolnar82 merged commit a3a33aa into apache:master May 9, 2024
2 checks passed
@smolnar82 smolnar82 deleted the KNOX-3037 branch May 9, 2024 20:59
pzampino
pzampino reviewed May 9, 2024
View reviewed changes
...rc/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java
// the token as the token_id so we will get that later
token = request.getParameter(CLIENT_SECRET);
parsed = Pair.of(TokenType.Passcode, token);
if (request.getParameter(CLIENT_SECRET) != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is to disallow the secret in a query param? Does it matter that this method (getParameter(String)) also returns posted form param values in addition to query param values?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pzampino pzampino pzampino left review comments

@lmccay lmccay lmccay approved these changes

@moresandeep moresandeep Awaiting requested review from moresandeep

Assignees

@smolnar82 smolnar82

Labels
knoxtoken OAuth
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smolnar82 @lmccay @pzampino