This is an automated email from the ASF dual-hosted git repository.
gavinchou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 2befeb38bc1 [improvement](log) Log the user name of HTTP requests for
auditing (#66745)
2befeb38bc1 is described below
commit 2befeb38bc1baeb789713ab937df08f482535030
Author: Xin Liao <[email protected]>
AuthorDate: Mon Aug 31 10:31:37 2026 +0800
[improvement](log) Log the user name of HTTP requests for auditing (#66745)
### What problem does this PR solve?
Problem Summary:
`HttpRequest::debug_string()` masks the whole `Authorization` header, so
the BE request log shows that a request carried credentials but not
whose, leaving operations such as `/api/update_config` without an audit
trail. Keep the user name of HTTP Basic credentials and mask only the
password, rendering `<user>:***MASKED***`. Every other sensitive header
(`token`, `auth-token`, `auth_code`, `proxy-authorization`), every
non-Basic scheme, and any credential that cannot be decoded stays masked
in full. What is emitted is a rendering, not the header value: the real
one is base64 encoded.
One more path wrote credentials to the logs in clear text and is fixed
as well: FE's `BaseController` logged the raw `Authorization` header,
that is `base64(user:password)`, at INFO level when parsing failed. This
is a parse failure rather than an authentication failure, so it runs
before the password is verified and the header often carries valid
credentials. Only whether the header was absent or malformed is logged
now.
Note: an earlier revision of this PR also rewrote the `permission
verification failed` log in `HttpHandlerWithAuth`, which streamed
`TCheckAuthRequest` and let the thrift-generated `printTo()` dump
`passwd`. #66618 has since landed a fix for that same leak, so this PR
was rebased onto it and no longer touches that file.
### Release note
None
### Check List (For Author)
- Test
- [x] Unit Test
- [x] Manual test (add detailed scripts or steps below)
`HttpRequestTest` covers the rendering of the `Authorization` header:
the user name is kept for Basic credentials (including a password
containing colons and a case insensitive scheme), while a malformed,
undecodable or non-Basic credential and every other sensitive header
stay masked in full.
Also verified on a single FE + BE cluster that no credential appears in
`fe.log`, `be.INFO` or `be.WARNING`.
- Behavior changed:
- [x] No.
- Does this need documentation?
- [x] No.
---
be/src/service/http/http_request.cpp | 32 ++++-
be/test/service/http/http_request_test.cpp | 142 +++++++++++++++++++++
.../doris/httpv2/controller/BaseController.java | 7 +-
3 files changed, 178 insertions(+), 3 deletions(-)
diff --git a/be/src/service/http/http_request.cpp
b/be/src/service/http/http_request.cpp
index e002e5986cb..e511ff577fd 100644
--- a/be/src/service/http/http_request.cpp
+++ b/be/src/service/http/http_request.cpp
@@ -32,6 +32,7 @@
#include "service/http/http_handler.h"
#include "service/http/http_headers.h"
#include "util/stack_util.h"
+#include "util/url_coding.h"
namespace doris {
@@ -44,6 +45,34 @@ static bool is_sensitive_header(const std::string&
header_name) {
iequal(header_name, HttpHeaders::AUTH_TOKEN) || iequal(header_name,
"auth_code");
}
+// Renders a sensitive header for logging. For HTTP Basic credentials the user
name is kept,
+// so that logs still answer "who issued this request", and only the password
is masked,
+// yielding "<user>:***MASKED***". Any other sensitive header, and any
credential we fail to
+// parse, is masked as a whole. The result is a rendering, not the header
value: the real one
+// is base64 encoded.
+static std::string mask_sensitive_header(const std::string& name, const
std::string& value) {
+ static const std::string kMasked = "***MASKED***";
+ if (!iequal(name, HttpHeaders::AUTHORIZATION)) {
+ return kMasked;
+ }
+
+ // Expected form: "Basic <base64(user:password)>"
+ auto pos = value.find(' ');
+ if (pos == std::string::npos || !iequal(value.substr(0, pos), "Basic")) {
+ return kMasked;
+ }
+ std::string decoded;
+ if (!base64_decode(value.substr(pos + 1), &decoded)) {
+ return kMasked;
+ }
+ // Note that the password may contain a colon, so split on the first one
only.
+ auto colon = decoded.find(':');
+ if (colon == std::string::npos) {
+ return kMasked;
+ }
+ return decoded.substr(0, colon) + ":" + kMasked;
+}
+
HttpRequest::HttpRequest(evhttp_request* evhttp_request) :
_ev_req(evhttp_request) {}
HttpRequest::~HttpRequest() {
@@ -96,7 +125,8 @@ std::string HttpRequest::debug_string() const {
<< "headers: \n";
for (auto& iter : _headers) {
if (is_sensitive_header(iter.first)) {
- ss << "key=" << iter.first << ", value=***MASKED***\n";
+ ss << "key=" << iter.first
+ << ", value=" << mask_sensitive_header(iter.first, iter.second)
<< "\n";
} else {
ss << "key=" << iter.first << ", value=" << iter.second << "\n";
}
diff --git a/be/test/service/http/http_request_test.cpp
b/be/test/service/http/http_request_test.cpp
new file mode 100644
index 00000000000..fdb9df43f75
--- /dev/null
+++ b/be/test/service/http/http_request_test.cpp
@@ -0,0 +1,142 @@
+// 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/http/http_request.h"
+
+#include <event2/http.h>
+#include <gtest/gtest.h>
+
+#include <cstring>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "service/http/http_headers.h"
+#include "util/url_coding.h"
+
+namespace doris {
+
+namespace {
+
+constexpr char kMasked[] = "***MASKED***";
+
+std::string basic_of(const std::string& credentials) {
+ std::string encoded;
+ base64_encode(credentials, &encoded);
+ return "Basic " + encoded;
+}
+
+// Renders debug_string() for a request carrying a single header.
+std::string debug_string_with_header(const std::string& name, const
std::string& value) {
+ auto* evhttp_req = evhttp_request_new(nullptr, nullptr);
+ HttpRequest req(evhttp_req);
+ req.set_header(name, value);
+ std::string dumped = req.debug_string();
+ evhttp_request_free(evhttp_req);
+ return dumped;
+}
+
+} // namespace
+
+class HttpRequestTest : public testing::Test {};
+
+// The user name is what makes a request attributable, so it is kept while the
password is not.
+TEST_F(HttpRequestTest, basic_auth_keeps_user_name) {
+ const std::string header = basic_of("root:Secret123");
+ const std::string dumped =
debug_string_with_header(HttpHeaders::AUTHORIZATION, header);
+
+ EXPECT_NE(dumped.find("key=Authorization, value=root:***MASKED***"),
std::string::npos)
+ << dumped;
+ EXPECT_EQ(dumped.find("Secret123"), std::string::npos) << dumped;
+ // The base64 blob decodes to the password, so it must not survive either.
+ EXPECT_EQ(dumped.find(header.substr(strlen("Basic "))), std::string::npos)
<< dumped;
+}
+
+// The password may contain colons, so only the first one separates it from
the user name.
+TEST_F(HttpRequestTest, basic_auth_password_containing_colons) {
+ const std::string dumped =
+ debug_string_with_header(HttpHeaders::AUTHORIZATION,
basic_of("admin:pa:ss:word"));
+
+ EXPECT_NE(dumped.find("key=Authorization, value=admin:***MASKED***"),
std::string::npos)
+ << dumped;
+ EXPECT_EQ(dumped.find("pa:ss:word"), std::string::npos) << dumped;
+}
+
+// RFC 7617 makes the scheme token case insensitive.
+TEST_F(HttpRequestTest, basic_auth_scheme_is_case_insensitive) {
+ std::string encoded;
+ base64_encode(std::string("alice:secret"), &encoded);
+
+ for (const std::string& scheme : {"Basic", "basic", "BASIC", "BaSiC"}) {
+ const std::string dumped =
+ debug_string_with_header(HttpHeaders::AUTHORIZATION, scheme +
" " + encoded);
+ EXPECT_NE(dumped.find("value=alice:***MASKED***"), std::string::npos)
+ << "scheme=" << scheme << ", dumped=" << dumped;
+ EXPECT_EQ(dumped.find("secret"), std::string::npos) << "scheme=" <<
scheme;
+ }
+}
+
+// Anything that is not parseable as Basic credentials is masked as a whole,
so a malformed
+// header can never leak the part that would have been the password.
+TEST_F(HttpRequestTest, unparseable_credentials_are_masked_entirely) {
+ std::string encoded;
+ base64_encode(std::string("alice:secret"), &encoded);
+
+ const std::vector<std::pair<std::string, std::string>> cases = {
+ // more than one space between the scheme and the credentials
+ {"two spaces", "Basic " + encoded},
+ // no scheme at all
+ {"no scheme", encoded},
+ // a scheme that does not carry user:password
+ {"bearer", "Bearer eyJhbGciOiJIUzI1NiJ9.payload"},
+ // not decodable
+ {"bad base64", "Basic !!!not-base64!!!"},
+ // decodes, but carries no colon to split on
+ {"no colon", basic_of("no-colon-here")},
+ // empty value
+ {"empty", ""},
+ };
+
+ for (const auto& [name, value] : cases) {
+ const std::string dumped =
debug_string_with_header(HttpHeaders::AUTHORIZATION, value);
+ EXPECT_NE(dumped.find(std::string("key=Authorization, value=") +
kMasked),
+ std::string::npos)
+ << "case=" << name << ", dumped=" << dumped;
+ EXPECT_EQ(dumped.find("secret"), std::string::npos) << "case=" << name;
+ EXPECT_EQ(dumped.find("payload"), std::string::npos) << "case=" <<
name;
+ }
+}
+
+// Only Basic credentials carry a user name; every other sensitive header
stays fully masked.
+TEST_F(HttpRequestTest, other_sensitive_headers_are_masked_entirely) {
+ for (const std::string& name :
+ {std::string("token"), std::string("auth_code"),
std::string(HttpHeaders::AUTH_TOKEN),
+ std::string(HttpHeaders::PROXY_AUTHORIZATION)}) {
+ const std::string dumped = debug_string_with_header(name,
"SUPERSECRET123");
+ EXPECT_NE(dumped.find(kMasked), std::string::npos) << "header=" <<
name << ", " << dumped;
+ EXPECT_EQ(dumped.find("SUPERSECRET123"), std::string::npos) <<
"header=" << name;
+ }
+}
+
+TEST_F(HttpRequestTest, non_sensitive_headers_are_untouched) {
+ const std::string dumped =
debug_string_with_header(HttpHeaders::USER_AGENT, "curl/7.76.1");
+
+ EXPECT_NE(dumped.find("key=User-Agent, value=curl/7.76.1"),
std::string::npos) << dumped;
+ EXPECT_EQ(dumped.find(kMasked), std::string::npos) << dumped;
+}
+
+} // namespace doris
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/httpv2/controller/BaseController.java
b/fe/fe-core/src/main/java/org/apache/doris/httpv2/controller/BaseController.java
index b5bb6294977..e4b1e383f41 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/httpv2/controller/BaseController.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/httpv2/controller/BaseController.java
@@ -340,8 +340,11 @@ public class BaseController {
throws UnauthorizedException {
ActionAuthorizationInfo authInfo = new ActionAuthorizationInfo();
if (!parseAuthInfo(request, authInfo)) {
- LOG.info("parse auth info failed, Authorization header {}, url {}",
- request.getHeader("Authorization"),
request.getRequestURI());
+ // Never log the Authorization header itself: it carries
base64(user:password),
+ // which is trivially decodable. Only record whether it was absent
or malformed.
+ LOG.info("parse auth info failed, Authorization header is {}, url
{}",
+ Strings.isNullOrEmpty(request.getHeader("Authorization"))
? "absent" : "malformed",
+ request.getRequestURI());
throw new UnauthorizedException("Need auth information.");
}
if (LOG.isDebugEnabled()) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
