Skip to content

[native] Add JWT processing when using http(s) comms #20290

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

Merged
majetideepak merged 1 commit into from
Oct 4, 2023
Merged

[native] Add JWT processing when using http(s) comms #20290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion .circleci/continue_config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,14 @@ jobs:
command: |
cd presto-native-execution
make velox-submodule
- run:
name: "Install JWT adapter dependency"
command: |
mkdir -p ${HOME}/adapter-deps/install
source /opt/rh/gcc-toolset-9/enable
set -xu
cd presto-native-execution
DEPENDENCY_DIR=${HOME}/adapter-deps PROMPT_ALWAYS_RESPOND=n ./scripts/setup-adapters.sh jwt
- run:
name: "Calculate merge-base date for CCache"
command: git show -s --format=%cd --date="format:%Y%m%d" $(git merge-base origin/master HEAD) | tee merge-base-date
Expand All @@ -102,6 +110,7 @@ jobs:
-DCMAKE_BUILD_TYPE=Debug \
-DPRESTO_ENABLE_PARQUET=ON \
-DPRESTO_ENABLE_REMOTE_FUNCTIONS=ON \
-DPRESTO_ENABLE_JWT=ON \
-DCMAKE_PREFIX_PATH=/usr/local \
-DCMAKE_CXX_COMPILER_LAUNCHER=ccache
ninja -C _build/debug -j 8
Expand Down Expand Up @@ -210,13 +219,14 @@ jobs:
cd presto-native-execution
make velox-submodule
- run:
name: "Install S3 adapter dependencies"
name: "Install all adapter dependencies"
command: |
mkdir -p ${HOME}/adapter-deps/install
source /opt/rh/gcc-toolset-9/enable
set -xu
cd presto-native-execution
DEPENDENCY_DIR=${HOME}/adapter-deps PROMPT_ALWAYS_RESPOND=n ./velox/scripts/setup-adapters.sh
DEPENDENCY_DIR=${HOME}/adapter-deps PROMPT_ALWAYS_RESPOND=n ./scripts/setup-adapters.sh jwt
- run:
name: "Build All"
command: |
Expand All @@ -232,6 +242,7 @@ jobs:
-DPRESTO_ENABLE_PARQUET=ON \
-DPRESTO_ENABLE_S3=ON \
-DPRESTO_ENABLE_REMOTE_FUNCTIONS=ON \
-DPRESTO_ENABLE_JWT=ON \
-DCMAKE_PREFIX_PATH=/usr/local \
-DCMAKE_CXX_COMPILER_LAUNCHER=ccache
ninja -C _build/release -j 8
Expand Down
25 changes: 23 additions & 2 deletions presto-docs/src/main/sphinx/develop/presto-native.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ HTTP endpoints related to tasks are registered to Proxygen in

* POST: v1/task: This processes a `TaskUpdateRequest`
* GET: v1/task: This returns a serialized `TaskInfo` (used for comprehensive
metrics, may be reported less frequently)
metrics, may be reported less frequently)
* GET: v1/task/status: This returns
a serialized `TaskStatus` (used for query progress tracking, must be reported
frequently)
Expand Down Expand Up @@ -104,5 +104,26 @@ The following properties allow the configuration of remote function execution:

The UDS (unix domain socket) path to communicate with a local remote
function server. If specified, takes precedence over
``remote-function-server.thrift.address`` and
``remote-function-server.thrift.address`` and
``remote-function-server.thrift.port``.

JWT authentication support
--------------------------

Prestissimo supports JWT authentication for internal communication.
For details on the generally supported parameters visit `JWT <../security/internal-communication.html#jwt>`_.

There is also an additional parameter:

``internal-communication.jwt.expiration-seconds``
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

* **Type** ``integer``
* **Default value:** ``300``

There is a time period between creating the JWT on the client
and verification by the server.
If the time period is less than or equal to the parameter value, the request
is valid.
If the time period exceeds the parameter value, the request is rejected as
authentication failure (HTTP 401).
6 changes: 6 additions & 0 deletions presto-native-execution/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ option(PRESTO_ENABLE_REMOTE_FUNCTIONS "Enable remote function support" OFF)

option(PRESTO_ENABLE_TESTING "Enable tests" ON)

option(PRESTO_ENABLE_JWT "Enable JWT (JSON Web Token) authentication" OFF)

# Set all Velox options below
add_compile_definitions(FOLLY_HAVE_INT128_T=1)

Expand Down Expand Up @@ -201,4 +203,8 @@ else()
add_definitions(-DVELOX_DISABLE_GOOGLETEST)
endif()

