Re: RFR[8u252] - MR3 - ALPN & RSASSA-PSS in Java SE 8

[Andrew John Hughes]

On 13/02/2020 22:32, Bradford Wetmore wrote:

snip...

>>
>> I wasn't looking at the web pages, but just at the patch file
>> (https://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS/webrev.01/jdk.patch)
>>
>> and comparing with the changesets from 11u.
> 
> I'm a hardcore webrev/frames guys, so I wouldn't have thought to closely
> look for this.
> 
> I believe I've discovered a "bug" in webrev when specifying specific
> revisions.  I use Mercurial Queues to handle my patches.  With these
> revisions:
> 
>     r1 = current tip
>     r2 = ALPN patch applied
>     r3 = PSS patch applied
> 
> If I have applied ALPN + PSS and I use:
> 
>     % webrev -r r2 -o webrev
> 
> to generate the PSS-only changes, I don't get the git headers.  If I
> don't specify -r, it defaults to r1 (current tip):
> 
>     % webrev -o webrev
> 
> Then I do see the git changes, but unfortunately both ALPN+PSS show up
> in a unified webrev.
> 

Yes, I've noticed this too when doing merges, as -r has to be used to
avoid webrev bringing in everything along one branch of the merge.

I'm not sure why though, as, in the script, the only difference when -r
is specified seems to be the revision used.

It does have its own logic for creating diffs so I can only imagine it
doesn't correctly pick up the rename when -r is used.

My best guess would be it doesn't pick up on renames correctly unless it
can see them in hg status.

>>> ---begin---
>>> 8230978: Add support for RSASSA-PSS Signature algorithm (Java SE 8)
>>> ...deleted...
>>> 8238502: sunmscapi.dll causing EXCEPTION_ACCESS_VIOLATION
>>> Summary: See comment in JDK-8230978 for details in Oracle JDK 8u and
>>> OpenJDK 8u
>>> Reviewed-by: valeriep, weijun, coffeys, pkoppula
>>> ---end---
>>>
>>
>> I'd still prefer it was something like:
>>
>> Summary: Contains elements of JDK-8051408 (see comments on JDK-8230978)
>>
>> the reason for this being that this changeset will then show up if
>> someone searches the repository for 8051408, but won't trigger the
>> database to create a backport issue for it.
> 
> Changed.
> 
>>> Did you want me to add you as a reviewer?
>>>
>>
>> Please.
> 
> Done.
> 
> Brad
> 

Thanks for the changes. Commits look good.
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: RFR[8u252] - MR3 - ALPN & RSASSA-PSS in Java SE 8

[Andrew John Hughes]

On 08/02/2020 00:10, Bradford Wetmore wrote:
> On 2/5/2020 9:40 PM, Andrew John Hughes wrote:> First of all, thanks
> again for posting these patches and also for the
>> comprehensive list of issues for both of them. They pretty much matched
>> up with what I saw when reviewing the patches
> 
> Great.
> 
>> The ALPN one looks pretty clean. My only concern there is the use of
>> "@since 8". Should this not be a reference to the maintenance release,
>> as these methods were not part of 8 at GA? Same applies to the usage in
>> the second patch too.
> 
> I originally had this as "@since 8 MR 3" but during internal review it
> was requested to use @since 8 instead.  This is what was presented/voted
> during the MR phase.
> 

Ah ok. I personally think the original MR 3 version would have been
better, but I guess we're stuck with this then.

>> With the RSA-PSS patch, it's hard to tell what's going on with the
>> MSCAPI code, because they appear as new files in the patch. Why was it
>> necessary to bring in JDK-8213009? I don't see an obvious reason for
>> this.
> 
> JDK-8213009 was mainly a refactor of the MSCAPI code to enhance the code
> layout, and to a lesser degree to better support Windows-native PSS
> calls using CNG (MS Windows Crypto Next Generation).  The SunMSCAPI
> provider is a mix of mostly CAPI (Windows Crypto API) and a few CNG
> calls when CAPI couldn't support everything needed.
> 
> The SunMSCAPI changes that followed would take significant effort to
> extricate these reorganization changes when backporting later changes
> (e.g. jdk/jdk).  We could have forward ported/merged the 8u41 code into
> OpenJDK 8, but unfortunately I haven't been given a lot of cycles for
> OpenJDK code.
> 

Ok. Backporting 8213009 to make future backports easier is the route I
would prefer anyway (and we've done similar in other cases). It's just
hard to tell the motivation with everything in one patch.

>> dependency of 8213010, but again, it's not obvious why that's required
>> here either. Some more details on why some of the less obvious bugs are
>> required would be helpful here.
> 
> Ok, I've added details to a comment in JDK-8230978.
> 

Thanks for this detailed list. It's very helpful.

>> If the refactoring is necessary, a
>> Git-style patch which shows the moves as renames (hg diff -g) would
>> help, so we can see what is actually being changed in these files.
> 
> I'm a bit confused, isn't the webrev already showing this:
> 
> http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS/webrev.01/
> ---begin---
> 
> ...deleted...
> 
> *    src/windows/classes/sun/security/mscapi/CKey.java* /(was
> src/windows/classes/sun/security/mscapi/Key.java)/
> 
> ///138 lines changed: 54 ins; 64 del; 20 mod; 81 unchg/
> 
> ...deleted...
> 
> *    src/windows/classes/sun/security/mscapi/CSignature.java /(was
> src/windows/classes/sun/security/mscapi/RSASignature.java)/
> 
> ///676 lines changed: 507 ins; 77 del; 92 mod; 355 unchg/
> ---end---
> 
> Looking at the index.html and subsequent "Frame" for this file, I can
> see both the rename and the old/new code side-by-side.

I wasn't looking at the web pages, but just at the patch file
(https://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS/webrev.01/jdk.patch)
and comparing with the changesets from 11u.

In the patch file, CKey.java (for example) appears as:

--- /dev/null   2020-01-30 10:24:41.74600 -0800
+++ new/src/windows/classes/sun/security/mscapi/CKey.java
2020-02-04 15:16:49.338689705 -0800
@@ -0,0 +1,155 @@

and same in
https://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS/webrev.01/src/windows/classes/sun/security/mscapi/CKey.java.patch

so it wasn't clear what actual changes were applied after the file was
moved.

It seems like webrev should be running hg diff with the --git option to
get a more useful diff in these cases.

In a git format patch, the same diff would appear as:

rename from src/windows/classes/sun/security/mscapi/Key.java
rename to src/windows/classes/sun/security/mscapi/CKey.java

and then the diff would be as if the move hadn't happened, just showing
the differences between the original Key.java and the final CKey.java.

> 
>> As you say, the RSA-PSS patch brings in truncated MessageDigests which
>> are part of "8051408: NIST SP 800-90A SecureRandom implementations". It
>> would be good if the summary field could be used to say "Contains
>> truncated MessageDigests from JDK-8051408", so that it then shows up if
>> someone is considering 8051408 at a later date
> 
> There are several comments to make which won't fit in the summary field
> which is supposed to only be one short line 

Re: RFR[8u252] - MR3 - ALPN & RSASSA-PSS in Java SE 8

[Andrew John Hughes]

On 04/02/2020 23:24, Bradford Wetmore wrote:
> I added a simple PSS 32-bit windows crash fix, which was previously
> reviewed in security-dev earlier today [0].
> 
>     8238502: sunmscapi.dll causing EXCEPTION_ACCESS_VIOLATION
> 
> The PSS webrev is now at version .01.
> 
> Otherwise, everything is identical from last week's request below.  The
> ALPN remains at version .00.
> 
> [0]
> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021203.html
> 
> 
> ---begin---
> 
> Good morning/afternoon/evening/night,
> 
> As announced on jdk8u-dev[1], there is a Maintenance Release in progress
> for Java SE 8 (i.e. JSR 337) [2] to include two security features
> important for TLS 1.3:
> 
> 1.  Application-Layer Protocol Negotiation (ALPN) [3][4]
> 2.  RSA Signature Scheme with Appendix: Probabilistic Signature Scheme
> (RSASSA-PSS) [5][6]
> 
> As mentioned in [1], if it wasn't too much work then we would like to
> contribute the changes required by this MR to the next appropriate
> OpenJDK 8 release, most likely 8u252.
> 
> Now that the MR3 balloting successfully concluded last night, I'd like
> to make that happen, and move the code into review.
> 
> The code is essentially what was reviewed for 8u41[7][8] and internally
> for what we expect to be in Oracle's 8u251 JDK, except the code in this
> review is based on the current JDK 8u workspace.  We also included code
> to allow the Windows platform to use PSS natively.
> 
> The specific bugs/backports (requested by the JDK8u maintainers) follow:
> 
> ALPN:
> =
> 8230977: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation
> Extension (Java SE 8)
> 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation
> Extension
> 8170282: Enable ALPN parameters to be supplied during the TLS handshake
> 8145849: ALPN: getHandshakeApplicationProtocol() always return null
> 8158978: ALPN not working when values are set directly on a SSLServerSocket
> 8171443: (spec) An ALPN callback function may also ignore ALPN
> 
> RSASSA-PSS:
> ===
> 8230978: Add support for RSASSA-PSS Signature algorithm (Java SE 8)
> 8175029: StackOverflowError in X509CRL and
> X509Certificate.verify(PublicKey, Provider)
> 8146293: Add support for RSASSA-PSS Signature algorithm
> 8205445: Add RSASSA-PSS Signature support to SunMSCAPI
> 8205720: KeyFactory#getKeySpec and translateKey throws
> NullPointerException with Invalid key
> 8206171: Signature#getParameters for RSASSA-PSS throws ProviderException
> when not initialized
> 8213009: Refactoring existing SunMSCAPI classes
> 8213010: Supporting keys created with certmgr.exe
> 8214096: sun.security.util.SignatureUtil passes null parameter, so JCE
> validation fails
> 8215694: keytool cannot generate RSASSA-PSS certificates
> 8221407: Windows 32bit build error in libsunmscapi/security.cpp
> 8216039: TLS with BC and RSASSA-PSS breaks ECDHServerKeyExchange
> 8223003: SunMSCAPI keys are not cleaned up
> 8223063: Support CNG RSA keys
> 8225745: NoSuchAlgorithmException exception for SHA256withECDSA with
> RSASSA-PSS support
> 8225180: SignedObject with invalid Key not throwing the
> InvalidKeyException in Windows
> 8236470: Deal with ECDSA using ecdsa-with-SHA2 plus hash algorithm as
> AlgorithmId
> Reviewed-by: valeriep, weijun, coffeys, pkoppula
> 
> Here are the reviews:
> 
> 1.  ALPN:
>  http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/ALPN
> 
> 2.  RSASSA-PSS:
>  http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS
> 
> Most of these changes are direct copies of the changesets applied
> in JDK 9+, but adjusted for JDK 8u.
> 
> Also, truncated MessageDigests (i.e. SHA-512/224, SHA-512/256) were
> added to the SUN Provider to support the corresponding truncated
> RSASSA-PSS Signatures.
> 
> Thanks,
> 
> Brad
> 
> [1]
> https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-November/010573.html
> [2] https://www.jcp.org/en/jsr/detail?id=337
> [3] https://bugs.openjdk.java.net/browse/JDK-8230977
> [4] https://bugs.openjdk.java.net/browse/JDK-8233417
> [5] https://bugs.openjdk.java.net/browse/JDK-8230978
> [6] https://bugs.openjdk.java.net/browse/JDK-8233418
> [7]
> https://mail.openjdk.java.net/pipermail/security-dev/2019-November/020900.html
> 
> [8] http://hg.openjdk.java.net/jdk8u/jdk8u41/
> 
> 

Hi Brad,

First of all, thanks again for posting these patches and also for the
comprehensive list of issues for both of them. They pretty much matched
up with what I saw when reviewing the patches.

The ALPN one looks pretty clean. My only concern there is the use of
"@since 8". Should this not be a reference to the maintenance release,
as these methods were not part of 8 at GA? Same applies to the usage in
the second patch too.

With the RSA-PSS patch, it's hard to tell what's going on with the
MSCAPI code, because they appear as new files in the patch. Why was it
necessary to bring in JDK-8213009? I don't see an obvious reason for
this. I know from the fix request on that bug for 11u 

Re: RFR[8u252] - MR3 - ALPN & RSASSA-PSS in Java SE 8

[Andrew John Hughes]

On 04/02/2020 23:24, Bradford Wetmore wrote:
> I added a simple PSS 32-bit windows crash fix, which was previously
> reviewed in security-dev earlier today [0].
> 
>     8238502: sunmscapi.dll causing EXCEPTION_ACCESS_VIOLATION
> 
> The PSS webrev is now at version .01.
> 
> Otherwise, everything is identical from last week's request below.  The
> ALPN remains at version .00.
> 
> [0]
> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021203.html
> 
> 

Thanks. Sorry for not getting to this just yet (work on 7u taking longer
than expected), but I would still like to take a look before it goes in.

I'll try and do that today.

Thanks for your patience,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: RFR[8u252] - MR3 - ALPN & RSASSA-PSS in Java SE 8

[Andrew John Hughes]

On 28/01/2020 21:00, Bradford Wetmore wrote:
> Good morning/afternoon/evening/night,
> 
> As announced on jdk8u-dev[1], there is a Maintenance Release in progress
> for Java SE 8 (i.e. JSR 337) [2] to include two security features
> important for TLS 1.3:
> 
> 1.  Application-Layer Protocol Negotiation (ALPN) [3][4]
> 2.  RSA Signature Scheme with Appendix: Probabilistic Signature Scheme
> (RSASSA-PSS) [5][6]
> 
> As mentioned in [1], if it wasn't too much work then we would like to
> contribute the changes required by this MR to the next appropriate
> OpenJDK 8 release, most likely 8u252.
> 
> Now that the MR3 balloting successfully concluded last night, I'd like
> to make that happen, and move the code into review.
> 
> The code is essentially what was reviewed for 8u41[7][8] and internally
> for what we expect to be in Oracle's 8u251 JDK, except the code in this
> review is based on the current JDK 8u workspace.  We also included code
> to allow the Windows platform to use PSS natively.
> 
> The specific bugs/backports (requested by the JDK8u maintainers) follow:
> 
> ALPN:
> =
> 8230977: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation
> Extension (Java SE 8)
> 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation
> Extension
> 8170282: Enable ALPN parameters to be supplied during the TLS handshake
> 8145849: ALPN: getHandshakeApplicationProtocol() always return null
> 8158978: ALPN not working when values are set directly on a SSLServerSocket
> 8171443: (spec) An ALPN callback function may also ignore ALPN
> 
> RSASSA-PSS:
> ===
> 8230978: Add support for RSASSA-PSS Signature algorithm (Java SE 8)
> 8175029: StackOverflowError in X509CRL and
> X509Certificate.verify(PublicKey, Provider)
> 8146293: Add support for RSASSA-PSS Signature algorithm
> 8205445: Add RSASSA-PSS Signature support to SunMSCAPI
> 8205720: KeyFactory#getKeySpec and translateKey throws
> NullPointerException with Invalid key
> 8206171: Signature#getParameters for RSASSA-PSS throws ProviderException
> when not initialized
> 8213009: Refactoring existing SunMSCAPI classes
> 8213010: Supporting keys created with certmgr.exe
> 8214096: sun.security.util.SignatureUtil passes null parameter, so JCE
> validation fails
> 8215694: keytool cannot generate RSASSA-PSS certificates
> 8221407: Windows 32bit build error in libsunmscapi/security.cpp
> 8216039: TLS with BC and RSASSA-PSS breaks ECDHServerKeyExchange
> 8223003: SunMSCAPI keys are not cleaned up
> 8223063: Support CNG RSA keys
> 8225745: NoSuchAlgorithmException exception for SHA256withECDSA with
> RSASSA-PSS support
> 8225180: SignedObject with invalid Key not throwing the
> InvalidKeyException in Windows
> 8236470: Deal with ECDSA using ecdsa-with-SHA2 plus hash algorithm as
> AlgorithmId
> Reviewed-by: valeriep, weijun, coffeys, pkoppula
> 
> Here are the reviews:
> 
> 1.  ALPN:
>  http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/ALPN
> 
> 2.  RSASSA-PSS:
>  http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS
> 
> Most of these changes are direct copies of the changesets applied
> in JDK 9+, but adjusted for JDK 8u.
> 
> Also, truncated MessageDigests (i.e. SHA-512/224, SHA-512/256) were
> added to the SUN Provider to support the corresponding truncated
> RSASSA-PSS Signatures.
> 
> Thanks,
> 
> Brad
> 
> [1]
> https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-November/010573.html
> [2] https://www.jcp.org/en/jsr/detail?id=337
> [3] https://bugs.openjdk.java.net/browse/JDK-8230977
> [4] https://bugs.openjdk.java.net/browse/JDK-8233417
> [5] https://bugs.openjdk.java.net/browse/JDK-8230978
> [6] https://bugs.openjdk.java.net/browse/JDK-8233418
> [7]
> https://mail.openjdk.java.net/pipermail/security-dev/2019-November/020900.html
> 
> [8] http://hg.openjdk.java.net/jdk8u/jdk8u41/
> 
> 

Thanks for posting these. I'll look over them in detail tomorrow and
compare with the 9+ changesets listed. It would have been preferable to
handle each backport individually, but I appreciate that it's more
expedient to handle them in bulk when porting from 8u41.

Thanks again,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: 8232019: Add LuxTrust certificate updates to the existing root program

[Andrew John Hughes]

On 19/12/2019 20:13, Severin Gehwolf wrote:

snip...

>>>
>>
>> Going on this & the similar Amazon fix, I'd say we should backport
>> JDK-8193255 & JDK-8225392 first. The previous updates which alter a
>> binary file have been pretty much unreviewable and, if there's a better
>> solution to that, I'd rather we had it sooner rather than later.
> 
> I agree with you that we should backport JDK-8193255. Question is when
> would be a good time to do this. Given that there would be some benefit
> for these to go into 8u242 if possible I'm not sure we should do JDK-
> 8193255 right now. After all, we've accepted this situation of having
> the binary blob for all of 8u222 and 8u232. Note that JDK-8189131 -
> which brought this in - was integrated in 8u222.
> 
> The risk of a backport of JDK-8193255 seems higher (the build system in
> 8u is different, there is a bug tail) than accepting these backports
> with the binary blob updates. Note that the backports as-is have been
> reviewed by Christoph Langer, Volker Simonis and internally by Martin
> Balao.
> 
> So, it looks like there are the following options:
>1. Accept backports of JDK-8232019 and JDK-8233223 into 8u242 as is.
>2. Backport JDK-8193255, JDK-8225392, JDK-8234245, JDK-8232019 and JDK-
>   8233223 to 8u242.
>3. Defer backports of 2) to 8u252 with the caveat that we won't have
>   certain certificates as compared to Oracle JDK.
> 
> I'm leaning towards option 1) or 3). Slight preference for 1)
> 
>> Likewise, judging by the comment on JDK-8234245, I'd say that needs to
>> be applied between the LuxTrust & Amazon ones:
>>
>> "This fixes an issue after JDK-8232019, so it needs to be included.
>> Patch applies cleanly."
> 
> Not sure what caused JDK-8234245. It might be that it was caused by the
> list of certs in the keystore not being ordered at first (fixed by JDK-
> 8225392?).
> 
> Thanks,
> Severin
> 

There's an option #4:

4. Propose these backports for 8u242 and do the correct backports, with
JDK-8193255 and friends, in 8u252.

8193255 is sensitive to the status of cacerts at the time it is applied.
It needs to be backported with cacerts containing the same certificates
as it did when applied in later versions, so that the textual
replacements match. By applying these backports in 8u252, we'd
complicate the 8193255 backport further by having to effectively include
backports of the Amazon & LuxTrust updates in it.

So what I'd suggest is:

1. Do the full backport series in 8u252 from 8193255 on.
2. Create a separate bug for 8u242 to add the new certificates and apply
for jdk8u-critical-request for that.

When the two are merged together, the webrev changes in 8u242 should be
the same as those in 8u252, and the changed cacerts binary can just be
deleted.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: 8232019: Add LuxTrust certificate updates to the existing root program

[Andrew John Hughes]

On 17/12/2019 19:30, Severin Gehwolf wrote:
> Hi,
> 
> Could I please get a review of this OpenJDK 8u backport of 8232019. The
> JDK 11 patch did not apply cleanly for a couple of reasons:
> 
>1. 8u still has the binary blob for cacerts (JDK-8193255 not
>   backported, yet). Instead, I've updated to the revision in jdk11u,
>   performed a build and copied the cacerts binary to 8u.
>2. JDK-8225392 not present in 8u, which added the checksum to
>   VerifyCACerts.java. Thus, the 8u backport does not include this
>   hunk. @bug annotation modified manually for the same reason.
> 
> Everything else is the same.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8232019
> webrev: 
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8232019/jdk8/01/webrev/
> 
> Testing: sun/security/lib/cacerts/VerifyCACerts.java and
>  security/infra/java/security/cert/CertPathValidator/certification
>  Pass, except for ActalisCA.java which is problem-listed and still
>  broken in HEAD (JDK-8224768)
> 
> Thoughts?
> 
> If reviewed, I'll try to get this in 8u242 via the critical fix request
> label workflow.
> 
> Thanks,
> Severin
> 

Going on this & the similar Amazon fix, I'd say we should backport
JDK-8193255 & JDK-8225392 first. The previous updates which alter a
binary file have been pretty much unreviewable and, if there's a better
solution to that, I'd rather we had it sooner rather than later.

Likewise, judging by the comment on JDK-8234245, I'd say that needs to
be applied between the LuxTrust & Amazon ones:

"This fixes an issue after JDK-8232019, so it needs to be included.
Patch applies cleanly."

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: RFR[8u41]: MR 3 - ALPN & RSASSA-PSS in Java SE 8

[Andrew John Hughes]

On 14/11/2019 02:05, Bradford Wetmore wrote:
> Xuelei/Valerie (+ any other codereviewers),
> 
> As announced on jdk8u-dev[1], there is a Maintenance Release in progress
> for Java SE 8 (i.e. JSR 337) [2] to include two security features
> important for TLS 1.3:
> 
> 1.  Application-Layer Protocol Negotiation (ALPN) [3][4]
> 2.  RSA Signature Scheme with Appendix: Probabilistic Signature Scheme
> (RSASSA-PSS) [5][6]
> 
> The Enhancement and CSR IDs are footnoted above/below.
> 
> To ensure compatibility across the active Java releases, we are
> backporting the APIs introduced in Java SE 9 and 11 respectively to Java
> SE 8.
> 
> This email is a Request For Review (RFR) of the two major pieces for
> this MR:
> 
> 1.  ALPN:
>     http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u41/open/ALPN
> 
> 2.  RSASSA-PSS:
>     http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u41/open/PSS
> 
> This includes the updates to the Specification and Reference
> Implementation (RI), which will be called JDK 8u41 [7].
> 
> Almost all of these changes are direct copies of the changesets applied
> in JDK 9+.
> 
> In addition to these features:
> 
> 1.  The file ADDITIONAL_LICENSE_INFO was added, which is identical to
> the same file in later releases.
> 
> 2.  Truncated MessageDigests (i.e. SHA-512/224, SHA-512/256) were added
> to the SUN Provider to support the corresponding truncated RSASSA-PSS
> Signatures.
> 
> Thanks,
> 
> Brad
> 
> [1]
> https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-November/010573.html
> [2] https://www.jcp.org/en/jsr/detail?id=337
> [3] https://bugs.openjdk.java.net/browse/JDK-8230977
> [4] https://bugs.openjdk.java.net/browse/JDK-8233417
> [5] https://bugs.openjdk.java.net/browse/JDK-8230978
> [6] https://bugs.openjdk.java.net/browse/JDK-8233418
> [7] http://hg.openjdk.java.net/jdk8u/jdk8u41/
> 

It's not clear which bug IDs these two webrevs apply to.

Note that changes for OpenJDK 8u require approval using the
jdk8u-fix-request label, as described at
https://wiki.openjdk.java.net/display/jdk8u/Main.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: 8226607: Inconsistent info between pcsclite.md and MUSCLE headers

[Andrew John Hughes]

On 02/09/2019 16:05, Severin Gehwolf wrote:
> On Mon, 2019-09-02 at 15:38 +0100, Andrew John Hughes wrote:
>>
>> On 26/08/2019 14:24, Severin Gehwolf wrote:
>>> Hi,
>>>
>>> Could I get a review of this follow-up fix for an 8u backport (JDK-
>>> 8218780)? This follow-up re-adds a COPYING file to the MUSCLE pcsc
>>> library header files removed by the JDK-8218780 backport. The patch
>>> differs from the version in JDK 11 since there is no pcsclite.md file
>>> in OpenJDK 8u.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8226607
>>> webrev: 
>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8226607/jdk8/01/webrev/
>>> JDK 11u changeset: 
>>> https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/9e304e99cbb2
>>>
>>> I intend to push this fix together with JDK-8218780 once it passed
>>> review and got approved.
>>>
>>> Thoughts?
>>>
>>> Thanks,
>>> Severin
>>>
>>>
>>>
>>
>> The *.md files in OpenJDK 9+ are the modular equivalent of the
>> THIRD_PARTY_README file found in each OpenJDK 8u repository. See my
>> review of JDK-8217676 [0] for Zhengyu for more details.
>>
>> For reference, the conversion took place in JDK-8169925.
> 
> Thanks for this. Updated webrev:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8226607/jdk8/02/webrev/
> 
> I intend to push the same updat to 
> THIRD_PARTY_README files on all other repos. Example here is jdk repo.
> Do you want to see webrevs of this THIRD_PARTY_README update for all 7
> other repos?
> 
> Thanks,
> Severin
> 
>> [0]
>> https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-August/010116.html
>> [1] https://bugs.openjdk.java.net/browse/JDK-8169925
> 

I'm happy assuming the same THIRD_PARTY_README change for all repos.

This looks fine to me. Can you flag it jdk8u-critical-request so we can
get this into 8u232 with JDK-8218780?

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: 8226607: Inconsistent info between pcsclite.md and MUSCLE headers

[Andrew John Hughes]

On 26/08/2019 14:24, Severin Gehwolf wrote:
> Hi,
> 
> Could I get a review of this follow-up fix for an 8u backport (JDK-
> 8218780)? This follow-up re-adds a COPYING file to the MUSCLE pcsc
> library header files removed by the JDK-8218780 backport. The patch
> differs from the version in JDK 11 since there is no pcsclite.md file
> in OpenJDK 8u.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8226607
> webrev: 
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8226607/jdk8/01/webrev/
> JDK 11u changeset: 
> https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/9e304e99cbb2
> 
> I intend to push this fix together with JDK-8218780 once it passed
> review and got approved.
> 
> Thoughts?
> 
> Thanks,
> Severin
> 
> 
> 

The *.md files in OpenJDK 9+ are the modular equivalent of the
THIRD_PARTY_README file found in each OpenJDK 8u repository. See my
review of JDK-8217676 [0] for Zhengyu for more details.

For reference, the conversion took place in JDK-8169925.

[0]
https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-August/010116.html
[1] https://bugs.openjdk.java.net/browse/JDK-8169925
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: 8218780: Update MUSCLE PCSC-Lite header files

[Andrew John Hughes]

On 02/09/2019 13:14, Severin Gehwolf wrote:

snip...

>>>
>>
>> Most of this looks good. I was a little confused at first because the
>> patch in your webrev looks quite different to the 11u changeset.
>> However, once applied locally to the 8u repo, the diff between the two
>> was as suggested and the PCSC library files (those in MUSCLE) were
>> identical. I don't know what webrev did in creating that patch.
>>
>> The bit I don't understand is why you've decided to drop the copyright
>> header change on the floor. Just because the original in 8u has 2014,
>> while the original in 11u had 2015, does not make the change inapplicable.
> 
> OK. I see. I wasn't sure how to deal with copyright year updates. I've
> added the copyright update back. The patch is now identical to JDK 11u
> (modulo differing copyright year base: 2014 - jdk 8 - vs. 2018 - jdk
> 11):
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218780/jdk8/02/webrev
> 

This looks good now (though still issues with the original diff vs local
application of it)

Please flag the bug for approval.

> 
>> A better alternative may actually be to backport JDK-8207233 [0] first
>> which is a nice little cleanup patch. I suspect this patch would then
>> apply cleanly as these PCSC files are rarely touched.
>>
>> [0] https://bugs.openjdk.java.net/browse/JDK-8207233
> 
> Hmm, this seems overkill just to get the copyright hunk to apply. I'd
> prefer to keep this dependency out of scope for this patch. While a
> nice clean-up it's not without risk backporting that too. Thoughts?
> 

Well, I think it's a useful cleanup too, but I'm not going to force it
as a dependency of this. I just thought it might make the review for
this fix redundant as it would then be a clean backport. I might look
into it myself at a later date.

> Thanks,
> Severin
> 

-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: 8218780: Update MUSCLE PCSC-Lite header files

[Andrew John Hughes]

On 26/08/2019 14:23, Severin Gehwolf wrote:
> Hi,
> 
> Could I please get a review of this MUSCLE header files update in
> OpenJDK 8u? I'd like to backport this bug as it's also going to be in
> Oracle JDK 8u231 (equiv to OpenJDK 8u232) as well. The OpenJDK 11 patch
> applies almost cleanly post path-unshuffelling. Changes which didn't
> apply were a copyright header update in pcsc.c. I've omitted these.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8218780
> webrev: 
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218780/jdk8/01/webrev/
> JDK 11u changeset: 
> https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/0bcc59a50c88
> 
> There is going to be a follow-up fix adding back COPYING via 
> JDK-8226607 which I propose for backport to OpenJDK 8u as well.
> 
> Testing: smartcardio sanity and build on Linux x86_64 (Fedora 30 and RHEL 6)
> 
> Thanks to Aleksey Shipilev who helped test this patch.
> 
> Thoughts?
> 
> Thanks,
> Severin
> 

Most of this looks good. I was a little confused at first because the
patch in your webrev looks quite different to the 11u changeset.
However, once applied locally to the 8u repo, the diff between the two
was as suggested and the PCSC library files (those in MUSCLE) were
identical. I don't know what webrev did in creating that patch.

The bit I don't understand is why you've decided to drop the copyright
header change on the floor. Just because the original in 8u has 2014,
while the original in 11u had 2015, does not make the change inapplicable.

A better alternative may actually be to backport JDK-8207233 [0] first
which is a nice little cleanup patch. I suspect this patch would then
apply cleanly as these PCSC files are rarely touched.

[0] https://bugs.openjdk.java.net/browse/JDK-8207233

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: JDK-8129988 introduces a new behavior when reading the javax.net.ssl.trustStore property.

[Andrew John Hughes]

Forwarding to security-dev as this was backported from later JDK versions:

On 09/08/2019 20:52, Alvarez, David wrote:
> Hello,
> 
> We have detected that JDK-8219988 [1], that has been included in OpenJDK 8u222
> included a non-documented change in the behavior of the
> javax.net.ssl.trustStore property. In previous versions, should this property
> point to a non-existent file, an empty KeyStore would be used instead. [2]
> 
> In newer versions, if the value of the property contains an invalid URL, the
> code will instead fall back and use the lib/security/cacerts file. [3]
> 
> However, there are two things about that change that caught our attention:
> - Whenever there is no javax.net.ssl.trustStore property set, the code will
> first look for lib/security/jssecacerts. If that file does not exist, then it
> will look for lib/security/cacerts. However, when the property is set to an
> invalid file, the fallback mechanism jumps directly to lib/security/cacerts,
> never checking lib/security/jssecacerts.
> 
> - This fallback mechanism for invalid javax.net.ssl.trustStore values reuses 
> the
> value of javax.net.ssl.trustStorePassword. If that property is set in
> conjunction with an invalid value in javax.net.ssl.trustStore the specified
> password will be used when attempting to read the lib/security/cacerts store.
> It seems unclear why this path of action is chosen here.
> 
> If the lib/security/cacerts has no password (and as far as I'm aware, that is
> the case in the majority of OpenJDK distributions, if not all), the operation
> will raise an exception. This exception mentions that 'Password verification
> failed', hiding the underlying cause (the property pointing to a bad 
> store).[4]
> 
> Although the new behavior isn't necessarily wrong, I think there is room for
> Improvement. Suggestions:
> 
> - Make sure lib/security/jssecacerts is checked during the fallback process.
> - Ignore the value of javax.net.ssl.trustStorePassword when we fallback to use
> the bundled jssecacerts or cacerts file.
> - Change the exception message to avoid confusion.
> 
> I would like to have your opinion on this.
> 
> Thanks,
> 
> David
>  
> --
> [1] https://bugs.openjdk.java.net/browse/JDK-8129988
> [2] 
> http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/ac2ef877d3e8/src/share/classes/sun/security/ssl/TrustManagerFactoryImpl.java#l132
> [3] 
> http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/2a9bea6e5e03/src/share/classes/sun/security/ssl/TrustStoreManager.java#l128
> [4] Here is a copy of the exception:
> Caused by: java.io.IOException: Keystore was tampered with, or password was 
> incorrect
> at sun.security.provider.JavaKeyStore.engineLoad(JavaKeyStore.java:785)
> at sun.security.provider.JavaKeyStore$JKS.engineLoad(JavaKeyStore.java:56)
> at 
> sun.security.provider.KeyStoreDelegator.engineLoad(KeyStoreDelegator.java:224)
> at 
> sun.security.provider.JavaKeyStore$DualFormatJKS.engineLoad(JavaKeyStore.java:70)
> at java.security.KeyStore.load(KeyStore.java:1445)
> at 
> sun.security.ssl.TrustStoreManager$TrustAnchorManager.loadKeyStore(TrustStoreManager.java:367)
> at 
> sun.security.ssl.TrustStoreManager$TrustAnchorManager.getTrustedCerts(TrustStoreManager.java:315)
> at 
> sun.security.ssl.TrustStoreManager.getTrustedCerts(TrustStoreManager.java:59)
> at 
> sun.security.ssl.TrustManagerFactoryImpl.engineInit(TrustManagerFactoryImpl.java:51)
> Caused by: java.security.UnrecoverableKeyException: Password verification 
> failed
> at sun.security.provider.JavaKeyStore.engineLoad(JavaKeyStore.java:783)
> 
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR Backport: 8208698: Improved ECC Implementation

[Andrew John Hughes]

On 28/06/2019 07:14, Alvarez, David wrote:
> Looks good to me
> 

Thanks David. Pushed:

https://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/9b5707865a97

-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR Backport: 8208698: Improved ECC Implementation

[Andrew John Hughes]

On 14/06/2019 23:33, Alvarez, David wrote:
> Hi,
> 
> Please review this backport of JDK-8208698: Improved ECC Implementation
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8208698
> Original: http://hg.openjdk.java.net/jdk/jdk/rev/752e57845ad2
> Webrev: http://cr.openjdk.java.net/~phh/8208698/webrev.8u.00/
> 
> JDK-8208698 is marked as jdk8u-critical-yes
> 
> This is the last of the three patches I was sending today, JDK-8181594, 
> JDK-8208648 and JDK-8208698
> 
> This backport is effectively also fixing JDK-8205476. However, JDK-8205476 
> includes some Javadoc changes and a test this patch is not bringing. If 
> needed, I could backport JDK-8205476 independently and do the webrev, or 
> modify the existing backport to ensure we do not wipe the secret.
> 
> There are other reasons why this patch did not apply cleanly
> 
> ECDHKeyAgreement.java: These are mostly caused by the missing JDK-8205476
> ECDSASignature.java: jdk supports SHA224inP1363Format which we don't. I 
> adapted the patch to ignore P1363 references (P1363 format here means 
> unencoded values)
> ECKeyPairGeneration.java: jdk8u is missing JDK-8182999, so the patching of 
> the initialize method had to be written manually.
> 
> Additionally, there were some other compilation changes, mostly to 
> accommodate for newer API methods not present in jdk8u like Optional.isEmpty 
> or Map.of
> 
> The changes I did to fix rejects are listed below
> 
> Thanks,
> David
> 
> 
> 
> diff --git a/src/jdk/src/share/classes/sun/security/ec/ECDHKeyAgreement.java 
> b/src/jdk/src/share/classes/sun/security/ec/ECDHKeyAgreement.java
> index e17f8393..5425179f 100644
> --- a/src/jdk/src/share/classes/sun/security/ec/ECDHKeyAgreement.java
> +++ b/src/jdk/src/share/classes/sun/security/ec/ECDHKeyAgreement.java
> @@ -104,40 +104,74 @@ public final class ECDHKeyAgreement extends 
> KeyAgreementSpi {
>  ("Key must be a PublicKey with algorithm EC");
>  }
> -ECPublicKey ecKey = (ECPublicKey)key;
> -ECParameterSpec params = ecKey.getParams();
> +this.publicKey = (ECPublicKey) key;
> -if (ecKey instanceof ECPublicKeyImpl) {
> -publicValue = ((ECPublicKeyImpl)ecKey).getEncodedPublicValue();
> -} else { // instanceof ECPublicKey
> -publicValue =
> -ECUtil.encodePoint(ecKey.getW(), params.getCurve());
> -}
> +ECParameterSpec params = publicKey.getParams();
>  int keyLenBits = params.getCurve().getField().getFieldSize();
>  secretLen = (keyLenBits + 7) >> 3;
>  return null;
>  }
> -// see JCE spec
> -@Override
> -protected byte[] engineGenerateSecret() throws IllegalStateException {
> -if ((privateKey == null) || (publicValue == null)) {
> -throw new IllegalStateException("Not initialized correctly");
> +private static void validateCoordinate(BigInteger c, BigInteger mod) {
> +if (c.compareTo(BigInteger.ZERO) < 0) {
> +throw new ProviderException("invalid coordinate");
>  }
> -byte[] s = privateKey.getS().toByteArray();
> -byte[] encodedParams =   // DER OID
> -ECUtil.encodeECParameterSpec(null, privateKey.getParams());
> +if (c.compareTo(mod) >= 0) {
> +throw new ProviderException("invalid coordinate");
> +}
> +}
> -try {
> +/*
> + * Check whether a public key is valid. Throw ProviderException
> + * if it is not valid or could not be validated.
> + */
> +private static void validate(ECOperations ops, ECPublicKey key) {
> +
> +// ensure that integers are in proper range
> +BigInteger x = key.getW().getAffineX();
> +BigInteger y = key.getW().getAffineY();
> +
> +BigInteger p = ops.getField().getSize();
> +validateCoordinate(x, p);
> +validateCoordinate(y, p);
> +
> +// ensure the point is on the curve
> +EllipticCurve curve = key.getParams().getCurve();
> +BigInteger rhs = x.modPow(BigInteger.valueOf(3), p).add(curve.getA()
> +.multiply(x)).add(curve.getB()).mod(p);
> +BigInteger lhs = y.modPow(BigInteger.valueOf(2), p).mod(p);
> +if (!rhs.equals(lhs)) {
> +throw new ProviderException("point is not on curve");
> +}
> -return deriveKey(s, publicValue, encodedParams);
> +// check the order of the point
> +ImmutableIntegerModuloP xElem = ops.getField().getElement(x);
> +ImmutableIntegerModuloP yElem = ops.getField().getElement(y);
> +AffinePoint affP = new AffinePoint(xElem, yElem);
> +byte[] order = key.getParams().getOrder().toByteArray();
> +ArrayUtil.reverse(order);
> +Point product = ops.multiply(affP, order);
> +if (!ops.isNeutral(product)) {
> +throw new ProviderException("point has incorrect order");
> +}
> -} catch 

Re: [11u] RFR (S): 8226880: Backport of JDK-8208698 (Improved ECC Implementation) should not bring parts of JDK-8205476 (KeyAgreement#generateSecret is not reset for ECDH based algorithm)

[Andrew John Hughes]

On 27/06/2019 11:12, Langer, Christoph wrote:
> Hi,
> 
>  
> 
> I made a mistake when bringing JDK-8226880 to 11u. The patch introduced
> coding of JDK-8205476 that should not be there. Here is a patch to fix this.
> 
>  
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8226880
> 
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8226880.11u.0/
> 
>  
> 
> As the backport is in 11.0.4, this fix needs to be applied there during
> closed rampdown.
> 
>  
> 
> Thanks
> 
> Christoph
> 
>  
> 

Looks good to me.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: Backport 8208648: ECC Field Arithmetic Enhancements

[Andrew John Hughes]

On 14/06/2019 22:37, Alvarez, David wrote:
> Hi,
> 
> Here is the proper RFR for 8208648: ECC Field Arithmetic Enhancements
> 
> Sorry for the confusion
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8208648
> Original: http://hg.openjdk.java.net/jdk/jdk/rev/746602d9682f
> Webrev: http://cr.openjdk.java.net/~phh/8208648/webrev.8u.00/
> 
> JDK-8208648 is marked as jdk8u-critical-yes
> 
> This is the second of a chain of three patches, JDK-8181594, JDK-8208648 and 
> JDK-8208698 I will be sending today.
> 
> The patch did not apply cleanly. The following conflicts appeared:
> 
> sun/security/util/ArrayUtil.java is not present in jdk8u. ArrayUtil is a 
> utility class with static methods. I created the file but only with the 
> static methods that were required for this patch (all of them were included 
> in the original patch).
> sun/security/util/math/intpoly/IntegerPolynomial1305.java had a minor 
> conflict due to mismatching of the context lines
> sun/security/util/math/intpoly/IntegerPolynomial.java had significant amount 
> of rejections, but they were mostly easy to fix, caused by context 
> mismatching.
> 
> Additionally, some of the new implementations of IntegerPolynomial contained 
> an @Override for a method (finalCarryReduceLast) that is not present in the 
> jdk8u version of IntegerPolynomial.java, so I removed the annotation.
> 
> Below are the relevant changes I've done to resolve the rejects and 
> compilation errors.
> 
> Thanks,
> David
> 
> 

With JDK-8203228 & JDK-8201317 now applied, the intpoly changes now
apply cleanly, so I've pushed this using your version of ArrayUtil.java.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [11u] RFR: 8208698: Improved ECC Implementation

[Andrew John Hughes]

On 28/05/2019 08:21, Langer, Christoph wrote:
> Hi,
> 
> please review this backport of JDK-8208698: Improved ECC Implementation.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8208698
> Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/752e57845ad2
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8208698.11u/
> 
> The patch did not apply cleanly because there were conflicts in 
> src/jdk.crypto.ec/share/classes/sun/security/ec/ECDHKeyAgreement.java due to 
> JDK-8205476 which is part of jdk/jdk but not of jdk11u-dev. Unfortunately, 
> JDK-8205476 can not be downported as prerequisite because it brings a 
> behavioral change and is associated with a CSR. So I resolved the rejects 
> manually. I add the rejects below.
> 
> Thanks
> Christoph
> 
> 

I'm just looking over this in backporting to 8u. You mention not being
able to include the JDK-8205476 change, but then it is included in the
patch.

JDK-8205476 is essentially:

@@ -127,7 +127,9 @@

 try {

-return deriveKey(s, publicValue, encodedParams);
+byte[] result = deriveKey(s, publicValue, encodedParams);
+publicValue = null;
+return result;

 } catch (GeneralSecurityException e) {
 throw new ProviderException("Could not derive key", e);

The same change is made in 11u by adopting the line in the 8208698 patch
verbatim:

-try {
-
-return deriveKey(s, publicValue, encodedParams);
-
-} catch (GeneralSecurityException e) {
-throw new ProviderException("Could not derive key", e);
-}
-
+Optional resultOpt = deriveKeyImpl(privateKey, publicKey);
+byte[] result = resultOpt.orElseGet(
+() -> deriveKeyNative(privateKey, publicKey)
+);
+publicKey = null;
+return result;

I think this should actually be:

return resultOpt.orElseGet(() -> deriveKeyNative(privateKey, publicKey));

if we want to avoid the JDK-8205476 change of nulling publicKey.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: Backport 8181594: Efficient and constant-time modular arithmetic

[Andrew John Hughes]

On 18/06/2019 20:30, Alvarez, David wrote:
> Here is the updated webrev with suggested changes:
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8181594
> Original: http://hg.openjdk.java.net/jdk/jdk/rev/d213d70182a9
> Webrev: http://cr.openjdk.java.net/~phh/8181594/webrev.8u.02/
> 
> --
> David
> 

Thanks. This looks good. I'll push it, along with JDK-8201317 & JDK-8203228.
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: Backport 8208648: ECC Field Arithmetic Enhancements

[Andrew John Hughes]

On 18/06/2019 19:37, Andrew John Hughes wrote:
> On 14/06/2019 22:37, Alvarez, David wrote:
>> Hi,
>>
>> Here is the proper RFR for 8208648: ECC Field Arithmetic Enhancements
>>
>> Sorry for the confusion
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8208648
>> Original: http://hg.openjdk.java.net/jdk/jdk/rev/746602d9682f
>> Webrev: http://cr.openjdk.java.net/~phh/8208648/webrev.8u.00/
>>
>> JDK-8208648 is marked as jdk8u-critical-yes
>>
>> This is the second of a chain of three patches, JDK-8181594, JDK-8208648 and 
>> JDK-8208698 I will be sending today.
>>
>> The patch did not apply cleanly. The following conflicts appeared:
>>
>> sun/security/util/ArrayUtil.java is not present in jdk8u. ArrayUtil is a 
>> utility class with static methods. I created the file but only with the 
>> static methods that were required for this patch (all of them were included 
>> in the original patch).
> 
> That seems fine. The file originally arises from JDK-8179098, which in
> turn fixes a performance regression introduced by JDK-8076112 adding
> bounds checks. That is a JDK & HotSpot change to mark intrinsics, which
> I doubt is suitable for backport, so I'm happy with ArrayUtil being
> created just by this patch.
> 
>> sun/security/util/math/intpoly/IntegerPolynomial1305.java had a minor 
>> conflict due to mismatching of the context lines
>> sun/security/util/math/intpoly/IntegerPolynomial.java had significant amount 
>> of rejections, but they were mostly easy to fix, caused by context 
>> mismatching.
>>
>> Additionally, some of the new implementations of IntegerPolynomial contained 
>> an @Override for a method (finalCarryReduceLast) that is not present in the 
>> jdk8u version of IntegerPolynomial.java, so I removed the annotation.
>>
> 
> I think this is because JDK-8203228 & JDK-8201317 are missing, which are
> also part of the 8u222 list. I'll post backports of those two, based on
> your JDK-8181594 work and then those changes should apply cleanly.
> 
>> Below are the relevant changes I've done to resolve the rejects and 
>> compilation errors.
>>
>> Thanks,
>> David
>>
>>
> 
> 

Looking at JDK-8203228 & JDK8201317, they both apply cleanly (once
changes to XDHKeyAgreement are dropped from the latter), so they can go
straight in after JDK-8181594 is pushed. You should then be able to
backport JDK-8208648 more simply.
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: Backport 8208648: ECC Field Arithmetic Enhancements

[Andrew John Hughes]

On 14/06/2019 22:37, Alvarez, David wrote:
> Hi,
> 
> Here is the proper RFR for 8208648: ECC Field Arithmetic Enhancements
> 
> Sorry for the confusion
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8208648
> Original: http://hg.openjdk.java.net/jdk/jdk/rev/746602d9682f
> Webrev: http://cr.openjdk.java.net/~phh/8208648/webrev.8u.00/
> 
> JDK-8208648 is marked as jdk8u-critical-yes
> 
> This is the second of a chain of three patches, JDK-8181594, JDK-8208648 and 
> JDK-8208698 I will be sending today.
> 
> The patch did not apply cleanly. The following conflicts appeared:
> 
> sun/security/util/ArrayUtil.java is not present in jdk8u. ArrayUtil is a 
> utility class with static methods. I created the file but only with the 
> static methods that were required for this patch (all of them were included 
> in the original patch).

That seems fine. The file originally arises from JDK-8179098, which in
turn fixes a performance regression introduced by JDK-8076112 adding
bounds checks. That is a JDK & HotSpot change to mark intrinsics, which
I doubt is suitable for backport, so I'm happy with ArrayUtil being
created just by this patch.

> sun/security/util/math/intpoly/IntegerPolynomial1305.java had a minor 
> conflict due to mismatching of the context lines
> sun/security/util/math/intpoly/IntegerPolynomial.java had significant amount 
> of rejections, but they were mostly easy to fix, caused by context 
> mismatching.
> 
> Additionally, some of the new implementations of IntegerPolynomial contained 
> an @Override for a method (finalCarryReduceLast) that is not present in the 
> jdk8u version of IntegerPolynomial.java, so I removed the annotation.
> 

I think this is because JDK-8203228 & JDK-8201317 are missing, which are
also part of the 8u222 list. I'll post backports of those two, based on
your JDK-8181594 work and then those changes should apply cleanly.

> Below are the relevant changes I've done to resolve the rejects and 
> compilation errors.
> 
> Thanks,
> David
> 
> 


-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: Backport 8181594: Efficient and constant-time modular arithmetic

[Andrew John Hughes]

On 14/06/2019 22:16, Alvarez, David wrote:
> Correction, this is the RFR for 8181594: Efficient and constant-time modular 
> arithmetic
> 
> On 2019-06-14, 14:13, "Alvarez, David"  wrote:
> 
> Hi,
> 
> Please review this backport of JDK-8181594: Efficient and constant-time 
> modular arithmetic
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8181594
> Original: http://hg.openjdk.java.net/jdk/jdk/rev/d213d70182a9
> Webrev: http://cr.openjdk.java.net/~phh/8181594/webrev.8u.00/
> 
> JDK-8181594 is marked as jdk8u-critical-yes
> 
> This is the first of a chain of three patches, JDK-8181594, JDK-8208648 
> and JDK-8208698 I will be sending today
> 
> The patch consists only of new files, so there were no conflicts. 
> However, 
> jdk/src/share/classes/sun/security/util/math/intpoly/IntegerPolynomial1305.java
>  makes use of VarHandle, so I had to replace that part with a ByteBuffer. 
> I’ve attached the differences between the original patch and my patch below.
> 
> Thanks,
> David
> 
> --- 
> a/src/jdk/src/share/classes/sun/security/util/math/intpoly/IntegerPolynomial1305.java
> +++ 
> b/src/jdk/src/share/classes/sun/security/util/math/intpoly/IntegerPolynomial1305.java
> @@ -26,7 +26,6 @@
> package sun.security.util.math.intpoly;
> 
> import java.lang.invoke.MethodHandles;
> -import java.lang.invoke.VarHandle;
> import java.math.BigInteger;
> import java.nio.*;
> 
> @@ -167,14 +166,13 @@ public class IntegerPolynomial1305 extends 
> IntegerPolynomial {
>  result[4] = (high >>> 40) + (highByte << 24L);
>  }
> 
> -private static final VarHandle AS_LONG_LE = MethodHandles
> -.byteArrayViewVarHandle(long[].class, ByteOrder.LITTLE_ENDIAN);
> -
>  protected void encode(byte[] v, int offset, int length, byte 
> highByte,
>long[] result) {
>  if (length == 16) {
> -long low = (long) AS_LONG_LE.get(v, offset);
> -long high = (long) AS_LONG_LE.get(v, offset + 8);
> +long low = ByteBuffer.wrap(v, offset, 8)
> +.order(ByteOrder.LITTLE_ENDIAN).getLong();
> +long high = ByteBuffer.wrap(v, offset + 8, 8)
> +.order(ByteOrder.LITTLE_ENDIAN).getLong();
>  encode(high, low, highByte, result);
>  } else {
>  super.encode(v, offset, length, highByte, result);
> 
> 
> 
> 
> 

Would it not make more sense to create one ByteBuffer of v.length and
then read two longs from it? That would seem to be a little more
efficient and closer to the original e.g.


asLongLEBuffer = ByteBuffer.wrap(v, offset,
v.length).order(ByteOrder.LITTLE_ENDIAN);
long low = asLongLEBuffer.getLong();
long high = asLongLEBuffer.getLong();
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: 8203190: SessionId.hashCode generates too many collisions

[Andrew John Hughes]

On 17/05/2019 17:00, Severin Gehwolf wrote:
> On Fri, 2019-05-17 at 16:28 +0100, Andrew John Hughes wrote:
>>
>> On 17/05/2019 12:37, Severin Gehwolf wrote:
>>
>> snip...
>>
>>> The reason was that it's not a good test to be run automatically. It
>>> would have to have some heuristic which it uses as "passed" and "fail".
>>> Checking in the code anyway has a tendency for it to bitrot. If you
>>> really feel strongly about it, I can add it. FWIW, the reference to the
>>> test isn't going away so it'll be available either way.
>>>
>>
>> I get that, but there are other manual tests in the repositories. I saw
>> one yesterday that required downloading a font before running it. I
>> think it better to have everything in one place rather than relying on
>> someone to find this e-mail thread.
>>
>> The bitrot argument seems a little odd, given it would be more open to
>> updates in the repositories rather than on a web server where only you
>> can update it :/
> 
> Sure. Here the webrev with the test:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/02/webrev/
> 
> OK to push?
> 
> Thanks,
> Severin
> 

Looks good to me.
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: 8203190: SessionId.hashCode generates too many collisions

[Andrew John Hughes]

On 17/05/2019 12:37, Severin Gehwolf wrote:

snip...

> 
> The reason was that it's not a good test to be run automatically. It
> would have to have some heuristic which it uses as "passed" and "fail".
> Checking in the code anyway has a tendency for it to bitrot. If you
> really feel strongly about it, I can add it. FWIW, the reference to the
> test isn't going away so it'll be available either way.
> 

I get that, but there are other manual tests in the repositories. I saw
one yesterday that required downloading a font before running it. I
think it better to have everything in one place rather than relying on
someone to find this e-mail thread.

The bitrot argument seems a little odd, given it would be more open to
updates in the repositories rather than on a web server where only you
can update it :/
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [8u] RFR: 8203190: SessionId.hashCode generates too many collisions

[Andrew John Hughes]

On 16/05/2019 18:51, Severin Gehwolf wrote:
> Hi,
> 
> Could I please get a review of this OpenJDK 8u only fix? JDKs 11+ don't
> seems to have this issue as with the TLS 1.3 feature (JDK-8196584)
> SessionId.hashCode() got changed to use Arrays.hashCode() already.
> 
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/01/webrev/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8203190
> 
> The rationale for the fix are these assumptions:
> 
> a) elements in trees on hash collision of LinkedHashMap used internally
> by the MemoryCache class become prohibitively large for many SessionId
> entries in the cache, b) moderate speed of the new hashCode() impl will
> not have a detrimental effect on performance overall.
> 
> Comparison of performance of hashCode impls[1]:
> 
> BenchmarkMode  Cnt Score Error  Units
> SessionIDBench.newHashCode  thrpt  100  43649538.284 ±  678702.696  ops/s
> SessionIDBench.oldHashCode  thrpt  100  94068843.923 ± 1379930.266  ops/s
> 
> Collision testing[2] showed that indeed, the current hashCode()
> implementation of SessionId produces more collissions and, thus,
> produce more elements in trees for collision resolution in the
> underlying LinkedHashMap. The default cache expiry is 24 hours per
> entry and this can result in millions of entries in the cache in some
> circumstances[3].
> 
> Before:
> ##
> Collision test for 100 sessions:
> 
> Total number of collisions: 4
> Max length of collision list over all buckets: 2
> 
> Collision test for 20480 sessions:
> 
> Total number of collisions: 18311
> Max length of collision list over all buckets: 30
> 
> Collision test for 1000 sessions:
> 
> Total number of collisions: 9996395
> Max length of collision list over all buckets: 9709
> ##
> 
> After:
> ##
> Collision test for 100 sessions:
> 
> Total number of collisions: 0
> 
> Collision test for 20480 sessions:
> 
> Total number of collisions: 0
> 
> Collision test for 1000 sessions:
> 
> Total number of collisions: 11530
> Max length of collision list over all buckets: 2
> ##
> 
> 
> Testing: Above testing, and make test. No new failures.
> 
> Thoughts?
> 
> Thanks,
> Severin
> 
> [1] 
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/SessionIDBench.java
> [2] 
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/SessionIdCollissionTest.java
> [3] https://bugs.openjdk.java.net/browse/JDK-8210985
> 

Change looks good.

Is there a reason the tests aren't included in the webrev? I think it
would be better to have them checked in, even if they can't be run
automatically.

They will need copyright headers and I'd correct the spelling of
'collision' too :-)
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: [RFR] [8u] 8220641, , New test KdcPolicy.java introduced by JDK-8164656 needs same change as JDK-8190690

[Andrew John Hughes]

On 15/03/2019 17:58, Hohensee, Paul wrote:
> +1
> 
> Paul
> 
> On 3/15/19, 4:00 AM, "jdk8u-dev on behalf of Aleksey Shipilev" 
>  wrote:
> 
>     On 3/15/19 5:55 AM, Andrew John Hughes wrote:
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8220641
> > Webrev: https://cr.openjdk.java.net/~andrew/openjdk8/8220641/webrev.01/
> 
> Looks good, ship it.
> 
> -Aleksey
> 
> 
> 

Thanks, guys. Just waiting on aph to set jdk8u-fix-yes on it now.
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

[RFR] [8u] 8220641, , New test KdcPolicy.java introduced by JDK-8164656 needs same change as JDK-8190690

[Andrew John Hughes]

Bug: https://bugs.openjdk.java.net/browse/JDK-8220641
Webrev: https://cr.openjdk.java.net/~andrew/openjdk8/8220641/webrev.01/

This is the patch we split out from my original post for 8175120. It
applies the same change to the @run command in KdcPolicy.java as was
applied to the tests deleted by 8175120 in JDK-8190690.

Without this fix, the test fails with a name lookup error. With it, it
passes.

[0] https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-March/008846.html
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew



signature.asc
Description: OpenPGP digital signature

Re: Enhance toString() to return structured info, for certificate and probably more

[Dr Andrew John Hughes]

On 10:41 Thu 25 Aug , Weijun Wang wrote:
 Hi All
 
 I was talking with Xuelei on how to better display certificate info. 
 There are 3 cases we can currently think of:
 
 1. debug output
 2. keytool/jarsigner output
 3. Java plugin display
 
 The 1st one is the most primitive one and can be a single string, the 
 2nd is also a string but have some format and needs to localized, the 
 third is a series of Swing controls.
 
 But the contents of all 3 are the same, and hopefully can be provided in 
 a single method. Each consumer can just display it in its own style, 
 with no need to understand Certificate fields, OIDs, etc. It's just like 
 XSLT transformation of XML files.
 
 So what shall I do? Let toString() outputs a long string in XML or JSON?
 

I be against that.  It goes against the usual expectation of toString output
being (fairly) human-readable.

 Or, create a new method describe() or toDescription() that returns a 
 UnmodifiableMap (based on a LinkedHashMap to preserve order)?
 
 I prefer the latter because there is no need to parse the output, and at 
 least in the case of certificate, since a certificate contains 
 extensions, it's very easy to stuff several maps inside another one. The 
 map's keys are strings, and values can be another map or a simple data 
 object, say, primitive, string, or Date.
 
 So this needs a new interface Descriptable. It can either be:
 
 interface sun.security.util.Descriptable;
 sun.security.x509.X509CertImpl implements Descriptable;
 sun.security.x509.Extension implements Descriptable;
 
 or
 
 interface java.security.Descriptable;
 java.security.cert.X509Certificate implements Descriptable;
 java.security.cert.Extension implements Descriptable;
 
 or if there are other people find it useful, it can be inside the 
 java.util package.
 

This sounds like a good idea to me, but would there be a standard set
of keys and values?  Especially if it's going to be part of
java.security, the keys and values should be part of the
method specification.  This becomes a harder task if it's generalised
into java.util.

BTW, it should be Describable :-)

 What's your ideas? Do you also need such a method?
 
 Thanks
 Max

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37

Re: [Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror]

[Dr Andrew John Hughes]

On 17:11 Tue 02 Aug , Alan Bateman wrote:
 Xuelei Fan wrote:
  :
  1. I noticed the copyright date of a few files are unchanged, please
  update them before you push the changes.

 This has come up a few times but I don't think it is strictly required. 
 Kelly or one of the release engineers run a script over the forest 
 periodically to fix up the date range. The nice thing (in my view 
 anyway) about not changing the headers is that it avoids clutter in the 
 patch and webrev. It also makes it a bit easier to transport patches to 
 other releases.
 

I was thinking the same thing.  Copyright header changes mixed into other
patches are a nightmare as regards backporting.

 -Alan
 

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37

Re: code without sources distributed in 7b130

[Dr Andrew John Hughes]

On 16:50 Tue 03 May , Matthias Klose wrote:
 [resending, had the jdk email address wrong]
 
 The following code distributed in 7b130 doesn't have any source code, or at 
 least nobody can find it.
 
 in langtools:
 
 test/tools/javac/T5090006/broken.jar
 
 could the source code be added to the test case?
 
 in jdk:
 test/sun/security/pkcs11/nss/lib/ various precompiled shared libraries. Is it 
 necessary to distribute these with the jdk sources? Checked that these files 
 are 
 available on Debian and Ubuntu distributions
 
 libsoftokn3.so: in /usr/lib, package libnss3-1d/libnss3
 libnss3.so: same
 libnssckbi.so: in /usr/lib/nss
 
 libnspr4.so: in /usr/lib, package libnspr4-0d/libnspr4
 libplds4.so: same
 libplc4.so: same
 
 thanks, Matthias

I agree these binaries shouldn't be in the repositories.  Seems things are 
slipping again.
Thanks for spotting.  Either hg is lying or these have been there since the 
start.

$ hg log -R jdk jdk/test/sun/security/pkcs11/nss/lib/linux-i586/libnssckbi.so 

Worst case, we can hg rm these from the IcedTea forest.  I've now made this 
accessible to
all IcedTea committers on http://icedtea.classpath.org/hg/icedtea7-forest

Could someone also explain
6581254: pkcs11 provider fails to parse configuration file contains windows 
short path
which introduces a file with hardcoded Windows paths
test/sun/security/pkcs11/Provider/csp.cfg ?
-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37

Re: 6998583 Code review request

[Dr Andrew John Hughes]

On 22 November 2010 14:20, Vincent Ryan vincent.x.r...@oracle.com wrote:
 Fix looks good Sean.

 On 22/11/2010 13:50, Seán Coffey wrote:
 Brad, Vinnie,

 This is a forward port of 6998583 to JDK 7. Can you review ?

 http://cr.openjdk.java.net/~coffeys/6998583/webrev.6998583.0/

 Thanks,
 Sean.



In:
+
+(InputStream)java.security.AccessController.doPrivileged
+(new java.security.PrivilegedExceptionAction() {
+public Object run() throws
URISyntaxException, IOException {
+if
(device.getProtocol().equalsIgnoreCase(file)) {
+return new FileInputStream(new
File(device.toURI()));
+} else {
+return new
BufferedInputStream(device.openStream(), 512);
+}

run could return InputStream, avoiding the cast, as
PrivilegedExceptionAction can take InputStream as a type parameter.

+devRandom = java.security.AccessController.doPrivileged
+(new java.security.PrivilegedExceptionAction() {
+public Object run() throws
URISyntaxException, IOException {
+if
(device.getProtocol().equalsIgnoreCase(file)) {
+return new FileInputStream(new
File(device.toURI()));
+} else {
+return new
BufferedInputStream(device.openStream(), 512);
+}

-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

Re: 6998583 Code review request

[Dr Andrew John Hughes]

On 25 November 2010 00:29, Dr Andrew John Hughes
gnu_and...@member.fsf.org wrote:
 On 22 November 2010 14:20, Vincent Ryan vincent.x.r...@oracle.com wrote:
 Fix looks good Sean.

 On 22/11/2010 13:50, Seán Coffey wrote:
 Brad, Vinnie,

 This is a forward port of 6998583 to JDK 7. Can you review ?

 http://cr.openjdk.java.net/~coffeys/6998583/webrev.6998583.0/

 Thanks,
 Sean.



 In:
 +
 +                    (InputStream)java.security.AccessController.doPrivileged
 +                        (new java.security.PrivilegedExceptionAction() {
 +                            public Object run() throws
 URISyntaxException, IOException {
 +                                if
 (device.getProtocol().equalsIgnoreCase(file)) {
 +                                    return new FileInputStream(new
 File(device.toURI()));
 +                                } else {
 +                                    return new
 BufferedInputStream(device.openStream(), 512);
 +                                }

 run could return InputStream, avoiding the cast, as
 PrivilegedExceptionAction can take InputStream as a type parameter.


Final example should have been:

 +                    devRandom = java.security.AccessController.doPrivileged
 +                        (new 
 java.security.PrivilegedExceptionActionInputStream() {
 +                            public InputStream run() throws
 URISyntaxException, IOException {
 +                                if
 (device.getProtocol().equalsIgnoreCase(file)) {
 +                                    return new FileInputStream(new
 File(device.toURI()));
 +                                } else {
 +                                    return new
 BufferedInputStream(device.openStream(), 512);
 +                                }




-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

Re: [security-dev 01547]: Re: PING: [PATCH FOR REVIEW]: 6763530: Fix breakage of NSS-based Elliptic Curve Cryptography in OpenJDK6

[Andrew John Hughes]

On 28 June 2010 01:05, Michael StJohns mstjo...@comcast.net wrote:
 Hi Andrew -

 I really need to work on fleshing out my emails... :-/

 The _20 release of the normal (non-OpenJDK) is missing this update.  I had 
 thought that this fix was supposed to be back-ported to the closed JDK 6 
 release.  I've got the OpenJDK built on my local machine, but the folks I'm 
 working with would rather use the pre-packaged Windows version etc etc etc... 
 *sigh*.


By its very nature, we don't know what's in the proprietary JDK.
That's an issue you need to take up with Oracle.  The bug is fixed in
OpenJDK6 and 7.

 Thanks - Mike


 At 07:17 PM 6/27/2010, Andrew John Hughes wrote:
 At 05:37 PM 6/27/2010, Michael StJohns wrote:
Hi guys -

I see from the Mercurial logs that this went in to both the jdk6 and jdk7 
repositories. Â For jdk6 - it's rev 302 which looks like this should have 
ended up in the _19 release

But all the files in lib/ext/sunpkcs11.jar  for _20 are all tagged as 1 
September 2009

Is the sunpkcs11.jar provider not getting regenerated and rebundled during 
the release process?

Mike

It has been:

$ hg log -R jdk -k 6763530
changeset:   302:82b80660cac3
user:        vinnie
date:        Thu Jan 21 23:59:41 2010 +
summary:     6763530: Cannot decode PublicKey (Proider SunPKCS11,
curve prime256v1)

and certainly is present on IcedTea6's builds.
--
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8






-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

Re: [security-dev 01563]: Subject lines in security-dev.

[Andrew John Hughes]

On 20 April 2010 14:57, Chris Hegarty chris.hega...@oracle.com wrote:
 Brad,

 Sorry to chime in late, but would it make server side rules/filtering
 simpler if we added the mailing list name in the subject line?

 The reason I ask is that since moving to a new mail server I can no longer
 create filters on the 'sender' ( or any other more exotic ) headers. This
 makes filtering a real pain for messages coming from the openjdk mailing
 lists. If the mailing list name was in the subject line... Of course I would
 need all the mailing lists I'm subscribed to to do the same.

 -Chris.

 Brad Wetmore wrote:

 Looking for opinions as to whether the current subject line format of
 the security-...@o.j.n emails is useful:

    Subject: [security-dev 01800]: The Real subject.

 I note a lot of the client (e.g. sounds, swing) lists have just the list
 name, and a lot of the server lists (e.g. tl, hotspot) don't have
 either.  security-dev is the only one doing both the list name and the
 message number.  It's not as useful as I originally thought when I
 configured it, but I could be talked into it either way.

 If you have an opinion, especially for keeping it the same, let me know
 privately, no need to clutter everyone's email.  But I'm thinking I may
 just remove the topic/number.  If I see a clear/obvious preference for
 keeping one or both, I'll certainly take that into consideration.

 Thanks,
 Brad
 security-dev admin




I think the mailing list name is useful and is used by a number of
other lists; with the number of OpenJDK lists, it's nice to be able to
see at a glance which one a mail was sent to.

I've never seen any use for the number.  Indeed, I didn't even know
what it meant until this thread.
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

Re: [PATCH FOR REVIEW]: Improve error output for NSS provider

[Andrew John Hughes]

On 16 April 2010 00:06, Bradford Wetmore bradford.wetm...@oracle.com wrote:
 Hi Andrew,

 I couldn't tell for sure if this was for OpenJDK 7 or OpenJDK 6.  I'm
 assuming the former?


You're correct.  I should have made it clearer in the initial e-mail,
though I did ask if it was ok to push it to tl which is 7 only.  The
patch does apply to both, so I'll ask Joe Darcy about backporting it
once it's in 7.

 Looks good.  I checked for any remaining missing CKR_* values in the
 PKCS11 spec version 2.20, this covers all the missing values.


Thanks.  These are all the values used by the current version of NSS
(3.12.3 IIRC).

 I've filed:

 6944361: Missing CKR_ values in PKCS11Exception

 Our reviewer names would be wetmore, valeriep.


Thanks.  Pushed: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/c444651077d2

 http://cr.openjdk.java.net/~andrew/nss/webrev.01/jdk.patch

 Specifying just the top level of the webrev is preferred.


Whoops!  Sorry, I usually do post the right URL.  Must have copied the
wrong one.

 Thanks!

 Brad


 On 4/12/2010 8:49 AM, Andrew John Hughes wrote:
 Hi,

 I'm trying to debug an issue with the NSS provider crashing on a
 number of JTreg tests.  See
 http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=469

 I found a couple of issues in doing so:

 * The stack trace is incomplete as a couple of exceptions are thrown
 using only the message without including the cause
 * The NSS wrapper can't handle a couple of newer NSS error codes

 This patch:

 http://cr.openjdk.java.net/~andrew/nss/webrev.01/jdk.patch

 fixes both issues and extends:

 java.security.cert.CertificateParsingException: java.io.IOException: subject
 key, Could not create EC public key
         at sun.security.x509.X509CertInfo.init(X509CertInfo.java:171)
         at sun.security.x509.X509CertImpl.parse(X509CertImpl.java:1747)
         at sun.security.x509.X509CertImpl.init(X509CertImpl.java:320)
         at 
 sun.security.provider.X509Factory.parseX509orPKCS7Cert(X509Factory.java:550)
         at 
 sun.security.provider.X509Factory.engineGenerateCertificates(X509Factory.java:434)
         at 
 java.security.cert.CertificateFactory.generateCertificates(CertificateFactory.java:444)
         at ReadCertificates.readCertificates(ReadCertificates.java:51)
         at ReadCertificates.main(ReadCertificates.java:86)
         at PKCS11Test.premain(PKCS11Test.java:79)
         at PKCS11Test.testDefault(PKCS11Test.java:113)
         at PKCS11Test.main(PKCS11Test.java:86)
         at ReadCertificates.main(ReadCertificates.java:57)
         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
         at 
 sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
         at 
 sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
         at java.lang.reflect.Method.invoke(Method.java:616)
         at 
 com.sun.javatest.regtest.MainAction$SameVMThread.run(MainAction.java:595)
         at java.lang.Thread.run(Thread.java:636)
 Caused by: java.io.IOException: subject key, Could not create EC public key
         at sun.security.x509.X509Key.parse(X509Key.java:174)
         at 
 sun.security.x509.CertificateX509Key.init(CertificateX509Key.java:75)
         at sun.security.x509.X509CertInfo.parse(X509CertInfo.java:705)
         at sun.security.x509.X509CertInfo.init(X509CertInfo.java:169)
         ... 17 more

 with:

 Caused by: java.security.InvalidKeyException: Could not create EC public key
         at sun.security.x509.X509Key.buildX509Key(X509Key.java:227)
         at sun.security.x509.X509Key.parse(X509Key.java:170)
         ... 20 more
 Caused by: java.security.spec.InvalidKeySpecException: Could not create EC
 public key
         at 
 sun.security.pkcs11.P11ECKeyFactory.engineGeneratePublic(P11ECKeyFactory.java:154)
         at java.security.KeyFactory.generatePublic(KeyFactory.java:321)
         at sun.security.x509.X509Key.buildX509Key(X509Key.java:223)
         ... 21 more
 Caused by: java.security.InvalidKeyException: Could not create EC public key
         at 
 sun.security.pkcs11.P11ECKeyFactory.implTranslatePublicKey(P11ECKeyFactory.java:117)
         at 
 sun.security.pkcs11.P11ECKeyFactory.engineGeneratePublic(P11ECKeyFactory.java:152)
         ... 23 more
 Caused by: sun.security.pkcs11.wrapper.PKCS11Exception:
 CKR_DOMAIN_PARAMS_INVALID
         at sun.security.pkcs11.wrapper.PKCS11.C_CreateObject(Native Method)
         at 
 sun.security.pkcs11.P11ECKeyFactory.generatePublic(P11ECKeyFactory.java:229)
         at 
 sun.security.pkcs11.P11ECKeyFactory.implTranslatePublicKey(P11ECKeyFactory.java:103)
         ... 24 more

 allowing the native NSS error to be seen.

 Ok to push to tl? If so, can I have a bug ID for this change?

 Thanks,




-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key

[PATCH FOR REVIEW]: Improve error output for NSS provider

[Andrew John Hughes]

Hi,

I'm trying to debug an issue with the NSS provider crashing on a
number of JTreg tests.  See
http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=469

I found a couple of issues in doing so:

* The stack trace is incomplete as a couple of exceptions are thrown
using only the message without including the cause
* The NSS wrapper can't handle a couple of newer NSS error codes

This patch:

http://cr.openjdk.java.net/~andrew/nss/webrev.01/jdk.patch

fixes both issues and extends:

java.security.cert.CertificateParsingException: java.io.IOException: subject
key, Could not create EC public key
at sun.security.x509.X509CertInfo.init(X509CertInfo.java:171)
at sun.security.x509.X509CertImpl.parse(X509CertImpl.java:1747)
at sun.security.x509.X509CertImpl.init(X509CertImpl.java:320)
at 
sun.security.provider.X509Factory.parseX509orPKCS7Cert(X509Factory.java:550)
at 
sun.security.provider.X509Factory.engineGenerateCertificates(X509Factory.java:434)
at 
java.security.cert.CertificateFactory.generateCertificates(CertificateFactory.java:444)
at ReadCertificates.readCertificates(ReadCertificates.java:51)
at ReadCertificates.main(ReadCertificates.java:86)
at PKCS11Test.premain(PKCS11Test.java:79)
at PKCS11Test.testDefault(PKCS11Test.java:113)
at PKCS11Test.main(PKCS11Test.java:86)
at ReadCertificates.main(ReadCertificates.java:57)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:616)
at 
com.sun.javatest.regtest.MainAction$SameVMThread.run(MainAction.java:595)
at java.lang.Thread.run(Thread.java:636)
Caused by: java.io.IOException: subject key, Could not create EC public key
at sun.security.x509.X509Key.parse(X509Key.java:174)
at 
sun.security.x509.CertificateX509Key.init(CertificateX509Key.java:75)
at sun.security.x509.X509CertInfo.parse(X509CertInfo.java:705)
at sun.security.x509.X509CertInfo.init(X509CertInfo.java:169)
... 17 more

with:

Caused by: java.security.InvalidKeyException: Could not create EC public key
at sun.security.x509.X509Key.buildX509Key(X509Key.java:227)
at sun.security.x509.X509Key.parse(X509Key.java:170)
... 20 more
Caused by: java.security.spec.InvalidKeySpecException: Could not create EC
public key
at 
sun.security.pkcs11.P11ECKeyFactory.engineGeneratePublic(P11ECKeyFactory.java:154)
at java.security.KeyFactory.generatePublic(KeyFactory.java:321)
at sun.security.x509.X509Key.buildX509Key(X509Key.java:223)
... 21 more
Caused by: java.security.InvalidKeyException: Could not create EC public key
at 
sun.security.pkcs11.P11ECKeyFactory.implTranslatePublicKey(P11ECKeyFactory.java:117)
at 
sun.security.pkcs11.P11ECKeyFactory.engineGeneratePublic(P11ECKeyFactory.java:152)
... 23 more
Caused by: sun.security.pkcs11.wrapper.PKCS11Exception:
CKR_DOMAIN_PARAMS_INVALID
at sun.security.pkcs11.wrapper.PKCS11.C_CreateObject(Native Method)
at 
sun.security.pkcs11.P11ECKeyFactory.generatePublic(P11ECKeyFactory.java:229)
at 
sun.security.pkcs11.P11ECKeyFactory.implTranslatePublicKey(P11ECKeyFactory.java:103)
... 24 more

allowing the native NSS error to be seen.

Ok to push to tl? If so, can I have a bug ID for this change?

Thanks,
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01761]: Re: Quick code review for 6909281

[Andrew John Hughes]

On 9 April 2010 14:18, Sean Mullan sean.mul...@oracle.com wrote:
 Hi Andrew,

 Could I get a quick code review for 6909281 which also needs to be fixed in
 JDK 7:

 http://cr.openjdk.java.net/~mullan/6909281/webrev/

 Thanks,
 Sean


Sure.  Looks good to me.
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01696]: Re: Please review new regression test for java.net.* API

[Andrew John Hughes]

On 18 March 2010 14:28, Christopher Hegarty -Sun Microsystems Ireland
christopher.hega...@sun.com wrote:
 Alan Bateman wrote:

 Pavel Tisnovsky wrote:

 Hi,

 please review new regression test for java.net.* API. This test check if
 the cacerts keytool database is configured properly and SSL is really
 working. The test should not fail if SSL is working (in other case it simply
 throws IOException). Webrev si available at
 http://cr.openjdk.java.net/~ptisnovs/TestHttps/

 Thanks in advance
 Pavel Tisnovsky

 I suspect the dependency on verisign.com will be problematic.  Isn't SSL
 already covered by the javax.net and https tests?

 I'm not sure what the prime motivation of the test is. Pavel, can you please
 elaborate?

 Reading between the lines I guess the test is verifying that the correct
  root Certification Authority is installed in cacerts, i.e. the cert from
 www.verisign.com can be validated.

 Alan is correct there are already tests for SSL/Https in javax.net, but I
 believe these use self signed certs, no dependency on cacerts.


Sounds like you have things spot on to me, Chris.

 -Chris.


 -Alan.




-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01702]: Re: Please review new regression test for java.net.* API

[Andrew John Hughes]

On 18 March 2010 14:57, Christopher Hegarty -Sun Microsystems Ireland
christopher.hega...@sun.com wrote:
 Pavel Tisnovsky wrote:

 Christopher Hegarty -Sun Microsystems Ireland wrote:

 Alan Bateman wrote:

 Pavel Tisnovsky wrote:

 Hi,

 please review new regression test for java.net.* API. This test check
 if the cacerts keytool database is configured properly and SSL is really
 working. The test should not fail if SSL is working (in other case it 
 simply
 throws IOException). Webrev si available at
 http://cr.openjdk.java.net/~ptisnovs/TestHttps/

 Thanks in advance
 Pavel Tisnovsky

 I suspect the dependency on verisign.com will be problematic.  Isn't SSL
 already covered by the javax.net and https tests?

 I'm not sure what the prime motivation of the test is. Pavel, can you
 please elaborate?

 Reading between the lines I guess the test is verifying that the correct
  root Certification Authority is installed in cacerts, i.e. the cert from
 www.verisign.com can be validated.

 Hi Chris, you guessed correctly :-) And we can use other URL if
 verisign.com is problematic.

 OK, so the test is trying to validate cacerts.

 Does it make sense to validate this certificate store in a general purpose
 regression test? The test will of course pass with Sun's priority build and
 probably RedHats too, since they contain the root certificate for verisign,
 but an OpenJDK build will not contain it, right? So the test will fail.

 Security folk:
  Do we currently have any tests with a dependency on cacerts?

 -Chris.




 Alan is correct there are already tests for SSL/Https in javax.net, but I
 believe these use self signed certs, no dependency on cacerts.

 -Chris.


 -Alan.



Yes, it will fail.

From an OpenJDK build:

 $ /mnt/builder/jdk7/j2sdk-image/bin/java TestHttps
Exception in thread main javax.net.ssl.SSLException:
java.lang.RuntimeException: Unexpected error:
java.security.InvalidAlgorithmParameterException: the trustAnchors
parameter must be non-empty

This has been posted about before; OpenJDK currently can't bootstrap
itself because it doesn't have a working cacerts store (the JAXP URL
uses https).

I don't know how to solve this; we can certainly have the cacerts file
populated on GNU/Linux systems, but I don't have a clue how you'd do
it on Solaris or Windows.  How do Sun populate it? Can that be shared?
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01705]: Re: Please review new regression test for java.net.* API

[Andrew John Hughes]

On 18 March 2010 15:07, Christopher Hegarty -Sun Microsystems Ireland
christopher.hega...@sun.com wrote:


 Sean Mullan wrote:

 

 Security folk:
  Do we currently have any tests with a dependency on cacerts?

 yes, but they would be in the closed tests.

 So we have your own non public tests for this. Maybe RedHat should take a
 similar approach then.

 -Chris.


 --Sean


We don't really do 'non public' at Red Hat.  The patch will be in
IcedTea which is publicly available, but it would be better for
upstream to remain as in sync with us as possible.
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01706]: Re: Please review new regression test for java.net.* API

[Andrew John Hughes]

On 18 March 2010 15:13, Sean Mullan sean.mul...@sun.com wrote:
 Andrew John Hughes wrote:

 This has been posted about before; OpenJDK currently can't bootstrap
 itself because it doesn't have a working cacerts store (the JAXP URL
 uses https).

 I don't know how to solve this; we can certainly have the cacerts file
 populated on GNU/Linux systems, but I don't have a clue how you'd do
 it on Solaris or Windows.  How do Sun populate it? Can that be shared?

 No. The agreements we have with CAs to include root CA certificates are for
 our product releases only, we can't (at least not right now) include them in
 OpenJDK.


So they don't just use system-installed ones? Ok.

 I haven't been following this thread in great detail, but don't existing
 JSSE tests cover this?


No, if they did we wouldn't need another test.

 --Sean





-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01708]: Re: Please review new regression test for java.net.* API

[Andrew John Hughes]

On 18 March 2010 18:40, Brad Wetmore bradford.wetm...@sun.com wrote:

 I have a couple important tasks to finish ASAP, so if there is more
 discussion, I'll have to jump in sometime next week, but wanted to add
 one thing before anything was done:

 Pavel wrote:
 And we can use other URL if verisign.com is problematic.

 We've tried to limit the reliance on servers outside our control for the
 open tests and to be as self-contained as possible, tho I'm sure there
 are still some tests that do this anyway.  IMHO, it's not exactly
 neighborly of OpenJDK to include tests that just bang on someone's
 server(s) for testing, even if the volume isn't terribly high.  I
 think we should check with the server's admin before we included such a
 test in the general repository.

 In the past we've also had transient network errors (servers or network
 down), so that was another reason to limit our external dependencies.
 But they still had to be investigated and took time.


https://jaxp.dev.java.net/files/documents/913/147490 seems an
appropriate URL to hit.  It's the very URL that causes the OpenJDK
build to fail to bootstrap itself and I assume Oracle do control
dev.java.net to some degree.

 Brad






 On 3/18/2010 8:50 AM, Pavel Tisnovsky wrote:
 Christopher Hegarty -Sun Microsystems Ireland wrote:
 Alan Bateman wrote:
 Pavel Tisnovsky wrote:
 Hi,

 please review new regression test for java.net.* API. This test
 check if the cacerts keytool database is configured properly and SSL
 is really working. The test should not fail if SSL is working (in
 other case it simply throws IOException). Webrev si available at
 http://cr.openjdk.java.net/~ptisnovs/TestHttps/

 Thanks in advance
 Pavel Tisnovsky
 I suspect the dependency on verisign.com will be problematic.  Isn't
 SSL already covered by the javax.net and https tests?

 I'm not sure what the prime motivation of the test is. Pavel, can you
 please elaborate?

 Reading between the lines I guess the test is verifying that the
 correct  root Certification Authority is installed in cacerts, i.e.
 the cert from www.verisign.com can be validated.

 Hi Chris, you guessed correctly :-) And we can use other URL if
 verisign.com is problematic.


 Alan is correct there are already tests for SSL/Https in javax.net,
 but I believe these use self signed certs, no dependency on cacerts.

 -Chris.


 -Alan.





-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01710]: Re: Please review new regression test for java.net.* API

[Andrew John Hughes]

On 18 March 2010 20:56, Christopher Hegarty -Sun Microsystems Ireland
christopher.hega...@sun.com wrote:
 Brad, Pavel, Andrew,

 I'm also not comfortable with this test, but what bothers me more than the
 reliance on an external server is the reliance on cacerts. While cacerts (or
 equivalent) is not part of OpenJDK I don't think it makes sense adding a
 test to OpenJDK that has a reliance on it.

 For now I think is makes more sense to add a test like this to wherever in
 the build process cacerts (or equivalent) is added.


The problem is nothing does in the OpenJDK build process.  So SSL is
always broken for OpenJDK builds.  Is this something we really want?

 -Chris

 Andrew John Hughes wrote:

 On 18 March 2010 18:40, Brad Wetmore bradford.wetm...@sun.com wrote:

 I have a couple important tasks to finish ASAP, so if there is more
 discussion, I'll have to jump in sometime next week, but wanted to add
 one thing before anything was done:

 Pavel wrote:

 And we can use other URL if verisign.com is problematic.

 We've tried to limit the reliance on servers outside our control for the
 open tests and to be as self-contained as possible, tho I'm sure there
 are still some tests that do this anyway.  IMHO, it's not exactly
 neighborly of OpenJDK to include tests that just bang on someone's
 server(s) for testing, even if the volume isn't terribly high.  I
 think we should check with the server's admin before we included such a
 test in the general repository.

 In the past we've also had transient network errors (servers or network
 down), so that was another reason to limit our external dependencies.
 But they still had to be investigated and took time.


 https://jaxp.dev.java.net/files/documents/913/147490 seems an
 appropriate URL to hit.  It's the very URL that causes the OpenJDK
 build to fail to bootstrap itself and I assume Oracle do control
 dev.java.net to some degree.

 Brad






 On 3/18/2010 8:50 AM, Pavel Tisnovsky wrote:

 Christopher Hegarty -Sun Microsystems Ireland wrote:

 Alan Bateman wrote:

 Pavel Tisnovsky wrote:

 Hi,

 please review new regression test for java.net.* API. This test
 check if the cacerts keytool database is configured properly and SSL
 is really working. The test should not fail if SSL is working (in
 other case it simply throws IOException). Webrev si available at
 http://cr.openjdk.java.net/~ptisnovs/TestHttps/

 Thanks in advance
 Pavel Tisnovsky

 I suspect the dependency on verisign.com will be problematic.  Isn't
 SSL already covered by the javax.net and https tests?

 I'm not sure what the prime motivation of the test is. Pavel, can you
 please elaborate?

 Reading between the lines I guess the test is verifying that the
 correct  root Certification Authority is installed in cacerts, i.e.
 the cert from www.verisign.com can be validated.

 Hi Chris, you guessed correctly :-) And we can use other URL if
 verisign.com is problematic.

 Alan is correct there are already tests for SSL/Https in javax.net,
 but I believe these use self signed certs, no dependency on cacerts.

 -Chris.

 -Alan.







