Changeset View
Standalone View
source/ps/ModIo.cpp
- 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 | |||||
wraitii: I feel like, as it stands now, it's short enough that people would just rewrite it, and it's… | |||||
Done Inline ActionsAlso I'm generally in favour of keeping licensing simple. wraitii: Also I'm generally in favour of keeping licensing simple. | |||||
Done Inline ActionsWell, assuming someone just wants to fix up a few things (the JSON part) and include it elsewhere having this as MIT seems nicer. Also given that the full MIT license is shorter than that blurp about this being GPL and where to get the full license if it wasn't included, that seems more like keeping it actually simple. leper: Well, assuming someone just wants to fix up a few things (the JSON part) and include it… | |||||
* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | |||||
*/ | |||||
// TODO: Comment on why we handle things this way (not relying on user input, not trusting the remote too much, etc.) | |||||
// TODO: Better error messages. Also ask upstream about some special queries with partially malformed replies | |||||
#include "precompiled.h" | |||||
#include "ModIo.h" | |||||
#include "lib/file/file_system.h" | |||||
Not Done Inline ActionsWhat the heck... leper: What the heck... | |||||
#include "lib/sysdep/sysdep.h" | |||||
#include "lib/sysdep/filesystem.h" | |||||
Done Inline ActionsWhy is there a blank line between full path includes, and why are those not sorted properly? leper: Why is there a blank line between full path includes, and why are those not sorted properly? | |||||
Not Done Inline ActionsWhy is there a blank line? leper: Why is there a blank line? | |||||
Done Inline ActionsI suppose you consider that a stylistic issue, but the way includes are handled here and elsewhere seems both ridiculous and maintenance-unfriendly. What if you move one file in that same directory elsewhere? Right you need to update not only that file, but all other files that include it. As opposed to only one of those with proper full paths. leper: I suppose you consider that a stylistic issue, but the way includes are handled here and… | |||||
Not Done Inline ActionsYou are right, it is maintenance-unfriendly, I hadn't thought of this. I put that back in order and I'll change that part of the conventions. Itms: You are right, it is maintenance-unfriendly, I hadn't thought of this. I put that back in order… | |||||
Done Inline ActionsHaving the ps/**.h separated from the others still keeps this slightly unfriendly. That said, moving towards proper full paths seems a lot nicer than moving to partial paths. leper: Having the `ps/**.h` separated from the others still keeps this slightly unfriendly.
That said… | |||||
#include "maths/MD5.h" | |||||
#include "ps/CLogger.h" | |||||
#include "ps/ConfigDB.h" | |||||
#include "ps/GameSetup/Paths.h" | |||||
#include "ps/Mod.h" | |||||
#include "scriptinterface/ScriptConversions.h" | |||||
#include "scriptinterface/ScriptInterface.h" | |||||
Done Inline ActionsIs this one used for anything? Itms: Is this one used for anything? | |||||
Done Inline Actionssetfill and setw for getting the md5 digest. leper: `setfill` and `setw` for getting the md5 digest. | |||||
Done Inline ActionsAh it builds without so I was confused. Itms: Ah it builds without so I was confused. | |||||
#include <sodium.h> | |||||
#include <iomanip> | |||||
#include <boost/algorithm/string/classification.hpp> | |||||
#include <boost/algorithm/string/split.hpp> | |||||
ModIo* g_ModIo = nullptr; | |||||
// TODO: add a compile-time config so we can build without this | |||||
struct DownloadCallbackData { | |||||
Done Inline ActionsGiven the view in phabricator, this comment (and possibly others) might need some fixing so those can be nicely read on some standard size. leper: Given the view in phabricator, this comment (and possibly others) might need some fixing so… | |||||
DownloadCallbackData(FILE* _fp) | |||||
: fp(_fp), md5() | |||||
{ | |||||
crypto_generichash_init(&hash_state, NULL, 0U, crypto_generichash_BYTES_MAX); | |||||
} | |||||
FILE* fp; | |||||
MD5 md5; | |||||
crypto_generichash_state hash_state; | |||||
Done Inline ActionsI would see this in ModIo.h. Itms: I would see this in `ModIo.h`. | |||||
}; | |||||
// TODO move this comment somewhere nicer? | |||||
/** | |||||
* mod.io interfacing code. | |||||
Not Done Inline Actionsagree elexis: agree | |||||
Not Done Inline ActionsThanks, I was worried that you didn't. leper: Thanks, I was worried that you didn't. | |||||
* 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 serve us anything we have not verified. | |||||
* Therefore we only allow mods to download one of the mods returned by the API (using indices). | |||||
* Everything downloaded from the API has its signature verified against our public key. | |||||
* 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. | |||||
* 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. | |||||
*/ | |||||
ModIo::ModIo() | |||||
: m_GamesRequest("/games") | |||||
{ | |||||
Not Done Inline ActionsThe URL must be separated from the logic elexis: The URL must be separated from the logic | |||||
Not Done Inline ActionsWhy? leper: Why? | |||||
Not Done Inline ActionsHardcoding: Reducing the effort to change URLs in case of changing the home URL. A sufficiently popular mod reusing the pyrogenesis engine or a mod modifying the mod downloader might want to change or extend the mod API key and agent. elexis: Hardcoding: Reducing the effort to change URLs in case of changing the home URL. A sufficiently… | |||||
Not Done Inline Actionsman 1 sed Given that a sufficiently popular mod will still be using the same engine the agent should still be the same. If it shouldn't be then it should also not be the same engine. Unless you didn't notice something the API key is in the configuration. leper: `man 1 sed`
Given that a sufficiently popular mod will still be using the same engine the… | |||||
// Get config values from the sytem namespace, or below (default); this can be overridden on the command line. | |||||
// We do this so a malicious mod cannot change the base url and get the user to make connections | |||||
// to someone else's endpoint. | |||||
g_ConfigDB.GetValue(CFG_SYSTEM, "modio.v1.baseurl", m_BaseUrl); | |||||
// TODO: Should we allow mods to actually change the two settings below? (Might be nice for total conversions.) | |||||
{ | |||||
Done Inline ActionsComment should explain the clueless reader how using this identifier rather than the common identifier adds security. elexis: Comment should explain the clueless reader how using this identifier rather than the common… | |||||
Not Done Inline ActionsStrange how that one TODO comment above does hint that this still needs to be done. One could also note the WIP in the title, or the PoC there. Onions... leper: Strange how that one TODO comment above does hint that this still needs to be done. One could… | |||||
std::string api_key; | |||||
g_ConfigDB.GetValue(CFG_SYSTEM, "modio.v1.api_key", api_key); | |||||
m_ApiKey = "api_key=" + api_key; | |||||
} | |||||
{ | |||||
std::string nameid; | |||||
g_ConfigDB.GetValue(CFG_SYSTEM, "modio.v1.name_id", nameid); | |||||
Done Inline ActionsI think that way is good. Full total conversions are likely to be still mods of 0 A.D. on mod.io, no? Itms: I think that way is good. Full total conversions are likely to be still mods of 0 A.D. on mod. | |||||
Not Done Inline ActionsThey would most likely be, however as roughly explained in the comment (and assuming they aren't installed like mods for some strange reason) they can still change this if they really want to. I guess I'll just change this to some rationale instead of a TODO. leper: They would most likely be, however as roughly explained in the comment (and assuming they… | |||||
m_IdQuery = "name_id="+nameid; | |||||
} | |||||
Done Inline ActionsThat comment, like most others I've had in other patches, does not start with TODO: which means it should be fixed before this is merged, or in case it isn't valid anymore removed. leper: That comment, like most others I've had in other patches, does not start with `TODO: ` which… | |||||
Not Done Inline ActionsAh thanks for giving the key ^^ Itms: Ah thanks for giving the key ^^ | |||||
Not Done Inline ActionsThings not properly indented are also things that should be fixed. leper: Things not properly indented are also things that should be fixed. | |||||
// Initialise everything except Win32 sockets (because our networking | |||||
// system already inits those) | |||||
curl_global_init(CURL_GLOBAL_ALL & ~CURL_GLOBAL_WIN32); | |||||
// TODO we should only do this in one single place (same for shutdown, currently we do it here and in the userreporter) | |||||
Done Inline ActionsNow compare this error to the one below, which states "invalid public key". So in one of these ENSUREs you have what should have happened, but didn't, and in another you have what happened, but shouldn't have. At least being consistent would be nice. Though I do prefer asserts that tell one what should have happened (and are explicit in doing so). leper: Now compare this error to the one below, which states "invalid public key". So in one of these… | |||||
m_Curl = curl_easy_init(); | |||||
ENSURE(m_Curl); | |||||
// Capture error messages | |||||
curl_easy_setopt(m_Curl, CURLOPT_ERRORBUFFER, m_ErrorBuffer); | |||||
// Fail if the server did | |||||
curl_easy_setopt(m_Curl, CURLOPT_FAILONERROR, 1L); | |||||
// Disable signal handlers (required for multithreaded applications) | |||||
curl_easy_setopt(m_Curl, CURLOPT_NOSIGNAL, 1L); | |||||
// To minimise security risks, don't support redirects | |||||
curl_easy_setopt(m_Curl, CURLOPT_FOLLOWLOCATION, 0L); | |||||
m_Headers = NULL; | |||||
Done Inline ActionsI still sort of disagree with having ENSUREs (or similar) have code that should always be called in them. Not that I expect someone to just #define ENSURE(p) them away, but given a few recent developments I want to be able to say "told you so" once that happens. leper: I still sort of disagree with having `ENSURE`s (or similar) have code that should always be… | |||||
Not Done Inline ActionsSo what was wrong about the if (!check) ENSURE(0 && "message") that I had in a previous iteration? (I did consider it was just a remark, not a change request, but you explicitly said I had ignored the comment so here I am ? ) Itms: So what was wrong about the `if (!check) ENSURE(0 && "message")` that I had in a previous… | |||||
Not Done Inline ActionsIt ended up inconsistent with the other one that got changed to ENSURE(foo && "bar"); above. leper: It ended up inconsistent with the other one that got changed to `ENSURE(foo && "bar");` above. | |||||
Not Done Inline ActionsAhhhhh. I would have preferred a comment on the other ENSURE then, but whatever. Itms: Ahhhhh. I would have preferred a comment on the other `ENSURE` then, but whatever. | |||||
std::string ua = "User-Agent: pyrogenesis "; | |||||
ua += curl_version(); | |||||
ua += " (https://play0ad.com/)"; | |||||
m_Headers = curl_slist_append(m_Headers, ua.c_str()); | |||||
Not Done Inline ActionsGiven that the above ENSURE got changed to have the call inside it this should be made consistent. Not that I really consider having calls in something that could not be present with some options a good idea, since having code within ASSERT or ENSURE (or for the really ambitious UNUSED) that has any non-verifying effects is likely going to cause bugs down the road. leper: Given that the above `ENSURE` got changed to have the call inside it this should be made… | |||||
Not Done Inline ActionsThis was not fixed, see the comment a bit above. The latter part of the comment was also not really fixed or considered... leper: This was not fixed, see the comment a bit above.
The latter part of the comment was also not… | |||||
Not Done Inline ActionsNot having this fail, and fail hard at that possibly opens an issue in that the resulting state of those values allows someone to sign things and get them accepted. I repeat, this has to fail hard. leper: Not having this fail, and fail hard at that possibly opens an issue in that the resulting state… | |||||
Not Done Inline ActionsNow fails hard again and prevents the key to stay set to something that could create security concerns (the value reset prevents me from being consistent with the other ENSURE but if I understood correctly (maybe I didn't) this wasn't your actual issue). Itms: Now fails hard again and prevents the key to stay set to something that could create security… | |||||
Not Done Inline ActionsToJSVal would possibly be nicer, not that the whole thing seems that important. leper: `ToJSVal` would possibly be nicer, not that the whole thing seems that important. | |||||
// TODO more? | |||||
curl_easy_setopt(m_Curl, CURLOPT_HTTPHEADER, m_Headers); | |||||
// TODO move elsewhere (comment) | |||||
// we only accept indices from JS when downloading something later on (security) | |||||
if (sodium_init() < 0) { | |||||
ENSURE(0); // TODO | |||||
} | |||||
Not Done Inline ActionsMaybe the date, unless that is sent by curl by default. And I'd love to add a X-Clacks-Overhead but maybe as a config option... Itms: Maybe the date, unless that is sent by curl by default. And I'd love to add a `X-Clacks… | |||||
Not Done Inline ActionsIs the date needed? And no it does not. I guess the user agent construction could also be factored out since we do that in about 2 places right now. I guess adding a header there should be easy. leper: Is the date needed? And no it does not.
I guess the user agent construction could also be… | |||||
Not Done Inline ActionsSounds good! Itms: Sounds good! | |||||
Not Done Inline ActionsThat's not really a shutdown hook, but whatever. leper: That's not really a shutdown hook, but whatever. | |||||
} | |||||
ModIo::~ModIo() | |||||
{ | |||||
curl_slist_free_all(m_Headers); | |||||
curl_easy_cleanup(m_Curl); | |||||
curl_global_cleanup(); // TODO | |||||
} | |||||
Done Inline ActionsEven though every self-respecting compiler should ensure that len*size does not change, it might help the reader if that was computed once, and would fit the comment above. leper: Even though every self-respecting compiler should ensure that `len*size` does not change, it… | |||||
size_t ModIo::ReceiveCallback(void* buffer, size_t size, size_t nmemb, void* userp) | |||||
{ | |||||
ModIo* self = static_cast<ModIo*>(userp); | |||||
self->m_ResponseData += std::string((char*)buffer, (char*)buffer+size*nmemb); | |||||
return size*nmemb; | |||||
} | |||||
Done Inline Actions(bit meh, but code appears indeed better with it) elexis: (bit meh, but code appears indeed better with it) | |||||
size_t ModIo::DownloadCallback(void* buffer, size_t size, size_t nmemb, void* userp) | |||||
{ | |||||
DownloadCallbackData* data = static_cast<DownloadCallbackData*>(userp); | |||||
Done Inline ActionsThose C++ style casts seem slightly unwieldy, and this function does not even attempt to handle dlnow > dltotal. Also this could be handled by a ternary, not that it really matters. leper: Those C++ style casts seem slightly unwieldy, and this function does not even attempt to handle… | |||||
Not Done Inline ActionsI think @vladislavbelov and the doxa discourage the use of C-style casts. Do we really care if curl passes something stupid to this callback? Added the ternary. Itms: I think @vladislavbelov and the doxa discourage the use of C-style casts. Do we really care if… | |||||
Not Done Inline ActionsI'm not sure I agree with any of those, given that some of those also misuse defines meant for parameters only and break code if those aren't defined. leper: I'm not sure I agree with any of those, given that some of those also misuse defines meant for… | |||||
size_t len = fwrite(buffer, size, nmemb, data->fp); | |||||
// Only update the hash with data we actually managed to write. | |||||
// In case we did not write all of it we will fail the download, | |||||
// but we do not want to have a possibly valid hash in that case. | |||||
data->md5.Update((const u8*)buffer, len*size); | |||||
crypto_generichash_update(&data->hash_state, (const u8*)buffer, len*size); | |||||
return len*size; | |||||
} | |||||
Done Inline ActionsThis is never reset, at least when reading this diff top-down, including the rest of this function. leper: This is never reset, at least when reading this diff top-down, including the rest of this… | |||||
Not Done Inline ActionsThis was done when tearing requests down. I agree it would be clearer here so I moved it. Itms: This was done when tearing requests down. I agree it would be clearer here so I moved it. | |||||
CURLcode ModIo::PerformRequest(const std::string& url) | |||||
{ | |||||
m_ErrorBuffer[0] = '\0'; | |||||
curl_easy_setopt(m_Curl, CURLOPT_URL, url.c_str()); | |||||
return curl_easy_perform(m_Curl); | |||||
} | |||||
const std::vector<ModIoModData>& ModIo::GetMods(const ScriptInterface& scriptInterface) | |||||
{ | |||||
m_ModData.clear(); | |||||
Not Done Inline ActionsShouldn't the whole member be initialized here? leper: Shouldn't the whole member be initialized here? | |||||
Not Done Inline ActionsI could set the status here but it is alredy done elsewhere, and for non-files I can't set it here. Itms: I could set the status here but it is alredy done elsewhere, and for non-files I can't set it… | |||||
// Set IO callbacks | |||||
curl_easy_setopt(m_Curl, CURLOPT_WRITEFUNCTION, ReceiveCallback); | |||||
curl_easy_setopt(m_Curl, CURLOPT_WRITEDATA, this); | |||||
// Get the game id if we didn't fetch it already | |||||
if (m_GameId.empty()) | |||||
Not Done Inline ActionsEven though this request is likely to be fast, I'd make the GUI behave like during the mod download and display a message box with something spinning. Itms: Even though this request is likely to be fast, I'd make the GUI behave like during the mod… | |||||
Not Done Inline ActionsCurrently it displays (unless you switch windows) the warning box, not too nice, but I did mention that someone else can do the GUI. leper: Currently it displays (unless you switch windows) the warning box, not too nice, but I did… | |||||
{ | |||||
CURLcode err = PerformRequest(m_BaseUrl+m_GamesRequest+"?"+m_ApiKey+"&"+m_IdQuery); | |||||
if (err != CURLE_OK) | |||||
{ | |||||
LOGERROR("Failure while querying for game id. Server response: %s; %s", curl_easy_strerror(err), m_ErrorBuffer); | |||||
return m_ModData; | |||||
} | |||||
Not Done Inline ActionsNo, well, better than before. Doing this after the failure is ok, but seems bad, the original code would likely leave the state in some somewhat random state at least, this leaves it in a predictable state, which, while it might be hard to generate the corresponding key, is quite bad. leper: No, well, better than before.
Doing this after the failure is ok, but seems bad, the original… | |||||
Not Done Inline ActionsNah, not really, well still not, see the main comment IIRC. leper: Nah, not really, well still not, see the main comment IIRC. | |||||
if (!ParseGameId(scriptInterface)) | |||||
return m_ModData; | |||||
} | |||||
// Get the actual mods | |||||
CURLcode err = PerformRequest(m_BaseUrl+m_GamesRequest+m_GameId+"/mods?"+m_ApiKey); | |||||
if (err != CURLE_OK) | |||||
{ | |||||
LOGERROR("Failure while querying for mods. Server response: %s; %s", curl_easy_strerror(err), m_ErrorBuffer); | |||||
return m_ModData; | |||||
} | |||||
Done Inline Actionswondering if the data quantity mandates to parse the response in C++ rather than benefiting from using JS to parse JS (not only ES6 but also many more basic functions, no spidermonkey terms, more people who can write JS than C++, if modio updates their protocol, we only need to update JS, could write protocol tests in JS for instance) elexis: wondering if the data quantity mandates to parse the response in C++ rather than benefiting… | |||||
Done Inline ActionsNo, they don't. The reason we don't do something stupid like that has been explained elsewhere. But maybe one should first try to figure out why I'm doing this the hard way instead of making this a nice attack vector. leper: No, they don't. The reason we don't do something stupid like that has been explained elsewhere. | |||||
if (!ParseMods(scriptInterface)) | |||||
m_ModData.clear(); // Failed during parsing, make sure we don't provide partial data | |||||
PrintMods(); | |||||
return m_ModData; | |||||
} | |||||
#define FAIL(...) STMT(LOGERROR(__VA_ARGS__); CLEANUP(); return false;) | |||||
/** | |||||
* Parses the current content of m_ResponseData to extract m_GameId. | |||||
* | |||||
* The JSON data is expected to look like | |||||
* { "data": [{"id": 42, ...}, ...], ... } | |||||
Done Inline ActionsI'd have gone with if (error) { ... return; } /* expected code path */. leper: I'd have gone with `if (error) { ... return; } /* expected code path */`. | |||||
* where we are only interested in the value of the id property. | |||||
Done Inline Actions(didn't that already throw an error if it failed to parse?) elexis: (didn't that already throw an error if it failed to parse?) | |||||
Not Done Inline ActionsAh, right because only a single error should ever be shown. Also note that there is no TODO comment regarding error handling anywhere. I guess I should open tickets for all of those. leper: Ah, right because only a single error should ever be shown. Also note that there is no TODO… | |||||
* | |||||
* @returns true iff it successfully parsed the id. | |||||
*/ | |||||
bool ModIo::ParseGameIdResponse(const ScriptInterface& scriptInterface, const std::string& responseData, int& id) | |||||
{ | |||||
#define CLEANUP() id = -1; | |||||
JSContext* cx = scriptInterface.GetContext(); | |||||
JS::RootedValue gameResponse(cx); | |||||
if (!scriptInterface.ParseJSON(responseData, &gameResponse)) | |||||
FAIL("Failed to parse response as JSON."); | |||||
Not Done Inline ActionsI should have mentioned it earlier, but having that first parameter with a name to explain why we construct it and what is is going to be used for might be nice. Also with my C hat on not using that err variable anywhere but in the condition of the if on the following line, you could just inline that... leper: I should have mentioned it earlier, but having that first parameter with a name to explain why… | |||||
Not Done Inline ActionsThe different parts of the address are named so this looks pretty clear to me already. And no err is used to format the error message. Itms: The different parts of the address are named so this looks pretty clear to me already. And no… | |||||
//LOGWARNING("%s", m_ResponseData); | |||||
Not Done Inline ActionsI always make mistakes with this, but don't we usually omit the semicolon and call CLEANUP();? Wouldn't break the FAIL macro. Itms: I always make mistakes with this, but don't we usually omit the semicolon and call `CLEANUP();`? | |||||
Not Done Inline ActionsCould be ommitted, but doesn't really change anything. leper: Could be ommitted, but doesn't really change anything. | |||||
Not Done Inline ActionsOne should wrap it in a STMT (aka do { ... } while(0)) if that isn't considered localized enough to not really matter, might do that at some point. leper: One should wrap it in a `STMT` (aka `do { ... } while(0)`) if that isn't considered localized… | |||||
Done Inline ActionsSame as above for the if (err) { /* ... */ return; }. leper: Same as above for the `if (err) { /* ... */ return; }`. | |||||
if (!gameResponse.isObject()) | |||||
Not Done Inline Actions(wasnt there a way to merge those 2 lines?) elexis: (wasnt there a way to merge those 2 lines?) | |||||
Not Done Inline ActionsNo, there is none. The other case a few lines above is completely different. leper: No, there is none. The other case a few lines above is completely different. | |||||
FAIL("response not an object"); | |||||
JS::RootedObject gameResponseObj(cx, gameResponse.toObjectOrNull()); | |||||
JS::RootedValue dataVal(cx); | |||||
if (!JS_GetProperty(cx, gameResponseObj, "data", &dataVal)) | |||||
FAIL("data property not in response."); | |||||
// [{"id": 42, ...}, ...] | |||||
if (!dataVal.isObject()) | |||||
FAIL("data property not an object"); | |||||
JS::RootedObject data(cx, dataVal.toObjectOrNull()); | |||||
u32 length; | |||||
if (!JS_IsArrayObject(cx, data) || !JS_GetArrayLength(cx, data, &length) || !length) | |||||
FAIL("data property not an array with at least one element"); | |||||
// {"id": 42, ...} | |||||
JS::RootedValue first(cx); | |||||
if (!JS_GetElement(cx, data, 0, &first)) | |||||
FAIL("couldn't get first element."); | |||||
id = -1; | |||||
// TODO: This check currently does not really do anything if "id" is present... | |||||
// meaning we can set id to be null and we get id=0. | |||||
// There is a script value conversion check failed warning, but that does not change anything. | |||||
// TODO: We should probably make those fail (hard), if that isn't done by default (which it probably should) | |||||
// the we should add a templated variant that does. | |||||
Done Inline ActionsCLEANUP()(;) Itms: `CLEANUP()`(`;`) | |||||
Not Done Inline ActionsNo, because this does not any cleanup. This just initializes something so the below works regardless of what the user passed us. leper: No, because this does not any cleanup. This just initializes something so the below works… | |||||
Not Done Inline ActionsAh right sorry! Itms: Ah right sorry! | |||||
// TODO check if id < 0 (<=?) and if yes also fail here | |||||
// NOTE: FromJSProperty does set things to probably 0 even if there is some stupid type conversion | |||||
// So we check for <= 0, so we actually get proper results here... | |||||
// Valid ids are always > 0. | |||||
if (!ScriptInterface::FromJSProperty(cx, first, "id", id) || id <= 0) | |||||
FAIL("couldn't get id"); | |||||
LOGWARNING("id=%d", id); | |||||
return true; | |||||
#undef CLEANUP | |||||
} | |||||
Done Inline ActionsI seem to recall that this comment told about a hook at one point in time. That hook should clean up the temporary file if a download was started before the program termination. leper: I seem to recall that this comment told about a hook at one point in time. That hook should… | |||||
Not Done Inline ActionsAdded the cleanup code in the dtor. Itms: Added the cleanup code in the dtor. | |||||
bool ModIo::ParseGameId(const ScriptInterface& scriptInterface) | |||||
{ | |||||
int id = -1; | |||||
if (!ParseGameIdResponse(scriptInterface, m_ResponseData, id)) | |||||
Not Done Inline ActionsThat status seems like a lie. leper: That status seems like a lie. | |||||
Not Done Inline ActionsTrue, it is more a FAILED_DOWNLOAD_START, but there won't be any difference of behavior, and the error message makes explicit that a file I/O was the thing that failed. Itms: True, it is more a `FAILED_DOWNLOAD_START`, but there won't be any difference of behavior, and… | |||||
{ | |||||
m_ResponseData.clear(); | |||||
return false; | |||||
} | |||||
m_ResponseData.clear(); | |||||
Done Inline Actionsif (error) { /* ... */ return; } leper: `if (error) { /* ... */ return; }` | |||||
m_GameId = "/" + std::to_string(id); | |||||
return true; | |||||
} | |||||
Not Done Inline ActionsThough I guess it is just not a nice match for the issues that were deemed important for downloading mods. leper: Though I guess it is just not a nice match for the issues that were deemed important for… | |||||
/** | |||||
* Parses the current content of m_ResponseData into m_ModData. | |||||
* | |||||
Done Inline ActionsShouldn't there be a check if the mod was already downloaded? elexis: Shouldn't there be a check if the mod was already downloaded? | |||||
Not Done Inline ActionsYes, because the function is really named DownloadModOnlyIfWeDidNotDoSoAlready. leper: Yes, because the function is really named `DownloadModOnlyIfWeDidNotDoSoAlready`. | |||||
* The JSON data is expected to look like | |||||
* { data: [modobj1, modobj2, ...], ... (including result_count) } | |||||
* where modobjN has the following structure | |||||
* { homepage: "url", name: "displayname", nameid: "short-non-whitespace-name", | |||||
* summary: "short desc.", modfile: { version: "1.2.4", filename: "asdf.zip", | |||||
* filehash: { md5: "deadbeef" }, filesize: 1234, download: { binary_url: "someurl", ... } }, ... }. | |||||
* Only the listed properties are of interest to consumers, and we flatten | |||||
* the modfile structure as that simplifies handling and there are no conflicts. | |||||
*/ | |||||
bool ModIo::ParseModsResponse(const ScriptInterface& scriptInterface, const std::string& responseData, std::vector<ModIoModData>& modData) | |||||
{ | |||||
// Make sure we don't end up passing partial results back | |||||
Done Inline Actionsremove questionmark if these calls dont already complain (no remaining diskspace is one possibility) elexis: remove questionmark if these calls dont already complain (no remaining diskspace is one… | |||||
#define CLEANUP() modData.clear(); | |||||
JSContext* cx = scriptInterface.GetContext(); | |||||
JS::RootedValue modResponse(cx); | |||||
if (!scriptInterface.ParseJSON(responseData, &modResponse)) | |||||
FAIL("Failed to parse response as JSON."); | |||||
Done Inline ActionsWhen do you imagine this ENSURE to not be satisfied? leper: When do you imagine this `ENSURE` to not be satisfied? | |||||
//LOGWARNING("%s", m_ResponseData); | |||||
Done Inline Actionscould expose some of curls options as user config options elexis: could expose some of curls options as user config options | |||||
Done Inline ActionsTo mess things up for the sake of messing things up? No. leper: To mess things up for the sake of messing things up? No. | |||||
Done Inline ActionsRate limiting, disable cert check, proxies, retry times might not be useless. When considdering patches, we should explore things and decide things to be useless or say it can be done later. But declaring every curl option as messing things up preemptively seems wrong. elexis: Rate limiting, disable cert check, proxies, retry times might not be useless. When considdering… | |||||
Not Done Inline ActionsDisabling cert checks for something that installs something? Should I just open a reverse shell to make things even easier? leper: Disabling cert checks for something that installs something? Should I just open a reverse shell… | |||||
if (!modResponse.isObject()) | |||||
Not Done Inline ActionsDoes this handle all cases, if not why not? If it does, why is the default here? Nice compilers complain about missing cases already. leper: Does this handle all cases, if not why not?
If it does, why is the `default` here? Nice… | |||||
Not Done Inline ActionsYes it handles all cases where a status change is needed. I fail again to understand the default remark: in other cases (that might happen) we should do nothing here. Itms: Yes it handles all cases where a status change is needed. I fail again to understand the… | |||||
Not Done Inline ActionsA switch over an enum where all entries of the enum are handled does not need a default case. If it does that might hide compiler warnings when someone changes the entries of the enum. leper: A switch over an enum where all entries of the enum are handled does not need a default case. | |||||
Not Done Inline ActionsNot all the entries are handled, as you can see. And if the status is none of those three, we shouldn't do anything here, hence default: break;. What is wrong? Itms: Not all the entries are handled, as you can see. And if the status is none of those three, we… | |||||
FAIL("response not an object"); | |||||
JS::RootedObject modResponseObj(cx, modResponse.toObjectOrNull()); | |||||
JS::RootedValue dataVal(cx); | |||||
if (!JS_GetProperty(cx, modResponseObj, "data", &dataVal)) | |||||
FAIL("data property not in response."); | |||||
// [modobj1, modobj2, ... ] | |||||
if (!dataVal.isObject()) | |||||
FAIL("data property not an object"); | |||||
JS::RootedObject data(cx, dataVal.toObjectOrNull()); | |||||
u32 length; | |||||
if (!JS_IsArrayObject(cx, data) || !JS_GetArrayLength(cx, data, &length) || !length) | |||||
FAIL("data property not an array with at least one element"); | |||||
modData.clear(); | |||||
modData.reserve(length); | |||||
for (u32 i = 0; i < length; ++i) | |||||
{ | |||||
JS::RootedValue el(cx); | |||||
if (!JS_GetElement(cx, data, i, &el) || !el.isObject()) | |||||
Done Inline ActionsIf there is a return here, why is there an else? leper: If there is a `return` here, why is there an `else`? | |||||
FAIL("Failed to get array element object"); | |||||
modData.emplace_back(); | |||||
#define COPY_STRINGS(prefix, obj, ...) \ | |||||
for (const std::string& prop : { __VA_ARGS__ }) \ | |||||
{ \ | |||||
std::string val; \ | |||||
if (!ScriptInterface::FromJSProperty(cx, obj, prop.c_str(), val)) \ | |||||
Not Done Inline ActionsOne could do something like int in_queue; while((message = curl_multi_info_read(m_CurlMulti, &in_queue)) && message->msg == CURLMSG_CONE && message->easy_handle == m_Curl Also should it really be easy_handle? leper: One could do something like
```
int in_queue;
while((message = curl_multi_info_read(m_CurlMulti… | |||||
Not Done Inline ActionsThat looks a bit cumbersome to me, I like the do...while. Personal taste I guess. Is this a VLC joke? ? Yes, multi_handle groups several easy ones, one have to check the message comes from the one we care about. This check is not necessary as we only have one handle in the multi, but better safe than worry. Itms: That looks a bit cumbersome to me, I like the `do...while`. Personal taste I guess.
Is this a… | |||||
Not Done Inline ActionsThe message that satisfies message->msg == CURLMSG_DONE and message->easy_handle == m_Curl may not be skipped, because it is the only one that triggers the error code handling below. elexis: The message that satisfies `message->msg == CURLMSG_DONE` and `message->easy_handle == m_Curl`… | |||||
Not Done Inline ActionsCondition comes from https://gitlab.com/na-Itms/0ad/commit/cdc324f7f57bf3098c196c59a92718b1ab4d1e87
elexis: Condition comes from https://gitlab.com/na-Itms/0ad/commit/cdc324f7f57bf3098c196c59a92718b1ab4d… | |||||
FAIL("failed to get %s from %s", prop, #obj);\ | |||||
modData.back().properties.emplace(prefix+prop, val); \ | |||||
} | |||||
// TODO: Currently the homepage field does not contain a non-null value for any entry. | |||||
COPY_STRINGS("", el, "name", "name_id", "summary"); | |||||
// Now copy over the modfile part, but without the pointless substructure | |||||
JS::RootedObject elObj(cx, el.toObjectOrNull()); | |||||
// TODO handle it being null | |||||
JS::RootedValue modFile(cx); | |||||
if (!JS_GetProperty(cx, elObj, "modfile", &modFile)) | |||||
FAIL("Failed to get modfile data"); | |||||
if (!modFile.isObject()) | |||||
FAIL("modfile not an object"); | |||||
Not Done Inline ActionsWhy isn't this done immediately after the if above? As far as I can see stillRunning is never set after the curl_multi_perform call. leper: Why isn't this done immediately after the `if` above? As far as I can see `stillRunning` is… | |||||
Not Done Inline ActionsI'm not sure whether all sources of curl easy errors also decrement the multi's number of running handles. If there is an error that doesn't bring stillRunning to 0 for some reason, the error will never be processed/displayed and the function will keep returning false. Itms: I'm not sure whether **all** sources of curl easy errors also decrement the multi's number of… | |||||
// TODO: actually kill filename? (we ignore it) | |||||
COPY_STRINGS("", modFile, "version", "filename", "filesize"); | |||||
JS::RootedObject modFileObj(cx, modFile.toObjectOrNull()); | |||||
JS::RootedValue filehash(cx); | |||||
if (!JS_GetProperty(cx, modFileObj, "filehash", &filehash)) | |||||
FAIL("Failed to get filehash data"); | |||||
COPY_STRINGS("filehash_", filehash, "md5"); | |||||
JS::RootedValue download(cx); | |||||
if (!JS_GetProperty(cx, modFileObj, "download", &download)) | |||||
FAIL("Failed to get download data"); | |||||
COPY_STRINGS("", download, "binary_url"); | |||||
// Parse metadata_blob (sig+deps) | |||||
std::string metadata_blob; | |||||
if (!ScriptInterface::FromJSProperty(cx, modFile, "metadata_blob", metadata_blob)) | |||||
FAIL("failed to get metadata_blob from modFile"); | |||||
JS::RootedValue metadata(cx); | |||||
if (!scriptInterface.ParseJSON(metadata_blob, &metadata)) | |||||
FAIL("Failed to parse metadata_blob as JSON."); | |||||
if (!metadata.isObject()) | |||||
FAIL("metadata_blob not decoded as an object"); | |||||
if (!ScriptInterface::FromJSProperty(cx, metadata, "minisigs", modData.back().minisigs)) | |||||
FAIL("failed to get minisigs from metadata"); | |||||
if (!ScriptInterface::FromJSProperty(cx, metadata, "dependencies", modData.back().dependencies)) | |||||
FAIL("failed to get dependencies from metadata"); | |||||
Done Inline ActionsWhy would it? To reduce confusion about this question: What is one of the very few atomic operations on a file system? leper: Why would it?
To reduce confusion about this question: What is one of the very few atomic… | |||||
Not Done Inline ActionsSorry it used to fail before the pyromod patch was correctly hooked. Deleted the comment. Itms: Sorry it used to fail before the pyromod patch was correctly hooked. Deleted the comment. | |||||
#undef COPY_STRINGS | |||||
Done Inline ActionsMoving. Sorry about the suspense, but well, you seemed like you needed some. leper: Moving.
Sorry about the suspense, but well, you seemed like you needed some. | |||||
} | |||||
return true; | |||||
#undef CLEANUP | |||||
} | |||||
#undef FAIL | |||||
Done Inline ActionsAnd this right here seems like a spot where one could wonder why there is no default here. leper: And this right here seems like a spot where one could wonder why there is no `default` here. | |||||
bool ModIo::ParseMods(const ScriptInterface& scriptInterface) | |||||
{ | |||||
if (!ParseModsResponse(scriptInterface, m_ResponseData, m_ModData)) | |||||
{ | |||||
m_ResponseData.clear(); | |||||
return false; | |||||
} | |||||
Done Inline ActionsThis might be shorter if written as auto bar = foo(baz); baz.clear(); if (bar) return; leper: This might be shorter if written as
```auto bar = foo(baz); baz.clear(); if (bar) return;``` | |||||
m_ResponseData.clear(); | |||||
return true; | |||||
} | |||||
void ModIo::DownloadMod(size_t idx) | |||||
{ | |||||
if (idx >= m_ModData.size()) | |||||
Done Inline ActionsThe original patch had a nice sequence of checks (if !foo fail; if !bar fail; if !valid fail; everything correct, do baz), that seems completely lost here. leper: The original patch had a nice sequence of checks (if !foo fail; if !bar fail; if !valid fail… | |||||
return; | |||||
// TODO: do this asynchronously? or at least tell the gui that we are doing something -> progress bar (or something spinning) | |||||
Not Done Inline ActionsThe progress bar would be great for a file download. If asynchronously is a PITA, a GUI popup would work, with a mandatory abort button. Itms: The progress bar would be great for a file download. If asynchronously is a PITA, a GUI popup… | |||||
const Paths paths(g_args); | |||||
const OsPath modUserPath = paths.UserData()/"mods"; | |||||
Done Inline ActionsSo now we need double the free space, because placing the file in nearly the right place would be too simple? The original code meant to use the special OS handling code only. leper: So now we need double the free space, because placing the file in nearly the right place would… | |||||
const OsPath modPath = modUserPath/m_ModData[idx].properties["name_id"]; | |||||
// TODO: Always call create? | |||||
if (!DirectoryExists(modPath) && INFO::OK != CreateDirectories(modPath, 0700, false)) | |||||
return; // TODO complain? | |||||
// Name the file after the name_id, since using the filename would mean that | |||||
// we could end up with multiple zip files in the folder that might not work | |||||
// as expected for a user (since a later version might remove some files | |||||
// that aren't compatible anymore with the engine version). | |||||
// So we ignore the filename provided by the API and assume that we do not | |||||
// care about handling update.zip files. If that is the case we would need | |||||
// a way to find out what files are required by the current one and which | |||||
// should be removed for everything to work. This seems to be too complicated | |||||
// so we just do not support that usage. | |||||
// NOTE: We do save the file under a slightly different name from the final | |||||
Done Inline ActionsThis would do well with a comment. leper: This would do well with a comment. | |||||
// one, to ensure that in case a download aborts and the file stays | |||||
// around, the game will not attempt to open the file which has not | |||||
// been verified. | |||||
const OsPath filePath = modPath/(m_ModData[idx].properties["name_id"]+".zip.temp"); | |||||
DownloadCallbackData callbackData(sys_OpenFile(filePath, "w")); | |||||
if (!callbackData.fp) | |||||
return; // TODO error handling | |||||
// Set IO callbacks | |||||
Not Done Inline ActionsThis could possibly be changed to if (ret) m_GameId = ...; return ret;, but I have changed that code a few times already, and I'm still not quite happy with it. leper: This could possibly be changed to `if (ret) m_GameId = ...; return ret;`, but I have changed… | |||||
curl_easy_setopt(m_Curl, CURLOPT_WRITEFUNCTION, DownloadCallback); | |||||
curl_easy_setopt(m_Curl, CURLOPT_WRITEDATA, (void*)&callbackData); | |||||
Not Done Inline ActionsMaybe it should go to the same _modcache folder used in the pyromod patch. Itms: Maybe it should go to the same `_modcache` folder used in the pyromod patch. | |||||
Not Done Inline ActionsMaybe, though I'm not that certain where that one is, or if that location makes a lot of sense. Part of the reason for placing it nearly where it should be is that it is most likely going to be on the right filesystem already, so a move to the right name is going to be fast. (Apart from being atomic.) leper: Maybe, though I'm not that certain where that one is, or if that location makes a lot of sense. | |||||
LOGERROR("Trying to download %s", m_ModData[idx].properties["binary_url"].c_str()); | |||||
// The download link will most likely redirect elsewhere, so allow that. | |||||
// We verify the validity of the file below. | |||||
curl_easy_setopt(m_Curl, CURLOPT_FOLLOWLOCATION, 1L); | |||||
// TODO: Restrict how often we can be redirected? | |||||
// Download the file | |||||
CURLcode err = PerformRequest(m_ModData[idx].properties["binary_url"]); | |||||
fclose(callbackData.fp); | |||||
callbackData.fp = NULL; | |||||
// To minimise security risks, don't support redirects for queries | |||||
curl_easy_setopt(m_Curl, CURLOPT_FOLLOWLOCATION, 0L); | |||||
if (err != CURLE_OK) | |||||
{ | |||||
LOGERROR("Failure while downloading mod file. Server response: %s; %s", curl_easy_strerror(err), m_ErrorBuffer); | |||||
if (wunlink(filePath) != 0) | |||||
LOGERROR("Failed to delete file."); | |||||
return; | |||||
} | |||||
Done Inline ActionsAn invalid filesize is most likely only a negative value. Incorrect or wrong seem to be better choices. leper: An invalid filesize is most likely only a negative value. Incorrect or wrong seem to be better… | |||||
Done Inline ActionsHm, maybe mismatch would be another option. leper: Hm, maybe mismatch would be another option. | |||||
if (!VerifyDownload(filePath, callbackData, m_ModData[idx])) | |||||
{ | |||||
LOGERROR("File %s failed to verify.", filePath.string8()); | |||||
// delete the file again as it does not match | |||||
if (wunlink(filePath) != 0) | |||||
LOGERROR("Failed to delete file."); | |||||
return; | |||||
} | |||||
const OsPath finalFilePath = modPath/(m_ModData[idx].properties["name_id"]+".zip"); | |||||
if (wrename(filePath, finalFilePath) != 0) | |||||
{ | |||||
LOGERROR("failed to rename file."); | |||||
wunlink(filePath); | |||||
return; | |||||
} | |||||
LOGERROR("downloaded something and verified it successfully."); | |||||
// TODO: hook into .pyromod code to do the win32 specific thing because win32... | |||||
// everything else should already work, given that we placed the file in the correct location already. | |||||
} | |||||
bool ModIo::VerifyDownload(const OsPath& filePath, DownloadCallbackData& callbackData, const ModIoModData& modData) | |||||
{ | |||||
{ | |||||
u64 filesize = std::stoull(modData.properties.at("filesize")); | |||||
if (filesize != FileSize(filePath)) | |||||
{ | |||||
LOGERROR("Invalid filesize."); | |||||
return false; | |||||
} | |||||
Done Inline ActionsEverything else does work :) Itms: Everything else does work :) | |||||
} | |||||
// MD5 (because upstream provides it) | |||||
// Just used to make sure there was no obvious corruption during transfer. | |||||
{ | |||||
u8 digest[MD5::DIGESTSIZE]; | |||||
callbackData.md5.Final(digest); | |||||
std::stringstream md5digest; | |||||
md5digest << std::hex << std::setfill('0'); | |||||
for (size_t i = 0; i < MD5::DIGESTSIZE; ++i) | |||||
md5digest << std::setw(2) << (int)digest[i]; | |||||
if (modData.properties.at("filehash_md5") != md5digest.str()) | |||||
{ | |||||
LOGERROR("Invalid file. Expected md5 %s, got %s.", modData.properties.at("filehash_md5").c_str(), md5digest.str()); | |||||
return false; | |||||
} | |||||
} | |||||
// TODO XXX: Get upstream to actually have proper hashes that are collision-resistant. | |||||
// (someone might want to fix the hashes we provide for our downloads) | |||||
// (that someone might also want to add those hashes in other places than right next to the downloads) | |||||
// TODO: This comment above can be ignored, we have signing. | |||||
// Verify file signature. | |||||
// Used to make sure that the downloaded file was actually checked and signed | |||||
// by Wildfire Games. | |||||
// TODO: move rationale to a slightly better location | |||||
// Having signatures that _we_ create for the files one can retrieve from the API is | |||||
// a requirement. Else the API could just feed the users whatever in case of a compromise. | |||||
// We need signatures so we do not have to trust the API to provide files we actually want | |||||
// to be distributed. This means mods checked by us to be non-malicious only. | |||||
// If things are signed a compromised API can just continue to feed old files that might have | |||||
Done Inline ActionsDo you really think that this will be fixed at some point in time? leper: Do you really think that this will be fixed at some point in time? | |||||
Not Done Inline ActionsI created #5128. Itms: I created #5128. | |||||
// issues that nobody noticed when we signed the files. | |||||
// Note that this does not prevent all possible attacks a package manager/update system should | |||||
// defend against. This is not an update system, however other possible attack vectors should | |||||
// be evalutated if they apply and how to fix those. (See https://github.com/theupdateframework/specification/blob/master/tuf-spec.md for some ideas.) | |||||
unsigned char hash_fin[crypto_generichash_BYTES_MAX] = {}; | |||||
if (crypto_generichash_final(&callbackData.hash_state, hash_fin, sizeof hash_fin) != 0) | |||||
{ | |||||
LOGERROR("failed to compute final hash"); | |||||
Done Inline ActionsThis document is fascinating, thanks for the link. My suggestion for the better location is with the top-level comment, which might be moved to the header file. Itms: This document is fascinating, thanks for the link. My suggestion for the better location is… | |||||
Done Inline ActionsI think it could be moved there, but this specific function should retain at least some explanation of what is done and why we do it. leper: I think it could be moved there, but this specific function should retain at least some… | |||||
return false; | |||||
} | |||||
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] = {}; | |||||
}; | |||||
// TODO: is there actually a use-case where a client needs multiple keys? | |||||
// having some files signed by more than one key is nice since we can rotate keys | |||||
// and some mods might be compatible with more than a single version. | |||||
// TODO: related to that, what if one signature is valid, and another is invalid (in the actual check)? (probably consider it invalid since well wtf) | |||||
// TODO: get this from the config? | |||||
for (const std::string& pk_str : {"RWTA6VIoth2Q1PFLsRILr3G7NB+mwwO8BSGoXs63X6TQgNGM4cE8Pvd6"}) | |||||
{ | |||||
// TODO possibly allocate this using sodium_malloc (and then sodium_free) so possible decoding failures are taken care of by the heap memory protection there? | |||||
PKStruct pk; | |||||
size_t bin_len = 0; | |||||
if (sodium_base642bin((unsigned char*)&pk, sizeof pk, pk_str.c_str(), pk_str.size(), NULL, &bin_len, NULL, sodium_base64_VARIANT_ORIGINAL) != 0 || bin_len != sizeof pk) | |||||
{ | |||||
LOGERROR("failed to decode base64 key"); | |||||
continue; | |||||
} | |||||
//LOGWARNING("sig_alg='%c%c'", pk.sig_alg[0], pk.sig_alg[1]); | |||||
//LOGWARNING("keynum='%01x%01x%01x%01x%01x%01x%01x%01x'", pk.keynum[0], pk.keynum[1], pk.keynum[2], pk.keynum[3], pk.keynum[4], pk.keynum[5], pk.keynum[6], pk.keynum[7]); | |||||
for (const std::string& file_sig : modData.minisigs) | |||||
{ | |||||
// Format of a .minisig file (created using minisign(1) with -SHm file.zip) | |||||
// untrusted comment: .*\nb64sign_of_file\ntrusted comment: .*\nb64sign_of_sign_of_file_and_trusted_comment | |||||
std::vector<std::string> sig_lines; | |||||
boost::split(sig_lines, file_sig, boost::is_any_of("\n")); | |||||
if (sig_lines.size() < 4) | |||||
{ | |||||
LOGERROR("invalid (too short) sig"); | |||||
continue; | |||||
} | |||||
// We only _really_ care about the second line which is the signature of the file (b64-encoded) | |||||
// Also handling the other signature is nice, but not really required. | |||||
Done Inline ActionsFor this I suppose not indenting it is nicer so it is obvious it is related to the define. leper: For this I suppose not indenting it is nicer so it is obvious it is related to the define. | |||||
const std::string& msg_sig = sig_lines[1]; | |||||
//LOGWARNING("sig extracted: '%s'", msg_sig); | |||||
// TODO possibly allocate this using sodium_malloc (see above) | |||||
Not Done Inline ActionsMaybe just resizeing modData would be a bit nicer. leper: Maybe just `resize`ing `modData` would be a bit nicer. | |||||
SigStruct sig; | |||||
if (sodium_base642bin((unsigned char*)&sig, sizeof sig, msg_sig.c_str(), msg_sig.size(), NULL, &bin_len, NULL, sodium_base64_VARIANT_ORIGINAL) != 0 || bin_len != sizeof sig) | |||||
{ | |||||
LOGERROR("failed to decode base64 sig"); | |||||
continue; | |||||
} | |||||
//LOGWARNING("sig_alg='%c%c'", sig.sig_alg[0], sig.sig_alg[1]); | |||||
//LOGWARNING("keynum='%01x%01x%01x%01x%01x%01x%01x%01x'", sig.keynum[0], sig.keynum[1], sig.keynum[2], sig.keynum[3], sig.keynum[4], sig.keynum[5], sig.keynum[6], sig.keynum[7]); | |||||
Not Done Inline ActionsIt might be possible to enforce that this field is set, or if it isn't have it default to the mod.io url of that mod. The latter might require upstream to help out with this. leper: It might be possible to enforce that this field is set, or if it isn't have it default to the… | |||||
cassert(sizeof pk.keynum == sizeof sig.keynum); | |||||
if (memcmp(&sig.sig_alg, "ED", 2) != 0) | |||||
{ | |||||
LOGERROR("only hashed minisign signatures are supported"); | |||||
continue; | |||||
} | |||||
if (memcmp(&pk.keynum, &sig.keynum, sizeof sig.keynum) != 0) | |||||
continue; // mismatched key, try another one | |||||
if (crypto_sign_verify_detached(sig.sig, hash_fin, sizeof hash_fin, pk.pk) != 0) | |||||
{ | |||||
LOGERROR("failed to verify signature"); | |||||
return false; | |||||
} | |||||
// TODO: also check the trusted comment? (seems slightly pointless, but since we have it, why not check that nobody even tried tampering with it) | |||||
return true; | |||||
} | |||||
} | |||||
LOGERROR("did not find a matching valid signature"); | |||||
return false; | |||||
} | |||||
// TODO: nuke this at some point | |||||
void ModIo::PrintMods() | |||||
{ | |||||
printf("[\n"); | |||||
for (const auto& a : m_ModData) | |||||
{ | |||||
printf(" {\n"); | |||||
for (const auto& b : a.properties) | |||||
printf(" \"%s\": \"%s\",\n", b.first.c_str(), b.second.c_str()); | |||||
printf(" \"dependencies\": ["); | |||||
for (const auto& c : a.dependencies) | |||||
printf("\"%s\", ", c.c_str()); | |||||
printf("]\n"); | |||||
Done Inline Actionsmetadata_blob would possibly be a bit more explicit. leper: `metadata_blob` would possibly be a bit more explicit. | |||||
printf(" \"minisigs\": ["); | |||||
for (const auto& d : a.minisigs) | |||||
printf("\"%s\", ", d.c_str()); | |||||
printf("]\n"); | |||||
Done Inline ActionsSame as above. leper: Same as above. | |||||
printf(" },\n"); | |||||
} | |||||
printf("]\n"); | |||||
} | |||||
Done Inline ActionsThe resize thing above would allow using FAIL here, which might make this easier to understand. leper: The `resize` thing above would allow using `FAIL` here, which might make this easier to… | |||||
Done Inline ActionsWe shouldn't FAIL here, since we need to check the other mods, right? If I am I don't see the difference between emplace_back and resize. Itms: We shouldn't `FAIL` here, since we need to check the other mods, right? If I am I don't see the… | |||||
Done Inline ActionsRight, ignore that one. leper: Right, ignore that one. | |||||
Done Inline ActionsWhile this is indeed a TODO that starts with TODO: it should really be fixed before this is comitted. Checking a few chars against something known shouldn't be that complicated. leper: While this is indeed a `TODO` that starts with `TODO:` it should really be fixed before this is… | |||||
Not Done Inline ActionsI'd have used starts_with here, which isn't in a standard that can be relied on yet, but we do use the boost version of that somewhere already. leper: I'd have used `starts_with` here, which isn't in a standard that can be relied on yet, but we… | |||||
Done Inline ActionsDoing this in that order (that is before the continue below) could possibly cause issues if another signing algorithm is supported by some future version, and old clients could thus error out. Swapping this with the keynum check would allow working around that issue by having a new key (which new releases should do anyway) for the new version that would also have a different keynum. leper: Doing this in that order (that is before the continue below) could possibly cause issues if… | |||||
Not Done Inline ActionsThis err is not supposed to be the same as the ParseSignature one, since FAILing in ParseSignature is not supposed to make ParseModsResponse fail. So I removed propagation (the signature parsing error is actually only used in tests). Itms: This `err` is not supposed to be the same as the `ParseSignature` one, since `FAIL`ing in… |
I feel like, as it stands now, it's short enough that people would just rewrite it, and it's also somewhat coupled with 0 A.D.'s engine so MIT-ing this might be a "lie" to an extent.
But that's not a very strong feeling.