On Fri, 29 Aug 2025 23:33:16 GMT, Thomas Fitzsimmons <[email protected]> wrote:
>> Thank you for reviewing.
>>
>>> Hmm, I find it somewhat obscure that the config variant property changes
>>> the value of the config file name.
>>
>> Yes, I see your point.
>>
>>> With this new config variant property, it assumes that the confg file name
>>> has a "." which is probably true most if not all times.
>>
>> The regular expression supports appending to a file without a ".":
>>
>>
>> $ jshell -q
>> jshell> "kryoptic".replaceFirst("(\.[^\.]*)?$", "-" + "sensitive" + "$1");
>> $1 ==> "kryoptic-sensitive"
>>
>>
>> I should have added this case to the comment you mentioned above, will do in
>> the expanded comment you requested.
>>
>>> We should document all these properties so it's clear their precedence as
>>> well as the assumptions/implications. All these security can be set
>>> independently, right? It's a bit strange that you set the CUSTOM_P11_CONFIG
>>> NAME and then setting the config variant property would actually changes
>>> the config file to a different name.
>>
>> Yes, conceptually I am treating file pairs like `p11-nss.txt` and
>> `p11-nss-sensitive.txt` (and `p11-kryoptic.txt`,
>> `p11-kryoptic-sensitive.txt`) as variants of the same configuration. When
>> `CUSTOM_P11_CONFIG_VARIANT` is set, `CUSTOM_P11_CONFIG_NAME`'s meaning
>> becomes something like "base configuration file name".
>>
>> Given the current test suite, and how I am specifying the use of Kryoptic, I
>> wouldn't expect both `CUSTOM_P11_CONFIG_VARIANT` and
>> `CUSTOM_P11_CONFIG_NAME` (or `CUSTOM_P11_CONFIG`) to be specified by the
>> user at the same time. `CUSTOM_P11_CONFIG_VARIANT` is meant for hard-coding
>> in tests that invoke the test VM separately in sensitive and normal modes.
>>
>>> Perhaps check the existence of the file and error out with the config file
>>> and its path if the check fails, this way, it's crystal clear.
>>
>> OK, I can do that. I will add a `/** ... */` block above `getNssConfig`.
>> These two changes will hopefully reduce the weirdness of the
>> `CUSTOM_P11_CONFIG_NAME`/`CUSTOM_P11_CONFIG_VARIANT` combination. I will
>> also document the existing `CUSTOM_P11_CONFIG_NAME` versus
>> `CUSTOM_P11_CONFIG` precedence since that might also be surprising when both
>> are set.
>
> Done. See what you think. I am still open to other options. One idea I
> played around with was adding a `CONFIG_P11_CONFIG_BASE_NAME` that is just
> part of the file name, say `p11-nss` by default. Then in `getNssConfig` the
> file name could be built from the base and variant strings, instead of by
> changing the name. Conceptually that might be clearer but I think it would
> be a more invasive change given that the existing code deals with file names.
> And, having experimented with it, I think the new exception you suggested
> will make it fairly obvious what to fix if someone hits a missing
> configuration file.
It occurred to me that I might be able to simplify this patch if I use the same
configuration file names for `Kryoptic` configuration, but allow setting a
property to override the base path "./nss" (`nssDirDestination` in
`copyNssFiles`). Then the other tests wouldn't need any changes but could
still run against `Kryoptic` even though they hard-code
`p11-nss-sensitive.txt`. The weirdness would then be that the `Kryoptic`
configuration files are not named after their provider, but this could be
documented. I will try this approach tomorrow.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26325#discussion_r2314185051
