On Thu, 23 May 2024 18:23:28 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> I renamed the method. >> >> I don't want to over-generalize the code when I don't know what we'll >> need/want in the future. The tests in this PR just create CA and end-entity >> certs and with a couple exceptions, the tests in this PR are all of the >> tests that fail when the system clock is set to 2050. The exceptions are >> also client/server tests that don't need code signer and TSA certificates. >> >> When considering your comment, I went through the tests in this PR and found >> and removed some additional redundancy. > > The problem with generalizing an end-entity certificate is that there is not > much you can generalize other than the cA field of the BasicConstraints > extension being false (or not including the BC extension). Right now the > newEndEntity() method looks like it is for TLS server certificates based on > the KeyUsage extension (but it is also missing the KeyAgreement bit). But you > could go further and add the EKU extension with the TLS server bit set. It > shouldn't be used for TLS client certificates because they would have > different KU extension values. Same for code signing end entity certificates. > If you don't make it more specific, I feel that it is likely to be used to > create certificates incorrectly. > > So if you only need to create TLS server certs, I suggest renaming this > method to newTLSServer() to make it clear it is for TLS server certs only. > > If a test needs to create certs which don't conform to a specific profile or > have invalid fields, then it is probably better off calling > CertificateBuilder methods itself rather than trying to create a helper > method that satisfies all types of use cases. The intent of these methods was not to create full certificates to satisfy lots of use cases but to create a CertificateBuilder object with the common fields (subject, serial number, validity, etc.) set. I see what you're saying about some of the extensions and created `newServerCertificateBuilder` and `newClientCertificateBuilder` methods that set key usage and the EKU extension for each. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18958#discussion_r1624772738
