1406891 - Make pk12util documentation mention only working ciphers

Status: RESOLVED FIXED

(NSS :: Documentation, enhancement)

Description

[Daiki Ueno [:ueno]]

Attachment: pk12util-doc-fix.patch

Currently, the pk12util documentation mentions several unsupported ciphers (e.g. RC5 or 128/192-bit Camellia).  The attached patch removes them and adds compatibility notes for clarification.

Comment 1

[Hubert Kario]

Comment on attachment 8916563 [details] [diff] [review]
pk12util-doc-fix.patch

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

> is the default for the overall package encryption

there is no "package encryption" in PKCS#12, the key and the certificate are encrypted independently


none of RC2-CBC, DES-EDE3-CBC nor DES-CBC-Pad create PBES2 encrypted files

I haven't checked the names used for ciphers, but I'm not sure if the descriptions of them actually explain anything that the names don't

Comment 2

[Kai Engert (:KaiE:)]

Hubert: you're complaining about a pre-existing wording that Daiki isn't changing, and which is apparently unrelated to the issue that should be fixed in this bug. I don't think that's a reason to reject the patch. If you want to see this fixed, which I think could be done in a follow-up change, please don't just say "it's wrong", but please be proactive and suggest a better wording.

Comment 3

[Kai Engert (:KaiE:)]

(In reply to Hubert Kario from comment #1)
> 
> none of RC2-CBC, DES-EDE3-CBC nor DES-CBC-Pad create PBES2 encrypted files

Hubert, can you please suggest what should be said instead?

I see Daiki changed the header for a section:

-        <term>Symmetric CBC ciphers for PKCS#5 V2</term>
+        <term>PKCS #5 PBES2</term>


Should the old header be kept for the following items?

+           <listitem><para>DES-CBC-Pad (<userinput>DES-CBC</userinput>)</para></listitem>
+           <listitem><para>DES-EDE3-CBC-Pad (<userinput>DES-EDE3-CBC</userinput>) (the default for key encryption)</para></listitem>
+           <listitem><para>RC2-CBC-Pad (<userinput>RC2-CBC</userinput>)</para></listitem>
+           <listitem><para>AES-CBC-Pad (<userinput>AES-128-CBC</userinput>, <userinput>AES-192-CBC</userinput>, and <userinput>AES-256-CBC</userinput>)</para></listitem>

Comment 4

[Kai Engert (:KaiE:)]

(In reply to Hubert Kario from comment #1)
> I haven't checked the names used for ciphers, but I'm not sure if the
> descriptions of them actually explain anything that the names don't

I don't understand your comment. What are you suggesting?

Comment 5

[Hubert Kario]

(In reply to Kai Engert (:kaie:) from comment #2)
> Hubert: you're complaining about a pre-existing wording that Daiki isn't
> changing, and which is apparently unrelated to the issue that should be
> fixed in this bug. I don't think that's a reason to reject the patch. If you
> want to see this fixed, which I think could be done in a follow-up change,
> please don't just say "it's wrong", but please be proactive and suggest a
> better wording.

we've discussed this, we've agreed to fix the man page properly, not just patch it up

I don't see how splitting the issue into multiple bugs helps in that

(In reply to Kai Engert (:kaie:) from comment #3)
> (In reply to Hubert Kario from comment #1)
> > 
> > none of RC2-CBC, DES-EDE3-CBC nor DES-CBC-Pad create PBES2 encrypted files
> 
> Hubert, can you please suggest what should be said instead?

that they create PBES1 encrypted files

> I see Daiki changed the header for a section:
> 
> -        <term>Symmetric CBC ciphers for PKCS#5 V2</term>
> +        <term>PKCS #5 PBES2</term>
> 
> 
> Should the old header be kept for the following items?

not in general, with exception of AES

> 
> +           <listitem><para>DES-CBC-Pad
> (<userinput>DES-CBC</userinput>)</para></listitem>
> +           <listitem><para>DES-EDE3-CBC-Pad
> (<userinput>DES-EDE3-CBC</userinput>) (the default for key
> encryption)</para></listitem>
> +           <listitem><para>RC2-CBC-Pad
> (<userinput>RC2-CBC</userinput>)</para></listitem>
> +           <listitem><para>AES-CBC-Pad (<userinput>AES-128-CBC</userinput>,
> <userinput>AES-192-CBC</userinput>, and
> <userinput>AES-256-CBC</userinput>)</para></listitem>



(In reply to Kai Engert (:kaie:) from comment #4)
> (In reply to Hubert Kario from comment #1)
> > I haven't checked the names used for ciphers, but I'm not sure if the
> > descriptions of them actually explain anything that the names don't
> 
> I don't understand your comment. What are you suggesting?

I'm saying that the change

-           <listitem>DES-CBC</listitem>
+           <listitem><para>DES-CBC-Pad(<userinput>DES-CBC</userinput>)</para></listitem>

doesn't seem to me to be clearing things up, rather it does make it harder to follow and understand, so it either should be more explicit or not done

Comment 6

[Kai Engert (:KaiE:)]

Daiki, will you create an updated patch?

Comment 7

[Daiki Ueno [:ueno]]

Attachment: pk12util-doc-fix-v2.patch

(In reply to Hubert Kario from comment #1)

> none of RC2-CBC, DES-EDE3-CBC nor DES-CBC-Pad create PBES2 encrypted files

OK, these are mapped to PKCS #12 or PBES1 in:
https://searchfox.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11pbe.c#258

I removed them as well as the deprecated ciphers.

Comment 8

[Daiki Ueno [:ueno]]

Attachment: pk12util-doc-fix-v3.patch

As suggested by Hubert off-line, removed the mention of DES-CBC, which cannot be imported with OpenSSL.

Also expanded the compatibility notes indicating that the ciphers not listed in this document are not officially supported.

Comment 9

[Hubert Kario]

Comment on attachment 8922237 [details] [diff] [review]
pk12util-doc-fix-v3.patch

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

"PKCS #12 V2 PBE With SHA-1 And 2KEY Triple DES-CBC" doesn't seem to be supported - trying to use it for either key or certificate encryption causes the export to fail

Comment 10

[Hubert Kario]

> When not in FIPS mode, PKCS #12 SHA-1 and 40-bit RC4 is used for certificate encryption. When in FIPS mode, there is no certificate encryption.

It's possible to do that manually too, so maybe change that line to "...there is no certificate encryption (which can be achieved manually by specifying <option>-C NONE</option>)"

Comment 11

[Kai Engert (:KaiE:)]

Hubert, could you please suggest the wording that you'd be willing to exist? That might prevent having to do another patch/review cycle. We're short on time for 3.34

Comment 12

[Kai Engert (:KaiE:)]

to exist => to accept

Comment 13

[Daiki Ueno [:ueno]]

Attachment: pk12util-doc-fix-v4.patch

Removed the mention of the non-working 2-key triple DES and added the way to disable encryption.

Comment 14

[Daiki Ueno [:ueno]]

Attachment: pk12util-doc-fix-v5.patch

Removed the mention of "-c NONE", which turned out to be not working.  Also made the compatibility notes clearer by rewording the support status.

Comment 15

[Hubert Kario]

Comment on attachment 8922779 [details] [diff] [review]
pk12util-doc-fix-v5.patch

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

looks good, thanks!

Comment 16

[Daiki Ueno [:ueno]]

Thank you for the review.  Landed as:
https://hg.mozilla.org/projects/nss/rev/0b57f11ba442