Skip to content

Commit 0f09637

Browse files
maxlambrechtmaxlambrecht
committed
fix(x509): enforce leaf SVID validation and reject multiple URI SANs
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
1 parent 8844a6b commit 0f09637

5 files changed

Lines changed: 174 additions & 51 deletions

File tree

‎java-spiffe-core/src/main/java/io/spiffe/internal/CertificateUtils.java‎

Lines changed: 55 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import java.security.spec.EncodedKeySpec;
1414
import java.security.spec.InvalidKeySpecException;
1515
import java.security.spec.PKCS8EncodedKeySpec;
16+
import java.util.ArrayList;
1617
import java.util.Base64;
1718
import java.util.Collection;
1819
import java.util.Collections;
@@ -24,15 +25,17 @@
2425
import static io.spiffe.internal.KeyUsage.CRL_SIGN;
2526
import static io.spiffe.internal.KeyUsage.DIGITAL_SIGNATURE;
2627
import static io.spiffe.internal.KeyUsage.KEY_CERT_SIGN;
27-
import static org.apache.commons.lang3.StringUtils.startsWith;
2828

2929
/**
3030
* Common certificate utility methods.
3131
*/
3232
public class CertificateUtils {
3333

3434
private static final String SPIFFE_PREFIX = "spiffe://";
35+
private static final int SAN_TYPE_INDEX = 0;
3536
private static final int SAN_VALUE_INDEX = 1;
37+
// X.509 SubjectAlternativeName type for a URI value.
38+
private static final int URI_SAN_TYPE = 6;
3639
private static final String PUBLIC_KEY_INFRASTRUCTURE_ALGORITHM = "PKIX";
3740
private static final String X509_CERTIFICATE_TYPE = "X.509";
3841

@@ -143,16 +146,25 @@ public static boolean isCA(final X509Certificate cert) {
143146

144147
public static boolean hasKeyUsageCertSign(final X509Certificate cert) {
145148
boolean[] keyUsage = cert.getKeyUsage();
149+
if (keyUsage == null) {
150+
return false;
151+
}
146152
return keyUsage[KEY_CERT_SIGN.index()];
147153
}
148154

149155
public static boolean hasKeyUsageDigitalSignature(final X509Certificate cert) {
150156
boolean[] keyUsage = cert.getKeyUsage();
157+
if (keyUsage == null) {
158+
return false;
159+
}
151160
return keyUsage[DIGITAL_SIGNATURE.index()];
152161
}
153162

154163
public static boolean hasKeyUsageCRLSign(final X509Certificate cert) {
155164
boolean[] keyUsage = cert.getKeyUsage();
165+
if (keyUsage == null) {
166+
return false;
167+
}
156168
return keyUsage[CRL_SIGN.index()];
157169
}
158170

@@ -168,16 +180,53 @@ private static EncodedKeySpec getEncodedKeySpec(final byte[] privateKeyBytes, Ke
168180
}
169181

170182
private static List<String> getSpiffeIds(final X509Certificate certificate) throws CertificateParsingException {
171-
if (certificate.getSubjectAlternativeNames() == null) {
183+
final List<String> uriSans = getUriSubjectAlternativeNames(certificate);
184+
if (uriSans.size() > 1) {
185+
throw new CertificateParsingException("Certificate contains multiple URI SANs");
186+
}
187+
if (uriSans.isEmpty()) {
172188
return Collections.emptyList();
173189
}
174-
return certificate.getSubjectAlternativeNames()
175-
.stream()
176-
.map(san -> (String) san.get(SAN_VALUE_INDEX))
177-
.filter(uri -> startsWith(uri, SPIFFE_PREFIX))
190+
return uriSans.stream()
191+
.filter(uri -> uri != null && uri.startsWith(SPIFFE_PREFIX))
178192
.collect(Collectors.toList());
179193
}
180194

195+
private static List<String> getUriSubjectAlternativeNames(final X509Certificate certificate)
196+
throws CertificateParsingException {
197+
final Collection<List<?>> subjectAlternativeNames = certificate.getSubjectAlternativeNames();
198+
if (subjectAlternativeNames == null) {
199+
return Collections.emptyList();
200+
}
201+
202+
final List<String> uriSans = new ArrayList<>();
203+
for (List<?> sanEntry : subjectAlternativeNames) {
204+
final String uriSan = getUriSubjectAlternativeNameValue(sanEntry);
205+
if (uriSan != null) {
206+
uriSans.add(uriSan);
207+
}
208+
}
209+
return uriSans;
210+
}
211+
212+
private static String getUriSubjectAlternativeNameValue(final List<?> sanEntry) {
213+
if (sanEntry == null || sanEntry.size() <= SAN_VALUE_INDEX) {
214+
return null;
215+
}
216+
217+
final Object sanType = sanEntry.get(SAN_TYPE_INDEX);
218+
if (!(sanType instanceof Integer) || ((Integer) sanType) != URI_SAN_TYPE) {
219+
return null;
220+
}
221+
222+
final Object sanValue = sanEntry.get(SAN_VALUE_INDEX);
223+
if (!(sanValue instanceof String)) {
224+
return null;
225+
}
226+
227+
return (String) sanValue;
228+
}
229+
181230
private static PrivateKey generatePrivateKeyWithSpec(final EncodedKeySpec keySpec, AsymmetricKeyAlgorithm algorithm) throws NoSuchAlgorithmException, InvalidKeySpecException {
182231
PrivateKey privateKey = null;
183232
switch (algorithm) {

‎java-spiffe-core/src/main/java/io/spiffe/svid/x509svid/X509Svid.java‎

Lines changed: 13 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -217,24 +217,20 @@ private static X509Svid createX509Svid(final byte[] certsBytes,
217217
final PrivateKey privateKey = generatePrivateKey(privateKeyBytes, keyFileFormat, x509Certificates);
218218
final SpiffeId spiffeId = getSpiffeId(x509Certificates);
219219

220-
validateLeafSpiffeId(spiffeId);
221-
validateLeafCertificate(x509Certificates.get(0));
220+
try {
221+
X509SvidChecks.validateLeafForParsing(x509Certificates.get(0), spiffeId);
222222

223-
// there are intermediate CA certificates
224-
if (x509Certificates.size() > 1) {
225-
validateSigningCertificates(x509Certificates);
223+
// there are intermediate CA certificates
224+
if (x509Certificates.size() > 1) {
225+
validateSigningCertificates(x509Certificates);
226+
}
227+
} catch (CertificateException e) {
228+
throw new X509SvidException(e.getMessage(), e);
226229
}
227230

228231
return new X509Svid(spiffeId, x509Certificates, privateKey, hint);
229232
}
230233

231-
private static void validateLeafSpiffeId(final SpiffeId spiffeId) throws X509SvidException {
232-
final String path = spiffeId.getPath();
233-
if (path == null || path.isEmpty()) {
234-
throw new X509SvidException("Leaf certificate SPIFFE ID must have a non-root path");
235-
}
236-
}
237-
238234
private static SpiffeId getSpiffeId(final List<X509Certificate> x509Certificates) throws X509SvidException {
239235
final SpiffeId spiffeId;
240236
try {
@@ -273,35 +269,11 @@ private static List<X509Certificate> generateX509Certificates(final byte[] certs
273269

274270
private static void validateSigningCertificates(final List<X509Certificate> certificates) throws X509SvidException {
275271
for (int i = 1; i < certificates.size(); i++) {
276-
verifyCaCert(certificates.get(i));
277-
}
278-
}
279-
280-
private static void verifyCaCert(final X509Certificate cert) throws X509SvidException {
281-
if (!CertificateUtils.isCA(cert)) {
282-
throw new X509SvidException("Signing certificate must have CA flag set to true");
283-
}
284-
if (!CertificateUtils.hasKeyUsageCertSign(cert)) {
285-
throw new X509SvidException("Signing certificate must have 'keyCertSign' as key usage");
286-
}
287-
}
288-
289-
private static void validateLeafCertificate(final X509Certificate leaf) throws X509SvidException {
290-
if (CertificateUtils.isCA(leaf)) {
291-
throw new X509SvidException("Leaf certificate must not have CA flag set to true");
292-
}
293-
validateKeyUsageOfLeafCertificate(leaf);
294-
}
295-
296-
private static void validateKeyUsageOfLeafCertificate(final X509Certificate leaf) throws X509SvidException {
297-
if (!CertificateUtils.hasKeyUsageDigitalSignature(leaf)) {
298-
throw new X509SvidException("Leaf certificate must have 'digitalSignature' as key usage");
299-
}
300-
if (CertificateUtils.hasKeyUsageCertSign(leaf)) {
301-
throw new X509SvidException("Leaf certificate must not have 'keyCertSign' as key usage");
302-
}
303-
if (CertificateUtils.hasKeyUsageCRLSign(leaf)) {
304-
throw new X509SvidException("Leaf certificate must not have 'cRLSign' as key usage");
272+
try {
273+
X509SvidChecks.validateSigningCertificate(certificates.get(i));
274+
} catch (CertificateException e) {
275+
throw new X509SvidException(e.getMessage(), e);
276+
}
305277
}
306278
}
307279
}

‎java-spiffe-core/src/main/java/io/spiffe/svid/x509svid/X509SvidValidator.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,14 +43,17 @@ public static void verifyChain(
4343
Objects.requireNonNull(chain, "chain must not be null");
4444
Objects.requireNonNull(x509BundleSource, "x509BundleSource must not be null");
4545

46-
TrustDomain trustDomain = CertificateUtils.getTrustDomain(chain);
46+
final SpiffeId spiffeId = CertificateUtils.getSpiffeId(chain.get(0));
47+
TrustDomain trustDomain = spiffeId.getTrustDomain();
4748
X509Bundle x509Bundle = x509BundleSource.getBundleForTrustDomain(trustDomain);
4849

4950
try {
5051
CertificateUtils.validate(chain, x509Bundle.getX509Authorities());
5152
} catch (CertPathValidatorException e) {
5253
throw new CertificateException("Cert chain cannot be verified", e);
5354
}
55+
56+
X509SvidChecks.validateLeafConstraints(chain.get(0), spiffeId);
5457
}
5558

5659
/**
@@ -77,6 +80,7 @@ public static void verifySpiffeId(X509Certificate x509Certificate,
7780
}
7881

7982
SpiffeId spiffeId = CertificateUtils.getSpiffeId(x509Certificate);
83+
X509SvidChecks.validateLeafConstraints(x509Certificate, spiffeId);
8084
if (!spiffeIdSet.contains(spiffeId)) {
8185
String error = String.format("SPIFFE ID %s in X.509 certificate is not accepted", spiffeId);
8286
log.warning(String.format("Client SPIFFE ID validation failed: %s", error));

‎java-spiffe-core/src/test/java/io/spiffe/internal/CertificateUtilsTest.java‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import io.spiffe.utils.CertAndKeyPair;
77
import io.spiffe.utils.TestUtils;
88
import org.junit.jupiter.api.Test;
9+
import org.mockito.Mockito;
910

1011
import java.io.IOException;
1112
import java.net.URISyntaxException;
@@ -40,13 +41,13 @@ void generateCertificates_ofPEMByteArray_returnsListWithOneX509Certificate() thr
4041
final Path path = Paths.get(toUri("testdata/internal/cert.pem"));
4142
final byte[] certBytes = Files.readAllBytes(path);
4243

43-
List<X509Certificate> x509CertificateList;
44-
SpiffeId spiffeId = null;
44+
final SpiffeId spiffeId;
4545
try {
46-
x509CertificateList = CertificateUtils.generateCertificates(certBytes);
46+
List<X509Certificate> x509CertificateList = CertificateUtils.generateCertificates(certBytes);
4747
spiffeId = CertificateUtils.getSpiffeId(x509CertificateList.get(0));
4848
} catch (CertificateException e) {
4949
fail("Not expected exception. Should have generated the certificates", e);
50+
return;
5051
}
5152

5253
assertEquals("spiffe://example.org/test", spiffeId.toString());
@@ -180,6 +181,22 @@ void testGetSpiffeId_certNotContainSpiffeId_throwsCertificateException() throws
180181
}
181182
}
182183

184+
@Test
185+
void testGetSpiffeId_certContainsMultipleUriSans_throwsCertificateException() throws Exception {
186+
X509Certificate certificate = Mockito.mock(X509Certificate.class);
187+
Mockito.when(certificate.getSubjectAlternativeNames()).thenReturn(Arrays.asList(
188+
Arrays.asList(6, "spiffe://domain.test/workload"),
189+
Arrays.asList(6, "https://example.org/workload")
190+
));
191+
192+
try {
193+
CertificateUtils.getSpiffeId(certificate);
194+
fail("exception is expected");
195+
} catch (CertificateException e) {
196+
assertEquals("Certificate contains multiple URI SANs", e.getMessage());
197+
}
198+
}
199+
183200
@Test
184201
void testGetTrustDomain() throws Exception {
185202
final CertAndKeyPair rootCa = createRootCA("C = US, O = SPIFFE", "spiffe://domain.test");

‎java-spiffe-core/src/test/java/io/spiffe/svid/x509svid/X509SvidValidatorTest.java‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import io.spiffe.utils.CertAndKeyPair;
99
import org.junit.jupiter.api.BeforeEach;
1010
import org.junit.jupiter.api.Test;
11+
import org.mockito.Mockito;
1112

1213
import java.io.IOException;
1314
import java.net.URISyntaxException;
@@ -114,6 +115,70 @@ void verifySpiffeId_givenAnEmptySupplier_throwsCertificateException() {
114115

115116
}
116117

118+
@Test
119+
void verifySpiffeId_leafWithCaFlagSetToTrue_throwsCertificateException() throws Exception {
120+
X509Certificate certificate = mockLeafCertificate("spiffe://example.org/test", false, false, true);
121+
try {
122+
X509SvidValidator.verifySpiffeId(certificate, () -> Sets.newHashSet(SpiffeId.parse("spiffe://example.org/test")));
123+
fail("Should have thrown CertificateException");
124+
} catch (CertificateException e) {
125+
assertEquals("Leaf certificate must not have CA flag set to true", e.getMessage());
126+
}
127+
}
128+
129+
@Test
130+
void verifySpiffeId_leafWithKeyCertSign_throwsCertificateException() throws Exception {
131+
X509Certificate certificate = mockLeafCertificate("spiffe://example.org/test", true, false, false);
132+
try {
133+
X509SvidValidator.verifySpiffeId(certificate, () -> Sets.newHashSet(SpiffeId.parse("spiffe://example.org/test")));
134+
fail("Should have thrown CertificateException");
135+
} catch (CertificateException e) {
136+
assertEquals("Leaf certificate must not have 'keyCertSign' as key usage", e.getMessage());
137+
}
138+
}
139+
140+
@Test
141+
void verifySpiffeId_leafWithCRLSign_throwsCertificateException() throws Exception {
142+
X509Certificate certificate = mockLeafCertificate("spiffe://example.org/test", false, true, false);
143+
try {
144+
X509SvidValidator.verifySpiffeId(certificate, () -> Sets.newHashSet(SpiffeId.parse("spiffe://example.org/test")));
145+
fail("Should have thrown CertificateException");
146+
} catch (CertificateException e) {
147+
assertEquals("Leaf certificate must not have 'cRLSign' as key usage", e.getMessage());
148+
}
149+
}
150+
151+
@Test
152+
void verifySpiffeId_leafSpiffeIdWithoutPath_throwsCertificateException() throws Exception {
153+
X509Certificate certificate = mockLeafCertificate("spiffe://example.org", false, false, false);
154+
try {
155+
X509SvidValidator.verifySpiffeId(certificate, () -> Sets.newHashSet(SpiffeId.parse("spiffe://example.org")));
156+
fail("Should have thrown CertificateException");
157+
} catch (CertificateException e) {
158+
assertEquals("Leaf certificate SPIFFE ID must have a non-root path", e.getMessage());
159+
}
160+
}
161+
162+
@Test
163+
void verifySpiffeId_multipleUriSans_throwsCertificateException() throws Exception {
164+
X509Certificate certificate = Mockito.mock(X509Certificate.class);
165+
boolean[] keyUsage = new boolean[9];
166+
keyUsage[0] = true;
167+
Mockito.when(certificate.getBasicConstraints()).thenReturn(-1);
168+
Mockito.when(certificate.getKeyUsage()).thenReturn(keyUsage);
169+
Mockito.when(certificate.getSubjectAlternativeNames()).thenReturn(Arrays.asList(
170+
Arrays.asList(6, "spiffe://example.org/test"),
171+
Arrays.asList(6, "https://example.org/other")
172+
));
173+
174+
try {
175+
X509SvidValidator.verifySpiffeId(certificate, () -> Sets.newHashSet(SpiffeId.parse("spiffe://example.org/test")));
176+
fail("Should have thrown CertificateException");
177+
} catch (CertificateException e) {
178+
assertEquals("Certificate contains multiple URI SANs", e.getMessage());
179+
}
180+
}
181+
117182
@Test
118183
void checkSpiffeId_nullX509Certificate_throwsNullPointerException() throws CertificateException {
119184
try {
@@ -153,4 +218,20 @@ void verifyChain_nullBundleSource_throwsNullPointerException() throws Certificat
153218
assertEquals("x509BundleSource must not be null", e.getMessage());
154219
}
155220
}
221+
222+
private X509Certificate mockLeafCertificate(String uriSan, boolean keyCertSign, boolean crlSign, boolean isCa)
223+
throws Exception {
224+
X509Certificate certificate = Mockito.mock(X509Certificate.class);
225+
boolean[] keyUsage = new boolean[9];
226+
keyUsage[0] = true;
227+
keyUsage[5] = keyCertSign;
228+
keyUsage[6] = crlSign;
229+
230+
Mockito.when(certificate.getBasicConstraints()).thenReturn(isCa ? 0 : -1);
231+
Mockito.when(certificate.getKeyUsage()).thenReturn(keyUsage);
232+
Mockito.when(certificate.getSubjectAlternativeNames()).thenReturn(Collections.singletonList(
233+
Arrays.asList(6, uriSan)
234+
));
235+
return certificate;
236+
}
156237
}

0 commit comments

Comments
 (0)