Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throwing serverCertificateHashes related errors #489

Merged
merged 9 commits into from
May 23, 2023
Merged

Throwing serverCertificateHashes related errors #489

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
22f947e
Fix serverCertificateHashes related spec
nidhijaju Mar 9, 2023
ea82b1a
throw trust error
nidhijaju Mar 10, 2023
7ab2060
fix
nidhijaju Mar 10, 2023
eeabeb7
throw WebTransportError instead of SecurityError
nidhijaju Apr 20, 2023
50b9e17
move to step 7.2
nidhijaju Apr 21, 2023
b8019aa
add valid cert clarification
nidhijaju Apr 21, 2023
1113b98
fix
nidhijaju Apr 21, 2023
2ea5b59
address comments
nidhijaju Apr 25, 2023
e950163
clarify conditions
nidhijaju Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ agent MUST run the following steps:
1. Let |serverCertificateHashes| be {{WebTransport/constructor(url, options)/options}}'s
{{WebTransportOptions/serverCertificateHashes}} if it exists, and null otherwise.
1. If |dedicated| is false and |serverCertificateHashes| is non-null, then [=throw=] a
{{TypeError}}.
{{NotSupportedError}} exception.
1. Let |requireUnreliable| be {{WebTransport/constructor(url, options)/options}}'s
{{WebTransportOptions/requireUnreliable}}.
1. Let |congestionControl| be {{WebTransport/constructor(url, options)/options}}'s
Expand Down Expand Up @@ -758,15 +758,16 @@ agent MUST run the following steps:
[=ReadableStream/set up/pullAlgorithm=] set to |pullUnidirectionalStreamAlgorithm|, and
[=ReadableStream/set up/highWaterMark=] set to 0.
1. [=Initialize WebTransport over HTTP=] with |transport|, |parsedURL|, |dedicated|,
|requireUnreliable|, and |congestionControl|.
|requireUnreliable|, |congestionControl|, and |serverCertificateHashes|.
1. Return |transport|.

</div>

<div algorithm>
To <dfn>initialize WebTransport over HTTP</dfn>, given a {{WebTransport}} object
<var>transport</var>, a [=URL record=] |url|, a boolean |dedicated|, a boolean
|http3Only|, and a {{WebTransportCongestionControl}} |congestionControl|, run these steps.
|http3Only|, a {{WebTransportCongestionControl}} |congestionControl|, and a
sequence&lt;{{WebTransportHash}}&gt; |serverCertificateHashes|, run these steps.

1. Let |client| be |transport|'s [=relevant settings object=].
1. Let |origin| be |client|'s [=environment settings object/origin=].
Expand All @@ -790,7 +791,12 @@ To <dfn>initialize WebTransport over HTTP</dfn>, given a {{WebTransport}} object
1. Let |connection| be the result of [=obtain a connection|obtaining a connection=] with
|networkPartitionKey|, |url|, false, |newConnection|, and |http3Only|. If the user agent
supports more than one congestion control algorithm, choose one appropriate for
|congestionControl| for sending of data on this |connection|.
|congestionControl| for sending of data on this |connection|. When obtaining a connection, if
|serverCertificateHashes| is specified, instead of using the default certificate verification
algorithm, consider the certificate valid if it meets the [=custom certificate
requirements=] and if [=verify a certificate hash|verifying the certificate hash=] against
|serverCertificateHashes| returns true. If either condition is not met, let |connection| be
failure.
Comment on lines +794 to +799
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ekinnear ekinnear May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

1. If |connection| is failure, then abort the remaining steps and [=queue a network task=] with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

|transport| to run these steps:
1. If |transport|.{{[[State]]}} is `"closed"` or `"failed"`, then abort these steps.
Expand Down Expand Up @@ -1175,22 +1181,22 @@ that determine how WebTransport connection is established and used.
</div>

<div algorithm>
To <dfn>compute a certificate hash</dfn>, do the following:
1. Let |cert| be the input certificate, represented as a DER encoding of
To <dfn>compute a certificate hash</dfn>, given a |certificate|, perform the following steps:
1. Let |cert| be |certificate|, represented as a DER encoding of
Certificate message defined in [[!RFC5280]].
1. Compute the SHA-256 hash of |cert| and return the computed value.

</div>

<div algorithm>
To <dfn>verify a certificate hash</dfn>, do the following:
1. Let |hashes| be the input array of hashes.
1. Let |referenceHash| be the [=compute a certificate hash|computed hash=] of the input certificate.
To <dfn>verify a certificate hash</dfn>, given a |certificate| and an array of hashes |hashes|,
perform the following steps:
1. Let |referenceHash| be the result of [=computing a certificate hash=] with |certificate|.
1. For every hash |hash| in |hashes|:
1. If |hash|.{{WebTransportHash/value}} is not null:
1. If |hash|.{{WebTransportHash/value}} is not null and |hash|.{{WebTransportHash/algorithm}}
is an [=ASCII case-insensitive=] match with "sha-256":
1. Let |hashValue| be the byte sequence which |hash|.{{WebTransportHash/value}} represents.
1. If {{WebTransportHash/algorithm}} of |hash| is an [=ASCII case-insensitive=] match with "sha-256", and |hashValue| is equal
to |referenceHash|, the certificate is valid. Return true.
1. If |hashValue| is equal to |referenceHash|, return true.
1. Return false.

</div>
Expand Down