Update Opportunistic Security for HTTP

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

50 Branch
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 2

2 years ago
Created attachment 8789438 [details] [diff] [review]
update opportunisitic encryption
Attachment #8789438 - Flags: review?(hurley)
(Assignee)

Updated

2 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

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

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7078c20114e2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

2 years ago
Summary: update opportunisitic http:// security → Update Opportunistic Security for HTTP

Updated

2 years ago
Blocks: 1309809

Updated

2 years ago
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.