Add JWT for internal communication by nmahadevuni · Pull Request #19706 · prestodb/presto

nmahadevuni · 2023-05-19T09:51:48Z

Supports JWT for authentication of internal requests. This is required for secure internal communication, especially when used in conjunction with external
user authentication such as PASSWORD, LDAP etc

Cherry-pick of trinodb/trino#2032 Cherry-pick of trinodb/trino#2093 Cherry-pick of trinodb/trino#2202 Cherry-pick of trinodb/trino#11944

Test plan -

== RELEASE NOTES ==

General Changes
* Add support for internal authentication using JWT. Can be configured using configs "internal-communication.jwt.enabled=[true/false]" and "internal-communication.shared-secret=<shared-secret-value>"

tdcmeehan

Can you please add documentation of this new configuration parameter in internal-communication.rst? Also I think this warrants release notes.

tdcmeehan · 2023-05-22T14:20:55Z

CC some Meta folks as I think this may require some config properties updates in Meta clusters. @ajaygeorge @amitkdutta @neeradsomanchi

ethanyzhang · 2023-05-22T14:26:17Z

Hi @skairali, please also review this PR. Thanks!

amitkdutta · 2023-05-22T17:27:26Z

A noob question. What's the value of internal-communication.shared-secret, and is it always used when https is enabled? CC: @neeradsomanchi @pranjalssh

pranjalssh · 2023-05-22T17:34:04Z

presto-main/src/main/java/com/facebook/presto/server/InternalAuthenticationManager.java

isRequiredSharedSecretSet() contains !isHttpsRequired() so this exception message doesn't match with that. Do we need to set this for http or https?

nmahadevuni · 2023-05-22T17:35:54Z

A noob question. What's the value of internal-communication.shared-secret, and is it always used when https is enabled? CC: @neeradsomanchi @pranjalssh

It can be any value. A large random value is generally recommended. The same value should be added to all nodes' config properties. With this PR, it will become mandatory to use shared secret when HTTPS is enabled(which is the case when external authentication is used or for secure internal communication). The nodes will fail to start if shared secret is not set when HTTPS is used. I will add documentation change soon.

amitkdutta · 2023-05-22T18:41:22Z

A noob question. What's the value of internal-communication.shared-secret, and is it always used when https is enabled? CC: @neeradsomanchi @pranjalssh

It can be any value. A large random value is generally recommended. The same value should be added to all nodes' config properties. With this PR, it will become mandatory to use shared secret when HTTPS is enabled(which is the case when external authentication is used or for secure internal communication). The nodes will fail to start if shared secret is not set when HTTPS is used. I will add documentation change soon.

@nmahadevuni
It might be useful to make it optional initially. We need to figure out how to deploy it. We also need to pass this secret in native worker. Currently it takes following properties:

https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/http/HttpServer.cpp#L112C11-L117

In wangle::SSLContextConfig
https://github.com/facebook/wangle/blob/main/wangle/ssl/SSLContextConfig.h#L53-L56

I see they have following properties

    std::string certPath;
    std::string keyPath;
    std::string passwordPath;
    bool isBuffer{false};

Is this shared secret maps to passworkdPath?

ajaygeorge · 2023-05-23T00:28:01Z

presto-main/src/main/java/com/facebook/presto/server/HttpServerModule.java

Curious. Why are we copying stuff over from prestodb/airlift
https://github.com/prestodb/airlift/blob/master/http-server/src/main/java/com/facebook/airlift/http/server/HttpServerModule.java

Global AuthenticationFilter(part of the airlift module) was bound in the HttpServerModule. We had to enhance this AuthenticationFilter to use the new InternalAuthenticationManager. So I copied over the HttpServerModule to presto and bind the new AuthenticationFilter here.

I haven't had a chance to read through the entire code, so I maybe completely off here :) .
If binding in a new filter is the concern, can't we do something similar to what ranger module did for wiring in RangerBasicAuthHttpRequestFilter

presto/presto-hive/src/main/java/com/facebook/presto/hive/security/ranger/RangerBasedAccessControlModule.java

Lines 36 to 39 in 09d5b33

