Closed Bug 1059928 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch less-SECOidTag.patch (obsolete) — Splinter Review
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.
Attachment #8480737 - Flags: review?(dkeeler)
Attachment #8480737 - Attachment is obsolete: true
Attachment #8481668 - Attachment is obsolete: true
Attachment #8480737 - Flags: review?(dkeeler)
Attachment #8481668 - Flags: review?(dkeeler)
Attachment #8482556 - Flags: review?(dkeeler)
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.
Attachment #8482556 - Flags: review?(dkeeler) → review+
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.
Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/635e60240df2
Target Milestone: --- → mozilla35
https://hg.mozilla.org/mozilla-central/rev/635e60240df2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.