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
