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 which version of NSS does that have? > >> >>> * Why was a new provider used instead of the existing >>> sun.security.pkcs11.SunPKCS11 provider? I noticed this has not be >>> removed, yet >>> it appears to provide duplicate functionality unless I'm mistaken. >>> This does perhaps mean we could fix the issues with this changeset >>> simply >>> by allowing the ec subdirectory to be turned off, but there may be >>> something about the new provider I'm missing. >> >> We introduced the new SunEC provider because we wanted a fast compact >> ECC implementation that we could ship on all platforms. We have not >> bundled all of NSS - only its ECC implementation. >> > > Yeah I noticed that. I suppose the big question is how interchangable > are SunEC and PKCS11? Could we just turn off SunEC, given we already > have NSS support via PKCS11? If so, just making SunEC optional would > solve this IMO. > >> >> >>> * I notice that a number of algorithms are replaced with NULL. I >>> assume there is some (perhaps legal) reason for this? >> >> Which ones? >> > > This is the change I'm referring to: > > /* mapping between ECCurveName enum and pointers to ECCurveParams */ > static const ECCurveParams *ecCurve_map[] = { > NULL, /* ECCurve_noName */ > - &ecCurve_NIST_P192, /* ECCurve_NIST_P192 */ > - &ecCurve_NIST_P224, /* ECCurve_NIST_P224 */ > + NULL, /* ECCurve_NIST_P192 */ > + NULL, /* ECCurve_NIST_P224 */ > &ecCurve_NIST_P256, /* ECCurve_NIST_P256 */ > &ecCurve_NIST_P384, /* ECCurve_NIST_P384 */ > &ecCurve_NIST_P521, /* ECCurve_NIST_P521 */ > - &ecCurve_NIST_K163, /* ECCurve_NIST_K163 */ > - &ecCurve_NIST_B163, /* ECCurve_NIST_B163 */ > - &ecCurve_NIST_K233, /* ECCurve_NIST_K233 */ > - &ecCurve_NIST_B233, /* ECCurve_NIST_B233 */ > - &ecCurve_NIST_K283, /* ECCurve_NIST_K283 */ > - &ecCurve_NIST_B283, /* ECCurve_NIST_B283 */ > - &ecCurve_NIST_K409, /* ECCurve_NIST_K409 */ > - &ecCurve_NIST_B409, /* ECCurve_NIST_B409 */ > - &ecCurve_NIST_K571, /* ECCurve_NIST_K571 */ > - &ecCurve_NIST_B571, /* ECCurve_NIST_B571 */ > - &ecCurve_X9_62_PRIME_192V2, /* ECCurve_X9_62_PRIME_192V2 */ > - &ecCurve_X9_62_PRIME_192V3, /* ECCurve_X9_62_PRIME_192V3 */ > - &ecCurve_X9_62_PRIME_239V1, /* ECCurve_X9_62_PRIME_239V1 */ > - &ecCurve_X9_62_PRIME_239V2, /* ECCurve_X9_62_PRIME_239V2 */ > - &ecCurve_X9_62_PRIME_239V3, /* ECCurve_X9_62_PRIME_239V3 */ > - &ecCurve_X9_62_CHAR2_PNB163V1, /* ECCurve_X9_62_CHAR2_PNB163V1 */ > - &ecCurve_X9_62_CHAR2_PNB163V2, /* ECCurve_X9_62_CHAR2_PNB163V2 */ > - &ecCurve_X9_62_CHAR2_PNB163V3, /* ECCurve_X9_62_CHAR2_PNB163V3 */ > - &ecCurve_X9_62_CHAR2_PNB176V1, /* ECCurve_X9_62_CHAR2_PNB176V1 */ > - &ecCurve_X9_62_CHAR2_TNB191V1, /* ECCurve_X9_62_CHAR2_TNB191V1 */ > - &ecCurve_X9_62_CHAR2_TNB191V2, /* ECCurve_X9_62_CHAR2_TNB191V2 */ > - &ecCurve_X9_62_CHAR2_TNB191V3, /* ECCurve_X9_62_CHAR2_TNB191V3 */ > - &ecCurve_X9_62_CHAR2_PNB208W1, /* ECCurve_X9_62_CHAR2_PNB208W1 */ > - &ecCurve_X9_62_CHAR2_TNB239V1, /* ECCurve_X9_62_CHAR2_TNB239V1 */ > - &ecCurve_X9_62_CHAR2_TNB239V2, /* ECCurve_X9_62_CHAR2_TNB239V2 */ > - &ecCurve_X9_62_CHAR2_TNB239V3, /* ECCurve_X9_62_CHAR2_TNB239V3 */ > - &ecCurve_X9_62_CHAR2_PNB272W1, /* ECCurve_X9_62_CHAR2_PNB272W1 */ > - &ecCurve_X9_62_CHAR2_PNB304W1, /* ECCurve_X9_62_CHAR2_PNB304W1 */ > - &ecCurve_X9_62_CHAR2_TNB359V1, /* ECCurve_X9_62_CHAR2_TNB359V1 */ > - &ecCurve_X9_62_CHAR2_PNB368W1, /* ECCurve_X9_62_CHAR2_PNB368W1 */ > - &ecCurve_X9_62_CHAR2_TNB431R1, /* ECCurve_X9_62_CHAR2_TNB431R1 */ > - &ecCurve_SECG_PRIME_112R1, /* ECCurve_SECG_PRIME_112R1 */ > - &ecCurve_SECG_PRIME_112R2, /* ECCurve_SECG_PRIME_112R2 */ > - &ecCurve_SECG_PRIME_128R1, /* ECCurve_SECG_PRIME_128R1 */ > - &ecCurve_SECG_PRIME_128R2, /* ECCurve_SECG_PRIME_128R2 */ > - &ecCurve_SECG_PRIME_160K1, /* ECCurve_SECG_PRIME_160K1 */ > - &ecCurve_SECG_PRIME_160R1, /* ECCurve_SECG_PRIME_160R1 */ > - &ecCurve_SECG_PRIME_160R2, /* ECCurve_SECG_PRIME_160R2 */ > - &ecCurve_SECG_PRIME_192K1, /* ECCurve_SECG_PRIME_192K1 */ > - &ecCurve_SECG_PRIME_224K1, /* ECCurve_SECG_PRIME_224K1 */ > - &ecCurve_SECG_PRIME_256K1, /* ECCurve_SECG_PRIME_256K1 */ > - &ecCurve_SECG_CHAR2_113R1, /* ECCurve_SECG_CHAR2_113R1 */ > - &ecCurve_SECG_CHAR2_113R2, /* ECCurve_SECG_CHAR2_113R2 */ > - &ecCurve_SECG_CHAR2_131R1, /* ECCurve_SECG_CHAR2_131R1 */ > - &ecCurve_SECG_CHAR2_131R2, /* ECCurve_SECG_CHAR2_131R2 */ > - &ecCurve_SECG_CHAR2_163R1, /* ECCurve_SECG_CHAR2_163R1 */ > - &ecCurve_SECG_CHAR2_193R1, /* ECCurve_SECG_CHAR2_193R1 */ > - &ecCurve_SECG_CHAR2_193R2, /* ECCurve_SECG_CHAR2_193R2 */ > - &ecCurve_SECG_CHAR2_239K1, /* ECCurve_SECG_CHAR2_239K1 */ > - &ecCurve_WTLS_1, /* ECCurve_WTLS_1 */ > - &ecCurve_WTLS_8, /* ECCurve_WTLS_8 */ > - &ecCurve_WTLS_9, /* ECCurve_WTLS_9 */ > + NULL, /* ECCurve_NIST_K163 */ > + NULL, /* ECCurve_NIST_B163 */ > + NULL, /* ECCurve_NIST_K233 */ > + NULL, /* ECCurve_NIST_B233 */ > + NULL, /* ECCurve_NIST_K283 */ > + NULL, /* ECCurve_NIST_B283 */ > + NULL, /* ECCurve_NIST_K409 */ > + NULL, /* ECCurve_NIST_B409 */ > + NULL, /* ECCurve_NIST_K571 */ > + NULL, /* ECCurve_NIST_B571 */ > + NULL, /* ECCurve_X9_62_PRIME_192V2 */ > + NULL, /* ECCurve_X9_62_PRIME_192V3 */ > + NULL, /* ECCurve_X9_62_PRIME_239V1 */ > + NULL, /* ECCurve_X9_62_PRIME_239V2 */ > + NULL, /* ECCurve_X9_62_PRIME_239V3 */ > + NULL, /* ECCurve_X9_62_CHAR2_PNB163V1 */ > + NULL, /* ECCurve_X9_62_CHAR2_PNB163V2 */ > + NULL, /* ECCurve_X9_62_CHAR2_PNB163V3 */ > + NULL, /* ECCurve_X9_62_CHAR2_PNB176V1 */ > + NULL, /* ECCurve_X9_62_CHAR2_TNB191V1 */ > + NULL, /* ECCurve_X9_62_CHAR2_TNB191V2 */ > + NULL, /* ECCurve_X9_62_CHAR2_TNB191V3 */ > + NULL, /* ECCurve_X9_62_CHAR2_PNB208W1 */ > + NULL, /* ECCurve_X9_62_CHAR2_TNB239V1 */ > + NULL, /* ECCurve_X9_62_CHAR2_TNB239V2 */ > + NULL, /* ECCurve_X9_62_CHAR2_TNB239V3 */ > + NULL, /* ECCurve_X9_62_CHAR2_PNB272W1 */ > + NULL, /* ECCurve_X9_62_CHAR2_PNB304W1 */ > + NULL, /* ECCurve_X9_62_CHAR2_TNB359V1 */ > + NULL, /* ECCurve_X9_62_CHAR2_PNB368W1 */ > + NULL, /* ECCurve_X9_62_CHAR2_TNB431R1 */ > + NULL, /* ECCurve_SECG_PRIME_112R1 */ > + NULL, /* ECCurve_SECG_PRIME_112R2 */ > + NULL, /* ECCurve_SECG_PRIME_128R1 */ > + NULL, /* ECCurve_SECG_PRIME_128R2 */ > + NULL, /* ECCurve_SECG_PRIME_160K1 */ > + NULL, /* ECCurve_SECG_PRIME_160R1 */ > + NULL, /* ECCurve_SECG_PRIME_160R2 */ > + NULL, /* ECCurve_SECG_PRIME_192K1 */ > + NULL, /* ECCurve_SECG_PRIME_224K1 */ > + NULL, /* ECCurve_SECG_PRIME_256K1 */ > + NULL, /* ECCurve_SECG_CHAR2_113R1 */ > + NULL, /* ECCurve_SECG_CHAR2_113R2 */ > + NULL, /* ECCurve_SECG_CHAR2_131R1 */ > + NULL, /* ECCurve_SECG_CHAR2_131R2 */ > + NULL, /* ECCurve_SECG_CHAR2_163R1 */ > + NULL, /* ECCurve_SECG_CHAR2_193R1 */ > + NULL, /* ECCurve_SECG_CHAR2_193R2 */ > + NULL, /* ECCurve_SECG_CHAR2_239K1 */ > + NULL, /* ECCurve_WTLS_1 */ > + NULL, /* ECCurve_WTLS_8 */ > + NULL, /* ECCurve_WTLS_9 */ > NULL /* ECCurve_pastLastCurve */ > }; > > > It could be a NSS version issue, but seems more deliberate to me. > It leaves three curves: > &ecCurve_NIST_P256, /* ECCurve_NIST_P256 */ > &ecCurve_NIST_P384, /* ECCurve_NIST_P384 */ > &ecCurve_NIST_P521, /* ECCurve_NIST_P521 */ > >> >>> >>> I'm afraid my current impression of this changeset is that it doesn't >>> help us with packaging OpenJDK for GNU/Linux distributions at all, but >>> instead makes things ten times worse as there is now a stale NSS to >>> contend with. Not only are there the issues with bit rot I alluded to >>> last time, but as you mention in your reply there are legal issues >>> with EC support. Notably, I've found that Fedora doesn't ship any EC >>> support (https://bugzilla.redhat.com/show_bug.cgi?id=492124) so we'd >>> have to rip this out in packages for that distribution (and it's >>> dubious whether others should be shipping it). IANAL, so I won't >>> comment further on such issues, but suffice to say this changeset >>> significantly reduces the options for handling NSS support downstream. >>> In contrast, the existing support in 1.6 is almost ideal, once you've >>> discovered how it works; the distro packager can choose whether to >>> enable support or not and they don't have to worry about rotting >>> security code in OpenJDK. Maybe I'm missing something but this makes >>> me think this would have been better as a local addition to Sun's >>> proprietary builds rather than adding it to OpenJDK. >>> >>> I try to be as positive as I can about the OpenJDK project, but I'm >>> sorry to say that changesets like this don't help. I actually find >>> them quite depressing. As I said in my previous email, there appears >>> to have been no discussion of this change; it was merely committed >>> with no public review and appeared in b70. Meanwhile, myself and >>> other external contributors have to spend days trying to get replies >>> to emails to even get a simple bug fix in (I've lost count of how many >>> I still have waiting; there must be at least four or five). That's >>> just not fair and doesn't bode well for a successful community >>> project. >>> >>> Thanks, >> >> > > 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 >
I've added this changeset: http://hg.openjdk.java.net/icedtea/jdk7/jdk/rev/2a1a7fb44226 to the IcedTea project's JDK7 forest to solve this issue. If it looks ok, then give me a bug ID and I'll push it to tl-gate. -- 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