-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01712]: Re: Please review new regression test for java.net.* API

[Andrew John Hughes]

On 18 March 2010 21:12, Christopher Hegarty -Sun Microsystems Ireland
christopher.hega...@sun.com wrote:
 Andrew John Hughes wrote:

 On 18 March 2010 20:56, Christopher Hegarty -Sun Microsystems Ireland
 christopher.hega...@sun.com wrote:

 Brad, Pavel, Andrew,

 I'm also not comfortable with this test, but what bothers me more than
 the
 reliance on an external server is the reliance on cacerts. While cacerts
 (or
 equivalent) is not part of OpenJDK I don't think it makes sense adding a
 test to OpenJDK that has a reliance on it.

 For now I think is makes more sense to add a test like this to wherever
 in
 the build process cacerts (or equivalent) is added.


 The problem is nothing does in the OpenJDK build process.  So SSL is
 always broken for OpenJDK builds.  Is this something we really want?

 This is certainly not ideal, but is a separate issue to the test, right? It
 seems Sean or someone in the security team should comment on the possibility
 of adding root CA's to OpenJDK, until then I don't see any requirement for a
 test.


My thoughts too.  We have a solution for GNU/Linux where cacerts is
populated from the crt files found on the system (installed by Mozilla
and the like).  I don't know what the equivalent would be for Windows
and Solaris though.  A quick look on my OpenSolaris box didn't find
any crt files but I only looked in installed packages.  I presume
firefox may bring some in if it's available.

 -Chris.


 -Chris

 Andrew John Hughes wrote:

 On 18 March 2010 18:40, Brad Wetmore bradford.wetm...@sun.com wrote:

 I have a couple important tasks to finish ASAP, so if there is more
 discussion, I'll have to jump in sometime next week, but wanted to add
 one thing before anything was done:

 Pavel wrote:

 And we can use other URL if verisign.com is problematic.

 We've tried to limit the reliance on servers outside our control for
 the
 open tests and to be as self-contained as possible, tho I'm sure there
 are still some tests that do this anyway.  IMHO, it's not exactly
 neighborly of OpenJDK to include tests that just bang on someone's
 server(s) for testing, even if the volume isn't terribly high.  I
 think we should check with the server's admin before we included such a
 test in the general repository.

 In the past we've also had transient network errors (servers or network
 down), so that was another reason to limit our external dependencies.
 But they still had to be investigated and took time.

 https://jaxp.dev.java.net/files/documents/913/147490 seems an
 appropriate URL to hit.  It's the very URL that causes the OpenJDK
 build to fail to bootstrap itself and I assume Oracle do control
 dev.java.net to some degree.

 Brad






 On 3/18/2010 8:50 AM, Pavel Tisnovsky wrote:

 Christopher Hegarty -Sun Microsystems Ireland wrote:

 Alan Bateman wrote:

 Pavel Tisnovsky wrote:

 Hi,

 please review new regression test for java.net.* API. This test
 check if the cacerts keytool database is configured properly and
 SSL
 is really working. The test should not fail if SSL is working (in
 other case it simply throws IOException). Webrev si available at
 http://cr.openjdk.java.net/~ptisnovs/TestHttps/

 Thanks in advance
 Pavel Tisnovsky

 I suspect the dependency on verisign.com will be problematic.  Isn't
 SSL already covered by the javax.net and https tests?

 I'm not sure what the prime motivation of the test is. Pavel, can you
 please elaborate?

 Reading between the lines I guess the test is verifying that the
 correct  root Certification Authority is installed in cacerts, i.e.
 the cert from www.verisign.com can be validated.

 Hi Chris, you guessed correctly :-) And we can use other URL if
 verisign.com is problematic.

 Alan is correct there are already tests for SSL/Https in javax.net,
 but I believe these use self signed certs, no dependency on cacerts.

 -Chris.

 -Alan.









