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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

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;

Comment 1

[:Cykesiopka]

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).

Comment 2

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

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 3

[:Cykesiopka]

Attachment: MozReview Request: Bug 1004149 - Remove some dead code. r=keeler

Review commit: https://reviewboard.mozilla.org/r/40641/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40641/

Comment 4

[:Cykesiopka]

Attachment: MozReview Request: Bug 1004149 - Return mozilla::pkix::Result values in nsNSSHttpInterface functions. r=keeler

Review commit: https://reviewboard.mozilla.org/r/40643/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40643/

Comment 5

[:Cykesiopka]

Attachment: MozReview Request: Bug 1004149 - Add some missing OCSP URL tests. r=keeler

Review commit: https://reviewboard.mozilla.org/r/40645/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40645/

Comment 6

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

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.

Comment 7

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

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 8

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

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:[e-mail]: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)

Comment 9

[:Cykesiopka]

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).

Comment 10

[:Cykesiopka]

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.

Comment 11

[:Cykesiopka]

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

> Hmmm - is there anything that actually enforces that the client didn't try to access http://user:[e-mail]: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.

Comment 12

[:Cykesiopka]

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.)

Comment 13

[:Cykesiopka]

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/

Comment 14

[:Cykesiopka]

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/

Comment 15

[:Cykesiopka]

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/

Comment 16

[:Cykesiopka]

Try request is in comment 12. Probably should've actually published the updated diffs first...

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f164ff25450e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c6888880270
https://hg.mozilla.org/integration/mozilla-inbound/rev/e14e69dd0ff6

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for windows build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=24121996&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/905d35af02ec

Comment 19

[:Cykesiopka]

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 20

[:Cykesiopka]

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 21

[:Cykesiopka]

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/

Comment 22

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cd49bae9bc8

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b482f3e7f0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a2c5b46e55b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8305b744af0c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b482f3e7f0d
https://hg.mozilla.org/mozilla-central/rev/8a2c5b46e55b
https://hg.mozilla.org/mozilla-central/rev/8305b744af0c