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

HTTP download enhancements #222

Open
wants to merge 7 commits into
base: master
from

Conversation

@slipher
Copy link
Member

commented Aug 12, 2019

Refactor the HTTP download code with better handling of libcurl errors.
Introduce the PAKSERVER file security feature (see the comments in dl_main.cpp for explanation).

Reviewers may prefer to look at the combined (as opposed to per-commit) diffs for dl_main.cpp as I moved some stuff around multiple times.

Before merging, we should get everyone running a server with HTTP downloads enabled to add the PAKSERVER file.

slipher added some commits Aug 4, 2019

Check the return codes for most libcurl functions
Also report it as a failure if the download request returns a redirect.
Use a C++ object (with RAII etc.) to manage curl downloads
The underlying calls to libcurl are mostly the same, except that a
'multi' handle is created per request rather than globally, and it was
possible to remove the progress callback since the only part being used
was the number of bytes, which can also be determined from the write
callback.
// model, CORS headers are used to give scripts permission to access resources. We couldn't really
// implement that here since there is no origin tracking of cgame binaries.
const Str::StringRef PAKSERVER_FILE_NAME = "PAKSERVER";
const Str::StringRef PAKSERVER_FILE_CONTENT_PREFIX = "This server is configured to host Daemon engine packages";

This comment has been minimized.

Copy link
@illwieckz

illwieckz Aug 13, 2019

Member

While I'm 100% in favor of such feature, I really don't like this kind of sentence.

I think it would be better to stick to one case and to avoid whitespace (a bit like this one before the (C) sutff).

Also, I'm not sure that's needed to reference the Dæmon engine name. Like the DPK format, this kind of mechanism can be implemented by other engines. We also don't need such long string. The magic number can be far shorter. All we need is to reduce the probability that a file with the required name and the required content can appear unintentionally. Something like GRANTED sounds enough to me.

Also, why uppercase ?

This comment has been minimized.

Copy link
@slipher

slipher Aug 13, 2019

Author Member

I want the magic string to be self-documenting. As such, spaces make it easier to read and I don't see the advantage to removing them. Upper/lower case, no big deal to me. I'd be open to changing it to something that doesn't include "Daemon".

Uppercasing is one possible convention for magic filenames, to make them look distinct from the other files.

This comment has been minimized.

Copy link
@illwieckz

illwieckz Aug 13, 2019

Member

I would prefer to leverage RFC 5785 (See for example: eff.org/.well-known/dnt-policy.txt) and do something like:

.well-known/pak-server.txt

It requires an extra step (creating a directory), but that would not reinvent the wheel.

