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
