Please find comments in line.
On 5/27/2015 4:40 PM, Mandy Chung wrote:
On May 27, 2015, at 4:17 PM, Valerie Peng<valerie.p...@oracle.com> wrote:
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.
What is the issue you concern about with ServiceLoader to load providers when
it matches its provider name rather than the classname? SPI allows you to move
away from using the classname in the configuration and separate implementation
from interfaces.
There are several things...or questions that I'd like to clarify before
making the switch.
Not all of them are related to this switch though, but still, I don't
want to complicate things further by adding new changes into this mix.
First, provider name seems "looser" than provider class name. With just
a name, what happens when there is a name collision?
Is there an ordering on how ServiceLoader loads/looks up services? If
yes/no, does it affect provider validation?
Second, it's a bit awkward in the SunPKCS11 provider case. For SunPKCS11
provider, its name is set in the configuration file which is passed to
the provider object upon the configure call. When ServiceLoader loads
the SunPKCS11 provider object, you can't use the provider name to look
for a match as the provider itself doesn't have that info yet. Sure, we
can always do special handling for SunPKCS11 to make this work. But this
special handling isn't needed for the class name setting.
Lastly, this overlaps with Tony's JEP and his current proposal fits
better with class name setting (this may just be my personal opinion).
So, I feel it's better to clarify all these questions before making the
switch.
I did mention about this (not using provider name in java.security) when
sending out my webrev to security alias but didn't not notify Alan and
you separately. Sorry~
Valerie
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.
For the first push, it’s okay to leave them loaded by the legacy mechanism via
reflection until you sort out the provider name issue before you make them true
service providers. But I still think ProviderLoader.load should not tie with
the classname unless there is a strong reason to.
Running test/java/lang/SecurityManager/CheckSecurityProvider.java with a
security manager should cover more than without.
Ok with me.
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.
If you are using the provider name to load service providers, this adds
additional verification. I’m surprised that your webrev was changed as I
didn’t see any mail about your concern that leads to the change.
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…
This test verifies 6856415 that needs to use sun.* API and ensures that the
right Exception is thrown. Ok - it’s okay to leave it then.
Mandy
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
