1301117 - Update Opportunistic Security for HTTP

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

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.

Comment 1

[Patrick McManus [:mcmanus]]

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

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: update opportunisitic encryption

Comment 3

[u408661]

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

Comment 4

[Patrick McManus [:mcmanus]]

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

Comment 5

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7078c20114e2afee1c05dc65cb853b6ab388afe9
Bug 1301117 - update opportunisitic encryption r=hurley

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7078c20114e2