Skip to content

Commit 3ee95b6

Browse files
mrtsmrts
committed
Backport OCSP update time validation and thread-safety and CA certificate expiration fixes and CertificateData Optional API from main
WE2-1132 Signed-off-by: Mart Somermaa <[email protected]>
1 parent 6a8df7e commit 3ee95b6

27 files changed

+517
-425
lines changed

‎pom.xml‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,17 @@
1212

1313
<properties>
1414
<maven.version>3.3.9</maven.version>
15-
<maven-surefire-plugin.version>2.22.2</maven-surefire-plugin.version>
15+
<maven-surefire-plugin.version>3.5.4</maven-surefire-plugin.version>
1616
<java.version>1.8</java.version>
1717
<jjwt.version>0.11.5</jjwt.version>
18-
<jackson.version>2.13.4.2</jackson.version>
18+
<jackson.version>2.18.5</jackson.version>
1919
<slf4j.version>1.7.36</slf4j.version>
2020
<bouncycastle.version>1.70</bouncycastle.version>
2121
<okhttp.version>4.10.0</okhttp.version>
22-
<junit-jupiter.version>5.8.2</junit-jupiter.version>
23-
<assertj.version>3.23.1</assertj.version>
24-
<mockito.version>4.6.1</mockito.version>
25-
<jacoco.version>0.8.5</jacoco.version>
22+
<junit-jupiter.version>5.12.0</junit-jupiter.version>
23+
<assertj.version>3.27.3</assertj.version>
24+
<mockito.version>4.11.0</mockito.version>
25+
<jacoco.version>0.8.12</jacoco.version>
2626
<sonar.coverage.jacoco.xmlReportPaths>
2727
${project.basedir}/../jacoco-coverage-report/target/site/jacoco-aggregate/jacoco.xml
2828
</sonar.coverage.jacoco.xmlReportPaths>
@@ -84,7 +84,7 @@
8484
</dependency>
8585
<dependency>
8686
<groupId>org.mockito</groupId>
87-
<artifactId>mockito-core</artifactId>
87+
<artifactId>mockito-inline</artifactId>
8888
<version>${mockito.version}</version>
8989
<scope>test</scope>
9090
</dependency>
@@ -129,7 +129,7 @@
129129

130130
<!-- For publishing the library to GitHub Packages/GitLab Package Repository -->
131131
<distributionManagement>
132-
<!-- Github Packages does not currently support public access, so disabled until it does.
132+
<!-- GitHub Packages does not currently support public access, so disabled until it does.
133133
See https://github.community/t/download-from-github-package-registry-without-authentication/14407
134134
<repository>
135135
<id>github</id>

‎src/main/java/eu/webeid/security/certificate/CertificateData.java‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -32,43 +32,44 @@
3232
import java.security.cert.CertificateEncodingException;
3333
import java.security.cert.X509Certificate;
3434
import java.util.Arrays;
35+
import java.util.Optional;
3536
import java.util.stream.Collectors;
3637