-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01542]: Re: PING: [PATCH FOR REVIEW]: 6763530: Fix breakage of NSS-based Elliptic Curve Cryptography in OpenJDK6

[Andrew John Hughes]

2010/1/20 Michael StJohns mstjo...@comcast.net:
 Hi - this seems to have stalled out again.  Any chance of revival?


Never mind stalled, it doesn't appear to have even started to begin with!

We do ship the patch with IcedTea6.  If Sun don't want the fix for
OpenJDK itself, I guess that's their problem.

 Mike


 At 12:33 PM 9/24/2009, Vincent Ryan wrote:
Hello Andrew,

I'll need a little more time to come up to speed on this fix. I'm concerned 
that
there may be interoperability or backwards compatibility issues.



Andrew John Hughes wrote:
 2009/9/2 Andrew John Hughes gnu_and...@member.fsf.org:
 2009/9/2 Michael StJohns mstjo...@comcast.net:
 At 09:38 PM 9/1/2009, Andrew John Hughes wrote:
 2009/9/2 Michael StJohns mstjo...@comcast.net:
  This appears to be related specifically to PKCS11.  Specifically, 
 PKCS11
 v2.20 has some ambiguity of the representation of an EC point (which is
 different in the text than an ASN1 ECPoint).

 This is being clarified in v2.30 with the unencoded point format 
 (e.g.the
 format described in  X9.62, where the first octet indicates the 
 encoding and
 there are either N or 2N octets following)Â  being the expected value, 
 but
 with PKCS11 providers allowed - legacy - to accept either.

 One of the reasons for going that way was how the JDK PKCS11 provider 
 had
 interpreted the issue and implemented its code.

 I don't support this fix - among other things, this fix only deals with 
 1/2
 of the problem.  The other half is related to encoding the value.  
 Also,
 changing the code at decodePoint seems further into the stack than 
 needed
 and may affect other uses of that method.

 That's really too vague to be of much help in improving the patch.
 You seem to be saying little more than 'I don't like it'.
 Sorry about that.  My point was that your patch didn't completely solve 
 the problem and that the point at where you were fixing it could have 
 some bad side effects for anyone calling decodePoint directly.


 There's an existing JDK bug on this coming at it from a different 
 direction
 - 6763530 ... and there may be considerations at

 https://bugzilla.mozilla.org/show_bug.cgi?id=480280

 It seems likely that's the NSS change that causes the current failure.
 The fix I submitted here is based on the way this is handle in NSS.
 In fact, the code is similar enough to suggest that one was developed
 from the other.
 Â that should be looked at.
 The JDK bug is not really 'from a different direction', it's reporting
 exactly the same error but from a less trivial example (I get the same
 failure while trying to create an example key, while this seems to
 require specific hardware if I'm reading it correctly).
 Not exactly.  You're using the NSS as a PKCS11 module - this problem 
 would occur with any PKCS11 module that implements EC stuff.


 Also see 6779460 which is mostly a duplicate of
 6763530.

 The patch on 6779460 seems wrong.  It means that the method will
 return a DER-encoded value where it would either have returned an
 uncompressed value before or failed.
 My point exactly as I mentioned in the comments.  :-)


 It's probable that the fix I suggested at 6763530Â  (in comments 
 submitted 29
 Nov 08) may be a better approach given the NSS fixes.  I believe it 
 will fix
 the keytool problem noted in the original message.

 Ok, I can see the logic in the fix and it would appear to work, though
 I haven't tested it.
 Given the patch was written nine months ago, why has it not been
 applied?  If it had, it would have saved me hours having to debug this
 same issue again.
 Yup.  I did do a search for PKCS11 related bugs when I encountered the 
 same problem and did find the original error.

 Do you have an SCA with Sun? If so, I'll create a webrev based on your
 patch and we can finally get this fixed.  Without it, NSS support is
 completely broken in OpenJDK6 which makes me wonder why this is a low
 priority bug!
 I do have an SCA on file.  Note that the recommendation from the NSS guys 
 was to raise the priority.

 The reason I haven't submitted this is because I submitted a different EC 
 fix  https://bugs.openjdk.java.net/show_bug.cgi?id=100048 per the 
 documented process
  and was waiting on progress there before continuing.  I've got a number 
 of EC and PKCS11 related fixes I'd like to submit, but I was trying for a 
 worked example before proceeding.  And then I got busy with some other 
 things...

 Mike





 Mike





 At 04:39 PM 9/1/2009, Joe Darcy wrote:

 Andrew John Hughes wrote:

 2009/8/28 Andrew John Hughes gnu_and...@member.fsf.org:

 In OpenJDK6, the elliptic curve cryptography algorithms are available
 if the PKCS11 provider is configured to point to NSS. See:

 http://blogs.sun.com/andreas/entry/the_java_pkcs_11_provider

 If NSS is configured as specified in this blog, keytool can be used to
 generate a key as follows:

 Hello.

 Allowing keytool and friends to work in more cases if the provider is
 capable seems fine

