github-actions[bot] commented on code in PR #65971:
URL: https://github.com/apache/doris/pull/65971#discussion_r3638541830
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/AuthenticatorManager.java:
##########
@@ -163,6 +169,23 @@ public boolean authenticate(ConnectContext context,
MysqlHandshakePacket handshakePacket) throws
IOException {
String remoteIp = context.getMysqlChannel().getRemoteIp();
+ CertificateAuthDecision certDecision =
CERT_RUNTIME_AUTH_SERVICE.authenticateLive(
+ userName, remoteIp, channel.getClientCertificate());
+ if (certDecision.isReject()) {
+ context.getState().setError(ErrorCode.ERR_ACCESS_DENIED_ERROR,
+ certDecision.getErrorMessage() == null ? "TLS certificate
verification failed"
+ : certDecision.getErrorMessage());
+ MysqlProto.sendResponsePacket(context);
+ return false;
+ }
+ if (certDecision.shouldSkipPasswordVerification()) {
Review Comment:
Returning here skips more than the password comparison: it also skips the
only `PasswordPolicyManager.checkAccountLockedAndPasswordExpiration()` call,
which lives inside `UserManager`'s password-success paths. The same early
return appears in HTTP and both forwarded load-auth paths, so enabling
`tls_cert_based_auth_ignore_password` lets a valid certificate authenticate a
locked or expired account. Please add a password-independent
identity/account-state check and run it before every certificate-only success;
the option should bypass password verification, not account policy.
##########
be/src/service/http/utils.cpp:
##########
@@ -119,6 +119,14 @@ bool parse_basic_auth(const HttpRequest& req, AuthInfo*
auth) {
// set user ip
auth->user_ip.assign(req.remote_host() != nullptr ? req.remote_host() :
"");
+ auth->cert_pem = req.header(HTTP_HEADER_CLIENT_CERT_PEM);
Review Comment:
The values copied here still come directly from the client's HTTP headers.
`HttpRequest::init_from_evhttp()` accepts every inbound header, and
`set_request_auth()` later serializes any nonempty subject/SAN as
`TCertBasedAuth`; FE has no way to distinguish that from metadata produced by a
verified TLS session. In particular, a build with HTTP excluded from unified
TLS can receive these headers over plaintext and use them in
`authenticateForwarded`, so a client can assert the SAN of a
certificate-restricted user. Please strip these names at the public HTTP
boundary and populate forwarded certificate identity only from the TLS session
(or a cryptographically authenticated BE assertion), rejecting it entirely on
plaintext/excluded routes.
##########
fe/fe-core/src/main/java/org/apache/doris/auth/certificate/SanEntryCodec.java:
##########
@@ -0,0 +1,145 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.auth.certificate;
+
+import com.google.common.collect.ImmutableMap;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+
+public final class SanEntryCodec {
+ private static final Map<String, String> CANONICAL_TYPES =
ImmutableMap.<String, String>builder()
+ .put("EMAIL", "email")
+ .put("RFC822", "email")
+ .put("DNS", "DNS")
+ .put("URI", "URI")
+ .put("IP", "IP Address")
+ .put("IP ADDRESS", "IP Address")
+ .build();
+
+ private SanEntryCodec() {
+ }
+
+ public static List<String> parseAndNormalize(String raw) {
+ if (raw == null || raw.trim().isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ Set<String> entries = new LinkedHashSet<>();
+ for (String part : raw.split(",", -1)) {
+ String normalized = normalizeEntry(part);
+ if (normalized.isEmpty()) {
+ throw invalidEntry(part);
+ }
+ entries.add(normalized);
+ }
+ return new ArrayList<>(entries);
+ }
+
+ public static boolean containsAll(Collection<String> requiredEntries,
Collection<String> presentedEntries) {
+ if (requiredEntries == null || presentedEntries == null
+ || requiredEntries.isEmpty() || presentedEntries.isEmpty()) {
+ return false;
+ }
+ Set<String> normalizedPresented = new LinkedHashSet<>();
+ for (String presented : presentedEntries) {
+ String normalized = normalizeEntry(presented);
+ if (!normalized.isEmpty()) {
+ normalizedPresented.add(normalized);
+ }
+ }
+ for (String required : requiredEntries) {
+ String normalizedRequired = normalizeEntry(required);
+ if (normalizedRequired.isEmpty() ||
!normalizedPresented.contains(normalizedRequired)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ public static String toSqlString(Collection<String> entries) {
+ if (entries == null || entries.isEmpty()) {
+ return "";
+ }
+ return String.join(", ", entries);
+ }
+
+ public static String normalizeEntry(String entry) {
+ if (entry == null) {
+ return "";
+ }
+ String trimmed = entry.trim();
+ if (trimmed.isEmpty()) {
+ return "";
+ }
+
+ int colon = trimmed.indexOf(':');
+ if (colon <= 0 || colon == trimmed.length() - 1) {
+ return "";
+ }
+
+ String rawType = trimmed.substring(0,
colon).trim().toUpperCase(Locale.ROOT);
+ String canonicalType = CANONICAL_TYPES.get(rawType);
+ if (canonicalType == null) {
+ return "";
+ }
+
+ String value = trimmed.substring(colon + 1).trim();
+ if (value.isEmpty()) {
+ return "";
+ }
+ while (value.endsWith(".")) {
Review Comment:
This applies one textual rule to every SAN type and then `containsAll()`
exact-compares the result. It therefore rejects valid equivalents such as
`DNS:Example.com` versus `DNS:example.com` (and compressed versus expanded
IPv6), while stripping `.` from case-sensitive URI values can make two distinct
identities compare equal. Please canonicalize by SAN type—DNS/IDNA case
handling, parsed address bytes for IP, and component-aware URI/email rules—and
add both equivalence and non-equivalence tests.
##########
build.sh:
##########
@@ -523,16 +620,35 @@ if [[ "${BUILD_BE_JAVA_EXTENSIONS}" -eq 1 &&
"${TARGET_SYSTEM}" == 'Darwin' ]];
fi
fi
-if [[ -z "${WITH_TDE_DIR}" ]]; then
- WITH_TDE_DIR=''
-fi
if [[ -z "${ENABLE_VARIANT_NESTED_GROUP}" ]]; then
ENABLE_VARIANT_NESTED_GROUP='OFF'
fi
if [[ -z "${VARIANT_NESTED_GROUP_MODULE_DIR}" ]]; then
VARIANT_NESTED_GROUP_MODULE_DIR=''
fi
+EXTRA_FE_MODULES="${EXTRA_FE_MODULES:-}"
+EXTRA_BE_MODULES="${EXTRA_BE_MODULES:-}"
+EXTRA_CLOUD_MODULES="${EXTRA_CLOUD_MODULES:-}"
+
+parse_extra_modules "FE_EXTRA" "${EXTRA_FE_MODULES}" "${DORIS_HOME}/fe" "fe"
+parse_extra_modules "BE_EXTRA" "${EXTRA_BE_MODULES}" "${DORIS_HOME}/be/src"
"be"
+parse_extra_modules "CLOUD_EXTRA" "${EXTRA_CLOUD_MODULES}"
"${DORIS_HOME}/cloud/src" "cloud"
+
+BE_EXTRA_CMAKE_ARGS=()
+for ((i = 0; i < ${#BE_EXTRA_FEATURE_KEYS[@]}; i++)); do
+ feature_name="$(feature_to_cmake_name "${BE_EXTRA_FEATURE_KEYS[i]}")"
+ BE_EXTRA_CMAKE_ARGS+=("-DENABLE_${feature_name}=ON")
Review Comment:
These arguments only turn features on. If a caller configures this reused
build directory with `EXTRA_BE_MODULES=tls=...` and later reruns with an empty
list, CMake keeps both cached `ENABLE_TLS=ON` and `TLS_MODULE_DIR`, so the
supposedly default build still compiles/packages the old TLS module. The cloud
and UT loops have the same behavior. Please make the feature set part of the
build-directory identity or explicitly clear/disable features removed since the
previous configure; a configure-twice regression test would catch this.
##########
be/src/service/server/oss/be_server_starter_factory.cpp:
##########
@@ -0,0 +1,317 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "service/server/be_server_starter_factory.h"
+
+#include <brpc/server.h>
+#include <brpc/ssl_options.h>
+#include <butil/endpoint.h>
+#include <gen_cpp/BackendService.h>
+#include <gen_cpp/HeartbeatService.h>
+// IWYU pragma: no_include <bthread/errno.h>
+#include <errno.h> // IWYU pragma: keep
+#include <gflags/gflags_declare.h>
+#include <string.h>
+
+#include <algorithm>
+#include <memory>
+
+#include "agent/heartbeat_server.h"
+#include "cloud/cloud_internal_service.h"
+#include "cloud/config.h"
+#include "common/config.h"
+#include "common/logging.h"
+#include "runtime/exec_env.h"
+#include "service/arrow_flight/flight_sql_service.h"
+#include "service/backend_options.h"
+#include "service/backend_service.h"
+#include "service/http_service.h"
+#include "service/internal_service.h"
+#include "storage/storage_engine.h"
+#include "util/mem_info.h"
+#include "util/thrift_server.h"
+
+namespace brpc {
+DECLARE_uint64(max_body_size);
+DECLARE_int64(socket_max_unwritten_bytes);
+DECLARE_bool(usercode_in_pthread);
+} // namespace brpc
+
+namespace doris::server {
+namespace {
+
+class OssBrpcServiceStarter final : public IServerStarter {
+public:
+ OssBrpcServiceStarter(ExecEnv* env, int port, int num_threads)
+ : _env(env),
+ _server(std::make_unique<brpc::Server>()),
+ _port(port),
+ _num_threads(num_threads) {
+ if (config::brpc_usercode_in_pthread) {
+ brpc::FLAGS_usercode_in_pthread = true;
+ }
+ brpc::FLAGS_max_body_size = config::brpc_max_body_size;
+ brpc::FLAGS_socket_max_unwritten_bytes =
+ config::brpc_socket_max_unwritten_bytes != -1
+ ? config::brpc_socket_max_unwritten_bytes
+ : std::max((int64_t)1073741824, (MemInfo::mem_limit()
/ 1024) * 20);
+ }
+
+ ~OssBrpcServiceStarter() override { join(); }
+
+ Status start() override {
+ if (config::enable_tls) {
+ return Status::NotSupported("BE BRPC TLS requires TLS module");
+ }
+
+ if (config::is_cloud_mode()) {
+ _server->AddService(
+ new
CloudInternalServiceImpl(_env->storage_engine().to_cloud(), _env),
+ brpc::SERVER_OWNS_SERVICE);
+ } else {
+ _server->AddService(new
PInternalServiceImpl(_env->storage_engine().to_local(), _env),
+ brpc::SERVER_OWNS_SERVICE);
+ }
+
+ brpc::ServerOptions options;
+ if (_num_threads != -1) {
+ options.num_threads = _num_threads;
+ }
+ options.idle_timeout_sec = config::brpc_idle_timeout_sec;
+ if (config::enable_https) {
+ auto* ssl_options = options.mutable_ssl_options();
+ ssl_options->default_cert.certificate =
config::ssl_certificate_path;
+ ssl_options->default_cert.private_key =
config::ssl_private_key_path;
+ }
+ options.has_builtin_services = config::enable_brpc_builtin_services;
+
+ butil::EndPoint point;
+ if (butil::str2endpoint(BackendOptions::get_service_bind_address(),
_port, &point) < 0) {
+ return Status::InternalError("convert address failed, host={},
port={}", "[::0]",
+ _port);
+ }
+ LOG(INFO) << "BRPC server bind to host: " <<
BackendOptions::get_service_bind_address()
+ << ", port: " << _port;
+ if (_server->Start(point, &options) != 0) {
+ char buf[64];
+ LOG(WARNING) << "start brpc failed, errno=" << errno
+ << ", errmsg=" << strerror_r(errno, buf, 64) << ",
port=" << _port;
+ return Status::InternalError("start brpc service failed");
+ }
+ return Status::OK();
+ }
+
+ void stop() override {
+ if (_server != nullptr) {
+ static_cast<void>(_server->Stop(1000));
Review Comment:
Discarding this result makes `_stop_requested` mean “stop succeeded.” Normal
shutdown calls `stop()` and then `join()`, where the flag produces a synthetic
zero and invokes `Server::Join()` even if this `Stop(1000)` failed. The deleted
`BRpcService::join()` deliberately skipped `Join()` on that failure because it
can never return. Please retain the actual stop result and only join after a
successful stop.
##########
be/src/util/brpc_client_cache.h:
##########
@@ -246,6 +247,10 @@ class BrpcClientCache {
const std::string&
connection_type = "",
const std::string&
connection_group = "") {
brpc::ChannelOptions options;
+ Status status =
doris::client::configure_brpc_channel_options(&options);
+ if (!status.ok()) {
+ throw status;
Review Comment:
A provider is allowed to return a non-OK `Status`, but this throws the plain
`doris::Status` object through an API whose callers only handle a null client.
There is no `catch(Status)` in BE/cloud, and `Status` is not a
`std::exception`, so a certificate/configuration error can escape a query or
request worker instead of following the existing channel failure path. Please
propagate `Status` explicitly (for example with `StatusOr`) or return null
consistently with `Channel::Init()` failures.
##########
build.sh:
##########
@@ -523,16 +620,35 @@ if [[ "${BUILD_BE_JAVA_EXTENSIONS}" -eq 1 &&
"${TARGET_SYSTEM}" == 'Darwin' ]];
fi
fi
-if [[ -z "${WITH_TDE_DIR}" ]]; then
- WITH_TDE_DIR=''
-fi
if [[ -z "${ENABLE_VARIANT_NESTED_GROUP}" ]]; then
ENABLE_VARIANT_NESTED_GROUP='OFF'
fi
if [[ -z "${VARIANT_NESTED_GROUP_MODULE_DIR}" ]]; then
VARIANT_NESTED_GROUP_MODULE_DIR=''
fi
+EXTRA_FE_MODULES="${EXTRA_FE_MODULES:-}"
+EXTRA_BE_MODULES="${EXTRA_BE_MODULES:-}"
Review Comment:
This replacement silently drops the existing `WITH_TDE_DIR` interface. A
release build environment that still exports only that variable now succeeds
with no TDE FE module and no `ENABLE_TDE`, producing an unencrypted OSS
artifact instead of an explicit migration error. Please translate
`WITH_TDE_DIR` to the new FE/BE module entries during a compatibility window,
or fail loudly when it is set; the same handling is needed in `run-be-ut.sh`.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/AuthenticatorManager.java:
##########
@@ -178,6 +201,9 @@ public boolean authenticate(ConnectContext context,
}
AuthenticateRequest request = primaryRequest.get();
+ if (preferredUserIdentity != null) {
Review Comment:
`preferredUserIdentity` is only a hint on the request:
`DefaultAuthenticator` consumes it, but LDAP, plugin, and integration
authenticators independently choose the first matching/JIT identity, and
`finishSuccessfulAuthentication()` installs that response without checking it
against the certificate decision. A certificate verified for one
host/SAN-constrained account can therefore finish login as another account with
different roles. Please make the certificate identity a hard binding across
primary and fallback authenticators and reject mismatched responses.
##########
fe/fe-core/src/main/java/org/apache/doris/auth/certificate/CertificateRuntimeAuthFactory.java:
##########
@@ -0,0 +1,59 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.auth.certificate;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.Iterator;
+import java.util.ServiceLoader;
+
+public final class CertificateRuntimeAuthFactory {
+ private static final Logger LOG =
LogManager.getLogger(CertificateRuntimeAuthFactory.class);
+ private static volatile CertificateRuntimeAuthService service;
+
+ private CertificateRuntimeAuthFactory() {
+ }
+
+ public static CertificateRuntimeAuthService getInstance() {
+ if (service != null) {
+ return service;
+ }
+ synchronized (CertificateRuntimeAuthFactory.class) {
+ if (service != null) {
+ return service;
+ }
+ ServiceLoader<CertificateRuntimeAuthService> loader =
+ ServiceLoader.load(CertificateRuntimeAuthService.class);
+ Iterator<CertificateRuntimeAuthService> iterator =
loader.iterator();
+ if (iterator.hasNext()) {
+ service = iterator.next();
+ LOG.info("Using CertificateRuntimeAuthService implementation:
{}", service.getClass().getName());
+ } else {
+ LOG.info("No customized CertificateRuntimeAuthService found,
fallback to no-op implementation");
Review Comment:
Falling back to this no-op service fails open:
`NoOpCertificateRuntimeAuthService` returns `NOT_APPLICABLE`, which every
caller treats as permission to continue password-only authentication—even for
an identity stored with `REQUIRE SAN`. A missing/mispackaged service descriptor
thus silently removes the account's certificate factor. Please validate the
required provider set at startup or, at minimum, fail closed for
certificate-restricted identities when the runtime verifier is absent.
##########
fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java:
##########
@@ -1296,8 +1356,8 @@ private TLoadTxnBeginResult
loadTxnBeginImpl(TLoadTxnBeginRequest request, Strin
// TODO: deprecated, removed in 3.1, use token instead.
} else if (Strings.isNullOrEmpty(request.getToken())) {
checkSingleTablePasswordAndPrivs(request.getUser(),
request.getPasswd(), request.getDb(),
- request.getTbl(),
- request.getUserIp(), PrivPredicate.LOAD);
+ request.getTbl(), request.getUserIp(), PrivPredicate.LOAD,
+ toForwardedCertificateInfo(request.getCertBasedAuth()));
Review Comment:
The certificate identity is now forwarded for begin, commit/pre-commit, and
2PC, but `TLoadTxnRollbackRequest` still has no `cert_based_auth`. A
certificate-only load can therefore open a transaction and then fail password
authentication on the error rollback path; BE also ignores the returned
rollback status, leaving the transaction open until cleanup. Please carry the
same certificate-bound identity through rollback and check/report its
application status, with a test that forces a certificate-only load failure.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/controller/BaseController.java:
##########
@@ -251,12 +257,22 @@ protected void checkTblAuth(UserIdentity currentUser,
String catalog, String db,
}
// return currentUserIdentity from Doris auth
- protected UserIdentity checkPassword(ActionAuthorizationInfo authInfo)
+ protected UserIdentity checkPassword(ActionAuthorizationInfo authInfo,
HttpServletRequest request)
throws UnauthorizedException {
+ CertificateAuthDecision certDecision = tryCertificateAuth(authInfo,
request);
Review Comment:
This can accept a certificate-only request on a follower, but several REST
actions then call `forwardToMaster()`. That helper copies only HTTP headers
into a new `RestTemplate` request; the servlet X.509 certificate attribute used
here is not transferable, so the master authenticates the request again without
the certificate and rejects a user whose password was intentionally skipped.
Please forward a trusted, identity-bound authentication assertion (or route
before authenticating) and add a certificate-only follower-to-master test.
##########
fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java:
##########
@@ -1248,6 +1299,15 @@ public TLoadTxnBeginResult
loadTxnBegin(TLoadTxnBeginRequest request) throws TEx
if (LOG.isDebugEnabled()) {
LOG.debug("receive txn begin request: {}, backend: {}", request,
clientAddr);
}
+ if (request.isSetCertBasedAuth()) {
+ TCertBasedAuth certAuth = request.getCertBasedAuth();
+ LOG.info("loadTxnBegin forwarded cert auth: san={}, subject={},
issuer={}",
Review Comment:
This adds one INFO record for every `loadTxnBegin`, including the normal
no-certificate case at line 1309; the surrounding request trace is debug-only
for a reason. On stream-load-heavy clusters this will flood/rotate FE logs, and
when certificates are present it also exposes SAN/subject/issuer identity
metadata at INFO. Please keep this at debug (or log only exceptional
verification failures with rate limiting).
##########
fe/fe-core/src/main/java/org/apache/doris/DorisFE.java:
##########
@@ -250,7 +266,8 @@ public static void start(String dorisHomeDir, String
pidDir, String[] args, Star
httpServer.setKeyStorePassword(Config.key_store_password);
httpServer.setKeyStoreType(Config.key_store_type);
httpServer.setKeyStoreAlias(Config.key_store_alias);
- httpServer.setEnableHttps(Config.enable_https);
+ httpServer.setEnableHttps(Config.enable_https
Review Comment:
In the unified HTTP TLS configuration this deliberately leaves
`Config.enable_https` false while the feature provider supplies the secure
connector. `BaseController.addSession()` and `updateCookieAge()` still derive
the auth cookie's `Secure` flag only from `Config.enable_https`, so sessions
created over unified TLS are issued as non-Secure and may be sent on a
plaintext request to the same host. Please derive cookie security from the
actual request/connector (and update the remaining scheme helpers) rather than
the legacy switch.
##########
be/src/util/thrift_client.cpp:
##########
@@ -21,14 +21,25 @@
#include <thrift/transport/TTransportException.h>
// IWYU pragma: no_include <bits/chrono.h>
#include <chrono> // IWYU pragma: keep
+#include <memory>
#include <string>
#include <thread>
#include "absl/strings/substitute.h"
+#include "util/client_connection_provider.h"
namespace doris {
+ThriftClientImpl::ThriftClientImpl(const std::string& ipaddress, int port)
+ : _ipaddress(ipaddress),
+ _port(port),
+ _socket(doris::client::create_thrift_client_socket(ipaddress, port))
{}
+
Status ThriftClientImpl::open() {
+ if (!_socket) {
Review Comment:
This null check cannot protect the normal cached-client path.
`ClientCacheHelper::_create_client()` calls `client_impl->set_conn_timeout()`
before `open_with_retry()`, and that setter dereferences `_socket`
unconditionally. If a TLS provider reports socket/credential setup failure by
returning null—as this new branch anticipates—the process segfaults before
reaching this `RpcError`. Please propagate a status from socket creation before
constructing/caching the client, or make every pre-open socket operation
null-safe.
##########
cloud/CMakeLists.txt:
##########
@@ -420,11 +432,11 @@ set(DORIS_CLOUD_LIBS
Snapshot
)
-if (EXISTS ${SRC_DIR}/enterprise/CMakeLists.txt)
- add_subdirectory(${SRC_DIR}/enterprise)
- if (DORIS_ADDITIONAL_DEFINITIONS)
- add_definitions(${DORIS_ADDITIONAL_DEFINITIONS})
- endif()
+if (ENABLE_SNAPSHOT)
Review Comment:
This removes the previous auto-discovery contract: an enterprise checkout
containing `cloud/src/enterprise/CMakeLists.txt` used to compile it
automatically (and `cloud/test/enterprise` likewise). With `ENABLE_SNAPSHOT`
defaulting OFF, the same checkout/build now succeeds as an OSS cloud binary and
silently omits both implementation and tests unless every caller has migrated
to `EXTRA_CLOUD_MODULES`. Please preserve the old directory probe as a
compatibility shim or fail loudly when the enterprise tree is present without
the new feature entry.
##########
fe/fe-core/src/main/java/org/apache/doris/auth/certificate/SanEntryCodec.java:
##########
@@ -0,0 +1,145 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.auth.certificate;
+
+import com.google.common.collect.ImmutableMap;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+
+public final class SanEntryCodec {
+ private static final Map<String, String> CANONICAL_TYPES =
ImmutableMap.<String, String>builder()
+ .put("EMAIL", "email")
+ .put("RFC822", "email")
+ .put("DNS", "DNS")
+ .put("URI", "URI")
+ .put("IP", "IP Address")
+ .put("IP ADDRESS", "IP Address")
+ .build();
+
+ private SanEntryCodec() {
+ }
+
+ public static List<String> parseAndNormalize(String raw) {
+ if (raw == null || raw.trim().isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ Set<String> entries = new LinkedHashSet<>();
+ for (String part : raw.split(",", -1)) {
Review Comment:
Splitting unconditionally on `,` makes the supported URI SAN type lossy
because comma is a valid URI sub-delimiter. For example,
`URI:spiffe://trust/workload,a` becomes one truncated URI plus an invalid
untyped entry, and `toSqlString()` uses the same unescaped delimiter for round
trips. Please use a structured list representation or define a shared escaping
grammar across SQL, persistence, SHOW, and forwarded certificate decoding, with
a comma-bearing URI regression test.
##########
build.sh:
##########
@@ -140,6 +142,101 @@ function copy_common_files() {
cp -r -p "${DORIS_HOME}/dist/licenses" "$1/"
}
+trim_whitespace() {
+ local value="$1"
+ value="${value#"${value%%[![:space:]]*}"}"
+ value="${value%"${value##*[![:space:]]}"}"
+ printf '%s' "${value}"
+}
+
+is_valid_extra_module_feature() {
+ local feature="$1"
+ [[ "${feature}" =~ ^[A-Za-z][A-Za-z0-9_-]*$ ]]
+}
+
+feature_to_cmake_name() {
+ local feature="$1"
+ printf '%s' "${feature}" | tr '[:lower:]-' '[:upper:]_'
+}
+
+parse_extra_modules() {
+ local array_prefix="$1"
+ local spec_value="$2"
+ local base_dir="$3"
+ local module_type="$4"
+ local entry feature module_path existing
+ local -a feature_keys=()
+ local -a module_paths=()
+
+ if [[ -z "${spec_value}" ]]; then
+ eval "${array_prefix}_FEATURE_KEYS=()"
+ eval "${array_prefix}_MODULE_PATHS=()"
+ return
+ fi
+
+ IFS=',' read -r -a entries <<<"${spec_value}"
+ for entry in "${entries[@]}"; do
+ entry="$(trim_whitespace "${entry}")"
+ if [[ -z "${entry}" ]]; then
+ echo "Invalid ${array_prefix} module spec: empty entry"
+ exit 1
+ fi
+ if [[ "${entry}" != *=* ]]; then
+ echo "Invalid ${array_prefix} module spec '${entry}': expected
feature=module_path"
+ exit 1
+ fi
+
+ feature="${entry%%=*}"
+ module_path="${entry#*=}"
+ feature="$(trim_whitespace "${feature}")"
+ module_path="$(trim_whitespace "${module_path}")"
+
+ if [[ -z "${feature}" || -z "${module_path}" ]]; then
+ echo "Invalid ${array_prefix} module spec '${entry}': feature and
module_path must be non-empty"
+ exit 1
+ fi
+ if ! is_valid_extra_module_feature "${feature}"; then
+ echo "Invalid ${array_prefix} feature '${feature}': use letters,
digits, '-' or '_' and start with a letter"
+ exit 1
+ fi
+
+ for existing in "${feature_keys[@]}"; do
Review Comment:
Duplicate checking uses the raw case-sensitive key, but
`feature_to_cmake_name()` later uppercases and maps `-` to `_`. Consequently
`tls=first,TLS=second` (or `foo-bar=first,foo_bar=second`) passes this check
and emits the same CMake variables twice, silently selecting the last module.
Please canonicalize before storing/checking keys and reject collisions in this
parser and both UT-script copies.
##########
run-fe-ut.sh:
##########
@@ -114,15 +188,17 @@ if [[ "${RUN}" -eq 1 ]]; then
# sh run-fe-ut.sh --run
org.apache.doris.utframe.DemoTest#testCreateDbAndTable+test2
if [[ "${COVERAGE}" -eq 1 ]]; then
- "${MVN_CMD}" test jacoco:report -DfailIfNoTests=false -Dtest="$1"
+ "${MVN_CMD}" test jacoco:report -pl "${MVN_MODULES}" -am
-DfailIfNoTests=false -Dtest="$1"
else
- "${MVN_CMD}" test -Dcheckstyle.skip=true -DfailIfNoTests=false
-Dtest="$1"
+ "${MVN_CMD}" test -pl "${MVN_MODULES}" -am -Dcheckstyle.skip=true
-DfailIfNoTests=false \
+ -Dtest="$1"
fi
else
echo "Run Frontend UT"
if [[ "${COVERAGE}" -eq 1 ]]; then
- "${MVN_CMD}" test jacoco:report -DfailIfNoTests=false
-Dmaven.test.failure.ignore=true
+ "${MVN_CMD}" test jacoco:report -pl "${MVN_MODULES}" -am
-DfailIfNoTests=false \
+ -Dmaven.test.failure.ignore=true
else
- "${MVN_CMD}" test -Dcheckstyle.skip=true -DfailIfNoTests=false
+ "${MVN_CMD}" test -pl "${MVN_MODULES}" -am -Dcheckstyle.skip=true
-DfailIfNoTests=false
Review Comment:
The previous default ran `mvn test` for the whole FE reactor. `-pl
fe-common,fe-core -am` now selects only those projects and their upstream
dependencies, so independent/downstream suites such as `hive-udf`,
`be-java-extensions`, and the password/LDAP authentication plugin modules are
silently omitted whenever no extra module is requested. Please preserve the
full-reactor default (or explicitly select every existing module plus the
extras) so this generic-module change does not reduce baseline FE UT coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/TlsOptions.java:
##########
@@ -159,8 +160,15 @@ public void analyze() throws AnalysisException {
+ "are not supported in current version");
}
- if (san != null && san.isEmpty()) {
- throw new AnalysisException("SAN value cannot be empty");
+ if (san != null) {
+ if (san.trim().isEmpty()) {
+ throw new AnalysisException("SAN value cannot be empty");
+ }
+ try {
Review Comment:
Existing images/edit logs can contain values accepted by the old analyzer,
such as `REQUIRE SAN 'example.com'`. This new parser rejects them for lacking a
type, while `UserIdentity` still restores the raw persisted value; `SHOW CREATE
USER` then emits the unchanged string even though this version cannot parse it.
Please define a migration or explicit compatibility path for legacy untyped SAN
values and cover old image/edit-log plus SHOW/parse-again round trips.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
