8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation

[andrew-m-leonard]

Addition of a configure option --with-cacerts-src='user cacerts folder' to allow developers to specify their own cacerts PEM folder for generation of the cacerts store using the deterministic openjdk GenerateCacerts tool.

Signed-off-by: Andrew Leonard anleonar@redhat.com

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation

Reviewers

• Erik Joelsson (@erikj79 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6647/head:pull/6647
$ git checkout pull/6647

Update a local copy of the PR:
$ git checkout pull/6647
$ git pull https://git.openjdk.java.net/jdk pull/6647/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6647

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6647.diff

[bridgekeeper]

👋 Welcome back aleonard! 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]

@andrew-m-leonard The following labels will be automatically applied to this pull request:

• build
• core-libs

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.

[mlbridge]

Webrevs

• 01: Full - Incremental (16c5ca6b)
• 00: Full (1084c4e1)

[mrserb]

I have a question related to the custom cacerts which can be added to the OpenJDK bundle. How do you pass the tests like test/jdk/sun/security/lib/cacerts/VerifyCACerts.java using that custom jdk bundle? Probably we can add an additional configuration to that test so it will check the custom cacerts passed to the build as well?

[coffeys]

/label add security

[openjdk]

@coffeys
The security label was successfully added.

[andrew-m-leonard]

I have a question related to the custom cacerts which can be added to the OpenJDK bundle. How do you pass the tests like test/jdk/sun/security/lib/cacerts/VerifyCACerts.java using that custom jdk bundle? Probably we can add an additional configuration to that test so it will check the custom cacerts passed to the build as well?

@mrserb
So VerifyCACerts is specific to the make/data/cacerts certificates, the README specifically states there that when those are updated VerifyCACerts needs updating. It checks things like fingerprints etc..

If a developer or other provider decide to provide their own cacerts file, then it is up to them to have verified and trust those certificates. They won't run the VerifyCACerts which is specific to the openjdk certs.
This is the case at Adoptium for example, which uses the Mozilla trusted CA certs.

[erikj79]

Looks good!

[openjdk]

@andrew-m-leonard This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation

Reviewed-by: erikj

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 1 new commit pushed to the master branch:

• 3d98ec1: 8273056: java.util.random does not correctly sample exponential or Gaussian distributions

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[seanjmullan]

I don’t have any major concerns with this change, as long as the default cacerts are still the ones that are in the JDK. As an aside, using Mozilla's root certificates might be fine for TLS certificates, but if you need to support code signing certificates you may run into issues with missing CAs as Mozilla's Root program does not support that use case. Also, by overriding the roots included in the JDK, you are taking on the responsibility (which is significant, in my opinion) of ensuring that those roots are trusted and have not been compromised, revoked, have weak keys, etc.

[andrew-m-leonard]

I don’t have any major concerns with this change, as long as the default cacerts are still the ones that are in the JDK. As an aside, using Mozilla's root certificates might be fine for TLS certificates, but if you need to support code signing certificates you may run into issues with missing CAs as Mozilla's Root program does not support that use case. Also, by overriding the roots included in the JDK, you are taking on the responsibility (which is significant, in my opinion) of ensuring that those roots are trusted and have not been compromised, revoked, have weak keys, etc.

@seanjmullan Thanks Sean, I'll pass your comment on, cheers Andrew

[andrew-m-leonard]

/integrate

[openjdk]

Going to push as commit dc2abc9.
Since your change was applied there have been 6 commits pushed to the master branch:

• 8d9cb2e: 8278078: Cannot reference super before supertype constructor has been called
• 65960f7: 8277753: Long*VectorTests.java fail with "bad AD file" on x86_32 after JDK-8276162
• 3889af3: 8277906: Incorrect type for IV phi of long counted loops after CCP
• d2b16c8: 8276126: Dump time class transformation causes heap objects of non-boot classes to be archived
• 7696897: 8276837: [macos]: Error when signing the additional launcher
• 3d98ec1: 8273056: java.util.random does not correctly sample exponential or Gaussian distributions

Your commit was automatically rebased without conflicts.

[openjdk]

@andrew-m-leonard Pushed as commit dc2abc9.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[magicus]

Does this even work?! You are reassigning the variable after it has been used. The := assignment means that it not a macro.

[magicus]

I would have expected to see something like:

ifneq ($(CACERTS_SRC), )
  GENDATA_CACERTS_SRC := $(CACERTS_SRC)
else
  GENDATA_CACERTS_SRC := $(TOPDIR)/make/data/cacerts/
endif

at line 63.

[andrew-m-leonard]

you make a valid point, but i've tested this numerous times, but let me check again

[andrew-m-leonard]

my assumption was the recipe gets resolved later

[andrew-m-leonard]

this was my understanding: https://www.gnu.org/software/make/manual/html_node/Variables-in-Recipes.html

This occurs after make has finished reading all the makefiles and the target is determined to be out of date; so, the recipes for targets which are not rebuilt are never expanded. 

but i'm going to double check I was checking the resultant cacerts correctly in my tests

[erikj79]

Oh, I didn't expand the diff far enough to actually see the context correctly when I reviewed this as I would never have imagined the conditional to be placed after the rule. While this will work as so far as using the correct files, incremental builds will not be correct, because the rules are defined in the first pass.

I very much agree with Magnus that this conditional belongs around line 63.

[andrew-m-leonard]

yes, thanks, feeling rather stupid here! i'll raise an issue to fix

[magicus]

@andrew-m-leonard Don't be. Make is a horrible programming language, both syntactically and semantically. It's taken me years to be somewhat comfortable with it, and often I just manage to get it right only by sticking to a few, well-proven and battle-hardened patterns. :)

[mrserb]

This is the case at Adoptium for example, which uses the Mozilla trusted CA certs.

But they didn't think skipping this test was too strong a step? For example validation of the certs expiration is quite useful. I tried to update the test to take into account additional certs, but it caused a merge conflict each time the certs in OpenJDK are updated. Probably we can add a config file that can inject/override some info in the test(at least skip the checksum validation)? By default this config file will be empty and will not be modified in the OpenJDK, but the vendors will be able to modify it. @wangweij @rhalade what do you think?

[rhalade]

The purpose of this test is to ensure integrity of the cacerts file along with basic validation of included roots. Having a config file with this information sounds like a good idea for now to be able to handle multiple files.