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

[Matthew Donovan]

On Tue, 3 Jun 2025 15:31:37 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> I think the way it currently is follows the builder pattern better. All of the 'set' methods return `this` which means if I change this method to a constructor, I'd have to duplicate the "set" code from all those methods.
>> 
>> I like the idea of using `Duration` but the API for it doesn't really lend itself to this use case. When the certificate is finally created, we write `Date` objects to the DerOutputStream so I'd have to choose a start time and then calculate the end time based on the duration. It's not hard, but it doesn't seem worth it when the common use case of this class is to generate short-lived certificates for a test. The default one-hour validity will cover the vast majority of tests.
>
>> I think the way it currently is follows the builder pattern better. All of the 'set' methods return `this` which means if I change this method to a constructor, I'd have to duplicate the "set" code from all those methods.
> 
> I'm not sure what you mean - can you give an example? I don't see any advantages of using a static method here, but I agree it works either way.
> 
>> I like the idea of using `Duration` but the API for it doesn't really lend itself to this use case. When the certificate is finally created, we write `Date` objects to the DerOutputStream so I'd have to choose a start time and then calculate the end time based on the duration. It's not hard, but it doesn't seem worth it when the common use case of this class is to generate short-lived certificates for a test. The default one-hour validity will cover the vast majority of tests.
> 
> I view this test utility class as being flexible enough for different cases. This is a useful method in that the other parameters are common fields that all certs usually have but it also means I can't use this method to to create a certificate with a longer, or shorter validity period. Alternatively, how about adding another method that hard-codes it as one hour:
> 
> `CertificateBuilder oneHourValidity()`

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.

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

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