On Wed, 4 Mar 2026 14:22:08 GMT, Mikhail Yankelevich <[email protected]>
wrote:
> Adding new methods to `X509Certificate` to return `Instant` instead of `Date`
> as well as moving away from `Date` in internal packages wherever possible.
Some comments.
Some comments.
src/java.base/share/classes/java/security/cert/X509Certificate.java line 1:
> 1: /*
Add `@since 27` to all new methods.
src/java.base/share/classes/java/security/cert/X509Certificate.java line 173:
> 171:
> 172: /**
> 173: * Checks that the given Instant is within the certificate's
Use `instant`. This is not a type name. Unless you want `{@code Instant}`.
src/java.base/share/classes/java/security/cert/X509Certificate.java line 184:
> 182: * {@code Date} and calls {@code checkValidity(date)}.
> 183: *
> 184: * @param instant the Instant to check against to see if this
> certificate
Same comment as above.
src/java.base/share/classes/java/security/cert/X509Certificate.java line 185:
> 183: *
> 184: * @param instant the Instant to check against to see if this
> certificate
> 185: * is valid at that date/time.
`that instant`.
src/java.base/share/classes/java/security/cert/X509Certificate.java line 384:
> 382: return date == null ? null : date.toInstant();
> 383: }
> 384:
These 2 methods need `@since 27`.
src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line
1:
> 1: /*
The `getSelfCertificate` methods here are used in many places and we cannot
remove them in this PR. It's worth creating a new JBS issue to rewrite it.
src/java.base/share/classes/sun/security/util/DerOutputStream.java line 568:
> 566: }
> 567:
> 568: DateTimeFormatter dateTimeFormatter =
`tz` above is useless.
src/java.base/share/classes/sun/security/util/DerValue.java line 1100:
> 1098: }
> 1099: if (end - start < 11 || end - start > 17)
> 1100: throw new IOException("DER UTC Time length error");
No need to change the message. It's about the ASN.1 type. Same as below.
src/java.base/share/classes/sun/security/x509/CertificateValidity.java line 57:
> 55: // Returns the first time the certificate is valid.
> 56: public Date getNotBefore() {
> 57: return new Date(notBefore.toEpochMilli());
Why not `Date.fromInstant(notBefore)`?
src/java.base/share/classes/sun/security/x509/CertificateValidity.java line 83:
> 81: * not valid
> 82: */
> 83: public CertificateValidity(Date notBefore, Date notAfter) {
Is this constructor still useful?
Shall we remove the `getNotAfter` and `getNotBefore` methods?
src/java.base/share/classes/sun/security/x509/CertificateValidity.java line 133:
> 131: DerOutputStream pair = new DerOutputStream();
> 132:
> 133: if (notBefore.toEpochMilli() < YR_2050) {
This could overflow. Maybe compare seconds?
src/java.base/share/classes/sun/security/x509/CertificateValidity.java line 173:
> 171: throws CertificateNotYetValidException, CertificateExpiredException {
> 172: /*
> 173: * we use the internal Dates rather than the passed in Date
It's "instant" now.
src/java.base/share/classes/sun/security/x509/PrivateKeyUsageExtension.java
line 198:
> 196: public void valid()
> 197: throws CertificateNotYetValidException, CertificateExpiredException {
> 198: Date now = new Date();
Use `Instant`.
src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 508:
> 506: public void checkValidity()
> 507: throws CertificateExpiredException, CertificateNotYetValidException {
> 508: Instant instant = Instant.now().truncatedTo(ChronoUnit.MILLIS);
Why do this?
test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/common/DynamicKeyStoreUtil.java
line 238:
> 236: final long oneDayInTheFuture = currentTime + (24 * 60 * 60 *
> 1000);
> 237: final Instant startDate =
> Instant.ofEpochMilli(oneMinuteInThePast);
> 238: final Instant expiryDate =
> Instant.ofEpochMilli(oneDayInTheFuture);
Maybe just use `Instant.now().minusSeconds(60)` etc?
test/jdk/java/security/cert/X509Certificate/VerifyDefault.java line 179:
> 177: () -> cert.checkValidity(
> 178: Instant.parse("2012-05-26T16:47:26Z")));
> 179:
Maybe you can check both `Instant` and `Date`. After all, we still need to
support the old methods.
You can also add new test to check the consistency.
test/jdk/sun/security/pkcs/pkcs10/PKCS10AttrEncoding.java line 66:
> 64: Object[] values = {
> 65: ObjectIdentifier.of("1.2.3.4"),
> 66: new GregorianCalendar(1970, 1, 25, 8, 56,
> 7).getTime().toInstant(),
Is there a modern way to create an `Instant`? `LocalDateTime`?
test/jdk/sun/security/pkcs/pkcs7/SignerOrder.java line 249:
> 247: firstDate = Instant.now();
> 248: lastDate = Instant.ofEpochMilli(
> 249: firstDate.toEpochMilli() + validity + 1000);
Use `Instant::plus`.
test/jdk/sun/security/util/DerInputBuffer/TimeParsing.java line 133:
> 131: Instant d1Instant = decodeUTCInstant(b);
> 132: if (!d0.equals(d1)
> 133: && d0.toInstant().toEpochMilli() != d1Instant.toEpochMilli()) {
Why `&&`? You always call this with a fixed milliseconds value, instead of
random `now()`.
test/jdk/sun/security/util/DerOutputStream/DerTimeEncoding.java line 2:
> 1: /*
> 2: * Copyright (c) 2023, 2026 Oracle and/or its affiliates. All rights
> reserved.
Add `,` after 2026.
test/jdk/sun/security/x509/X509CertImpl/CertExtensions.java line 149:
> 147: Instant notAfter =
> 148: Instant.ofEpochMilli(System.currentTimeMillis() +
> 149: 365L * 24 * 60 * 60 * 1000); //
> Valid for 1 year
Use `Instant::plus`.
test/jdk/sun/security/x509/X509CertImpl/CertExtensions.java line 207:
> 205: cal.set(2014, 03, 10, 12, 30, 30);
> 206: cal.set(2000, 11, 15, 12, 30, 30);
> 207: Date lastDate = cal.getTime();
Rewrite `lastDate`. Don't know why it's set twice.
-------------
PR Review: https://git.openjdk.org/jdk/pull/30047#pullrequestreview-3889801355
PR Review: https://git.openjdk.org/jdk/pull/30047#pullrequestreview-3924441073
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2884148021
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2884126353
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2884138483
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2884136457
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2913993368
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2913669431
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2913582178
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2914035441
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2884167988
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2884174830
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2914046113
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2914030203
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2913591094
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2884180750
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2913598986
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2913608270
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2913620477
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2913625705
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2914017459
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2914051619
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2913630951
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r2913724774
