Skip to content

fix(core): harden SPIFFE URI SAN parsing #434

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

Open
maxlambrecht wants to merge 1 commit into
base: main
Choose a base branch
from
+71 −6
Open

fix(core): harden SPIFFE URI SAN parsing #434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 10 additions & 3 deletions java-spiffe-core/src/main/java/io/spiffe/internal/CertificateUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.spiffe.internal;

import io.spiffe.exception.InvalidSpiffeIdException;
import io.spiffe.spiffeid.SpiffeId;
import io.spiffe.spiffeid.TrustDomain;

Expand All @@ -24,15 +25,16 @@
import static io.spiffe.internal.KeyUsage.CRL_SIGN;
import static io.spiffe.internal.KeyUsage.DIGITAL_SIGNATURE;
import static io.spiffe.internal.KeyUsage.KEY_CERT_SIGN;
import static org.apache.commons.lang3.StringUtils.startsWith;

/**
* Common certificate utility methods.
*/
public class CertificateUtils {

private static final String SPIFFE_PREFIX = "spiffe://";
private static final int SAN_TYPE_INDEX = 0;
private static final int SAN_VALUE_INDEX = 1;
private static final int URI_SAN_TYPE = 6;
private static final String PUBLIC_KEY_INFRASTRUCTURE_ALGORITHM = "PKIX";
private static final String X509_CERTIFICATE_TYPE = "X.509";

Expand Down Expand Up @@ -122,7 +124,11 @@ public static SpiffeId getSpiffeId(final X509Certificate certificate) throws Cer
throw new CertificateException("Certificate does not contain SPIFFE ID in the URI SAN");
}

return SpiffeId.parse(spiffeIds.get(0));
try {
return SpiffeId.parse(spiffeIds.get(0));
} catch (InvalidSpiffeIdException | IllegalArgumentException e) {
throw new CertificateException("Certificate contains invalid SPIFFE ID in the URI SAN", e);
}
}

/**
Expand Down Expand Up @@ -182,8 +188,9 @@ private static List<String> getSpiffeIds(final X509Certificate certificate) thro
}
return certificate.getSubjectAlternativeNames()
.stream()
.filter(san -> URI_SAN_TYPE == (Integer) san.get(SAN_TYPE_INDEX))
.map(san -> (String) san.get(SAN_VALUE_INDEX))
.filter(uri -> startsWith(uri, SPIFFE_PREFIX))
.filter(uri -> uri != null && uri.startsWith(SPIFFE_PREFIX))
.collect(Collectors.toList());
}

Expand Down
53 changes: 53 additions & 0 deletions java-spiffe-core/src/test/java/io/spiffe/internal/CertificateUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.security.cert.X509Certificate;
import java.security.spec.InvalidKeySpecException;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;

Expand All @@ -33,6 +34,8 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.fail;

public class CertificateUtilsTest {
Expand Down Expand Up @@ -170,6 +173,43 @@ void testGetSpiffeId() throws Exception {
assertEquals(SpiffeId.parse("spiffe://domain.test/workload"), spiffeId);
}

@Test
void testGetSpiffeId_certWithDnsAndIpSans_doesNotThrowClassCastException() throws Exception {
X509Certificate certificate = new CertificateWithSubjectAlternativeNames(Arrays.asList(
Arrays.asList(6, "spiffe://domain.test/workload"),
Arrays.asList(2, "workload.domain.test"),
Arrays.asList(7, "127.0.0.1")
));

SpiffeId spiffeId = CertificateUtils.getSpiffeId(certificate);

assertEquals(SpiffeId.parse("spiffe://domain.test/workload"), spiffeId);
}

@Test
void testGetSpiffeId_certWithNonStringSanType_doesNotThrowClassCastException() throws Exception {
X509Certificate certificate = new CertificateWithSubjectAlternativeNames(Arrays.asList(
Arrays.asList(6, "spiffe://domain.test/workload"),
Arrays.asList(0, new byte[]{1, 2, 3})
));

SpiffeId spiffeId = CertificateUtils.getSpiffeId(certificate);

assertEquals(SpiffeId.parse("spiffe://domain.test/workload"), spiffeId);
}

@Test
void testGetSpiffeId_certWithMalformedSpiffeUriSan_throwsCertificateException() throws Exception {
X509Certificate certificate = new CertificateWithSubjectAlternativeNames(Collections.singletonList(
Arrays.asList(6, "spiffe://domain.test/workload/invalid path")
));

CertificateException exception = assertThrows(CertificateException.class, () -> CertificateUtils.getSpiffeId(certificate));

assertEquals("Certificate contains invalid SPIFFE ID in the URI SAN", exception.getMessage());
assertInstanceOf(RuntimeException.class, exception.getCause());
}

@Test
void testGetSpiffeId_certNotContainSpiffeId_throwsCertificateException() throws Exception {
final CertAndKeyPair rootCa = createRootCA("C = US, O = SPIFFE", "spiffe://domain.test");
Expand Down Expand Up @@ -215,4 +255,17 @@ void keyUsageChecks_noKeyUsageExtension() throws Exception {
assertFalse(CertificateUtils.hasKeyUsageCertSign(certificate));
assertFalse(CertificateUtils.hasKeyUsageCRLSign(certificate));
}

private static class CertificateWithSubjectAlternativeNames extends DummyX509Certificate {
private final Collection<List<?>> subjectAlternativeNames;

private CertificateWithSubjectAlternativeNames(Collection<List<?>> subjectAlternativeNames) {
this.subjectAlternativeNames = subjectAlternativeNames;
}

@Override
public Collection<List<?>> getSubjectAlternativeNames() throws CertificateParsingException {
return subjectAlternativeNames;
}
}
}
11 changes: 8 additions & 3 deletions java-spiffe-core/src/test/java/io/spiffe/svid/x509svid/X509SvidTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.util.Arrays;
import java.util.stream.Stream;
Expand All @@ -25,6 +26,7 @@
import static io.spiffe.utils.X509CertificateTestUtils.createCertificateWithUriSans;
import static io.spiffe.utils.X509CertificateTestUtils.createRootCA;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.fail;
Expand Down Expand Up @@ -346,12 +348,15 @@ void parseRaw_leafSpiffeIdWithRootOnlyPath_isRejected() throws Exception {
byte[] certBytes = leaf.getCertificate().getEncoded();
byte[] keyBytes = leaf.getKeyPair().getPrivate().getEncoded();

InvalidSpiffeIdException exception = assertThrows(
InvalidSpiffeIdException.class,
X509SvidException exception = assertThrows(
X509SvidException.class,
() -> X509Svid.parseRaw(certBytes, keyBytes)
);

assertEquals("Path cannot have a trailing slash", exception.getMessage());
assertEquals("Certificate contains invalid SPIFFE ID in the URI SAN", exception.getMessage());
assertInstanceOf(CertificateException.class, exception.getCause());
assertInstanceOf(InvalidSpiffeIdException.class, exception.getCause().getCause());
assertEquals("Path cannot have a trailing slash", exception.getCause().getCause().getMessage());
}

@Test
Expand Down