1227795 - Add NSS_DISABLE_LIBPKIX to allow compiling without libpkix

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kevin Brosnan [Ex-Mozilla]]

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.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Eugen can you investigate this. Thanks.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

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?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1227795-1.1-Remove-libpkix.patch

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

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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).

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1227795-1.2-Add-BUILD_LIBPKIX-flag-for-condition.patch

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?

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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)

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

I don't know why this is tracking 46, this is just something we'd like to do. Not critical.

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1227795-1.3-Add-NO_BUILD_LIBPKIX-build-flag.patch

Don't build libpkix iff NO_BUILD_LIBPKIX is defined.

Please redirect r? if you know a better suitable peer.

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1227795-2.1-Disable-building-libpkix.-r-keeler.patch

Disable building libpkix.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 13

[Nick Alexander :nalexander [he/him]]

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.

Comment 14

[(not currently active) Ted Mielczarek]

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.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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.)

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Nit: I think it would be good to update the bug summary, it doesn't quite reflect what's being worked on.

Comment 17

[Eugen Sawin [:esawin]]

Patch up for review: https://codereview.appspot.com/300000043/

Comment 18

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1227795-1.4-Add-NSS_DISABLE_LIBPKIX-to-allow-com.patch

Updating the NSS patch here for reference.

Comment 19

[Eugen Sawin [:esawin]]

Comment on attachment 8723062 [details] [diff] [review]
0002-Bug-1227795-2.1-Disable-building-libpkix.-r-keeler.patch

Moving patch 2 to bug 1272693.

Comment 20

[Franziskus Kiefer [:franziskus]]

Comments are on rietveld. but it seems like this is failing OCSP stapling tests.

Comment 21

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1227795-1.5-Add-NSS_DISABLE_LIBPKIX-to-allow-com.patch

Addressed comments.

Comment 22

[Franziskus Kiefer [:franziskus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c63ff8cbd84

Comment 23

[Franziskus Kiefer [:franziskus]]

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.

Comment 24

[Eugen Sawin [:esawin]]

Attachment: NSS_DISABLE_LIBPKIX build log

It builds for me on Linux x86_64.

Comment 25

[Eugen Sawin [:esawin]]

CERT_GetClassicOCSPDisabledPolicy isn't being removed by this patch, so maybe rebasing has gone wrong for you, Franziskus?

Comment 26

[Eugen Sawin [: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.

Comment 27

[Franziskus Kiefer [:franziskus]]

(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

Comment 28

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1227795-1.6-Add-NSS_DISABLE_LIBPKIX-to-allow-com.patch

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).

Comment 29

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1227795-1.7-Add-NSS_DISABLE_LIBPKIX-to-allow-com.patch

Fixed some whitespace issues: https://codereview.appspot.com/300000043/#ps60001

Comment 30

[Franziskus Kiefer [:franziskus]]

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

Comment 31

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1227795-2.1-Add-NSS_DISABLE_LIBPKIX-to-allow-com.patch

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.

Comment 32

[Franziskus Kiefer [:franziskus]]

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.

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 34

[Franziskus Kiefer [:franziskus]]

Attachment: remove-libpkix-tests.patch

disabling ocsp stapling and libpkix tests; adding builds with NSS_DISABLE_LIBPKIX to TC

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8764869 [details] [diff] [review]
remove-libpkix-tests.patch

Or... let's start with that. We can extend the tests later.

Comment 37

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/64a69537291a
https://hg.mozilla.org/projects/nss/rev/7e4c251717d4