Closed Bug 783579 Opened 9 years ago Closed 8 years ago

Add to certutil the ability to specify opFlags and attrFlags

Categories

(NSS :: Tools, enhancement, P2)

3.13.5
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: elio.maldonado.batiz, Assigned: rrelyea)

References

Details

Attachments

(3 files, 1 obsolete file)

Was reported by Chhistina Fu:
Christina Fu 2012-05-10 12:59:01 EDT

Description of problem:
When attached to an nethsm to generate EC key pairs, certutil fails with the following error:
certutil: unable to generate key(s)
: security library: received bad data.

Version-Release number of selected component (if applicable):
nss-3.12.10

How reproducible:
always

Steps to Reproduce:
1. Install Thales nethsm Connect per HSM vendor instruction; attach nethsm on client:
   modutil -dbdir . -nocertdb -add nfast -libfile /opt/nfast/toolkits/pkcs11/libcknfast.so
2. create password file and random file (e.g. cfu_pwfile, cfu_ran)
3. generate EC CSR with certutil:
  certutil -d . -h NHSM6000-OCS -f cfu_pwfile -R -s "cn=Christian Fu ECC test" -k ec -q nistp256 -a -z cfu_ran> cfu.csr
  
Actual results:
certutil: unable to generate key(s)
: security library: received bad data.

Expected results:
should work (it works for certicom)

Additional info:
Further investigation shows that certutil calls PK11_GenerateKeyPair() with hardcoded isSensitive flag set to true.  I think to get it to work we need to call PK11_GenerateKeyPairWithOpFlags() as what I did with JSS for our Java based subsystems so we can pass in the values that will enable nethsm to work.  I also know that lunasa would need a different set of values for isSensitive and extractability. 
I think certutil should expose "temporary", "sensitive", "extractable" and the op flags and mask. in order to support various hsms.
Assignee: nobody → rrelyea
Comment on attachment 652785 [details] [diff] [review]
Adds the ability to specify opFlags and attrFlags in certutil

since it's my patch, and it's to cert util, let's have elio review it.;).

bob
Attachment #652785 - Flags: review?(emaldona)
Attachment #652785 - Attachment is patch: true
Comment on attachment 652785 [details] [diff] [review]
Adds the ability to specify opFlags and attrFlags in certutil

Review of attachment 652785 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is good and it gets an r+ except that the help text that reads a bit awkward.
I suggest the following alternative wording on the key attributes flags description:

::: ./mozilla/security/nss/cmd/certutil/certutil.c.orig
@@ +1246,5 @@
>          "   -P dbprefix");
> +    FPS "%-20s PKCS #11 key Attributes\n",
> +        "   --keyAttrFlags attrflags.");
> +    FPS "%-20s Comma sparated list of the following pairs\n", "");
> +    FPS "%-20s (one pair member per list):\n", "");

FPS "%-20s PKCS #11 key Attributes\n",
		" --keyAttrFlags attrflags.");
		FPS "%-20s Comma separated list of key attribute attribute flags,\n", "");
		FPS "%-20s selected from the following list of choices:\n", "");
		FPS "%-20s {token | session} {public | private} {sensitive | insensitive}\n", "");
		FPS "%-20s {modifiable | unmodifiable} {extractable | unextractable}\n", "");

@@ +1253,5 @@
> +    FPS "%-20s PKCS #11 key Operation Flags\n",
> +        "   --keyOpFlagsOn opflags.");
> +    FPS "%-20s PKCS #11 key Operation Flags\n",
> +        "   --keyOpFlagsOff opflags.");
> +    FPS "%-20s Comma sparated list of one or more of the following:\n", "");

fix typo: s/sparated/separated/
FPS "%-20s Comma separated list of one or more of the following:\n", "");
Oops, look who's talking :-)
Attachment #652785 - Flags: review?(emaldona) → review+
Changes checked in to the tip for nss-3.15:
https://hg.mozilla.org/projects/nss/rev/94a1e80e5f71
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Severity: normal → enhancement
OS: Linux → All
Priority: -- → P2
Hardware: x86_64 → All
Target Milestone: --- → 3.15
1. "unextractable" is mapped to PK11_ATTR_EXTRACTABLE, which is
a copy-and-paste error.

2. GetFlags() has two fprintf(stderr) statements, which look like
debug messages. Should they be removed?
Attachment #753008 - Flags: superreview?(rrelyea)
Attachment #753008 - Flags: review?(emaldona)
The three new options are too long, so they break the alignment.
They also end with a period, not sure if that was intentional.

I suggest reformatting the help message as follows.

Before:

-G              Generate a new key pair
   ...
   --keyAttrFlags attrflags. PKCS #11 key Attributes
                     Comma separated list of key attribute attribute flags,
                     selected from the following list of choices:
                     {token | session} {public | private} {sensitive | insensitive}
                     {modifiable | unmodifiable} {extractable | unextractable}
   --keyOpFlagsOn opflags. PKCS #11 key Operation Flags
   --keyOpFlagsOff opflags. PKCS #11 key Operation Flags
                     Comma separated list of one or more of the following:
                     encrypt, decrypt, sign, sign_recover, verify,
                     verify_recover, wrap, unwrap, derive

After:

-G              Generate a new key pair
   ...
   --keyAttrFlags attrflags
                     PKCS #11 key Attributes.
                     Comma separated list of key attribute attribute flags,
                     selected from the following list of choices:
                     {token | session} {public | private} {sensitive | insensitive}
                     {modifiable | unmodifiable} {extractable | unextractable}
   --keyOpFlagsOn opflags
   --keyOpFlagsOff opflags
                     PKCS #11 key Operation Flags.
                     Comma separated list of one or more of the following:
                     encrypt, decrypt, sign, sign_recover, verify,
                     verify_recover, wrap, unwrap, derive
Attachment #753010 - Flags: superreview?(rrelyea)
Attachment #753010 - Flags: review?(emaldona)
Comment on attachment 753008 [details] [diff] [review]
Fix incorrect mapping of 'unextractable'. Remove debug messages in GetFlags.

Review of attachment 753008 [details] [diff] [review]:
-----------------------------------------------------------------

::: cmd/certutil/certutil.c
@@ +1987,5 @@
>      {NAME_SIZE(unmodifiable), PK11_ATTR_UNMODIFIABLE},
>      {NAME_SIZE(sensitive), PK11_ATTR_SENSITIVE},
>      {NAME_SIZE(insensitive), PK11_ATTR_INSENSITIVE},
>      {NAME_SIZE(extractable), PK11_ATTR_EXTRACTABLE},
> +    {NAME_SIZE(unextractable), PK11_ATTR_UNEXTRACTABLE}

Funny that it was correct in the one we used downstream.

@@ +2019,2 @@
>  	    char *tok;
>  

yes, it must have been "temporary" debugging code as you indicated.
Attachment #753008 - Flags: review?(emaldona) → review+
Attachment #753010 - Flags: review?(emaldona) → review+
After discussing with Elio, we determined that the second fprintf
message in GetFlags is a warning message and should be kept.  (In
fact it should become an error message.)  So I only remove the
first fprintf message.

https://hg.mozilla.org/projects/nss/rev/e9e054c6c03f
Attachment #753008 - Attachment is obsolete: true
Attachment #753008 - Flags: superreview?(rrelyea)
Attachment #753437 - Flags: checked-in+
Comment on attachment 753010 [details] [diff] [review]
Fix formatting of help message

r+ rrelyea
Attachment #753010 - Flags: superreview?(rrelyea) → superreview+
You need to log in before you can comment on or make changes to this bug.