[security-dev 01535]: Re: Please review changes in regression test test/java/security/Provider/Turkish.java

[Andrew John Hughes]

2010/1/18 Alan Bateman alan.bate...@sun.com:
 Andrew John Hughes wrote:

 :
 As mentioned by Joe
 (http://mail.openjdk.java.net/pipermail/jdk6-dev/2010-January/001135.html)
 patches for jdk6 should be sent to the jdk6-dev list before being
 pushed to the jdk6 tree.


 It might be good to also ping the mailing list for the area (as I don't
 think everyone is subscribed to jdk6-dev).

 -Alan.



Yes, sorry, I meant that jdk6-dev should be in addition to the normal
area list, if the patch is also intended for OpenJDK6.
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01424]: Re: Please review patch for regression test sun/security/tools/keytool/StartDate

[Andrew John Hughes]

2009/12/3 Pavel Tisnovsky ptisn...@redhat.com:
 Hi,

 patch for regression test sun/security/tools/keytool/StartDate.java
 (included in OpenJDK6) is exposed at

 http://cr.openjdk.java.net/~ptisnovs/StartDateTest/

 and prepared for review.

 This patch ensures, that this test does not fail on December(s) due to
 incorrect comparison of two months (12 != 0).

 Can you please review and possibly approve this patch?

 Pavel Tisnovsky
 Red Hat


