Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 12:13:03 GMT, Andrew Leonard wrote: >> 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 > > Andrew Leonard has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > deterministic cacerts generation > >Signed-off-by: Andrew Leonard > - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard 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. - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 19:12:37 GMT, Andrew Leonard wrote: >> 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. > > yes, thanks, feeling rather stupid here! i'll raise an issue to fix @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. :) - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 18:46:09 GMT, Erik Joelsson wrote: >> 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 > > 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. yes, thanks, feeling rather stupid here! i'll raise an issue to fix - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 18:03:50 GMT, Andrew Leonard wrote: >> my assumption was the recipe gets resolved later > > 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 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. - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 17:48:04 GMT, Andrew Leonard wrote: >> you make a valid point, but i've tested this numerous times, but let me >> check again > > my assumption was the recipe gets resolved later 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 - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 17:46:35 GMT, Andrew Leonard wrote: >> 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. > > you make a valid point, but i've tested this numerous times, but let me check > again my assumption was the recipe gets resolved later - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 17:35:36 GMT, Magnus Ihse Bursie wrote: >> make/modules/java.base/Gendata.gmk line 76: >> >>> 74: ifneq ($(CACERTS_SRC), ) >>> 75: GENDATA_CACERTS_SRC := $(CACERTS_SRC) >>> 76: endif >> >> Does this even work?! You are reassigning the variable after it has been >> used. The := assignment means that it not a macro. > > 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. you make a valid point, but i've tested this numerous times, but let me check again - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 12:13:03 GMT, Andrew Leonard wrote: >> 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 > > Andrew Leonard has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > deterministic cacerts generation > >Signed-off-by: Andrew Leonard > - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard make/modules/java.base/Gendata.gmk line 76: > 74: ifneq ($(CACERTS_SRC), ) > 75: GENDATA_CACERTS_SRC := $(CACERTS_SRC) > 76: endif Does this even work?! You are reassigning the variable after it has been used. The := assignment means that it not a macro. - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 17:33:49 GMT, Magnus Ihse Bursie wrote: >> Andrew Leonard has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> deterministic cacerts generation >> >>Signed-off-by: Andrew Leonard >> - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> determinsitic cacerts generation >> >>Signed-off-by: Andrew Leonard >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> determinsitic cacerts generation >> >>Signed-off-by: Andrew Leonard > > make/modules/java.base/Gendata.gmk line 76: > >> 74: ifneq ($(CACERTS_SRC), ) >> 75: GENDATA_CACERTS_SRC := $(CACERTS_SRC) >> 76: endif > > Does this even work?! You are reassigning the variable after it has been > used. The := assignment means that it not a macro. 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. - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 14:29:00 GMT, Sean Mullan wrote: > 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 - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 12:13:03 GMT, Andrew Leonard wrote: >> 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 > > Andrew Leonard has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > deterministic cacerts generation > >Signed-off-by: Andrew Leonard > - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew 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. - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 12:13:03 GMT, Andrew Leonard wrote: >> 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 > > Andrew Leonard has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > deterministic cacerts generation > >Signed-off-by: Andrew Leonard > - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard Looks good! - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Wed, 1 Dec 2021 18:47:41 GMT, Erik Joelsson wrote: >> Andrew Leonard has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> deterministic cacerts generation >> >>Signed-off-by: Andrew Leonard >> - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> determinsitic cacerts generation >> >>Signed-off-by: Andrew Leonard >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> determinsitic cacerts generation >> >>Signed-off-by: Andrew Leonard > > make/autoconf/jdk-options.m4 line 176: > >> 174: [specify alternative cacerts source folder containing >> certificates])]) >> 175: AC_MSG_CHECKING([for cacerts source]) >> 176: if test "x$with_cacerts_src" == x; then > > Before this if block, please assign an empty value to CACERTS_SRC. Otherwise, > if the user happens to have that variable set in the environment, that value > will get recorded by configure, which is usually not something we want. done - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
> 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 Andrew Leonard has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation Signed-off-by: Andrew Leonard - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc - 8278080: Add --with-cacerts-src='user cacerts folder' to enable determinsitic cacerts generation Signed-off-by: Andrew Leonard - 8278080: Add --with-cacerts-src='user cacerts folder' to enable determinsitic cacerts generation Signed-off-by: Andrew Leonard - Changes: - all: https://git.openjdk.java.net/jdk/pull/6647/files - new: https://git.openjdk.java.net/jdk/pull/6647/files/1084c4e1..16c5ca6b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6647=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6647=00-01 Stats: 1879 lines in 62 files changed: 1045 ins; 297 del; 537 mod Patch: https://git.openjdk.java.net/jdk/pull/6647.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6647/head:pull/6647 PR: https://git.openjdk.java.net/jdk/pull/6647
