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

fix: enforce w3c trace context value validation #777

Merged
merged 5 commits into from
Dec 5, 2021
Merged

fix: enforce w3c trace context value validation #777

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
885b7a9
fix: enforce w3c trace context value validation
minherz Dec 3, 2021
9000c57
🦉 Updates from OwlBot
gcf-owl-bot[bot] Dec 3, 2021
05e00c8
chore: fix bug in formatting validation
minherz Dec 3, 2021
18c4b1c
Merge branch 'minherz/fix-774' of https://github.com/googleapis/java-…
minherz Dec 3, 2021
ae8e923
chore: refactor w3c format regex
minherz Dec 3, 2021
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
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,13 @@ implementation 'com.google.cloud:google-cloud-logging'
If you are using Gradle without BOM, add this to your dependencies

```Groovy
implementation 'com.google.cloud:google-cloud-logging:3.5.0'
implementation 'com.google.cloud:google-cloud-logging:3.5.1'
```

If you are using SBT, add this to your dependencies

```Scala
libraryDependencies += "com.google.cloud" % "google-cloud-logging" % "3.5.0"
libraryDependencies += "com.google.cloud" % "google-cloud-logging" % "3.5.1"
```

## Authentication
Expand Down
33 changes: 14 additions & 19 deletions google-cloud-logging/src/main/java/com/google/cloud/logging/Context.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,17 @@

