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

feat: gateway support for tar.gz #9034

Closed
wants to merge 2 commits into from

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jun 10, 2022

Built on top of #9029 and adds tar.gz format.

The official MIME Type for .gz is application/gzip. However, that makes it harder for us to distinguish what type of file it is. Therefore, I appended +gzip to the MIME Type of the TAR.

License: MIT
Signed-off-by: Henrique Dias [email protected]

License: MIT
Signed-off-by: Henrique Dias <[email protected]>
@Jorropo
Copy link
Contributor

Jorropo commented Jun 10, 2022

I don't like that such CPU hogging feature (file compression) is accessible on the gateway using the default config.
I think this would be an easly exploitable DOS vector. (create a file with the same hard to compress (/dev/urandom data) 1MiB leaves, repeated over and over, since this will keep reading the data from the data store it would be mostly network free and be focused on compressing, using lots of CPU)

I would like this more if it was dynamically scaling the compression level based on the CPU load.
But I feel like this is asking for a lot.

@Jorropo
Copy link
Contributor

Jorropo commented Jun 10, 2022

Also if someone cared about that, I think they are running a gateway seriously, and if they are, they are using a reverse proxy for TLS.

Can't they just configure the reverse proxy to handle compression ?

@hacdias
Copy link
Member Author

hacdias commented Jun 10, 2022

That's a valid point @Jorropo! However, this gateway feature is also available via the public API (see also #7746), e.g.:

https://gateway.ipfs.io/api/v0/get?arg=QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG&compression=true&archive=true

I don't mind keeping #9029 only.

@hacdias hacdias mentioned this pull request Jun 10, 2022
6 tasks
@Jorropo
Copy link
Contributor

Jorropo commented Jun 10, 2022

api/v0 😞

I don't really mind if it's there but I think it should be optional. (I guess that a comment for the spec PR.)

Also I think it should be generalised.
If this gets in, there is no reason to not compress cars too. (but that a comment for opening a new issue)

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Let's park this until we agree on specs in ipfs/specs#288.
DoS concerns make this a good candidate for testing RFC process. 🥳

@hacdias when writing RFC for that PR, it would be useful to mention the concerns around .tar.gz variant in "Alternatives" and "Security" sections.

My hot take is that .tar from #9029 is fine, but .tar.gz from this PR comes with too much risk, but let's discuss that in ipfs/specs around RFC. 🙏

@lidel lidel added status/blocked Unable to be worked further until needs are met need/analysis Needs further analysis before proceeding labels Jun 14, 2022
@lidel
Copy link
Member

lidel commented Sep 30, 2022

Closing as we decided to only do TAR (compression can happen at transport layer).
Specs in ipfs/specs#288, impl. in #9029 – lets continue there 👍

@lidel lidel closed this Sep 30, 2022
@lidel lidel deleted the feat/download-tar.gz branch September 30, 2022 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/analysis Needs further analysis before proceeding status/blocked Unable to be worked further until needs are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants