Closed
Bug 1301117
Opened 8 years ago
Closed 8 years ago
Update Opportunistic Security for HTTP
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: mcmanus, Assigned: mcmanus)
References
()
Details
Attachments
(1 file)
81.01 KB,
patch
|
u408661
:
review+
|
Details | Diff | Splinter Review |
largely to be in line with httpwg.org/http-extensions/opsec.html That is an experimental document in last call. As part of that last call we'll providing feedback on the following aspects of it (to match the update) http:// uses of TLS (which we will require h2 for) must always be authenticated via TLS - even on the same host. (previously this was only required across hosts. The .well-known opt-in will always be required drop the tls-commit feature as it does not match the fallback semantics of alt-svc well. This update also makes the validated routes persistent between sessions. Runtime errors continue to purge stored validations (i.e. no latching) - though errors related to speculative connect are removed from that.
Assignee | ||
Comment 1•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf0fa654cc2f
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8789438 -
Flags: review?(hurley)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Comment on attachment 8789438 [details] [diff] [review] update opportunisitic encryption Review of attachment 8789438 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good to me. One thing (the number 2) I think really needs a change one way or another (see comment below), the rest of my comments I could take or leave. r=me with that change. ::: netwerk/protocol/http/AlternateServices.cpp @@ +361,5 @@ > +{ > + mValidated = false; > + nsresult code; > + > + do { Do you need the do/while here? Seems like you could just define NEXT_TOKEN to return instead - I'd find that slightly more readable (the early return/break is hidden in NEXT_TOKEN no matter what so no biggie there), but that could just be me. @@ +517,5 @@ > +class WellKnownChecker > +{ > +public: > + WellKnownChecker(nsIURI *uri, const nsCString &origin, uint32_t caps, nsHttpConnectionInfo *ci, AltSvcMapping *mapping) > + : mWaiting(2) Why 2? It makes sense reading later, but not here. Might be more readable to call this mChannelsFinished (or similar) and increment rather than decrement. At the very least, please comment. @@ +729,5 @@ > +NS_IMETHODIMP > +TransactionObserver::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + // only consider the first 32KB.. because really. 64... I mean 32k should be enough for anyone, right? RIGHT?! :-D @@ +914,5 @@ > + if (!mStorage) { > + mStorage = DataStorage::Get(NS_LITERAL_STRING("AlternateServices.txt")); > + if (mStorage) { > + bool storageWillPersist = false; > + if (NS_FAILED(mStorage->Init(storageWillPersist))) { You had me scared for a second that this was main thread io. Then I read datastorage.h and was pleasantly surprised! ::: netwerk/protocol/http/nsHttp.h @@ +85,5 @@ > #define NS_HTTP_ONPUSH_LISTENER (1<<9) > > +// Transactions with this flag should react to errors without side effects > +// First user is to prevent clearing of alt-svc cache on failed probe > +#define NS_HTTP_ERROR_SOFTLY (1<<10) I'm unreasonably amused by this name. ::: netwerk/protocol/http/nsHttpHandler.h @@ +247,5 @@ > mConnMgr->UpdateAltServiceMapping(map, proxyInfo, callbacks, caps, > originAttributes); > } > > + already_AddRefed<AltSvcMapping>GetAltServiceMapping(const nsACString &scheme, nit: space after > ::: netwerk/test/unit/test_http2.js @@ +1083,5 @@ > prefs.setBoolPref("network.http.spdy.enforce-tls-profile", false); > prefs.setBoolPref("network.http.altsvc.enabled", true); > prefs.setBoolPref("network.http.altsvc.oe", true); > + prefs.setCharPref("network.dns.localDomains", "foo.example.com, bar.example.com"); > + nit: empty line w/whitespace
Attachment #8789438 -
Flags: review?(hurley) → review+
Assignee | ||
Comment 4•8 years ago
|
||
> Do you need the do/while here? Seems like you could just define NEXT_TOKEN
> to return instead
There are obviously a lot of not-so-great ways to do this.. I'm not a fan of returns that are obscured (like NS_ENSURE_SUCCESS). I did add a comment here that the do {} while(0) is there to play the role of try/catch and that makes things easier to grok. In an update I also undef'd the macro at the end of the block :)
Assignee | ||
Comment 5•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7078c20114e2afee1c05dc65cb853b6ab388afe9 Bug 1301117 - update opportunisitic encryption r=hurley
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7078c20114e2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•8 years ago
|
Summary: update opportunisitic http:// security → Update Opportunistic Security for HTTP
You need to log in
before you can comment on or make changes to this bug.
Description
•