Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (security)
New webrev here: webrev: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/security/open/webrev/ incremental: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/security.incr/open/webrev/ Items yet to be resolved: * File follow-up to drop $ISA support in src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java * Get confirmation on the “solaris” property in test/jdk/sun/security/tools/keytool/KeyToolTest.java * Get confirmation if https://bugs.openjdk.java.net/browse/JDK-8039280 is enough to cover test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.java * Get confirmation on what to do about the "(secret == null)” block in test/jdk/sun/security/pkcs11/tls/TestPRF.java Cheers, Mikael > On May 3, 2020, at 10:12 PM, Mikael Vidstedt > wrote: > > > Please review this change which implements part of JEP 381: > > JBS: https://bugs.openjdk.java.net/browse/JDK-8244224 > webrev: > http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/security/open/webrev/ > JEP: https://bugs.openjdk.java.net/browse/JDK-8241787 > > > Note: When reviewing this, please be aware that this exercise was *extremely* > mind-numbing, so I appreciate your help reviewing all the individual changes > carefully. You may want to get that coffee cup filled up (or whatever keeps > you awake)! > > > Background: > > Because of the size of the total patch and wide range of areas touched, this > patch is one out of in total six partial patches which together make up the > necessary changes to remove the Solaris and SPARC ports. The other patches > are being sent out for review to mailing lists appropriate for the respective > areas the touch. An email will be sent to jdk-dev summarizing all the > patches/reviews. To be clear: this patch is *not* in itself complete and > stand-alone - all of the (six) patches are needed to form a complete patch. > Some changes in this patch may look wrong or incomplete unless also looking > at the corresponding changes in other areas. > > For convenience, I’m including a link below[1] to the full webrev, but in > case you have comments on changes in other areas, outside of the files > included in this thread, please provide those comments directly in the thread > on the appropriate mailing list for that area if possible. > > In case it helps, the changes were effectively produced by searching for and > updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. > More information about the areas impacted can be found in the JEP itself. > > > Testing: > > A slightly earlier version of this change successfully passed tier1-8, as > well as client tier1-2. Additional testing will be done after the first round > of reviews has been completed. > > Cheers, > Mikael > > [1] > http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (security)
> On May 5, 2020, at 7:29 PM, Weijun Wang wrote:
>
>
>
>> On May 6, 2020, at 6:51 AM, Mikael Vidstedt
>> wrote:
>>
>>
>> Max,
>>
>> Thank you so much for the thorough review! I’m working on an incremental
>> webrev but could use some help - comments/questions inline..
>>
>>> On May 4, 2020, at 6:58 AM, Weijun Wang wrote:
>>>
>>> I've gone through all files. Many of them are PKCS11-related, it will be
>>> nice if Valerie can confirm my findings.
>>>
>>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java:
>>>
>>> Maybe we should not support $ISA at all?
>>
>> It does seem like it could go, but I’m not comfortable making that change
>> myself. How about a follow-up issue?
>
> Sure.
>
>>
>>> test/jdk/com/sun/security/auth/login/ConfigFile/InconsistentError.java:
>>>
>>> Please also remove the whole else block from line 61.
>>
>> Fixed.
>>
>>> test/jdk/java/security/KeyPairGenerator/SolarisShortDSA.java:
>>>
>>> It's not worth keeping this test. Error has never occurred on other
>>> platforms.
>>
>> Fixed.
>>
>>> test/jdk/java/security/KeyStore/TestKeyStoreBasic.java:
>>>
>>> 128-135: There is no need to use a try-catch block.
>>
>> Fixed.
>>
>>> test/jdk/java/security/KeyStore/TestKeyStoreEntry.java:
>>>
>>> 54-60: No need
>>
>> Fixed.
>>
>>> 81-97: No need. Just a single run() is OK.
>>> 101: Remove the ", p” argument
>>
>> I’m not sure I understand what to do here. Help? :)
>
> Since there is only one provider having "jceks" you can
>
> 1 remove run() method
> 2 rename runTest(p) to run()
> 3 call 'KeyStore.getInstance("jceks")' without the "p" argument.
Thank you! I did exactly that.
>
>>
>>> test/jdk/java/security/MessageDigest/TestDigestIOStream.java:
>>> test/jdk/java/security/MessageDigest/TestSameLength.java:
>>> test/jdk/java/security/MessageDigest/TestSameValue.java:
>>>
>>> No need for isSHA3Supported(). The SUN provider should always be there.
>>
>> Fixed.
>>
>>> Inside the test where NoSuchAlgorithmException is caught, the catch block
>>> can be removed and no need to try
>>
>> Fixed.
>>
>>> test/jdk/java/security/MessageDigest/UnsupportedProvider.java:
>>>
>>> Not worth keeping this test.
>>
>> Fixed.
>>
>>> test/jdk/sun/security/jca/PreferredProviderTest.java:
>>>
>>> 53: remove "for other platform”
>>
>> Fixed.
>>
>>> test/jdk/sun/security/pkcs/pkcs8/PKCS8Test.java
>>>
>>> Remove the comments on lines 37-40
>>
>> Fixed.
>>
>>> test/jdk/sun/security/pkcs11/Config/ReadConfInUTF16Env.java:
>>>
>>> Not worth keeping the tests in this directory.
>>
>> Fixed.
>>
>>> test/jdk/sun/security/pkcs11/Mac/MacSameTest.java:
>>>
>>> No need for try
>>
>> Fixed.
>>
>>> test/jdk/sun/security/pkcs11/Provider/ConfigShortPath.java:
>>>
>>> Will this test ever run? There is no out-of-box SunPKCS11 provider. Even if
>>> one is configured manually, the name is likely to be SunPKCS11-XYZ,
>>
>> You tell me? :)
>
> This is why I included Valerie, I'll ping her.
Sounds good, I’ll wait for Valerie’s feedback.
Cheers,
Mikael
>
> Thanks,
> Max
>
>>
>>> test/jdk/sun/security/pkcs11/tls/TestKeyMaterial.java:
>>>
>>> No need for this catch block
>>
>> Fixed (removed the InvalidAlgorithmParameterException catch).
>>
>>> test/jdk/sun/security/tools/keytool/fakegen/PSS.java:
>>>
>>> Please also remove the comment on lines 35-37.
>>
>> Fixed.
>>
>>> test/jdk/sun/security/tools/keytool/fakegen/DefaultSignatureAlgorithm.java:
>>>
>>> Please also remove the comment on lines 36-38.
>>
>> Fixed.
>>
>> Cheers,
>> Mikael
>>
>>>
On May 4, 2020, at 1:12 PM, Mikael Vidstedt
wrote:
Please review this change which implements part of JEP 381:
JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
webrev:
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/security/open/webrev/
JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
Note: When reviewing this, please be aware that this exercise was
*extremely* mind-numbing, so I appreciate your help reviewing all the
individual changes carefully. You may want to get that coffee cup filled
up (or whatever keeps you awake)!
Background:
Because of the size of the total patch and wide range of areas touched,
this patch is one out of in total six partial patches which together make
up the necessary changes to remove the Solaris and SPARC ports. The other
patches are being sent out for review to mailing lists appropriate for the
respective areas the touch. An email will be sent to jdk-dev summarizing
all the patches/reviews. To be clear: this patch is *not* in itself
complete and stand-alone - all of the (six) patches are needed to form a
complete patch. Some changes in this patch may look wrong or incomplete
unless also looking at the corresponding changes in other areas.
For convenience, I’m including a link
Re: [15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration
> > It seems that existing impl of PBES2Parameters class only enforces that the > KDF algo is one of the HmacSHAxxx. But it does not throw exception if the > instance is requested with "PBEWithHmacSHA256AndAES_256" and then initialized > with DER encoding containing "PBEWithHmacSHA512AndAES_256". Perhaps it should > be further tightened up? I think so. There is a general "PBES2" that allows filling in the algorithms at init() but if they are already inside the algorithm name, then only the same can appear in the encoding. Filed https://bugs.openjdk.java.net/browse/JDK-8244564. Maybe we will backport it. --Max
Re: [15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration
Hi Max,
Thanks for the review, I find your comments very useful.
Please find responses inline.
On 5/6/2020 5:48 AM, Weijun Wang wrote:
- PBES2Parameters.java:
In parseKDF(), any OID can be accepted as kdfAlgo, wonder if it would lead to
unexpected result:
jshell> var a = AlgorithmParameters.getInstance("PBEWithHmacSHA256AndAES_256")
jshell> a.init(new PBEParameterSpec("hello".getBytes(), 100, new
IvParameterSpec("iv".getBytes(
jshell> var b = a.getEncoded()
jshell> b
b ==> byte[61] { 48, 59, 48, 40, 6, 9, 42, -122, 72, -122, -9, 13, 1, 5, 12,
48, 27, 4, 5, 104, 101, 108, 108, 111, 2, 1, 100, 2, 1, 32, 48, 12, 6, 8, 42,
-122, 72, -122, -9, 13, 2, 9, 5, 0, 48, 15, 6, 9, 96, -122, 72, 1, 101, 3, 4, 1,
42, 4, 2, 105, 118 }
// Modify HmacSHA512("1.2.840.113549.2.11") to MD2("1.2.840.113549.2.2")
jshell> b[41] = 2
$44 ==> 2
jshell> b
b ==> byte[61] { 48, 59, 48, 40, 6, 9, 42, -122, 72, -122, -9, 13, 1, 5, 12,
48, 27, 4, 5, 104, 101, 108, 108, 111, 2, 1, 100, 2, 1, 32, 48, 12, 6, 8, 42,
-122, 72, -122, -9, 13, 2, 2, 5, 0, 48, 15, 6, 9, 96, -122, 72, 1, 101, 3, 4, 1,
42, 4, 2, 105, 118 }
jshell> a = AlgorithmParameters.getInstance("PBEWithHmacSHA256AndAES_256")
jshell> a.init(b)
jshell> a
a ==> PBEWithMD2AndAES_256
Ok, good catch, I will add some check back in (webrev.03) to at least
match exsting impl.
It seems that existing impl of PBES2Parameters class only enforces that
the KDF algo is one of the HmacSHAxxx. But it does not throw exception
if the instance is requested with "PBEWithHmacSHA256AndAES_256" and then
initialized with DER encoding containing "PBEWithHmacSHA512AndAES_256".
Perhaps it should be further tightened up?
- PKCS9Attribute.java:
It looks you can declare and initialize and at the same time make it an element
in an array at the same time, like this
public static final ObjectIdentifier EMAIL_ADDRESS_OID = PKCS9_OIDS[1] =
ObjectIdentifier.of(KnownOIDs.EmailAddress);
Then there is no need to write two lines
EMAIL_ADDRESS_OID = PKCS9_OIDS[1] =
ObjectIdentifier.of(KnownOIDs.EmailAddress);
public static final ObjectIdentifier EMAIL_ADDRESS_OID;
Also, looks like the strings are never used anywhere inside JDK
public static final String EMAIL_ADDRESS_STR = "EmailAddress";
...
Can we remove them? If we want them back, it can be KnownOIDs.stdName.
Ok, I removed them and updated some more source and test to use the
other PKCS9Attribute constructor which takes an oid instead. It seems
that the constructor which takes String then become unused with this
change, so I commented it out (in webrev.03).
- AlgorithmId.java:
getName(): Can we use the collected map from 3rd party providers? I asked this
last time.
Sure. I have just replied to your other email. Will include in webrev.03.
- OIDMap.java:
Maybe we can simplify this file later.
Yes, looks like some more house scrubbing is needed later.
- KnownOIDs.java:
register(): Are you throwing a RuntimeException only when debug is true? This
sounds incorrect.
"already registered; no need to continue;" Can this happen? You
don't manually register preferred ones now.
Correct, should have removed the debug check. Missed this one.
Also removed the "already registered" part of code as it is now obsolete
(in webrev.03).
Thanks,
Valerie
No other comment.
Thanks,
Max
p.s. JGSS uses ObjectIdentifier also, but it also has its own public Oid class.
Maybe we can think about it in a different RFE.
On May 6, 2020, at 11:18 AM, Valerie Peng wrote:
Hi Max,
Webrev has been updated,
http://cr.openjdk.java.net/~valeriep/8242151/webrev.02/.
Major changes are:
- Moved oidTable caching from AlgorithmId class to ObjectIdentifier class. Made
ObjectIdentifier constructor private and callers have to use the of(String)
method which always check the oidTable cache before creating new instances.
Some more files (src files and tests) are added to the webrev due to the
private ObjectIdentifier constructor change.
- Arrays.asList() calls are now replaced with List.of() calls.
- KnownOIDs enum has been enhanced with registerName() method for ensuring that
same standard name can have at most one enum mapping. Added the method
stdName() instead of relying on toString(). Added aliases support to KnownOIDs
enums. Note that external aliases are in SecurityProviderConstants class. The
two non-oid BASE ones are removed and keytool/Main.java is updated.
- SecurityProviderConstants class will pick up the internal aliases and combine
with external aliases and handle multiple oid enums in its store(...) method.
- Updated SunRsaSign provider to use the same naming convention (append the
letter A) and fixed its KeyFactory and KeyPairGenerator to use the same oid as
before. Also update providers outside of the java.base module in a similar
fashion.
As for the comments below. Please find replies inline.
On 5/4/2020 6:24 PM,
Re: [15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration
Sure, I can include this in. Will address in webrev.03. Thanks, Valerie On 5/5/2020 3:33 AM, Weijun Wang wrote: I am playing with keytool + BouncyCastle and generate a key pair using the sigalg "SHA3-256WITHECDSA", and `keytool -list` cannot show the signature name. So I tried 2 changes in AlgorithmId.java: 1. add both the name->oid and oid->name mapping inside collectOIDAliases() for 3rd party providers 2. fallback to this mapping if KnownOIDs.findMatch() fails in getName(). Then I can see the signature name. You can consider including this or we can make this in a future RFE. Thanks, Max
Re: RFR: 8231958: Test for JDK-8228825: Enhance ECDSA
Ignore this review thread for now, I need to rework the fix. ~Shivangi On 06/05/20 11:00 PM, shivangi.g.gu...@oracle.com wrote: Hi, May I please find a sponsor for this patch? Bug: https://bugs.openjdk.java.net/browse/JDK-8231958 Description: There were few curves which were removed from supported list as part of JDK-8228825. These can be added back to supported list with jdk.tls.namedGroups property. The test “test/jdk/sun/security/ssl/CipherSuite/SupportedGroups.java” is enhanced to test one of those curves.As these curves are not supported by TLS1.3 so bypassed that specific curve for TLS1.3. Webrev: http://cr.openjdk.java.net/~sshivang/reviews/8231958/webrev.00/ The patch has been tested on mach5, and the individual test was passed. Thanks Shivangi
Re: openjdk 13 + ; SunEC, secp256r1 ecc encryption
On 5/6/20 7:42 AM, jjk wrote: Dear SMEs Do openJDk versions 13 and above support EC encryption? for me secp256r1 curve is enough as it is common amongst several other platforms (non-java). Is this something natively supported (13+)? I saw various on sunec.jar, native system.load(..) etc. Appreciate if someone can also point me to a simple working code for encryption and decryption. PS: I want to avoid third party libs such as BC etc.. if possible Thanks in anticipation -- Sent from: http://openjdk.5641.n7.nabble.com/OpenJDK-Security-Development-f69724.html Hi, spec256r1 is supported in all current releases. The native implementation is part of the java release. 14 uses a java only implementation and not the native implementation. A quick search for "java sign verify ECDSA" on google will provide you many examples on how to use it. Tony
openjdk 13 + ; SunEC, secp256r1 ecc encryption
Dear SMEs Do openJDk versions 13 and above support EC encryption? for me secp256r1 curve is enough as it is common amongst several other platforms (non-java). Is this something natively supported (13+)? I saw various on sunec.jar, native system.load(..) etc. Appreciate if someone can also point me to a simple working code for encryption and decryption. PS: I want to avoid third party libs such as BC etc.. if possible Thanks in anticipation -- Sent from: http://openjdk.5641.n7.nabble.com/OpenJDK-Security-Development-f69724.html
RFR: 8231958: Test for JDK-8228825: Enhance ECDSA
Hi, May I please find a sponsor for this patch? Bug: https://bugs.openjdk.java.net/browse/JDK-8231958 Description: There were few curves which were removed from supported list as part of JDK-8228825. These can be added back to supported list with jdk.tls.namedGroups property. The test “test/jdk/sun/security/ssl/CipherSuite/SupportedGroups.java” is enhanced to test one of those curves.As these curves are not supported by TLS1.3 so bypassed that specific curve for TLS1.3. Webrev: http://cr.openjdk.java.net/~sshivang/reviews/8231958/webrev.00/ The patch has been tested on mach5, and the individual test was passed. Thanks Shivangi
Re: [15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration
- PBES2Parameters.java:
In parseKDF(), any OID can be accepted as kdfAlgo, wonder if it would lead to
unexpected result:
jshell> var a = AlgorithmParameters.getInstance("PBEWithHmacSHA256AndAES_256")
jshell> a.init(new PBEParameterSpec("hello".getBytes(), 100, new
IvParameterSpec("iv".getBytes(
jshell> var b = a.getEncoded()
jshell> b
b ==> byte[61] { 48, 59, 48, 40, 6, 9, 42, -122, 72, -122, -9, 13, 1, 5, 12,
48, 27, 4, 5, 104, 101, 108, 108, 111, 2, 1, 100, 2, 1, 32, 48, 12, 6, 8, 42,
-122, 72, -122, -9, 13, 2, 9, 5, 0, 48, 15, 6, 9, 96, -122, 72, 1, 101, 3, 4,
1, 42, 4, 2, 105, 118 }
// Modify HmacSHA512("1.2.840.113549.2.11") to MD2("1.2.840.113549.2.2")
jshell> b[41] = 2
$44 ==> 2
jshell> b
b ==> byte[61] { 48, 59, 48, 40, 6, 9, 42, -122, 72, -122, -9, 13, 1, 5, 12,
48, 27, 4, 5, 104, 101, 108, 108, 111, 2, 1, 100, 2, 1, 32, 48, 12, 6, 8, 42,
-122, 72, -122, -9, 13, 2, 2, 5, 0, 48, 15, 6, 9, 96, -122, 72, 1, 101, 3, 4,
1, 42, 4, 2, 105, 118 }
jshell> a = AlgorithmParameters.getInstance("PBEWithHmacSHA256AndAES_256")
jshell> a.init(b)
jshell> a
a ==> PBEWithMD2AndAES_256
- PKCS9Attribute.java:
It looks you can declare and initialize and at the same time make it an element
in an array at the same time, like this
public static final ObjectIdentifier EMAIL_ADDRESS_OID = PKCS9_OIDS[1] =
ObjectIdentifier.of(KnownOIDs.EmailAddress);
Then there is no need to write two lines
EMAIL_ADDRESS_OID = PKCS9_OIDS[1] =
ObjectIdentifier.of(KnownOIDs.EmailAddress);
public static final ObjectIdentifier EMAIL_ADDRESS_OID;
Also, looks like the strings are never used anywhere inside JDK
public static final String EMAIL_ADDRESS_STR = "EmailAddress";
...
Can we remove them? If we want them back, it can be KnownOIDs.stdName.
- AlgorithmId.java:
getName(): Can we use the collected map from 3rd party providers? I asked this
last time.
- OIDMap.java:
Maybe we can simplify this file later.
- KnownOIDs.java:
register(): Are you throwing a RuntimeException only when debug is true? This
sounds incorrect.
"already registered; no need to continue;" Can this happen? You
don't manually register preferred ones now.
No other comment.
Thanks,
Max
p.s. JGSS uses ObjectIdentifier also, but it also has its own public Oid class.
Maybe we can think about it in a different RFE.
> On May 6, 2020, at 11:18 AM, Valerie Peng wrote:
>
> Hi Max,
>
> Webrev has been updated,
> http://cr.openjdk.java.net/~valeriep/8242151/webrev.02/.
>
> Major changes are:
>
> - Moved oidTable caching from AlgorithmId class to ObjectIdentifier class.
> Made ObjectIdentifier constructor private and callers have to use the
> of(String) method which always check the oidTable cache before creating new
> instances. Some more files (src files and tests) are added to the webrev due
> to the private ObjectIdentifier constructor change.
>
> - Arrays.asList() calls are now replaced with List.of() calls.
>
> - KnownOIDs enum has been enhanced with registerName() method for ensuring
> that same standard name can have at most one enum mapping. Added the method
> stdName() instead of relying on toString(). Added aliases support to
> KnownOIDs enums. Note that external aliases are in SecurityProviderConstants
> class. The two non-oid BASE ones are removed and keytool/Main.java is updated.
>
> - SecurityProviderConstants class will pick up the internal aliases and
> combine with external aliases and handle multiple oid enums in its store(...)
> method.
>
> - Updated SunRsaSign provider to use the same naming convention (append the
> letter A) and fixed its KeyFactory and KeyPairGenerator to use the same oid
> as before. Also update providers outside of the java.base module in a similar
> fashion.
>
> As for the comments below. Please find replies inline.
>
> On 5/4/2020 6:24 PM, Weijun Wang wrote:
>> Do you want to add OIDs in CurveDB into KnownOIDs as well?
>
> Sure, will do in webrev.03.
>
> Thanks,
>
> Valerie
>
>>
>> Thanks,
>> Max
[PATCH] 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests
Hello, A quick follow-up on this request.. -Original Message- From: Thejasvi Voniadka Sent: Monday, May 4, 2020 12:56 PM To: core-libs-...@openjdk.java.net Subject: [PATCH] 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests Hi, May I please find a sponsor for this patch? Bug: https://bugs.openjdk.java.net/browse/JDK-8244199 Description: The test "test/jdk/sun/management/jmxremote/bootstrap/RmiSslBootstrapTest.sh" fails intermittently on a lower release. While investigating the cause for the failure, I felt the diagnostic messaging built into the test was not adequate. Some of the issues observed: 1. If the test throws a RuntimeException, the full stack trace is not displayed. 2. The test has several logging statements. However, the logging environment is not properly initialized to allow levels such as FINE when needed. 3. The logging sequence could be improved: some messages end up at stdout, and others at stderr, which makes it harder to follow the output. The patch is to improve the test in these lines. Webrev: http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.full.00 I have also used this opportunity to clean-up the test code as a whole, in lines of removal of redundant logic, formatting, coding guidelines, etc If the above patch looks too confusing and is hard to follow, here is a simpler version that shows only the core logic changes (I intend to submit only the full patch to be committed though): http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.simple.00 The patch has been tested on mach5, and all jmxremote tests passed.
