Throwing serverCertificateHashes related errors

[nidhijaju]

This change specifies where the NotSupportedError and trust errors should be thrown when handling serverCertificateHashes. It also adds explicit parameters to the certificate hash algorithms.

Fixes #456 and fixes #359.

---

Preview | Diff

[ricea]

lgtm

[jan-ivar]

Meeting:

• @vasilvv to provide comments:
  • seems to be in the wrong place but no better place at the time
  • don't reveal info ahead of acceptance of connection

[jan-ivar]

@martinthomson to have a look.

[martinthomson]

When you say "both", do you refer to (hash check 1, hash check 2) or (hash checks, other stuff)?

Also, if the certificate is not valid, isn't there something you need to do? Like consider the connection to have failed?

[nidhijaju]

I was referring to both the certificate meeting the custom certificate requirements and verifying the hash against the serverCertificateHashes.

I tried rephrasing the sentences, and mentioning that the connection should have failed if either of those conditions are not met, but does it look better?

[martinthomson]

This bit about serverCertificateHashes might be better as a substep:

1. If |serverCertificateHashes| is specified, run the following steps:
    1.  If the certificate does not meet the [=custom certificate requirements=], mark the |connection| as failed.
    1. If the [=verify a certificate hash|verifying the certificate hash=] against |serverCertificateHashes| returns false, mark the |connection| as failed.

[nidhijaju]

I actually initially had this as substeps in earlier commits, but based on #489 (comment), I changed it to be part of the same step. Since these steps are a modification of the "obtain a connection" step in Fetch, I think we should keep this part of the same step so as to make it clear that these checks happen during the "obtain a connection" step, as opposed to after.

[martinthomson]

I don't think that conceptualizing these as part of "obtain a connection" helps. It is cleaner if you regard these as checks that occur after a connection was successfully obtained.

[nidhijaju]

I slightly prefer keeping it part of the same step, but either seems workable, so I'll update it to whatever seems best to you and @vasilvv.

[vasilvv]

I don't think that conceptualizing these as part of "obtain a connection" helps. It is cleaner if you regard these as checks that occur after a connection was successfully obtained.

I'm not sure I understand why this would be a separate check, since presumably you would want to perform it at the place you would normally perform certificate validation (as soon as you receive a certificate), which is in the middle of obtaining a connection (e.g. there might be steps after, like sending H/3 settings; also from error handling perspective, that means that a failure against certificate hash list is equivalent to failure against Web PKI in terms of how it's reported to the application).

[ekinnear]

It does seem like it ends up in a different subsystem and is something that is done during connection configuration at setup.

In other words, in code, this isn't something that we check afterwards, but rather something that we configure as part of setting up a connection object and starting it. We don't really want to connect somewhere and then decide "nevermind" because we didn't like the certificate presented during the handshake.

[martinthomson]

The phrasing here is a bit odd. Maybe if |connection| failed for any reason.

[nidhijaju]

"Obtain a connection" actually returns "failure" if the connection fails, which is what this step is checking.

[martinthomson]

Ah, I see what you are doing. This is like a Rust Result<T, E> where you get a composite object that is either success (a connection) or an error. You are checking if the result was an error.

The problem I have is that - while this fine if you are writing code, this is terribly stilted prose. It reads as a grammatical error.

[nidhijaju]

Yes, while I agree that it sounds odd, I think this is a fairly standard pattern throughout the spec ecosystem. For example, step 2 of https://url.spec.whatwg.org/#concept-url-parser and step 2 of https://encoding.spec.whatwg.org/#dom-textdecoder have similar text saying "if x is failure". There's also an open issue about clarifying what "failure" is at whatwg/infra#545, so perhaps when that is fixed, we can update it in the WebTransport spec.