This looks sane to me, and indeed the fix has already been applied in OpenJDK7:

http://hg.openjdk.java.net/jdk7/jdk7/jdk/rev/2c37083730b1

Joe, can we please backport this to OpenJDK6?
--
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01363]: Re: Elliptic curve bugs?

[Andrew John Hughes]

2009/11/2 Michael StJohns mstjo...@comcast.net:
 I submitted the fix a while ago at

 https://bugs.openjdk.java.net/show_bug.cgi?id=100048

 Still pending...  Mike


 At 03:38 AM 11/2/2009, Tomas Gustavsson wrote:

Hi,

I found this by for Elliptic curve crypto:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6738532

It's quite old and there seems to ba a quite easy resolution to the bug
(second suggested solutions).

Any plans to fix it?

Cheers,
Tomas





This one:

http://mail.openjdk.java.net/pipermail/security-dev/2009-September/001243.html

is in the same boat.  There only seems to be Vincent working on the
ECC stuff, and I guess he hasn't got to either of them yet.
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01277]: Re: ECC pkcs#11 bug]

[Andrew John Hughes]

2009/10/5 Tomas Gustavsson to...@primekey.se:

 Hi Vincent and Brad,

 I'm not sure how things are at Sun currently. We work with Sun here in
 Sweden so we've heard a bit about wait with the Oracle story.

 Anyhow I just want to let you know that if anyone is still working on
 crypto that this bug is very annoying, and affect all existing HSMs as
 far as I can see. ECC is rolling out pretty wide in europe now with new
 electronic passports and other ecc cards.
 So getting this fixed would be quite welcome, it's a small fix. I've
 tested it on SafeNet HSMs myself right now.


 Kind regards,
 Tomas Gustavsson
 PrimeKey Solutions AB


 Lars Silvén wrote:
  Forwarded Message 
 From: Brad Wetmore bradford.wetm...@sun.com
 To: Lars Silvén l...@primekey.se
 Cc: security-dev@openjdk.java.net, Vinnie Ryan vincent.r...@sun.com
 Subject: Re: [security-dev 00550]: Re: ECC pkcs#11 bug
 Date: Thu, 05 Feb 2009 11:34:49 -0800

 Hi Lars,

 I was hoping that Vincent Ryan had already contacted you about this.

 I got redirected from ECC to work on the OpenJDK Bugzilla instance,
 which is rolling out very soon.  Vincent took over the ECC work late
 last year along with your submission.  The short answer is, between a
 lengthy customer escalation and bugzilla, I've been so heads down for
 the last 4 months, I'm not sure how far he's gotten.

 Vinnie, can you provide more info?

 Brad


 Lars Silvén wrote:
 Brad,

 Any news about the p11 ECC bug.

 When will it be fixed?


 Best Regards,
 Lars



 Lars Silvén wrote:
 Hello,

 Thank you for taking care of this.
 We want this fix in both JDK 6 and 7. I like to know the release date for 
 the
 fix in both versions if possible.

 Lars

 Brad Wetmore wrote:
 Lars Silvén wrote:
 Hi Brad,

 Do you have everything you need to fix the bug.
 I believe so.  I haven't started looking at it closely yet, I'm still
 mopping up several fires.  Unfortunately, I'm the chef, busboy, and
 bottle washer for several projects here.

 Or is there anything more I could do to help.

 I have now also tested the nCipher HSM. To get their p11 working my
 patch had to be applied.

 Do you have any idea when we the fix could be released?
 Are you looking for JDK7, or 6?

 Brad

 Best Regards

 Brad Wetmore wrote:
 Lars Silvén wrote:
 Hi Brad,

 I have written a simple application that illustrates the problem:
 http://bunny.primekey.se/~lars/sunP11Bug/src/test/Main.java

 But you need a p11 module with ECC capability to run it. Do you have
 one?
 Yes.

 If not I could investigate if one of our HSM vendors could send you
 one.
 Also to verify that the public key actually is usable a JCA provider
 with ECC is needed.
 I'm going to be working on adding ECC to the JCE provider for JDK 7.

 Thanks for the case.

 Brad


  But for that you could use BouncyCastle.
 Start running the application without parameters and then you get a
 description of needed parameters.

 Lars


 Brad Wetmore wrote:
 Great, thanks for doing so.

 I'll be working on this fairly soon, so I'll get a bug filed.  Do you
 have a standalone test case for this already?  See step 3 of the
 contribute page.  If you do but you don't have it in jtreg format,
 I can
 get it into the jtreg format.

 Brad


 Lars Silvén wrote:
 Here is my SCA!

 //Lars


 Brad Wetmore wrote:
 Hi Lars,

 I have created a patch that is fixing the problem:
 This is Brad Wetmore, I am the Security group Moderator, and also
 the
 person who will be handling this when I get back to working on the
 Java
 ECC implementation.

 Unfortunately, I can't take your source contribution yet without a
 signed copy of the Sun Contribution Agreement in place.  This is
 done
 for your protection as well as the Sun's and the OpenJDK
 community's.

 Please see the following link for more information:

     http://openjdk.java.net/contribute/

 The Signatories of the SCA are eligible to donate code to all
 products
 and projects owned or managed by Sun:  signing it once means you can
 contribute code to any Sun-sponsored open source project.

 If you have recently signed it and it hasn't yet appeared in our
 database yet, just let me know.

 Discussions of the problem is fine, it's just the source that we
 can't
 take at this point.

 Thanks,

 Brad
 