3738
public final class CertificateData {
3839

39-
public static String getSubjectCN(X509Certificate certificate) throws CertificateEncodingException {
40+
public static Optional<String> getSubjectCN(X509Certificate certificate) throws CertificateEncodingException {
4041
return getSubjectField(certificate, BCStyle.CN);
4142
}
4243

43-
public static String getSubjectSurname(X509Certificate certificate) throws CertificateEncodingException {
44+
public static Optional<String> getSubjectSurname(X509Certificate certificate) throws CertificateEncodingException {
4445
return getSubjectField(certificate, BCStyle.SURNAME);
4546
}
4647

47-
public static String getSubjectGivenName(X509Certificate certificate) throws CertificateEncodingException {
48+
public static Optional<String> getSubjectGivenName(X509Certificate certificate) throws CertificateEncodingException {
4849
return getSubjectField(certificate, BCStyle.GIVENNAME);
4950
}
5051

51-
public static String getSubjectIdCode(X509Certificate certificate) throws CertificateEncodingException {
52+
public static Optional<String> getSubjectIdCode(X509Certificate certificate) throws CertificateEncodingException {
5253
return getSubjectField(certificate, BCStyle.SERIALNUMBER);
5354
}
5455

55-
public static String getSubjectCountryCode(X509Certificate certificate) throws CertificateEncodingException {
56+
public static Optional<String> getSubjectCountryCode(X509Certificate certificate) throws CertificateEncodingException {
5657
return getSubjectField(certificate, BCStyle.C);
5758
}
5859

59-
private static String getSubjectField(X509Certificate certificate, ASN1ObjectIdentifier fieldId) throws CertificateEncodingException {
60+
private static Optional<String> getSubjectField(X509Certificate certificate, ASN1ObjectIdentifier fieldId) throws CertificateEncodingException {
6061
return getField(new JcaX509CertificateHolder(certificate).getSubject(), fieldId);
6162
}
6263

63-
private static String getField(X500Name x500Name, ASN1ObjectIdentifier fieldId) throws CertificateEncodingException {
64+
private static Optional<String> getField(X500Name x500Name, ASN1ObjectIdentifier fieldId) {
6465
// Example value: [C=EE, CN=JÕEORG\,JAAK-KRISTJAN\,38001085718, 2.5.4.4=#0c074ac395454f5247, 2.5.4.42=#0c0d4a41414b2d4b524953544a414e, 2.5.4.5=#1311504e4f45452d3338303031303835373138]
6566
final RDN[] rdns = x500Name.getRDNs(fieldId);
6667
if (rdns.length == 0 || rdns[0].getFirst() == null) {
67-
throw new CertificateEncodingException("X500 name RDNs empty or first element is null");
68+
return Optional.empty();
6869
}
69-
return Arrays.stream(rdns)
70+
return Optional.of(Arrays.stream(rdns)
7071
.map(rdn -> IETFUtils.valueToString(rdn.getFirst().getValue()))
71-
.collect(Collectors.joining(", "));
72+
.collect(Collectors.joining(", ")));
7273
}
7374

7475
private CertificateData() {

‎src/main/java/eu/webeid/security/certificate/CertificateValidator.java‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,9 @@
2323
package eu.webeid.security.certificate;
2424

2525
import eu.webeid.security.exceptions.CertificateExpiredException;
26+
import eu.webeid.security.exceptions.CertificateNotTrustedException;
2627
import eu.webeid.security.exceptions.CertificateNotYetValidException;
2728
import eu.webeid.security.exceptions.JceException;
28-
import eu.webeid.security.exceptions.CertificateNotTrustedException;
2929

3030
import java.security.GeneralSecurityException;
3131
import java.security.InvalidAlgorithmParameterException;
@@ -57,31 +57,32 @@ public static void certificateIsValidOnDate(X509Certificate cert, Date date, Str
5757
}
5858
}
5959

60-
public static void trustedCACertificatesAreValidOnDate(Set<TrustAnchor> trustedCACertificateAnchors, Date date) throws CertificateNotYetValidException, CertificateExpiredException {
61-
for (TrustAnchor cert : trustedCACertificateAnchors) {
62-
certificateIsValidOnDate(cert.getTrustedCert(), date, "Trusted CA");
63-
}
64-
}
65-
6660
public static X509Certificate validateIsSignedByTrustedCA(X509Certificate certificate,
6761
Set<TrustAnchor> trustedCACertificateAnchors,
6862
CertStore trustedCACertificateCertStore,
69-
Date date) throws CertificateNotTrustedException, JceException {
63+
Date now) throws CertificateNotTrustedException, JceException, CertificateNotYetValidException, CertificateExpiredException {
64+
certificateIsValidOnDate(certificate, now, "User");
65+
7066
final X509CertSelector selector = new X509CertSelector();
7167
selector.setCertificate(certificate);
7268

7369
try {
7470
final PKIXBuilderParameters pkixBuilderParameters = new PKIXBuilderParameters(trustedCACertificateAnchors, selector);
7571
// Certificate revocation check is intentionally disabled as we do the OCSP check with SubjectCertificateNotRevokedValidator ourselves.
7672
pkixBuilderParameters.setRevocationEnabled(false);
77-
pkixBuilderParameters.setDate(date);
73+
pkixBuilderParameters.setDate(now);
7874
pkixBuilderParameters.addCertStore(trustedCACertificateCertStore);
7975

8076
// See the comment in buildCertStoreFromCertificates() below why we use the default JCE provider.
8177
final CertPathBuilder certPathBuilder = CertPathBuilder.getInstance(CertPathBuilder.getDefaultType());
8278
final PKIXCertPathBuilderResult result = (PKIXCertPathBuilderResult) certPathBuilder.build(pkixBuilderParameters);
8379

84-
return result.getTrustAnchor().getTrustedCert();
80+
final X509Certificate trustedCACert = result.getTrustAnchor().getTrustedCert();
81+
82+
// Verify that the trusted CA cert is presently valid before returning the result.
83+
certificateIsValidOnDate(trustedCACert, now, "Trusted CA");
84+
85+
return trustedCACert;
8586

8687
} catch (InvalidAlgorithmParameterException | NoSuchAlgorithmException e) {
8788
throw new JceException(e);

‎src/main/java/eu/webeid/security/util/DateAndTime.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,14 @@ public static void requirePositiveDuration(Duration duration, String fieldName)
4545

4646
public static class DefaultClock implements Clock {
4747

48-
public static final Clock INSTANCE = new DefaultClock();
48+
// Allows mocking of time-dependent behavior with Mockito.mockStatic() in tests.
49+
private static final Clock instance = new DefaultClock();
4950

51+
public static Clock getInstance() {
52+
return instance;
53+
}
54+
55+
@Override
5056
public Date now() {
5157
return new Date();
5258
}

‎src/main/java/eu/webeid/security/validator/AuthTokenValidationConfiguration.java‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838

3939
import static eu.webeid.security.util.Collections.newHashSet;
4040
import static eu.webeid.security.util.DateAndTime.requirePositiveDuration;
41-
import static eu.webeid.security.validator.ocsp.OcspUrl.AIA_ESTEID_2015;
4241

4342
/**
4443
* Stores configuration parameters for {@link AuthTokenValidatorImpl}.
@@ -49,6 +48,8 @@ public final class AuthTokenValidationConfiguration {
4948
private Collection<X509Certificate> trustedCACertificates = new HashSet<>();
5049
private boolean isUserCertificateRevocationCheckWithOcspEnabled = true;
5150
private Duration ocspRequestTimeout = Duration.ofSeconds(5);
51+
private Duration allowedOcspResponseTimeSkew = Duration.ofMinutes(15);
52+
private Duration maxOcspResponseThisUpdateAge = Duration.ofMinutes(2);
5253
private DesignatedOcspServiceConfiguration designatedOcspServiceConfiguration;
5354
// Don't allow Estonian Mobile-ID policy by default.
5455
private Collection<ASN1ObjectIdentifier> disallowedSubjectCertificatePolicies = newHashSet(
@@ -57,8 +58,7 @@ public final class AuthTokenValidationConfiguration {
5758
SubjectCertificatePolicies.ESTEID_SK_2015_MOBILE_ID_POLICY_V3,
5859
SubjectCertificatePolicies.ESTEID_SK_2015_MOBILE_ID_POLICY
5960
);
60-
// Disable OCSP nonce extension for EstEID 2015 cards by default.
61-
private Collection<URI> nonceDisabledOcspUrls = newHashSet(AIA_ESTEID_2015);
61+
private Collection<URI> nonceDisabledOcspUrls = new HashSet<>();
6262

6363
AuthTokenValidationConfiguration() {
6464
}
@@ -68,6 +68,8 @@ private AuthTokenValidationConfiguration(AuthTokenValidationConfiguration other)
6868
this.trustedCACertificates = Collections.unmodifiableSet(new HashSet<>(other.trustedCACertificates));
6969
this.isUserCertificateRevocationCheckWithOcspEnabled = other.isUserCertificateRevocationCheckWithOcspEnabled;
7070
this.ocspRequestTimeout = other.ocspRequestTimeout;
71+
this.allowedOcspResponseTimeSkew = other.allowedOcspResponseTimeSkew;
72+
this.maxOcspResponseThisUpdateAge = other.maxOcspResponseThisUpdateAge;
7173
this.designatedOcspServiceConfiguration = other.designatedOcspServiceConfiguration;
7274
this.disallowedSubjectCertificatePolicies = Collections.unmodifiableSet(new HashSet<>(other.disallowedSubjectCertificatePolicies));
7375
this.nonceDisabledOcspUrls = Collections.unmodifiableSet(new HashSet<>(other.nonceDisabledOcspUrls));
@@ -101,6 +103,22 @@ void setOcspRequestTimeout(Duration ocspRequestTimeout) {
101103
this.ocspRequestTimeout = ocspRequestTimeout;
102104
}
103105

106+
public Duration getAllowedOcspResponseTimeSkew() {
107+
return allowedOcspResponseTimeSkew;
108+
}
109+
110+
public void setAllowedOcspResponseTimeSkew(Duration allowedOcspResponseTimeSkew) {
111+
this.allowedOcspResponseTimeSkew = allowedOcspResponseTimeSkew;
112+
}
113+
114+
public Duration getMaxOcspResponseThisUpdateAge() {
115+
return maxOcspResponseThisUpdateAge;
116+
}
117+
118+
public void setMaxOcspResponseThisUpdateAge(Duration maxOcspResponseThisUpdateAge) {
119+
this.maxOcspResponseThisUpdateAge = maxOcspResponseThisUpdateAge;
120+
}
121+
104122
public DesignatedOcspServiceConfiguration getDesignatedOcspServiceConfiguration() {
105123
return designatedOcspServiceConfiguration;
106124
}
@@ -130,6 +148,8 @@ void validate() {
130148
throw new IllegalArgumentException("At least one trusted certificate authority must be provided");
131149
}
132150
requirePositiveDuration(ocspRequestTimeout, "OCSP request timeout");
151+
requirePositiveDuration(allowedOcspResponseTimeSkew, "Allowed OCSP response time-skew");
152+
requirePositiveDuration(maxOcspResponseThisUpdateAge, "Max OCSP response thisUpdate age");
133153
}
134154

135155
AuthTokenValidationConfiguration copy() {

‎src/main/java/eu/webeid/security/validator/AuthTokenValidatorBuilder.java‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ public AuthTokenValidatorBuilder withTrustedCertificateAuthorities(X509Certifica
7777
if (LOG.isDebugEnabled()) {
7878
LOG.debug("Trusted intermediate certificate authorities set to {}",
7979
configuration.getTrustedCACertificates().stream()
80-
.map(X509Certificate::getSubjectDN)
80+
.map(X509Certificate::getSubjectX500Principal)
8181
.collect(Collectors.toList()));
8282
}
8383
return this;
@@ -127,6 +127,38 @@ public AuthTokenValidatorBuilder withOcspRequestTimeout(Duration ocspRequestTime
127127
return this;
128128
}
129129

130+
/**
131+
* Sets the allowed time skew for OCSP response's thisUpdate and nextUpdate times.
132+
* This parameter is used to allow discrepancies between the system clock and the OCSP responder's clock,
133+
* which may occur due to clock drift, network delays or revocation updates that are not published in real time.
134+
* <p>
135+
* This is an optional configuration parameter, the default is 15 minutes.
136+
* The relatively long default is specifically chosen to account for one particular OCSP responder that used
137+
* CRLs for authoritative revocation info, these CRLs were updated every 15 minutes.
138+
*
139+
* @param allowedTimeSkew the allowed time skew
140+
* @return the builder instance for method chaining.
141+
*/
142+
public AuthTokenValidatorBuilder withAllowedOcspResponseTimeSkew(Duration allowedTimeSkew) {
143+
configuration.setAllowedOcspResponseTimeSkew(allowedTimeSkew);
144+
LOG.debug("Allowed OCSP response time skew set to {}", allowedTimeSkew);
145+
return this;
146+
}
147+
148+
/**
149+
* Sets the maximum age of the OCSP response's thisUpdate time before it is considered too old.
150+
* <p>
151+
* This is an optional configuration parameter, the default is 2 minutes.
152+
*
153+
* @param maxThisUpdateAge the maximum age of the OCSP response's thisUpdate time
154+
* @return the builder instance for method chaining.
155+
*/
156+
public AuthTokenValidatorBuilder withMaxOcspResponseThisUpdateAge(Duration maxThisUpdateAge) {
157+
configuration.setMaxOcspResponseThisUpdateAge(maxThisUpdateAge);
158+
LOG.debug("Maximum OCSP response thisUpdate age set to {}", maxThisUpdateAge);
159+
return this;
160+
}
161+
130162
/**
131163
* Adds the given URLs to the list of OCSP URLs for which the nonce protocol extension will be disabled.
132164
* The OCSP URL is extracted from the user certificate and some OCSP services don't support the nonce extension.

‎src/main/java/eu/webeid/security/validator/AuthTokenValidatorImpl.java‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,13 @@
2323
package eu.webeid.security.validator;
2424

2525
import com.fasterxml.jackson.databind.ObjectMapper;
26+
import com.fasterxml.jackson.databind.ObjectReader;
2627
import eu.webeid.security.authtoken.WebEidAuthToken;
2728
import eu.webeid.security.certificate.CertificateLoader;
2829
import eu.webeid.security.certificate.CertificateValidator;
2930
import eu.webeid.security.exceptions.JceException;
3031
import eu.webeid.security.exceptions.AuthTokenParseException;
3132
import eu.webeid.security.exceptions.AuthTokenException;
32-
import eu.webeid.security.validator.certvalidators.SubjectCertificateExpiryValidator;
3333
import eu.webeid.security.validator.certvalidators.SubjectCertificateNotRevokedValidator;
3434
import eu.webeid.security.validator.certvalidators.SubjectCertificatePolicyValidator;
3535
import eu.webeid.security.validator.certvalidators.SubjectCertificatePurposeValidator;
@@ -57,7 +57,7 @@ final class AuthTokenValidatorImpl implements AuthTokenValidator {
5757
private static final int TOKEN_MAX_LENGTH = 10000;
5858
private static final Logger LOG = LoggerFactory.getLogger(AuthTokenValidatorImpl.class);
5959

60-
private static final ObjectMapper objectMapper = new ObjectMapper();
60+
private static final ObjectReader OBJECT_READER = new ObjectMapper().readerFor(WebEidAuthToken.class);
6161

6262
private final AuthTokenValidationConfiguration configuration;
6363
private final SubjectCertificateValidatorBatch simpleSubjectCertificateValidators;
@@ -84,7 +84,6 @@ final class AuthTokenValidatorImpl implements AuthTokenValidator {
8484
trustedCACertificateCertStore = CertificateValidator.buildCertStoreFromCertificates(configuration.getTrustedCACertificates());
8585

8686
simpleSubjectCertificateValidators = SubjectCertificateValidatorBatch.createFrom(
87-
new SubjectCertificateExpiryValidator(trustedCACertificateAnchors)::validateCertificateExpiry,
8887
SubjectCertificatePurposeValidator::validateCertificatePurpose,
8988
new SubjectCertificatePolicyValidator(configuration.getDisallowedSubjectCertificatePolicies())::validateCertificatePolicies
9089
);
@@ -109,7 +108,7 @@ public WebEidAuthToken parse(String authToken) throws AuthTokenException {
109108
validateTokenLength(authToken);
110109
return parseToken(authToken);
111110
} catch (Exception e) {
112-
// Generally "log and rethrow" is an anti-pattern, but it fits with the surrounding logging style.
111+
// Generally "log and rethrow" is an antipattern, but it fits with the surrounding logging style.
113112
LOG.warn("Token parsing was interrupted:", e);
114113
throw e;
115114
}
@@ -121,7 +120,7 @@ public X509Certificate validate(WebEidAuthToken authToken, String currentChallen
121120
LOG.info("Starting token validation");
122121
return validateToken(authToken, currentChallengeNonce);
123122
} catch (Exception e) {
124-
// Generally "log and rethrow" is an anti-pattern, but it fits with the surrounding logging style.
123+
// Generally "log and rethrow" is an antipattern, but it fits with the surrounding logging style.
125124
LOG.warn("Token validation was interrupted:", e);
126125
throw e;
127126
}
@@ -138,7 +137,7 @@ private void validateTokenLength(String authToken) throws AuthTokenParseExceptio
138137

139138
private WebEidAuthToken parseToken(String authToken) throws AuthTokenParseException {
140139
try {
141-
final WebEidAuthToken token = objectMapper.readValue(authToken, WebEidAuthToken.class);
140+
final WebEidAuthToken token = OBJECT_READER.readValue(authToken);
142141
if (token == null) {
143142
throw new AuthTokenParseException("Web eID authentication token is null");
144143
}
@@ -185,7 +184,11 @@ private SubjectCertificateValidatorBatch getCertTrustValidators() {
185184
return SubjectCertificateValidatorBatch.createFrom(
186185
certTrustedValidator::validateCertificateTrusted
187186
).addOptional(configuration.isUserCertificateRevocationCheckWithOcspEnabled(),
188-
new SubjectCertificateNotRevokedValidator(certTrustedValidator, ocspClient, ocspServiceProvider)::validateCertificateNotRevoked
187+
new SubjectCertificateNotRevokedValidator(certTrustedValidator,
188+
ocspClient, ocspServiceProvider,
189+
configuration.getAllowedOcspResponseTimeSkew(),
190+
configuration.getMaxOcspResponseThisUpdateAge()
191+
)::validateCertificateNotRevoked
189192
);
190193
}
191194

0 commit comments

Comments
 (0)