Skip to content
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

Jump to bottom

HTTPCLIENT-2337: Sanitize X500Principal Logging in ClientTlsStrategy classes #581

Merged
merged 3 commits into from
Sep 22, 2024
Merged

HTTPCLIENT-2337: Sanitize X500Principal Logging in ClientTlsStrategy classes #581

merged 3 commits into from
Sep 22, 2024
+146 −2

Conversation

arturobernalg
Copy link
Member

This PR addresses HTTPCLIENT-2337, which involves potentially unsafe logging of X500Principal in SSLConnectionSocketFactory. The issue is caused by control characters in the X500Principal being logged without sanitization, which could interfere with log readability.

@ok2c
Copy link
Member

ok2c commented Sep 20, 2024

@arturobernalg I think we have kind of decided to not do it, but if no one objects I see no reason to not merge this change-set

@arturobernalg
Copy link
Member Author

please @garydgregory do another pass.

garydgregory
garydgregory approved these changes Sep 20, 2024
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

LGTM, TY @arturobernalg

garydgregory
garydgregory reviewed Sep 20, 2024
View reviewed changes
httpclient5/src/main/java/org/apache/hc/client5/http/ssl/SSLConnectionSocketFactory.java Outdated
final X500Principal peer = x509.getSubjectX500Principal();
LOG.debug("Sanitized peer principal: {}", toEscapedString(peer));
Copy link
Member

Choose a reason for hiding this comment

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

"Sanitized" -> "Escaped"?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

garydgregory
garydgregory approved these changes Sep 20, 2024
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

LGTM.

garydgregory
garydgregory reviewed Sep 21, 2024
View reviewed changes
httpclient5/src/main/java/org/apache/hc/client5/http/ssl/SSLConnectionSocketFactory.java Outdated
final StringBuilder sanitizedPrincipal = new StringBuilder(principalValue.length());
for (final char c : principalValue.toCharArray()) {
if (Character.isISOControl(c)) {
sanitizedPrincipal.append("\\x").append(String.format("%02x", (int) c));
Copy link
Member

Choose a reason for hiding this comment

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

I just remembered that we already do this kind of cleaning/escaping in org.apache.hc.core5.testing.nio.LoggingIOSession.logData(ByteBuffer, String). There may be an opportunity for some refactoring so that we use the same semantics for printing raw data.

@ok2c
Copy link
Member

ok2c commented Sep 21, 2024

@arturobernalg SSLConnectionSocketFactory has been deprecated in favor of DefaultClientTlsStrategy. I really think your time and efforts could be better spent elsewhere but if you really want to do it please move all these changes from SSLConnectionSocketFactory to AbstractClientTlsStrategy.

@arturobernalg
Copy link
Member Author

@arturobernalg SSLConnectionSocketFactory has been deprecated in favor of DefaultClientTlsStrategy. I really think your time and efforts could be better spent elsewhere but if you really want to do it please move all these changes from SSLConnectionSocketFactory to AbstractClientTlsStrategy.

@ok2c
Please check again.
Thank you.

ok2c
ok2c reviewed Sep 22, 2024
View reviewed changes
httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java Outdated
@@ -271,7 +272,7 @@ void verifySession(
final X509Certificate x509 = (X509Certificate) cert;
final X500Principal peer = x509.getSubjectX500Principal();

LOG.debug(" peer principal: {}", peer);
LOG.debug("Escaped peer principal: {}", toEscapedString(peer));
Copy link
Member

Choose a reason for hiding this comment

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

@arturobernalg "Escaped peer principal" sounds confusing to me. I would just leave it as "Peer principal".

@ok2c ok2c changed the title HTTPCLIENT-2337: Sanitize X500Principal Logging in SSLConnectionSocketFactory HTTPCLIENT-2337: Sanitize X500Principal Logging in ClientTlsStrategy classes Sep 22, 2024
@arturobernalg
HTTPCLIENT-2337: Add sanitizeX500Principal method to escape control c…
7d695c5 
…haracters in X500Principal. Escapes ISO control characters in X500Principal using hexadecimal representation.
@garydgregory
Copy link
Member

Unrelated random failure:

Error:  Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.046 s <<< FAILURE! - in org.apache.hc.client5.testing.sync.TestFutureRequestExecutionService
Error:  org.apache.hc.client5.testing.sync.TestFutureRequestExecutionService.shouldCancel  Time elapsed: 0.007 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: Expected java.lang.Exception to be thrown, but nothing was thrown.
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
	at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:73)
	at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:35)
	at org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3128)
	at org.apache.hc.client5.testing.sync.TestFutureRequestExecutionService.shouldCancel(TestFutureRequestExecutionService.java:117)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at java.util.ArrayList.forEach(ArrayList.java:1259)
	at java.util.ArrayList.forEach(ArrayList.java:1259)

garydgregory
garydgregory reviewed Sep 22, 2024
View reviewed changes
httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java Outdated
final StringBuilder sanitizedPrincipal = new StringBuilder(principalValue.length());
for (final char c : principalValue.toCharArray()) {
if (Character.isISOControl(c)) {
sanitizedPrincipal.append("\\x").append(String.format("%02x", (int) c));
Copy link
Member

Choose a reason for hiding this comment

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

Append once: sanitizedPrincipal.append(String.format("\\x%02x", (int) c));

@garydgregory garydgregory merged commit e9560a4 into apache:master Sep 22, 2024
10 checks passed
ok2c pushed a commit that referenced this pull request Oct 1, 2024
@arturobernalg @garydgregory @ok2c
HTTPCLIENT-2337: Sanitize X500Principal Logging in ClientTlsStrategy …
db623f8 
…classes (#581)

* HTTPCLIENT-2337: Add sanitizeX500Principal method to escape control characters in X500Principal. Escapes ISO control characters in X500Principal using hexadecimal representation.

* Remove "Escaped" from debug log message

* Use a single call to append() for each character in toEscapedString()

---------

Co-authored-by: Gary Gregory <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garydgregory garydgregory garydgregory approved these changes

@ok2c ok2c ok2c approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arturobernalg @ok2c @garydgregory