Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
Mystery solved. Didn't notice that. :P Valerie On 6/30/2016 7:00 PM, Wang Weijun wrote: On Jul 1, 2016, at 9:16 AM, Valerie Peng wrote: Weird, I still see -provider on the link below. http://cr.openjdk.java.net/~weijun/8130302/webrev.06/test/sun/security/tools/keytool/i18n.html.sdiff.html This is sdiff, you need to scroll to far right to see the new line. I should probably make the lines shorter one day. --Max If you saw differently, I need to book an urgent doc appt for my eyes... Valerie On 6/30/2016 5:40 PM, Wang Weijun wrote: Confused. http://cr.openjdk.java.net/~weijun/8130302/webrev.06/test/sun/security/tools/keytool/i18n.html.html has 53 keytool -list -storepass password -addprovider SUN Correct, SUN provider is not exported as a provider for ServiceLoader. I don't have preference to use -providerClass over -addprovider. Whatever is the expected usage is fine with me. OK. Thanks Max
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jul 1, 2016, at 9:16 AM, Valerie Peng wrote: > > Weird, I still see -provider on the link below. > http://cr.openjdk.java.net/~weijun/8130302/webrev.06/test/sun/security/tools/keytool/i18n.html.sdiff.html This is sdiff, you need to scroll to far right to see the new line. I should probably make the lines shorter one day. --Max > > If you saw differently, I need to book an urgent doc appt for my eyes... > Valerie > > On 6/30/2016 5:40 PM, Wang Weijun wrote: >> Confused. >> http://cr.openjdk.java.net/~weijun/8130302/webrev.06/test/sun/security/tools/keytool/i18n.html.html >> has 53 keytool -list -storepass password -addprovider SUN >>> Correct, SUN provider is not exported as a provider for ServiceLoader. I >>> don't have preference to use -providerClass over -addprovider. Whatever is >>> the expected usage is fine with me. >> OK. >> >> Thanks >> Max >> >
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
Weird, I still see -provider on the link below. http://cr.openjdk.java.net/~weijun/8130302/webrev.06/test/sun/security/tools/keytool/i18n.html.sdiff.html If you saw differently, I need to book an urgent doc appt for my eyes... Valerie On 6/30/2016 5:40 PM, Wang Weijun wrote: Confused. http://cr.openjdk.java.net/~weijun/8130302/webrev.06/test/sun/security/tools/keytool/i18n.html.html has 53 keytool -list -storepass password -addprovider SUN Correct, SUN provider is not exported as a provider for ServiceLoader. I don't have preference to use -providerClass over -addprovider. Whatever is the expected usage is fine with me. OK. Thanks Max
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jul 1, 2016, at 8:20 AM, Valerie Peng wrote:
>
> Max,
>
> Please find comments in line.
>
> On 6/29/2016 8:18 PM, Wang Weijun wrote:
>>> On Jun 30, 2016, at 9:39 AM, Valerie Peng wrote:
>>>
>>> Hi Max,
>>>
>>> Changes look fine. Just some comments/questions as below.
>>>
>>>
>>> - line 46, fix this {0} as well?
>> I don't see {0} in keytool/Resources.java.
>>
>> There is one in jarsigner/Resources.java:
>>
>>46 {"signerClass.is.not.a.signing.mechanism", "{0} is not a
>> signing mechanism"},
>>
>> You mean it's useless now?
> Right, it's under the jarsigner directory. My eyes were crossed. ;)
> Is this used still? I didn't find reference to this one.
It is useless after the jdk.security.jarsigner.JarSigner API. I'll remove it.
>>>
>>> - line 53, better to use -providerClass?
>> Both should work.
>>
>> -addprovider works because SUN is already a loaded provider.
>> -providerclass works because sun.security.provider.Sun is a public class in
>> the same module.
>>
>> I prefer -addprovider because -providerclass is only for legacy providers
>> loaded with reflection.
>>
>> In fact, I noticed that SUN is also not in
>> ServiceLoader.load(Provider.class), which means it is not a service. Is that
>> why you suggest the test load it with -providerclass?
> I only saw -provider not -addprovider?
Confused.
http://cr.openjdk.java.net/~weijun/8130302/webrev.06/test/sun/security/tools/keytool/i18n.html.html
has
53 keytool -list -storepass password -addprovider SUN
> Correct, SUN provider is not exported as a provider for ServiceLoader. I
> don't have preference to use -providerClass over -addprovider. Whatever is
> the expected usage is fine with me.
OK.
Thanks
Max
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
Max,
Please find comments in line.
On 6/29/2016 8:18 PM, Wang Weijun wrote:
On Jun 30, 2016, at 9:39 AM, Valerie Peng wrote:
Hi Max,
Changes look fine. Just some comments/questions as below.
- line 46, fix this {0} as well?
I don't see {0} in keytool/Resources.java.
There is one in jarsigner/Resources.java:
46 {"signerClass.is.not.a.signing.mechanism", "{0} is not a signing
mechanism"},
You mean it's useless now?
Right, it's under the jarsigner directory. My eyes were crossed. ;)
Is this used still? I didn't find reference to this one.
- line 53, better to use -providerClass?
Both should work.
-addprovider works because SUN is already a loaded provider.
-providerclass works because sun.security.provider.Sun is a public class in the
same module.
I prefer -addprovider because -providerclass is only for legacy providers
loaded with reflection.
In fact, I noticed that SUN is also not in ServiceLoader.load(Provider.class),
which means it is not a service. Is that why you suggest the test load it with
-providerclass?
I only saw -provider not -addprovider?
Correct, SUN provider is not exported as a provider for ServiceLoader. I
don't have preference to use -providerClass over -addprovider. Whatever
is the expected usage is fine with me.
Thanks,
Valerie
Thanks
Max
Thanks,
Valerie
On 6/28/2016 6:09 PM, Wang Weijun wrote:
Ping again to security-dev. Anyone can approve it?
The latest webrev is at
http://cr.openjdk.java.net/~weijun/8130302/webrev.06
Change from webrev.05 [1] is tiny.
Thanks
Max
[1] http://cr.openjdk.java.net/~weijun/8130302/webrev.06/interdiff.patch.html
On Jun 16, 2016, at 9:33 AM, Wang Weijun wrote:
On Jun 16, 2016, at 7:50 AM, Valerie Peng wrote:
No big difference to me.
Good, I'll remove the cast.
@security-dev, can someone approve the whole webrev.05?
http://cr.openjdk.java.net/~weijun/8130302/webrev.05
Thanks
Max
Valerie
On 6/15/2016 8:40 AM, Wang Weijun wrote:
On Jun 15, 2016, at 10:57 PM, Mandy Chung wrote:
241 throw (InvalidParameterException)
This cast should not be needed?
} catch (UcryptoException ue) {
throw (InvalidParameterException)
new InvalidParameterException("Error using " + configArg).
initCause(ue.getCause());
}
initCause() returns Throwable but the method's signature throws
InvalidParameterException.
Perhaps have a local variable for InvalidParameterException exception.
Valerie, are you OK with this?
--Max
Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
Hi Sean
Thanks for the comment. This exception is thrown by a helper class, which is
then caught by the application class and a new localized error message
including the provider name is shown to the user. Is that enough?
Thanks
Max
> On Jun 30, 2016, at 6:59 PM, Seán Coffey wrote:
>
> minor comment from me Max.
> src/java.base/share/classes/sun/security/tools/KeyStoreUtil.java
>
>
>> 293 throw new IllegalArgumentException("No provider found");
> Can you print the provider name that was being searched for in your exception
> ?
>
>
> Regards,
> Sean.
http://cr.openjdk.java.net/~weijun/8130302/webrev.06
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 30/06/2016 14:44, Wang Weijun wrote:
Hi Sean
Thanks for the comment. This exception is thrown by a helper class, which is
then caught by the application class and a new localized error message
including the provider name is shown to the user. Is that enough?
Perfect. That should do!
thanks,
Sean.
Thanks
Max
On Jun 30, 2016, at 6:59 PM, Seán Coffey wrote:
minor comment from me Max.
src/java.base/share/classes/sun/security/tools/KeyStoreUtil.java
293 throw new IllegalArgumentException("No provider found");
Can you print the provider name that was being searched for in your exception ?
Regards,
Sean.
http://cr.openjdk.java.net/~weijun/8130302/webrev.06
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
minor comment from me Max.
src/java.base/share/classes/sun/security/tools/KeyStoreUtil.java
293 throw new IllegalArgumentException("No provider found");
Can you print the provider name that was being searched for in your
exception ?
Regards,
Sean.
On 30/06/2016 04:18, Wang Weijun wrote:
On Jun 30, 2016, at 9:39 AM, Valerie Peng wrote:
Hi Max,
Changes look fine. Just some comments/questions as below.
- line 46, fix this {0} as well?
I don't see {0} in keytool/Resources.java.
There is one in jarsigner/Resources.java:
46 {"signerClass.is.not.a.signing.mechanism", "{0} is not a signing
mechanism"},
You mean it's useless now?
- line 151, maybe it should be calling the createUsingTestJDK? Otherwise the
default is to use the compiler JDK
Yes. I don't know this method before.
- line 53, better to use -providerClass?
Both should work.
-addprovider works because SUN is already a loaded provider.
-providerclass works because sun.security.provider.Sun is a public class in the
same module.
I prefer -addprovider because -providerclass is only for legacy providers
loaded with reflection.
In fact, I noticed that SUN is also not in ServiceLoader.load(Provider.class),
which means it is not a service. Is that why you suggest the test load it with
-providerclass?
Thanks
Max
Thanks,
Valerie
On 6/28/2016 6:09 PM, Wang Weijun wrote:
Ping again to security-dev. Anyone can approve it?
The latest webrev is at
http://cr.openjdk.java.net/~weijun/8130302/webrev.06
Change from webrev.05 [1] is tiny.
Thanks
Max
[1] http://cr.openjdk.java.net/~weijun/8130302/webrev.06/interdiff.patch.html
On Jun 16, 2016, at 9:33 AM, Wang Weijun wrote:
On Jun 16, 2016, at 7:50 AM, Valerie Peng wrote:
No big difference to me.
Good, I'll remove the cast.
@security-dev, can someone approve the whole webrev.05?
http://cr.openjdk.java.net/~weijun/8130302/webrev.05
Thanks
Max
Valerie
On 6/15/2016 8:40 AM, Wang Weijun wrote:
On Jun 15, 2016, at 10:57 PM, Mandy Chung wrote:
241 throw (InvalidParameterException)
This cast should not be needed?
} catch (UcryptoException ue) {
throw (InvalidParameterException)
new InvalidParameterException("Error using " + configArg).
initCause(ue.getCause());
}
initCause() returns Throwable but the method's signature throws
InvalidParameterException.
Perhaps have a local variable for InvalidParameterException exception.
Valerie, are you OK with this?
--Max
Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jun 30, 2016, at 9:39 AM, Valerie Peng wrote:
>
> Hi Max,
>
> Changes look fine. Just some comments/questions as below.
>
>
> - line 46, fix this {0} as well?
I don't see {0} in keytool/Resources.java.
There is one in jarsigner/Resources.java:
46 {"signerClass.is.not.a.signing.mechanism", "{0} is not a signing
mechanism"},
You mean it's useless now?
>
>
> - line 151, maybe it should be calling the createUsingTestJDK? Otherwise the
> default is to use the compiler JDK
Yes. I don't know this method before.
>
>
> - line 53, better to use -providerClass?
Both should work.
-addprovider works because SUN is already a loaded provider.
-providerclass works because sun.security.provider.Sun is a public class in the
same module.
I prefer -addprovider because -providerclass is only for legacy providers
loaded with reflection.
In fact, I noticed that SUN is also not in ServiceLoader.load(Provider.class),
which means it is not a service. Is that why you suggest the test load it with
-providerclass?
Thanks
Max
>
> Thanks,
> Valerie
>
> On 6/28/2016 6:09 PM, Wang Weijun wrote:
>> Ping again to security-dev. Anyone can approve it?
>>
>> The latest webrev is at
>>
>>http://cr.openjdk.java.net/~weijun/8130302/webrev.06
>>
>> Change from webrev.05 [1] is tiny.
>>
>> Thanks
>> Max
>>
>> [1] http://cr.openjdk.java.net/~weijun/8130302/webrev.06/interdiff.patch.html
>>
>>> On Jun 16, 2016, at 9:33 AM, Wang Weijun wrote:
>>>
>>>
On Jun 16, 2016, at 7:50 AM, Valerie Peng wrote:
No big difference to me.
>>> Good, I'll remove the cast.
>>>
>>> @security-dev, can someone approve the whole webrev.05?
>>>
>>> http://cr.openjdk.java.net/~weijun/8130302/webrev.05
>>>
>>> Thanks
>>> Max
>>>
Valerie
On 6/15/2016 8:40 AM, Wang Weijun wrote:
>> On Jun 15, 2016, at 10:57 PM, Mandy Chung wrote:
>>
241 throw (InvalidParameterException)
This cast should not be needed?
>>> } catch (UcryptoException ue) {
>>> throw (InvalidParameterException)
>>> new InvalidParameterException("Error using " + configArg).
>>> initCause(ue.getCause());
>>> }
>>>
>>> initCause() returns Throwable but the method's signature throws
>>> InvalidParameterException.
>>>
>> Perhaps have a local variable for InvalidParameterException exception.
> Valerie, are you OK with this?
>
> --Max
>
>> Mandy
>
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
Hi Max,
Changes look fine. Just some comments/questions as below.
- line 46, fix this {0} as well?
- line 151, maybe it should be calling the createUsingTestJDK? Otherwise
the default is to use the compiler JDK
- line 53, better to use -providerClass?
Thanks,
Valerie
On 6/28/2016 6:09 PM, Wang Weijun wrote:
Ping again to security-dev. Anyone can approve it?
The latest webrev is at
http://cr.openjdk.java.net/~weijun/8130302/webrev.06
Change from webrev.05 [1] is tiny.
Thanks
Max
[1] http://cr.openjdk.java.net/~weijun/8130302/webrev.06/interdiff.patch.html
On Jun 16, 2016, at 9:33 AM, Wang Weijun wrote:
On Jun 16, 2016, at 7:50 AM, Valerie Peng wrote:
No big difference to me.
Good, I'll remove the cast.
@security-dev, can someone approve the whole webrev.05?
http://cr.openjdk.java.net/~weijun/8130302/webrev.05
Thanks
Max
Valerie
On 6/15/2016 8:40 AM, Wang Weijun wrote:
On Jun 15, 2016, at 10:57 PM, Mandy Chung wrote:
241 throw (InvalidParameterException)
This cast should not be needed?
} catch (UcryptoException ue) {
throw (InvalidParameterException)
new InvalidParameterException("Error using " + configArg).
initCause(ue.getCause());
}
initCause() returns Throwable but the method's signature throws
InvalidParameterException.
Perhaps have a local variable for InvalidParameterException exception.
Valerie, are you OK with this?
--Max
Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
I will take a look tomorrow.
Thanks,
Valerie
On 6/28/2016 6:09 PM, Wang Weijun wrote:
Ping again to security-dev. Anyone can approve it?
The latest webrev is at
http://cr.openjdk.java.net/~weijun/8130302/webrev.06
Change from webrev.05 [1] is tiny.
Thanks
Max
[1] http://cr.openjdk.java.net/~weijun/8130302/webrev.06/interdiff.patch.html
On Jun 16, 2016, at 9:33 AM, Wang Weijun wrote:
On Jun 16, 2016, at 7:50 AM, Valerie Peng wrote:
No big difference to me.
Good, I'll remove the cast.
@security-dev, can someone approve the whole webrev.05?
http://cr.openjdk.java.net/~weijun/8130302/webrev.05
Thanks
Max
Valerie
On 6/15/2016 8:40 AM, Wang Weijun wrote:
On Jun 15, 2016, at 10:57 PM, Mandy Chung wrote:
241 throw (InvalidParameterException)
This cast should not be needed?
} catch (UcryptoException ue) {
throw (InvalidParameterException)
new InvalidParameterException("Error using " + configArg).
initCause(ue.getCause());
}
initCause() returns Throwable but the method's signature throws
InvalidParameterException.
Perhaps have a local variable for InvalidParameterException exception.
Valerie, are you OK with this?
--Max
Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
Ping again to security-dev. Anyone can approve it?
The latest webrev is at
http://cr.openjdk.java.net/~weijun/8130302/webrev.06
Change from webrev.05 [1] is tiny.
Thanks
Max
[1] http://cr.openjdk.java.net/~weijun/8130302/webrev.06/interdiff.patch.html
> On Jun 16, 2016, at 9:33 AM, Wang Weijun wrote:
>
>
>> On Jun 16, 2016, at 7:50 AM, Valerie Peng wrote:
>>
>> No big difference to me.
>
> Good, I'll remove the cast.
>
> @security-dev, can someone approve the whole webrev.05?
>
> http://cr.openjdk.java.net/~weijun/8130302/webrev.05
>
> Thanks
> Max
>
>> Valerie
>>
>> On 6/15/2016 8:40 AM, Wang Weijun wrote:
On Jun 15, 2016, at 10:57 PM, Mandy Chung wrote:
>> 241 throw (InvalidParameterException)
>>
>> This cast should not be needed?
>>
> } catch (UcryptoException ue) {
> throw (InvalidParameterException)
> new InvalidParameterException("Error using " + configArg).
> initCause(ue.getCause());
> }
>
> initCause() returns Throwable but the method's signature throws
> InvalidParameterException.
>
Perhaps have a local variable for InvalidParameterException exception.
>>> Valerie, are you OK with this?
>>>
>>> --Max
>>>
Mandy
>
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jun 16, 2016, at 7:50 AM, Valerie Peng wrote:
>
> No big difference to me.
Good, I'll remove the cast.
@security-dev, can someone approve the whole webrev.05?
http://cr.openjdk.java.net/~weijun/8130302/webrev.05
Thanks
Max
> Valerie
>
> On 6/15/2016 8:40 AM, Wang Weijun wrote:
>>> On Jun 15, 2016, at 10:57 PM, Mandy Chung wrote:
>>>
> 241 throw (InvalidParameterException)
>
> This cast should not be needed?
>
} catch (UcryptoException ue) {
throw (InvalidParameterException)
new InvalidParameterException("Error using " + configArg).
initCause(ue.getCause());
}
initCause() returns Throwable but the method's signature throws
InvalidParameterException.
>>> Perhaps have a local variable for InvalidParameterException exception.
>> Valerie, are you OK with this?
>>
>> --Max
>>
>>> Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
No big difference to me.
Valerie
On 6/15/2016 8:40 AM, Wang Weijun wrote:
On Jun 15, 2016, at 10:57 PM, Mandy Chung wrote:
241 throw (InvalidParameterException)
This cast should not be needed?
} catch (UcryptoException ue) {
throw (InvalidParameterException)
new InvalidParameterException("Error using " + configArg).
initCause(ue.getCause());
}
initCause() returns Throwable but the method's signature throws
InvalidParameterException.
Perhaps have a local variable for InvalidParameterException exception.
Valerie, are you OK with this?
--Max
Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jun 15, 2016, at 10:57 PM, Mandy Chung wrote:
>
>>> 241 throw (InvalidParameterException)
>>>
>>> This cast should not be needed?
>>>
>>
>> } catch (UcryptoException ue) {
>> throw (InvalidParameterException)
>> new InvalidParameterException("Error using " + configArg).
>> initCause(ue.getCause());
>> }
>>
>> initCause() returns Throwable but the method's signature throws
>> InvalidParameterException.
>>
>
> Perhaps have a local variable for InvalidParameterException exception.
Valerie, are you OK with this?
--Max
>
> Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jun 15, 2016, at 1:27 AM, Wang Weijun wrote:
>
> All suggestions accepted. Webrev updated at
>
> http://cr.openjdk.java.net/~weijun/8130302/webrev.05
>
>> 241 throw (InvalidParameterException)
>>
>> This cast should not be needed?
>>
>
> } catch (UcryptoException ue) {
>throw (InvalidParameterException)
>new InvalidParameterException("Error using " + configArg).
>initCause(ue.getCause());
> }
>
> initCause() returns Throwable but the method's signature throws
> InvalidParameterException.
>
Perhaps have a local variable for InvalidParameterException exception.
Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
All suggestions accepted. Webrev updated at
http://cr.openjdk.java.net/~weijun/8130302/webrev.05
> 241 throw (InvalidParameterException)
>
> This cast should not be needed?
>
} catch (UcryptoException ue) {
throw (InvalidParameterException)
new InvalidParameterException("Error using " + configArg).
initCause(ue.getCause());
}
initCause() returns Throwable but the method's signature throws
InvalidParameterException.
Thanks
Max
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jun 13, 2016, at 8:28 PM, Wang Weijun wrote:
>
> OK, please take a review at the new version at
>
> http://cr.openjdk.java.net/~weijun/8130302/webrev.04/
>
The new -addProvider option is good. I mostly reviewed KeyStoreUtil.java and
skimmed through other files that I assume the security team will review them in
details.
267 * Loads a security provider in a module with its name.
This does not limit to modules. It can load security providers from classpath
via ServiceLoader.
282 for (Provider p : ServiceLoader.load(Provider.class)) {
This should use ServiceLoader.load(Provider.class,
ClassLoader.getSystemClassLoader()) instead of loading with TCCL.
291 throw new IllegalArgumentException();
Nit: good to have a message even if it’s not used.
295 * Loads a non-modularized security provider with its full-qualified
name.
I suggest to reword it to “Loads a security provider by a fully-qualified class
name”
move line 306 to 317
241 throw (InvalidParameterException)
This cast should not be needed?
319 Class clazz;
320 if (cl != null) {
321 clazz = cl.loadClass(provClass);
322 } else {
323 clazz = Class.forName(provClass);
324 }
You should call Class.forName(provClass, false, cl) instead.
Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 14/06/2016 04:28, Wang Weijun wrote: OK, please take a review at the new version at http://cr.openjdk.java.net/~weijun/8130302/webrev.04/ Changes from webrev.03: 1. The new option name -addprovider is used, along with the changes in Resources.java. 2. In KeyStoreUtil::loadProviderByClass, special treatment for "sun.security.pkcs11.SunPKCS11" and "com.oracle.security.crypto.UcryptoProvider". 3. In KeyStoreUtil::loadProviderByName, check if the name is already loaded, configure and add it if necessary. As I said in my previous mail, this can be useful if something like SunPKCS11 is defined inside java.base. 4. Valarie asked me to bring in a change to the OracleUcrypto provider, which allows arbitrary config file. Changes are inside java.policy and UcryptoProvider.java. I assume someone folks on security libraries will review this. A general comment is that the options looks good to me. A minor comment is that the I assume it should be "class name" rather than "classname" in usage message. -Alan.
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
OK, please take a review at the new version at http://cr.openjdk.java.net/~weijun/8130302/webrev.04/ Changes from webrev.03: 1. The new option name -addprovider is used, along with the changes in Resources.java. 2. In KeyStoreUtil::loadProviderByClass, special treatment for "sun.security.pkcs11.SunPKCS11" and "com.oracle.security.crypto.UcryptoProvider". 3. In KeyStoreUtil::loadProviderByName, check if the name is already loaded, configure and add it if necessary. As I said in my previous mail, this can be useful if something like SunPKCS11 is defined inside java.base. 4. Valarie asked me to bring in a change to the OracleUcrypto provider, which allows arbitrary config file. Changes are inside java.policy and UcryptoProvider.java. Thanks Max > On Jun 13, 2016, at 12:23 PM, Mandy Chung wrote: > > >> On Jun 12, 2016, at 11:33 AM, Alan Bateman wrote: >> >> >> >> On 12/06/2016 13:44, Wang Weijun wrote: >>> I was about to send out a new webrev (CCC just approved) but noticed a >>> behavior change. >>> >>> Although "-addprovider SUN" is useless it still worked when I posted >>> webrev.03, but now it failed, because ServiceLoader.load(Provider.class) >>> does not contain "SUN" anymore. Maybe it is inside java.base and loaded in >>> a shortcut mode? >>> >> "SUN" ,"SunJCE", "SunRsaSign", and "SunJSSE" are built-in, I think Valerie >> has code in sun.security.jca.ProviderConfig for this. I don't recall >> java.base ever declaring that it `provides` these providers, except maybe >> via a META-INF/services configuration file for a short period from the >> original JCE work and the dropping the service configuration files. > > I think Alan is right. They were not loaded via ServiceLoader.load because > of the build complexity to get multiple service config files before the > module system went in jdk9. > > As it stands now, no provides java.security.Provider in java.base after > JDK-8157489 is resolved. > > Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jun 12, 2016, at 11:33 AM, Alan Bateman wrote: > > > > On 12/06/2016 13:44, Wang Weijun wrote: >> I was about to send out a new webrev (CCC just approved) but noticed a >> behavior change. >> >> Although "-addprovider SUN" is useless it still worked when I posted >> webrev.03, but now it failed, because ServiceLoader.load(Provider.class) >> does not contain "SUN" anymore. Maybe it is inside java.base and loaded in a >> shortcut mode? >> > "SUN" ,"SunJCE", "SunRsaSign", and "SunJSSE" are built-in, I think Valerie > has code in sun.security.jca.ProviderConfig for this. I don't recall > java.base ever declaring that it `provides` these providers, except maybe via > a META-INF/services configuration file for a short period from the original > JCE work and the dropping the service configuration files. I think Alan is right. They were not loaded via ServiceLoader.load because of the build complexity to get multiple service config files before the module system went in jdk9. As it stands now, no provides java.security.Provider in java.base after JDK-8157489 is resolved. Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 12/06/2016 13:44, Wang Weijun wrote: I was about to send out a new webrev (CCC just approved) but noticed a behavior change. Although "-addprovider SUN" is useless it still worked when I posted webrev.03, but now it failed, because ServiceLoader.load(Provider.class) does not contain "SUN" anymore. Maybe it is inside java.base and loaded in a shortcut mode? "SUN" ,"SunJCE", "SunRsaSign", and "SunJSSE" are built-in, I think Valerie has code in sun.security.jca.ProviderConfig for this. I don't recall java.base ever declaring that it `provides` these providers, except maybe via a META-INF/services configuration file for a short period from the original JCE work and the dropping the service configuration files. -Alan.
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
I was about to send out a new webrev (CCC just approved) but noticed a behavior
change.
Although "-addprovider SUN" is useless it still worked when I posted webrev.03,
but now it failed, because ServiceLoader.load(Provider.class) does not contain
"SUN" anymore. Maybe it is inside java.base and loaded in a shortcut mode?
Therefore I am looking to add some extra lines into the following method:
public static void loadProviderByName(String provName, String arg) {
+ Provider loaded = Security.getProvider(provName);
+ if (loaded != null) {
+ if (arg != null) {
+ loaded = loaded.configure(arg);
+ Security.addProvider(loaded);
+ }
+ return;
+ }
for (Provider p : ServiceLoader.load(Provider.class)) {
if (p.getName().equals(provName)) {
if (arg != null) {
p = p.configure(arg);
}
Security.addProvider(p);
return;
}
}
throw new IllegalArgumentException();
}
Although it might not be worth supporting "-addprovider SUN", but suppose one
day we have a provider inside java.base that requires an argument, this would
be useful.
If you are OK with this, I will send out a webrev.04 soon.
Thanks
Max
> On Feb 23, 2016, at 11:28 AM, Wang Weijun wrote:
>
> Webrev updated at http://cr.openjdk.java.net/~weijun/8130302/webrev.03/.
>
> -provider and loadProviderByName() are useless for jdk9/dev, and
> loadProviderByClass() only uses reflection.
>
> The SunPKCS11 compatibility line will be add in a sub-patch for jake.
>
> --Max
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 23/03/2016 08:12, Wang Weijun wrote: Now that jake/m3 is in jdk9/dev I assume I will just include the jake-related codes and directly push a single changeset to jdk9/dev. Right? Yes, changes like this this can be pushed via jdk9/dev. In the mean-time, we will continue to work in jigsaw/jake as there are many core areas where we'll need to prototype and iterate on. Some of these changes might be disruptive and/or require coordinated changes to the compiler and runtime so we'll work on them here before they go into jdk9/dev. -Alan
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> >> The SunPKCS11 compatibility line will be add in a sub-patch for jake. >> > > You can send me the jake’s delta patch once you push the change to jdk9/dev. I've been busy recently on DRBG and will be back working on this one. Now that jake/m3 is in jdk9/dev I assume I will just include the jake-related codes and directly push a single changeset to jdk9/dev. Right? Thanks Max > > Thanks > Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 26, 2016, at 1:03 AM, Sean Mullan wrote:
>
> On 02/18/2016 03:10 AM, Weijun Wang wrote:
>> There is another compatibility issue which is more important:
>>
>> Today, we tell users to load their own PKCS11 provider with
>>
>> -providerClass sun.security.pkcs11.SunPKCS11 -providerArg some.cfg
>>
>> and seems the new options should be
>>
>> -provider SunPKCS11 -providerArg some.cfg
>
> I thought I had opened a docs bug for that, but I can't find it now. So I
> will add a comment to https://bugs.openjdk.java.net/browse/JDK-8075902 that
> it should also be updated as part of that bug.
Strange I cannot open bugs.openjdk.java.net now.
I am planning to make it compatible in the delta-patch for jake:
public static void loadProviderByClass(
String provClass, String arg, ClassLoader cl) {
Provider prov;
+if (provClass.equals("sun.security.pkcs11.SunPKCS11")) {
+// For compatibility, SunPKCS11 is loadable with -providerClass.
+loadProviderByName("SunPKCS11", arg);
+return;
+}
loading class
Mandy suggested we only support this single class.
--Max
>
> --Sean
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 02/18/2016 03:10 AM, Weijun Wang wrote: There is another compatibility issue which is more important: Today, we tell users to load their own PKCS11 provider with -providerClass sun.security.pkcs11.SunPKCS11 -providerArg some.cfg and seems the new options should be -provider SunPKCS11 -providerArg some.cfg I thought I had opened a docs bug for that, but I can't find it now. So I will add a comment to https://bugs.openjdk.java.net/browse/JDK-8075902 that it should also be updated as part of that bug. --Sean
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 22, 2016, at 7:28 PM, Wang Weijun wrote: > > Webrev updated at http://cr.openjdk.java.net/~weijun/8130302/webrev.03/. > > -provider and loadProviderByName() are useless for jdk9/dev, and > loadProviderByClass() only uses reflection. > I reviewed the provider loading part and looks good to me. > The SunPKCS11 compatibility line will be add in a sub-patch for jake. > You can send me the jake’s delta patch once you push the change to jdk9/dev. Thanks Mandy > --Max > > >> On Feb 23, 2016, at 10:25 AM, Wang Weijun wrote: >> >> You mean not supporting all pre-loaded providers in modules, but only one or two popular ones? >>> >>> I meant not support -providerClass for arbitrary providers loaded via >>> service loader. The only exception is SunPKCS11. In other words, >>> -providerClass can only be used to load legacy security provider via >>> Class::forName. >> >> Accepted. >> >>> >>> The man page should make it clear that -provider SunPKCS11 is recommended >>> instead of -providerClass sun.security.provider.SunPKCS11. It’s just an >>> alias to help migration. >> >> Yes. >> >> security-dev? I still see no comment from you. >> >> Thanks >> Max >> >>> >>> Mandy >>> Thanks Max > > Mandy >>> >> >
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
Webrev updated at http://cr.openjdk.java.net/~weijun/8130302/webrev.03/. -provider and loadProviderByName() are useless for jdk9/dev, and loadProviderByClass() only uses reflection. The SunPKCS11 compatibility line will be add in a sub-patch for jake. --Max > On Feb 23, 2016, at 10:25 AM, Wang Weijun wrote: > > >>> >>> You mean not supporting all pre-loaded providers in modules, but only one >>> or two popular ones? >>> >> >> I meant not support -providerClass for arbitrary providers loaded via >> service loader. The only exception is SunPKCS11. In other words, >> -providerClass can only be used to load legacy security provider via >> Class::forName. > > Accepted. > >> >> The man page should make it clear that -provider SunPKCS11 is recommended >> instead of -providerClass sun.security.provider.SunPKCS11. It’s just an >> alias to help migration. > > Yes. > > security-dev? I still see no comment from you. > > Thanks > Max > >> >> Mandy >> >>> Thanks >>> Max >>> >>> Mandy >>> >> >
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
>> >> You mean not supporting all pre-loaded providers in modules, but only one or >> two popular ones? >> > > I meant not support -providerClass for arbitrary providers loaded via service > loader. The only exception is SunPKCS11. In other words, -providerClass can > only be used to load legacy security provider via Class::forName. Accepted. > > The man page should make it clear that -provider SunPKCS11 is recommended > instead of -providerClass sun.security.provider.SunPKCS11. It’s just an > alias to help migration. Yes. security-dev? I still see no comment from you. Thanks Max > > Mandy > >> Thanks >> Max >> >> >>> >>> Mandy >> >
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 22, 2016, at 5:55 PM, Wang Weijun wrote:
>
303 // A provider in module can also be use class name
304 if (p.getClass().getName().equals(provClass)) {
ProviderConfig::getProvider doesn’t compare the classname. I thought we
agree to discourage the use of -providerClass to load a provider and also
will be consistent with java.security.
>>>
>>> We discourage it, but there are quite some examples like this on the net.
>>> It is the only way to load a SunPKCS11 provider with a user-specified
>>> config file.
>>
>>
>> Is there any particular providers you mostly concern about (SUN, PKCS11?)?
>
> SunPKCS11.
>
> -providerClass has 2 usages now:
>
> 1. Load a 3rd-party provider. This is not a problem. If it's still on
> classpath, -providerClass will still be used. If it's in a module, people
> will know to use -provider with name.
>
> 2. Load a JDK provider with config. Currently this is only SunPKCS11 which I
> want to keep existing command line still working. Some tests have
> "-providerClass sun.security.provider.Sun" but it's useless.
>
>> I prefer to keep -providerClass for legacy non-service providers to avoid
>> inconsistency with java.security config. Perhaps you can add aliases for few
>> specific provider ie. -providerClass sun.security.provider.Sun is alias to
>> -provider SUN and document them in the man page to help migration.
>
> You mean not supporting all pre-loaded providers in modules, but only one or
> two popular ones?
>
I meant not support -providerClass for arbitrary providers loaded via service
loader. The only exception is SunPKCS11. In other words, -providerClass can
only be used to load legacy security provider via Class::forName.
The man page should make it clear that -provider SunPKCS11 is recommended
instead of -providerClass sun.security.provider.SunPKCS11. It’s just an alias
to help migration.
Mandy
> Thanks
> Max
>
>
>>
>> Mandy
>
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
>>>
>>> 303 // A provider in module can also be use class name
>>> 304 if (p.getClass().getName().equals(provClass)) {
>>>
>>> ProviderConfig::getProvider doesn’t compare the classname. I thought we
>>> agree to discourage the use of -providerClass to load a provider and also
>>> will be consistent with java.security.
>>
>> We discourage it, but there are quite some examples like this on the net. It
>> is the only way to load a SunPKCS11 provider with a user-specified config
>> file.
>
>
> Is there any particular providers you mostly concern about (SUN, PKCS11?)?
SunPKCS11.
-providerClass has 2 usages now:
1. Load a 3rd-party provider. This is not a problem. If it's still on
classpath, -providerClass will still be used. If it's in a module, people will
know to use -provider with name.
2. Load a JDK provider with config. Currently this is only SunPKCS11 which I
want to keep existing command line still working. Some tests have
"-providerClass sun.security.provider.Sun" but it's useless.
> I prefer to keep -providerClass for legacy non-service providers to avoid
> inconsistency with java.security config. Perhaps you can add aliases for few
> specific provider ie. -providerClass sun.security.provider.Sun is alias to
> -provider SUN and document them in the man page to help migration.
You mean not supporting all pre-loaded providers in modules, but only one or
two popular ones?
Thanks
Max
>
> Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 21, 2016, at 1:45 AM, Wang Weijun wrote:
>
>
>> On Feb 20, 2016, at 4:01 AM, Mandy Chung wrote:
>>
>>
>>> On Feb 19, 2016, at 2:47 AM, Wang Weijun wrote:
>>>
>>> Updated at the same URL
>>>
>>> http://cr.openjdk.java.net/~weijun/8130302/webrev.01
>>>
>>> The help looks like this now:
>>>
>>> -provider add security provider by name (e.g. SunPKCS11)
>>> [-providerArg ] configure argument for -provider
>>> -providerclass add security provider by fully-qualified classname
>>> [-providerArg ] configure argument for -providerclass
>>>
>>
>> The help message looks good.
>>
>> On the change, I suggest not to duplicate the code from ProviderConfig (I
>> mentioned in my previous reply).
>
> Dup or not dup?
Not to dup. But you have the point better to duplicate the code.
>
>> One way to do is to add sun.security.jca.Providers.addProvider(String name,
>> String argument) that will do the same as loading a provider listed in
>> java.security config file (ProviderConfig::getProvider I believe). I think
>> this change can go into jdk9/dev as ProviderConfig has the right changes
>> there.
>
> I still like to write some new lines. ProviderConfig is not public and I
> don't intend to make it so. Since keytool/jarsigner does not need to care
> about security manager, there is no need for those doPrivileged calls. Also,
> I still want the compatibility lines below.
>
>>
>> 303 // A provider in module can also be use class name
>> 304 if (p.getClass().getName().equals(provClass)) {
>>
>> ProviderConfig::getProvider doesn’t compare the classname. I thought we
>> agree to discourage the use of -providerClass to load a provider and also
>> will be consistent with java.security.
>
> We discourage it, but there are quite some examples like this on the net. It
> is the only way to load a SunPKCS11 provider with a user-specified config
> file.
Is there any particular providers you mostly concern about (SUN, PKCS11?)? I
prefer to keep -providerClass for legacy non-service providers to avoid
inconsistency with java.security config. Perhaps you can add aliases for few
specific provider ie. -providerClass sun.security.provider.Sun is alias to
-provider SUN and document them in the man page to help migration.
Mandy
>
>>
>> 1719 testOK("", "-list -storepass password" +
>> 1720 " -providerClass sun.security.provider.Sun" +
>> 1721 " -keystore x.jks -storetype JKS”);
>>
>> This should use -providerName. You may want to test both
>> “sun.security.provider.Sun” and “SUN”.
>
> -providerName is not needed because KeyPairGenerator will pick it anyway. I
> still need "-providerClass sun.security.provider.Sun" so it runs on jdk9/dev.
> The jake change can use "-provider SUN".
>
>>
>> ProviderConfig::getProvider has some fast path to support both classname and
>> provider name for our built-in security providers for compatibility because
>> these names are used in java.security.
>
> I see them. Performance enhancement? Probably not crucial here since a normal
> user should never use -providerClass to load these providers. -providerClass
> should only be used when 1) a config argument is needed 2) the provider is
> not registered in java.security.
>
> Thanks
> Max
>
>>
>> Mandy
>>
>
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
[I thought I sent out this but cannot find it in my inbox. Send again] New webrev at http://cr.openjdk.java.net/~weijun/8130302/webrev.02/ This is for jdk9/dev. I'll send out a sub-patch for jake, which includes an extra addRead call and more test changes. security-dev, I haven't heard any feedback from you. Please respond. Thanks Max > On Feb 19, 2016, at 6:47 PM, Wang Weijun wrote: > > Updated at the same URL > > http://cr.openjdk.java.net/~weijun/8130302/webrev.01 > > The help looks like this now: > > -provider add security provider by name (e.g. SunPKCS11) > [-providerArg ] configure argument for -provider > -providerclass add security provider by fully-qualified classname > [-providerArg ] configure argument for -providerclass > > --Max > >> On Feb 19, 2016, at 5:58 PM, Wang Weijun wrote: >> >> >>> On Feb 19, 2016, at 5:36 PM, Alan Bateman wrote: >>> >>> >>> On 19/02/2016 09:07, Wang Weijun wrote: : I don't want the line to be too long. Is the preferred terminal width still 80 now? I noticed the java help output still fits with 80 chars but javac is already not. >>> It could fit on a second line, the java launcher usage output does this. I >>> see jarsigner already uses multiple lines so it shouldn't be an issue. >> >> Help screen of jarsigner is hardcoded, but keytool is a little different. I >> will find a way to make it look nice. >> >> --Max >> >
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 20, 2016, at 4:01 AM, Mandy Chung wrote:
>
>
>> On Feb 19, 2016, at 2:47 AM, Wang Weijun wrote:
>>
>> Updated at the same URL
>>
>> http://cr.openjdk.java.net/~weijun/8130302/webrev.01
>>
>> The help looks like this now:
>>
>> -provider add security provider by name (e.g. SunPKCS11)
>> [-providerArg ] configure argument for -provider
>> -providerclass add security provider by fully-qualified classname
>> [-providerArg ] configure argument for -providerclass
>>
>
> The help message looks good.
>
> On the change, I suggest not to duplicate the code from ProviderConfig (I
> mentioned in my previous reply).
Dup or not dup?
> One way to do is to add sun.security.jca.Providers.addProvider(String name,
> String argument) that will do the same as loading a provider listed in
> java.security config file (ProviderConfig::getProvider I believe). I think
> this change can go into jdk9/dev as ProviderConfig has the right changes
> there.
I still like to write some new lines. ProviderConfig is not public and I don't
intend to make it so. Since keytool/jarsigner does not need to care about
security manager, there is no need for those doPrivileged calls. Also, I still
want the compatibility lines below.
>
> 303 // A provider in module can also be use class name
> 304 if (p.getClass().getName().equals(provClass)) {
>
> ProviderConfig::getProvider doesn’t compare the classname. I thought we agree
> to discourage the use of -providerClass to load a provider and also will be
> consistent with java.security.
We discourage it, but there are quite some examples like this on the net. It is
the only way to load a SunPKCS11 provider with a user-specified config file.
>
> 1719 testOK("", "-list -storepass password" +
> 1720 " -providerClass sun.security.provider.Sun" +
> 1721 " -keystore x.jks -storetype JKS”);
>
> This should use -providerName. You may want to test both
> “sun.security.provider.Sun” and “SUN”.
-providerName is not needed because KeyPairGenerator will pick it anyway. I
still need "-providerClass sun.security.provider.Sun" so it runs on jdk9/dev.
The jake change can use "-provider SUN".
>
> ProviderConfig::getProvider has some fast path to support both classname and
> provider name for our built-in security providers for compatibility because
> these names are used in java.security.
I see them. Performance enhancement? Probably not crucial here since a normal
user should never use -providerClass to load these providers. -providerClass
should only be used when 1) a config argument is needed 2) the provider is not
registered in java.security.
Thanks
Max
>
> Mandy
>
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 19, 2016, at 2:47 AM, Wang Weijun wrote:
>
> Updated at the same URL
>
> http://cr.openjdk.java.net/~weijun/8130302/webrev.01
>
> The help looks like this now:
>
> -provider add security provider by name (e.g. SunPKCS11)
> [-providerArg ] configure argument for -provider
> -providerclass add security provider by fully-qualified classname
> [-providerArg ] configure argument for -providerclass
>
The help message looks good.
On the change, I suggest not to duplicate the code from ProviderConfig (I
mentioned in my previous reply). One way to do is to add
sun.security.jca.Providers.addProvider(String name, String argument) that will
do the same as loading a provider listed in java.security config file
(ProviderConfig::getProvider I believe). I think this change can go into
jdk9/dev as ProviderConfig has the right changes there.
303 // A provider in module can also be use class name
304 if (p.getClass().getName().equals(provClass)) {
ProviderConfig::getProvider doesn’t compare the classname. I thought we agree
to discourage the use of -providerClass to load a provider and also will be
consistent with java.security.
1719 testOK("", "-list -storepass password" +
1720 " -providerClass sun.security.provider.Sun" +
1721 " -keystore x.jks -storetype JKS”);
This should use -providerName. You may want to test both
“sun.security.provider.Sun” and “SUN”.
ProviderConfig::getProvider has some fast path to support both classname and
provider name for our built-in security providers for compatibility because
these names are used in java.security.
Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 19/02/2016 11:17, Wang Weijun wrote: : When and which repo should this change go into? Certainly this is all about jake but there are a lot of other changes, esp, the option list. Good question. Ideally jdk9/dev as we are trying to minimize the changes in the jake forest. If you do that they there should only be a small delta patch to carry in jake, right? -Alan.
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 19, 2016, at 6:58 PM, Alan Bateman wrote: > > One other thing is whether we should add a section to JEP 261 on these tool > updates. I had originally assumed that the updates to these tools would > follow the module system into JDK 9 but you've turned up early :-) When and which repo should this change go into? Certainly this is all about jake but there are a lot of other changes, esp, the option list. --Max
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 19/02/2016 10:47, Wang Weijun wrote: Updated at the same URL http://cr.openjdk.java.net/~weijun/8130302/webrev.01 The help looks like this now: -provider add security provider by name (e.g. SunPKCS11) [-providerArg ] configure argument for -provider -providerclass add security provider by fully-qualified classname [-providerArg ] configure argument for -providerclass --Max This looks good to me and I see you've switched to using List too. Mandy may have comments so let's how off pushing this until she looks at the latest patch. One other thing is whether we should add a section to JEP 261 on these tool updates. I had originally assumed that the updates to these tools would follow the module system into JDK 9 but you've turned up early :-) -Alan
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
Updated at the same URL http://cr.openjdk.java.net/~weijun/8130302/webrev.01 The help looks like this now: -provider add security provider by name (e.g. SunPKCS11) [-providerArg ] configure argument for -provider -providerclass add security provider by fully-qualified classname [-providerArg ] configure argument for -providerclass --Max > On Feb 19, 2016, at 5:58 PM, Wang Weijun wrote: > > >> On Feb 19, 2016, at 5:36 PM, Alan Bateman wrote: >> >> >> On 19/02/2016 09:07, Wang Weijun wrote: >>> : >>> I don't want the line to be too long. Is the preferred terminal width still >>> 80 now? I noticed the java help output still fits with 80 chars but javac >>> is already not. >> It could fit on a second line, the java launcher usage output does this. I >> see jarsigner already uses multiple lines so it shouldn't be an issue. > > Help screen of jarsigner is hardcoded, but keytool is a little different. I > will find a way to make it look nice. > > --Max >
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 19, 2016, at 5:36 PM, Alan Bateman wrote: > > > On 19/02/2016 09:07, Wang Weijun wrote: >> : >> I don't want the line to be too long. Is the preferred terminal width still >> 80 now? I noticed the java help output still fits with 80 chars but javac is >> already not. > It could fit on a second line, the java launcher usage output does this. I > see jarsigner already uses multiple lines so it shouldn't be an issue. Help screen of jarsigner is hardcoded, but keytool is a little different. I will find a way to make it look nice. --Max
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 19/02/2016 09:07, Wang Weijun wrote: : I don't want the line to be too long. Is the preferred terminal width still 80 now? I noticed the java help output still fits with 80 chars but javac is already not. It could fit on a second line, the java launcher usage output does this. I see jarsigner already uses multiple lines so it shouldn't be an issue. In general then I think we have a lot of inconsistencies in our command line tools. This goes for option names (more so with the new tools where we've started to use GNU style), whether no args prints usage or not, and of course the usage output itself. I personally find it annoying when usage output wraps (which of course depends on fonts and terminal width). A minor annoyance but the list of providers (and provide classes) is a maintained in jarsigner as a Vector, any reason why this can't be a List? No reason. It's just history. I can fix it. Thank, I realize this is old code. -Alan
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 19, 2016, at 4:42 PM, Alan Bateman wrote: > > On 19/02/2016 08:22, Wang Weijun wrote: >> A new webrev at >> >>http://cr.openjdk.java.net/~weijun/8130302/webrev.01/ >> >> The options for keytool have >> >> -provider [-providerArg ]add a provider by name >> -providerclass [-providerArg ] add a provider by classname >> >> (omit some words because line is too long) >> >> for jarsigner >> >> [-provideradd a security provider by a provider name >> [-providerArg ]] ... configure argument for -provider >> >> [-providerClass add a security provider by a fully-qualified >> classname >> [-providerArg ]] ... configure argument for -providerClass >> >> In the test AltProvider.java, I compiled 2 classes (DummyProvider and >> module-info) for a new module and they manually create the modulepath >> directory. Is there a more formal way to do that? >> > Thanks, this looks quite good. If the usage output "add a provider by name" > had something like "e.g. SunPKCS11" or other example name then I think it > would be clearer. I don't want the line to be too long. Is the preferred terminal width still 80 now? I noticed the java help output still fits with 80 chars but javac is already not. > > A minor annoyance but the list of providers (and provide classes) is a > maintained in jarsigner as a Vector, any reason why this can't be a List? No reason. It's just history. I can fix it. --Max > > -Alan.
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 19/02/2016 08:22, Wang Weijun wrote: A new webrev at http://cr.openjdk.java.net/~weijun/8130302/webrev.01/ The options for keytool have -provider [-providerArg ]add a provider by name -providerclass [-providerArg ] add a provider by classname (omit some words because line is too long) for jarsigner [-provideradd a security provider by a provider name [-providerArg ]] ... configure argument for -provider [-providerClass add a security provider by a fully-qualified classname [-providerArg ]] ... configure argument for -providerClass In the test AltProvider.java, I compiled 2 classes (DummyProvider and module-info) for a new module and they manually create the modulepath directory. Is there a more formal way to do that? Thanks, this looks quite good. If the usage output "add a provider by name" had something like "e.g. SunPKCS11" or other example name then I think it would be clearer. A minor annoyance but the list of providers (and provide classes) is a maintained in jarsigner as a Vector, any reason why this can't be a List? -Alan.
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
A new webrev at http://cr.openjdk.java.net/~weijun/8130302/webrev.01/ The options for keytool have -provider [-providerArg ]add a provider by name -providerclass [-providerArg ] add a provider by classname (omit some words because line is too long) for jarsigner [-provideradd a security provider by a provider name [-providerArg ]] ... configure argument for -provider [-providerClass add a security provider by a fully-qualified classname [-providerArg ]] ... configure argument for -providerClass In the test AltProvider.java, I compiled 2 classes (DummyProvider and module-info) for a new module and they manually create the modulepath directory. Is there a more formal way to do that? Thanks Max > On Feb 18, 2016, at 10:55 PM, Mandy Chung wrote: > > >> On Feb 18, 2016, at 1:52 AM, Alan Bateman wrote: >> >> >> On 18/02/2016 09:08, Weijun Wang wrote: >>> OK, but with -providerClass I'd like to support a class name even if it is >>> already defined in a module as a service and has its own name. This makes >>> sure old commands still work. >> I think it should work fine but I assume we would want to discourage this. >> That is, these security providers are service provider and there should be >> no need for anyone to know the name of the implementation class. >> > > Exactly. > > Note that the current ProviderConfig implementation doesn’t support > security.provider.N= if this is loaded from the another named > module. It should throw IAE. > > Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 18, 2016, at 1:52 AM, Alan Bateman wrote: > > > On 18/02/2016 09:08, Weijun Wang wrote: >> OK, but with -providerClass I'd like to support a class name even if it is >> already defined in a module as a service and has its own name. This makes >> sure old commands still work. > I think it should work fine but I assume we would want to discourage this. > That is, these security providers are service provider and there should be no > need for anyone to know the name of the implementation class. > Exactly. Note that the current ProviderConfig implementation doesn’t support security.provider.N= if this is loaded from the another named module. It should throw IAE. Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 18/02/2016 09:08, Weijun Wang wrote: OK, but with -providerClass I'd like to support a class name even if it is already defined in a module as a service and has its own name. This makes sure old commands still work. I think it should work fine but I assume we would want to discourage this. That is, these security providers are service provider and there should be no need for anyone to know the name of the implementation class. : I think by "master" it means the class implementing Provider but not the one implementing KeyStore or KeyPairGenerator. It's not needed. Thanks, I think the docs + usage message will be clearer once this is dropped. -Alan.
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
OK, but with -providerClass I'd like to support a class name even if it is already defined in a module as a service and has its own name. This makes sure old commands still work. The existing -providerClass takes a class name and works as before. The -provider takes the name of a security provider and locates the provider with that name. For -provider then an example in the usage message would make it very clear. Will file a sub-task. You are right that it would be simple code to fallback and handle both but this will just lead to mis-use and will make it harder to change in the future. For the java.security file then the fallback was important because it seemed common for 3rd party providers to add security providers there. It's not obvious that it is important here. BTW: Docs and help output use the term "provider master class". Is the word "master" needed? It hints of master key or the like but it's really the name of the security provider implementation class. I think by "master" it means the class implementing Provider but not the one implementing KeyStore or KeyPairGenerator. It's not needed. Thanks Max -Alan
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 18/02/2016 08:10, Weijun Wang wrote: : Today, we tell users to load their own PKCS11 provider with -providerClass sun.security.pkcs11.SunPKCS11 -providerArg some.cfg and seems the new options should be -provider SunPKCS11 -providerArg some.cfg Why not just support all these formats? It's not really difficult and I don't think it's harmful, no ambiguity, simple code... I think the current proposal keeps things simple, it's exactly what I was trying to get to in the original mails. The existing -providerClass takes a class name and works as before. The -provider takes the name of a security provider and locates the provider with that name. For -provider then an example in the usage message would make it very clear. You are right that it would be simple code to fallback and handle both but this will just lead to mis-use and will make it harder to change in the future. For the java.security file then the fallback was important because it seemed common for 3rd party providers to add security providers there. It's not obvious that it is important here. BTW: Docs and help output use the term "provider master class". Is the word "master" needed? It hints of master key or the like but it's really the name of the security provider implementation class. -Alan
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
In keytool help, we will write -providerAdd a security provider with its name “Add a security provider by the provider’s name” -providerArgOptional argument for -provider above -providerClass Add a security provider with its class name “Add a security provider by a fully-qualified classname” -providerArgOptional argument for -providerClass above This is also what you are thinking about, right? Yes this matches what I suggest. You think the implementation should strictly match the help above, and I think we can treat -provider and -providerClass the same and perform a try-name-first-try-class-second trick just like what sun.security.jca.ProviderConfig.ProviderLoader::load is doing. You may consider adding a method in sun.security.jca.ProviderConfig for keytool to call rather than duplicating the logic. The -providerClass was introduced in https://bugs.openjdk.java.net/browse/JDK-4938224: To avoid confusion, the -provider option should be renamed to -providerClass. The -provider should still be supported (although not documented) for compatibility. I still see 3 regression tests using -provider this way and I don't want to break them. The option is not documented and repurpose -provider option may not be commonly used. These regression tests should be re-examined whether they can be updated to use -providerClass. Yes, they can be updated, but I can imagine there are external customer doing the same. There is another compatibility issue which is more important: Today, we tell users to load their own PKCS11 provider with -providerClass sun.security.pkcs11.SunPKCS11 -providerArg some.cfg and seems the new options should be -provider SunPKCS11 -providerArg some.cfg Why not just support all these formats? It's not really difficult and I don't think it's harmful, no ambiguity, simple code... --Max
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 17, 2016, at 8:04 PM, Wang Weijun wrote: > >> >> On Feb 18, 2016, at 9:21 AM, Mandy Chung wrote: >> >>> >>> On Feb 17, 2016, at 4:46 PM, Wang Weijun wrote: >>> >>> On Feb 18, 2016, at 5:15 AM, Mandy Chung wrote: Can I say -providerClass -providerArg is equivalent to extending java.security to add “security.provider.N=NAME ARG”? >>> >>> Yes. >>> I suggest to keep -providerClass and -providerArg only for legacy security provider (i.e. not a service provider to java.security.Provider). For security providers that are converted to service provider: What about updating -provider [:] option such that (1) it accepts “provider name” only (not class name) and (2) an optional argument? Although it’s an incompatible change, for legacy security provider, they can still use -providerClass option. >>> >>> Why must only "provider name”? >> >> Consistent with security.provider. specified in java.security. >> >> For security providers in a named module, they must be a service provider. >> Security providers can also be a service provider that will help migration. >> >> security.provider. must specify the name of the security provider which >> is used to compare with the providers loaded by ServiceLoader. A security >> provider can choose to use its fully-qualified classname be the provider >> name if you like. Provider::getName is used to match the specified name (see >> sun.security.jca.ProviderConfig.ProviderLoader) >> >> If the provider is not found via service loader, i.e. >> security.provider.= for legacy security >> providers in unnamed module, it will call Class.forName and newInstance to >> construct the security provider instance. All packages in unnamed modules >> are exported and so Class::newInstance call will succeed (java.base can read >> unnamed module in the implementation). > > In keytool help, we will write > > -providerAdd a security provider with its name “Add a security provider by the provider’s name” > -providerArgOptional argument for -provider above > -providerClass Add a security provider with its class name “Add a security provider by a fully-qualified classname” > -providerArgOptional argument for -providerClass above > > This is also what you are thinking about, right? > Yes this matches what I suggest. > You think the implementation should strictly match the help above, and I > think we can treat -provider and -providerClass the same and perform a > try-name-first-try-class-second trick just like what > sun.security.jca.ProviderConfig.ProviderLoader::load is doing. You may consider adding a method in sun.security.jca.ProviderConfig for keytool to call rather than duplicating the logic. > > The -providerClass was introduced in > https://bugs.openjdk.java.net/browse/JDK-4938224: > > To avoid confusion, the -provider option should > be renamed to -providerClass. The -provider should still > be supported (although not documented) for compatibility. > > I still see 3 regression tests using -provider this way and I don't want to > break them. > The option is not documented and repurpose -provider option may not be commonly used. These regression tests should be re-examined whether they can be updated to use -providerClass. Mandy > --Max > >> >>> >>> We can document this way (-providerClass for legacy and -provider for new) >>> and still treat -providerClass and -provider the same (which is what we are >>> doing now) internally. I cannot see any harm and it is compatible. >>> >>> Even java.security supports both name and class now, right? >>> >> >> See above. >> >> Mandy >> >>> Thanks >>> Max >>> Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 18, 2016, at 9:21 AM, Mandy Chung wrote: > >> >> On Feb 17, 2016, at 4:46 PM, Wang Weijun wrote: >> >> >>> On Feb 18, 2016, at 5:15 AM, Mandy Chung wrote: >>> >>> Can I say -providerClass -providerArg is equivalent to >>> extending java.security to add “security.provider.N=NAME ARG”? >> >> Yes. >> >>> >>> I suggest to keep -providerClass and -providerArg only for legacy security >>> provider (i.e. not a service provider to java.security.Provider). >>> >>> For security providers that are converted to service provider: >>> >>> What about updating -provider [:] option such that (1) it >>> accepts “provider name” only (not class name) and (2) an optional argument? >>> Although it’s an incompatible change, for legacy security provider, they >>> can still use -providerClass option. >> >> Why must only "provider name”? > > Consistent with security.provider. specified in java.security. > > For security providers in a named module, they must be a service provider. > Security providers can also be a service provider that will help migration. > > security.provider. must specify the name of the security provider which is > used to compare with the providers loaded by ServiceLoader. A security > provider can choose to use its fully-qualified classname be the provider name > if you like. Provider::getName is used to match the specified name (see > sun.security.jca.ProviderConfig.ProviderLoader) > > If the provider is not found via service loader, i.e. > security.provider.= for legacy security > providers in unnamed module, it will call Class.forName and newInstance to > construct the security provider instance. All packages in unnamed modules > are exported and so Class::newInstance call will succeed (java.base can read > unnamed module in the implementation). In keytool help, we will write -providerAdd a security provider with its name -providerArgOptional argument for -provider above -providerClass Add a security provider with its class name -providerArgOptional argument for -providerClass above This is also what you are thinking about, right? You think the implementation should strictly match the help above, and I think we can treat -provider and -providerClass the same and perform a try-name-first-try-class-second trick just like what sun.security.jca.ProviderConfig.ProviderLoader::load is doing. The -providerClass was introduced in https://bugs.openjdk.java.net/browse/JDK-4938224: To avoid confusion, the -provider option should be renamed to -providerClass. The -provider should still be supported (although not documented) for compatibility. I still see 3 regression tests using -provider this way and I don't want to break them. --Max > >> >> We can document this way (-providerClass for legacy and -provider for new) >> and still treat -providerClass and -provider the same (which is what we are >> doing now) internally. I cannot see any harm and it is compatible. >> >> Even java.security supports both name and class now, right? >> > > See above. > > Mandy > >> Thanks >> Max >> >>> >>> Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 17, 2016, at 4:46 PM, Wang Weijun wrote: > > >> On Feb 18, 2016, at 5:15 AM, Mandy Chung wrote: >> >> Can I say -providerClass -providerArg is equivalent to >> extending java.security to add “security.provider.N=NAME ARG”? > > Yes. > >> >> I suggest to keep -providerClass and -providerArg only for legacy security >> provider (i.e. not a service provider to java.security.Provider). >> >> For security providers that are converted to service provider: >> >> What about updating -provider [:] option such that (1) it accepts >> “provider name” only (not class name) and (2) an optional argument? >> Although it’s an incompatible change, for legacy security provider, they can >> still use -providerClass option. > > Why must only "provider name”? Consistent with security.provider. specified in java.security. For security providers in a named module, they must be a service provider. Security providers can also be a service provider that will help migration. security.provider. must specify the name of the security provider which is used to compare with the providers loaded by ServiceLoader. A security provider can choose to use its fully-qualified classname be the provider name if you like. Provider::getName is used to match the specified name (see sun.security.jca.ProviderConfig.ProviderLoader) If the provider is not found via service loader, i.e. security.provider.= for legacy security providers in unnamed module, it will call Class.forName and newInstance to construct the security provider instance. All packages in unnamed modules are exported and so Class::newInstance call will succeed (java.base can read unnamed module in the implementation). > > We can document this way (-providerClass for legacy and -provider for new) > and still treat -providerClass and -provider the same (which is what we are > doing now) internally. I cannot see any harm and it is compatible. > > Even java.security supports both name and class now, right? > See above. Mandy > Thanks > Max > >> >> Mandy >
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 18, 2016, at 5:15 AM, Mandy Chung wrote: > > Can I say -providerClass -providerArg is equivalent to extending > java.security to add “security.provider.N=NAME ARG”? Yes. > > I suggest to keep -providerClass and -providerArg only for legacy security > provider (i.e. not a service provider to java.security.Provider). > > For security providers that are converted to service provider: > > What about updating -provider [:] option such that (1) it accepts > “provider name” only (not class name) and (2) an optional argument? Although > it’s an incompatible change, for legacy security provider, they can still use > -providerClass option. Why must only "provider name"? We can document this way (-providerClass for legacy and -provider for new) and still treat -providerClass and -provider the same (which is what we are doing now) internally. I cannot see any harm and it is compatible. Even java.security supports both name and class now, right? Thanks Max > > Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Feb 16, 2016, at 5:20 PM, Weijun Wang wrote: > > > > On 2/16/2016 22:54, Alan Bateman wrote: >> >> On 16/02/2016 14:44, Weijun Wang wrote: >>> Please review the code change at >>> >>> http://cr.openjdk.java.net/~weijun/8130302/webrev.00/ >>> >>> I didn't abandon -providerClass and go all the way to -provideName >>> because -providerClass has a sub-option -providerArg that can be used >>> to further configure the provider. Also I think we still need to >>> support legacy providers that are not defined in modules. >>> >>> With this fix, -providerClass accepts both a provider name and a >>> provider class name. Some doc change will be needed. >> How is -providerName used today? I'm just trying to understand why these >> tools have had both -providerName and -providerClass options when they >> appear to take the same value. > > Technically they are independent. > > With -providerClass/-providerArg, the provider is added into system and > getInstance() calls (of keyStore, KeyPairGenerator, etc) can use it. > > On the other hand, -providerName can be used to specifically tell > KeyPairGenerator which provider to use. For example, although both SUN and > SunPKCS11 providers support RSA key pair generation, you cannot store keys > generated by SunPKCS11 into a file-based keystore because the private key is > kept inside the hardware token. In this case, you might want to tell keytool > which provider should be used. > -providerName is only used to specify the provider name for KeyStore.getInstance(storetype, providerName) call. I think it’s best to keep this as it is. > This bug is about loading providers not registered in java.security, which is > what -providerClass/-providerArg is doing now. -providerClass and > -providerName used to take different values, one class name, and one provider > name. It is after modularization that -providerClass is able to take a > provider name. Can I say -providerClass -providerArg is equivalent to extending java.security to add “security.provider.N=NAME ARG”? I suggest to keep -providerClass and -providerArg only for legacy security provider (i.e. not a service provider to java.security.Provider). For security providers that are converted to service provider: What about updating -provider [:] option such that (1) it accepts “provider name” only (not class name) and (2) an optional argument? Although it’s an incompatible change, for legacy security provider, they can still use -providerClass option. Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 2/17/2016 18:33, Alan Bateman wrote: On 17/02/2016 01:20, Weijun Wang wrote: : Technically they are independent. With -providerClass/-providerArg, the provider is added into system and getInstance() calls (of keyStore, KeyPairGenerator, etc) can use it. On the other hand, -providerName can be used to specifically tell KeyPairGenerator which provider to use. For example, although both SUN and SunPKCS11 providers support RSA key pair generation, you cannot store keys generated by SunPKCS11 into a file-based keystore because the private key is kept inside the hardware token. In this case, you might want to tell keytool which provider should be used. This bug is about loading providers not registered in java.security, which is what -providerClass/-providerArg is doing now. -providerClass and -providerName used to take different values, one class name, and one provider name. It is after modularization that -providerClass is able to take a provider name. What would you think about keeping them independent? That is, the value to -providerName is a security provider name, the value to -providerClass is a class name. The -providerArg can work with both, at least I assume it can because this was the motive for the configure method that Valerie added. -providerClass can be provided multiple times but -providerName can only appear once. If we reuse -providerName as -providerClass in the module era, the existing usage of -providerName (used in getInstance) will break. I ask because the only reason for the java.security file behavior is to preserve legacy usage for those that configured it with class names in the past. In fact, -providerClass has an alias -provider but it's not documented. I don't know about the history but this name seems more appropriate now if my code change is adopted. Thanks Max -Alan
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 17/02/2016 01:20, Weijun Wang wrote: : Technically they are independent. With -providerClass/-providerArg, the provider is added into system and getInstance() calls (of keyStore, KeyPairGenerator, etc) can use it. On the other hand, -providerName can be used to specifically tell KeyPairGenerator which provider to use. For example, although both SUN and SunPKCS11 providers support RSA key pair generation, you cannot store keys generated by SunPKCS11 into a file-based keystore because the private key is kept inside the hardware token. In this case, you might want to tell keytool which provider should be used. This bug is about loading providers not registered in java.security, which is what -providerClass/-providerArg is doing now. -providerClass and -providerName used to take different values, one class name, and one provider name. It is after modularization that -providerClass is able to take a provider name. What would you think about keeping them independent? That is, the value to -providerName is a security provider name, the value to -providerClass is a class name. The -providerArg can work with both, at least I assume it can because this was the motive for the configure method that Valerie added. I ask because the only reason for the java.security file behavior is to preserve legacy usage for those that configured it with class names in the past. -Alan
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 2/16/2016 22:54, Alan Bateman wrote: On 16/02/2016 14:44, Weijun Wang wrote: Please review the code change at http://cr.openjdk.java.net/~weijun/8130302/webrev.00/ I didn't abandon -providerClass and go all the way to -provideName because -providerClass has a sub-option -providerArg that can be used to further configure the provider. Also I think we still need to support legacy providers that are not defined in modules. With this fix, -providerClass accepts both a provider name and a provider class name. Some doc change will be needed. How is -providerName used today? I'm just trying to understand why these tools have had both -providerName and -providerClass options when they appear to take the same value. Technically they are independent. With -providerClass/-providerArg, the provider is added into system and getInstance() calls (of keyStore, KeyPairGenerator, etc) can use it. On the other hand, -providerName can be used to specifically tell KeyPairGenerator which provider to use. For example, although both SUN and SunPKCS11 providers support RSA key pair generation, you cannot store keys generated by SunPKCS11 into a file-based keystore because the private key is kept inside the hardware token. In this case, you might want to tell keytool which provider should be used. This bug is about loading providers not registered in java.security, which is what -providerClass/-providerArg is doing now. -providerClass and -providerName used to take different values, one class name, and one provider name. It is after modularization that -providerClass is able to take a provider name. Thanks Max
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
On 16/02/2016 14:44, Weijun Wang wrote: Please review the code change at http://cr.openjdk.java.net/~weijun/8130302/webrev.00/ I didn't abandon -providerClass and go all the way to -provideName because -providerClass has a sub-option -providerArg that can be used to further configure the provider. Also I think we still need to support legacy providers that are not defined in modules. With this fix, -providerClass accepts both a provider name and a provider class name. Some doc change will be needed. How is -providerName used today? I'm just trying to understand why these tools have had both -providerName and -providerClass options when they appear to take the same value. -Alan.
