Skip to content

Commit d688c39

Browse files
maxlambrechtmaxlambrecht
authored
fix(spiffeid): tighten path validation and segment construction (#420)
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
1 parent ff80784 commit d688c39

4 files changed

Lines changed: 200 additions & 32 deletions

File tree

‎java-spiffe-core/src/main/java/io/spiffe/spiffeid/SpiffeId.java‎

Lines changed: 29 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ public final class SpiffeId {
2525
static final String DOT_SEGMENT = "Path cannot contain dot segments";
2626
static final String EMPTY_SEGMENT = "Path cannot contain empty segments";
2727
static final String TRAILING_SLASH = "Path cannot have a trailing slash";
28+
static final String MISSING_LEADING_SLASH = "Path must start with '/'";
2829

2930
private final TrustDomain trustDomain;
3031

@@ -51,7 +52,7 @@ public static SpiffeId fromSegments(TrustDomain trustDomain, String... segments)
5152

5253
StringBuilder path = new StringBuilder();
5354
for (String p : segments) {
54-
validatePath(p);
55+
validatePathSegment(p);
5556
path.append('/');
5657
path.append(p);
5758
}
@@ -137,37 +138,41 @@ public String toString() {
137138
*/
138139
public static void validatePath(String path) {
139140
if (StringUtils.isBlank(path)) {
140-
throw new IllegalArgumentException(EMPTY);
141+
throw new InvalidSpiffeIdException(EMPTY);
142+
}
143+
144+
if (!path.startsWith("/")) {
145+
throw new InvalidSpiffeIdException(MISSING_LEADING_SLASH);
141146
}
142147

143-
int segmentStart = 0;
144-
int segmentEnd = 0;
148+
// Keep trailing empty segments so "/a/" is distinguished from "/a"
149+
String[] segments = path.substring(1).split("/", -1);
150+
for (int i = 0; i < segments.length; i++) {
151+
String segment = segments[i];
152+
boolean lastSegment = i == segments.length - 1;
145153

146-
for (; segmentEnd < path.length(); segmentEnd++) {
147-
char c = path.charAt(segmentEnd);
148-
if (c == '/') {
149-
switch (path.substring(segmentStart, segmentEnd)) {
150-
case "/":
151-
throw new InvalidSpiffeIdException(EMPTY_SEGMENT);
152-
case "/.":
153-
case "/..":
154-
throw new InvalidSpiffeIdException(DOT_SEGMENT);
155-
}
156-
segmentStart = segmentEnd;
157-
continue;
154+
if (segment.isEmpty()) {
155+
throw new InvalidSpiffeIdException(lastSegment ? TRAILING_SLASH : EMPTY_SEGMENT);
158156
}
157+
158+
validatePathSegment(segment);
159+
}
160+
}
161+
162+
private static void validatePathSegment(String segment) {
163+
if (StringUtils.isEmpty(segment)) {
164+
throw new InvalidSpiffeIdException(EMPTY);
165+
}
166+
167+
if (".".equals(segment) || "..".equals(segment)) {
168+
throw new InvalidSpiffeIdException(DOT_SEGMENT);
169+
}
170+
171+
for (char c : segment.toCharArray()) {
159172
if (!isValidPathSegmentChar(c)) {
160173
throw new InvalidSpiffeIdException(BAD_PATH_SEGMENT_CHAR);
161174
}
162175
}
163-
164-
switch (path.substring(segmentStart, segmentEnd)) {
165-
case "/":
166-
throw new InvalidSpiffeIdException(TRAILING_SLASH);
167-
case "/.":
168-
case "/..":
169-
throw new InvalidSpiffeIdException(DOT_SEGMENT);
170-
}
171176
}
172177

173178
private static boolean isValidPathSegmentChar(char c) {

‎java-spiffe-core/src/test/java/io/spiffe/spiffeid/SpiffeIdTest.java‎

Lines changed: 135 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,11 @@
1111
import java.util.stream.Collectors;
1212
import java.util.stream.Stream;
1313

14+
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
1415
import static org.junit.jupiter.api.Assertions.assertEquals;
1516
import static org.junit.jupiter.api.Assertions.assertFalse;
17+
import static org.junit.jupiter.api.Assertions.assertNotEquals;
18+
import static org.junit.jupiter.api.Assertions.assertThrows;
1619
import static org.junit.jupiter.api.Assertions.assertTrue;
1720
import static org.junit.jupiter.api.Assertions.fail;
1821

@@ -61,15 +64,28 @@ void testParseValidSpiffeId(String input, TrustDomain expectedTrustDomain, Strin
6164

6265
static Stream<Arguments> provideTestValidSpiffeIds() {
6366
return Stream.of(
67+
Arguments.of("spiffe://trustdomain", TrustDomain.parse("trustdomain"), ""),
6468
Arguments.of("spiffe://trustdomain/path", TrustDomain.parse("trustdomain"), "/path"),
6569
Arguments.of("spiffe://trustdomain/path1/path2", TrustDomain.parse("trustdomain"), "/path1/path2"),
6670
Arguments.of("spiffe://trustdomain/PATH1/PATH2", TrustDomain.parse("trustdomain"), "/PATH1/PATH2"),
6771
Arguments.of("spiffe://trustdomain/9eebccd2-12bf-40a6-b262-65fe0487d453", TrustDomain.parse("trustdomain"), "/9eebccd2-12bf-40a6-b262-65fe0487d453"),
72+
Arguments.of("spiffe://a_b.example/foo", TrustDomain.parse("a_b.example"), "/foo"),
73+
Arguments.of("spiffe://1.2.3.4/service", TrustDomain.parse("1.2.3.4"), "/service"),
6874
Arguments.of("SPIFFE://trustdomain/path", TrustDomain.parse("trustdomain"), "/path"),
6975
Arguments.of("SpIfFe://TrUsTdOmAiN/Workload", TrustDomain.parse("trustdomain"), "/Workload")
7076
);
7177
}
7278

79+
static Stream<String> provideNonDnsShapedTrustDomains() {
80+
return Stream.of(
81+
"example..org",
82+
".example.org",
83+
"example.org.",
84+
"-example.org",
85+
"example-.org"
86+
);
87+
}
88+
7389
@ParameterizedTest
7490
@MethodSource("provideInvalidSpiffeIds")
7591
void testParseInvalidSpiffeId(String input, String expected) {
@@ -101,6 +117,8 @@ static Stream<Arguments> provideInvalidSpiffeIds() {
101117
Arguments.of("spiffe://trustdomain/../other", "Path cannot contain dot segments"),
102118
Arguments.of("spiffe://trustdomain/", "Path cannot have a trailing slash"),
103119
Arguments.of("spiffe://trustdomain/path/", "Path cannot have a trailing slash"),
120+
Arguments.of("spiffe://[::1]/service", "Trust domain characters are limited to lowercase letters, numbers, dots, dashes, and underscores"),
121+
Arguments.of("spiffe://[2001:db8::1]/service", "Trust domain characters are limited to lowercase letters, numbers, dots, dashes, and underscores"),
104122
Arguments.of("xspiffe://trustdomain/path", "Scheme is missing or invalid")
105123
);
106124
}
@@ -140,13 +158,13 @@ void testOfInvalid(TrustDomain trustDomain, String[] inputPath, String expectedE
140158
static Stream<Arguments> provideInvalidArguments() {
141159
return Stream.of(
142160
Arguments.of(null, new String[]{""}, "trustDomain must not be null"),
143-
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"/ele%5ment"}, "Path segment characters are limited to letters, numbers, dots, dashes, and underscores"),
144-
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"/path/"}, "Path cannot have a trailing slash"),
145-
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"/ /"}, "Path segment characters are limited to letters, numbers, dots, dashes, and underscores"),
146-
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"/"}, "Path cannot have a trailing slash"),
147-
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"//"}, "Path cannot contain empty segments"),
148-
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"/./"}, "Path cannot contain dot segments"),
149-
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"/../"}, "Path cannot contain dot segments")
161+
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{""}, "Cannot be empty"),
162+
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"ele%5ment"}, "Path segment characters are limited to letters, numbers, dots, dashes, and underscores"),
163+
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"/service"}, "Path segment characters are limited to letters, numbers, dots, dashes, and underscores"),
164+
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"service/"}, "Path segment characters are limited to letters, numbers, dots, dashes, and underscores"),
165+
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"foo/bar"}, "Path segment characters are limited to letters, numbers, dots, dashes, and underscores"),
166+
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{"."}, "Path cannot contain dot segments"),
167+
Arguments.of(TrustDomain.parse("trustdomain"), new String[]{".."}, "Path cannot contain dot segments")
150168
);
151169
}
152170

