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.

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

Public keys in request headers #21

Open
ddworken opened this issue Oct 9, 2024 · 12 comments
Open

Public keys in request headers #21

ddworken opened this issue Oct 9, 2024 · 12 comments

Comments

@ddworken
Copy link
Contributor

ddworken commented Oct 9, 2024

One thing that seems potentially useful to support is specifying the expected public key in request headers. For example, if an application specifies integrity="ed25519-[base64-encoded public key]", the request could include an Expected-Integrity: ed25519-[base64-encoded public key] request header to allow the backend to know that signature-based SRI is expected, and it will be validated against the given public key.

The concrete use case I have in mind is enabling an extremely basic form of key rotation. Suppose that a widely used library is signed with key A, but that key ends up getting compromised. The library owner would then want to start signing it with key B, but existing clients would already be pinning key A. If the backend knows what key the client has pinned, it could serve the corresponding signature.

This probably isn't critical for the MVP, so if it is controversial at all, I'd be inclined to skip this. But if it is non-controversial, this does seem like a nice capability that would be easy to support.

@mikewest
Copy link
Member

mikewest commented Oct 9, 2024

Assuming we use the framework in #16, I think we'd get Accept-Signature for "free" (insofar as it's defined in https://www.rfc-editor.org/rfc/rfc9421.html#section-5.1, though implemented nowhere). I guess we'd send a hash as the keyid (though that doesn't seem to be how it's defined... hrm)?

Spelling aside, there's some interaction with CORS here if we attach this header to that @annevk might wish to weigh in on. I think we'd also want to think a little bit about whether this is a communication channel to the server that we need to worry about. I'm not sure it is any different than GET parameters, but folks have had feedback on other proposals in this area we should make sure we consider.

Marking this as enhancement, just because I don't think we'd wait to ship this on having this feature.

@mikewest
Copy link
Member

mikewest commented Oct 9, 2024

Actually, reading that spec a little more closely, I'm not at all sure how Accept-Signature works. I clearly need to spend more than 5m skimming it, as it's somewhat complicated. :)

@mikewest
Copy link
Member

If we add this, we should also add some developer-facing text to the security considerations section to note that some configurations might leak the public key used to sign the response, as @annevk noted in #26.

@ddworken
Copy link
Contributor Author

ddworken commented Dec 7, 2024

One other advantage to this that I want to call out: It may be useful for websites to use this to prevent public key pinning. The use case I'm imagining is that a site might want to rely solely on server-initiated checks, e.g. because they're worried about a lack of key rotation. If requests contain the public key, they could reject these requests to guarantee that they're only relying on server-initiated checks.

@ddworken
Copy link
Contributor Author

ddworken commented Dec 9, 2024

One other use case for this: This makes it possible for JS libraries to understand which clients have enabled client opt-ins to signature SRI. This is potentially useful to allow sites to understand their security risks and run targeted outreach to increase deployment.

@mikewest
Copy link
Member

I started prototyping this as something like

Accept-Signatures: sig1=("identity-digest";sf);keyid="JrQLj5P/89iXES9+vFgrIy29clF9CC/oPPsw3c5D0bs=";tag="sri"

