From d4ac2eaf9395973f70ecb793ee0fb8ea5332c879 Mon Sep 17 00:00:00 2001 From: jwatson Date: Wed, 19 Aug 2020 11:22:34 -0700 Subject: [PATCH 1/3] Update the TraceState key validations to more closely match the w3c spec. --- .../io/opentelemetry/trace/TraceState.java | 39 ++++++++++++++++--- .../opentelemetry/trace/TraceStateTest.java | 29 +++++++++++++- 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/api/src/main/java/io/opentelemetry/trace/TraceState.java b/api/src/main/java/io/opentelemetry/trace/TraceState.java index 6f114961488..208b359dcce 100644 --- a/api/src/main/java/io/opentelemetry/trace/TraceState.java +++ b/api/src/main/java/io/opentelemetry/trace/TraceState.java @@ -235,26 +235,53 @@ public static Entry create(String key, String value) { // Key is opaque string up to 256 characters printable. It MUST begin with a lowercase letter, and // can only contain lowercase letters a-z, digits 0-9, underscores _, dashes -, asterisks *, and // forward slashes /. For multi-tenant vendor scenarios, an at sign (@) can be used to prefix the - // vendor name. + // vendor name. The tenant id (before the '@') is limited to 240 characters and the vendor id is + // limited to 13 characters. If in the multi-tenant vendor format, then the first character + // may additionally be digit. + // // todo: benchmark this implementation private static boolean validateKey(String key) { - if (key.length() > KEY_MAX_SIZE || key.isEmpty() || !isNumberOrDigit(key.charAt(0))) { + if (key.length() > KEY_MAX_SIZE || key.isEmpty() || !isLowercaseLetterOrDigit(key.charAt(0))) { return false; } int atSeenCount = 0; for (int i = 1; i < key.length(); i++) { char c = key.charAt(i); - if (!isNumberOrDigit(c) && c != '_' && c != '-' && c != '@' && c != '*' && c != '/') { + if (!isLowercaseLetterOrDigit(c) + && c != '_' + && c != '-' + && c != '@' + && c != '*' + && c != '/') { return false; } - if ((c == '@') && (++atSeenCount > 1)) { - return false; + int atPosition = 0; + if (c == '@') { + atPosition = i; + // tenant id must be 240 characters or less + if (i > 240) { + return false; + } + atSeenCount++; + if (atSeenCount > 1) { + return false; + } } + if (atSeenCount == 1) { + // vendor id must be 13 characters or less + if ((i - atPosition) > 13) { + return false; + } + } + } + if (atSeenCount == 0) { + // if it's not the vendor format (with an '@' sign), the key must start with a letter. + return !Character.isDigit(key.charAt(0)); } return true; } - private static boolean isNumberOrDigit(char ch) { + private static boolean isLowercaseLetterOrDigit(char ch) { return (ch >= 'a' && ch <= 'z') || (ch >= '0' && ch <= '9'); } diff --git a/api/src/test/java/io/opentelemetry/trace/TraceStateTest.java b/api/src/test/java/io/opentelemetry/trace/TraceStateTest.java index 0b60ff47ecf..9769d9d9f8f 100644 --- a/api/src/test/java/io/opentelemetry/trace/TraceStateTest.java +++ b/api/src/test/java/io/opentelemetry/trace/TraceStateTest.java @@ -72,8 +72,9 @@ void invalidFirstKeyCharacter() { @Test void firstKeyCharacterDigitIsAllowed() { - TraceState result = EMPTY.toBuilder().set("1_key", FIRST_VALUE).build(); - assertThat(result.get("1_key")).isEqualTo(FIRST_VALUE); + // note: a digit is only allowed if the key is in the tenant format (with an '@') + TraceState result = EMPTY.toBuilder().set("1@tenant", FIRST_VALUE).build(); + assertThat(result.get("1@tenant")).isEqualTo(FIRST_VALUE); } @Test @@ -88,6 +89,30 @@ void testValidAtSignVendorNamePrefix() { assertThat(result.get("1@nr")).isEqualTo(FIRST_VALUE); } + @Test + void testVendorIdLongerThan13Characters() { + assertThrows( + IllegalArgumentException.class, + () -> EMPTY.toBuilder().set("1@nrabcdefghijkl", FIRST_VALUE).build()); + } + + @Test + void tenantIdLongerThan240Characters() { + char[] chars = new char[241]; + Arrays.fill(chars, 'a'); + String tenantId = new String(chars); + assertThrows( + IllegalArgumentException.class, + () -> EMPTY.toBuilder().set(tenantId + "@nr", FIRST_VALUE).build()); + } + + @Test + void testNonVendorFormatFirstKeyCharacter() { + assertThrows( + IllegalArgumentException.class, + () -> EMPTY.toBuilder().set("1acdfrgs", FIRST_VALUE).build()); + } + @Test void testMultipleAtSignNotAllowed() { assertThrows( From 18f20313e4bf259c4da5dca857bbf5055901fd20 Mon Sep 17 00:00:00 2001 From: jwatson Date: Thu, 20 Aug 2020 11:32:45 -0700 Subject: [PATCH 2/3] rework method and variable names for more clarity --- .../io/opentelemetry/trace/TraceState.java | 57 ++++++++++++------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/api/src/main/java/io/opentelemetry/trace/TraceState.java b/api/src/main/java/io/opentelemetry/trace/TraceState.java index 208b359dcce..efc68259760 100644 --- a/api/src/main/java/io/opentelemetry/trace/TraceState.java +++ b/api/src/main/java/io/opentelemetry/trace/TraceState.java @@ -46,6 +46,8 @@ public abstract class TraceState { private static final int VALUE_MAX_SIZE = 256; private static final int MAX_KEY_VALUE_PAIRS = 32; private static final TraceState DEFAULT = TraceState.builder().build(); + private static final int MAX_TENANT_ID_SIZE = 240; + public static final int MAX_VENDOR_ID_SIZE = 13; /** * Returns the default {@code TraceState} with no entries. @@ -241,48 +243,61 @@ public static Entry create(String key, String value) { // // todo: benchmark this implementation private static boolean validateKey(String key) { - if (key.length() > KEY_MAX_SIZE || key.isEmpty() || !isLowercaseLetterOrDigit(key.charAt(0))) { + if (key.length() > KEY_MAX_SIZE + || key.isEmpty() + || isNotLowercaseLetterOrDigit(key.charAt(0))) { return false; } - int atSeenCount = 0; + boolean isMultiTenantVendorKey = false; for (int i = 1; i < key.length(); i++) { char c = key.charAt(i); - if (!isLowercaseLetterOrDigit(c) - && c != '_' - && c != '-' - && c != '@' - && c != '*' - && c != '/') { + if (isNotLegalKeyCharacter(c)) { return false; } - int atPosition = 0; + // the location of the '@' sign, or -1 if it isn't there. + int atPosition = -1; if (c == '@') { - atPosition = i; - // tenant id must be 240 characters or less - if (i > 240) { + // you can't have 2 '@' signs + if (isMultiTenantVendorKey) { return false; } - atSeenCount++; - if (atSeenCount > 1) { + atPosition = i; + isMultiTenantVendorKey = true; + // tenant id (the part to the left of the '@' sign) must be 240 characters or less + if (i > MAX_TENANT_ID_SIZE) { return false; } } - if (atSeenCount == 1) { - // vendor id must be 13 characters or less - if ((i - atPosition) > 13) { + if (isMultiTenantVendorKey) { + // vendor id (the part to the right of the '@' sign) must be 13 characters or less + int numberOfCharactersPastTheAt = i - atPosition; + if (numberOfCharactersPastTheAt > MAX_VENDOR_ID_SIZE) { return false; } } } - if (atSeenCount == 0) { + if (!isMultiTenantVendorKey) { // if it's not the vendor format (with an '@' sign), the key must start with a letter. - return !Character.isDigit(key.charAt(0)); + return isNotDigit(key.charAt(0)); } return true; } - private static boolean isLowercaseLetterOrDigit(char ch) { - return (ch >= 'a' && ch <= 'z') || (ch >= '0' && ch <= '9'); + private static boolean isNotLegalKeyCharacter(char c) { + return isNotLowercaseLetterOrDigit(c) + && c != '_' + && c != '-' + && c != '@' + && c != '*' + && c != '/'; + } + + private static boolean isNotLowercaseLetterOrDigit(char ch) { + return (ch < 'a' || ch > 'z') && isNotDigit(ch); + } + + private static boolean isNotDigit(char ch) { + return ch < '0' || ch > '9'; } // Value is opaque string up to 256 characters printable ASCII RFC0020 characters (i.e., the range From 585922d441ee0556ffb28dd78e37562bb362473b Mon Sep 17 00:00:00 2001 From: jwatson Date: Fri, 21 Aug 2020 11:06:04 -0700 Subject: [PATCH 3/3] Add some tests and fix a bug --- .../java/io/opentelemetry/trace/TraceState.java | 8 +------- .../java/io/opentelemetry/trace/TraceStateTest.java | 13 +++++++++++++ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/io/opentelemetry/trace/TraceState.java b/api/src/main/java/io/opentelemetry/trace/TraceState.java index efc68259760..d6945d3a380 100644 --- a/api/src/main/java/io/opentelemetry/trace/TraceState.java +++ b/api/src/main/java/io/opentelemetry/trace/TraceState.java @@ -254,24 +254,18 @@ private static boolean validateKey(String key) { if (isNotLegalKeyCharacter(c)) { return false; } - // the location of the '@' sign, or -1 if it isn't there. - int atPosition = -1; if (c == '@') { // you can't have 2 '@' signs if (isMultiTenantVendorKey) { return false; } - atPosition = i; isMultiTenantVendorKey = true; // tenant id (the part to the left of the '@' sign) must be 240 characters or less if (i > MAX_TENANT_ID_SIZE) { return false; } - } - if (isMultiTenantVendorKey) { // vendor id (the part to the right of the '@' sign) must be 13 characters or less - int numberOfCharactersPastTheAt = i - atPosition; - if (numberOfCharactersPastTheAt > MAX_VENDOR_ID_SIZE) { + if (key.length() - i > MAX_VENDOR_ID_SIZE) { return false; } } diff --git a/api/src/test/java/io/opentelemetry/trace/TraceStateTest.java b/api/src/test/java/io/opentelemetry/trace/TraceStateTest.java index 9769d9d9f8f..d3029340916 100644 --- a/api/src/test/java/io/opentelemetry/trace/TraceStateTest.java +++ b/api/src/test/java/io/opentelemetry/trace/TraceStateTest.java @@ -77,6 +77,12 @@ void firstKeyCharacterDigitIsAllowed() { assertThat(result.get("1@tenant")).isEqualTo(FIRST_VALUE); } + @Test + void testValidLongTenantId() { + TraceState result = EMPTY.toBuilder().set("12345678901234567890@nr", FIRST_VALUE).build(); + assertThat(result.get("12345678901234567890@nr")).isEqualTo(FIRST_VALUE); + } + @Test void invalidKeyCharacters() { assertThrows( @@ -96,6 +102,13 @@ void testVendorIdLongerThan13Characters() { () -> EMPTY.toBuilder().set("1@nrabcdefghijkl", FIRST_VALUE).build()); } + @Test + void testVendorIdLongerThan13Characters_longTenantId() { + assertThrows( + IllegalArgumentException.class, + () -> EMPTY.toBuilder().set("12345678901234567890@nrabcdefghijkl", FIRST_VALUE).build()); + } + @Test void tenantIdLongerThan240Characters() { char[] chars = new char[241];