Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java
The test can also report which providers it is testing and/or skipping, so that anyone checking the .jtr file can verify the behavior. Maybe it fails if expected modules or providers are not found. -- Jon On 06/29/2016 10:50 AM, Mandy Chung wrote: Valerie’s suggestion is a good one. The test will require a minimum image with cross-platform security providers to run the test while it can still verify other platform-specific providers. Mandy On Jun 29, 2016, at 10:41 AM, Valerie Peng wrote: It's not like the test silently passes as the test still covers the cross-platform modules. The way I view this is that the platform=specific modules are "optional" and we update the expected result by detecting their presence (or the not). It's not a hack or workaround, but rather an enhancement for the test to handle different images. Just my .02, Valerie On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote: On Jun 28, 2016, at 5:22 PM, Valerie Peng wrote: One of the purpose of this test is to test the ordering (see the initial bug which this test is for: JDK-6997010). The original test already detects the OS and will skip certain providers accordingly. Instead of splitting the test into multiple platform-specific tests, maybe we can keep the original test but add module-presence checking? Is there API available to query if certain module are present? ModuleFinder.ofSystem().find(String). We can have only the cross-platform modules listed in @modules and make the test to pass silently if the required platform-specific modules are not present. So, for example, on windows, if the test would be executed against an image which have no jdk.crypto.mscapi, the test will not run any checks and report pass. This would help to avoid the additional test creation, but it will add another silently passing test, which is less clean. Mandy? Shura If yes, then we can leave out the platform-specific providers from the @modules line and skip the providers if either the OS does not match or the module is not present. If we can't query what modules are available, then we may have to think of something else. Valerie On 6/27/2016 12:27 PM, Mandy Chung wrote: I’m including security-dev which would be a better list to review this test fix. Valerie, Does this test have to be order-sensitive? I think this test would be cleaner to make it order-insensitive and simply test the security provider initialization. See my comments below. On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline wrote: Hi. Please take a look on a suggested for for the java/lang/SecurityManager/CheckSecurityProvider.java test. The test in question depend on a list of modules, some of them are platform-specific. Listing all the dependencies in one test is causing the test to be skipped on every platform. In an offline conversation it was decided that it is better to split this tests into a few tests to declare the per-platform module dependencies. The bug: https://bugs.openjdk.java.net/browse/JDK-8158670 The suggested fix: http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/ The copyright header start year of the new tests should be 2016. I would suggest to make CheckSecurityProvide a platform-neutral test, i.e., - drop @requires - make line 94-97 to ignore the platform-dependent provider if it’s present in the white list If we could make this test order-insensitive, it’d be cleaner to maintain a platform-neutral list of security providers and one list for the platform-dependent security providers for each platform. Just an idea. Mandy
Re: [9] RFR 8068290: JCK: 2 api/java_security/cert/ tests fail
Should engineSetKeyEntry make the same check? --Max > On Jul 2, 2016, at 5:52 AM, Vincent Ryan wrote: > > Please review this change to correct two failing JCK tests. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8068290 > Webrev: http://cr.openjdk.java.net/~vinnie/8068290/webrev.00/ > > PKCS12 is the default keystore type in JDK 9 and its implementation stores > only X.509 certificates > although the KeyStore API allows any Certificate object to be stored. > This fix checks that the supplied certificate is an X509Certificate object > before storing it. > > Thanks. >
[9] RFR 8068290: JCK: 2 api/java_security/cert/ tests fail
Please review this change to correct two failing JCK tests. Bug: https://bugs.openjdk.java.net/browse/JDK-8068290 Webrev: http://cr.openjdk.java.net/~vinnie/8068290/webrev.00/ PKCS12 is the default keystore type in JDK 9 and its implementation stores only X.509 certificates although the KeyStore API allows any Certificate object to be stored. This fix checks that the supplied certificate is an X509Certificate object before storing it. Thanks.
Re: [9] RFR 8136459: MessageDigest.isEqual is not a "simple byte compare"
Looks fine. --Sean On 07/01/2016 05:32 PM, Valerie Peng wrote: Sean, Do you have one minute to spare for this trivial javadoc update? Can you please take a look at the CCC too? I plan to fast track it once you reviewed it. Bug: https://bugs.openjdk.java.net/browse/JDK-8136459 Webrev: http://cr.openjdk.java.net/~valeriep/8136459/webrev.00/ Thanks, Valerie
[9] RFR 8136459: MessageDigest.isEqual is not a "simple byte compare"
Sean, Do you have one minute to spare for this trivial javadoc update? Can you please take a look at the CCC too? I plan to fast track it once you reviewed it. Bug: https://bugs.openjdk.java.net/browse/JDK-8136459 Webrev: http://cr.openjdk.java.net/~valeriep/8136459/webrev.00/ Thanks, Valerie
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: 8159180 Remove default setting for jdk.security.provider.preferred
Thanks Tony On 06/29/2016 05:16 PM, Xuelei Fan wrote: Looks fine to me. Thanks, Xuelei On 6/30/2016 2:50 AM, Anthony Scarpino wrote: Hi, I need a review of this simple change to to undo the default settings for the jdk.security.provider.preferred. The only code changes are test related. The test now sets the property to test the previous default functionality. http://cr.openjdk.java.net/~ascarpino/8159180/webrev/ thanks Tony
Re: RFR: 8154015 Apply algorithm constraints to timestamped code
On 07/01/2016 12:39 PM, Sean Mullan wrote:
Looks good, just one comment below ...
On 06/30/2016 05:31 PM, Anthony Scarpino wrote:
Unless otherwise specified below, it was accepted..
http://cr.openjdk.java.net/~ascarpino/8154015/webrev.02/
Tony
PKIX.java:
107 this.params = ((PKIXTimestampParameters)
params).getPKIXBuilderParameters();
Shouldn't this be:
this.params = (PKIXBuilderParameters) params;
The passed in params doesn't itself contain the original
PKIXBuilderParamters data. It's the wrapper with an internal
PKIXBuilderParameter object it holds the data.
getPKIXBuilderParameters() passes the orignal PKIXBuilderParameters
object.
Without it being setup this way I don't see how I can get and set the
timestamp.
Right, you still need to get the timestamp, but all the other methods in
params are overidden and just call through to the wrapped object, so it
seems like you can still do this:
if (params instanceof PKIXTimestampParameters) {
timestamp = ((PKIXTimestampParameters) params).getTimestamp();
this.params = (PKIXBuilderParameters) params;
}
and then you don't need the getPKIXBuilderParameters() method.
--Sean
I see what you're saying now... Additionally, I don't need to typecast
params. I can remove the 'else' and just have 'this.params = params;'
thanks
Tony
Re: RFR: 8154015 Apply algorithm constraints to timestamped code
Looks good, just one comment below ...
On 06/30/2016 05:31 PM, Anthony Scarpino wrote:
Unless otherwise specified below, it was accepted..
http://cr.openjdk.java.net/~ascarpino/8154015/webrev.02/
Tony
PKIX.java:
107 this.params = ((PKIXTimestampParameters)
params).getPKIXBuilderParameters();
Shouldn't this be:
this.params = (PKIXBuilderParameters) params;
The passed in params doesn't itself contain the original
PKIXBuilderParamters data. It's the wrapper with an internal
PKIXBuilderParameter object it holds the data.
getPKIXBuilderParameters() passes the orignal PKIXBuilderParameters object.
Without it being setup this way I don't see how I can get and set the
timestamp.
Right, you still need to get the timestamp, but all the other methods in
params are overidden and just call through to the wrapped object, so it
seems like you can still do this:
if (params instanceof PKIXTimestampParameters) {
timestamp = ((PKIXTimestampParameters) params).getTimestamp();
this.params = (PKIXBuilderParameters) params;
}
and then you don't need the getPKIXBuilderParameters() method.
--Sean
Re: RFR: 8157847: Deprecate the java.security.acl API with forRemoval=true
On 07/01/2016 12:41 PM, Mandy Chung wrote: On Jul 1, 2016, at 5:31 AM, Sean Mullan wrote: Please review the changes for this RFE to mark the java.security.acl API with forRemoval=true. The intention is to remove this API in JDK 10. The APIs in java.security.acl were deprecated in 1.9 but have had the following warning in the package description for many releases: "The classes and interfaces in this package have been superseded by classes in the java.security package. See that package and, for example, java.security.Permission for details." Since there have been suitable replacements since 1.2, there is no reason to retain this package and it should be removed. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8157847/webrev.00/ Should be @since=“9” rather than “1.9”. Otherwise, looks okay. Good catch. Thanks, Sean
Re: RFR: 8157847: Deprecate the java.security.acl API with forRemoval=true
> On Jul 1, 2016, at 5:31 AM, Sean Mullan wrote: > > Please review the changes for this RFE to mark the java.security.acl API with > forRemoval=true. The intention is to remove this API in JDK 10. > > The APIs in java.security.acl were deprecated in 1.9 but have had the > following warning in the package description for many releases: > > "The classes and interfaces in this package have been superseded by classes > in the java.security package. See that package and, for example, > java.security.Permission for details." > > Since there have been suitable replacements since 1.2, there is no reason to > retain this package and it should be removed. > > webrev: http://cr.openjdk.java.net/~mullan/webrevs/8157847/webrev.00/ Should be @since=“9” rather than “1.9”. Otherwise, looks okay. Mandy
Re: [9] RFR 8157667: sun/security/x509/URICertStore/ExtensionsWithLDAP.java has to be updated due to JDK-8134577
Ping again ;-) John Jiang On 2016/6/28 6:42, John Jiang wrote: Please review this patch. Thanks! John Jiang On 2016/6/24 16:55, John Jiang wrote: Hi, Due to JDK-8134577, the test sun/security/x509/URICertStore/ExtensionsWithLDAP.java cannot resolve hosts from a local name service provider. This fix creates a simple Socks4 proxy, and use it to resolve hosts from Socket. Issue: https://bugs.openjdk.java.net/browse/JDK-8157667 Webrev: http://cr.openjdk.java.net/~jjiang/8157667/webrev.01 Please note that, this fix provides two separated local hosts files. Each of them contains only one host entry. And the test is executed twice in deed. Each time, the test focus only one specific host, and uses one hosts file accordingly. If one hosts file includes two entries, like the below, 127.0.0.1 ldap.host.for.crldp 127.0.0.1 ldap.host.for.aia then, only the first host, ldap.host.for.crldp, is resolved by the proxy. If I misunderstand something, or you have better solution, please let me know. Thanks! John Jiang
Re: RFR: 8157847: Deprecate the java.security.acl API with forRemoval=true
Change looks fine. Thanks Max > On Jul 1, 2016, at 8:31 PM, Sean Mullan wrote: > > Please review the changes for this RFE to mark the java.security.acl API with > forRemoval=true. The intention is to remove this API in JDK 10. > > The APIs in java.security.acl were deprecated in 1.9 but have had the > following warning in the package description for many releases: > > "The classes and interfaces in this package have been superseded by classes > in the java.security package. See that package and, for example, > java.security.Permission for details." > > Since there have been suitable replacements since 1.2, there is no reason to > retain this package and it should be removed. > > webrev: http://cr.openjdk.java.net/~mullan/webrevs/8157847/webrev.00/ > > --Sean
RFR: 8157847: Deprecate the java.security.acl API with forRemoval=true
Please review the changes for this RFE to mark the java.security.acl API with forRemoval=true. The intention is to remove this API in JDK 10. The APIs in java.security.acl were deprecated in 1.9 but have had the following warning in the package description for many releases: "The classes and interfaces in this package have been superseded by classes in the java.security package. See that package and, for example, java.security.Permission for details." Since there have been suitable replacements since 1.2, there is no reason to retain this package and it should be removed. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8157847/webrev.00/ --Sean
Re: RFR: 8157707: Deprecate the java.security.Certificate API with forRemoval=true
looks fine to me.
Xuelei
> On Jul 1, 2016, at 8:08 PM, Sean Mullan wrote:
>
> Please review the changes for this RFE to mark the java.security.Certificate
> API with forRemoval=true. The intention is to remove this API in JDK 10.
>
> The java.security.Certificate API has been deprecated since 1.2 and has been
> superseded by java.security.cert.Certificate and related classes since 1.2.
> It is no longer necessary to retain this class.
>
> --- a/src/java.base/share/classes/java/security/Certificate.javaWed Jun
> 15 14:21:24 2016 +0200
> +++ b/src/java.base/share/classes/java/security/Certificate.javaFri Jul
> 01 08:07:25 2016 -0400
> @@ -60,9 +60,10 @@
> * This Certificate interface is entirely deprecated and
> * is here to allow for a smooth transition to the new
> * package.
> + * This class is subject to removal in a future version of Java
> SE.
> * @see java.security.cert.Certificate
> */
> -@Deprecated
> +@Deprecated(since="1.2", forRemoval=true)
> public interface Certificate {
>
> --Sean
RFR: 8157707: Deprecate the java.security.Certificate API with forRemoval=true
Please review the changes for this RFE to mark the
java.security.Certificate API with forRemoval=true. The intention is to
remove this API in JDK 10.
The java.security.Certificate API has been deprecated since 1.2 and has
been superseded by java.security.cert.Certificate and related classes
since 1.2. It is no longer necessary to retain this class.
--- a/src/java.base/share/classes/java/security/Certificate.java Wed Jun
15 14:21:24 2016 +0200
+++ b/src/java.base/share/classes/java/security/Certificate.java Fri Jul
01 08:07:25 2016 -0400
@@ -60,9 +60,10 @@
* This Certificate interface is entirely deprecated and
* is here to allow for a smooth transition to the new
* package.
+ * This class is subject to removal in a future version of
Java SE.
* @see java.security.cert.Certificate
*/
-@Deprecated
+@Deprecated(since="1.2", forRemoval=true)
public interface Certificate {
--Sean