It's straightforward enough to spec and implement, though I think there are a few things to think through:

  1. The RFC suggests that the server processing the accept-signature header MUST return a signature with the same set of covered components, and MAY return additional headers with more components if desired. This is unfortunate, as I'm confident that different servers are going to have different sets of components they care to sign (some folks might care about @path, others about content-type, etc.). I think we'd want to suggest in the SRI profile that servers make use of the "MAY ignore any signature request that does not fit application parameters" carveout in the next sentence, and just sign resources in a way that makes them happy (which could certainly take the key in Accept-Signatures into account.

  2. We need to think through the interaction with CORS. As the header isn't Sec- prefixed, we have to deal with it being set on fetch() directly, or add it to the list of forbidden request headers (or mint a new header, I suppose). I can see justifications for either path: adding it to the forbidden list would leave the UA in control, making the header reasonably safe to send without preflights. It would also prevent developers from using their own Message Signature-based protocol through fetch(), which seems like a bit of an overreach. A more flexible alternative might be to add some processing to the CORS-safelisted request-header as we already do for accept and accept-language. We'd likely want to decide whether the header is a well-formed serialized dictionary with the right types or not? In this model, one expected signature could remain under the 128 byte limit (though two signatures would blow past it; somewhat unfortunate to trigger a preflight to support key rotation, but we can discuss that, I suppose).

  3. We likewise should think through whether this should be treated similarly to Fetch Metatdata or Sec-Purpose, attached to requests as they hit the network (which is how I modeled it in the prototype), or whether it should be more like Accept and Accept-Language, appended to the request headers early on. It made sense to me to treat it like a security-sensitive header that the browser should control, but I'm second guessing that after looking at the potential Fetch integration more closely.

I'd appreciate y'all's opinions! :)

@ddworken
Copy link
Contributor Author

  1. The RFC suggests that the server processing the accept-signature header MUST return a signature with the same set of covered components, and MAY return additional headers with more components if desired. This is unfortunate, as I'm confident that different servers are going to have different sets of components they care to sign (some folks might care about @path, others about content-type, etc.). I think we'd want to suggest in the SRI profile that servers make use of the "MAY ignore any signature request that does not fit application parameters" carveout in the next sentence, and just sign resources in a way that makes them happy (which could certainly take the key in Accept-Signatures into account.

+1. Mandating servers to sign specific components effectively makes it impossible to deploy build-time signing since there is a large combination of possible components. So changing this to allow servers to sign whatever they want sounds like a positive change to me.

  1. We need to think through the interaction with CORS. As the header isn't Sec- prefixed, we have to deal with it being set on fetch() directly, or add it to the list of forbidden request headers (or mint a new header, I suppose). I can see justifications for either path: adding it to the forbidden list would leave the UA in control, making the header reasonably safe to send without preflights. It would also prevent developers from using their own Message Signature-based protocol through fetch(), which seems like a bit of an overreach. A more flexible alternative might be to add some processing to the CORS-safelisted request-header as we already do for accept and accept-language. We'd likely want to decide whether the header is a well-formed serialized dictionary with the right types or not? In this model, one expected signature could remain under the 128 byte limit (though two signatures would blow past it; somewhat unfortunate to trigger a preflight to support key rotation, but we can discuss that, I suppose).

I don't have a strong opinion here.

My gut is that adding it to the forbidden request headers list seems reasonable, though I'm somewhat concerned that there could be sites out there already sending Accept-Signatures request headers, which could make this a breaking change? In which case it might be easier and safer to go for the CORS-safelisted approach? Or alternatively just move to specifying this as Sec-Accept-Signature to avoid these issues altogether?

  1. We likewise should think through whether this should be treated similarly to Fetch Metatdata or Sec-Purpose, attached to requests as they hit the network (which is how I modeled it in the prototype), or whether it should be more like Accept and Accept-Language, appended to the request headers early on. It made sense to me to treat it like a security-sensitive header that the browser should control, but I'm second guessing that after looking at the potential Fetch integration more closely.

I also don't have a strong opinion here. Treating it as non-security-sensitive seems like it may be ok to me.

@mikewest
Copy link
Member

I'm somewhat concerned that there could be sites out there already sending Accept-Signatures request headers, which could make this a breaking change?

This is a very good point. I'll dig through HTTP Archive.

I don't have a strong opinion here.
...
I also don't have a strong opinion here.

I'm kinda hoping that @annevk might. :)

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 16, 2024
As discussed in WICG/signature-based-sri#21,
this CL sketches out what an `Accept-Signatures` header might look like.

Bug: 383409575
Change-Id: I8369aa4f3ea44ebfbdcb0daab111000863e017e0
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 16, 2024
As discussed in WICG/signature-based-sri#21,
this CL sketches out what an `Accept-Signatures` header might look like.

Bug: 383409575
Change-Id: I8369aa4f3ea44ebfbdcb0daab111000863e017e0
@annevk
Copy link
Contributor

annevk commented Dec 16, 2024

I'm curious what made you second case your initial notion?

I am somewhat concerned about all these headers we are adding on the side as they contribute to the overall request size and are not properly accounted for. And while it's reasonable that it doesn't count towards the fetchLater() budget, it does contribute to the amount of headers a server can process, which in the past has led to cookie length detection attacks and such.

Perhaps what we need to do is something along these lines:

  1. Redo the accounting of how many header bytes go out and how many of those are attacker-controlled.
  2. Keep track of all these headers on the side somewhere in Fetch.
  3. Give advice to servers what their budget needs to be for cross-origin requests if they don't do CORS. Our aim at one point was 8 kibibyte, but I suspect those servers are now toast.

@mikewest
Copy link
Member

I'm curious what made you second case your initial notion?

Basically the concerns around potential existing usage @ddworken pointed to (though I found none in HTTP Archive, I worry about logged-in applications using an RFC9421-compliant scheme different than the profile proposed in this doc; I wouldn't want to break those), and consistency with Accept and Accept-Language which are both set early on (step 13/14 of fetch).

I think it makes sense to allow developers to set Accept-Signature headers. I also think it makes sense for the browser to set SRI-driven Accept-Signature headers when we hit the network, just as we do for Origin and fetch metadata. We could do both, I suppose, by unconditionally appending Accept-Signature in HTTP-network-or-cache fetch to send it alongside any signature request that application logic might have made.

I am somewhat concerned about all these headers we are adding on the side as they contribute to the overall request size and are not properly accounted for.

Yup. I share this concern (though I also share your intuition that servers that can't handle some larger-than-ideal amount of data in request headers are probably already a lost cause). For this specific header, I think we can mitigate the risk to some extent within the confines of the existing spec language. If we wanted to make key rotation possible without preflights, we unfortunately need more bytes (or to define some new and less-verbose header off to the side, which might be reasonable, but seems like an unfortunate divergence from what's written in RFC9421).

Perhaps what we need to do is something along these lines:

  1. Redo the accounting of how many header bytes go out and how many of those are attacker-controlled.
  2. Keep track of all these headers on the side somewhere in Fetch.
  3. Give advice to servers what their budget needs to be for cross-origin requests if they don't do CORS. Our aim at one point was 8 kibibyte, but I suspect those servers are now toast.

These sound like reasonable things to try doing.

aarongable pushed a commit to chromium/chromium that referenced this issue Dec 16, 2024
As discussed in WICG/signature-based-sri#21,
this CL sketches out what an `Accept-Signatures` header might look like.

Bug: 383409575
Change-Id: I8369aa4f3ea44ebfbdcb0daab111000863e017e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087895
Reviewed-by: Kenichi Ishibashi <[email protected]>
Commit-Queue: Mike West <[email protected]>
Reviewed-by: Camille Lamy <[email protected]>
Reviewed-by: Joe DeBlasio <[email protected]>
Reviewed-by: Takashi Toyoshima <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1396609}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 16, 2024
As discussed in WICG/signature-based-sri#21,
this CL sketches out what an `Accept-Signatures` header might look like.

Bug: 383409575
Change-Id: I8369aa4f3ea44ebfbdcb0daab111000863e017e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087895
Reviewed-by: Kenichi Ishibashi <[email protected]>
Commit-Queue: Mike West <[email protected]>
Reviewed-by: Camille Lamy <[email protected]>
Reviewed-by: Joe DeBlasio <[email protected]>
Reviewed-by: Takashi Toyoshima <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1396609}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 16, 2024
As discussed in WICG/signature-based-sri#21,
this CL sketches out what an `Accept-Signatures` header might look like.

Bug: 383409575
Change-Id: I8369aa4f3ea44ebfbdcb0daab111000863e017e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087895
Reviewed-by: Kenichi Ishibashi <[email protected]>
Commit-Queue: Mike West <[email protected]>
Reviewed-by: Camille Lamy <[email protected]>
Reviewed-by: Joe DeBlasio <[email protected]>
Reviewed-by: Takashi Toyoshima <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1396609}
@annevk
Copy link
Contributor

