308630 - Reduce PSM to only do SSL

Status: RESOLVED WONTFIX

(Core :: Security: CAPS, defect)

Description

[Doug Turner (:dougt)]

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

When defining MINIMO, only build the parts of NSS that are required for basic
SSL support. I was able to reduce the size of the dll (pipnss) from around 300
to a bit over 110k (uncompressed on disk arm binary size).

Comment 2

[Robert Relyea]

Comment on attachment 196128 [details] [diff] [review]
patch v.1

one thing I would like to see is the explicit list of things that don't work in
minimo.

From the code removed, I can see the following does not work:

1)CRL checking.
2)encryption protection of your web passwords (SDR).
3)Almost any for of cert management:
  i) importing and exporting certs (PKCS 12 isn't there).
  ii) acquiring certs from the web (KeyGen).
  iii) the cert manager itself (CertCach, CertDB, etc.).
4) SSL client auth does not appear to work (given that, cert management becomes
less important -- you don't need client certs for a browser).
5)Smart tokens (PKCS #11 devices) will only partially work (no
insertion/removal, and no management of the tokens).

I think we should reevaluate 1, 2, and 4 (particularly 2 and 4). 3 and 5 could
be managed by external applications on the hotsync host (you can acquire,
import and manage certs using a xul app on the host which modifies your PDA's
db and uses the sync to update it.

As far as insertion removal goes, we probably don't need to worry until
bluetooth smart cards are available...

bob

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 196128 [details] [diff] [review]
patch v.1

yes, that is a close list to what I had.  It also disables the crypto js object
and NTLM.

I do not think that either of these are required immediately in Minimo.

As for:

CRL checking -- i do not think we do this in FireFox currently, right?

SDR -- Yes, this might be re-enabled, but right now there isn't a wallet
implementation for Minimi; hence no need.

SSL Client Auth is not supported in Minimo.

thanks for the review.

Comment 4

[Darin Fisher]

Comment on attachment 196128 [details] [diff] [review]
patch v.1

>+ifndef MINIMO

Why is reducing PSM to only do SSL specific to Minimo?

I think you should define a specific build option for
this configuration much like --disable-xul:

> ifdef MOZ_XUL
> CPPSRCS += nsCertTree.cpp
> endif

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 196128 [details] [diff] [review]
patch v.1

kai, what is your take on this?  would MOZ_PSM_LITE be okay with you?

Comment 6

[Kai Engert (:KaiE:)]

> CRL checking -- i do not think we do this in FireFox currently, right?

If the user clicks a link to a CRL, it is being installed into Firefox (actually into NSS' database) and NSS will make use of it.

Comment 7

[Kai Engert (:KaiE:)]

> what is your take on this?  would MOZ_PSM_LITE be okay with you?

I have several thoughts on this.

I think we should NOT hardcode the requirements of a specific application (Minimo) into the general codebase.

You suggest to introduce a term LITE and grant ownership of the LITE term to you and Minimo, and whenever your requirements change, you want us to adjust that mode within the shared codebase.

I would prefer to find an approach that is more general.

So instead of suggesting an application specific #define, I would prefer a patch to selectively compile indivudual functionality.

However, if you want to compile only a subset of the functionality, instead of adding #defines to the code that make it harder to read and more difficult to maintain, ideally PSM should be made more modular, by adding the ability to disable modules to the build system.

You are suggesting an exclusive definition "eveything but minimal SSL", but it's not possible to combine different exclusive definition (maybe some application wants to use secure messaging only, but not SSL).

Instead of using "everything but" definitions, I'd like to see "individual disable" definitions, because those defines can be easily combined.

So what about the following: Split PSM into two separate modules. A "base and SSL module", and an "everything else" module.

Or even three modules, a base module (for the generic NSS library handling), an SSL module (and you add defines for MOZ_PSM_DISABLE_CLIENT_AUTH), and a third module that contains all the rest.

I don't want to cause too much additional work, but I'd prefer it very much if we can find a more general solution, that will work for other applications with other needs, too.

Can we find some compromise?

Comment 8

[Doug Turner (:dougt)]

I would be happy with any solution that saves over the 300k in codesize that I have found with this patch.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 196128 [details] [diff] [review]
patch v.1

I'm not happy with application specific defines.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Patch v.2

This patch disable everything your patch disabled, and maybe a bit more.

It introduces a bunch of MOZ_PSM_DISABLE_* defines.

I have not tested whether these defines work individually. However, If you enable them all, you should get your desired result.

If desired, more work can be invested at a later time to make the individual statements work correctly.

I assume you are able to add the disable defines to the MINIMO build system.

Comment 11

[Kai Engert (:KaiE:)]

Attachment: Patch v.3

Corrected patch.

 MOZ_PSM_DISABLE_CERT_AND_CRL_DOWNLOAD
 MOZ_PSM_DISABLE_CERT_MANAGEMENT
 MOZ_PSM_DISABLE_CIPHER_INFORMATION
 MOZ_PSM_DISABLE_DOM_CRYPTO
 MOZ_PSM_DISABLE_NTLM_AUTH
 MOZ_PSM_DISABLE_PKCS11
 MOZ_PSM_DISABLE_SECRET_DECODER_RING
 MOZ_PSM_DISABLE_SMARTCARD_EXTRAS
 MOZ_PSM_DISABLE_SMIME

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 205003 [details] [diff] [review]
Patch v.3

I think this adds a large amount of flexiblity for an embedder, but this change does increase the module's managablity.

if we go this way, we will have to add the right configure / config fu to make these #ifdef work.

Comment 13

[Kai Engert (:KaiE:)]

> if we go this way, we will have to add the right configure / config fu to make
> these #ifdef work.

You don't think it's as simple as e.g. adding the #ifdef statements to the Minimo mozconfig?

Comment 14

[Doug Turner (:dougt)]

i am not sure.  something for a build guy to tell us.  I think since you have Makefile.in changes, you will have to be touching autoconf.mk.in, but I could be wrong.  In any case, that is a detail....

Are you comfortable with having 9 or so new ifdefs?

Comment 15

[Kai Engert (:KaiE:)]

> Are you comfortable with having 9 or so new ifdefs?

Not perfectly comfortable, but much more comfortable than having application specific defines.

If I understand correctly, this is also what Bob and Darin suggested.

Comment 16

[Kai Engert (:KaiE:)]

> I think since you have Makefile.in changes,

Your patch had changed the file in the same way. I just followed your initial proposal.

Comment 17

[Doug Turner (:dougt)]

futuring.

Comment 18

[Kai Engert (:KaiE:)]

Doug, do you still need this? Can we close the bug? I guess the patch has bitrotted by now.

Comment 19

[Doug Turner (:dougt)]

i do not need it, but others might run into the same thing.  Lets mark this as WONTFIX or future it.

Comment 20

[Kai Engert (:KaiE:)]

Since I do not see related requests, and nobody has commented or added themselves to this bug within the last year, I'm resolving this as WONTFIX, because nobody seems to require it currently.