On Tue, 3 Jun 2025 15:31:37 GMT, Sean Mullan <mul...@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