annevk commented Dec 16, 2024

Provided we do the header accounting cleanup I think I would be okay with setting Accept-Signature conditionally, similar to User-Agent. That way web developers have control (and get a preflight), but we can set it too if they haven't set it (and we don't get a preflight). I think that would accomplish your goals.

I think appending our value would just as likely confuse existing servers as they probably don't have a robust parser in place, but I suppose it's an option as well and the security implications are about the same (as by the time we append the preflight is already taken care of).

@mikewest
Copy link
Member

Ok. That sounds like a reasonable compromise. I'll sketch out some spec language around that

cc @yoavweiss, who I know has been interested in questions around the accounting for browser controlled headers (and/or the allowlisting of things like Sec-* prefixed headers) in the past.

mikewest added a commit that referenced this issue Dec 17, 2024
This patch adds initial support for setting the `Accept-Signature`
header as we've been discussing in #21. It does not yet handle the
CORS-preflight side of things, which I'll handle in a subsequent
monkey-patch.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 18, 2024
…tures`., a=testonly

Automatic update from web-platform-tests
[Signature-based SRI] Send `Accept-Signatures`.

As discussed in WICG/signature-based-sri#21,
this CL sketches out what an `Accept-Signatures` header might look like.

Bug: 383409575
Change-Id: I8369aa4f3ea44ebfbdcb0daab111000863e017e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087895
Reviewed-by: Kenichi Ishibashi <[email protected]>
Commit-Queue: Mike West <[email protected]>
Reviewed-by: Camille Lamy <[email protected]>
Reviewed-by: Joe DeBlasio <[email protected]>
Reviewed-by: Takashi Toyoshima <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1396609}

--

wpt-commits: ee6608efe7e349ddba4864fbdfc9524ee023cf60
wpt-pr: 49700
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Dec 19, 2024
…tures`., a=testonly

Automatic update from web-platform-tests
[Signature-based SRI] Send `Accept-Signatures`.

As discussed in WICG/signature-based-sri#21,
this CL sketches out what an `Accept-Signatures` header might look like.

Bug: 383409575
Change-Id: I8369aa4f3ea44ebfbdcb0daab111000863e017e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087895
Reviewed-by: Kenichi Ishibashi <[email protected]>
Commit-Queue: Mike West <[email protected]>
Reviewed-by: Camille Lamy <[email protected]>
Reviewed-by: Joe DeBlasio <[email protected]>
Reviewed-by: Takashi Toyoshima <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1396609}

--

wpt-commits: ee6608efe7e349ddba4864fbdfc9524ee023cf60
wpt-pr: 49700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants