Closed Bug 1004149 Opened 7 years ago Closed 5 years ago

nsNSSHttpInterface functions should return mozilla::pkix::Result result values

Categories

(Core :: Security: PSM, defect)

Product:
Core
Component:
Security: PSM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: keeler, Assigned: Cykesiopka)

References

Details

Attachments

(3 files)

MozReview Request: Bug 1004149 - Remove some dead code. r=keeler
58 bytes, text/x-review-board-request
keeler
: review+
Details
MozReview Request: Bug 1004149 - Return mozilla::pkix::Result values in nsNSSHttpInterface functions. r=keeler
58 bytes, text/x-review-board-request
keeler
: review+
Details
MozReview Request: Bug 1004149 - Add some missing OCSP URL tests. r=keeler
58 bytes, text/x-review-board-request
keeler
: review+
Details 
nsNSSHttpInterface functions need to call PR_SetError when they fail. In particular, these functions (probably):
575   v1.createSessionFcn = createSessionFcn;
576   v1.keepAliveSessionFcn = keepAliveFcn;
577   v1.freeSessionFcn = freeSessionFcn;
578   v1.createFcn = createFcn;
579   v1.setPostDataFcn = setPostDataFcn;
580   v1.addHeaderFcn = addHeaderFcn;
581   v1.trySendAndReceiveFcn = trySendAndReceiveFcn;
582   v1.cancelFcn = cancelFcn;
583   v1.freeFcn = freeFcn;
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Hmm, it looks like the code in question is no longer called by NSS, so we don't have to do the silly PR_SetError() + PR_GetError() dance anymore. I'm morphing this bug to be about returning error/success values directly (mozilla::pkix::Result seems most sensible here).
Summary: nsNSSHttpInterface functions need to call PR_SetError when they fail → nsNSSHttpInterface functions should return mozilla::pkix::Result result values
FWIW, that whole implementation can be improved in a number of ways now that the API doesn't have to conform to what NSS is expecting.
Comment on attachment 8731518 [details]
MozReview Request: Bug 1004149 - Remove some dead code. r=keeler

https://reviewboard.mozilla.org/r/40641/#review37353

Cool.

