Closed Bug 305835 Opened 19 years ago Closed 11 years ago

Remove NSS_ENABLE_ECC ifdefs in libssl

Categories

(NSS :: Libraries, defect, P2)

3.10
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: wtc, Assigned: rrelyea)

Details

(Whiteboard: ECC)

Attachments

(4 files, 1 obsolete file)

We should remove NSS_ENABLE_ECC ifdefs wherever possible.
The current plan is to remove all the NSS_ENABLE_ECC ifdefs
outside lib/freebl, lib/softoken, and lib/ssl.
This patch is for mozilla/security/nss/lib.  I will
attach another patch for mozilla/security/nss/cmd later.
Attachment #193765 - Flags: review?(nelson)
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.11
This patch is the mechanical removal of NSS_ENABLE_ECC
ifdefs from mozilla/security/nss/cmd.  It doesn't
remove the ifdefs from bltest, selfserv, strsclnt,
and tstclnt because these tests depend on code lib/freebl
and lib/ssl that will remain ifdef'ed.

This patch is not ready for review because I want to
review the ECC code in nss/cmd first.  It is not
as straightforward as the ECC code enabled by the nss/lib
patch, which merely adds new EC cases to existing switch
statements.  There are at least two things in this patch
I am not sure about.

1. In certutil/certutil.c, I don't understand the new
error message in the last "hunk" for that file:
    if ((keytype != dsaKey) && (keytype != ecKey)) {
	PR_fprintf(PR_STDERR, "%s -q: specifies a PQG file for DSA keys" \
		   " (-k dsa) or a named curve for EC keys (-k ec)\n)",
		   progName);

2. In selfserv/selfserv.c, I don't understand what the new "-e ec_nickname"
option is for.
Hi Wan-Teh,

  My comments are embedded within your note
below.

(In reply to comment #2)

> 1. In certutil/certutil.c, I don't understand the new
> error message in the last "hunk" for that file:
>     if ((keytype != dsaKey) && (keytype != ecKey)) {
>         PR_fprintf(PR_STDERR, "%s -q: specifies a PQG file for DSA keys" \
>                    " (-k dsa) or a named curve for EC keys (-k ec)\n)",
>                    progName);

   The -q option has been expanded so it can now be used
  to specify the ECC parameters for ECC keys (i.e. when the
  key type is "-k ec"). Currently the -q option is used to 
  specify DSA parameters for DSA keys. An example:

   certutil -S -n nickname -k ec -q secp160r1 ...
  
   will generate a self signed CA cert containing a
   EC public key on the curve secp160r1.

> 2. In selfserv/selfserv.c, I don't understand what the new "-e ec_nickname"
> option is for.

    For this one, I'll have to rely on my memory. 
    If I remember correctly, if a host has two different
    certificates, e.g. an RSA and an ECC cert, one needs
    to be able to pass two different nicknames for it.
    This -e option tells the code which nickname to use
    to access the ECC certificate.

    Should this have been done differently?

    I hope this helps,

    vipul
In answer to the question about the -e option:

A single SSL server socket can be configured with multiple certificates, one
for each family of Key Exchange algorithms, e.g. a socket could have an RSA
cert, a DSA cert, an ECDSA cert and (until recently) a fortezza cert.  

IMO, programs like certutil should allow the -n <nickname> option to be
specified more than once in the command line, and for each nickname given,
it should fetch the cert associated with that nickname, figure out what type
it is, and configure the socket for that cert with that type.  

But back when fortezza support was added to selfserv, the nickname for the
fortezza cert was specified with a new option, -f, and selfserv relied on
the command line option, rather than the contents of the cert, to know to
configure the socket with that fortezza cert.  IINM, one consequence of this
approach is that it is possible to misconfigure the server by specifying 
the nickname of a cert with the wrong type.

When ECC support was added to selfserv, it was patterned after the fortezza
code.  IMO, it would be very desirable for someone to change selfserv to
use only the -n option for the specification of all nicknames, and to 
permit the specification of multiple nicknames for certs of multiple types.
Nelson,

I don't believe any additional nickname arguments are needed. The nickname is
really an alias for a subject. Thus, separate nicknames are only needed if we
want the certs for each key type to also have different subjects.

When the server needs to configure its certificates, it can query all the certs
that match the nickname/subject by creating a subject list. Then, it can filter
that list and narrow it down to only certificates that match the required key
type(s).
(In reply to comment #5)
> [...] Then, it can filter that list and narrow it down to only 
> certificates that match the required key type(s).

Where does it get the list of "required key types(s)"?
That's the central issue here, IMO. 

Fortezza's answer was to add the -f option to indicate that fortezza 
key type was required.  ECC adds -e for a similar purpose.  Notice that
these 3 (-n, -f, -e) are not mututally exclusive.  

I think you're proposing that the list include all types of keys/certs
that match the nickname.  Yes?  If that were the case, and the test DB had
multiple certs with different key types and the same subject name (and 
hence the same nickname), how would the selfserv user control which one(s)
of those certs s/he wanted to use?  
would make 

Comment on attachment 193765 [details] [diff] [review]
Patch for nss/lib (checked in)

r=nelson for the unifdeffing (only). 
I did not review for the correctness of the code enabled by the unifdeffing.  
I also didn't check to see if this was complete; that is, I did not check that
no NSS_ENABLE_ECC ifdefs remain after applying this patch. But if we find more,
we can remove them in a subsequent patch.
Attachment #193765 - Flags: review?(nelson) → review+
Comment on attachment 193765 [details] [diff] [review]
Patch for nss/lib (checked in)

I checked in this patch on the NSS trunk for NSS 3.11.
Attachment #193765 - Attachment description: Patch for nss/lib → Patch for nss/lib (checked in)
I reviewed the ECC code in nss/lib that is now
always compiled after the NSS_ENABLE_ECC ifdefs
were removed.  This patch contains the additional
changes I proposed and also my questions.

Vipul, could you look at the "XXX" comments in
this patch and answer my questions?  My questions
are:
1. We set pubk->u.ec.size to 0 when we create a
new EC public key and actually calculate it when
needed.  Are you trying to do lazy evaluation?
I am worried that when multiple threads try to
set pubk->u.ec.size to the actual value at the
same time, they may step on each other.

2. The length of u.ec.publicValue is in bytes,
but it may be converted from and to a subjectPublicKey,
which is a bit string.	Is the length of an EC
subjectPublicKey in bits always a multiple of 8?
If not, we need to save the exact length in bits
in order to convert back to a subjectPublicKey
with the original length in bits.
(In reply to comment #9)

Hi Wan-Teh,

My comments are embedded below.

> Vipul, could you look at the "XXX" comments in
> this patch and answer my questions?  My questions
> are:
> 1. We set pubk->u.ec.size to 0 when we create a
> new EC public key and actually calculate it when
> needed.  Are you trying to do lazy evaluation?
> I am worried that when multiple threads try to
> set pubk->u.ec.size to the actual value at the
> same time, they may step on each other.

   I'm agin relying on memory to answer this one but 
   I think the issue was that the code that parses
   the encoded EC parameters was at a different
   layer in NSS.  I also seem to remember that at 
   one point we did move the decoding code. It
   is now in nss/lib/softoken/ecdecode.c but I haven't
   checked if that now makes it available to seckey.c and
   pk11akey.c.

> 
> 2. The length of u.ec.publicValue is in bytes,
> but it may be converted from and to a subjectPublicKey,
> which is a bit string.  Is the length of an EC
> subjectPublicKey in bits always a multiple of 8?
> If not, we need to save the exact length in bits
> in order to convert back to a subjectPublicKey
> with the original length in bits.

   Key sizes are not always a multiple of 8 and
   the code does the right thing when it is about
   to use a key. However, as I recall, there are
   a few layers where the exact key size info
   isn't available (I think keeping it would have
   required changes to existing structures and
   there was some uncertainty whether making
   the change would break anything, e.g. NSS
   strives for binary API compatibility). In particular,
   one place where this is an issue is the piece of code in NSS
   where the key sizes used for key agreement and
   authentication in an SSL handshake are printed 
   (I am having trouble finding that code but you 
   might know what I'm talking about). There, the
   code only had access to the size in bytes so the
   printed value was incorrect (and different from
   the correct value used, e.g. 163-bit was printed as
   168 if I recall correctly).

   I know this info is very sketchy but I haven't spent
   much time trying to look for the missing info. Let
   me know if you'd like me to (I'm hoping you can find it
   more quickly based on what I mentioned above).

   vipul

   
   
I propose that we not try to complete this until 3.12.
We may have need to continue to be able to build 3.11.x without ECC.
Whiteboard: ECC
Target Milestone: 3.11 → 3.12
Per our meeting, we agreed to remove this ifdefs from libssl. This should be done for 3.11.x so that backports from 3.12 are easier.
Summary: Remove NSS_ENABLE_ECC ifdefs wherever possible → Remove NSS_ENABLE_ECC ifdefs in libssl
Target Milestone: 3.12 → 3.11.1
QA Contact: jason.m.reid → libraries
Attachment #222094 - Flags: superreview?
Attachment #222094 - Flags: review?(wtchang)
OK, this is the patch, the other was the full file...
Attachment #222094 - Attachment is obsolete: true
Attachment #222098 - Flags: superreview?
Attachment #222098 - Flags: review?(wtchang)
Attachment #222094 - Flags: superreview?
Attachment #222094 - Flags: review?(wtchang)
Attachment #222098 - Flags: superreview? → superreview?(nelson)
Comment on attachment 222098 [details] [diff] [review]
patch: Allow unconditional access to the TLS cipher suite numbers even if ECC is turned off

r=wtc.  I also verified that this is the last NSS_ENABLE_ECC
ifdef in NSS public header files that we should remove.  The
only remaining ifdef is in nss.h for the value of the
NSS_VERSION macro (a string), which we can't remove.
Attachment #222098 - Flags: review?(wtchang) → review+
Attachment #222098 - Attachment description: patch: Allow unconditial access to the TLS cipher suite numbers even if ECC is turned of → patch: Allow unconditional access to the TLS cipher suite numbers even if ECC is turned off
Attachment #222098 - Flags: superreview?(nelson) → superreview+
The target milestone is already released. Resetting target milestone.
Target Milestone: 3.11.1 → ---
Blocks: FIPS2008
Assignee: wtc → rrelyea
No longer blocks: FIPS2008
Bob, what is the status of this bug?
It has a patch that got r+ over two years ago, but has not been committed.
Do you no longer wish to do this fix?  If so, please resolve WONTFIX
It's pretty obvious we're not going to do this.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: