1063006 - Centralize use of NSS for cryptography into one place in the mozilla::pkix test suite

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: centralize-test-crypto-stuff.patch

In order to run the mozilla::pkix tests without requiring NSS, one will need to swap out the code that uses NSS for code that uses another crypto library. This patch puts all the NSS-using crypto stuff into one file so that that can be cone in a convenient and maintainable way, without any negative impact on Gecko. (In fact, even if one doesn't want to swap out NSS for something else, this patch makes the tests more maintainable.)

Comment 1

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8484331 [details] [diff] [review]
centralize-test-crypto-stuff.patch

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

I'm going to redirect this to rbarnes.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8484331 [details] [diff] [review]
centralize-test-crypto-stuff.patch

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

David, I can understand that you're probably busy. This review isn't urgent. I'd rather wait for you to review it, rather than have somebody less qualified review it. Thanks for your understanding!

Comment 3

[Richard Barnes [:rbarnes]]

Comment on attachment 8484331 [details] [diff] [review]
centralize-test-crypto-stuff.patch

I'm still going to try to review this, in addition to keeler.

Comment 4

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8484331 [details] [diff] [review]
centralize-test-crypto-stuff.patch

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

Looks good to me with some updates to comments.

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +13,5 @@
>  #include "secerr.h"
>  
> +namespace mozilla { namespace pkix { namespace test {
> +
> +// ownership of publicKey and privateKey is transfered

nit: out of date comment?

::: security/pkix/test/lib/moz.build
@@ +20,5 @@
>  # See the License for the specific language governing permissions and
>  # limitations under the License.
>  
>  SOURCES += [
> +    'pkixtestcrypto.cpp',

Is it worth it to have "nss" in the name of this file to specify that it's implemented using NSS?

::: security/pkix/test/lib/pkixtestcrypto.cpp
@@ +43,3 @@
>  {
> +public:
> +  // NSSTestKeyPair takes ownership of publicKey and privateKey.

I don't see a "publicKey" - is this comment out of date? Oh, does this refer to spki and spk? (hmmm - but for those it copies the data, right?)

@@ +95,5 @@
>  
> +// This private function is also used by Gecko's PSM test framework
> +// (OCSPCommon.cpp).
> +//
> +// Ownership of publicKey and privateKey is transfered

Again, this comment should be updated.

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +379,2 @@
>        signatureAlgorithmDER.assign(alg_sha256WithRSAEncryption,
> +                                    sizeof(alg_sha256WithRSAEncryption));

nit: this line doesn't need to change (looks like it just got indented one space too many)

@@ +502,2 @@
>  {
> +  // It may be the case that privateKeyResult references to the same

nit: s/to //

@@ +502,3 @@
>  {
> +  // It may be the case that privateKeyResult references to the same
> +  // TestKeyPair as issuerKeyPair;  thus, we can't set keyPairResult until

nit: s/;  /; / (there are two spaces after the ';')
super-pedantic English style nit: I think it would be more natural to either end the first sentence instead of using a semicolon or just omit 'thus' (oh, I see - it was already like that. Well, it really doesn't matter in any case.)

@@ +1015,5 @@
> +    // SubjectPublicKeyInfo structure, but we need just the subjectPublicKey
> +    // part.
> +    Reader input(context.certID.issuerSubjectPublicKeyInfo);
> +    Reader contents;
> +    if (der::ExpectTagAndGetValue(input, der::SEQUENCE, contents) != Success) {

Hmmm - having to decode something using the library we're testing seems like not a great situation to be in. Let's try to do this as little as possible.

::: security/pkix/test/lib/pkixtestutil.h
@@ +165,2 @@
>                                      SignatureAlgorithm signatureAlgorithm,
> +                            /*out*/ ScopedTestKeyPair& privateKey);

nit: update the parameter names to match the implementation

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8484331 [details] [diff] [review]
centralize-test-crypto-stuff.patch

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

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +13,5 @@
>  #include "secerr.h"
>  
> +namespace mozilla { namespace pkix { namespace test {
> +
> +// ownership of publicKey and privateKey is transfered

changed to "Ownership of privateKey is transfered."

::: security/pkix/test/lib/moz.build
@@ +20,5 @@
>  # See the License for the specific language governing permissions and
>  # limitations under the License.
>  
>  SOURCES += [
> +    'pkixtestcrypto.cpp',

I renamed it to pkixtestnss.cpp, to be consistent with pkixnss.cpp.

::: security/pkix/test/lib/pkixtestcrypto.cpp
@@ +43,3 @@
>  {
> +public:
> +  // NSSTestKeyPair takes ownership of publicKey and privateKey.

I've removed the reference to publicKey.

(Originally, I wrote the patch so that TestKeyPair had a publicKey member. But then I realized I really needed only the serialized form of the public key, and then I forgot to update the comments after I made that change.)

@@ +95,5 @@
>  
> +// This private function is also used by Gecko's PSM test framework
> +// (OCSPCommon.cpp).
> +//
> +// Ownership of publicKey and privateKey is transfered

Done. I also added a period at the end of the sentence.

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +379,2 @@
>        signatureAlgorithmDER.assign(alg_sha256WithRSAEncryption,
> +                                    sizeof(alg_sha256WithRSAEncryption));

Undone.

@@ +502,2 @@
>  {
> +  // It may be the case that privateKeyResult references to the same

Fixed.

@@ +502,3 @@
>  {
> +  // It may be the case that privateKeyResult references to the same
> +  // TestKeyPair as issuerKeyPair;  thus, we can't set keyPairResult until

Fixed.

@@ +1015,5 @@
> +    // SubjectPublicKeyInfo structure, but we need just the subjectPublicKey
> +    // part.
> +    Reader input(context.certID.issuerSubjectPublicKeyInfo);
> +    Reader contents;
> +    if (der::ExpectTagAndGetValue(input, der::SEQUENCE, contents) != Success) {

I agree, in general.

Note that ExpectTagAndGetValue and similar are not really under test in the tests that use this function; they are tested in pkixder_*_tests.cpp. However, again, I agree this is less than ideal.

::: security/pkix/test/lib/pkixtestutil.h
@@ +165,2 @@
>                                      SignatureAlgorithm signatureAlgorithm,
> +                            /*out*/ ScopedTestKeyPair& privateKey);

Done.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6d009cc55f

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d6d009cc55f

Comment 8

[Martin Thomson [:mt:]]

Attachment: Bug 1063006 - Fixup randomness pointer, r?keeler

I found this when trying to copy the code. It looks like it was taking the
address of the RANDOM_NUMBER pointer. I also moved some initialization
out of the loop and used const_cast instead of a C-style cast.

Not sure about the formatting constraints on mozpkix. Should we be running
clang-format here?

Comment 9

[Martin Thomson [:mt:]]

Performing some bug necromancy here...

https://hg.mozilla.org/projects/nss/rev/1ca1e27e073668931cd76408e062fb372fe6608c