httpClientBinder(binder).bindHttpClient("ranger", ForRangerInfo.class)

.withConfigDefaults(config -> config.setAuthenticationEnabled(false))

.withFilter(RangerBasicAuthHttpRequestFilter.class);

}

I did something similar, added below line of code in ServerSecurityModule to bind the new filter, but it didn't take effect.
newSetBinder(binder, Filter.class, TheServlet.class).addBinding() .to(AuthenticationFilter.class).in(Scopes.SINGLETON);

There is a HttpServerBinder too, but that doesn't have any method to bind Filter class.

nmahadevuni · 2023-05-25T15:29:24Z

A noob question. What's the value of internal-communication.shared-secret, and is it always used when https is enabled? CC: @neeradsomanchi @pranjalssh

It can be any value. A large random value is generally recommended. The same value should be added to all nodes' config properties. With this PR, it will become mandatory to use shared secret when HTTPS is enabled(which is the case when external authentication is used or for secure internal communication). The nodes will fail to start if shared secret is not set when HTTPS is used. I will add documentation change soon.

@nmahadevuni It might be useful to make it optional initially. We need to figure out how to deploy it. We also need to pass this secret in native worker. Currently it takes following properties:

https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/http/HttpServer.cpp#L112C11-L117

In wangle::SSLContextConfig https://github.com/facebook/wangle/blob/main/wangle/ssl/SSLContextConfig.h#L53-L56

I see they have following properties
    std::string certPath;
    std::string keyPath;
    std::string passwordPath;
    bool isBuffer{false};
Is this shared secret maps to passworkdPath?

passwordPath is not shared secret. JWT is a new kind of authentication which kind of overrides the certificate based authentication. I will try to make this optional.

skairali

Seems working in my testing

tdcmeehan · 2023-06-12T14:08:16Z

Please add a release note

nmahadevuni · 2023-06-13T05:44:22Z

Please add a release note

@tdcmeehan Added a release note. Please review.

tdcmeehan · 2023-06-15T17:40:06Z

Will merge tomorrow unless there's any more objections CC: @ajaygeorge @pranjalssh @amitkdutta

zacw7

Thanks for making the change. I don't see issues % a couple nits. Will let the experts make the call.

zacw7 · 2023-06-15T20:50:55Z

presto-main/src/main/java/com/facebook/presto/server/HttpServerModule.java

Is this variable used anywhere?

No. Will remove it.

zacw7 · 2023-06-15T21:08:06Z

presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java

Is there method used anywhere? Also I'm not sure if this is the right place as I couldn't find similar usage.

Will remove it.

Sorry, this method is annotated with AssertTrue, so it will be checked at server startup. Keeping it.

Adding creation and verification of a JWT signed using HMAC SHA-256. The JWT is used for authentication of internal communication. On the Java side PR prestodb#19706 added authentication for internal communication using a JWT token signed with HMAC SHA-256. Adding this comment enables Prestissimo to to be configured to verify internal communication requests, e.g from a Java coordinator. New system configuration options: - internal-communication.jwt.enabled=[true/false] - internal-communication.shared-secret=<shared-secret-value> - internal-communication.jwt.expiration-seconds=<value in seconds> A new external dependency jet-cpp from the https://github.com/Thalhammer/jwt-cpp repo is added to a new setup-adapter.sh script for Prestissimo. In the dependency directory run "<path to presto-native-execution>/scripts/setup-adapter.sh jwt" to install it The jwt-cpp project handles the creation/parsing/verification of the JWT. A new build options are added to the build environment: PRESTO_ENABLE_JWT - default off, if on adds jet creation and verification capability, turned on during the pipeline build.

