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

Signed Headers should either mention representation metadata or forbid them #36

Open
ioggstream opened this issue Aug 29, 2018 · 12 comments · May be fixed by #72
Open

Signed Headers should either mention representation metadata or forbid them #36

ioggstream opened this issue Aug 29, 2018 · 12 comments · May be fixed by #72
Labels
enhancement New feature, extension quality Clarity, consistency, effectiveness

Comments

@ioggstream
Copy link
Contributor

ioggstream commented Aug 29, 2018

I expect

Representation metadata (Content-Type, Content-Encoding, Content-Language) should be either:

  • included in Signed headers
  • forbidden

Because: see notes

Instead

There's no clear indication about the relation between Digest and Representation modifier.

Notes

https://tools.ietf.org/html/rfc7231#section-3.1 indicates the following representations metadata

  • Content-Type
  • Content-Encoding - in requests too
  • Content-Language
  • Content-Location

As Digest must be calculated on the "selected representation" which is tied to representation metadata

We either should:

  • forbid Content-Encoding in request
  • add Content-Encoding to Signature headers

Probably, a similar reasoning should be done for Content-Language.

@ioggstream
Copy link
Contributor Author

@mnot does it sound reasonable to you?

@ioggstream ioggstream changed the title Signed Headers should either mention representation modifiers Signed Headers should either mention representation modifiers or forbid them Aug 30, 2018
@mnot
Copy link

mnot commented Aug 30, 2018

All of these headers can affect how a recipient interprets the message. Content-Type is the most obvious, of course. So I'd include them.

@msporny
Copy link
Contributor

msporny commented Aug 30, 2018

@ioggstream @mnot, yes, I agree that doing so would provide a smaller security attack surface. That said, we already have lots of deployed implementations in production (we have no idea exactly how many): #1

Would it be acceptable if we stated that it is recommended that those items are signed as well, but for a base level conforming implementation, they don't need to be? Something to this effect:

If Content-Type, Content-Encoding, Content-Language, or Content-Location are included in headers, they SHOULD be included in the list of headers that are signed.

If Content-Digest is included in the list of headers that are signed, then Content-Type, Content-Encoding, Content-Language, and Content-Location SHOULD be included as well.

Could we generalize the language to something like?

If any Content-* header exists in the message, then it SHOULD be included in the list of headers that are signed.

@dlongley
Copy link
Contributor

Most implementations verify after decoding content... so signing Content-Encoding may be tricky/the wrong thing to do.

@msporny
Copy link
Contributor

msporny commented Aug 30, 2018

signing Content-Encoding may be tricky/the wrong thing to do.

Ugh, right... In general, the spec tries not to be super prescriptive about which headers are signed because, in general, when we try to make a particular header mandatory, someone seems to appear with a legitimate reason why it doesn't work for their use case (e.g., proxies, wrong part of the processing pipeline, middle-boxes making it impossible to relay the message, etc.)

@ioggstream
Copy link
Contributor Author

ioggstream commented Aug 30, 2018

@msporny

the spec tries not to be super prescriptive about which headers are signed

That's clear. The point is:

  1. Content-{Type,Encoding,Language,Location} representation metadata
    they act on the representation (aka the content)
  2. should I trust a signed request from somebody in the middle that tampered the representation?

As banking apis relies on this spec, I'd make it as simpler as possible, but not simpler;

If Content-Type, Content-Encoding, Content-Language, or Content-Location are included in headers, they SHOULD be included in the list of headers that are signed.

This SHOULD would open the door to broken implementations that hit badly JWT. Let's make different mistakes ;)

If Content-Digest is included

Do you mean Digest, or you're thinking to another header?

If any Content-* header exists in the message, then it SHOULD be included in the list of headers that are signed.

There are various non-standard Content-* headers (eg. Content-MD5) which are not representation metadata, so I'd rather reference "all representation metadata referenced in RFC7231". @mnot knows better than me.

@ioggstream
Copy link
Contributor Author

ioggstream commented Aug 30, 2018

@dlongley afaik as Digest is a property of the selected representation:

  • MUST be calculated on the Content-Encoded payload
  • MUST be validated before decoding.

so signing Content-Encoding may be tricky/the wrong thing to do.

I understand this position, but afaik "Content-Encoding" is part of the semantics: without content-encoding I cannot know whether I downloaded a "gzipped file" or a gzip-encoded html page object.

And when signatures are involved it means you have signed different things, that is:

  • in the first case a text/html full of garbage bytes
  • in the second, a proper text/html

@ioggstream ioggstream changed the title Signed Headers should either mention representation modifiers or forbid them Signed Headers should either mention representation metadata or forbid them Aug 30, 2018
@LPardue
Copy link

LPardue commented Aug 31, 2018

We have used Signature with Instance digests and ended up asking ourselves some similar questions, I posted these to the HTTP mailing list: https://lists.w3.org/Archives/Public/ietf-http-wg/2018AprJun/0192.html

One of the salient points of that discussion was from Julien Reschke:

AFAIR, there were arguments about whether RFC 3230 actually got the
terminology right (it tries to clarify RFC 2616 but IMHO added even more
confusion).
If you are serious about this, you may want to update RFC 3230 based on
the terminology in RFC 723*.

Since then there has been great progress on updating the core HTTP documents, but I think the fundamental concerns still remain.

@ioggstream
Copy link
Contributor Author

@LPardue I had a brief discussion on updating RFC 3230 to reference "Selected Representation" here

How did it end with your Digest header?

PS: check your 19jul emails ;)

@ioggstream
Copy link
Contributor Author

@msporny probably the first step is to add all those considerations to the [Security] section.

@ioggstream
Copy link
Contributor Author

@ioggstream
Copy link
Contributor Author

ioggstream commented May 24, 2019

Most implementations verify after decoding content... so signing Content-Encoding may be tricky/the wrong thing to do.

@dlongley in https://ioggstream.github.io/draft-polli-resource-digests-http/draft-polli-resource-digests-http.html#rfc.section.3 we introduce two identity digest algoritms:

  • eg: id-sha-256=fafafafa= contains the sha256 of the representation with no content coding applied.

Still you should explicitly include Content-Encoding in the signature to protect the recipient from malicious intermediaries removing content-encoding and altering the semantic of the message.

This is now nicely explained in https://ioggstream.github.io/draft-polli-resource-digests-http/draft-polli-resource-digests-http.html#rfc.section.2

ioggstream added a commit to ioggstream/http-signatures that referenced this issue Jun 3, 2019
@ioggstream ioggstream linked a pull request Jun 3, 2019 that will close this issue
@liamdennehy liamdennehy added enhancement New feature, extension quality Clarity, consistency, effectiveness labels Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, extension quality Clarity, consistency, effectiveness
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants