-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8308592: Framework for CA interoperability testing #390
Conversation
👋 Welcome back andrew! A progress list of the required criteria for merging this PR into |
This backport pull request has now been updated with issue from the original commit. |
/issue add 8272708,8288132,8268678,8270280 |
@gnu-andrew Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: |
|
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. The reason why certignarootca
test fails is the different defaults for jdk.tls.client.enableStatusRequestExtension
between JDK 11 and JDK 8 in SSLContextImpl.java
. The former has it set to true
the latter to false
as per the TLS 1.3 backport to 8. Without it the ClientHello
won't have the status_request
extension, which is required for the test to pass.
I suggest to set this to true
in the affected test only (or set it to true globally in CAInterop.java
). Either way I'd include this in this backport.
6929927
to
5d885d5
Compare
@gnu-andrew this pull request can not be integrated into git checkout JDK-8308592
git fetch https://git.openjdk.org/jdk8u-dev.git pr/389
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge pr/389"
git push |
The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout JDK-8308592
git fetch https://git.openjdk.org/jdk8u-dev.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
0dc8bad
to
04ae02c
Compare
@gnu-andrew Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
…ckport of TLS 1.3
Good catch. Looks like all tests pass with this enabled in
|
Yeah, I'm not going to merge over half a dozen commits manually, bot, when Git is smart enough to match them against the ones you pushed to 8u-dev and just drop them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
/approval request This change reworks the certificate tests to use test portals rather than having to maintain copies of the certificates in the OpenJDK repository and so should reduce the maintenance cost of these tests going forward. The fix has already been backported to the other LTS JDKs (11u, 17u, 21u). The backport required manually removing some tests which were not in their latest versions, a couple of dependent backports to provide us with SkippedException and a passing Actalis test, and 8u-specific modifications (change of cacerts location and type, enable jdk.tls.client.enableStatusRequestExtension). All tests passed successfully. |
@gnu-andrew |
/approve yes |
@jerboaa |
@gnu-andrew This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
@gnu-andrew Could you please integrate this? Thanks! |
Thought I had, sorry. |
Going to push as commit b610be3.
Your commit was automatically rebased without conflicts. |
@gnu-andrew Pushed as commit b610be3. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi all,
This pull request contains a backport of commit da57d2a1 from the openjdk/jdk repository.
The commit being backported was authored by Rajan Halade on 19 Sep 2023 and was reviewed by Sean Mullan. It was backported to 17u on 2023-09-28 and 11u on 2023-10-11
It reworks the certificate tests to use test portals rather than having to maintain copies of the certificates in the OpenJDK repository and so should reduce the maintenance cost of these tests going forward. The fix has already been backported to 21u and Oracle-17u.
There were conflicts in the removals of
ActalisCA.java
,BuypassCA.java
,LetsEncryptCA.java
andQuoVadisCA.java
, due to changes not in 8u. For the first of these, we backported JDK-8297955 (see #388) as it also contains a code change. As the other three are only test changes, we assumed these bugs to be resolved by the replacement of the individual tests withCAInterop.java
.The
ValidatePathWithURL.java
class had to be modified slightly to work with the 8ucacerts
. These changes were based on the existingValidatePathWithParams.java
.All tests pass with the exception of the OCSP case of
certignarootca
. This fails withCaused by: java.security.cert.CertPathValidatorException: Certificate does not specify OCSP responder
. While not ideal, I don't think this is an issue with the testing backport so I'd suggest we look into this and fix it later, while getting this in so that pending certificate updates can go in. It seems to be 8u specific (the case passed on 11u). The CRL case does pass as doesCertignaCA.java
(not sure why there are two tests for this one).Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/390/head:pull/390
$ git checkout pull/390
Update a local copy of the PR:
$ git checkout pull/390
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/390/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 390
View PR using the GUI difftool:
$ git pr show -t 390
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/390.diff
Webrev
Link to Webrev Comment