744651 - Refactor secutil into high and low level components

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[Elio Maldonado]

On fedora and RHEL 6 and above we ship nss split into three packages, nss (higher levels), nss-softokn (the crypto boundary) adn nss-util. As part of nss-softoken we build blapitest (and fiptest) and run the bltest suite as part of the build. Blapitest along with the rest of the nss security tools use the secutil library for various services. Some of the utilities require the higher levels of nss while some require softoken and utils. I currently produce a subset of secutil with only what the crypto tools need. This is not satisfactory as I have to carry a patch and update it as the utilities change. 

It would be best to refactor secutil into low and high-level components. By low-level I refer to the basic functionality that doesn't depend of the higher levels of nss, just what util and softoken provide. That would be command line argument parsing, simple error messages, dumping of the components of DSA and RSA keys.

Comment 1

[Elio Maldonado]

Attachment: Refactoring of secutil

This is my first attempt at this refactoring.

Comment 2

[Elio Maldonado]

Attachment: Refactoring of secutil V2 - add missing pieces

Comment 3

[Elio Maldonado]

Attachment: Refactoring of secutil v3

Comment 4

[Elio Maldonado]

Attachment: Suplemmentary patch to ease the downstream maintainers' life

When building softoken without the higher layers of nss, as we do on fedora and rhel, the package maintainer would set the NSS_BUILD_SOFTOKEN_ONLY variable so that only whats vialable and needed gets compiled.

Comment 5

[Robert Relyea]

Comment on attachment 614850 [details] [diff] [review]
Refactoring of secutil v3

r+ rrelyea

I have a question about methodology. Did you walk through and take out all the functions that don't need to call nss and above, or did you just take the functions needed for blapitest.c.

I prefer the latter, but the former still gets an r+.

bob

Comment 6

[Robert Relyea]

Comment on attachment 614865 [details] [diff] [review]
Suplemmentary patch to ease the downstream maintainers' life

r+ with one caveat:

Fix the line with 2 header files on it. It should be
     = basicutil.h \
       secutil.h   \


This is extremely consistent throughout NSS.
(I should have made the same comment on the previous patch as well).

bob

Comment 7

[Elio Maldonado]

