On Wed, 20 May 2026 17:39:00 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. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Mikhail Yankelevich has updated the pull request incrementally with one > additional commit since the last revision: > > comments src/java.base/share/classes/java/security/cert/X509Certificate.java line 134: > 132: * the first and last dates (and times) on which the certificate > 133: * is valid. It is defined in > 134: * ASN.1 as: Can combine these 2 lines. 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 I prefer if we use the term "date" as this is what [RFC 5280 uses when defining the Validity field](https://www.rfc-editor.org/rfc/rfc5280#section-4.1.2.5), thus I think "date" will be more familiar to users. I suggest the following re-wording: "Checks that the given date (represented as an `Instant`) is within the certificate's ..." src/java.base/share/classes/java/security/cert/X509Certificate.java line 188: > 186: * > 187: * @throws CertificateExpiredException if the certificate has > expired > 188: * with respect to {@code Instant} supplied. s/to/to the/ src/java.base/share/classes/java/security/cert/X509Certificate.java line 188: > 186: * > 187: * @throws CertificateExpiredException if the certificate has > expired > 188: * with respect to {@code Instant} supplied. s/{@code Instant}/{@code instant}/ (to be consistent with checkValidity(Date). src/java.base/share/classes/java/security/cert/X509Certificate.java line 190: > 188: * with respect to {@code Instant} supplied. > 189: * @throws CertificateNotYetValidException if the certificate is > not > 190: * yet valid with respect to {@code Instant} supplied. s/to/to the/ src/java.base/share/classes/java/security/cert/X509Certificate.java line 190: > 188: * with respect to {@code Instant} supplied. > 189: * @throws CertificateNotYetValidException if the certificate is > not > 190: * yet valid with respect to {@code Instant} supplied. s/{@code Instant}/{@code instant}/ (to be consistent with `checkValidity(Date)`. src/java.base/share/classes/java/security/cert/X509Certificate.java line 191: > 189: * @throws CertificateNotYetValidException if the certificate is > not > 190: * yet valid with respect to {@code Instant} supplied. > 191: * @throws NullPointerException if the supplied instant is null. put code font around null. src/java.base/share/classes/java/security/cert/X509Certificate.java line 194: > 192: * > 193: * @see #checkValidity() > 194: * @since 27 Should be 28. src/java.base/share/classes/java/security/cert/X509Certificate.java line 354: > 352: * and returns the output as an {@code Instant} value. > 353: * If {@code getNotBefore()} returns {@code null}, this method > throws a > 354: * {@code NullPointerException} This sentence should be removed. `getNotBefore` should never return `null` because this field in the certificate must be included. If it does, this is an implementation bug. src/java.base/share/classes/java/security/cert/X509Certificate.java line 358: > 356: * @return the start date of the validity period (never {@code > null}). > 357: * @see #checkValidity() > 358: * @since 27 Should be 28. src/java.base/share/classes/java/security/cert/X509Certificate.java line 364: > 362: if (date == null) { > 363: throw new NullPointerException("notBefore is null"); > 364: } Remove these lines, see above for rationale. src/java.base/share/classes/java/security/cert/X509Certificate.java line 389: > 387: * and returns the output as an {@code Instant} value. > 388: * If {@code getNotAfter()} returns {@code null}, this method throws > a > 389: * {@code NullPointerException} This sentence should be removed. `getNotAfter` should never return `null` because this field in the certificate must be included. If it does, this is an implementation bug. src/java.base/share/classes/java/security/cert/X509Certificate.java line 391: > 389: * {@code NullPointerException} > 390: * > 391: * @return the end date of the validity period (never {@code null}). It is not necessary to say "never null", it is implied. src/java.base/share/classes/java/security/cert/X509Certificate.java line 393: > 391: * @return the end date of the validity period (never {@code null}). > 392: * @see #checkValidity() > 393: * @since 27 Should be 28. src/java.base/share/classes/java/security/cert/X509Certificate.java line 399: > 397: if (date == null) { > 398: throw new NullPointerException("notAfter is null"); > 399: } Remove these lines, see above for rationale. src/java.base/share/classes/sun/security/util/DerOutputStream.java line 566: > 564: DateTimeFormatter dateTimeFormatter = > 565: DateTimeFormatter.ofPattern(pattern, Locale.US); > 566: byte[] time = > (d.atZone(ZoneId.of("GMT")).format(dateTimeFormatter)) I don't think the outer parens are necessary. src/java.base/share/classes/sun/security/util/DerValue.java line 1110: > 1108: } > 1109: if (end - start < 11 || end - start > 17) > 1110: throw new IOException("DER UTC Time Instant length error"); Don't include "Instant" in the exception message, this is an implementation detail and does not need to be exposed to applications. Restore the previous message. src/java.base/share/classes/sun/security/util/DerValue.java line 1136: > 1134: } > 1135: if (end - start < 13) > 1136: throw new IOException("DER Generalized Instant length > error"); Don't include "Instant" in the exception message, this is an implementation detail and does not need to be exposed to applications. Restore the previous message. src/java.base/share/classes/sun/security/x509/CertificateValidity.java line 174: > 172: * we use the internal Instants rather than the passed in Instant > 173: * because someone could override the Instant methods isAfter() > 174: * and isBefore() to do something entirely different. This comment can be removed since Instant is final and these methods are only accessible internally. src/java.base/share/classes/sun/security/x509/PrivateKeyUsageExtension.java line 202: > 200: > 201: /** > 202: * Verify that the passed time is within the validity period. s/passed/passed in/ src/java.base/share/classes/sun/security/x509/PrivateKeyUsageExtension.java line 216: > 214: * we use the internal Dates rather than the passed in Date > 215: * because someone could override the Date methods after() > 216: * and before() to do something entirely different. This comment can be removed since Instant is final and these methods are only accessible internally. src/java.base/share/classes/sun/security/x509/PrivateKeyUsageExtension.java line 229: > 227: > 228: /** > 229: * Verify that the passed time is within the validity period. s/passed/passed in/ src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 543: > 541: */ > 542: @Override > 543: public void checkValidity(Instant instant) Put `@since 28` here to be consistent with other new methods that have been overridden. src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 691: > 689: * > 690: * @return the start instant of the validity period. > 691: * @since 27 Should be 28. src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 711: > 709: * > 710: * @return the end instant of the validity period. > 711: * @since 27 Should be 28. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3499109002 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498713734 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3499091107 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3499103311 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3499092014 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3499101667 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3499094528 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498972041 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3499048528 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498973148 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3499045210 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498995654 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3499000951 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498973998 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3499038516 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498669050 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498591496 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498595798 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498905389 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498780756 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498776479 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498778367 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498731491 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498736539 PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3498737388