if(PRESTO_ENABLE_JWT)
add_compile_definitions(PRESTO_ENABLE_JWT)
endif()

add_subdirectory(presto_cpp)
2 changes: 2 additions & 0 deletions presto-native-execution/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ PRESTO_ENABLE_PARQUET ?= "OFF"
PRESTO_ENABLE_S3 ?= "OFF"
PRESTO_ENABLE_HDFS ?= "OFF"
PRESTO_ENABLE_REMOTE_FUNCTIONS ?= "OFF"
PRESTO_ENABLE_JWT ?= "OFF"
EXTRA_CMAKE_FLAGS ?= ""

CMAKE_FLAGS := -DTREAT_WARNINGS_AS_ERRORS=${TREAT_WARNINGS_AS_ERRORS}
Expand All @@ -35,6 +36,7 @@ CMAKE_FLAGS += -DPRESTO_ENABLE_PARQUET=$(PRESTO_ENABLE_PARQUET)
CMAKE_FLAGS += -DPRESTO_ENABLE_S3=$(PRESTO_ENABLE_S3)
CMAKE_FLAGS += -DPRESTO_ENABLE_HDFS=$(PRESTO_ENABLE_HDFS)
CMAKE_FLAGS += -DPRESTO_ENABLE_REMOTE_FUNCTIONS=$(PRESTO_ENABLE_REMOTE_FUNCTIONS)
CMAKE_FLAGS += -DPRESTO_ENABLE_JWT=$(PRESTO_ENABLE_JWT)

SHELL := /bin/bash

Expand Down
9 changes: 9 additions & 0 deletions presto-native-execution/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,15 @@ This dependency can be installed by running the script below from the

`./velox/scripts/setup-adapters.sh aws`

To enable JWT authentication support, set `PRESTO_ENABLE_JWT = "ON"` in
the environment.

JWT authentication support needs the [JWT CPP](https://github.com/Thalhammer/jwt-cpp) library.
This dependency can be installed by running the script below from the
`presto/presto-native-execution` directory.

`./scripts/setup-adapters.sh jwt`

* After installing the above dependencies, from the
`presto/presto-native-execution` directory, run `make`
* For development, use
Expand Down
15 changes: 15 additions & 0 deletions presto-native-execution/presto_cpp/main/PrestoServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "presto_cpp/main/http/HttpServer.h"
#include "presto_cpp/main/http/filters/AccessLogFilter.h"
#include "presto_cpp/main/http/filters/HttpEndpointLatencyFilter.h"
#include "presto_cpp/main/http/filters/InternalAuthenticationFilter.h"
#include "presto_cpp/main/http/filters/StatsFilter.h"
#include "presto_cpp/main/operators/BroadcastExchangeSource.h"
#include "presto_cpp/main/operators/BroadcastWrite.h"
Expand Down Expand Up @@ -163,6 +164,15 @@ void PrestoServer::run() {
clientCertAndKeyPath = optionalClientCertPath.value();
}

if (systemConfig->internalCommunicationJwtEnabled()) {
#ifndef PRESTO_ENABLE_JWT
VELOX_USER_FAIL("Internal JWT is enabled but not supported");
#endif
VELOX_USER_CHECK(
!(systemConfig->internalCommunicationSharedSecret().empty()),
"Internal JWT is enabled without a corresponding shared secret");
}

nodeVersion_ = systemConfig->prestoVersion();
httpExecThreads = systemConfig->httpExecThreads();
environment_ = nodeConfig->nodeEnvironment();
Expand Down Expand Up @@ -658,6 +668,11 @@ PrestoServer::getHttpServerFilters() {
httpServer_.get()));
}

// Always add the authentication filter to make sure the worker configuration
// is in line with the overall cluster configuration e.g. cannot have a worker
// without JWT enabled.
Copy link
Collaborator

@majetideepak majetideepak Sep 5, 2023

Choose a reason for hiding this comment

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

@nmahadevuni @tdcmeehan from the Java implementation https://github.com/prestodb/presto/pull/19706/files, we understood that the authentication filter is always added even if JWT is disabled. This is to check if the HTTP headers have the presto-internal-bearer and throw an error if JWT is disabled. Did we get this right?

Copy link
Collaborator

@majetideepak majetideepak Sep 6, 2023

Choose a reason for hiding this comment

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

@amitkdutta, We want to get your input on this as well. On the Java side, we always seem to add an authentication filter to catch misconfiguration. In this work, we are adding an authentication filter to Prestissimo that simply checks the HTTP headers if a token is set.
Any concerns?

Copy link
Member

@nmahadevuni nmahadevuni Sep 7, 2023

