Closed Bug 1029341 Opened 6 years ago Closed 6 years ago

Factor out extension parsing code from mozilla::pkix's pkixocsp into a reusable function

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

I think we're testing extension-related stuff quite extensively (sorry) already, but let me know if you think we need more tests.
Attachment #8444954 - Flags: review?(dkeeler)
Comment on attachment 8444954 [details] [diff] [review]
factor-out-OptionalExtensions.patch

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

Cool. I'm not sure Bind2_2nd is necessary. Also, OptionalExtensions should be consistent about using/not using the der:: namespace. r=me with those comments addressed.
We do have pretty good test coverage, but it's not complete. When we get OCSP gtests, I think we'd be in a good position to add more.

::: security/pkix/include/pkix/bind.h
@@ +92,5 @@
>    void operator=(const Bind2&) /*= delete*/;
>  };
>  
> +template <typename R, typename B1, typename P1, typename B2>
> +class Bind2_2nd

I'm not seeing where this is used.

::: security/pkix/lib/pkixder.h
@@ +670,5 @@
> +    Input tagged;
> +    if (ExpectTagAndGetValue(input, tag, tagged) != der::Success) {
> +      return Failure;
> +    }
> +    if (ExpectTagAndGetValue(tagged, der::SEQUENCE, extensions) != der::Success) {

We're already in the der:: namespace, right? 'der::SEQUENCE', 'der::Success' etc. shouldn't be necessary unless for stylistic decisions (in which case, each Failure, OIDTag, etc. should be 'der::Failure', 'der::OIDTag', etc. to be consistent).
Attachment #8444954 - Flags: review?(dkeeler) → review+
I removed the bind() overload I added. It was for a different implementation of this patch that used der::Nested. I also removed all the der:: namespace qualifiers. Thanks for catching these oversights and thanks for the quick review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/a10da316a35f
Assignee: nobody → brian
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla33
sorry had to back this out in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=152f6b19ad9c since one of this 2 csets seems to caused a bustage in device builds and B2g Emulator like https://tbpl.mozilla.org/php/getParsedLog.php?id=42422444&tree=Mozilla-Inbound
(In reply to Carsten Book [:Tomcat] from comment #3)
> sorry had to back this out in
> https://tbpl.mozilla.org/?tree=Mozilla-
> Inbound&onlyunstarred=1&rev=152f6b19ad9c since one of this 2 csets seems to
> caused a bustage in device builds and B2g Emulator like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=42422444&tree=Mozilla-
> Inbound

The backout was caused by another patch that landed at the same time. Relanding:
https://hg.mozilla.org/integration/mozilla-inbound/rev/904975e569b5
https://hg.mozilla.org/mozilla-central/rev/904975e569b5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Regression checking has lead me to this patch which is causing problems with LastPass.  I get a message that LP was unable to login to it's secure server.  Furthermore, when I try to install the latest version of LP via this link https://lastpass.com/lppre.xpi I get the following error message:

-------
Secure Connection Failed

An error occurred during a connection to lastpass.com. The response from the OCSP server was corrupted or improperly formed. (Error code: sec_error_ocsp_malformed_response)

The page you are trying to view cannot be shown because the authenticity of the received data could not be verified.
Please contact the website owners to inform them of this problem.
------
Blocks: 1031022
No longer blocks: 1031022
Depends on: 1031022
No longer depends on: 1031022
You need to log in before you can comment on or make changes to this bug.