What bug are we discussing here? I don't see any patch or bug ID.
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01278]: Re: ECC pkcs#11 bug]

[Andrew John Hughes]

2009/10/6 Tomas Gustavsson to...@primekey.se:

 Hi Andrew,

 I guess no bug Id was created after all.
 The issue is that the pkcs#11 library returns a tag-length-value
 encoding for an EC public key, but the Sun provider expects something
 else. So when trying to read the public key from pkcs#11 we get an
 exception.

 The patch, which is very small and backwards compatible (if there are
 pkcs#11's that does return the value originally expected), can be found
 here:
 http://bunny.primekey.se/~lars/sunP11Bug/patch.txt

 A simple test case:
 http://bunny.primekey.se/~lars/sunP11Bug/src/test/Main.java

 We've been in contact with an HSM vendor (Utimaco) and they claim that
 the tag-length-value is the right way. Since we tested this with several
 different HSMs it seems they are in agreement as well :-)
 (I can forward their explanation as well if needed).

 Kind regards,
 Tomas

 PS: Lars (who is my collegue) has completed the Sun Contribution
 Agreement.


 Andrew John Hughes wrote:
 2009/10/5 Tomas Gustavsson to...@primekey.se:
 Hi Vincent and Brad,

 I'm not sure how things are at Sun currently. We work with Sun here in
 Sweden so we've heard a bit about wait with the Oracle story.

 Anyhow I just want to let you know that if anyone is still working on
 crypto that this bug is very annoying, and affect all existing HSMs as
 far as I can see. ECC is rolling out pretty wide in europe now with new
 electronic passports and other ecc cards.
 So getting this fixed would be quite welcome, it's a small fix. I've
 tested it on SafeNet HSMs myself right now.


 Kind regards,
 Tomas Gustavsson
 PrimeKey Solutions AB


 Lars Silvén wrote:
  Forwarded Message 
 From: Brad Wetmore bradford.wetm...@sun.com
 To: Lars Silvén l...@primekey.se
 Cc: security-dev@openjdk.java.net, Vinnie Ryan vincent.r...@sun.com
 Subject: Re: [security-dev 00550]: Re: ECC pkcs#11 bug
 Date: Thu, 05 Feb 2009 11:34:49 -0800

 Hi Lars,

 I was hoping that Vincent Ryan had already contacted you about this.

 I got redirected from ECC to work on the OpenJDK Bugzilla instance,
 which is rolling out very soon.  Vincent took over the ECC work late
 last year along with your submission.  The short answer is, between a
 lengthy customer escalation and bugzilla, I've been so heads down for
 the last 4 months, I'm not sure how far he's gotten.

 Vinnie, can you provide more info?

 Brad


 Lars Silvén wrote:
 Brad,

 Any news about the p11 ECC bug.

 When will it be fixed?


 Best Regards,
 Lars



 Lars Silvén wrote:
 Hello,

 Thank you for taking care of this.
 We want this fix in both JDK 6 and 7. I like to know the release date 
 for the
 fix in both versions if possible.

 Lars

 Brad Wetmore wrote:
 Lars Silvén wrote:
 Hi Brad,

 Do you have everything you need to fix the bug.
 I believe so.  I haven't started looking at it closely yet, I'm still
 mopping up several fires.  Unfortunately, I'm the chef, busboy, and
 bottle washer for several projects here.

 Or is there anything more I could do to help.

 I have now also tested the nCipher HSM. To get their p11 working my
 patch had to be applied.

 Do you have any idea when we the fix could be released?
 Are you looking for JDK7, or 6?

 Brad

 Best Regards

 Brad Wetmore wrote:
 Lars Silvén wrote:
 Hi Brad,

 I have written a simple application that illustrates the problem:
 http://bunny.primekey.se/~lars/sunP11Bug/src/test/Main.java

 But you need a p11 module with ECC capability to run it. Do you have
 one?
 Yes.

 If not I could investigate if one of our HSM vendors could send you
 one.
 Also to verify that the public key actually is usable a JCA provider
 with ECC is needed.
 I'm going to be working on adding ECC to the JCE provider for JDK 7.

 Thanks for the case.

 Brad


  But for that you could use BouncyCastle.
 Start running the application without parameters and then you get a
 description of needed parameters.

 Lars


 Brad Wetmore wrote:
 Great, thanks for doing so.

 I'll be working on this fairly soon, so I'll get a bug filed.  Do 
 you
 have a standalone test case for this already?  See step 3 of the
 contribute page.  If you do but you don't have it in jtreg format,
 I can
 get it into the jtreg format.

 Brad


 Lars Silvén wrote:
 Here is my SCA!

 //Lars


 Brad Wetmore wrote:
 Hi Lars,

 I have created a patch that is fixing the problem:
 This is Brad Wetmore, I am the Security group Moderator, and also
 the
 person who will be handling this when I get back to working on the
 Java
 ECC implementation.

 Unfortunately, I can't take your source contribution yet without a
 signed copy of the Sun Contribution Agreement in place.  This is
 done
 for your protection as well as the Sun's and the OpenJDK
 community's.

 Please see the following link for more information:

     http://openjdk.java.net/contribute/

 The Signatories of the SCA are eligible to donate code to all
 products
 and projects owned or managed by Sun:  signing it once means you 
 can

[security-dev 01244]: PING: [PATCH FOR REVIEW]: 6763530: Fix breakage of NSS-based Elliptic Curve Cryptography in OpenJDK6

[Andrew John Hughes]

2009/9/2 Andrew John Hughes gnu_and...@member.fsf.org:
 2009/9/2 Michael StJohns mstjo...@comcast.net:
 At 09:38 PM 9/1/2009, Andrew John Hughes wrote:
2009/9/2 Michael StJohns mstjo...@comcast.net:
  This appears to be related specifically to PKCS11.  Specifically, PKCS11
 v2.20 has some ambiguity of the representation of an EC point (which is
 different in the text than an ASN1 ECPoint).

 This is being clarified in v2.30 with the unencoded point format (e.g.the
 format described in  X9.62, where the first octet indicates the encoding 
 and
 there are either N or 2N octets following)Â  being the expected value, but
 with PKCS11 providers allowed - legacy - to accept either.

 One of the reasons for going that way was how the JDK PKCS11 provider had
 interpreted the issue and implemented its code.

 I don't support this fix - among other things, this fix only deals with 1/2
 of the problem.  The other half is related to encoding the value.  Also,
 changing the code at decodePoint seems further into the stack than needed
 and may affect other uses of that method.


