RFR: 6447816: Provider filtering (getProviders) is not working with OR'd conditions [v2]

[Weijun Wang]

On Thu, 25 Aug 2022 02:59:42 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> Existing provider filtering code only handles two standard attribute "KeySize" and "ImplementedIn", the rest are compared by exact match. Over time, more standard attributes are added which contain multiple values separated by "|". We should enhance the provider filtering code to better compare these.
>> 
>> Documentation update for this is tracked separately under https://bugs.openjdk.org/browse/JDK-6447817.
>> 
>> Thanks in advance for review~
>
> Valerie Peng has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update test to use SHA256 and DSA throughout.

Some comments.

src/java.base/share/classes/java/security/Security.java line 530:

> 528:             // <crypto_service>.<algo_or_type> <attr_name>:<attr_value>
> 529:             key = filter.substring(0, index);
> 530:             value = filter.substring(index + 1);

Do you need to ensure `value` is non-empty here? I am worried that somewhere later a `string.indexOf("")` is called and it's always true.

src/java.base/share/classes/java/security/Security.java line 599:

> 597:         // Returns all installed providers
> 598:         // if the selection criteria is null.
> 599:         if ((keySet == null) || (allProviders == null)) {

I'm not sure, but can `keySet` or `allProviders` be null? Or you meant `isEmpty()`?

src/java.base/share/classes/java/security/Security.java line 931:

> 929:                     // check individual component for match and bail if no match
> 930:                     if (prop.indexOf(st.nextToken()) == -1) {
> 931:                         return false;

So if `value` has several sub-values, all of them must appear in the `prop` value. Do we need to make this clear in the spec?

Also, you use `indexOf` instead of an exact match to a sub-value in `prop`. Is this always correct? I am wondering if a value can be substring of a different value. I see you support simple class name in the test. Is it worth we doing this? I would rather be strict at the beginning.

src/java.base/share/classes/java/security/Security.java line 945:

> 943: 
> 944:     static String[] getFilterComponents(String filterKey, String filterValue) {
> 945: 

Can we create a new class for the return value plus `filterValue`? Then we can make `isCriterionSatisfied(Provider)` a method of it.

test/jdk/java/security/Security/ProviderFiltering.java line 89:

> 87:         doit(key + ":" + valComp2 + " ", p);
> 88:         // 4. partial value, e.g. class name only
> 89:         doit(key + ":" + valComp2CN, p);

Do we really need to support cases 2-4? In fact, the only place mentioning spaces in the spec is about the ones between "SHA256withDSA" and "SupportedKeyClasses". I'd rather you add a test case on it.

test/jdk/java/security/Security/ProviderFiltering.java line 109:

> 107:         filters.put("Signature.SHA256withDSA", "");
> 108:         doit(filters, p);
> 109:         filters.put("Cipher.Nonexisting", "");

Even if it's not "Nonexisting", I assume it's still empty after filtering?

-------------

PR: https://git.openjdk.org/jdk/pull/10008