chore(proxy): print request safety in proxy#1715
chore(proxy): print request safety in proxy#1715CooooolFrog wants to merge 6 commits intodragonflyoss:mainfrom
Conversation
CooooolFrog
force-pushed
the
main
branch
2 times, most recently
from
9d5f49f to
b344a49
Compare
CooooolFrog
force-pushed
the
main
branch
2 times, most recently
from
ac87ca5 to
b49fb44
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1715 +/- ##
==========================================
- Coverage 50.29% 50.21% -0.08%
==========================================
Files 86 86
Lines 21977 21998 +21
==========================================
- Hits 11053 11046 -7
- Misses 10924 10952 +28
🚀 New features to boost your workflow:
|
Signed-off-by: CooooolFrog <zuliangwanghust@gmail.com>
Signed-off-by: CooooolFrog <zuliangwanghust@gmail.com>
Signed-off-by: CooooolFrog <zuliangwanghust@gmail.com>
Signed-off-by: CooooolFrog <zuliangwanghust@gmail.com>
Signed-off-by: CooooolFrog <zuliangwanghust@gmail.com>
Signed-off-by: CooooolFrog <zuliangwanghust@gmail.com>
CormickKneey
left a comment
CormickKneey
left a comment
There was a problem hiding this comment.
Thanks for this PR — security is indeed important. I've left some review comments.
Additionally, unit tests are essential for this kind of change. Please take a look and address them when you get a chance.
| safe_headers, | ||
| ); | ||
|
|
||
| if !sensitive_headers.is_empty() { |
There was a problem hiding this comment.
This logs the actual values of Authorization, Cookie, etc.
Recommendation: Log only the header names, not their values. Or redact values to something like [REDACTED].
| "x-auth-token", | ||
| ]; | ||
|
|
||
| let blacklist_set: HashSet<&str> = HEADER_BLACKLIST.iter().cloned().collect(); |
There was a problem hiding this comment.
This allocates a new HashSet per invocation. For a proxy that handles high request volume, this is unnecessary overhead.
With only 6 entries, a linear scan over the const slice is actually faster.
| "{} | method={}, uri={}, version={:?}, safe_headers={:?}", | ||
| log_info, | ||
| request.method(), | ||
| request.uri(), |
There was a problem hiding this comment.
Query strings often carry tokens (?token=xxx, ?access_key=xxx). This should either be sanitized or at least called out as a known limitation.
| let (mut safe_headers, mut sensitive_headers) = (Vec::new(), Vec::new()); | ||
| for (name, value) in request.headers().iter() { | ||
| if blacklist_set.contains(&name.as_str()) { | ||
| sensitive_headers.push((name, value)); |
There was a problem hiding this comment.
No need to allocate a Vec, you can use a bool flag like this for better performance:
let sensitive_names: Vec<&str> = request.headers().keys()
.filter(|name| SENSITIVE_HEADERS.contains(&name.as_str()))
.map(|name| name.as_str())
.collect();
if !sensitive_names.is_empty() {
debug!("{} | redacted sensitive headers: {:?}", log_info, sensitive_names);
}| .boxed() | ||
| } | ||
|
|
||
| /// log_request safely output information from the request through logs |
There was a problem hiding this comment.
capitalize first word
|
|
||
| /// log_request safely output information from the request through logs | ||
| fn log_request(request: &Request<hyper::body::Incoming>, log_info: &str) { | ||
| const HEADER_BLACKLIST: &[&str] = &[ |
There was a problem hiding this comment.
Missing commonly sensitive headers:
x-api-keyx-csrf-token/x-xsrf-tokenwww-authenticate(may leak auth scheme details)x-forwarded-for(PII — client IP)
Consider whether an allowlist approach (log only known-safe headers) would be more secure than a blocklist, which requires ongoing maintenance.
2 participants
Description
This PR addresses a potential security problem mentioned in this issue (#1559) by changing the output logs for request-related data from the info level to the debug level, thus preventing the direct output of sensitive information.
Related Issue
#1559