@@ -239,4 +257,114 @@ void memberOf_aTrustDomainAndASpiffeIdWithDifferentTrustDomain_ReturnsFalse() {
239257

240258
assertFalse(isMemberOf);
241259
}
260+
261+
@Test
262+
void parseMixedCaseSchemeAndTrustDomain_toStringReturnsCanonicalFormAndPreservesPathCase() {
263+
SpiffeId spiffeId = SpiffeId.parse("SPIFFE://EXAMPLE.ORG/MyService");
264+
265+
assertEquals("spiffe://example.org/MyService", spiffeId.toString());
266+
}
267+
268+
@Test
269+
void parseEquivalentIdsWithDifferentSchemeAndTrustDomainCase_areEqual() {
270+
SpiffeId lowercase = SpiffeId.parse("spiffe://example.org/service");
271+
SpiffeId uppercaseScheme = SpiffeId.parse("SPIFFE://example.org/service");
272+
SpiffeId uppercaseTrustDomain = SpiffeId.parse("spiffe://EXAMPLE.ORG/service");
273+
SpiffeId uppercaseBoth = SpiffeId.parse("SPIFFE://EXAMPLE.ORG/service");
274+
275+
assertEquals(lowercase, uppercaseScheme);
276+
assertEquals(lowercase, uppercaseTrustDomain);
277+
assertEquals(lowercase, uppercaseBoth);
278+
assertEquals(lowercase.hashCode(), uppercaseBoth.hashCode());
279+
}
280+
281+
@Test
282+
void parseIdsWithDifferentPathCase_areNotEqual() {
283+
SpiffeId lowercasePath = SpiffeId.parse("spiffe://example.org/service");
284+
SpiffeId uppercasePath = SpiffeId.parse("spiffe://example.org/Service");
285+
286+
assertNotEquals(lowercasePath, uppercasePath);
287+
}
288+
289+
@ParameterizedTest
290+
@MethodSource("provideNonDnsShapedTrustDomains")
291+
void parseSpiffeIdWithNonDnsShapedTrustDomain_isAccepted(String trustDomainName) {
292+
SpiffeId spiffeId = SpiffeId.parse("spiffe://" + trustDomainName + "/service");
293+
294+
assertEquals(TrustDomain.parse(trustDomainName), spiffeId.getTrustDomain());
295+
assertEquals("/service", spiffeId.getPath());
296+
assertEquals("spiffe://" + trustDomainName + "/service", spiffeId.toString());
297+
}
298+
299+
@ParameterizedTest
300+
@MethodSource("provideNonDnsShapedTrustDomains")
301+
void fromSegmentsWithNonDnsShapedTrustDomain_isAccepted(String trustDomainName) {
302+
TrustDomain trustDomain = TrustDomain.parse(trustDomainName);
303+
304+
SpiffeId spiffeId = SpiffeId.fromSegments(trustDomain, "service");
305+
306+
assertEquals(trustDomain, spiffeId.getTrustDomain());
307+
assertEquals("/service", spiffeId.getPath());
308+
assertEquals("spiffe://" + trustDomainName + "/service", spiffeId.toString());
309+
}
310+
311+
@ParameterizedTest
312+
@MethodSource("provideInvalidSegmentsForFromSegments")
313+
void fromSegments_invalidSegment_throwsInvalidSpiffeIdException(String segment, String expectedMessage) {
314+
InvalidSpiffeIdException ex = assertThrows(
315+
InvalidSpiffeIdException.class,
316+
() -> SpiffeId.fromSegments(TrustDomain.parse("example.org"), segment));
317+
assertEquals(expectedMessage, ex.getMessage());
318+
}
319+
320+
static Stream<Arguments> provideInvalidSegmentsForFromSegments() {
321+
return Stream.of(
322+
Arguments.of(null, SpiffeId.EMPTY),
323+
Arguments.of("", SpiffeId.EMPTY),
324+
Arguments.of(" ", SpiffeId.BAD_PATH_SEGMENT_CHAR)
325+
);
326+
}
327+
328+
@ParameterizedTest
329+
@MethodSource("provideInvalidPathsForValidatePath")
330+
void validatePath_invalidPath_throwsInvalidSpiffeIdException(String path, String expectedMessage) {
331+
InvalidSpiffeIdException ex = assertThrows(
332+
InvalidSpiffeIdException.class,
333+
() -> SpiffeId.validatePath(path));
334+
assertEquals(expectedMessage, ex.getMessage());
335+
}
336+
337+
static Stream<Arguments> provideInvalidPathsForValidatePath() {
338+
return Stream.of(
339+
Arguments.of(" ", SpiffeId.EMPTY),
340+
Arguments.of("foo", SpiffeId.MISSING_LEADING_SLASH),
341+
Arguments.of("foo/bar", SpiffeId.MISSING_LEADING_SLASH),
342+
Arguments.of("/foo//bar", SpiffeId.EMPTY_SEGMENT),
343+
Arguments.of("/./other", SpiffeId.DOT_SEGMENT),
344+
Arguments.of("/../other", SpiffeId.DOT_SEGMENT),
345+
Arguments.of("/foo/.", SpiffeId.DOT_SEGMENT),
346+
Arguments.of("/foo/..", SpiffeId.DOT_SEGMENT),
347+
Arguments.of("/foo/", SpiffeId.TRAILING_SLASH),
348+
Arguments.of("/", SpiffeId.TRAILING_SLASH),
349+
Arguments.of("/ ", SpiffeId.BAD_PATH_SEGMENT_CHAR),
350+
Arguments.of("/foo%5Cbar", SpiffeId.BAD_PATH_SEGMENT_CHAR),
351+
Arguments.of("/foo bar", SpiffeId.BAD_PATH_SEGMENT_CHAR)
352+
);
353+
}
354+
355+
@ParameterizedTest
356+
@MethodSource("provideValidPathsForValidatePath")
357+
void validatePath_validPath_doesNotThrow(String path) {
358+
assertDoesNotThrow(() -> SpiffeId.validatePath(path));
359+
}
360+
361+
static Stream<String> provideValidPathsForValidatePath() {
362+
return Stream.of(
363+
"/foo",
364+
"/foo/bar",
365+
"/PATH/path",
366+
"/.../svc",
367+
"/9eebccd2-12bf-40a6-b262-65fe0487d453"
368+
);
369+
}
242370
}

