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

RESOLVED FIXED in mozilla35

Status

()

Core
Security: PSM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 8484331 [details] [diff] [review]
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.)
Attachment #8484331 - Flags: review?(dkeeler)
Depends on: 1061021
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.
Attachment #8484331 - Flags: review?(dkeeler) → review?(rlb)
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!
Attachment #8484331 - Flags: review?(rlb) → review?(dkeeler)
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.
Attachment #8484331 - Flags: review?(rlb)
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
Attachment #8484331 - Flags: review?(dkeeler) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/7d6d009cc55f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Attachment #8484331 - Flags: review?(rlb)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.