import com.google.cloud.logging.HttpRequest.RequestMethod;
import com.google.common.base.MoreObjects;
import com.google.common.base.Strings;
import java.util.Objects;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/** Class to hold context attributes including information about {@see HttpRequest} and tracing. */
public class Context {
// validate W3C trace context value on load according to the existing version format.
// see https://www.w3.org/TR/trace-context/#traceparent-header-field-values for details.
private static final Pattern W3C_TRACE_CONTEXT_FORMAT =
Pattern.compile(
"^00-(?!00000000000000000000000000000000)[0-9abcdef]{32}-(?!000000000000)[0-9abcdef]{12}-[0-9abcdef]{2}$");
minherz marked this conversation as resolved.
Show resolved Hide resolved
private final HttpRequest request;
private final String traceId;
private final String spanId;
Expand Down Expand Up @@ -142,26 +148,15 @@ public Builder loadCloudTraceContext(String cloudTrace) {
*/
public Builder loadW3CTraceParentContext(String traceParent) throws IllegalArgumentException {
if (traceParent != null) {
String[] fields = traceParent.split("-");
if (fields.length > 3) {
String versionFormat = fields[0];
if (!versionFormat.equals("00")) {
throw new IllegalArgumentException("Not supporting versionFormat other than \"00\"");
}
} else {
Matcher validator = W3C_TRACE_CONTEXT_FORMAT.matcher(traceParent.toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if toLowerCase() is needed. The spec says "Vendors MUST ignore the traceparent when the parent-id is invalid (for example, if it contains non-lowercase hex characters)."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know whether we should be so strict about it. A chance that two different trace ids will be produced that differ only by a specific letter register seems low to me. But this way we can avoid exceptions that interrupt the metadata collection in the library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use then toUpperCase() to be safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

My vote would be to just ignore any traceparent headers with invalid characters as the spec suggests, that upper-case "MUST" in the docs seems pretty serious haha.

My interpretation is that header parsing is meant to be farily strict, we're not supposed to alter the fields to make the data fit our expectations. But I'll leave the decision up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you about the standard. My concern is that ignoring it (or throwing and catching exception on the same account) would be very hard to troubleshoot for application developers. To be a little more liberal on this might prove to be closer to the spirit of the shared library.

if (!validator.matches()) {
throw new IllegalArgumentException(
"Invalid format of the header value. Expected \"00-traceid-spanid-arguments\"");
}
String traceId = fields[1];
if (!traceId.isEmpty()) {
setTraceId(traceId);
}
if (!Strings.isNullOrEmpty(traceId)) {
String spanId = fields[2];
if (!spanId.isEmpty()) {
setSpanId(spanId);
}
"Invalid format of the header value. The value does not match W3C Trace Context version \"00\"");
}
String[] fields = traceParent.split("-");
setTraceId(fields[1]);
setSpanId(fields[2]);
// fields[3] contains flag(s)
}
return this;
}
Expand Down
44 changes: 5 additions & 39 deletions google-cloud-logging/src/test/java/com/google/cloud/logging/ContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,50 +132,16 @@ public void testParsingCloudTraceContext() {

@Test
public void testParsingW3CTraceParent() {
final String TRACEPARENT_NO_TRACE = "00--SPAN_ID-FLAGS";
final String TRACEPARENT_TRACE_ONLY = "00-" + TEST_TRACE_ID + "--SPAN_ID-FLAGS";
final String TRACEPARENT_TRACE_FULL = "00-" + TEST_TRACE_ID + "-" + TEST_SPAN_ID + "-FLAGS";
final String W3C_TEST_TRACE_ID = "12345678901234567890123456789012";
final String W3C_TEST_SPAN_ID = "123456789012";
final String W3C_TRACE_CONTEXT = "00-" + W3C_TEST_TRACE_ID + "-" + W3C_TEST_SPAN_ID + "-00";

Context.Builder builder = Context.newBuilder();

builder.loadW3CTraceParentContext(null);
assertTraceAndSpan(builder.build(), null, null);
builder.loadW3CTraceParentContext(TRACEPARENT_NO_TRACE);
assertTraceAndSpan(builder.build(), null, null);
builder.loadW3CTraceParentContext(TRACEPARENT_TRACE_ONLY);
assertTraceAndSpan(builder.build(), TEST_TRACE_ID, null);
builder.loadW3CTraceParentContext(TRACEPARENT_TRACE_FULL);
assertTraceAndSpan(builder.build(), TEST_TRACE_ID, TEST_SPAN_ID);
}

@Test(expected = IllegalArgumentException.class)
public void testEmptyW3CTraceParent() {
Context.Builder builder = Context.newBuilder();
builder.loadW3CTraceParentContext("");
}

@Test(expected = IllegalArgumentException.class)
public void testInvalidFormatW3CTraceParent() {
Context.Builder builder = Context.newBuilder();
builder.loadW3CTraceParentContext("TRACE_ID/SPAN_ID;o=TRACE_TRUE");
}

@Test(expected = IllegalArgumentException.class)
public void testInvalidFormatW3CTraceParent1Dash() {
Context.Builder builder = Context.newBuilder();
builder.loadW3CTraceParentContext("00-TRACE_ID");
}

@Test(expected = IllegalArgumentException.class)
public void testInvalidFormatW3CTraceParentWithoutFlag() {
Context.Builder builder = Context.newBuilder();
builder.loadW3CTraceParentContext("00-TRACE_ID-SPAN_ID-");
}

@Test(expected = IllegalArgumentException.class)
public void testInvalidVersionW3CTraceParent() {
Context.Builder builder = Context.newBuilder();
builder.loadW3CTraceParentContext("01-TRACE_ID-SPAN_ID-FLAGS");
builder.loadW3CTraceParentContext(W3C_TRACE_CONTEXT);
assertTraceAndSpan(builder.build(), W3C_TEST_TRACE_ID, W3C_TEST_SPAN_ID);
}

private void assertTraceAndSpan(Context context, String expectedTraceId, String expectedSpanId) {
Expand Down
57 changes: 57 additions & 0 deletions google-cloud-logging/src/test/java/com/google/cloud/logging/InvalidContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright 2021 Google LLC
*
* Licensed 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
*
* https://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 com.google.cloud.logging;

import java.util.Arrays;
import java.util.Collection;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

@RunWith(Parameterized.class)
public class InvalidContextTest {
@Parameters
public static Collection<String> data() {
final String[] INVALID_W3C_TRACE_CONTEXTS = {
"",
"abc/efg",
"01-something",
"00-123456789012345678901234567890",
"00-12345678901234567890123456789012",
"00-12345678901234567890123456789012345",
"00-12345678901234567890123456789012-12345678901",
"00-12345678901234567890123456789012-123456789012",
"00-12345678901234567890123456789012-1234567890123",
"00-12345678901234567890123456789012-123456789012-1",
"00-12345678901234567890123456789012-123456789012-123"
};
return Arrays.asList(INVALID_W3C_TRACE_CONTEXTS);
}

private final String traceContext;

public InvalidContextTest(String traceContext) {
this.traceContext = traceContext;
}

@Test(expected = IllegalArgumentException.class)
public void testAssertionInvalidContext() {
Context.Builder builder = Context.newBuilder();
builder.loadW3CTraceParentContext(traceContext);
}
}