1059928 - Remove use of SECOidTag from mozilla::pkix testing library's interface

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Attachment: less-SECOidTag.patch

This patch removes all SECOidTag references from pkixtestutil.h, so that SECOidTag is only used internally within pkixtestutil.cpp. This is part of the effort to remove NSPR and NSS dependencies from mozilla::pkix, and it is also a prerequisite to my work that will ultimately fix bug 1039166.

Comment 1

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

Attachment: Part 2: remove-AlgorithmIdentifier.patch

Comment 2

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

Attachment: remove-SECOidTag-from-test-interface.patch

Comment 3

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

https://tbpl.mozilla.org/?tree=Try&rev=19cb0c40d69d

Comment 4

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8482556 [details] [diff] [review]
remove-SECOidTag-from-test-interface.patch

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

Looks good with comments addressed.

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +637,5 @@
> +                       ByteString(tlv_id_kp_serverAuth,
> +                                  sizeof(tlv_id_kp_serverAuth)),
> +                       ByteString(tlv_id_kp_OCSPSigning,
> +                                  sizeof(tlv_id_kp_OCSPSigning))));
> +  Input tamperedResponseInput;

TamperOnce modifies the data pointed to by response, right? It doesn't seem necessary to have this additional Input.

::: security/pkix/test/lib/pkixtestutil.h
@@ +102,5 @@
>  // The result will be owned by the arena
>  const SECItem* ASCIIToDERName(PLArenaPool* arena, const char* cn);
>  
>  // Replace one substring in item with another of the same length, but only if
> +// the substring was found exactly once. The "same lenght" restriction is

nit: "length"

@@ +132,5 @@
>  //
>  // The return value, if non-null, is owned by the arena in the context and
>  // MUST NOT be freed.
>  SECItem* CreateEncodedCertificate(PLArenaPool* arena, long version,
> +                                  Input signature,

Please add some documentation that this is a DER encoding of a SEQUENCE of a signature algorithm OID.

@@ -198,5 @@
>    SECItem const* const* certs; // non-owning pointer to certs to embed
>  
>    // The following fields are on a per-SingleResponse basis. In the future we
>    // may support including multiple SingleResponses per response.
> -  SECOidTag certIDHashAlg;

This is a step backwards in terms of testing non-sha1 algorithms, but I guess since we hadn't made much progress on that anyway, it's not a big deal right now.

Comment 5

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

Comment on attachment 8482556 [details] [diff] [review]
remove-SECOidTag-from-test-interface.patch

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

Thanks for the quick review!

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +637,5 @@
> +                       ByteString(tlv_id_kp_serverAuth,
> +                                  sizeof(tlv_id_kp_serverAuth)),
> +                       ByteString(tlv_id_kp_OCSPSigning,
> +                                  sizeof(tlv_id_kp_OCSPSigning))));
> +  Input tamperedResponseInput;

No. tamperedResponse is a *copy* of response, so we need a new Input for tamperedResponse.

::: security/pkix/test/lib/pkixtestutil.h
@@ +102,5 @@
>  // The result will be owned by the arena
>  const SECItem* ASCIIToDERName(PLArenaPool* arena, const char* cn);
>  
>  // Replace one substring in item with another of the same length, but only if
> +// the substring was found exactly once. The "same lenght" restriction is

fixed.

@@ +132,5 @@
>  //
>  // The return value, if non-null, is owned by the arena in the context and
>  // MUST NOT be freed.
>  SECItem* CreateEncodedCertificate(PLArenaPool* arena, long version,
> +                                  Input signature,

Added:

// signature is assumed to be the DER encoding of an AlgorithmIdentifer.

@@ -198,5 @@
>    SECItem const* const* certs; // non-owning pointer to certs to embed
>  
>    // The following fields are on a per-SingleResponse basis. In the future we
>    // may support including multiple SingleResponses per response.
> -  SECOidTag certIDHashAlg;

I agree. I don't think SHA-2 support for CertID/ResponseID is going to be added any time soon.

Comment 6

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

Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/635e60240df2

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/635e60240df2