RFR: 8325766: Review seclibs tests for cert expiry [v4]

[Matthew Donovan]

On Tue, 3 Jun 2025 17:29:13 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> As a constructor, the code would look like this
>> 
>> 
>> public CertificateBuilder(String subjectName,
>>                               PublicKey publicKey, PublicKey caKey, KeyUsage... keyUsages)
>>             throws CertificateException, IOException {
>>         factory = CertificateFactory.getInstance("X.509");
>>         SecureRandom random = new SecureRandom();
>> 
>>         boolean [] keyUsage = new boolean[KeyUsage.values().length];
>>         for (KeyUsage ku : keyUsages) {
>>             keyUsage[ku.ordinal()] = true;
>>         }
>> 
>>         this.subjectName = new X500Principal(subjectName);
>>         this.publicKey = Objects.requireNonNull(publicKey, "Caught null public key");
>>         notBefore = (Date)Date.from(Instant.now().minus(1, ChronoUnit.HOURS));
>>         notAfter = Date.from(Instant.now().plus(1, ChronoUnit.HOURS));
>>         serialNumber = BigInteger.valueOf(random.nextLong(1000000)+1);
>>         byte[] keyIdBytes = new KeyIdentifier(publicKey).getIdentifier();
>>         Extension e = new SubjectKeyIdentifierExtension(keyIdBytes);
>>         extensions.put(e.getId(), e);
>> 
>>         KeyIdentifier kid = new KeyIdentifier(caKey);
>>         e = new AuthorityKeyIdentifierExtension(kid, null, null);
>>         extensions.put(e.getId(), e);
>> 
>>         if (keyUsages.length != 0) {
>>             e = new KeyUsageExtension(keyUsage);
>>             extensions.put(e.getId(), e);
>>         }
>>     }
>> 
>> 
>> 
>>>   it also means I can't use this method to to create a certificate with a longer, or shorter validity period
>> 
>> There are methods to set the not-before and not-after fields. Any field set in this static method can be changed:
>> 
>> `
>> newCertificateBuilder(...).notAfter(Date.from(Instant.now().plus(10, TimeUnit.YEARS)));
>> `
>> 
>> I don't like using `Date` here and would be happy to overload it to take `Instant` as well.
>
>> As a constructor, the code would look like this
> 
> <snip>
> 
> Ah, I see. Well technically you could call the set methods from the ctor but you would get `this-escape` compiler warnings, which you probably want to avoid.
> 
>> > it also means I can't use this method to to create a certificate with a longer, or shorter validity period
>> 
>> There are methods to set the not-before and not-after fields. Any field set in this static method can be changed:
>> 
>> `newCertificateBuilder(...).notAfter(Date.from(Instant.now().plus(10, TimeUnit.YEARS)));`
>> 
>> I don't like using `Date` here and would be happy to overload it to take `Instant` as well.
> 
> Yes, but I don't think the static method which is generically named `newCertificateBuilder` should be hard-coding a one hour validity period, that detail should be either in a different method (my preference) or this method should be named more clearly like `oneHourCertificateBuilder`. I realize this is just a test method, but this is a nicely designed API that has been very useful so I would prefer if we keep the flexibility.

I removed the default one-hour validity from the builder method and created a `setOneHourValidity()` method.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23700#discussion_r2303642791