That's really too vague to be of much help in improving the patch.
You seem to be saying little more than 'I don't like it'.

 Sorry about that.  My point was that your patch didn't completely solve the 
 problem and that the point at where you were fixing it could have some bad 
 side effects for anyone calling decodePoint directly.


 There's an existing JDK bug on this coming at it from a different direction
 - 6763530 ... and there may be considerations at

 https://bugzilla.mozilla.org/show_bug.cgi?id=480280


It seems likely that's the NSS change that causes the current failure.
 The fix I submitted here is based on the way this is handle in NSS.
In fact, the code is similar enough to suggest that one was developed
from the other.

 Â that should be looked at.

The JDK bug is not really 'from a different direction', it's reporting
exactly the same error but from a less trivial example (I get the same
failure while trying to create an example key, while this seems to
require specific hardware if I'm reading it correctly).

 Not exactly.  You're using the NSS as a PKCS11 module - this problem would 
 occur with any PKCS11 module that implements EC stuff.


Also see 6779460 which is mostly a duplicate of
 6763530.


The patch on 6779460 seems wrong.  It means that the method will
return a DER-encoded value where it would either have returned an
uncompressed value before or failed.

 My point exactly as I mentioned in the comments.  :-)



 It's probable that the fix I suggested at 6763530Â  (in comments submitted 
 29
 Nov 08) may be a better approach given the NSS fixes.  I believe it will 
 fix
 the keytool problem noted in the original message.


Ok, I can see the logic in the fix and it would appear to work, though
I haven't tested it.
Given the patch was written nine months ago, why has it not been
applied?  If it had, it would have saved me hours having to debug this
same issue again.

 Yup.  I did do a search for PKCS11 related bugs when I encountered the same 
 problem and did find the original error.

Do you have an SCA with Sun? If so, I'll create a webrev based on your
patch and we can finally get this fixed.  Without it, NSS support is
completely broken in OpenJDK6 which makes me wonder why this is a low
priority bug!

 I do have an SCA on file.  Note that the recommendation from the NSS guys 
 was to raise the priority.

 The reason I haven't submitted this is because I submitted a different EC 
 fix  https://bugs.openjdk.java.net/show_bug.cgi?id=100048 per the documented 
 process
  and was waiting on progress there before continuing.  I've got a number of 
 EC and PKCS11 related fixes I'd like to submit, but I was trying for a 
 worked example before proceeding.  And then I got busy with some other 
 things...

 Mike





 Mike





 At 04:39 PM 9/1/2009, Joe Darcy wrote:

 Andrew John Hughes wrote:

 2009/8/28 Andrew John Hughes gnu_and...@member.fsf.org:

 In OpenJDK6, the elliptic curve cryptography algorithms are available
 if the PKCS11 provider is configured to point to NSS. See:

 http://blogs.sun.com/andreas/entry/the_java_pkcs_11_provider

 If NSS is configured as specified in this blog, keytool can be used to
 generate a key as follows:

 Hello.

 Allowing keytool and friends to work in more cases if the provider is
 capable seems fine to me.

 Security team, do you have concerns about this patch?

 Thanks,

 -Joe




--
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8




 Ok here is a new webrev:

 http://cr.openjdk.java.net/~andrew/6763530/webrev.02/

 with a slightly revised version of your change (you can't throw a
 PKCS11Exception which only takes

[security-dev 01200]: Re: PKCS11 and Win X64?

[Andrew John Hughes]

2009/9/10 Michael StJohns mstjo...@comcast.net:
 That makes sense - but was a surprise.  Let me see if I can get one of the 
 companies I work with to contribute their simulator - not to be shipped, but 
 to be tested against.  The simulator runs at 32bits, but the pkcs11 library 
 that talks to the simulator has builds for win 32/64 linux 32/64 and Solaris.


This OpenJDK7 changeset might help in the future on Windows x86_64:

http://hg.openjdk.java.net/jdk7/jdk7/jdk/rev/1ff7163fc5f7

It would be nice if it did help someone.  It doesn't help us with IcedTea.

Interesting Windows on x86_64 seems to be one of the builds that's
always tested, whereas GNU/Linux on x86_64 isn't -- strange given the
latter is older and far more widespread.
See http://mail.openjdk.java.net/pipermail/jdk7-dev

 Mike



 At 08:05 PM 9/9/2009, Brad Wetmore wrote:
Hi Michael,

In JDK6, the primary reason we did not ship a win64 version of the SunPKCS11 
provider is that we did not have access to any PKCS#11 implementation to test 
with on win64. We did not want to ship something that could not be tested at 
all and could potentially be DOA.

I looked for a bugid, but don't see one offhand.

It shouldn't be too hard to enable such a build.

Brad


Michael StJohns wrote:
Any idea why the win x64 jdk and jre 6 builds  (1.6.0_16) are shipping 
without the sunpkcs11.jar?  The only posted bug I see on this is from 1.5 (

    6571044) and its marked as resolved.
    I'm in the process of moving over to an x64 system and this is a
    prelim to doing an x64 build of  jdk6.  Its possible this is already
    resolved, but I'm not tracking the build system stuff so it probably
    got past me.
    Thanks - Mike






-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01201]: Re: 6840752: Provide out-of-the-box support for ECC algorithms

[Andrew John Hughes]

2009/9/10 Andrew John Hughes gnu_and...@member.fsf.org:
 2009/9/9 Vincent Ryan vincent.r...@sun.com:
 Hello Andrew,

 I realize that you, along with others in the Linux community, are less
 than satisfied with the changeset to provide out-of-the-box support for
 ECC algorithms.

 As I mentioned earlier, we were quite constrained in what we could
 openly discuss before we pushed. However, now that we have pushed I
 am eager to fix any problems that I've introduced.


 Yes, I can understand that to an extent, but I find it hard to believe
 that you had to push it before it could even be discussed.  Why could
 the same patch that was pushed not have been posted for public review
 instead?

 This seems to be a more general issue.  This is endemic behaviour that
 I've seen from the majority of Sun engineers working on OpenJDK (there
 are thankfully some exceptions) and I've blogged about this in more
 detail: http://blog.fuseyism.com/index.php/2009/09/08/im-so-tired/

 We wish to reconcile the conflicting demands of including an ECC
 implementation for platforms without underlying ECC support with the
 exclusion of the ECC implementation on platforms with underlying ECC
 support. I'd like to solicit input from security-dev on how best to
 achieve this.


 It's good to hear you're open to changing this.  There is a third
 option you've missed; the demand of not wanting ECC support at all.
 You'll be aware that there are legal issues from your own discussions
 on this within Sun, and the change in direction that occurred.  Not
 having ECC support needs to be an option as well.

 The existing ECC implementation already fulfilled two of these
 demands; it could be enabled on platforms with ECC support but this
 wasn't the default case.  We can make this easier with IcedTea by
 detecting NSS at build time and auto-generating the configuration if
 the user wishes.  This also can be used to ship it 'out of the box' on
 distributions if required; all the distro packager has to do is build
 IcedTea with NSS support enabled and then make their binary depend on
 it.

 So the real problem here is that Sun's proprietary builds can't ship
 it 'out of the box' because they don't know if the system it ends up
 on will have NSS and, even if it does, where it will be located.  I
 can understand how that's a problem that needs to be fixed, but we
 need a way of disabling that.  If the PKCS11 provider is still
 suitable, then making building the ec directory would actually be
 enough:

 ifndef DISABLE_NSS
  SUBDIRS += ec
 endif

 Job done.  A more complex solution is to link against the system NSS
 instead of the provided C sources.  I've managed to do this with the
 following change:

 diff -r 7a23bfc44c92 make/sun/security/ec/Makefile
 --- a/make/sun/security/ec/Makefile     Tue Sep 08 18:03:43 2009 +0100
 +++ b/make/sun/security/ec/Makefile     Wed Sep 09 23:50:24 2009 +0100
 @@ -153,7 +153,9 @@
   #
   # C and C++ files
   #
 +ifndef USE_SYSTEM_NSS
   include FILES_c.gmk
 +endif

   FILES_cpp = ECC_JNI.cpp

 @@ -185,6 +187,11 @@
     OTHER_LDLIBS += $(JVMLIB)
   else
     OTHER_LDLIBS = -ldl $(JVMLIB) $(LIBCXX)
 +    ifdef USE_SYSTEM_NSS
 +      OTHER_LDLIBS += -Wl,-rpath $(SYSTEM_NSS_DIR) -Wl,-rpath
 $(SYSTEM_NSPR_DIR) \
 +        -L$(SYSTEM_NSS_DIR) -L$(SYSTEM_NSPR_DIR) -lnssutil3 -lnss3 \
 +        -lplds4 -lplc4 -lnspr4 -lsoftokn -lfreebl
 +    endif
   endif

   include $(BUILDDIR)/common/Mapfile-vers.gmk

 but unfortunately, while the resulting sunecc library is dynamically
 linked against NSS, it causes HotSpot to segfault in
 sun.security.ec.ECKeyPairGenerator.generateECKeyPair(I[B[B)[J.  I'm
 still looking into this, I assume there is either some mismatch in the
 versions of NSS or local changes in the Sun copy.  As you say, only
 part of the library was imported into OpenJDK; does this mean that the
 JNI code is not using published interfaces for NSS?

 Your proposal to supply an NSS config file for the SunPKCS11 provider
 is one approach but what about platforms where an ECC-enabled NSS is
 not present?



 It's only really an idea that works where we have an autoconf wrapper
 to detect NSS at build time, and which also allows it to be disabled.
 The patch to IcedTea automatically finds out where NSS is installed,
 via pkg-config, and writes the config file based on that.  I don't
 know of a portable way of doing that in OpenJDK's makefiles as
 pkg-config won't be available on all platforms.

 snip...


  * Which version of NSS were these sources pulled from?  Running diff
 -bu on them, and ignoring the additional copyright headers,
  there are still a large number of changes.  I suspect this is
 because the version is older than my system copy (3.12.3); notably my
  testing shows it does not exhibit the bug discussed in

 http://mail.openjdk.java.net/pipermail/security-dev/2009-September/001167.html
 (which
  incidentally is still awaiting review).

 The sources were pulled from OpenSolaris 2009.06.


 Ok, so

[security-dev 01197]: Re: 6840752: Provide out-of-the-box support for ECC algorithms

[Andrew John Hughes]

2009/9/9 Vincent Ryan vincent.r...@sun.com:
 Hello Andrew,

 I realize that you, along with others in the Linux community, are less
 than satisfied with the changeset to provide out-of-the-box support for
 ECC algorithms.

 As I mentioned earlier, we were quite constrained in what we could
 openly discuss before we pushed. However, now that we have pushed I
 am eager to fix any problems that I've introduced.


Yes, I can understand that to an extent, but I find it hard to believe
that you had to push it before it could even be discussed.  Why could
the same patch that was pushed not have been posted for public review
instead?

This seems to be a more general issue.  This is endemic behaviour that
I've seen from the majority of Sun engineers working on OpenJDK (there
are thankfully some exceptions) and I've blogged about this in more
detail: http://blog.fuseyism.com/index.php/2009/09/08/im-so-tired/

 We wish to reconcile the conflicting demands of including an ECC
 implementation for platforms without underlying ECC support with the
 exclusion of the ECC implementation on platforms with underlying ECC
 support. I'd like to solicit input from security-dev on how best to
 achieve this.


It's good to hear you're open to changing this.  There is a third
option you've missed; the demand of not wanting ECC support at all.
You'll be aware that there are legal issues from your own discussions
on this within Sun, and the change in direction that occurred.  Not
having ECC support needs to be an option as well.

The existing ECC implementation already fulfilled two of these
demands; it could be enabled on platforms with ECC support but this
wasn't the default case.  We can make this easier with IcedTea by
detecting NSS at build time and auto-generating the configuration if
the user wishes.  This also can be used to ship it 'out of the box' on
distributions if required; all the distro packager has to do is build
IcedTea with NSS support enabled and then make their binary depend on
it.

So the real problem here is that Sun's proprietary builds can't ship
it 'out of the box' because they don't know if the system it ends up
on will have NSS and, even if it does, where it will be located.  I
can understand how that's a problem that needs to be fixed, but we
need a way of disabling that.  If the PKCS11 provider is still
suitable, then making building the ec directory would actually be
enough:

ifndef DISABLE_NSS
  SUBDIRS += ec
endif

Job done.  A more complex solution is to link against the system NSS
instead of the provided C sources.  I've managed to do this with the
following change:

diff -r 7a23bfc44c92 make/sun/security/ec/Makefile
--- a/make/sun/security/ec/Makefile Tue Sep 08 18:03:43 2009 +0100
+++ b/make/sun/security/ec/Makefile Wed Sep 09 23:50:24 2009 +0100
@@ -153,7 +153,9 @@
   #
   # C and C++ files
   #
+ifndef USE_SYSTEM_NSS
   include FILES_c.gmk
+endif

   FILES_cpp = ECC_JNI.cpp

@@ -185,6 +187,11 @@
 OTHER_LDLIBS += $(JVMLIB)
   else
 OTHER_LDLIBS = -ldl $(JVMLIB) $(LIBCXX)
+ifdef USE_SYSTEM_NSS
+  OTHER_LDLIBS += -Wl,-rpath $(SYSTEM_NSS_DIR) -Wl,-rpath
$(SYSTEM_NSPR_DIR) \
+-L$(SYSTEM_NSS_DIR) -L$(SYSTEM_NSPR_DIR) -lnssutil3 -lnss3 \
+-lplds4 -lplc4 -lnspr4 -lsoftokn -lfreebl
+endif
   endif

   include $(BUILDDIR)/common/Mapfile-vers.gmk

but unfortunately, while the resulting sunecc library is dynamically
linked against NSS, it causes HotSpot to segfault in
sun.security.ec.ECKeyPairGenerator.generateECKeyPair(I[B[B)[J.  I'm
still looking into this, I assume there is either some mismatch in the
versions of NSS or local changes in the Sun copy.  As you say, only
part of the library was imported into OpenJDK; does this mean that the
JNI code is not using published interfaces for NSS?

 Your proposal to supply an NSS config file for the SunPKCS11 provider
 is one approach but what about platforms where an ECC-enabled NSS is
 not present?



It's only really an idea that works where we have an autoconf wrapper
to detect NSS at build time, and which also allows it to be disabled.
The patch to IcedTea automatically finds out where NSS is installed,
via pkg-config, and writes the config file based on that.  I don't
know of a portable way of doing that in OpenJDK's makefiles as
pkg-config won't be available on all platforms.

snip...


  * Which version of NSS were these sources pulled from?  Running diff
 -bu on them, and ignoring the additional copyright headers,
  there are still a large number of changes.  I suspect this is
 because the version is older than my system copy (3.12.3); notably my
  testing shows it does not exhibit the bug discussed in

 http://mail.openjdk.java.net/pipermail/security-dev/2009-September/001167.html
 (which
  incidentally is still awaiting review).

 The sources were pulled from OpenSolaris 2009.06.


Ok, so which version of NSS does that have?


  * Why was a new provider used instead of the existing
 

[security-dev 01171]: Re: [PATCH FOR REVIEW]: 6763530: Fix breakage of NSS-based Elliptic Curve Cryptography in OpenJDK6

[Andrew John Hughes]

2009/9/3 Michael StJohns mstjo...@comcast.net:



 At 03:14 PM 9/2/2009, Andrew John Hughes wrote:
Ok here is a new webrev:

http://cr.openjdk.java.net/~andrew/6763530/webrev.02/

with a slightly revised version of your change (you can't throw a
PKCS11Exception which only takes a long ID from the native code, so I
changed this to an IllegalArgumentException).

 Yeah - when I realized this a while later (when I actually started building 
 the JDK from source) I actually considered changing PKCS11Exception to 
 implement constructors with just a message and with a message and a code.   
 If you throw with just a message the code would get set to CKR_GENERAL_ERROR. 
  If you throw with message and a code, the message for the code would get 
 prepended to the provided message.  That's another topic though.

 This particular error comes under the heading of one that shouldn't happen - 
 we did the explicit encoding so the toByteArray() shouldn't fail.  That's 
 pretty much the definition of a runtime error.  Maybe use the little used 
 PKCS11RuntimeError instead of the IllegalArgumentException?


IllegalArgumentException seems appropriate to me as it's a failure
with one of the arguments to that method (the params).  Assuming
PKCS11RuntimeError is in the same vein as PKCS11Exception, we don't
want to use them as they pertain to the native NSS code which isn't
the result of the fault here.  The main thing is that the IOException
is available via the cause of the exception that actually gets thrown.


 Mike







-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01168]: [PATCH FOR REVIEW]: 6763530: Fix breakage of NSS-based Elliptic Curve Cryptography in OpenJDK6

[Andrew John Hughes]

2009/9/2 Michael StJohns mstjo...@comcast.net:
 At 09:38 PM 9/1/2009, Andrew John Hughes wrote:
2009/9/2 Michael StJohns mstjo...@comcast.net:
  This appears to be related specifically to PKCS11.  Specifically, PKCS11
 v2.20 has some ambiguity of the representation of an EC point (which is
 different in the text than an ASN1 ECPoint).

 This is being clarified in v2.30 with the unencoded point format (e.g.the
 format described in  X9.62, where the first octet indicates the encoding 
 and
 there are either N or 2N octets following)Â  being the expected value, but
 with PKCS11 providers allowed - legacy - to accept either.

 One of the reasons for going that way was how the JDK PKCS11 provider had
 interpreted the issue and implemented its code.

 I don't support this fix - among other things, this fix only deals with 1/2
 of the problem.  The other half is related to encoding the value.  Also,
 changing the code at decodePoint seems further into the stack than needed
 and may affect other uses of that method.


That's really too vague to be of much help in improving the patch.
You seem to be saying little more than 'I don't like it'.

 Sorry about that.  My point was that your patch didn't completely solve the 
 problem and that the point at where you were fixing it could have some bad 
 side effects for anyone calling decodePoint directly.


 There's an existing JDK bug on this coming at it from a different direction
 - 6763530 ... and there may be considerations at

 https://bugzilla.mozilla.org/show_bug.cgi?id=480280


It seems likely that's the NSS change that causes the current failure.
 The fix I submitted here is based on the way this is handle in NSS.
In fact, the code is similar enough to suggest that one was developed
from the other.

 Â that should be looked at.

The JDK bug is not really 'from a different direction', it's reporting
exactly the same error but from a less trivial example (I get the same
failure while trying to create an example key, while this seems to
require specific hardware if I'm reading it correctly).

 Not exactly.  You're using the NSS as a PKCS11 module - this problem would 
 occur with any PKCS11 module that implements EC stuff.


Also see 6779460 which is mostly a duplicate of
 6763530.


The patch on 6779460 seems wrong.  It means that the method will
return a DER-encoded value where it would either have returned an
uncompressed value before or failed.

 My point exactly as I mentioned in the comments.  :-)



 It's probable that the fix I suggested at 6763530Â  (in comments submitted 
 29
 Nov 08) may be a better approach given the NSS fixes.  I believe it will 
 fix
 the keytool problem noted in the original message.


Ok, I can see the logic in the fix and it would appear to work, though
I haven't tested it.
Given the patch was written nine months ago, why has it not been
applied?  If it had, it would have saved me hours having to debug this
same issue again.

 Yup.  I did do a search for PKCS11 related bugs when I encountered the same 
 problem and did find the original error.

Do you have an SCA with Sun? If so, I'll create a webrev based on your
patch and we can finally get this fixed.  Without it, NSS support is
completely broken in OpenJDK6 which makes me wonder why this is a low
priority bug!

 I do have an SCA on file.  Note that the recommendation from the NSS guys was 
 to raise the priority.

 The reason I haven't submitted this is because I submitted a different EC fix 
  https://bugs.openjdk.java.net/show_bug.cgi?id=100048 per the documented 
 process
  and was waiting on progress there before continuing.  I've got a number of 
 EC and PKCS11 related fixes I'd like to submit, but I was trying for a worked 
 example before proceeding.  And then I got busy with some other things...

 Mike





 Mike





 At 04:39 PM 9/1/2009, Joe Darcy wrote:

 Andrew John Hughes wrote:

 2009/8/28 Andrew John Hughes gnu_and...@member.fsf.org:

 In OpenJDK6, the elliptic curve cryptography algorithms are available
 if the PKCS11 provider is configured to point to NSS. See:

 http://blogs.sun.com/andreas/entry/the_java_pkcs_11_provider

 If NSS is configured as specified in this blog, keytool can be used to
 generate a key as follows:

 Hello.

 Allowing keytool and friends to work in more cases if the provider is
 capable seems fine to me.

 Security team, do you have concerns about this patch?

 Thanks,

 -Joe




--
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8




Ok here is a new webrev:

http://cr.openjdk.java.net/~andrew/6763530/webrev.02/

with a slightly revised version of your change (you can't throw a
PKCS11Exception which only takes a long ID from the native code, so I
changed

[security-dev 01165]: Re: PING 1: [PATCH FOR REVIEW]: Elliptic Curve Cryptography in OpenJDK6 with NSS

[Andrew John Hughes]

2009/9/2 Michael StJohns mstjo...@comcast.net:
  This appears to be related specifically to PKCS11.  Specifically, PKCS11
 v2.20 has some ambiguity of the representation of an EC point (which is
 different in the text than an ASN1 ECPoint).

 This is being clarified in v2.30 with the unencoded point format (e.g.the
 format described in  X9.62, where the first octet indicates the encoding and
 there are either N or 2N octets following)  being the expected value, but
 with PKCS11 providers allowed - legacy - to accept either.

 One of the reasons for going that way was how the JDK PKCS11 provider had
 interpreted the issue and implemented its code.

 I don't support this fix - among other things, this fix only deals with 1/2
 of the problem.  The other half is related to encoding the value.  Also,
 changing the code at decodePoint seems further into the stack than needed
 and may affect other uses of that method.


That's really too vague to be of much help in improving the patch.
You seem to be saying little more than 'I don't like it'.

 There's an existing JDK bug on this coming at it from a different direction
 - 6763530 ... and there may be considerations at

 https://bugzilla.mozilla.org/show_bug.cgi?id=480280


It seems likely that's the NSS change that causes the current failure.
 The fix I submitted here is based on the way this is handle in NSS.
In fact, the code is similar enough to suggest that one was developed
from the other.

  that should be looked at.

The JDK bug is not really 'from a different direction', it's reporting
exactly the same error but from a less trivial example (I get the same
failure while trying to create an example key, while this seems to
require specific hardware if I'm reading it correctly).

Also see 6779460 which is mostly a duplicate of
 6763530.


The patch on 6779460 seems wrong.  It means that the method will
return a DER-encoded value where it would either have returned an
uncompressed value before or failed.


 It's probable that the fix I suggested at 6763530  (in comments submitted 29
 Nov 08) may be a better approach given the NSS fixes.  I believe it will fix
 the keytool problem noted in the original message.


Ok, I can see the logic in the fix and it would appear to work, though
I haven't tested it.
Given the patch was written nine months ago, why has it not been
applied?  If it had, it would have saved me hours having to debug this
same issue again.

Do you have an SCA with Sun? If so, I'll create a webrev based on your
patch and we can finally get this fixed.  Without it, NSS support is
completely broken in OpenJDK6 which makes me wonder why this is a low
priority bug!

 Mike





 At 04:39 PM 9/1/2009, Joe Darcy wrote:

 Andrew John Hughes wrote:

 2009/8/28 Andrew John Hughes gnu_and...@member.fsf.org:

 In OpenJDK6, the elliptic curve cryptography algorithms are available
 if the PKCS11 provider is configured to point to NSS. See:

 http://blogs.sun.com/andreas/entry/the_java_pkcs_11_provider

 If NSS is configured as specified in this blog, keytool can be used to
 generate a key as follows:

 Hello.

 Allowing keytool and friends to work in more cases if the provider is
 capable seems fine to me.

 Security team, do you have concerns about this patch?

 Thanks,

 -Joe




-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01140]: Re: 6840752: Provide out-of-the-box support for ECC algorithms

[Andrew John Hughes]

2009/8/28 Max (Weijun) Wang weijun.w...@sun.com:
 On Aug 28, 2009, at 10:17 PM, Andrew John Hughes wrote:

 2009/8/28 Max (Weijun) Wang weijun.w...@sun.com:

 On Aug 28, 2009, at 9:56 AM, Andrew John Hughes wrote:

 2009/8/28 Max (Weijun) Wang weijun.w...@sun.com:

 On Aug 27, 2009, at 9:52 PM, Andrew John Hughes wrote:

 The problem is more the fact that it's an additional copy rather than
 using the system installation, which means it has to be patched for
 bugs and security fixes separately.  For IcedTea, I'll look at
 providing and using the option of using the system NSS and will also
 submit this for review here if there is interest in providing such an
 option.

 Since Java security is already provider based, I guess you can simply
 write
 one provider named NSS and remove all other security.provider.n lines
 in
 jre/lib/security/java.security.

 Max



 Sounds like the JDK6 solution :)

 No, this is the real Java solution. :)


 ?

 I mean if you really want 100% Fedora Crypto Consolidation so that every
 app's crypto call goes to a single library, you need to create your own Java
 security provider to bridge JCA/JCE calls to this library and remove all the
 others.


Yeah I got that, but I think that's a much more long term goal! :)

What I'm immediately concerned about is not having a duplicate copy of
NSS replete with separate bugs in OpenJDK.



 I think the simpler fix is to just provide an option for the calls to
 the native code to use the system library rather than the included
 copy (some of the new files appear to be verbatim copies of files from
 NSS AFAICS).  But I need to look at this in more detail.

 This only redirects native calls to your centralized ones, but JRE
 includes
 a lot of pure Java providers. If they are still listed in the
 java.security
 file, your so called Fedora Crypto Consolidation is not 100% complete.


 It's not mine, and I was merely referencing that as to why using NSS
 for ECC in the end was a good thing.

 OK. But that's a better thing (at least for Fedora).

 Max


 Thanks
 Max


 Thanks,
 --
 Andrew :-)

 Free Java Software Engineer
 Red Hat, Inc. (http://www.redhat.com)

 Support Free Java!
 Contribute to GNU Classpath and the OpenJDK
 http://www.gnu.org/software/classpath
 http://openjdk.java.net

 PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
 Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8





 --
 Andrew :-)

 Free Java Software Engineer
 Red Hat, Inc. (http://www.redhat.com)

 Support Free Java!
 Contribute to GNU Classpath and the OpenJDK
 http://www.gnu.org/software/classpath
 http://openjdk.java.net

 PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
 Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8





-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01130]: [PATCH FOR REVIEW]: Elliptic Curve Cryptography in OpenJDK6 with NSS

