RFR 8133151: Preferred provider configuration for JCE

[Anthony Scarpino]

On 10/15/2015 05:20 PM, Valerie Peng wrote:
> Hi Tony,
>
> Here are the comments for the remaining file.
>
> <ProviderList.java>
> - line 621, is there a particular reason to return immediately when
> s.ids is null? If there is, it should be commented here. The
> get(ServiceList s) method will fall back to s.type and s.algorithm when
> s.ids is null. Would be nice to state the reason for the difference in
> implementation.

I don't think there is a reason.  I suspect it was an odd assumption 
that testing didn't reveal as a problem.. I'll change it to check type 
and algorithm if ids is null

Going through the code I noticed the get(ServiceList) and get(type, 
algorithm) methods are not used, so I am removing them.  They were there 
from older code that now uses getAll(...).

> - line 698, the method name "compare" seems a little misleading, it only
> compares type and algorithm and it returns boolean instead of integer as
> the compare in other classes. Maybe names like "lookup" or "match" would
> be better...

Sure..

> - line 705, there is no check on "t" being non-null, why not use
> type.compareToIgnoreCase(t) instead?

Yep

> - line 710, exact match on algorithm? I thought u will support partial
> match, e.g. AES entry matching AES/CBC/PKCS5Padding request. Why don't u
> use "toString()" in the debug output as it provides more info.

The comment is old, I'll remove "full".  The code does check for partial 
matches because classes like Cipher break up the transformation into 
pieces (Cipher.getTransforms()) creating many ServiceId's with AES, 
AES/CBC, AES/CBC/PKCS5Padding, etc.

toString() fine.  I added it to the previous debug.println too.

> - line 722, why is "type" not included in the toString()?

You've found a lot of code cleanup that I missed.  It was that from from 
older version of the code before I started checking type.

The webrev is updated
http://cr.openjdk.java.net/~ascarpino/8133151/webrev.02/

>
> Thanks,
> Valerie
>
> On 10/15/2015 11:42 AM, Valerie Peng wrote:
>>
>> <MakeJavaSecurity.java>
>> - line 58-59, the "[openjdk target cpu architecture]" one should be
>> moved up. The optional restricted packages file names are at the end.
>>
>> <General>
>> - for the javadoc changes, the approved CCC has @implNote instead of
>> @implSpec.
>> Instead of just {@code getProviders}, it seems {@code
>> Security.getProviders} is clearer.
>>
>> <XMLSignatureFactory.java>
>> - line 262 - 267, given that there is an argument specifying provider
>> name, I don't think your changes applies to this method. If correct,
>> the javadoc change should be removed.
>>
>> <java.security>
>> - looks fine.
>>
>> I will continue to look at ProviderList.java and send u comments in a
>> separate email.
>> Thanks,
>> Valerie
>>
>> On 10/9/2015 10:06 AM, Anthony Scarpino wrote:
>>> Ping for a security review..
>>>
>>> Tony
>>>
>>> On 10/02/2015 10:08 AM, Anthony Scarpino wrote:
>>>> Hi all,
>>>>
>>>> I'm need a review of the last developement piece to JEP 246, the
>>>> configuration changes.
>>>>
>>>> I've copied the build-dev in case there were any comments on the minor
>>>> changes in the make directory related to the java.security file.
>>>>
>>>> http://cr.openjdk.java.net/~ascarpino/8133151/webrev/
>>>>
>>>> thanks
>>>>
>>>> Tony
>>>