::: security/manager/ssl/nsNSSCallbacks.h:96
(Diff revision 1)
>  
>  public:
>    static SECStatus createFcn(SEC_HTTP_SERVER_SESSION session,
>                               const char *http_protocol_variant,
>                               const char *path_and_query_string,
>                               const char *http_request_method, 

This might be a good time to remove trailing whitespace (and fixup tabs if there are any?) but no big deal.
Attachment #8731518 - Flags: review?(dkeeler) → review+
Attachment #8731519 - Flags: review?(dkeeler) → review+
Comment on attachment 8731519 [details]
MozReview Request: Bug 1004149 - Return mozilla::pkix::Result values in nsNSSHttpInterface functions. r=keeler

https://reviewboard.mozilla.org/r/40643/#review37355

This looks good. From a larger viewpoint, the structure of the API that actually fetches the OCSP response is an artifact of NSS' OCSP API. Since we don't use that anymore, it's much less clear and understandable than it needs to be. I think there would be a benefit to reworking that API by essentially removing it from nsNSSCallbacks and reimplementing it in OCSPRequestor.cpp. This would be a non-trivial undertaking, though, and we would have to be wary of regressions, of course. That's probably best for another bug.

::: security/certverifier/OCSPRequestor.h:16
(Diff revision 1)
>  #include "secmodt.h"
>  
>  namespace mozilla { namespace psm {
>  
> -// The memory returned is owned by the given arena.
> -SECItem* DoOCSPRequest(PLArenaPool* arena, const char* url,
> +// The memory returned via |encodedResponse| is owned by the given arena.
> +Result DoOCSPRequest(PLArenaPool* arena, const char* url,

Maybe this is something for another bug/patch, but I think we can get rid of the PLArenaPool and SECItems in this interface. The encodedRequest starts as an array of uint8_t, gets encapsulated in a SECItem, and then gets reinterpreted as a char*. Since we know its size the whole time, though (OCSP_REQUEST_MAX_LENGTH), I imagine we could just pass around a reference to that array (as well as the actual length of the data). As for the data returned, we could maybe just box it up in an nsCString of some kind and return it (by reference, I guess).

::: security/certverifier/OCSPRequestor.cpp:139
(Diff revision 1)
>    }
>    nsAutoCString
>      hostname(url + authorityPos + hostnamePos,
>               static_cast<nsACString_internal::size_type>(hostnameLen));
>  
>    SEC_HTTP_SERVER_SESSION serverSessionPtr = nullptr;

It would be nice to stop using SEC_HTTP_SERVER_SESSION and SEC_HTTP_REQUEST_SESSION in favor of their actual types (then again, if/when we rework the entire API, those types might go away altogether).
Comment on attachment 8731520 [details]
MozReview Request: Bug 1004149 - Add some missing OCSP URL tests. r=keeler

https://reviewboard.mozilla.org/r/40645/#review37417

LGTM with comments addressed.

::: security/manager/ssl/tests/unit/test_ocsp_url.js:128
(Diff revision 1)
>      ocspResponder.stop(run_next_test);
>    });
>  
> +  add_test(() => {
> +    clearOCSPCache();
> +    let ocspResponder = start_ocsp_responder(["user-pass"], [""]);

Hmmm - is there anything that actually enforces that the client didn't try to access http://user:pass@www.example.com:8888/ ?

::: security/manager/ssl/tests/unit/test_ocsp_url/bad-scheme.pem.certspec:3
(Diff revision 1)
>  issuer:int
>  subject:bad-scheme
> -extension:authorityInformationAccess:/www.example.com
> +extension:authorityInformationAccess:/www.example.com/

Why this change? (and similar, later)
Attachment #8731520 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/40641/#review37353

> This might be a good time to remove trailing whitespace (and fixup tabs if there are any?) but no big deal.

Done in part 2 (trailing whitespace anyways).
https://reviewboard.mozilla.org/r/40643/#review37355

I concur. It looks like Bug 1004154 is already filed on moving the code, so I'll just morph it to be more broad.

> Maybe this is something for another bug/patch, but I think we can get rid of the PLArenaPool and SECItems in this interface. The encodedRequest starts as an array of uint8_t, gets encapsulated in a SECItem, and then gets reinterpreted as a char*. Since we know its size the whole time, though (OCSP_REQUEST_MAX_LENGTH), I imagine we could just pass around a reference to that array (as well as the actual length of the data). As for the data returned, we could maybe just box it up in an nsCString of some kind and return it (by reference, I guess).

This would be nice, but indeed, probably for another bug.

> It would be nice to stop using SEC_HTTP_SERVER_SESSION and SEC_HTTP_REQUEST_SESSION in favor of their actual types (then again, if/when we rework the entire API, those types might go away altogether).

Yeah, probably saves some work by just reworking the API and getting rid of these then.
https://reviewboard.mozilla.org/r/40645/#review37417

> Hmmm - is there anything that actually enforces that the client didn't try to access http://user:pass@www.example.com:8888/ ?

I couldn't figure out a way to do this sadly.
I'll add a comment at least so it's clear that the test doesn't actually ensure user:pass wasn't sent.

> Why this change? (and similar, later)

I did this in the spirit of "test one aspect at a time", since URLs lacking a path is already tested by no-path-url.
Blocks: 1004154
Thanks for the review!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b52d13b2c9
(I cancelled the Windows build because after 2-4x the normal build time, it still wasn't done.)
Keywords: checkin-needed
Comment on attachment 8731518 [details]
MozReview Request: Bug 1004149 - Remove some dead code. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40641/diff/1-2/
Attachment #8731518 - Attachment description: MozReview Request: Bug 1004149 - Remove some dead code. → MozReview Request: Bug 1004149 - Remove some dead code. r=keeler
Comment on attachment 8731519 [details]
MozReview Request: Bug 1004149 - Return mozilla::pkix::Result values in nsNSSHttpInterface functions. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40643/diff/1-2/
Attachment #8731519 - Attachment description: MozReview Request: Bug 1004149 - Return mozilla::pkix::Result values in nsNSSHttpInterface functions. → MozReview Request: Bug 1004149 - Return mozilla::pkix::Result values in nsNSSHttpInterface functions. r=keeler
Comment on attachment 8731520 [details]
MozReview Request: Bug 1004149 - Add some missing OCSP URL tests. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40645/diff/1-2/
Attachment #8731520 - Attachment description: MozReview Request: Bug 1004149 - Add some missing OCSP URL tests. → MozReview Request: Bug 1004149 - Add some missing OCSP URL tests. r=keeler
Try request is in comment 12. Probably should've actually published the updated diffs first...
Comment on attachment 8731518 [details]
MozReview Request: Bug 1004149 - Remove some dead code. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40641/diff/2-3/
Comment on attachment 8731519 [details]
MozReview Request: Bug 1004149 - Return mozilla::pkix::Result values in nsNSSHttpInterface functions. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40643/diff/2-3/
Comment on attachment 8731520 [details]
MozReview Request: Bug 1004149 - Add some missing OCSP URL tests. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40645/diff/2-3/
You need to log in before you can comment on or make changes to this bug.