Add 'unsafe-webtransport-hashes' keyword to connect-src

[jan-ivar]

Fixes #683. Assumes a unsafe-WebTransport-hash list flag on request (seems simplest post w3c/webtransport#697).

@dveditz @annevk @martinthomson WDYT?

---

Preview | Diff

[annevk]

Looks reasonable overall. @antosart probably needs to review.

[antosart]

Do we also need to adapt the post-request check? The interaction between fetch an webtransport is not totally clear to me, but I guess it would also go through https://fetch.spec.whatwg.org/#ref-for-should-block-response%E2%91%A0 ?

[antosart]

Also, we probably need WPTs.

@mikewest after conversations at TPAC, do we already have an updated process for changes to the spec?

[mikewest]

No, it's been on my list since TPAC, but I haven't gotten a proposal out yet.

Certainly will require WPT coverage and signals from other vendors, though. I think we have the latter here, but not the former?

[jan-ivar]

I've added the post-request check and used "unsafe-webtransport-hashes is not empty" for now. Thanks for the reviews!

(Unless there are any takers) I'll work on WPT next!

[jan-ivar]

Build is failing on "No 'dfn' refs found for 'webtransport-hashes'" which are provided by whatwg/fetch#1896.

WPT tests are in https://phabricator.services.mozilla.com/D276378.

[antosart]

Thank you. Let's wait for whatwg/fetch#1896. to be merged then.

A comment on the WPT, which I'll leave here for simplicity: Could you wait for the violation to be reported (only if CSP is expected to block, of course) in order to avoid flakiness of the test? Also, could you avoid the variable logs and instead separate the assertions? Something like:

const spvPromise = newPromise(resolve =>  { 
  window.onsecuritypolicyviolation = e => resolve(e.violatedDirective) 
}; 
const webTransportPromise = new Promise(resolve => {
 try {
    const wt = new WebTransport(url, options);
    t.add_cleanup(() => wt.close());
    wt.closed.catch(() => {});
    await wt.ready;
    resolve(true);
  } catch (e) {
    if (e.name != "WebTransportError") throw e;
    resolve(false);
  }
});
assert_equals(await webTransportPromise, expectAllowed);
if (expectViolation) {
  assert_equals(await spvPromise, "connect-src");
}

(feel free to improve further).

[jan-ivar]

Thanks for the feedback @antosart. But wouldn't that relax the test, which also tests that the event is NOT always fired?

I tried to align with the other connect-src tests (like connect-src-websocket-blocked.sub.html) here (modulo the arguably outdated async_test-based logTest.sub.js due to its poor error handling, timing out on any TypeError).

The CSP tests seem to observe a time window and test both that events are fired and NOT fired when appropriate. — I misread how logTest.js works.

Awaiting the event would cause timeouts on failure instead of reporting said failure like the other CSP tests do.

If we're concerned about intermittents I can increase the max time window to 1 second to match the other CSP tests. — I can add a 1 second timeout for when an event is expected.

[antosart]

Ah sorry. I was thinking about reports (which have a history of making tests flaky) but you are actually testing securitypolicyviolations, which should be fired deterministically (it should be enough to let the message loop run enough times). I'll take that back :)

[jan-ivar]

@antosart no the feedback was good. I've restructured the tests to be less timing dependent and match existing CSP tests. They now await the event with a 1 second timeout, same as logTest.js (once I understood it properly). Thanks!

[jan-ivar]

Aligned with upstream name tweak. Should be good to merge right after whatwg/fetch#1896.

[jan-ivar]

Tests merged in web-platform-tests/wpt#57217.

[antosart]

Thanks. IIUC there is a typo in the third test case in connect-src-webtransport-allowed.sub.https.html right? Since we want to check that even when specifying serverCertificateHashes, the request is blocked if csp does not allow unsafe-webtransport-hashes. Does the fix in https://chromium-review.googlesource.com/c/chromium/src/+/7502859 make sense to you?

[jan-ivar]

Hi @antosart, apologies for the late response. Yes your fix corrects the WPT to match the text...

Unfortunately, I suspect it's my text that is wrong, not the WPT. I believe the WPT (before your fix) matched our intent stated in #683 (comment).

I'll update the text tomorrow. Sorry for the churn on this. Thank goodness for tests, and your keen eye!

[jan-ivar]

@antosart @annevk algorithm updated (now stricter) PTAL. (Note: PR Preview appears out of date)

[jan-ivar]

Ping @antosart

[antosart]

Sorry for the delay! The PR looks fine to me, but just to clarify: the currently specced behaviour in this PR is that

CSP: connect-src 'self' would result in

• webtransport to a same-origin url without serverCertificateHashes to be allowed,
• webtransport to a same-origin url with severCertificateHashes to be blocked (even if the url matches the allowlist).

Is this correct? Is this what we want? And is that because specifying serverCertificateHashes in webtransport makes it somehow more dangerous so that an additional keyword in connect-src is needed?

If so, let's add back the testcase to the WPT (I think it makes sense to keep both

  {url: url2, options: {serverCertificateHashes}, expected: "connect-src"}
  {url: url1, options: {serverCertificateHashes}, expected: "connect-src"}

in connect-src-webtransport-allowed.sub.https.html)

[martinthomson]

Is this correct? Is this what we want? And is that because specifying serverCertificateHashes in webtransport makes it somehow more dangerous so that an additional keyword in connect-src is needed?

This is correct, yes. It's not so much about dangerous, but because the way that the identification happens. The existing system operates on names, with an assumption that the Web PKI has an active role. Listing hashes is fundamentally different.

serverCertificateHashes creates a distinct identity. That's true, even if the URL has the same path, because the process by which that URL is authenticated is different. That makes it a different identity even if serverCertificateHashes can deliver a very close approximation¹.

Footnotes

1. Yes, the listed hashes could be the hashes of the certificates from the web PKI certificates you would get from connecting to the given URL. Even there - aside from it being elaborate and pointless to specify hashes rather than a name - it's still a different entity at the other end: the web PKI might recognize different certificates later due to addition or revocation, where the hashes lack any of that uncertainty/flexibility. ↩

[antosart]

Thanks for the explanation! Makes sense to me. Then this PR looks fine.

I went ahead and fixed the WPT in https://chromium-review.googlesource.com/c/chromium/src/+/7566522, I'll wait for that to land and then can merge this PR.

[antosart]

Ok, tests are now merged. I think this is good to go!