‎java-spiffe-core/src/test/java/io/spiffe/spiffeid/TrustDomainTest.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,25 @@ void testTrustDomainFromNameMixedCase_isNormalizedToLowercase() {
2626
assertEquals("trustdomain", trustDomain.getName());
2727
}
2828

29+
@Test
30+
void testTrustDomainFromNameWithUnderscore() {
31+
TrustDomain trustDomain = TrustDomain.parse("trust_domain_name.example.com");
32+
assertEquals("trust_domain_name.example.com", trustDomain.getName());
33+
}
34+
35+
@Test
36+
void testTrustDomainFromIpv4Name() {
37+
TrustDomain trustDomain = TrustDomain.parse("1.2.3.4");
38+
assertEquals("1.2.3.4", trustDomain.getName());
39+
}
40+
41+
@ParameterizedTest
42+
@MethodSource("provideNonDnsShapedTrustDomains")
43+
void testTrustDomainFromNonDnsShapedName_isAccepted(String input) {
44+
TrustDomain trustDomain = TrustDomain.parse(input);
45+
assertEquals(input, trustDomain.getName());
46+
}
47+
2948
@Test
3049
void testFromIdStringWithoutPath() {
3150
TrustDomain trustDomain = TrustDomain.parse("spiffe://trustdomain");
@@ -90,6 +109,16 @@ static Stream<Arguments> provideInvalidTrustDomain() {
90109
);
91110
}
92111

