Closed
Bug 1004149
Opened 10 years ago
Closed 8 years ago
nsNSSHttpInterface functions should return mozilla::pkix::Result result values
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: keeler, Assigned: Cykesiopka)
References
Details
Attachments
(3 files)
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 | ||
Updated•8 years ago
|
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
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
Reporter | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40641/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40641/
Attachment #8731518 -
Flags: review?(dkeeler)
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40643/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40643/
Attachment #8731519 -
Flags: review?(dkeeler)
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40645/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40645/
Attachment #8731520 -
Flags: review?(dkeeler)
Reporter | ||
Comment 6•8 years ago
|
||
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+
Reporter | ||
Updated•8 years ago
|
Attachment #8731519 -
Flags: review?(dkeeler) → review+
Reporter | ||
Comment 7•8 years ago
|
||
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).
Reporter | ||
Comment 8•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
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).
Assignee | ||
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
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.
Assignee | ||
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
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
Assignee | ||
Comment 14•8 years ago
|
||
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
Assignee | ||
Comment 15•8 years ago
|
||
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
Assignee | ||
Comment 16•8 years ago
|
||
Try request is in comment 12. Probably should've actually published the updated diffs first...
Comment 17•8 years ago
|
||
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
Keywords: checkin-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
Flags: needinfo?(cykesiopka.bmo)
Assignee | ||
Comment 19•8 years ago
|
||
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/
Assignee | ||
Comment 20•8 years ago
|
||
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/
Assignee | ||
Comment 21•8 years ago
|
||
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/
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cd49bae9bc8
Flags: needinfo?(cykesiopka.bmo)
Keywords: checkin-needed
Comment 23•8 years ago
|
||
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
Keywords: checkin-needed
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0b482f3e7f0d https://hg.mozilla.org/mozilla-central/rev/8a2c5b46e55b https://hg.mozilla.org/mozilla-central/rev/8305b744af0c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•