8325766: Review seclibs tests for cert expiry

[mpdonova]

For this PR, I identified TLS tests that can fail due to hard-code certificates expiring. I updated those tests to use certificates that are generated programmatically. This includes adding some helper methods to the CertificateBuilder class to create builder objects with common default values. The builder can be further customized to meet the needs of the test. I also refactored some of the tests to put common functionality in base classes, further reducing duplicated code.

This PR does not include changes to SSLContextTemplate and the tests that use it. These tests require significant refactoring to incorporate programmatically generated certificates and should be a separate task.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8325766: Review seclibs tests for cert expiry (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18958/head:pull/18958
$ git checkout pull/18958

Update a local copy of the PR:
$ git checkout pull/18958
$ git pull https://git.openjdk.org/jdk.git pull/18958/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18958

View PR using the GUI difftool:
$ git pr show -t 18958

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18958.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mdonovan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@mpdonova The following labels will be automatically applied to this pull request:

• net
• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[seanjmullan]

Suggest renaming method to newSelfSignedCA().

[seanjmullan]

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.

[mpdonova]

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.

[seanjmullan]

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.

[mpdonova]

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.

[mlbridge]

Webrevs

• 02: Full - Incremental (b6ffb24b)
• 01: Full - Incremental (d52e1265)
• 00: Full (f211b615)

[bridgekeeper]

@mpdonova This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@mpdonova This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.