Hello again Andrew, Sorry for the delay getting to your request.
Your mechanism to control the inclusion of the SunEC provider looks like a fine solution. I've created the following CR: http://bugs.sun.com/view_bug.do?bug_id=6882745 Andrew John Hughes wrote: > 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.