(In reply to Robert Relyea from comment #5)
> Comment on attachment 614850 [details] [diff] [review]
> Refactoring of secutil v3
> 
> r+ rrelyea
> 
> I have a question about methodology. Did you walk through and take out all
> the functions that don't need to call nss and above, or did you just take
> the functions needed for blapitest.c.
> 
> I prefer the latter, but the former still gets an r+.
> 
> bob

If I understand your question correctly I did the latter. I did it in two steps:
Step 1: Keep for basicutil.{c.h} only the portions needed by blapitest, the subset that compiles with nss-softokn were the higher layers of nss aren't available.
Step 2. Remove from secutil.{c|h} everything I have collected in Step 1 for basicutil.{c.h}

Comment 8

[Elio Maldonado]

Changed committed into the trunk:
Checking in mozilla/security/nss/cmd/bltest/blapitest.c;
/cvsroot/mozilla/security/nss/cmd/bltest/blapitest.c,v  <--  blapitest.c
new revision: 1.66; previous revision: 1.65
done
RCS file: /cvsroot/mozilla/security/nss/cmd/lib/basicutil.c,v
done
Checking in mozilla/security/nss/cmd/lib/basicutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.c,v  <--  basicutil.c
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/lib/basicutil.h,v
done
Checking in mozilla/security/nss/cmd/lib/basicutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.h,v  <--  basicutil.h
initial revision: 1.1
done
Checking in mozilla/security/nss/cmd/lib/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.17; previous revision: 1.16
done
Checking in mozilla/security/nss/cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.116; previous revision: 1.115
done
Checking in mozilla/security/nss/cmd/lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.42; previous revision: 1.41
done

Comment 9

[Elio Maldonado]

Attachment: The patch as was applied in the trunk

Comment 10

[Elio Maldonado]

Fixing a broken Tinderbox:
Checking in basicutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.c,v  <--  basicutil.c
new revision: 1.2; previous revision: 1.1
done
Checking in secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.117; previous revision: 1.116
done

Comment 11

[Elio Maldonado]

Attachment: Move secu_PrintECPublicKey from basicutil.c to secutil.c

Corrects the previously applied patch. Fixes Tinderbox breakage.

Comment 12

[Elio Maldonado]

Attachment: Purge the headers of duplicate declarations.

Remove from basicutil.h several declarations that belong, are alreaday, in secutil.h and from secutil one hat's in basicutil.

Comment 13

[Robert Relyea]

Comment on attachment 620534 [details] [diff] [review]
Purge the headers of duplicate declarations.

r+ Yes, these look like the belong to secutil.h... with the following exceptions:

SECU_RegisterDynamicOids() could be in either. it only requires nssutil.h. If you implement it in secutil.c then removing it from basicutil.h is fine.

bob

Comment 14

[Elio Maldonado]

SECU_RegisterDynamicOids() is implemented in moreoids.c, one of the so-called higher level files.
grep -r SECU_RegisterDynamicOids mozilla/security/nss
mozilla/security/nss/cmd/certutil/certutil.c:    	SECU_RegisterDynamicOids();
mozilla/security/nss/cmd/pp/pp.c:    SECU_RegisterDynamicOids();
mozilla/security/nss/cmd/signver/signver.c:    SECU_RegisterDynamicOids();
mozilla/security/nss/cmd/checkcert/checkcert.c:    SECU_RegisterDynamicOids();
mozilla/security/nss/cmd/ocspclnt/ocspclnt.c:    SECU_RegisterDynamicOids();
mozilla/security/nss/cmd/vfyserv/vfyserv.c:	SECU_RegisterDynamicOids();
mozilla/security/nss/cmd/lib/moreoids.c:SECU_RegisterDynamicOids(void)
mozilla/security/nss/cmd/lib/secutil.h:extern SECStatus SECU_RegisterDynamicOids(void);
mozilla/security/nss/cmd/crlutil/crlutil.c:    SECU_RegisterDynamicOids();
mozilla/security/nss/cmd/vfychain/vfychain.c:    SECU_RegisterDynamicOids();

shows it used by the higher level test tools,not by blapitest.c. That's why I declared it in secutil.h. 

moreoids.c itself
$ grep include mozilla/security/nss/cmd/lib/moreoids.c
#include "secoid.h"
#include "secmodt.h" /* for CKM_INVALID_MECHANISM */
it includes lower level headers secoid.h from util and secmodt.h from softoken. So, it could go either way. Unless we anticipate using OIDs on a future softoken only test tool, I better keep SECU_RegisterDynamicOids were it is.

Comment 15

[Elio Maldonado]

Changes committed to the trunk:
Checking in mozilla/security/nss/cmd/lib/basicutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.h,v  <--  basicutil.h
new revision: 1.2; previous revision: 1.1
done
Checking in mozilla/security/nss/cmd/lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.43; previous revision: 1.42
done

Comment 16

[Elio Maldonado]

Attachment: Move SECU_Print{RSA|DSA}PublicKey to secutil.{c|h}

Two more that belong in the higher-level secutil part. They use SECKEYPublicKey, defined in key.h cryptohi. bltest already has its own dump_rsakey, dump_dsakey, etc. for printing key information out.

Comment 17

[Robert Relyea]

Comment on attachment 621703 [details] [diff] [review]
Move SECU_Print{RSA|DSA}PublicKey to secutil.{c|h}

r+ 

OK, Yes, SECKEYPublicKey is higher level. blapi uses SECKEYRawPublic key and company.

bob

Comment 18

[Elio Maldonado]

Patch committed to trunk:
Checking in basicutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.c,v  <--  basicutil.c
new revision: 1.3; previous revision: 1.2
done
Checking in basicutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.h,v  <--  basicutil.h
new revision: 1.3; previous revision: 1.2
done
Checking in secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.118; previous revision: 1.117
done
Checking in secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.44; previous revision: 1.43
done

Comment 19

[Kai Engert [:KaiE:]]

Did this break tinderbox?

gcc -o Linux2.6_x86_glibc_PTH_DBG.OBJ/blapitest.o -c -g -ansi -D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE -fPIC -Di386 -DLINUX2_1 -m32 -Wall -Werror-implicit-function-declaration -Wno-switch -pipe -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -DDEBUG -UNDEBUG -DDEBUG_tinderbox -D_REENTRANT -DNSS_ENABLE_ECC -DNSS_ECC_MORE_THAN_SUITE_B -DUSE_UTIL_DIRECTLY -DNSS_USE_STATIC_LIBS -I../../../nss/lib/softoken -I../../../../dist/Linux2.6_x86_glibc_PTH_DBG.OBJ/include -I../../../../dist/public/nss -I../../../../dist/private/nss -I../../../../dist/public/seccmd -I../../../../dist/public/dbm -I../../../../dist/public/softoken  blapitest.c
blapitest.c:513: error: expected =, ,, ;, asm or __attribute__ before * token
blapitest.c: In function pubkeyInitKey:
blapitest.c:1941: error: implicit declaration of function getECParams
blapitest.c:1941: warning: assignment makes pointer from integer without a cast

Comment 20

[Kai Engert [:KaiE:]]

backed out

Checking in lib/basicutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.c,v  <--  basicutil.c
new revision: 1.4; previous revision: 1.3
done
Checking in lib/basicutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.h,v  <--  basicutil.h
new revision: 1.4; previous revision: 1.3
done
Checking in lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.119; previous revision: 1.118
done
Checking in lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.45; previous revision: 1.44
done

Comment 21

[Elio Maldonado]

I will restore the patch as I have a temporary fix in blapitest that should make this one not break the build and we need this patch. As mentioned, the blapitest change is temporary and I must to find a proper fix. See our comments for Bug 746842

Comment 22

[Elio Maldonado]

Patch restored in trunk:
Checking in basicutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.c,v  <--  basicutil.c
new revision: 1.5; previous revision: 1.4
done
Checking in basicutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.h,v  <--  basicutil.h
new revision: 1.5; previous revision: 1.4
done
Checking in secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.120; previous revision: 1.119
done
Checking in secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.46; previous revision: 1.45
done

Comment 23

[Elio Maldonado]

Attachment: blapitest temporary workaround

This patch has already been applied to fix Tinderbox breakage after the changes made here. A proper fix would be to move the typedef to an existing lower level header such as those found on util. As far as I can tell none seem to be suitable so I may create a new header.

Comment 24

[Elio Maldonado]

Supplementary patch to enable building the softoken subset checked in to trunk:
Checking in mozilla/security/nss/cmd/lib/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.18; previous revision: 1.17
done