Closed
Bug 305835
Opened 19 years ago
Closed 11 years ago
Remove NSS_ENABLE_ECC ifdefs in libssl
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: wtc, Assigned: rrelyea)
Details
(Whiteboard: ECC)
Attachments
(4 files, 1 obsolete file)
19.79 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
12.65 KB,
patch
|
Details | Diff | Splinter Review | |
5.49 KB,
patch
|
Details | Diff | Splinter Review | |
940 bytes,
patch
|
wtc
:
review+
nelson
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•19 years ago
|
||
This patch is for mozilla/security/nss/lib. I will attach another patch for mozilla/security/nss/cmd later.
Attachment #193765 -
Flags: review?(nelson)
Reporter | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.11
Reporter | ||
Comment 2•19 years ago
|
||
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.
Comment 3•19 years ago
|
||
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
Comment 4•19 years ago
|
||
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.
Comment 5•19 years ago
|
||
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).
Comment 6•19 years ago
|
||
(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 7•19 years ago
|
||
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+
Reporter | ||
Comment 8•19 years ago
|
||
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)
Reporter | ||
Comment 9•19 years ago
|
||
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.
Comment 10•19 years ago
|
||
(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
Comment 11•18 years ago
|
||
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
Comment 12•18 years ago
|
||
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
Updated•18 years ago
|
QA Contact: jason.m.reid → libraries
Assignee | ||
Comment 13•18 years ago
|
||
Attachment #222094 -
Flags: superreview?
Attachment #222094 -
Flags: review?(wtchang)
Assignee | ||
Comment 14•18 years ago
|
||
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)
Assignee | ||
Updated•18 years ago
|
Attachment #222098 -
Flags: superreview? → superreview?(nelson)
Reporter | ||
Comment 15•18 years ago
|
||
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+
Updated•18 years ago
|
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+
Comment 16•17 years ago
|
||
The target milestone is already released. Resetting target milestone.
Target Milestone: 3.11.1 → ---
Assignee | ||
Updated•16 years ago
|
Assignee: wtc → rrelyea
Comment 17•16 years ago
|
||
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
Comment 18•11 years ago
|
||
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.
Description
•