Re: [15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration
Some files have trailing spaces.
KnownOIDs.java:
- Is there an order for the fields? I see they are grouped but now always
ordered.
- Unused "import java.security.Provider;"
- 1.3.14.3.2.29 used to be an alias for SHA1withRSA, now it's not. Is this
intended?
- s/_/-/ in process() is now used for
SHA3_224 -> SHA3-224
SHA3_256 -> SHA3-256
SHA3_384 -> SHA3-384
SHA3_512 -> SHA3-512
HmacSHA3_224 -> HmacSHA3-224
HmacSHA3_256 -> HmacSHA3-256
HmacSHA3_384 -> HmacSHA3-384
HmacSHA3_512 -> HmacSHA3-512
SHA3_224withRSA -> SHA3-224withRSA
SHA3_256withRSA -> SHA3-256withRSA
SHA3_384withRSA -> SHA3-384withRSA
SHA3_512withRSA -> SHA3-512withRSA
RSASSA_PSS -> RSASSA-PSS
CHACHA20_POLY1305 -> CHACHA20-POLY1305
Can we just hardcode the stdName in constructor and remove the substitution?
It looks fragile and expensive to me. What if we invent a name like
AES_128overSHA3_256?
- Now that you've added EC curve names starting with a lowercase letter, can
we also use "serverAuth"?
- I wonder if we can split the aliases by hand, i.e. modify
secp256r1("1.2.840.10045.3.1.7",
"secp256r1 [NIST P-256, X9.62 prime256v1]"),
to
secp256r1("1.2.840.10045.3.1.7",
"secp256r1", "NIST P-256", "X9.62 prime256v1"),
After all the names will be split into pieces, and we can also use KnownOIDs
inside NamedCurve.
PBES2Parameters.java:
- It's a little pity we need to hardcode several names here. Is
'o.stdName().startsWith("HmacSHA")' enough? This looks like a hack but can save
us some hassle if we support more later.
Everything else looks fine.
Thanks,
Max
> On May 12, 2020, at 9:25 AM, Valerie Peng wrote:
>
> Thanks for filing the bug for PBES2Parameters class.
>
> Webrev for 8242151 is updated at:
> http://cr.openjdk.java.net/~valeriep/8242151/webrev.03/
>
> It addresses:
>
> - added KnownOIDs for CurveDB class
> - updated the KDF parsing code in PBES2Parameters class to match existing
> behavior
> - removed the String constants of PKCS9Attribute class and commented out its
> constructor which takes String argument
> - use 3rd party aliasing info in AlgorithmId.getName() impl
> - misc changes to KnownOIDs class regarding the register() impl
>
> Thanks,
>
> Valerie
>
> On 5/6/2020 6:59 PM, Weijun Wang wrote:
>>> 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: RFR: 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests
Ping.. -Original Message- From: Thejasvi Voniadka Sent: Friday, May 8, 2020 6:17 AM To: core-libs-...@openjdk.java.net; jmx-...@openjdk.java.net; security-dev@openjdk.java.net Subject: RFR: 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests Hi, May someone please sponsor 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 and FINER. 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 the full patch to be pushed though): http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.simple.00 The patch has been tested on mach5, and all jmxremote tests passed.
Re: [15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration
Thanks for filing the bug for PBES2Parameters class. Webrev for 8242151 is updated at: http://cr.openjdk.java.net/~valeriep/8242151/webrev.03/ It addresses: - added KnownOIDs for CurveDB class - updated the KDF parsing code in PBES2Parameters class to match existing behavior - removed the String constants of PKCS9Attribute class and commented out its constructor which takes String argument - use 3rd party aliasing info in AlgorithmId.getName() impl - misc changes to KnownOIDs class regarding the register() impl Thanks, Valerie On 5/6/2020 6:59 PM, Weijun Wang wrote: 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: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (security)
Incremental changes look fine. Thanks, Valerie On 5/11/2020 2:27 PM, Mikael Vidstedt wrote: New webrev fixing Basic.policy based on Valeries’s comment: webrev: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.03/security/open/webrev/ incremental: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.03/security.incr/open/webrev/ Cheers, Mikael On May 7, 2020, at 8:54 PM, Mikael Vidstedt wrote: New webrev: webrev: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.02/security/open/webrev/ incremental: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.02/security.incr/open/webrev/ Items to be resolved: * File follow-up to drop $ISA support in src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java Cheers, Mikael On May 6, 2020, at 10:05 PM, Mikael Vidstedt wrote: 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] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)
Finally found the bottom of the review. ;) I'll send you minor nits
(unused imports/etc.) in a separate email.
src/jdk.crypto.ec/share/classes/sun/security/ec/ed/EdDSAOperations.java
--
136: Here you are calling sun.security.util.ArrayUtil.reverse() (and
swap()), but you also added a reverse code in EdDSAPublicKeyImpl.java.
I would suggest removing the one in EdDSAPublicKeyImpl and make those
calls call into ArrayUtil.
141: Wouldn't it be better to use a temporary variable here instead of
2 array reversals?
src/jdk.crypto.ec/share/classes/sun/security/ec/ed/EdDSASignature.java
--
115-118: You're calling edKey.getPoint() twice here.
139/150: indent problem.
test/jdk/sun/security/ec/ed/TestEdDSA.java
--
362: Extra //XXX
test/jdk/sun/security/ec/ed/TestEdOps.java
--
Please put in where you got these test vectors.
206: Could you put in a quick note here why you're doing this? i.e. to
avoid draining the system's entropy pool by using a seeded PRNG.
Thanks,
Brad
On 5/4/2020 6:12 PM, Bradford Wetmore wrote:
All minor nits, can be done later if it won't be a problem to make minor
API wording tweaks.
On 5/4/2020 10:17 AM, Anthony Scarpino wrote:
On 2/25/20 12:49 PM, Anthony Scarpino wrote:
Hi
I need a code review for the EdDSA support in JEP 339. The code
builds on the existing java implemented constant time classes used
for XDH and the NIST curves. The change also adds classes to the
public API to support EdDSA operations.
Here is the final code review for the JEP. As the JEP is preparing to
move to Propose-to-Target, if you have comments please state if they
must be fixed as part of the initial putback.
https://cr.openjdk.java.net/~ascarpino/8166597/webrev.05/
Javadoc issues remain throughout java.security.*.
e.g. EdDCPoint
@param y the y-coordinate, represented using a BigInteger
->
@param y the y-coordinate, represented using a @{code BigInteger}
a boolean indicating whether the x-coordinate is odd.
->
a boolean indicating whether the x-coordinate is odd
src/java.base/share/classes/sun/security/provider/SHA3.java
--
114: Is this comment about the 2-bit suffix still correct?
src/java.base/share/classes/sun/security/util/KeyUtil.java
--
2: Copyright date.
110: Do you want to go with the hardcoded values, or fix the commented
out code?
src/java.base/share/classes/java/security/interfaces/EdECPrivateKey.java
--
38: Very minor nit: EdEC reads a bit awkward to my eye: that term isn't
used in the RFC. Change or keep. Maybe:
An EdEC private key
->
An Edwards-Curve private key ...
or
An {@code EdECPrivateKey} ...
Noticed some new typos, or you could do a minor replacement and reduce
some of the duplicate wording.
51:
the an {@code Optional}
->
an {@code Optional}
52:
Ff
->
If
Alternate idea: You could take out the second sentence in the method
description above and replace the @return with:
* @return the private key byte array, or an empty {@code Optional}
if the
* key cannot be extracted (e.g. if the provider is a hardware
token
* and the private key is not allowed to leave the crypto
boundary).
This was based on the XEC wording. Your call/choice.
src/java.base/share/classes/java/security/interfaces/EdECPublicKey.java
--
37: Same comment about EdEC. Change or keep.
src/java.base/share/classes/java/security/spec/EdDSAParameterSpec.java
--
92: Repeat of previous comment from last week. You replied "Ok" but
didn't
see a change yet.
"...bound to the signature" sounds premature. This is the context bound
to the EdDSAParameteerSpec, which hasn't been yet bound to the signature.
src/java.base/share/classes/java/security/spec/EdECPoint.java
--
38: Same comment about EdEC. Change or keep.
test/jdk/sun/security/ec/ed/TestEdDSA.java
--
Nits:
211-212: Indention problem
159/257/258/301: Extra whitespace.
313: The context value was set correctly from a previous test, but
wouldn't hurt to reiterate it here.
Brad
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (security)
New webrev fixing Basic.policy based on Valeries’s comment: webrev: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.03/security/open/webrev/ incremental: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.03/security.incr/open/webrev/ Cheers, Mikael > On May 7, 2020, at 8:54 PM, Mikael Vidstedt > wrote: > > > New webrev: > > webrev: > http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.02/security/open/webrev/ > incremental: > http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.02/security.incr/open/webrev/ > > Items to be resolved: > > * File follow-up to drop $ISA support in > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java > > Cheers, > Mikael > >> On May 6, 2020, at 10:05 PM, Mikael Vidstedt >> wrote: >> >> >> 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 8244674: Third-party code version check
Looks good to me. --Sean On 5/8/20 11:29 PM, Weijun Wang wrote: Please take a review at https://cr.openjdk.java.net/~weijun/8244674/webrev.00 I've consolidated 2 old tests into one and added a new case. Thanks, Max
Re: ECDSA Algorithms not available with jlink
Hi,
Thanks for letting us know about this. We would like to figure out what
might be causing this. However, this is not the best forum for debugging
issues.
Can you please file a bug at https://bugreport.java.com/bugreport/ ?
Can you also attach the debug output when running with "java
-Djava.security.debug=all ..." to the bug report.
A test program that reproduces the issue would also be useful, but
probably not necessary unless we cannot reproduce the issue.
Thanks,
Sean
On 5/11/20 4:50 AM, Michele Adduci wrote:
Hello,
my name is Michele Adduci and I've freshly subscribed to this mailing
list because I'm seeking for advice/help on an issue that arose during
the creation of a new application and I couldn't find my answers by
searching online.
I'm using JDK 14.0.1 GA and trying to create a small redistributable
version of my application using the combo jlink + jpackage.
In my application, I'm trying to validate some signatures with
algorithm such a||s SHA256withECDSA, SHA384withECDSA and
SHA512withECDSA. If I run my application with the standard JDK
distribution, the application is working correctly, but if I run it
with the result of jlink+jpackage, I get errors like
"SHA256withECSA Signature not available"
I've double-checked that libsunec.so is in place under the
'runtime/lib' folder, but I've seen that the SunEC provider isn't
loaded at all during the start (just SUN and SunRSA are being loaded).
The jlink modules I'm using are:
java.desktop,java.smartcardio,java.management,java.sql,java.logging,java.net.http,java.xml.crypto,jdk.crypto.ec
and my jlink command is the following one:
jlink --strip-debug \
--strip-native-commands \
--bind-services \
--no-header-files \
--no-man-pages \
--compress=2 \
--module-path ${JAVA_HOME}/jmods \
--add-modules
java.desktop,java.smartcardio,java.management,java.sql,java.logging,java.net.http,java.xml.crypto,jdk.crypto.ec
\
--output jlink-runtime
What am I doing wrong? The issue is present in both Windows and Linux
- I cannot test on Mac.
Kind regards and thanks in advance for your help
Michele Adduci
ECDSA Algorithms not available with jlink
Hello,
my name is Michele Adduci and I've freshly subscribed to this mailing
list because I'm seeking for advice/help on an issue that arose during
the creation of a new application and I couldn't find my answers by
searching online.
I'm using JDK 14.0.1 GA and trying to create a small redistributable
version of my application using the combo jlink + jpackage.
In my application, I'm trying to validate some signatures with algorithm
such a||s SHA256withECDSA, SHA384withECDSA and SHA512withECDSA. If I
run my application with the standard JDK distribution, the application
is working correctly, but if I run it with the result of jlink+jpackage,
I get errors like
"SHA256withECSA Signature not available"
I've double-checked that libsunec.so is in place under the 'runtime/lib'
folder, but I've seen that the SunEC provider isn't loaded at all during
the start (just SUN and SunRSA are being loaded).
The jlink modules I'm using are:
java.desktop,java.smartcardio,java.management,java.sql,java.logging,java.net.http,java.xml.crypto,jdk.crypto.ec
and my jlink command is the following one:
jlink --strip-debug \
--strip-native-commands \
--bind-services \
--no-header-files \
--no-man-pages \
--compress=2 \
--module-path ${JAVA_HOME}/jmods \
--add-modules
java.desktop,java.smartcardio,java.management,java.sql,java.logging,java.net.http,java.xml.crypto,jdk.crypto.ec
\
--output jlink-runtime
What am I doing wrong? The issue is present in both Windows and Linux -
I cannot test on Mac.
Kind regards and thanks in advance for your help
Michele Adduci
smime.p7s
Description: S/MIME Cryptographic Signature