112+
static Stream<String> provideNonDnsShapedTrustDomains() {
113+
return Stream.of(
114+
"example..org",
115+
".example.org",
116+
"example.org.",
117+
"-example.org",
118+
"example-.org"
119+
);
120+
}
121+
93122
@Test
94123
void testNewSpiffeId() {
95124
TrustDomain trustDomain = TrustDomain.parse("test.domain");
@@ -135,6 +164,12 @@ void testParseInvalidScheme_httpScheme_throwsInvalidScheme() {
135164
() -> TrustDomain.parse("http://example.org"));
136165
}
137166

167+
@Test
168+
void testParseIpv6TrustDomain_throwsInvalidTrustDomain() {
169+
assertThrows(InvalidSpiffeIdException.class,
170+
() -> TrustDomain.parse("[::1]"));
171+
}
172+
138173
@Test
139174
void testParseColonNotFollowedBySlash_validatesAsTrustDomain() {
140175
assertThrows(InvalidSpiffeIdException.class,

‎java-spiffe-core/src/test/java/io/spiffe/svid/jwtsvid/JwtSvidParseInsecureTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ void testParseInsecure_nullAudience_throwsNullPointerException() throws JwtSvidE
8484
try {
8585
KeyPair key1 = TestUtils.generateECKeyPair(Curve.P_521);
8686
TrustDomain trustDomain = TrustDomain.parse("test.domain");
87-
SpiffeId spiffeId = trustDomain.newSpiffeId("/host");
87+
SpiffeId spiffeId = trustDomain.newSpiffeId("host");
8888
Set<String> audience = Collections.singleton("audience");
8989
Date expiration = new Date(System.currentTimeMillis() + 3600000);
9090
JWTClaimsSet claims = TestUtils.buildJWTClaimSet(audience, spiffeId.toString(), expiration);

0 commit comments

Comments
 (0)