RFR 7191662: JCE providers should be located via ServiceLoader,

[Valerie Peng]

Hi, Mandy,

I have second thought on using the provider name for the java.security 
file...
There are a few things that I want to discuss/brainstorm internally to 
make sure that there is no "surprise" later.
Thus, I had not made the switch in this webrev. It doesn't affect the 
overall functionality.

The legacyLoad() is for finding providers the old-fashioned way, e.g. 
through reflection, instead of ServiceLoader.
The legacyLoad can only work using class name.

Running test/java/lang/SecurityManager/CheckSecurityProvider.java with a 
security manager should cover more than without.
I don't see much value in adding provider name verification? Verifying 
class name seems enough in making sure that the providers are present as 
expected.

As for test/tools/launcher/MiscTests.java, it's for verifying a bug in 
launcher, so I have stick with the internal API as I am not sure if 
switching to public API may be too great of a change as the call path 
changed significantly...

Thanks,
Valerie

On 5/26/2015 3:27 PM, Mandy Chung wrote:
> sun/security/jca/ProviderConfig.java
>
>  287         /**
>  288          * Loads the provider with the specified class name.
>  289          *
>  290          * @param name the name of the provider class
>  291          * @return the Provider, or null if it cannot be found or 
> loaded
>  292          * @throws ProviderException all other exceptions are 
> ignored
>  293          */
>  294         public Provider load(String classname) {
>                             :
>
>  305                     if (p.getClass().getName().equals(classname)) {
>  306                         return p;
>  307                     }
>
> Is this load method supposed to take the provider name instead of the 
> classname?
> line 305 should check against p.getName() instead?  The legacyLoad 
> method is
> for the fallback to the class name.
>
> java.security now uses classname.  Did you decide to use security 
> provider name instead?
> Even so, the legacyLoader method should be able to find them and the 
> load method
> looks to me should check the provider name instead.
>
> test/java/lang/SecurityManager/CheckSecurityProvider.java
>    you add this test to run with security manager.  I wonder if you want
>    to run with and without security manager through @run othervm.
>
>    Now each security provider has a name.  Should this test verify the 
> provider
>    name as well?
>
> test/tools/launcher/MiscTests.java
>    - does this test need to call sun.security.pkcs11 internal API? Can 
> this be
>      changed to call public API?
>
> I didn't review other files.
> Mandy
>
> On 05/26/2015 01:57 PM, Sean Mullan wrote:
>> This all looks fine to me (except for the Makefile stuff which I'll 
>> leave to others).
>>
>> --Sean
>>
>> On 05/21/2015 12:21 AM, Valerie Peng wrote:
>>> Sean,
>>>
>>> Could you please review this change? The changes are mostly the same as
>>> the prototype in Jake, but I have to make some modification due to the
>>> difference in ServiceLoader lookup in OpenJDK (corresponding
>>> META-INF/services/java.security.Provider files in each module) and the
>>> related makefile change (merge their content into one for the final
>>> image build). Then, I adjusted the Provider.configure() method to 
>>> take a
>>> single String argument to be consistent with the "providerarg" option
>>> that keytool defined.
>>>
>>> In addition, I also made some misc changes, such as configuring the
>>> providers inside ProviderConfig instead of ProviderLoader, add back the
>>> doPrivileged block to all the provider constructors. I also have second
>>> thought on making the switch to privider name (instead of provider 
>>> class
>>> name) in java.security file, so I reverted the changes on that - that
>>> SunPKCS11 provider has its name specified in its configuration file, so
>>> when ServiceLoader loads the PKCS11 provider, the configuration file 
>>> has
>>> not been passed to it, so the name is not known at that time. Thus,
>>> using the class name for the provider list entry seems to fit the flow
>>> better. I also updated the default policy for SunPKCS11 provider given
>>> its recent change of using sun.misc.
>>>
>>> Webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/
>>> CCC: http://ccc.us.oracle.com/7191662
>>>
>>> Thanks,
>>> Valerie
>>>
>