Closed Bug 1227795 Opened 5 years ago Closed 5 years ago

Add NSS_DISABLE_LIBPKIX to allow compiling without libpkix

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

(firefox45 affected)

RESOLVED FIXED
Tracking Status
firefox45 --- affected

People

(Reporter: kbrosnan, Assigned: esawin)

References

Details

Attachments

(4 files, 7 obsolete files)

434.80 KB, text/plain
Details
9.99 KB, patch
Details | Diff | Splinter Review
1.13 KB, patch
Details | Diff | Splinter Review
3.31 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
Was chatting with keeler and he mentioned that we are still building & shipping the old cert verification library. Could be a few to several hundred KB (uncompressed?). 

Putting this in Android for now to do the investigation of how much of a win this is.
Eugen can you investigate this. Thanks.
Assignee: nobody → esawin
tracking-fennec: --- → ?
Flags: needinfo?(esawin)
Eugen hasn't found anything that would prevent building the NSS libpkix. David, is that what you were referring to here? What should we be doing?
Flags: needinfo?(esawin) → needinfo?(dkeeler)
Barring any unforeseen dependencies, all of the code that's in security/nss/lib/libpkix is basically dead to the platform. It's part of NSS, though, so we can't easily remove it. There might already exist (or we might be able to add) a compile flag or #define or something so we can skip building it.
Flags: needinfo?(dkeeler)
tracking-fennec: ? → 46+
Summary: investigate if we are still building the old certificate verification library (not mozilla::pkix or insanity::pkix) → Remove the old certificate verification library (not mozilla::pkix or insanity::pkix)
WIP: this patch prevents building libpkix without breaking builds [1] and rips out all code references to it. Next step would be to review the required code-related changes in this patch and plant substitutes where required.
David, can you have a look at this?

As you can see from this patch, adding a libpkix build flag would create a bit of a mess, is there a benefit for keeping libpkix around at all?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a0611770a7b
Attachment #8704181 - Flags: feedback?(dkeeler)
For the purposes of Firefox and related products, there's no reason to keep libpkix around. However, since it's part of NSS (which has much more strict backwards-compatibility requirements) it's unlikely we'll be able to remove it in a way that will be useful. I can certainly bring it up at the next meeting I'm at, but I'm pretty sure that's what the response will be. If that's the case, we can either maintain a mozilla-specific patch to NSS that removes it (which isn't a very desirable prospect) or we can go the build flag route (the reason I think this isn't as bad is the majority of the changes will be in NSS, not mozilla-central, so as long as it works we don't really have to worry about it).
Attachment #8704181 - Flags: feedback?(dkeeler) → feedback+
This patch puts building libpkix behind the BUILD_LIBPKIX flag.

It doesn't touch security/nss/tests/* files, as I'm not sure what's the best approach there would be. As long as libpkix remains in NSS, we can't remove the tests, so do we want to leave the test scripts as they are or make them conditional to the build flag?
Attachment #8704181 - Attachment is obsolete: true
Attachment #8706996 - Flags: review?(dkeeler)
Comment on attachment 8706996 [details] [diff] [review]
0001-Bug-1227795-1.2-Add-BUILD_LIBPKIX-flag-for-condition.patch

Review of attachment 8706996 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but a couple of notes:

* everything but the nss.symbols change will have to be reviewed by an NSS peer (you might file a separate bug for that to keep things from getting confusing)
* the nss.symbols change should be in its own patch (r=me for that)
* it might make sense to switch BUILD_LIBPKIX to NO_BUILD_LIBPKIX or something so that the default behavior is unchanged (or keep the same name but set the default in the appropriate NSS build file)
Attachment #8706996 - Flags: review?(dkeeler)
I don't know why this is tracking 46, this is just something we'd like to do. Not critical.
tracking-fennec: 46+ → ?
tracking-fennec: ? → +
Don't build libpkix iff NO_BUILD_LIBPKIX is defined.

Please redirect r? if you know a better suitable peer.
Attachment #8706996 - Attachment is obsolete: true
Attachment #8723061 - Flags: review?(ttaubert)
Attachment #8723061 - Flags: feedback?(dkeeler)
Disable building libpkix.
Attachment #8723062 - Flags: review?(dkeeler)
Comment on attachment 8723061 [details] [diff] [review]
0001-Bug-1227795-1.3-Add-NO_BUILD_LIBPKIX-build-flag.patch

Review of attachment 8723061 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. To check this in to NSS, you'll have to prepare a patch against the NSS repo itself: https://hg.mozilla.org/projects/nss/
Also, we'll probably want to modify CERT_SetUsePKIXForValidation and CERT_PKIXVerifyCert to return SECFailure (and call PORT_SetError(PR_NOT_IMPLEMENTED_ERROR)) when NO_BUILD_LIBPKIX is defined.
From a process standpoint, we'll probably want to do this in two bugs: one NSS bug will add the build flag (this patch) and the other (a Core :: Security: PSM bug) can set the flag.
A non-moco employee NSS peer should also probably review this patch (maybe Wan-Teh?)

::: security/nss/lib/certhigh/manifest.mn
@@ +27,5 @@
> +	$(NULL)
> +
> +ifndef NO_BUILD_LIBPKIX
> +CSRCS += \
> +	certvfypkix.c \

Hmmm - since this file implements some publicly-exposed functions, I think we'll want to actually compile it, but have any public functions just return SECFailure/PRFalse/whatever's appropriate without actually doing anything.
Attachment #8723061 - Flags: feedback?(dkeeler) → feedback+
Comment on attachment 8723062 [details] [diff] [review]
0002-Bug-1227795-2.1-Disable-building-libpkix.-r-keeler.patch

Review of attachment 8723062 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but I think a build peer should also sign off on this.
Attachment #8723062 - Flags: review?(dkeeler) → review+
Comment on attachment 8723062 [details] [diff] [review]
0002-Bug-1227795-2.1-Disable-building-libpkix.-r-keeler.patch

Review of attachment 8723062 [details] [diff] [review]:
-----------------------------------------------------------------

I think ted is the best build peer for this.
Attachment #8723062 - Flags: review?(ted)
Comment on attachment 8723062 [details] [diff] [review]
0002-Bug-1227795-2.1-Disable-building-libpkix.-r-keeler.patch

Review of attachment 8723062 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few quibbles.

::: config/external/nss/Makefile.in
@@ +310,5 @@
> +ifdef NO_BUILD_LIBPKIX
> +DEFAULT_GMAKE_FLAGS += NO_BUILD_LIBPKIX=1
> +else
> +NSS_DIRS += nss/lib/libpkix
> +endif

I think you should make this non-conditional. It's not a very large patch to back out if it causes problems.

::: configure.in
@@ +9101,5 @@
>  AC_DEFINE(NO_NSPR_10_SUPPORT)
>  
> +# Don't build NSS libpkix
> +NO_BUILD_LIBPKIX=1
> +AC_SUBST(NO_BUILD_LIBPKIX)

Is there a reason to make this an AC_SUBST? Doesn't seem very valuable.
Attachment #8723062 - Flags: review?(ted)
Comment on attachment 8723061 [details] [diff] [review]
0001-Bug-1227795-1.3-Add-NO_BUILD_LIBPKIX-build-flag.patch

Review of attachment 8723061 [details] [diff] [review]:
-----------------------------------------------------------------

What David said. (FWIW, I did something similar in bug 1246928.)
Attachment #8723061 - Flags: review?(ttaubert)
Nit: I think it would be good to update the bug summary, it doesn't quite reflect what's being worked on.
Patch up for review: https://codereview.appspot.com/300000043/
tracking-fennec: + → ---
Component: General → Libraries
OS: Unspecified → All
Product: Firefox for Android → NSS
Hardware: Unspecified → All
Summary: Remove the old certificate verification library (not mozilla::pkix or insanity::pkix) → Add NSS_DISABLE_LIBPKIX to allow compiling without libpkix
Version: Trunk → trunk
Updating the NSS patch here for reference.
Attachment #8723061 - Attachment is obsolete: true
Blocks: 1272693
Comment on attachment 8723062 [details] [diff] [review]
0002-Bug-1227795-2.1-Disable-building-libpkix.-r-keeler.patch

Moving patch 2 to bug 1272693.
Attachment #8723062 - Attachment is obsolete: true
Comments are on rietveld. but it seems like this is failing OCSP stapling tests.
Addressed comments.
Attachment #8752225 - Attachment is obsolete: true
hm, NSS doesn't even compile with NSS_DISABLE_LIBPKIX=1 and this patch applied

Undefined symbols for architecture x86_64:
  "_CERT_GetClassicOCSPDisabledPolicy", referenced from:
     -exported_symbol[s_list] command line option
  "_CERT_GetClassicOCSPEnabledHardFailurePolicy", referenced from:
     -exported_symbol[s_list] command line option
  "_CERT_GetClassicOCSPEnabledSoftFailurePolicy", referenced from:
     -exported_symbol[s_list] command line option

You'll have to remove those symbols from nss.def if NSS_DISABLE_LIBPKIX is set.
Flags: needinfo?(esawin)
It builds for me on Linux x86_64.
Flags: needinfo?(esawin)
Sorry, CERT_GetClassicOCSPDisabledPolicy is in fact being removed by one of the blocks.
I'm not sure why that's not getting picked up by my local build.
(In reply to Eugen Sawin [:esawin] from comment #26)
> Sorry, CERT_GetClassicOCSPDisabledPolicy is in fact being removed by one of
> the blocks.
> I'm not sure why that's not getting picked up by my local build.

I see two possible solutions to this. i) create a new export list (.def) that doesn't contain those functions and use that one if NSS gets compiled with NSS_DISABLE_LIBPKIX=1 (this is not really nice). ii) remove those functions from the export list if compiled with NSS_DISABLE_LIBPKIX=1. We preprocess all .def files anyway to make them compatible with each platform [1]. Here we could strip functions.
Both solutions are a bit hacky, but that's the best I can think of right now.
An alternative would also be to keep those functions and only assert if they are used in the NSS_DISABLE_LIBPKIX=1 case, i.e. don't remove them but leave an essentially empty stub there.

[1] https://dxr.mozilla.org/nss/rev/84e4d119e86229a853dde246be525fc3562045d3/nspr/config/rules.mk#390
Flags: needinfo?(esawin)
Thanks Franziskus. I've addressed your and wtc1's comments.
I've chosen option iii, leaving the function stubs and asserting when used as that's in line with what we do in the rest of the patch.

Updated patch is on https://codereview.appspot.com/300000043/#ps40001 (and here).
Attachment #8762950 - Attachment is obsolete: true
Flags: needinfo?(esawin)
Sorry for being the bearer of bad news again, but it doesn't build with opt (BUILD_OPT=1). Not sure why though.

> make: *** No rule to make target ../../../dist/Linux4.5_x86_64_cc_glibc_PTH_64_OPT.OBJ/lib/libpkixtop.a.  Stop.
> make[2]: *** [../../coreconf/rules.mk:882: ../../../dist/Linux4.5_x86_64_cc_glibc_PTH_64_OPT.OBJ/lib/libpkixtop.a] Error 1
Flags: needinfo?(esawin)
Thanks again, Franziskus. Apparently neither a clean (make clean) build nor building Firefox on try could catch this build issue.
To reproduce it locally, I had to delete all object files in ../nspr/ and ../dist before rebuilding.

Here is part 2: https://codereview.appspot.com/300000043/#ps80001

I can merge them once reviewed.
Flags: needinfo?(esawin)
Thanks Eugen, that patch's looking good.

I'll have a look at the tests today as this patch (as to expect) breaks some. This probably won't break Firefox tests but we have to make sure that we have NSS CI running with NSS_DISABLE_LIBPKIX as well.
Priority: -- → P1
Target Milestone: --- → 3.26
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #32)
> I'll have a look at the tests today as this patch (as to expect) breaks
> some. This probably won't break Firefox tests but we have to make sure that
> we have NSS CI running with NSS_DISABLE_LIBPKIX as well.

I can prepare a patch to add these builds to Taskcluster, you could push the two together then. Or we push them one after the other, doesn't matter too much.
disabling ocsp stapling and libpkix tests; adding builds with NSS_DISABLE_LIBPKIX to TC
Attachment #8764869 - Flags: review?(ttaubert)
Comment on attachment 8764869 [details] [diff] [review]
remove-libpkix-tests.patch

Review of attachment 8764869 [details] [diff] [review]:
-----------------------------------------------------------------

The changes to the shell scripts look good, but I think we should create a new platform for the time being, and run all tests with the new build flag flipped on.
Attachment #8764869 - Flags: review?(ttaubert) → feedback+
Comment on attachment 8764869 [details] [diff] [review]
remove-libpkix-tests.patch

Or... let's start with that. We can extend the tests later.
Attachment #8764869 - Flags: feedback+ → review+
You need to log in before you can comment on or make changes to this bug.