[Andrew John Hughes]

In OpenJDK6, the elliptic curve cryptography algorithms are available
if the PKCS11 provider is configured to point to NSS. See:

http://blogs.sun.com/andreas/entry/the_java_pkcs_11_provider

If NSS is configured as specified in this blog, keytool can be used to
generate a key as follows:

$ keytool -v -genkeypair -keyalg EC -keysize 256 -keystore ectest.jks
-storepass test12 -dname CN=ECC Test

With NSS 3.12.3 (the current version), this fails as follows:

java.lang.RuntimeException: Could not parse key values
at 
sun.security.pkcs11.P11Key$P11ECPublicKey.fetchValues(P11Key.java:1028)
at 
sun.security.pkcs11.P11Key$P11ECPublicKey.getEncodedInternal(P11Key.java:1038)
at sun.security.pkcs11.P11Key.getEncoded(P11Key.java:126)
at 
sun.security.x509.CertificateX509Key.encode(CertificateX509Key.java:105)
at sun.security.x509.X509CertInfo.emit(X509CertInfo.java:819)
at sun.security.x509.X509CertInfo.encode(X509CertInfo.java:189)
at sun.security.x509.X509CertImpl.sign(X509CertImpl.java:528)
at sun.security.x509.X509CertImpl.sign(X509CertImpl.java:486)
at 
sun.security.x509.CertAndKeyGen.getSelfCertificate(CertAndKeyGen.java:288)
at sun.security.tools.KeyTool.doGenKeyPair(KeyTool.java:1223)
at sun.security.tools.KeyTool.doCommands(KeyTool.java:827)
at sun.security.tools.KeyTool.run(KeyTool.java:194)
at sun.security.tools.KeyTool.main(KeyTool.java:188)
Caused by: java.io.IOException: Point does not match field size
at sun.security.ec.ECParameters.decodePoint(ECParameters.java:95)
at 
sun.security.pkcs11.P11ECKeyFactory.decodePoint(P11ECKeyFactory.java:78)
at 
sun.security.pkcs11.P11Key$P11ECPublicKey.fetchValues(P11Key.java:1023)
... 12 more

I did a bit of debugging, and the exception is caused by the array
generated by NSS being too large (for a keysize of 256, its size is 67
rather than 65).

Looking at the NSS code, it turns out that it handles a case which the
code in ECParameters.decodePoint doesn't:

   /* special note: We can't just use the first byte to
determine
 * between these 2 cases because both
EC_POINT_FORM_UNCOMPRESSED
 * and SEC_ASN1_OCTET_STRING are 0x04 */

/* handle the non-DER encoded case (UNCOMPRESSED only) */
if (pubKey-u.ec.publicValue.data[0] == EC_POINT_FORM_UNCOMPRESSED
 pubKey-u.ec.publicValue.len == keyLen) {
break; /* key was not DER encoded, no need to unwrap */
}

/* if we ever support compressed, handle it here */

/* handle the encoded case */
if ((pubKey-u.ec.publicValue.data[0] == SEC_ASN1_OCTET_STRING)
 pubKey-u.ec.publicValue.len  keyLen) {
SECItem publicValue;
SECStatus rv;

rv = SEC_QuickDERDecodeItem(arena, publicValue,
 SEC_ASN1_GET(SEC_OctetStringTemplate),
 pubKey-u.ec.publicValue);
/* nope, didn't decode correctly */
if ((rv != SECSuccess)
|| (publicValue.data[0] != EC_POINT_FORM_UNCOMPRESSED)
|| (publicValue.len != keyLen)) {
crv = CKR_ATTRIBUTE_VALUE_INVALID;
break;
}
/* replace our previous with the decoded key */
pubKey-u.ec.publicValue = publicValue;
break;
}

The code in decodePoint, by comparison, assumes that a block beginning
with 0x04 is uncompressed, then throws an exception because the array
is too large.  The fact is that the array returned by NSS is DER
encoded, and this encoding specifies an octet string using a header of
0x04.  Luckily enough, there is a DER encoder in sun.security.util
which can decode this and with the following webrev:

http://cr.openjdk.java.net/~andrew/ec/webrev.01/jdk.patch

we can quite easily support this format.

With the patch applied:

$ keytool -v -genkeypair -keyalg EC -keysize 256 -keystore ectest.jks
-storepass test12 -dname CN=ECC Test
Generating 256 bit EC key pair and self-signed certificate
(SHA1withECDSA) with a validity of 90 days
for: CN=ECC Test
Enter key password for mykey
(RETURN if same as keystore password):
[Storing ectest.jks]

$ keytool -v -list -keystore ectest.jks -storepass test12 -dname CN=ECC Test

Keystore type: JKS
Keystore provider: SUN

Your keystore contains 1 entry

Alias name: mykey
Creation date: 27-Aug-2009
Entry type: PrivateKeyEntry
Certificate chain length: 1
Certificate[1]:
Owner: CN=ECC Test
Issuer: CN=ECC Test
Serial number: 4a97068b
Valid from: Thu Aug 27 23:19:55 BST 2009 until: Wed Nov 25 22:19:55 GMT 2009
Certificate fingerprints:
 MD5:  10:39:9E:CA:11:50:CD:BF:61:BC:16:1F:B2:43:52:E6
 SHA1: F7:8F:80:77:48:51:C6:3B:49:89:28:A8:5E:5F:7C:ED:D1:BD:CF:BE
 

[security-dev 01136]: Re: 6840752: Provide out-of-the-box support for ECC algorithms

[Andrew John Hughes]

2009/8/28 Max (Weijun) Wang weijun.w...@sun.com:

 On Aug 27, 2009, at 9:52 PM, Andrew John Hughes wrote:

 The problem is more the fact that it's an additional copy rather than
 using the system installation, which means it has to be patched for
 bugs and security fixes separately.  For IcedTea, I'll look at
 providing and using the option of using the system NSS and will also
 submit this for review here if there is interest in providing such an
 option.

 Since Java security is already provider based, I guess you can simply write
 one provider named NSS and remove all other security.provider.n lines in
 jre/lib/security/java.security.

 Max



Sounds like the JDK6 solution :)

I think the simpler fix is to just provide an option for the calls to
the native code to use the system library rather than the included
copy (some of the new files appear to be verbatim copies of files from
NSS AFAICS).  But I need to look at this in more detail.

Thanks,
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01114]: Re: hg: jdk7/tl/jdk: 6843995: Added RowsetFactory and Deprecate COMMIT_ON_ACCEPT_CHANGES, make constants final that needed to be.

[Andrew John Hughes]

2009/8/22 Mark Wielaard m...@klomp.org:
 Hi Andrew,

 On Fri, 2009-08-21 at 20:35 +0100, Andrew John Hughes wrote:
 2009/8/21 Mark Wielaard m...@klomp.org:
  On Thu, 2009-08-20 at 15:40 -0700, Mark Reinhold wrote:
  This change was integrated prematurely.  I've rolled it back in the
  jdk7/tl/jdk repository.
 
  If at all possible, please don't do this. It plays havoc with already
  checked out repos and/or automatic clones/backups. You force people to
  recreate their local repos if you purge a commit that was already public
  like this. Could you use a normal hg backout instead in the future? That
  will make sure the integrity of the repo isn't compromised.
 
 It would be much cleaner to just do a commit reversing the patch (mjw,
 is that what you mean by a backout or something else?)

 Yes. hg backout creates a reverse changeset that cancels out an
 earlier commit (but does so in a way that keeps the integrity of
 repository, plus the whole history). See also: hg backout --help

 I really don't like the idea of sanitising the repos to remove all
 mistakes and there's also no guarantee everyone will do this and push
 it in every forest.

  Thanks,
 
  Mark



Ah, so it autocreates what I was talking about.

Cheers,
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01116]: 6840752: Provide out-of-the-box support for ECC algorithms

[Andrew John Hughes]

With this changeset:

http://hg.openjdk.java.net/jdk7/jdk7/jdk/rev/1ff7163fc5f7

the new ECC was added to OpenJDK.  When I first read about this, I'd
assumed we were getting a Java-based implementation.  The final
changeset seem to just be an inclusion of the NSS code into the
OpenJDK codebase, which adds yet another case where a system library
is replicated internally (the others being libjpeg, libpng, zlib, lcms
and probably others I've missed).

Is this correct? Were there local modifications to this code as well?

As seems to be common practice with OpenJDK, this changeset just
appeared with very little, if any, public discussion.
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[security-dev 01107]: Re: hg: jdk7/tl/jdk: 6843995: Added RowsetFactory and Deprecate COMMIT_ON_ACCEPT_CHANGES, make constants final that needed to be.

[Andrew John Hughes]

2009/8/21 Mark Wielaard m...@klomp.org:
 Hi Mark,

 On Thu, 2009-08-20 at 15:40 -0700, Mark Reinhold wrote:
 This change was integrated prematurely.  I've rolled it back in the
 jdk7/tl/jdk repository.

 If at all possible, please don't do this. It plays havoc with already
 checked out repos and/or automatic clones/backups. You force people to
 recreate their local repos if you purge a commit that was already public
 like this. Could you use a normal hg backout instead in the future? That
 will make sure the integrity of the repo isn't compromised.


It would be much cleaner to just do a commit reversing the patch (mjw,
is that what you mean by a backout or something else?)

I really don't like the idea of sanitising the repos to remove all
mistakes and there's also no guarantee everyone will do this and push
it in every forest.

 Thanks,

 Mark





-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8