Changeset View
Standalone View
source/ps/ModIo.h
- This file was added.
/* Copyright (C) 2018 Wildfire Games. | |||||
* | |||||
* Permission is hereby granted, free of charge, to any person obtaining | |||||
* a copy of this software and associated documentation files (the | |||||
* "Software"), to deal in the Software without restriction, including | |||||
* without limitation the rights to use, copy, modify, merge, publish, | |||||
* distribute, sublicense, and/or sell copies of the Software, and to | |||||
* permit persons to whom the Software is furnished to do so, subject to | |||||
* the following conditions: | |||||
* | |||||
* The above copyright notice and this permission notice shall be included | |||||
* in all copies or substantial portions of the Software. | |||||
* | |||||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, | |||||
* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF | |||||
* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. | |||||
* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY | |||||
* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, | |||||
* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE | |||||
* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | |||||
*/ | |||||
#ifndef INCLUDED_MODIO | |||||
#define INCLUDED_MODIO | |||||
#include <string> | |||||
#include <sodium/crypto_sign.h> | |||||
leper: What is this doing in the header? That is an implementation detail only. | |||||
Done Inline ActionsWhy the newline? leper: Why the newline? | |||||
Not Done Inline ActionsI wanted to separate that from the 0 A.D. headers since it is almost a library include but not quite, but that's just silly. Fixed locally. Itms: I wanted to separate that from the 0 A.D. headers since it is almost a library include but not… | |||||
#include "lib/external_libraries/curl.h" | |||||
Done Inline ActionsNo newline after this? leper: No newline after this? | |||||
Not Done Inline ActionsFixed the include conventions for all the new files. Itms: Fixed the include conventions for all the new files. | |||||
class DownloadCallbackData; | |||||
class ScriptInterface; | |||||
Done Inline Actions(ModIO?) elexis: (ModIO?) | |||||
// TODO: Allocate instance of the below two using sodium_malloc? | |||||
Done Inline ActionsIf I'm not mistaken this data is actually some ModData, since the result from the API is parsed to our own data format. This is not important as long as mod.io is the only way of retrieving it, however that means GetMods maybe should return something that can be used after g_ModIo is deleted (I'd pass a mutable reference to that function). Changing the name would clarify that. Itms: If I'm not mistaken this data is actually some `ModData`, since the result from the API is… | |||||
Not Done Inline ActionsIt is mostly ModData returned by the API (after some parsing). Storing the output from GetMods after g_ModIo stops existing leads you to issues of trying to download something that is not what you want, or might just not work. The data returned by GetMods is only valid for as long as the instance from which you got it exists, and only for that specific instance. So no. leper: It is mostly ModData returned by the API (after some parsing). Storing the output from… | |||||
struct PKStruct { | |||||
unsigned char sig_alg[2] = {}; // == "Ed" | |||||
unsigned char keynum[8] = {}; // should match the keynum in the sigstruct, else this is the wrong key | |||||
unsigned char pk[crypto_sign_PUBLICKEYBYTES] = {}; | |||||
}; | |||||
struct SigStruct { | |||||
unsigned char sig_alg[2] = {}; // "ED" (since we only support the hashed mode) | |||||
unsigned char keynum[8] = {}; // should match the keynum in the PKStruct | |||||
unsigned char sig[crypto_sign_BYTES] = {}; | |||||
}; | |||||
struct ModIoModData | |||||
{ | |||||
std::map<std::string, std::string> properties; | |||||
std::vector<std::string> dependencies; | |||||
SigStruct sig; | |||||
}; | |||||
/** | |||||
* mod.io API interfacing code. | |||||
* | |||||
* Overview | |||||
Not Done Inline ActionsThis looks very over-engineered. leper: This looks very over-engineered. | |||||
Not Done Inline ActionsYeah structs seem to be my little indulgence. Do you see a better-looking alternative? Itms: Yeah `struct`s seem to be my little indulgence. Do you see a better-looking alternative? | |||||
Not Done Inline ActionsThat it uses a struct isn't bad, that it got so big that a struct was a nice solution is what is questionable. std::pair<int, double> seems like a possibility, or use an enum there instead as that already exists. Also while there use enum class. leper: That it uses a `struct` isn't bad, that it got so big that a `struct` was a nice solution is… | |||||
Not Done Inline ActionsI'll repeat the enum class thing since it seem to have been ignored. leper: I'll repeat the `enum class` thing since it seem to have been ignored. | |||||
* | |||||
* This class interfaces with a remote API provider that returns a list of mod files. | |||||
* These can then be downloaded after some cursory checking of well-formedness of the returned | |||||
* metadata. | |||||
* Downloaded files are checked for well formedness by validating that they fit the size and hash | |||||
* indicated by the API, then we check if the file is actually signed by a trusted key, and only | |||||
* if all of that is success the file is actually possible to be loaded as a mod. | |||||
* | |||||
* Security considerations | |||||
* | |||||
* This both distrusts the loaded JS mods, and the API as much as possible. | |||||
* We do not want a malicious mod to use this to download arbitrary files, nor do we want the API | |||||
* to make us download something we have not verified. | |||||
* Therefore we only allow mods to download one of the mods returned by this class (using indices). | |||||
* | |||||
* This (mostly) necessitates parsing the API responses here, as opposed to in JS. | |||||
* One could alternatively parse the responses in a locked down JS context, but that would require | |||||
* storing that code in here, or making sure nobody can overwrite it. Also this would possibly make | |||||
* some of the needed accesses for downloading and verifying files a bit more complicated. | |||||
* | |||||
* Everything downloaded from the API has its signature verified against our public key. | |||||
* This is a requirement, as otherwise a compromise of the API would result in users installing | |||||
* possibly malicious files. | |||||
* So a compromised API can just serve old files that we signed, so in that case there would need | |||||
* to be an issue in that old file that was missed. | |||||
* | |||||
* To limit the extend to how old those files could be the signing key should be rotated | |||||
* regularly (e.g. every release). To allow old versions of the engine to still use the API | |||||
* files can be signed by both the old and the new key for some amount of time, that however | |||||
* only makes sense in case a mod is compatible with both engine versions. | |||||
* | |||||
* TODO: One should probably sign the new key with the old one to ensure a nice upgrade path, | |||||
* though that does not really make any difference since releases aren't signed either. | |||||
* And if releases are signed, then we can just rely on that instead of complicating things. | |||||
* | |||||
* Note that this does not prevent all possible attacks a package manager/update system should | |||||
* defend against. This is intentionally not an update system since proper package managers already | |||||
* exist. However there is some possible overlap in attack vectors and these should be evalutated | |||||
* whether they apply and to what extend we can fix that on our side (or how to get the API provider | |||||
* to help us do so). For a list of some possible issues see: | |||||
* https://github.com/theupdateframework/specification/blob/master/tuf-spec.md | |||||
* | |||||
* The mod.io settings are also locked down such that only mods that have been authorized by us | |||||
* show up in API queries. This is both done so that all required information (dependencies) | |||||
* are stored for the files, and that only mods that have been checked for being ok are acutally | |||||
* shown to users. | |||||
*/ | |||||
class ModIo | |||||
{ | |||||
NONCOPYABLE(ModIo); | |||||
public: | |||||
Not Done Inline ActionsWith (partially) manually assigned values for that enum one could do that with a single bit operation even. But that is just me thinking about how to make something shorter or nicer that should most likely not even exist in such a form. leper: With (partially) manually assigned values for that enum one could do that with a single bit… | |||||
ModIo(); | |||||
~ModIo(); | |||||
const std::vector<ModIoModData>& GetMods(const ScriptInterface& scriptinterface); | |||||
void DownloadMod(size_t idx); | |||||
private: | |||||
static size_t ReceiveCallback(void* buffer, size_t size, size_t nmemb, void* userp); | |||||
static size_t DownloadCallback(void* buffer, size_t size, size_t nmemb, void* userp); | |||||
CURLcode PerformRequest(const std::string& url); | |||||
static bool ParseGameIdResponse(const ScriptInterface& scriptInterface, const std::string& responseData, int& id); | |||||
static bool ParseModsResponse(const ScriptInterface& scriptInterface, const std::string& responseData, std::vector<ModIoModData>& modData, const PKStruct& pk); | |||||
static bool ParseSignature(const std::vector<std::string>& minisigs, SigStruct& sig, const PKStruct& pk); | |||||
bool ParseGameId(const ScriptInterface& scriptInterface); | |||||
bool ParseMods(const ScriptInterface& scriptInterface); | |||||
bool VerifyDownload(const OsPath& filePath, DownloadCallbackData& callbackData, const ModIoModData& modData) const; | |||||
Not Done Inline ActionsIf that is a switch statement that is meant to handle everything, then not having a default clause (which NODEFAULT is with some additional code to ensure that at runtime) would cause certain compilers to emit a warning on a switch on an enum given the proper flags. leper: If that is a `switch` statement that is meant to handle everything, then not having a `default`… | |||||
Not Done Inline ActionsYes, so this is correct? Your sentence is not clear to me. Itms: Yes, so this is correct? Your sentence is not clear to me. | |||||
Not Done Inline ActionsSeems like it. leper: Seems like it. | |||||
// Url parts | |||||
std::string m_BaseUrl; | |||||
std::string m_GamesRequest; | |||||
std::string m_GameId; | |||||
// Query parameters | |||||
Done Inline ActionsIf that handles all the cases, not having a default case is preferable since some modern compilers then emit warnings in case one is added, instead of having a runtime check like NODEFAULT. That said this function looks quite useless. leper: If that handles all the cases, not having a `default` case is preferable since some modern… | |||||
Not Done Inline ActionsI added that because of another warning I got, which I fixed differently. Itms: I added that because of another warning I got, which I fixed differently. | |||||
std::string m_ApiKey; | |||||
std::string m_IdQuery; | |||||
CURL* m_Curl; | |||||
curl_slist* m_Headers; | |||||
char m_ErrorBuffer[CURL_ERROR_SIZE]; | |||||
std::string m_ResponseData; | |||||
PKStruct m_pk; | |||||
std::vector<ModIoModData> m_ModData; | |||||
friend class TestModIo; | |||||
}; | |||||
extern ModIo* g_ModIo; | |||||
Done Inline ActionsThis has absolutely not place in a header file, this is an implementation detail only. Leaking the structure of this, both means longer compile times on updates to purely internal structures, and could incite someone to use this outside of the implementation of this class. leper: This has absolutely not place in a header file, this is an implementation detail only. Leaking… | |||||
#endif // INCLUDED_MODIO | |||||
Done Inline ActionsShould probably be removed, once this has been understood and been implemented. If that does not seem sufficient then it should be rephrased into something that is not a TODO. leper: Should probably be removed, once this has been understood and been implemented. If that does… | |||||
Done Inline ActionsHas this been done? leper: Has this been done? | |||||
Not Done Inline ActionsAs I told you I read this with great interest. Since links stick out in editors I'm sure others have/will. Itms: As I told you I read this with great interest. Since links stick out in editors I'm sure others… | |||||
Not Done Inline ActionsThen I wonder why you answered a question about having downloaded more than the total and still going that way. leper: Then I wonder why you answered a question about having downloaded more than the total and still… | |||||
Done Inline ActionsShould this be a proper doxygen comment? leper: Should this be a proper doxygen comment? |
What is this doing in the header? That is an implementation detail only.