2009/9/18 Vincent Ryan <vincent.r...@sun.com>: > Hello again Andrew, > > Sorry for the delay getting to your request. >
No problem. > 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 > Thanks. Pushed: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/39c15c0a71f7 > > > > 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. > -- 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
