Closed Bug 1301117 Opened 3 years ago Closed 3 years ago

Update Opportunistic Security for HTTP

Categories

(Core :: Networking: HTTP, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

References

()

Details

Attachments

(1 file)

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.
Attachment #8789438 - Flags: review?(hurley)
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+
> 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 :)
https://hg.mozilla.org/mozilla-central/rev/7078c20114e2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Summary: update opportunisitic http:// security → Update Opportunistic Security for HTTP
Blocks: 1309809
No longer blocks: 1309809
Duplicate of this bug: 1309809
You need to log in before you can comment on or make changes to this bug.