Also, we can imagine a game server provider would be able to automatically add such path as an alias (like most do with letsencrypt's .well-known/acme-challenge directory) without requiring customers to take care of it themselves.

I prefer a simple word in the file itself because of the “tutorial aspect“, the shortest and the less error prone it is, the better it is, we could tell people to do that:

mkdir -p .well-known
echo granted > .well-known/pak-server.txt

I worry about the uppercase because I'm already annoyed by the uppercase of DEPS in dpk files. When we switch from PK3 to DPK extension we had opportunity to rename it (even put it in a folder so we can easily add feature to the format), but I did not see the need for it. Now that I wrote a tool that automatically port tremulous maps by taking care of all issues including casing by lowercasing everything… I had to write a special rule in my tool to exclude README.md and… DEPS, a file we had control over. So, by default I'm a bit unpleased about uppercase files. In any way if we go the .well-known way it would be inconsistent to create an uppercase file name in a lowercase directory. I have nothing against uppercase content in anyway, but it's just require extra typing for nothing then it's more error prone, and there would be people to ask if it's required to be uppercase or not.

I know I split hairs, but that's very important to split hairs on such topic since once such things is done, we can hardly change it later and would have to live with it for better and for worse.

Edit: may be enabled is better than granted.

This comment has been minimized.

Copy link
@illwieckz

illwieckz Aug 13, 2019

Member

Hmmm, .well-known mechanism is expected to be on /, not in a subdirectory, and we can serve paks in web servers subdirectories. Maybe it's a bad idea to do .well-known stuff the way .well-known stuff was never meant…

So to sum it up, my feeling is:

  • it's not bad if it's a dot-hidden file like .pakserver (I have a strong feeling for this)
  • it's not bad if content is only one string without space, even better one word like enabled (I have strong feeling for this)
  • it's not bad if the case is consistent in name and in file (I have strong feeling for this)
  • it's not bad if it's lower case (but I would not enforce it), but the dot hidden file already makes it special without need for caps.

This comment has been minimized.

Copy link
@slipher

slipher Aug 13, 2019

Author Member

I have some concerns about using a dot name. If there are some HTTP servers that don't serve dot files by default, that could cause headaches. Also I prefer it to show up in the HTTP server's directory listing, so that someone who is setting up a server can easily imitate a working example.

I can see the point in shortening the file contents, but one word is too vague and ungoogleable. How about ALLOW_UNRESTRICTED_DOWNLOAD or some case/punctuation variant thereof?

Also, I have realized an annoying flaw in the current version of the PR, which is that if you have paks in subdirectories, you'd have to create a PAKSERVER file in all subdirectories (I didn't realize it's possible to have / in pak names when I wrote the code). So now I'm thinking of changing the download protocol to send a base URL plus a relative one, rather than a single URL. That way PAKSERVER only has to be in the base directory.

This comment has been minimized.

Copy link
@illwieckz

illwieckz Aug 13, 2019

Member

How about ALLOW_UNRESTRICTED_DOWNLOAD or some case/punctuation variant thereof?

I'm ok with ALLOW_UNRESTRICTED_DOWNLOAD, I think it's better to use same-case-plus-separator than mixing case.

At this point we may say "why not ALLOW UNRESTRICTED DOWNLOAD with spaces?", but I know that some keyboard layout put a non-breaking space character on [shift+space] (which is crazy, we all agree), that's why I'm not very happy with spaces between upper case letters.

Also, I have realized an annoying flaw in the current version of the PR, which is that if you have paks in subdirectories, you'd have to create a PAKSERVER file in all subdirectories (I didn't realize it's possible to have / in pak names when I wrote the code). So now I'm thinking of changing the download protocol to send a base URL plus a relative one, rather than a single URL. That way PAKSERVER only has to be in the base directory.

If I'm right, it's already “base directory plus relative pak path” in engine, so it's better to keep that behavior all the way down.

@illwieckz
Copy link
Member

left a comment

Oh, I thought you were removing it because of #16 (this would be better to put this string in a config file shipped with the game that to hardcode it in engine).

It doesn't work because it redirects to HTTPS. The client supports neither redirects, nor HTTPS.

That's a mistake and I'm guilty. I'll fix that 😁

If I strongly believe it's better to not hardcode this path, this path must work in any way.

@illwieckz

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

This is fixed, http://dl.unvanquished.net redirects to https, but http://dl.unvanquished.net/pkg/ is available on both http and https.

@slipher

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Removed the commit changing the default download address.

@illwieckz

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

I'm not sure you pushed your latest branch

@slipher slipher force-pushed the slipher:curls branch from 2f9dd61 to c571b7b Aug 16, 2019

@slipher

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

I'm not sure you pushed your latest branch

Fixed. Anyway, what do you think about the potential issues with dot files I mentioned above?

@illwieckz

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

If there are some HTTP servers that don't serve dot files by default, that could cause headaches. Also I prefer it to show up in the HTTP server's directory listing, so that someone who is setting up a server can easily imitate a working example.

Hmm, right, that's very strong. I just tested default apache and nginx default listing behavior and they both hide hidden dot files. I truly agree the behavior must be explicit and obvious, and for example people must be able to download the PAKSERVER file themselves to do it by imitation. So, let's go for PAKSERVER without dot and with caps.

Since PAKSERVER name is explicit, I still think the ENABLED content would be enough.

The PAKSERVER ENABLED combination looks explicit enough and self-explaining to me.

@slipher

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

Apparently there is a hosting company called Pakserver. So "pakserver enabled" might not be enough to find relevant information in a search engine...

Maybe we could call it DPKSERVER instead.

@illwieckz

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

Well, it's not like if our engine was named “Daemon” 😁

I also thought about DPKSERVER, which is not bad as it is coherent with the whole DPK strategy. I'm not sure it have to be DPK centric in any way, the engine itself being compatible with legacy PK3 and other project may mimic us to avoid the same kind of issue.

I also thought about something: our game does not fetch game content from website but it may be possible in the future to do it (the way War§ow does with blog posts), in that case it would make sense to rely on the same PAKSERVER mechanism, the PAKSERVER would not be very meaningful, but it's probably not a big issue. What is important is that it's meaningful for paks.

And yes, of course PAKistan has SERVERs.

Instead of the « server » meaning, why not something like directory, registry, library, repository, collection, or something like that?… in fact we really don't care about the server, what is important is what is served, what is available. The directory containing paks is not a server itself, it's a collection or a repository. We don't flag a server (or we would better use the .well-known mechanism at server root), we flag a directory, there may be multiple directories on the same server. The “directory” word is not the good one because of the existing pk3dir/dpkdir which has another meaning. But after some thought I think it would be better to use a name about the repository itself than a name about the server. What do you think about it?

@illwieckz

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

PAKDEPOT ? (we can even explore the « store » meaning, like in « appstore », there is a lot of words available: storage, cellar, vault…)

Of course there is already existing pakdepot entries in search engine results, same with pakstore

@slipher slipher force-pushed the slipher:curls branch from c571b7b to b88d07d Aug 20, 2019

@slipher

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

OK, I changed the message to ALLOW_UNRESTRICTED_DOWNLOAD and left the filename as PAKSERVER. It does not need to be a totally unique word as long as the content is. Is that all right @illwieckz ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.