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.

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

ISSUE-2674 Strip sensitive data from the url #6417

Merged
merged 20 commits into from
Oct 7, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4ba4e71
ISSUE-2674 Strip sensitive data from the url when used in the clientA…
MALPI Aug 3, 2022
bc1ff06
Update instrumentation-api-semconv/src/main/java/io/opentelemetry/ins…
MALPI Aug 5, 2022
89699d1
Update instrumentation-api-semconv/src/main/java/io/opentelemetry/ins…
MALPI Aug 5, 2022
4da196e
Issue-2674 Replace regular expression with index of in order to avoid…
MALPI Aug 22, 2022
76e9bf8
Issue-2674 Add check to only do replacement logic in case the address…
MALPI Aug 23, 2022
6adfacd
Issue-2674 Fix issues reported by checkstyle, apply suggestion not to…
MALPI Aug 23, 2022
5d90ea7
ISSUE-2674 Add check for the scheme and a few more test cases
MALPI Sep 19, 2022
c9dcfb9
ISSUE-2674 Change to parameterized test
MALPI Sep 22, 2022
462c6d2
Merge branch 'main' into bug/issue-2674
MALPI Sep 22, 2022
8904b56
ISSUE-2674 Add few more test cases
MALPI Sep 23, 2022
bb30785
ISSUE-2674 Check for the end of the host by making use of code from U…
MALPI Sep 23, 2022
1101c16
Update instrumentation-api-semconv/src/main/java/io/opentelemetry/ins…
MALPI Sep 27, 2022
92ee662
Update instrumentation-api-semconv/src/main/java/io/opentelemetry/ins…
MALPI Sep 27, 2022
8434511
Update instrumentation-api-semconv/src/main/java/io/opentelemetry/ins…
MALPI Sep 27, 2022
1f41637
Update instrumentation-api-semconv/src/main/java/io/opentelemetry/ins…
MALPI Sep 27, 2022
ffdea7d
ISSUE-2674 Fix unit tests
MALPI Sep 28, 2022
a25875e
ISSUE-2674 drop questionmarkindex variable and check if at character …
MALPI Oct 5, 2022
6b97196
Update instrumentation-api-semconv/src/main/java/io/opentelemetry/ins…
MALPI Oct 5, 2022
85f4c4f
ISSUE-2674 avoid looking up at character twice
MALPI Oct 6, 2022
054debc
Merge remote-tracking branch 'origin/bug/issue-2674' into bug/issue-2674
MALPI Oct 6, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,49 @@ public static <REQUEST, RESPONSE> HttpClientAttributesExtractorBuilder<REQUEST,
@Override
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
super.onStart(attributes, parentContext, request);
internalSet(attributes, SemanticAttributes.HTTP_URL, getter.url(request));
internalSet(attributes, SemanticAttributes.HTTP_URL, stripSensitiveData(getter.url(request)));
}

@Nullable
private static String stripSensitiveData(@Nullable String url) {
if (url == null || url.isEmpty()) {
return url;
}

int schemeEndIndex = url.indexOf(':');

if (schemeEndIndex == -1) {
// not a valid url
return null;
MALPI marked this conversation as resolved.
Show resolved Hide resolved
}

int len = url.length();
if (len <= schemeEndIndex + 2
|| url.charAt(schemeEndIndex + 1) != '/'
|| url.charAt(schemeEndIndex + 2) != '/') {
// has no authority component
return null;
MALPI marked this conversation as resolved.
Show resolved Hide resolved
}

// look for the end of the host:
// ':' ==> start of port, or
// '/', '?', '#' ==> start of path
MALPI marked this conversation as resolved.
Show resolved Hide resolved
int index;
for (index = schemeEndIndex + 3; index < len; index++) {
char c = url.charAt(index);
if (c == '/' || c == '?' || c == '#') {
break;
}
}

int atIndex = url.indexOf("@");
MALPI marked this conversation as resolved.
Show resolved Hide resolved
int questionMarkIndex = url.indexOf("?");
mateuszrzeszutek marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

is questionMarkIndex really needed?


// '@' char is present and is not a query param
if (atIndex == -1 || (questionMarkIndex != -1 && atIndex > index)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should also check whether @ is the last character and if it is return the url as is. Try with https://github.com@ java.net.URI parses it so that github.com@ is authority not userinfo.

Copy link
Member

Choose a reason for hiding this comment

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

atIndex > index will always be false -- as Lauri's mentioned, you don't need that questionMarkIndex variable.

return url;
}
return url.substring(0, schemeEndIndex + 3) + url.substring(atIndex + 1);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
import static org.assertj.core.api.Assertions.entry;
import static org.junit.jupiter.params.provider.Arguments.arguments;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.Attributes;
Expand All @@ -19,8 +20,12 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

class HttpClientAttributesExtractorTest {

Expand Down Expand Up @@ -112,6 +117,38 @@ void normal() {
asList("654", "321")));
}

@ParameterizedTest
@MethodSource("stripUrlArguments")
void stripBasicAuthTest(String url, String expectedResult) {
Map<String, String> request = new HashMap<>();
request.put("url", url);

stripRequestTest(request, expectedResult);
}

private static Stream<Arguments> stripUrlArguments() {
return Stream.of(
arguments("https://user1:[email protected]", "https://github.com"),
arguments("https://user1:[email protected]/path/", "https://github.com/path/"),
arguments("https://user1:[email protected]#test.html", "https://github.com#test.html"),
arguments("https://user1:[email protected]?foo=b@r", "https://github.com?foo=b@r"),
arguments(
"https://user1:[email protected]/p@th?foo=b@r", "https://github.com/p@th?foo=b@r"),
arguments("https://github.com/p@th?foo=b@r", "https://github.com/p@th?foo=b@r"),
arguments("https://github.com#[email protected]", "https://github.com#[email protected]"),
arguments("user1:[email protected]", null));
}

private static void stripRequestTest(Map<String, String> request, String expected) {
HttpClientAttributesExtractor<Map<String, String>, Map<String, String>> extractor =
HttpClientAttributesExtractor.builder(new TestHttpClientAttributesGetter()).build();

AttributesBuilder attributes = Attributes.builder();
extractor.onStart(attributes, Context.root(), request);

assertThat(attributes.build()).containsOnly(entry(SemanticAttributes.HTTP_URL, expected));
}

@Test
void invalidStatusCode() {
Map<String, String> request = new HashMap<>();
Expand Down