303316 - Make TLS algorithms callable via freebl API (blapi)

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

There are several crypto algorithms that are implemented in softoken,
rather than in freebl.  Consequently, they cannot be called through 
the freebl API, and cannot be called by other code than softoken.

Shortly I will attach a set of patches that collectively move the 
implementations of HMAC, and TLS's Psuedo-Random Function (PRF) from 
softoken to freebl, and add them to blapi.  Also, since all the hash
functions are in freebl, I will move the table of hash functions used
by the HASH_ macros to freebl.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I did this work on the performance hacks branch.  You can see all the diffs here:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=NSS&branch=NSS_PERFORMANCE_HACKS_BRANCH&branchtype=match&dir=mozilla%2Fsecurity%2Fnss%2Flib%2Fsoftoken%2Cmozilla%2Fsecurity%2Fnss%2Flib%2Ffreebl&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-07-30&maxdate=2005-08-01&cvsroot=%2Fcvsroot

I am trying to turn this into a sensible patch for attachment, but it's a 
challenge.  This change depends on some prior work not yet checked in.
See bug 303334 for that work.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Changes to files that moved to freebl

This is the first of several patches that together will show all the 
changes for this bug.

I moved 3 files from softoken to freebl in their entirety.
In the course of that move, I had to make a few changes to them.
This patch shows the changes made to them, as if I had made the
changes to the files while they were in softoken, and then moved them.
Note that I add a function to intialize the HMAC hash context without
allocating a new one, and then reimplemented the old function that 
always allocated a new one.

There are (at least) 2 more patches coming:
- one for the changes made to files that stay in softoken.
- one for the files changed in freebl

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: changes to files that remain in softoken

These are the changes to the files that remain in softoken.
The table of raw hash functions has moved to freebl, and there is 
now a function that returns a pointer to an element of that table.
(That new function will appear in another of these patches, because
it is part of freebl.)

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: changes to freebl files and new tlsprfalg.c

This patch shows the changes to existing freebl files,
and one new file containing the TLS PRF.
The TLS PRF code was removed from a file in softoken, but 
that file remains in softoken, and this new file comes to freebl.

This third patch file completes the set of 3 that document this change.

Comment 5

[Wan-Teh Chang]

Comment on attachment 191563 [details] [diff] [review]
Changes to files that moved to freebl

r=wtc.	But I have some comments and suggested changes.

1. Please change the function name SEC_GetRawHashObject.
The SEC_ prefix has never been applied to the freebl
functions.  I also don't like "Raw".  Some suggested
names:
- BL_GetHashObject
- HASH_GetRawHashObject (cf. HASH_GetHashObject in
  lib/cryptohi/sechash.h)

Question: where do you declare SEC_GetRawHashObject?

Comment: it is strange that freebl/softoken code includes
sechash.h, a header from lib/cryptohi.