Choose a reason for hiding this comment

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

Authentication filter has a list of authenticators that will be applied to all incoming requests. The list of authenticators can be empty if we didn't configure any authentication though. This is not specific to JWT. There are other authenticators PASSWORD,CERTIFICATE etc., When JWT is enabled, the request will have the header "X-Presto-Internal-Bearer". In effect, this is also one of the authenticators. If none is present, then the request is passed through, but if one or more authenticators are present, at least one should be successful for each request to go through.

Copy link
Contributor Author

@czentgr czentgr Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

" If none is present, then the request is passed through"
I think this is a problem.

Looking at the Java code then it only validates internal requests if the presto bearer is set. If not, then it falls through - and if not further authenticators are available the request passes through and is processed.

(in doFilter

        if (internalAuthenticationManager.isInternalRequest(request)) { <--- presto bearer present or not
            Principal principal = internalAuthenticationManager.authenticateInternalRequest(request);
            if (principal == null) {
                response.sendError(SC_UNAUTHORIZED);
                return;
            }
            nextFilter.doFilter(withPrincipal(request, principal), response);
            return;
        }

)

As a result, even if the server is configured to validate (JWT) requests, an attacker sending a simple request without the bearer passes authentication. This doesn't seem right.

In the C++ implementation the bearer is required if worker has JWT authentication configured.
It also rejects a request if the bearer is present and the worker is not configured for JWT authentication.

This hardens the worker and cluster config. I think on the Java side there is a security leak.
We should add similar behavior to Java.

Copy link
Collaborator

@majetideepak majetideepak Sep 7, 2023

Choose a reason for hiding this comment

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

CC: @tdcmeehan

Copy link
Member

@nmahadevuni nmahadevuni Sep 11, 2023

Choose a reason for hiding this comment

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

The checks for bearer and JWT config can be hardened only on workers since they don't need to worry about external authentication. In coordinator(where it can a worker too), we cannot know upfront if the request is internal or external. So, if we harden these checks for internal JWT, then we will fail all external requests.

filters.push_back(
std::make_unique<http::filters::InternalAuthenticationFilterFactory>());
return filters;
}

Expand Down
18 changes: 18 additions & 0 deletions presto-native-execution/presto_cpp/main/common/Configs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,9 @@ SystemConfig::SystemConfig() {
NUM_PROP(kTaskRunTimeSliceMicros, 50'000),
BOOL_PROP(kIncludeNodeInSpillPath, false),
NUM_PROP(kOldTaskCleanUpMs, 60'000),
STR_PROP(kInternalCommunicationJwtEnabled, "false"),
STR_PROP(kInternalCommunicationSharedSecret, ""),
NUM_PROP(kInternalCommunicationJwtExpirationSeconds, 300),
};
}

Expand Down Expand Up @@ -558,6 +561,21 @@ int32_t SystemConfig::oldTaskCleanUpMs() const {
return optionalProperty<int32_t>(kOldTaskCleanUpMs).value();
}

// The next three toggles govern the use of JWT for authentication
// for communication between the cluster nodes.
bool SystemConfig::internalCommunicationJwtEnabled() const {
return optionalProperty<bool>(kInternalCommunicationJwtEnabled).value();
}

std::string SystemConfig::internalCommunicationSharedSecret() const {
return optionalProperty(kInternalCommunicationSharedSecret).value();
}

int32_t SystemConfig::internalCommunicationJwtExpirationSeconds() const {
return optionalProperty<int32_t>(kInternalCommunicationJwtExpirationSeconds)
.value();
}

NodeConfig::NodeConfig() {
registeredProps_ =
std::unordered_map<std::string, folly::Optional<std::string>>{
Expand Down
14 changes: 14 additions & 0 deletions presto-native-execution/presto_cpp/main/common/Configs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,14 @@ class SystemConfig : public ConfigBase {
static constexpr std::string_view kRemoteFunctionServerCatalogName{
"remote-function-server.catalog-name"};

/// Options to configure the internal (in-cluster) JWT authentication.
static constexpr std::string_view kInternalCommunicationJwtEnabled{
"internal-communication.jwt.enabled"};
static constexpr std::string_view kInternalCommunicationSharedSecret{
"internal-communication.shared-secret"};
static constexpr std::string_view kInternalCommunicationJwtExpirationSeconds{
"internal-communication.jwt.expiration-seconds"};

SystemConfig();

static SystemConfig* instance();
Expand Down Expand Up @@ -459,6 +467,12 @@ class SystemConfig : public ConfigBase {
bool includeNodeInSpillPath() const;

int32_t oldTaskCleanUpMs() const;

bool internalCommunicationJwtEnabled() const;

std::string internalCommunicationSharedSecret() const;

int32_t internalCommunicationJwtExpirationSeconds() const;
};

/// Provides access to node properties defined in node.properties file.
Expand Down
6 changes: 6 additions & 0 deletions presto-native-execution/presto_cpp/main/http/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
# limitations under the License.
add_library(presto_http HttpClient.cpp HttpServer.cpp)

if(PRESTO_ENABLE_JWT)
add_compile_definitions(JWT_DISABLE_PICOJSON)
target_include_directories(
presto_http PRIVATE ${CMAKE_SOURCE_DIR}/presto_cpp/external/json)
endif()

add_subdirectory(filters)

target_link_libraries(
Expand Down
34 changes: 33 additions & 1 deletion presto-native-execution/presto_cpp/main/http/HttpClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,12 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#ifdef PRESTO_ENABLE_JWT
#include <folly/ssl/OpenSSLHash.h> // @manual
#include <jwt-cpp/jwt.h> // @manual
#include <jwt-cpp/traits/nlohmann-json/traits.h> //@manual
#endif // PRESTO_ENABLE_JWT
#include <velox/common/base/Exceptions.h>

#include "presto_cpp/main/common/Configs.h"
#include "presto_cpp/main/http/HttpClient.h"

Expand Down Expand Up @@ -346,4 +350,32 @@ folly::SemiFuture<std::unique_ptr<HttpResponse>> HttpClient::sendRequest(
return future;
}

void RequestBuilder::addJwtIfConfigured() {
#ifdef PRESTO_ENABLE_JWT
if (SystemConfig::instance()->internalCommunicationJwtEnabled()) {
// If JWT was enabled the secret cannot be empty.
auto secretHash = std::vector<uint8_t>(SHA256_DIGEST_LENGTH);
folly::ssl::OpenSSLHash::sha256(
folly::range(secretHash),
folly::ByteRange(folly::StringPiece(
SystemConfig::instance()->internalCommunicationSharedSecret())));

const auto time = std::chrono::system_clock::now();
const auto token =
jwt::create<jwt::traits::nlohmann_json>()
.set_subject(NodeConfig::instance()->nodeId())
.set_issued_at(time)
.set_expires_at(
time +
std::chrono::seconds{
SystemConfig::instance()
->internalCommunicationJwtExpirationSeconds()})
.sign(jwt::algorithm::hs256{std::string(
reinterpret_cast<char*>(secretHash.data()),
secretHash.size())});
header(kPrestoInternalBearer, token);
}
#endif // PRESTO_ENABLE_JWT
}

} // namespace facebook::presto::http
3 changes: 3 additions & 0 deletions presto-native-execution/presto_cpp/main/http/HttpClient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,15 @@ class RequestBuilder {

folly::SemiFuture<std::unique_ptr<HttpResponse>>
send(HttpClient* client, const std::string& body = "", int64_t delayMs = 0) {
addJwtIfConfigured();
header(proxygen::HTTP_HEADER_CONTENT_LENGTH, std::to_string(body.size()));
headers_.ensureHostHeader();
return client->sendRequest(headers_, body, delayMs);
}

private:
void addJwtIfConfigured();

proxygen::HTTPMessage headers_;
};

Expand Down
2 changes: 2 additions & 0 deletions presto-native-execution/presto_cpp/main/http/HttpConstants.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ namespace facebook::presto::http {
const uint16_t kHttpOk = 200;
const uint16_t kHttpAccepted = 202;
const uint16_t kHttpNoContent = 204;
const uint16_t kHttpUnauthorized = 401;
const uint16_t kHttpNotFound = 404;
const uint16_t kHttpInternalServerError = 500;

const char kMimeTypeApplicationJson[] = "application/json";
const char kMimeTypeApplicationThrift[] = "application/x-thrift+binary";
static const char kPrestoInternalBearer[] = "X-Presto-Internal-Bearer";
} // namespace facebook::presto::http
7 changes: 6 additions & 1 deletion presto-native-execution/presto_cpp/main/http/filters/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@
# limitations under the License.

add_library(http_filters AccessLogFilter.cpp HttpEndpointLatencyFilter.cpp
StatsFilter.cpp)
InternalAuthenticationFilter.cpp StatsFilter.cpp)

if(PRESTO_ENABLE_JWT)
target_include_directories(
http_filters PRIVATE ${CMAKE_SOURCE_DIR}/presto_cpp/external/json)
endif()

target_link_libraries(http_filters presto_common ${PROXYGEN_LIBRARIES})

Expand Down
Loading