On Wed, 22 May 2024 15:01:52 GMT, Matthew Donovan <mdono...@openjdk.org> wrote:
>> test/jdk/java/security/testlibrary/CertificateBuilder.java line 417: >> >>> 415: * @throws Exception >>> 416: */ >>> 417: public static CertificateBuilder >>> createClientCertificateBuilder(String subjectName, PublicKey publicKey, >> >> Suggest renaming to `newEndEntity()`. I think it would be more useful to >> pass in the type of end-entity certificate you want to create as a >> parameter, like TLS Server, TLS client, code signer, Time Stamp Authority, >> etc, and then the method creates the recommended key usage and extended key >> usages for each type, and maybe some of the extensions as well, although you >> may need more parameters and overloaded methods in those cases. You can use >> an Enum for the type. See `sun.security.validator` classes for similar ideas. > > 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18958#discussion_r1612137610
