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

ETag behaviour violates RFC7232 and related issues #3868

Closed
recmo opened this issue Apr 18, 2017 · 4 comments · Fixed by #3869
Closed

ETag behaviour violates RFC7232 and related issues #3868

recmo opened this issue Apr 18, 2017 · 4 comments · Fixed by #3869
Labels
exp/novice Someone with a little familiarity can pick up kind/bug A bug in existing code (including security flaws) topic/gateway Topic gateway

Comments

@recmo
Copy link

recmo commented Apr 18, 2017

Version information:

go-ipfs version: 0.4.9-dev-db43aab
Repo version: 5
System version: amd64/linux
Golang version: go1.8.1

Type: Bug

Severity: Medium

Description:

While working on #3861, I noticed a couple of major and minor issues with the current gateway ETag behaviour:

  1. The ETag is incorrectly formatted: ETags need to be enclosed in double quotes. (RFC7232)
  2. The ETag for a request like /ipfs/QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG/readme is readme, not the hash of the content as one would like.
  3. If a If-None-Match header is provided for an /ipns/ URL, it can incorrectly return a 304 Not Modified.
  4. /ipns/ could use an ETag to improve performance (see /ipns/ should redirect to /ipfs/ and/or cache #3861).

Point 2 will break configurations where a reverse proxy rewrites requests by adding an /ipfs/hash/ prefix. If the operator changes hash, the request will incorrectly return 304.

Proposal
  1. Always use the fully resolved CID from api.ResolveNode as the ETag (also for IPNS).
  2. Format the result as a valid "Strong Validator" (double quotes around the encoded CID).

The reason to use the fully resolved CID instead of the IPFS dir root is to better support the above mentioned reverse-proxy-rewrite scenario. A single file update will only result in the ETag for this file to be changed.

Impact

This proposal will change all the ETags. This will briefly result in increased cache-misses until the new ETags are used. After that, the number of cache hits will be strictly higher.

@ghost ghost added kind/bug A bug in existing code (including security flaws) exp/novice Someone with a little familiarity can pick up topic/gateway Topic gateway labels Apr 18, 2017
@ghost
Copy link

ghost commented Apr 18, 2017

Proposal
  1. Always use the fully resolved CID from api.ResolveNode as the ETag (also for IPNS).
  2. Format the result as a valid "Strong Validator" (double quotes around the encoded CID).

Thanks, and agreed! 👍 Do you want to give it a try?

@Kubuxu
Copy link
Member

Kubuxu commented Apr 18, 2017

👍 for that

Same as @lgierth are you willing to handle it?

A single file update will only result in the ETag for this file to be changed.

This is very good idea. Go for it.

@recmo
Copy link
Author

recmo commented Apr 18, 2017

I wrote the code before I wrote the issue. I'll format it into a PR now.

recmo pushed a commit to recmo/go-ipfs that referenced this issue Apr 18, 2017
* Always use the fully resolved CID from api.ResolveNode
  as the ETag (also for IPNS).

* Format the result as a valid "Strong Validator"
  (double quotes around the encoded CID).

Fixes ipfs#3868

License: MIT
Signed-off-by: Remco Bloemen <remco@2π.com>
@Kubuxu
Copy link
Member

Kubuxu commented Apr 20, 2017

@recmo if something is not done from this issues, please extract it into separate one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/novice Someone with a little familiarity can pick up kind/bug A bug in existing code (including security flaws) topic/gateway Topic gateway
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants