Skip to content

Commit c2bd558

Browse files
maxlambrechtmaxlambrecht
committed
address PR comments
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
1 parent ad79293 commit c2bd558

4 files changed

Lines changed: 30 additions & 42 deletions

File tree

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

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -106,22 +106,16 @@ public static void validate(final List<X509Certificate> chain, final Collection<
106106
/**
107107
* Extracts the SPIFFE ID from an X.509 certificate.
108108
* <p>
109-
* It iterates over the list of SubjectAlternativesNames, read each entry, takes the value from the index
110-
* defined in SAN_VALUE_INDEX and filters the entries that starts with the SPIFFE_PREFIX and returns the first.
109+
* It inspects the URI Subject Alternative Name entries and parses the single SPIFFE ID value when present.
111110
*
112111
* @param certificate a {@link X509Certificate}
113112
* @return an instance of a {@link SpiffeId}
114-
* @throws CertificateException if the certificate contains multiple SPIFFE IDs, or does not contain any, or
115-
* the SAN extension cannot be decoded
113+
* @throws CertificateException if the certificate contains multiple URI SANs, does not contain a SPIFFE ID
114+
* in the URI SAN, or the SAN extension cannot be decoded
116115
*/
117116
public static SpiffeId getSpiffeId(final X509Certificate certificate) throws CertificateException {
118117
List<String> spiffeIds = getSpiffeIds(certificate);
119-
120-
if (spiffeIds.size() > 1) {
121-
throw new CertificateException("Certificate contains multiple SPIFFE IDs");
122-
}
123-
124-
if (spiffeIds.size() < 1) {
118+
if (spiffeIds.isEmpty()) {
125119
throw new CertificateException("Certificate does not contain SPIFFE ID in the URI SAN");
126120
}
127121

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ private static X509Svid createX509Svid(final byte[] certsBytes,
218218
final SpiffeId spiffeId = getSpiffeId(x509Certificates);
219219

220220
try {
221-
X509SvidChecks.validateLeafForParsing(x509Certificates.get(0), spiffeId);
221+
X509SvidChecks.validateLeafConstraints(x509Certificates.get(0), spiffeId);
222222

223223
// there are intermediate CA certificates
224224
if (x509Certificates.size() > 1) {

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

Lines changed: 4 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,10 @@ private X509SvidChecks() {
2828
* Contract:
2929
* <ul>
3030
* <li>The certificate is treated as a leaf and MUST NOT be a CA certificate.</li>
31+
* <li>The certificate MUST set {@code digitalSignature} key usage.</li>
3132
* <li>The certificate MUST NOT set {@code keyCertSign} or {@code cRLSign} key usages.</li>
3233
* <li>The SPIFFE ID MUST have a non-root path component.</li>
3334
* </ul>
34-
* This method intentionally excludes parse-only requirements (for example, requiring
35-
* {@code digitalSignature}, which is defined in section 4.3) so callers can reuse this
36-
* baseline policy in multiple contexts.
3735
*
3836
* @param leaf leaf certificate to validate
3937
* @param spiffeId parsed SPIFFE ID from the leaf certificate
@@ -47,6 +45,9 @@ static void validateLeafConstraints(final X509Certificate leaf, final SpiffeId s
4745
if (CertificateUtils.isCA(leaf)) {
4846
throw new CertificateException("Leaf certificate must not have CA flag set to true");
4947
}
48+
if (!CertificateUtils.hasKeyUsageDigitalSignature(leaf)) {
49+
throw new CertificateException("Leaf certificate must have 'digitalSignature' as key usage");
50+
}
5051
if (CertificateUtils.hasKeyUsageCertSign(leaf)) {
5152
throw new CertificateException("Leaf certificate must not have 'keyCertSign' as key usage");
5253
}
@@ -60,28 +61,6 @@ static void validateLeafConstraints(final X509Certificate leaf, final SpiffeId s
6061
}
6162
}
6263

63-
/**
64-
* Validates leaf-certificate requirements for {@link X509Svid} parsing.
65-
* <p>
66-
* Contract:
67-
* <ul>
68-
* <li>Applies all checks from {@link #validateLeafConstraints(X509Certificate, SpiffeId)}.</li>
69-
* <li>Additionally requires {@code digitalSignature} key usage to be set (section 4.3).</li>
70-
* </ul>
71-
* This parse-time contract is intentionally stricter than authentication-time baseline checks.
72-
*
73-
* @param leaf leaf certificate to validate
74-
* @param spiffeId parsed SPIFFE ID from the leaf certificate
75-
* @throws CertificateException if any parse-time leaf requirement is violated
76-
*/
77-
static void validateLeafForParsing(final X509Certificate leaf, final SpiffeId spiffeId)
78-
throws CertificateException {
79-
if (!CertificateUtils.hasKeyUsageDigitalSignature(leaf)) {
80-
throw new CertificateException("Leaf certificate must have 'digitalSignature' as key usage");
81-
}
82-
validateLeafConstraints(leaf, spiffeId);
83-
}
84-
8564
static void validateSigningCertificate(final X509Certificate cert) throws CertificateException {
8665
Objects.requireNonNull(cert, "cert must not be null");
8766

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

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ void verifySpiffeId_givenAnEmptySupplier_throwsCertificateException() {
117117

118118
@Test
119119
void verifySpiffeId_leafWithCaFlagSetToTrue_throwsCertificateException() throws Exception {
120-
X509Certificate certificate = mockLeafCertificate("spiffe://example.org/test", false, false, true);
120+
X509Certificate certificate = mockLeafCertificate("spiffe://example.org/test", true, false, false, true);
121121
try {
122122
X509SvidValidator.verifySpiffeId(certificate, () -> Sets.newHashSet(SpiffeId.parse("spiffe://example.org/test")));
123123
fail("Should have thrown CertificateException");
@@ -126,9 +126,20 @@ void verifySpiffeId_leafWithCaFlagSetToTrue_throwsCertificateException() throws
126126
}
127127
}
128128

129+
@Test
130+
void verifySpiffeId_leafWithoutDigitalSignature_throwsCertificateException() throws Exception {
131+
X509Certificate certificate = mockLeafCertificate("spiffe://example.org/test", false, false, 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 have 'digitalSignature' as key usage", e.getMessage());
137+
}
138+
}
139+
129140
@Test
130141
void verifySpiffeId_leafWithKeyCertSign_throwsCertificateException() throws Exception {
131-
X509Certificate certificate = mockLeafCertificate("spiffe://example.org/test", true, false, false);
142+
X509Certificate certificate = mockLeafCertificate("spiffe://example.org/test", true, true, false, false);
132143
try {
133144
X509SvidValidator.verifySpiffeId(certificate, () -> Sets.newHashSet(SpiffeId.parse("spiffe://example.org/test")));
134145
fail("Should have thrown CertificateException");
@@ -139,7 +150,7 @@ void verifySpiffeId_leafWithKeyCertSign_throwsCertificateException() throws Exce
139150

140151
@Test
141152
void verifySpiffeId_leafWithCRLSign_throwsCertificateException() throws Exception {
142-
X509Certificate certificate = mockLeafCertificate("spiffe://example.org/test", false, true, false);
153+
X509Certificate certificate = mockLeafCertificate("spiffe://example.org/test", true, false, true, false);
143154
try {
144155
X509SvidValidator.verifySpiffeId(certificate, () -> Sets.newHashSet(SpiffeId.parse("spiffe://example.org/test")));
145156
fail("Should have thrown CertificateException");
@@ -150,7 +161,7 @@ void verifySpiffeId_leafWithCRLSign_throwsCertificateException() throws Exceptio
150161

151162
@Test
152163
void verifySpiffeId_leafSpiffeIdWithoutPath_throwsCertificateException() throws Exception {
153-
X509Certificate certificate = mockLeafCertificate("spiffe://example.org", false, false, false);
164+
X509Certificate certificate = mockLeafCertificate("spiffe://example.org", true, false, false, false);
154165
try {
155166
X509SvidValidator.verifySpiffeId(certificate, () -> Sets.newHashSet(SpiffeId.parse("spiffe://example.org")));
156167
fail("Should have thrown CertificateException");
@@ -219,11 +230,15 @@ void verifyChain_nullBundleSource_throwsNullPointerException() throws Certificat
219230
}
220231
}
221232

222-
private X509Certificate mockLeafCertificate(String uriSan, boolean keyCertSign, boolean crlSign, boolean isCa)
233+
private X509Certificate mockLeafCertificate(String uriSan,
234+
boolean digitalSignature,
235+
boolean keyCertSign,
236+
boolean crlSign,
237+
boolean isCa)
223238
throws Exception {
224239
X509Certificate certificate = Mockito.mock(X509Certificate.class);
225240
boolean[] keyUsage = new boolean[9];
226-
keyUsage[0] = true;
241+
keyUsage[0] = digitalSignature;
227242
keyUsage[5] = keyCertSign;
228243
keyUsage[6] = crlSign;
229244

0 commit comments

Comments
 (0)