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

Add support for AuthorizedIdentity credentials proxying #23546

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

prithvip
Copy link
Contributor

@prithvip prithvip commented Aug 29, 2024

Description

This change allows for AuthorizedIdentity to be included as a claim in a JWT token. This is required for prestodb/rfcs#23, however this contribution is valuable on its own.

Motivation and Context

Today, Presto supports credentials proxying through means of a JWT token. The proxy authenticates the client, and constructs a JWT bearer token. The token is then signed and sent to the Presto coordinator in the "AUTHORIZATION" HTTP Header. The coordinator trusts the proxy if the token was signed using a shared secret (HMAC), or RSA key.

"Principal" can be proxied through a JWT token, by means of the "subject" claim. However, the AccessControl::selectAuthorizedIdentity method requires submittal of all the X509 certificates that the client presented. This is a problem in a proxy setup, because JWT credentials could only proxy the principal, and the certificates themselves would not be forwarded. To fully support proxying, this change allows for AuthorizedIdentity to be included as a claim in the same JWT token.

An alternative approach could be to forward the X509 certificates in a client-cert header, and implement IETF RFC 9440 (https://datatracker.ietf.org/doc/rfc9440/). However, this has two major drawbacks: 1) the proxy server would only be able to authenticate clients using certificate-based authentication 2) there would be issues with header size limitations. Thus, we chose to use the JWT mechanism instead.

Impact

No user-facing changes or performance impact.

Test Plan

Unit testing

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Add support to proxy AuthorizedIdentity via JWT :pr:`23546`

@prithvip prithvip requested review from shrinidhijoshi and a team as code owners August 29, 2024 09:51
@prithvip prithvip changed the title Add support for AuthorizedUser credentials proxying Add support for AuthorizedIdentity credentials proxying Aug 29, 2024
@prithvip prithvip force-pushed the authorized-identity-forwarding branch from 766e3bd to bfd988c Compare August 29, 2024 10:01
import static java.util.Base64.getMimeDecoder;
import static java.util.Objects.requireNonNull;

public class JsonWebTokenAuthenticator
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we can add the support of passing AuthorizedIdentity in the airlift code itself instead of moving the class into presto-main and adding this functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered this approach, but it would require airlift to understand Presto-specific concepts such as AuthorizedIdentity. There are some authenticators in airlift and some authenticators in Presto, so I think it is reasonable to move this authenticator to Presto.

abhiseksaikia
abhiseksaikia previously approved these changes Sep 9, 2024
Copy link
Contributor

@abhiseksaikia abhiseksaikia left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -369,7 +369,7 @@
<dependency>
<groupId>io.jsonwebtoken</groupId>
<artifactId>jjwt-jackson</artifactId>
<scope>runtime</scope>
<scope>compile</scope>
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to remove this line. compile is the default scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

rschlussel
rschlussel previously approved these changes Sep 9, 2024
Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

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

looks good, just small comment about removing the unneeded compile scope declaration.

@prithvip prithvip merged commit fd2615e into prestodb:master Sep 17, 2024
56 checks passed
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 tasks
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.

3 participants