Adding creation and verification of a JWT signed using HMAC SHA-256. The JWT is used for authentication of internal communication. On the Java side PR #19706 added authentication for internal communication using a JWT token signed with HMAC SHA-256. Adding this comment enables Prestissimo to to be configured to verify internal communication requests, e.g from a Java coordinator. New system configuration options: - internal-communication.jwt.enabled=[true/false] - internal-communication.shared-secret=<shared-secret-value> - internal-communication.jwt.expiration-seconds=<value in seconds> A new external dependency jet-cpp from the https://github.com/Thalhammer/jwt-cpp repo is added to a new setup-adapter.sh script for Prestissimo. In the dependency directory run "<path to presto-native-execution>/scripts/setup-adapter.sh jwt" to install it The jwt-cpp project handles the creation/parsing/verification of the JWT. A new build options are added to the build environment: PRESTO_ENABLE_JWT - default off, if on adds jet creation and verification capability, turned on during the pipeline build.

Adding creation and verification of a JWT signed using HMAC SHA-256. The JWT is used for authentication of internal communication. On the Java side PR prestodb#19706 added authentication for internal communication using a JWT token signed with HMAC SHA-256. Adding this comment enables Prestissimo to to be configured to verify internal communication requests, e.g from a Java coordinator. New system configuration options: - internal-communication.jwt.enabled=[true/false] - internal-communication.shared-secret=<shared-secret-value> - internal-communication.jwt.expiration-seconds=<value in seconds> A new external dependency jet-cpp from the https://github.com/Thalhammer/jwt-cpp repo is added to a new setup-adapter.sh script for Prestissimo. In the dependency directory run "<path to presto-native-execution>/scripts/setup-adapter.sh jwt" to install it The jwt-cpp project handles the creation/parsing/verification of the JWT. A new build options are added to the build environment: PRESTO_ENABLE_JWT - default off, if on adds jet creation and verification capability, turned on during the pipeline build.

nmahadevuni requested a review from a team as a code owner May 19, 2023 09:51

nmahadevuni requested a review from presto-oss May 19, 2023 09:51

nmahadevuni force-pushed the internal_communication_auth_fix branch 3 times, most recently from 55c39ef to 7cf685a Compare May 20, 2023 12:55

nmahadevuni changed the title ~~[WIP]: Add JWT for internal communication~~ Add JWT for internal communication May 20, 2023

tdcmeehan requested changes May 22, 2023

View reviewed changes

tdcmeehan self-assigned this May 22, 2023

ethanyzhang linked an issue May 22, 2023 that may be closed by this pull request

When we enable internal communication TLS and custom AUTH together presto is failing #19235

Closed

pranjalssh reviewed May 22, 2023

View reviewed changes

ajaygeorge reviewed May 23, 2023

View reviewed changes

nmahadevuni force-pushed the internal_communication_auth_fix branch 2 times, most recently from 62550c2 to 3c5f1fd Compare May 26, 2023 04:38

skairali approved these changes Jun 9, 2023

View reviewed changes

czentgr mentioned this pull request Jun 12, 2023

[native] Add JWT for internal communication #19861

Closed

tdcmeehan approved these changes Jun 13, 2023

View reviewed changes

tdcmeehan requested review from ajaygeorge and pranjalssh June 13, 2023 14:46

ajaygeorge requested a review from zacw7 June 15, 2023 17:44

zacw7 reviewed Jun 15, 2023

View reviewed changes

wanglinsong mentioned this pull request Jul 27, 2023

Add release notes for 0.283 #20402

Merged

28 tasks

	httpClientBinder(binder).bindHttpClient("ranger", ForRangerInfo.class)
	.withConfigDefaults(config -> config.setAuthenticationEnabled(false))
	.withFilter(RangerBasicAuthHttpRequestFilter.class);
	}

Conversation

nmahadevuni commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented May 22, 2023

Uh oh!

ethanyzhang commented May 22, 2023

Uh oh!

amitkdutta commented May 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmahadevuni commented May 22, 2023

Uh oh!

amitkdutta commented May 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaygeorge May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmahadevuni May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmahadevuni commented May 25, 2023

Uh oh!

skairali left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented Jun 12, 2023

Uh oh!

nmahadevuni commented Jun 13, 2023

Uh oh!

tdcmeehan commented Jun 15, 2023

Uh oh!

zacw7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nmahadevuni commented May 19, 2023 •

edited

Loading

ajaygeorge May 23, 2023 •

edited

Loading

nmahadevuni May 25, 2023 •

edited

Loading