I don’t see your testcase in the webrev can you add it in and re-generate. Thanks.
> On 11 Nov 2015, at 12:08, Langer, Christoph <christoph.lan...@sap.com> wrote: > > Hi Vincent, > > so this is the final version which should be pushed: > http://cr.openjdk.java.net/~clanger/webrevs/8139436.2/ > <http://cr.openjdk.java.net/~clanger/webrevs/8139436.2/> > > I removed an unnecessary blank in a method call and updated the copyright > information. I also ran another build and verified with my test case. > > Thanks in advance for taking care. > Christoph > > From: Vincent Ryan [mailto:vincent.x.r...@oracle.com > <mailto:vincent.x.r...@oracle.com>] > Sent: Mittwoch, 11. November 2015 08:46 > To: Mike StJohns <mstjo...@comcast.net <mailto:mstjo...@comcast.net>>; > Langer, Christoph <christoph.lan...@sap.com <mailto:christoph.lan...@sap.com>> > Cc: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net> > Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete > data > > Explicitly referencing the Sun JCE provider makes the jdk.crypto.mscapi > module depend on the java.base module. > And that’s OK. > > I can sponsor this changeset. > > > On 10 Nov 2015, at 22:39, Mike StJohns <mstjo...@comcast.net > <mailto:mstjo...@comcast.net>> wrote: > > On 11/10/2015 4:12 PM, Langer, Christoph wrote: > Hi folks, > > is there any feedback/review for this change? > > I missed the 4 Nov message when it came past. No further objections. But > still unclear if modularization will permit this. > > Mike > > > > Thanks > Christoph > > From: Langer, Christoph > Sent: Mittwoch, 4. November 2015 12:11 > To: 'Michael StJohns' <mstjo...@comcast.net> <mailto:mstjo...@comcast.net> > Cc: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net> > Subject: RE: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete > data > > Hi Mike, > > eventually I’ve made a new webrev implementing your suggestion for a simpler > patch: > > http://cr.openjdk.java.net/~clanger/webrevs/8139436.1/ > <http://cr.openjdk.java.net/~clanger/webrevs/8139436.1/> > > @All: Is that a valid fix for this issue? Would anyone mind to review/sponsor > this? > > Thanks > Christoph > > > From: Michael StJohns [mailto:mstjo...@comcast.net > <mailto:mstjo...@comcast.net>] > Sent: Donnerstag, 15. Oktober 2015 02:09 > To: Langer, Christoph <christoph.lan...@sap.com > <mailto:christoph.lan...@sap.com>> > Cc: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net> > Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete > data > > Sorry for the top post, I'm wring this on an iPad on a plane. > > > It's perfectly acceptable for a provider to prefer its version of stuff over > even something early in the provider list. Usually, that has more to do > with key material ( for example pkcs11 instantiations), but there's no reason > why it shouldn't apply to certificates. > > Mike > > > > Sent from my iPad > > On Oct 14, 2015, at 18:35, Langer, Christoph <christoph.lan...@sap.com > <mailto:christoph.lan...@sap.com>> wrote: > > Hi Mike, > > thanks for your comments on this. > > I agree that the change is quite critical and my approach might not be good. > However, as for your suggestion to specify the sun provider for the > certificate factory, I’d say this is not quite right, too. Because, if you > have loaded a different provider on top of the providers list, you’ll intend > to use its facilities by default. Maybe the idea would be to use the sun > provider as fallback in case of an exception? > > The other suggestion to check for certChain.length in KeyStore.getCertificate > and return null would be an improvement compared to throwing an > ArrayIndexOutOfBoundsException – however, it would still be difficult to find > the root cause why a certificate could not be loaded. > > What do you think? > > @all: Are there other opinions? > > Thanks and best regards > Christoph > > From: security-dev [mailto:security-dev-boun...@openjdk.java.net > <mailto:security-dev-boun...@openjdk.java.net>] On Behalf Of Mike StJohns > Sent: Mittwoch, 14. Oktober 2015 02:17 > To: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net> > Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete > data > > Hi - > > I took a look and this probably isn't the correct way to fix this. > > A simpler change might be to specify the sun provider when requesting the > certificate factory. I hesitate to say that definitively as modularization > guidance may restrict that approach? > > The belt and suspenders approach is to catch the bad certificate exception > and return null. That appears to be the correct contract for > KeyStore.getCertificate(String alias). (e.g. "if (certChain.length == 0) > return null;") > > Mike > > > On 10/12/2015 5:04 PM, Langer, Christoph wrote: > Hi, > > please review a change proposal regarding an issue in the Microsoft Security > API (mscapi). > > Bug: https://bugs.openjdk.java.net/browse/JDK-8139436 > <https://bugs.openjdk.java.net/browse/JDK-8139436> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8139436.0/ > <http://cr.openjdk.java.net/~clanger/webrevs/8139436.0/> > > I stumbled over the issue when using an old IAIK security provider. It would > throw java.security.cert.CertificateException upon parsing ECC based > certificates when generating Certificate objects. The way it is right now, > such exceptions are silently caught and the Windows Keystore is created with > incomplete data. Upon accessing such ECC certificates from the Keystore > object, e.g. when iterating over it, you’ll get exceptions like: > Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 0 > at > sun.security.mscapi.KeyStore.engineGetCertificate(KeyStore.java:313) > at > sun.security.mscapi.KeyStore$ROOT.engineGetCertificate(KeyStore.java:60) > at java.security.KeyStore.getCertificate(KeyStore.java:1095) > At that point it is not obvious what the real root cause for that is. > > With my change, loading of the keystore would already throw like this: > java.io.IOException: java.security.KeyStoreException: Exception occurred > generating certificate object for alias DigiCert Assured ID Root G3 > at sun.security.mscapi.KeyStore.engineLoad(KeyStore.java:780) > at sun.security.mscapi.KeyStore$ROOT.engineLoad(KeyStore.java:60) > at java.security.KeyStore.load(KeyStore.java:1459) > at > WindowsCertificateReaderTest.main(WindowsCertificateReaderTest.java:18) > Caused by: java.security.KeyStoreException: Exception occurred generating > certificate object for alias DigiCert Assured ID Root G3 > at sun.security.mscapi.KeyStore.loadKeysOrCertificateChains(Native > Method) > at sun.security.mscapi.KeyStore.engineLoad(KeyStore.java:777) > ... 3 more > Caused by: java.security.cert.CertificateException: Error parsing > certificates! iaik.asn1.DerInputException: Next ASN.1 object is no OBJECT > IDENTIFIER! > at iaik.x509.CertificateFactory.engineGenerateCertificates(Unknown > Source) > at > java.security.cert.CertificateFactory.generateCertificates(CertificateFactory.java:462) > at sun.security.mscapi.KeyStore.generateCertificate(KeyStore.java:869) > ... 5 more > This is more obvious when it comes to analyzing such an issue. > > Also, I added a property “sun.security.mscapi.ignoreFailingCertificates” > which, when set to true, will cause skipping of certificates that failed with > Exception. That might be a nice workaround option if one is not particularly > interested in a failing certificate. > > You can reproduce all this with the test coding in the OpenJDK Bug, the IAIK > provider 3.15 which is downloadable > here:http://jcewww.iaik.tu-graz.ac.at/sic/Download > <http://jcewww.iaik.tu-graz.ac.at/sic/Download> (educational/research > version, needs registration) and ECC certificates in the Windows Root > certificate store. > > Would you think this change is reasonable and worthwile? > > Thanks & Best regards > Christoph
