-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[native] Add JWT processing when using http(s) comms #20290
Conversation
TODOs left
|
Test results from running HttpTest gtest.
|
324dfc8
to
0871a56
Compare
@aditi-pandit @majetideepak Please take a look when you get a chance. |
0871a56
to
c774be5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a quick read and have some very high level comments.
...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/external/jwt-cpp/traits/nlohmann-json/traits.h
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/external/jwt-cpp/traits/nlohmann-json/traits.h
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/external/jwt-cpp/traits/nlohmann-json/defaults.h
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/external/jwt-cpp/traits/nlohmann-json/defaults.h
Outdated
Show resolved
Hide resolved
|
||
// If the internal communication is not enabled the filter | ||
// is not added to process incoming requests. | ||
void JWTTokenFilter::onRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you need to add any runtime counters in this logic ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counter to count number of bad requests?
Not sure exactly what you mean. The Java code didn't seem to add counters so I didn't consider it.
presto-native-execution/presto_cpp/main/http/filters/JwtTokenFilter.cpp
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/main/http/filters/JwtTokenFilter.cpp
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/main/http/filters/JwtTokenFilter.cpp
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/main/http/filters/CMakeLists.txt
Outdated
Show resolved
Hide resolved
c774be5
to
a779518
Compare
The test appears to have run into a connection issue with hive when testing insertion into partitioned tables.
Two tests failed as a result. |
a779518
to
ed89762
Compare
@czentgr I looked at the https://github.com/Thalhammer/jwt-cpp repo and it looks like there are still some changes being merged. How often do we have to update this code? |
Do we know the overhead of JWT? |
No, probably would need to add benchmark tests to compare. Overhead is JSON parsing and generating the HS256 signature of the header/payload. It would think it is more or less negligible. The token are not that large and I assume the openssl implementation for the hashing is efficient. |
0.6.0 was the last release. The current one is (was) in release candidate stage (and has been for a while). Perhaps it updated now since I last checked. I was thinking of adding it to the setup scripts (and had ti running like that) but then saw the json module was also added as code. Edit: something like Velox does but using CMake to access github and build/install I guess would work. I tried to use that module in the prestissimo CMake and there were some issues with the macros (certain variables related to Velox need to be set). |
As discussed with @majetideepak
|
Trying to re-use the nlogman jsopn file is not possible. The jwt-cpp will look for
the file in the system path. Without changing the code it is not possible to use the already included file that resides in I guess there is a larger discussion on reuse of components and how they are accessed. Perhaps the nlohmann/json should also be a dependency. Dependencies could be installed via the setup scripts or the cmake files (that download and build the project). |
I wrote a small benchmark and the results show that the difference is negligible (10,000 requests). I ran multiple times in my local environment. The time seemed to vary occasionally quite a bit. Here are example outputs:
My environment is probably not the best to test at it is varying too much depending on what else is going on. |
ed89762
to
f24f4c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@czentgr final set of comments. Looks good overall. Can you also verify on a standalone cluster?
* **Type** ``integer`` | ||
* **Default value:** ``300`` | ||
|
||
There is a time period between ceating the JWT on the client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: creating
@@ -161,6 +162,15 @@ void PrestoServer::run() { | |||
"Https Client Certificates are not configured correctly"); | |||
} | |||
clientCertAndKeyPath = optionalClientCertPath.value(); | |||
|
|||
if (systemConfig->internalCommunicationJwtEnabled()) { | |||
#ifdef PRESTO_JWT_ENABLED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be #ifndef
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are correct.
|
||
if(PRESTO_ENABLE_JWT) | ||
target_include_directories( | ||
http_filters PUBLIC ${CMAKE_SOURCE_DIR}/presto_cpp/external/json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be PRIVATE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was private but then it needs another line in the upper cmake (for the http client). I tried to avoid that.
|
||
namespace facebook::presto::http::filters { | ||
|
||
/// If the internal communication is not enabled the filter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma after enabled If the internal communication is not enabled,
namespace facebook::presto::http::filters { | ||
|
||
/// If the internal communication is not enabled the filter | ||
/// is not added to process incoming requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process's
INSTANTIATE_TEST_CASE_P( | ||
HTTPJwtTest, | ||
HttpJwtTestSuite, | ||
::testing::Values(true, false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an empty newline at the end.
4c4446d
to
f37c65e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small formatting suggestions for the docs page presto-docs/src/main/sphinx/develop/presto-native.rst.
Also - and there may be a good reason for this - I wanted to ask why this page is not included in the Developer index in presto-docs/src/main/sphinx/develop.rst?
-------------------------- | ||
|
||
Prestissimo supports JWT authentication for internal communication. | ||
For details on the generally supported parameters visit :doc:`JWT</security/internal-communication>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For details on the generally supported parameters visit :doc:`JWT</security/internal-communication>`. | |
For details on the generally supported parameters visit `JWT </security/internal-communication.html#jwt>`_. |
Links directly to the JWT section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure the proposed change generates the desired link (it is changed to an external link). Locally, it creates file:///security/internal-communication.html#jwt
which is not correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting: I built locally with this proposed change (yesterday, and just now to retest after your comment) and the link works for me, opening the page at the JWT target.
If it doesn't work for you, please ignore this suggestion and I'll look into this later. Thanks for checking!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works
`JWT <../security/internal-communication.html#jwt>`_.
(relative path needed the .. to switch dirs properly).
I will make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, and thank you for letting me know what you found as this is useful elsewhere in the docs!
Thanks you @steveburnett . |
Thanks! I must not have pulled the latest merges for master when I asked that: the link on develop.rst that I wasn't seeing when I wrote my question is in place now. |
f37c65e
to
66c187b
Compare
@steveburnett would you please re-review the latest changes (based on your comments) and approve, if ok? Not sure if your requested changes block the progression. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (docs)
I don't think I block anything, honestly, but everything looks great to me now so I'm happy to approve from a docs perspective if it matters! Just did. |
66c187b
to
cb8cb16
Compare
auto rawSystemConfig = | ||
std::make_unique<core::MemConfigMutable>(initalSystemConfig); | ||
auto systemConfig = SystemConfig::instance(); | ||
systemConfig->initialize(std::move(rawSystemConfig)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should create a helper SystemConfig* jwtSystemConfig();
where we set the correct config. Probably reuse this inside basicJwtTest
.
We then only use the changedSystemConfig
to change the JWT config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably have a separate server and client config with override.
See https://github.com/facebookincubator/velox/blob/main/velox/connectors/hive/storage_adapters/s3fs/tests/MinioServer.h#L51
}); | ||
}; | ||
} | ||
} // namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@czentgr looks good overall. Made a couple of comments. I will merge this tomorrow if you can address the comments today.
@amitkdutta Let us know if you have any comments on this.
cb8cb16
to
c3b5514
Compare
{std::string(NodeConfig::kNodeId), std::string("testnode")}}; | ||
std::unique_ptr<Config> rawNodeConfig = | ||
std::make_unique<core::MemConfig>(nodeConfigValues); | ||
nodeConfig_->initialize(std::move(rawNodeConfig)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need the nodeConfig_
class variable.
return response; | ||
} | ||
|
||
const std::shared_ptr<MemoryPool> memoryPool_ = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only used once inside produceHttpResponse
. Let's remove this and get the defaultMemoryManager at the call site.
|
||
NodeConfig* nodeConfig_ = NodeConfig::instance(); | ||
|
||
const std::unordered_map<std::string, std::string> defaultSystemConfig_{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be part of jwtSystemConfig
as well.
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.
c3b5514
to
21b67a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing all the comments @czentgr
@tdcmeehan Can we get an approval from |
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:
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
"/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.
Test plan - (Please fill in how you tested your changes)
The PR adds a number of unit tests to the gtest HttpTest suite:
Resolves issue #19861.