2. The new HMAC_Init function now duplicates some of the
code in HMAC_Destroy.  It may be a good idea to turn the
duplicated code into a function.  (Bob Relyea uses the
naming convention XXX_DestroyStatic for such functions
although I don't like the use of "static" here.)  I don't
have a strong opinion on whether this change is worthwhile
because the HMACContext structure is unlikely to change.

3. Nit: in alghmac.h, change "hashObj" to "hash_obj" in
the existing HMAC_Create declaration to be consistent.

Comment 6

[Wan-Teh Chang]

Comment on attachment 191564 [details] [diff] [review]
changes to files that remain in softoken

r=wtc.	This patch was easy to review.

Comment 7

[Wan-Teh Chang]

Comment on attachment 191650 [details] [diff] [review]
changes to freebl files and new tlsprfalg.c

r=wtc.	But I have some suggested changes.

1. I suggest that you simply make alghmac.h part of blapi.h
to be consistent with how the functions for the other algorithms
are declared.  They don't have their own headers.

If you still want alghmac.h, it doesn't need to be added to
the PRIVATE_EXPORTS list because it is not used outside the
lib/freebl directory.

2. Nit: In ldvector.c, we have:

> static const struct FREEBLVectorStr vector = 
> {
>@@ -207,6 +208,17 @@
...
>+    HMAC_Destroy,
>+    HMAC_Create,
>+    HMAC_Init,
>+    HMAC_Begin,
>+    HMAC_Update,
>+    HMAC_Finish,
>+    HMAC_Clone,
>+
>     /* End of Version 3.008. */
> };

It looks nicer to list HMAC_Destroy after HMAC_Init
or HMAC_Create.

I guess you've updated the version (3.008).

3. In tlsprfalg.c, please fix the file name in this comment:

>+/* tlsprf.c - TLS Pseudo Random Function (PRF) implementation

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Checked in on Trunk

Checking in freebl/alghmac.c;     new rev: 1.2;    previous rev: 1.1
Checking in freebl/alghmac.h;     new rev: 1.2;    previous rev: 1.1
Checking in freebl/blapi.h;       new rev: 1.20;   previous rev: 1.19
Checking in freebl/ldvector.c;    new rev: 1.10;   previous rev: 1.9
Checking in freebl/loader.c;      new rev: 1.20;   previous rev: 1.19
Checking in freebl/loader.h;      new rev: 1.14;   previous rev: 1.13
Checking in freebl/manifest.mn;   new rev: 1.38;   previous rev: 1.37
Checking in freebl/rawhash.c;     new rev: 1.2;    previous rev: 1.1
Checking in freebl/tlsprfalg.c;   new rev: 1.2;    previous rev: 1.1
Removing softoken/alghmac.c;      new rev: delete; previous rev: 1.12
Removing softoken/alghmac.h;      new rev: delete; previous rev: 1.7
Removing softoken/rawhash.c;      new rev: delete; previous rev: 1.5
Checking in softoken/lowpbe.c;    new rev: 1.12;   previous rev: 1.11
Checking in softoken/manifest.mn; new rev: 1.23;   previous rev: 1.22
Checking in softoken/pkcs11c.c;   new rev: 1.64;   previous rev: 1.63
Checking in softoken/pkcs11i.h;   new rev: 1.39;   previous rev: 1.38
Checking in softoken/tlsprf.c;    new rev: 1.6;    previous rev: 1.5

Requested changes will be made just before releasing the carpool

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Addresses review comments

Summary of changes in patch:
1. Rename SEC_GetRawHashObject to HASH_GetRawHashObject  everywhere
   It is declared in blapi.h .
2. in alghmac.h, change "hashObj" to "hash_obj"
3. in ldvector.c and loader.h, reorder the HMAC_ functions to match 
   order of use.

I didn't attempt to eliminate code duplication between HMAC_Init and 
HMAC_Destroy because it appears the entire duplication consists of 
these two lines:

    if (cx->hash != NULL)
	cx->hashobj->destroy(cx->hash, PR_TRUE);

I fixed the comment in the first line of tlsprfalg.c before checking 
it in, so it's not in this patch.

I left alghmac.h as a separate header.	It's included by blapi.h and
therefore must be as accessible as blapi.h, which is a privte export.
Since it's in freebl, if we don't make it a private kexport, how 
will the sources in softoken access it?

Comment 10

[Wan-Teh Chang]

You are right. alghmac.h needs to be in PRIVATE_EXPORTS
because it is needed by lib/softoken.

Comment 11

[Wan-Teh Chang]

Comment on attachment 191867 [details] [diff] [review]
Addresses review comments

r=wtc.	I think it's better to name the array BLHashObjects
and the function BL_GetHashObject because it's not clear what
"Raw" means, but I don't want to waste too much of your time.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the review, Wan-Teh.
Checking in freebl/alghmac.h;   new revision: 1.3;  previous revision: 1.2
Checking in freebl/blapi.h;     new revision: 1.21; previous revision: 1.20
Checking in freebl/ldvector.c;  new revision: 1.11; previous revision: 1.10
Checking in freebl/loader.c;    new revision: 1.21; previous revision: 1.20
Checking in freebl/loader.h;    new revision: 1.15; previous revision: 1.14
Checking in freebl/rawhash.c;   new revision: 1.3;  previous revision: 1.2
Checking in freebl/tlsprfalg.c; new revision: 1.3;  previous revision: 1.2
Checking in softoken/lowpbe.c;  new revision: 1.13; previous revision: 1.12
Checking in softoken/pkcs11c.c; new revision: 1.65; previous revision: 1.64