{"thread":{"id":"45810","subject":"Proposal for missing blob support in Git repos","startedAt":"2017-04-26T22:14:12Z","lastAt":"2017-05-04T17:09:41Z","messageCount":12,"participants":["Jonathan Tan","Junio C Hamano","Brandon Williams","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"318004","messageId":"20170426221346.25337-1-jonathantanmy@google.com","threadId":"45810","inReplyTo":null,"subject":"Proposal for missing blob support in Git repos","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-26T22:13:46Z","receivedAt":"2017-04-26T22:14:12Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Here is a proposal for missing blob support in Git repos. There have\nbeen several other proposals [1] [2]; this is similar to those except\nthat I have provided a more comprehensive analysis of the changes that\nneed to be done, and have made those changes (see commit below the\nscissors line).\n\nThis proposal is limited to local handling using a user-specified hook.\nI have another proposal out [3] for what a potential default hook should\nbe - how Git on the server can serve blobs to repos that have missing\nblobs.\n\n[1] <20170304191901.9622-1-markbt@efaref.net>\n[2] <1488999039-37631-1-git-send-email-git@jeffhostetler.com>\n[3] <ffd92ad9-39fe-c76b-178d-6e3d6a425037@google.com>\n\n-- 8< --\nsha1_file, fsck: add missing blob hook support\n\nCurrently, Git does not support repos with very large numbers of blobs\nor repos that wish to minimize manipulation of certain blobs (for\nexample, because they are very large) very well, even if the user\noperates mostly on part of the repo, because Git is designed on the\nassumption that every blob referenced by a tree object is available\nsomewhere in the repo storage.\n\nAs a first step to reducing this problem, add rudimentary support for\nmissing blobs by teaching sha1_file to invoke a hook whenever a blob is\nrequested but missing, and by updating fsck to tolerate missing blobs.\nThe hook is a shell command that can be configured through \"git config\";\nthis hook takes in a list of hashes and writes (if successful) the\ncorresponding objects to the repo's local storage.\n\nThis commit does not include support for generating such a repo; neither\nhas any command (other than fsck) been modified to either tolerate\nmissing blobs (without invoking the hook) or be more efficient in\ninvoking the missing blob hook. Only a fallback is provided in the form\nof sha1_file invoking the missing blob hook when necessary.\n\nIn order to determine the code changes in sha1_file.c necessary, I\ninvestigated the following:\n (1) functions in sha1_file that take in a hash, without the user\n     regarding how the object is stored (loose or packed)\n (2) functions in sha1_file that operate on packed objects (because I\n     need to check callers that know about the loose/packed distinction\n     and operate on both differently, and ensure that they can handle\n     the concept of objects that are neither loose nor packed)\n\nFor (1), I looked through all non-static functions in sha1_file.c that\ntake in an unsigned char * parameter. The ones that are relevant, and my\nmodifications to them to resolve this problem, are:\n - sha1_object_info_extended (fixed in this commit)\n - sha1_object_info (auto-fixed by sha1_object_info_extended)\n - read_sha1_file_extended (fixed by fixing read_object)\n - read_object_with_reference (auto-fixed by read_sha1_file_extended)\n - force_object_loose (only called from builtin/pack-objects.c, which\n   already knows that at least one pack contains this object)\n - has_sha1_file_with_flags (fixed in this commit)\n - assert_sha1_type (auto-fixed by sha1_object_info)\n\nAs described in the list above, several changes have been included in\nthis commit to fix the necessary functions.\n\nFor (2), I looked through the same functions as in (1) and also\nfor_each_packed_object. The ones that are relevant are:\n - parse_pack_index\n   - http - indirectly from http_get_info_packs\n - find_pack_entry_one\n   - this searches a single pack that is provided as an argument; the\n     caller already knows (through other means) that the sought object\n     is in a specific pack\n - find_sha1_pack\n   - fast-import - appears to be an optimization to not store a\n     file if it is already in a pack\n   - http-walker - to search through a struct alt_base\n   - http-push - to search through remote packs\n - has_sha1_pack\n   - builtin/fsck - fixed in this commit\n   - builtin/count-objects - informational purposes only (check if loose\n     object is also packed)\n   - builtin/prune-packed - check if object to be pruned is packed (if\n     not, don't prune it)\n   - revision - used to exclude packed objects if requested by user\n   - diff - just for optimization\n - for_each_packed_object\n   - reachable - only to find recent objects\n   - builtin/fsck - fixed in this commit\n   - builtin/cat-file - see below\n\nAs described in the list above, builtin/fsck has been updated. I have\nleft builtin/cat-file alone; this means that cat-file\n--batch-all-objects will only operate on objects physically in the repo.\n\nSome alternative designs that I considered but rejected:\n\n - Storing a list of hashes of missing blobs, possibly with metadata\n   (much like the shallow list). Having such a list would allow for\n   things like better error messages, attaching metadata (for example,\n   file size or binary/text nature) to each blob, and configuring\n   different hooks for each blob, but it is difficult to scale to large\n   repos.\n - Adding a hook whenever a packed blob is requested, not on any blob.\n   That is, whenever we attempt to search the packfiles for a blob, if\n   it is missing (from the packfiles and from the loose object storage),\n   to invoke the hook (which must then store it as a packfile), open the\n   packfile the hook generated, and report that the blob is found in\n   that new packfile. This reduces the amount of analysis needed (in\n   that we only need to look at how packed blobs are handled), but\n   requires that the hook generate packfiles (or for sha1_file to pack\n   whatever loose objects are generated), creating one packfile for each\n   missing blob and potentially very many packfiles that must be\n   linearly searched. This may be tolerable now for repos that only have\n   a few missing blobs (for example, repos that only want to exclude\n   large blobs), and might be tolerable in the future if we have\n   batching support for the most commonly used commands, but is not\n   tolerable now for repos that exclude a large amount of blobs.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/config.txt | 10 +++++++\n builtin/fsck.c           |  6 +++++\n sha1_file.c              | 69 +++++++++++++++++++++++++++++++++++++++++++++---\n t/t3907-missing-blob.sh  | 50 +++++++++++++++++++++++++++++++++++\n 4 files changed, 131 insertions(+), 4 deletions(-)\n create mode 100644 t/t3907-missing-blob.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 475e874d5..04db83fe8 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -369,6 +369,16 @@ The default is false, except linkgit:git-clone[1] or linkgit:git-init[1]\n will probe and set core.ignoreCase true if appropriate when the repository\n is created.\n \n+core.missingBlobCommand::\n+\tIf set, whenever a blob in the local repo is attempted to be\n+\tread but is missing, invoke this shell command to generate or\n+\tobtain that blob before reporting an error. This shell command\n+\tshould take one or more hashes, each terminated by a newline, as\n+\tstandard input, and (if successful) should write the\n+\tcorresponding objects to the local repo (packed or loose).\n++\n+If set, fsck will not treat a missing blob as an error condition.\n+\n core.precomposeUnicode::\n \tThis option is only used by Mac OS implementation of Git.\n \tWhen core.precomposeUnicode=true, Git reverts the unicode decomposition\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex f76e4163a..5c60c0916 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -37,6 +37,7 @@ static int verbose;\n static int show_progress = -1;\n static int show_dangling = 1;\n static int name_objects;\n+static int missing_blob_ok;\n #define ERROR_OBJECT 01\n #define ERROR_REACHABLE 02\n #define ERROR_PACK 04\n@@ -93,6 +94,9 @@ static int fsck_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.missingblobcommand\"))\n+\t\tmissing_blob_ok = 1;\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -222,6 +226,8 @@ static void check_reachable_object(struct object *obj)\n \tif (!(obj->flags & HAS_OBJ)) {\n \t\tif (has_sha1_pack(obj->oid.hash))\n \t\t\treturn; /* it is in pack - forget about it */\n+\t\tif (missing_blob_ok && obj->type == OBJ_BLOB)\n+\t\t\treturn;\n \t\tprintf(\"missing %s %s\\n\", printable_type(obj),\n \t\t\tdescribe_object(obj));\n \t\terrors_found |= ERROR_REACHABLE;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1577e2d7d..de8f137a9 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2955,6 +2955,49 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \treturn (status < 0) ? status : 0;\n }\n \n+static char *missing_blob_command;\n+static int sha1_file_config(const char *conf_key, const char *value, void *cb)\n+{\n+\tif (!strcmp(conf_key, \"core.missingblobcommand\")) {\n+\t\tmissing_blob_command = xstrdup(value);\n+\t}\n+\treturn 0;\n+}\n+\n+static int configured;\n+static void ensure_configured(void)\n+{\n+\tif (configured)\n+\t\treturn;\n+\n+\tgit_config(sha1_file_config, NULL);\n+\tconfigured = 1;\n+}\n+\n+static void handle_missing_blob(const unsigned char *sha1)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tconst char *argv[] = {missing_blob_command, NULL};\n+\tchar input[GIT_MAX_HEXSZ + 1];\n+\n+\tmemcpy(input, sha1_to_hex(sha1), 40);\n+\tinput[40] = '\\n';\n+\n+\tcp.argv = argv;\n+\tcp.env = local_repo_env;\n+\tcp.use_shell = 1;\n+\n+\tif (pipe_command(&cp, input, sizeof(input), NULL, 0, NULL, 0)) {\n+\t\tdie(\"failed to load blob %s\", sha1_to_hex(sha1));\n+\t}\n+\n+\t/*\n+\t * The command above may have updated packfiles, so update our record\n+\t * of them.\n+\t */\n+\treprepare_packed_git();\n+}\n+\n int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)\n {\n \tstruct cached_object *co;\n@@ -2979,6 +3022,8 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \t\treturn 0;\n \t}\n \n+\tif (!has_sha1_file_with_flags(sha1, 0))\n+\t\treturn -1;\n \tif (!find_pack_entry(real, &e)) {\n \t\t/* Most likely it's a loose object. */\n \t\tif (!sha1_loose_object_info(real, oi, flags)) {\n@@ -3091,6 +3136,8 @@ static void *read_object(const unsigned char *sha1, enum object_type *type,\n \t\treturn xmemdupz(co->buf, co->size);\n \t}\n \n+\tif (!has_sha1_file_with_flags(sha1, 0))\n+\t\treturn NULL;\n \tbuf = read_packed_sha1(sha1, type, size);\n \tif (buf)\n \t\treturn buf;\n@@ -3480,17 +3527,31 @@ int has_sha1_pack(const unsigned char *sha1)\n int has_sha1_file_with_flags(const unsigned char *sha1, int flags)\n {\n \tstruct pack_entry e;\n+\tint already_retried = 0;\n \n \tif (!startup_info->have_repository)\n \t\treturn 0;\n+retry:\n \tif (find_pack_entry(sha1, &e))\n \t\treturn 1;\n \tif (has_loose_object(sha1))\n \t\treturn 1;\n-\tif (flags & HAS_SHA1_QUICK)\n-\t\treturn 0;\n-\treprepare_packed_git();\n-\treturn find_pack_entry(sha1, &e);\n+\tif (!(flags & HAS_SHA1_QUICK)) {\n+\t\treprepare_packed_git();\n+\t\tif (find_pack_entry(sha1, &e))\n+\t\t\treturn 1;\n+\t}\n+\n+\tif (!already_retried) {\n+\t\tensure_configured();\n+\t\tif (missing_blob_command) {\n+\t\t\talready_retried = 1;\n+\t\t\thandle_missing_blob(sha1);\n+\t\t\tgoto retry;\n+\t\t}\n+\t}\n+\n+\treturn 0;\n }\n \n int has_object_file(const struct object_id *oid)\ndiff --git a/t/t3907-missing-blob.sh b/t/t3907-missing-blob.sh\nnew file mode 100644\nindex 000000000..f03e53c23\n--- /dev/null\n+++ b/t/t3907-missing-blob.sh\n@@ -0,0 +1,50 @@\n+#!/bin/sh\n+\n+test_description='core.missingblobcommand option'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'sha1_object_info_extended and read_sha1_file (through git cat-file -p)' '\n+\trm -rf server client &&\n+\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tHASH=$(git hash-object server/1.t) &&\n+\n+\tgit init client &&\n+\ttest_config -C client core.missingblobcommand \\\n+\t\t\"git -C \\\"$(pwd)/server\\\" pack-objects --stdout | git unpack-objects\" &&\n+\tgit -C client cat-file -p \"$HASH\"\n+'\n+\n+test_expect_success 'has_sha1_file (through git cat-file -e)' '\n+\trm -rf server client &&\n+\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tHASH=$(git hash-object server/1.t) &&\n+\n+\tgit init client &&\n+\ttest_config -C client core.missingblobcommand \\\n+\t\t\"git -C \\\"$(pwd)/server\\\" pack-objects --stdout | git unpack-objects\" &&\n+\tgit -C client cat-file -e \"$HASH\"\n+'\n+\n+test_expect_success 'fsck' '\n+\trm -rf server client &&\n+\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tHASH=$(git hash-object server/1.t) &&\n+\techo hash is $HASH &&\n+\n+\tcp -r server client &&\n+\ttest_config -C client core.missingblobcommand \"this-command-is-not-actually-run\" &&\n+\trm client/.git/objects/$(echo $HASH | cut -c1-2)/$(echo $HASH | cut -c3-40) &&\n+\tgit -C client fsck\n+'\n+\n+test_done\n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"},{"id":"318347","messageId":"xmqqinllgrfl.fsf@gitster.mtv.corp.google.com","threadId":"45810","inReplyTo":"20170426221346.25337-1-jonathantanmy@google.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-01T03:57:18Z","receivedAt":"2017-05-01T03:57:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> In order to determine the code changes in sha1_file.c necessary, I\n> investigated the following:\n>  (1) functions in sha1_file that take in a hash, without the user\n>      regarding how the object is stored (loose or packed)\n>  (2) functions in sha1_file that operate on packed objects (because I\n>      need to check callers that know about the loose/packed distinction\n>      and operate on both differently, and ensure that they can handle\n>      the concept of objects that are neither loose nor packed)\n>\n> For (1), I looked through all non-static functions in sha1_file.c that\n> take in an unsigned char * parameter. The ones that are relevant, and my\n> modifications to them to resolve this problem, are:\n>  - sha1_object_info_extended (fixed in this commit)\n>  - sha1_object_info (auto-fixed by sha1_object_info_extended)\n>  - read_sha1_file_extended (fixed by fixing read_object)\n>  - read_object_with_reference (auto-fixed by read_sha1_file_extended)\n>  - force_object_loose (only called from builtin/pack-objects.c, which\n>    already knows that at least one pack contains this object)\n>  - has_sha1_file_with_flags (fixed in this commit)\n>  - assert_sha1_type (auto-fixed by sha1_object_info)\n>\n> As described in the list above, several changes have been included in\n> this commit to fix the necessary functions.\n>\n> For (2), I looked through the same functions as in (1) and also\n> for_each_packed_object. The ones that are relevant are:\n>  - parse_pack_index\n>    - http - indirectly from http_get_info_packs\n>  - find_pack_entry_one\n>    - this searches a single pack that is provided as an argument; the\n>      caller already knows (through other means) that the sought object\n>      is in a specific pack\n>  - find_sha1_pack\n>    - fast-import - appears to be an optimization to not store a\n>      file if it is already in a pack\n>    - http-walker - to search through a struct alt_base\n>    - http-push - to search through remote packs\n>  - has_sha1_pack\n>    - builtin/fsck - fixed in this commit\n>    - builtin/count-objects - informational purposes only (check if loose\n>      object is also packed)\n>    - builtin/prune-packed - check if object to be pruned is packed (if\n>      not, don't prune it)\n>    - revision - used to exclude packed objects if requested by user\n>    - diff - just for optimization\n>  - for_each_packed_object\n>    - reachable - only to find recent objects\n>    - builtin/fsck - fixed in this commit\n>    - builtin/cat-file - see below\n>\n> As described in the list above, builtin/fsck has been updated. I have\n> left builtin/cat-file alone; this means that cat-file\n> --batch-all-objects will only operate on objects physically in the repo.\n\nOne thing I wonder is what the performance impact of a change like\nthis to the codepath that wants to see if an object does _not_ exist\nin the repository.  When creating a new object by hashing raw data,\nwe see if an object with the same name already exists before writing\nthe compressed loose object out (or comparing the payload to detect\nhash collision).  With a \"missing blob\" support, we'd essentially\nspawn an extra process every time we want to create a new blob\nlocally, and most of the time that is done only to hear the external\ncommand to say \"no, we've never heard of such an object\", with a\npossibly large latency.\n\nIf we do not have to worry about that (or if it is no use to worry\nabout it, because we cannot avoid it if we wanted to do the lazy\nloading of objects from elsewhere), then the patch presented here\nlooked like a sensible first step towards the stated goal.\n\nThanks.\n"},{"id":"318400","messageId":"193d1d84-2386-c4c8-81ef-0042f0d8bb02@google.com","threadId":"45810","inReplyTo":"xmqqinllgrfl.fsf@gitster.mtv.corp.google.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-01T19:12:27Z","receivedAt":"2017-05-01T19:12:35Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 04/30/2017 08:57 PM, Junio C Hamano wrote:\n> One thing I wonder is what the performance impact of a change like\n> this to the codepath that wants to see if an object does _not_ exist\n> in the repository.  When creating a new object by hashing raw data,\n> we see if an object with the same name already exists before writing\n> the compressed loose object out (or comparing the payload to detect\n> hash collision).  With a \"missing blob\" support, we'd essentially\n> spawn an extra process every time we want to create a new blob\n> locally, and most of the time that is done only to hear the external\n> command to say \"no, we've never heard of such an object\", with a\n> possibly large latency.\n>\n> If we do not have to worry about that (or if it is no use to worry\n> about it, because we cannot avoid it if we wanted to do the lazy\n> loading of objects from elsewhere), then the patch presented here\n> looked like a sensible first step towards the stated goal.\n>\n> Thanks.\n\nThanks for your comments. If you're referring to the codepath involving \nwrite_sha1_file() (for example, builtin/hash-object -> index_fd or \nbuiltin/unpack-objects), that is fine because write_sha1_file() invokes \nfreshen_packed_object() and freshen_loose_object() directly to check if \nthe object already exists (and thus does not invoke the new mechanism in \nthis patch).\n\nHaving said that, looking at other parts of the fetching mechanism, \nthere are a few calls to has_sha1_file() and others that might need to \nbe checked. (We have already discussed one - the one in rev-list when \ninvoked to check connectivity.) I could take a look at that, but was \nhoping for discussion on what I've sent so far (so that I know that I'm \non the right track, and because it somewhat works, albeit slowly).\n"},{"id":"318424","messageId":"xmqq1ss8gnqn.fsf@gitster.mtv.corp.google.com","threadId":"45810","inReplyTo":"193d1d84-2386-c4c8-81ef-0042f0d8bb02@google.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-01T23:29:20Z","receivedAt":"2017-05-01T23:29:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Thanks for your comments. If you're referring to the codepath\n> involving write_sha1_file() (for example, builtin/hash-object ->\n> index_fd or builtin/unpack-objects), that is fine because\n> write_sha1_file() invokes freshen_packed_object() and\n> freshen_loose_object() directly to check if the object already exists\n> (and thus does not invoke the new mechanism in this patch).\n\nIs that a good thing, though?  It means that you an attacker can\nfeed one version to the remote object store your \"grab blob\" hook\ngets the blobs from, and have you add a colliding object locally,\nand the usual \"are we recording the same object as existing one?\"\ncheck is bypassed.\n\n"},{"id":"318434","messageId":"c0c8a0c3-582c-cf3b-3833-c918a0630f9f@google.com","threadId":"45810","inReplyTo":"xmqq1ss8gnqn.fsf@gitster.mtv.corp.google.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-02T00:33:01Z","receivedAt":"2017-05-02T00:33:08Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 05/01/2017 04:29 PM, Junio C Hamano wrote:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>> Thanks for your comments. If you're referring to the codepath\n>> involving write_sha1_file() (for example, builtin/hash-object ->\n>> index_fd or builtin/unpack-objects), that is fine because\n>> write_sha1_file() invokes freshen_packed_object() and\n>> freshen_loose_object() directly to check if the object already exists\n>> (and thus does not invoke the new mechanism in this patch).\n>\n> Is that a good thing, though?  It means that you an attacker can\n> feed one version to the remote object store your \"grab blob\" hook\n> gets the blobs from, and have you add a colliding object locally,\n> and the usual \"are we recording the same object as existing one?\"\n> check is bypassed.\n\nIf I understand this correctly, what you mean is the situation where the \nhook adds an object to the local repo, overriding another object of the \nsame name? If yes, I think that is the nature of executing an arbitrary \ncommand. If we really want to avoid that, we could drop the hook \nfunctionality (and instead, for example, provide the URL of a Git repo \ninstead from which we can communicate using a new fetch-blob protocol), \nalthough that would reduce the usefulness of this, especially during the \ntransition period in which we don't have any sort of batching of requests.\n"},{"id":"318435","messageId":"20170502003827.GB154031@google.com","threadId":"45810","inReplyTo":"c0c8a0c3-582c-cf3b-3833-c918a0630f9f@google.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T00:38:27Z","receivedAt":"2017-05-02T00:38:35Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/01, Jonathan Tan wrote:\n> On 05/01/2017 04:29 PM, Junio C Hamano wrote:\n> >Jonathan Tan <jonathantanmy@google.com> writes:\n> >\n> >>Thanks for your comments. If you're referring to the codepath\n> >>involving write_sha1_file() (for example, builtin/hash-object ->\n> >>index_fd or builtin/unpack-objects), that is fine because\n> >>write_sha1_file() invokes freshen_packed_object() and\n> >>freshen_loose_object() directly to check if the object already exists\n> >>(and thus does not invoke the new mechanism in this patch).\n> >\n> >Is that a good thing, though?  It means that you an attacker can\n> >feed one version to the remote object store your \"grab blob\" hook\n> >gets the blobs from, and have you add a colliding object locally,\n> >and the usual \"are we recording the same object as existing one?\"\n> >check is bypassed.\n> \n> If I understand this correctly, what you mean is the situation where\n> the hook adds an object to the local repo, overriding another object\n> of the same name? If yes, I think that is the nature of executing an\n> arbitrary command. If we really want to avoid that, we could drop\n> the hook functionality (and instead, for example, provide the URL of\n> a Git repo instead from which we can communicate using a new\n> fetch-blob protocol), although that would reduce the usefulness of\n> this, especially during the transition period in which we don't have\n> any sort of batching of requests.\n\nIf I understand correctly this is where we aim to be once all is said\nand done.  I guess the question is what are we willing to do during the\ntransition phase.\n\n-- \nBrandon Williams\n"},{"id":"318449","messageId":"xmqq37cof320.fsf@gitster.mtv.corp.google.com","threadId":"45810","inReplyTo":"c0c8a0c3-582c-cf3b-3833-c918a0630f9f@google.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-02T01:41:27Z","receivedAt":"2017-05-02T01:41:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> On 05/01/2017 04:29 PM, Junio C Hamano wrote:\n>> Jonathan Tan <jonathantanmy@google.com> writes:\n>>\n>>> Thanks for your comments. If you're referring to the codepath\n>>> involving write_sha1_file() (for example, builtin/hash-object ->\n>>> index_fd or builtin/unpack-objects), that is fine because\n>>> write_sha1_file() invokes freshen_packed_object() and\n>>> freshen_loose_object() directly to check if the object already exists\n>>> (and thus does not invoke the new mechanism in this patch).\n>>\n>> Is that a good thing, though?  It means that you an attacker can\n>> feed one version to the remote object store your \"grab blob\" hook\n>> gets the blobs from, and have you add a colliding object locally,\n>> and the usual \"are we recording the same object as existing one?\"\n>> check is bypassed.\n>\n> If I understand this correctly, what you mean is the situation where\n> the hook adds an object to the local repo, overriding another object\n> of the same name?\n\nNo.  \n\nwrite_sha1_file() pays attention to objects already in the local\nobject store to avoid hash collisions that can be used to replace a\nknown-to-be-good object and that is done as a security measure.\nWhat I am reading in your response was that this new mechanism\nbypasses that, and I was wondering if that is a good thing.\n\n"},{"id":"318544","messageId":"CAGf8dgK05+f4uX-8+iMFvQd0n2JP6YxJ18ag8uDaEH6qc6SgVQ@mail.gmail.com","threadId":"45810","inReplyTo":"xmqq37cof320.fsf@gitster.mtv.corp.google.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-02T17:21:59Z","receivedAt":"2017-05-02T17:22:08Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Mon, May 1, 2017 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>> On 05/01/2017 04:29 PM, Junio C Hamano wrote:\n>>> Jonathan Tan <jonathantanmy@google.com> writes:\n>>>\n>>>> Thanks for your comments. If you're referring to the codepath\n>>>> involving write_sha1_file() (for example, builtin/hash-object ->\n>>>> index_fd or builtin/unpack-objects), that is fine because\n>>>> write_sha1_file() invokes freshen_packed_object() and\n>>>> freshen_loose_object() directly to check if the object already exists\n>>>> (and thus does not invoke the new mechanism in this patch).\n>>>\n>>> Is that a good thing, though?  It means that you an attacker can\n>>> feed one version to the remote object store your \"grab blob\" hook\n>>> gets the blobs from, and have you add a colliding object locally,\n>>> and the usual \"are we recording the same object as existing one?\"\n>>> check is bypassed.\n>>\n>> If I understand this correctly, what you mean is the situation where\n>> the hook adds an object to the local repo, overriding another object\n>> of the same name?\n>\n> No.\n>\n> write_sha1_file() pays attention to objects already in the local\n> object store to avoid hash collisions that can be used to replace a\n> known-to-be-good object and that is done as a security measure.\n> What I am reading in your response was that this new mechanism\n> bypasses that, and I was wondering if that is a good thing.\n\nOh, what I meant was that write_sha1_file() bypasses the new\nmechanism, not that the new mechanism bypasses the checks in\nwrite_sha1_file().\n\nTo be clear, here's what happens when write_sha1_file() is invoked\n(before and after this patch - this patch does not affect\nwrite_sha1_file at all):\n1. (some details omitted)\n2. call freshen_packed_object\n3, call freshen_loose_object if necessary\n4. write object (if freshen_packed_object and freshen_loose_object do\nnot both return 0)\n\nNothing changes in this patch (whether a hook is defined or not).\n\nAnd here's what happens when has_sha1_file (or another function listed\nin the commit message) is invoked:\n1. check for existence of packed object of the requested name\n2. check for existence of loose object of the requested name\n3. check again for existence of packed object of the requested name\n4. if a hook is defined, invoke the hook and repeat 1-3\n\nHere, in step 4, the hook could do whatever it wants to the repository.\n"},{"id":"318553","messageId":"CACBZZX6jQtO_3zYjnvq0dhtWvUxb7vYLtQUWpFHLw1v-SteHcQ@mail.gmail.com","threadId":"45810","inReplyTo":"CAGf8dgK05+f4uX-8+iMFvQd0n2JP6YxJ18ag8uDaEH6qc6SgVQ@mail.gmail.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-02T18:32:05Z","receivedAt":"2017-05-02T18:32:32Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, May 2, 2017 at 7:21 PM, Jonathan Tan <jonathantanmy@google.com> wrote:\n> On Mon, May 1, 2017 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Jonathan Tan <jonathantanmy@google.com> writes:\n>>\n>>> On 05/01/2017 04:29 PM, Junio C Hamano wrote:\n>>>> Jonathan Tan <jonathantanmy@google.com> writes:\n>>>>\n>>>>> Thanks for your comments. If you're referring to the codepath\n>>>>> involving write_sha1_file() (for example, builtin/hash-object ->\n>>>>> index_fd or builtin/unpack-objects), that is fine because\n>>>>> write_sha1_file() invokes freshen_packed_object() and\n>>>>> freshen_loose_object() directly to check if the object already exists\n>>>>> (and thus does not invoke the new mechanism in this patch).\n>>>>\n>>>> Is that a good thing, though?  It means that you an attacker can\n>>>> feed one version to the remote object store your \"grab blob\" hook\n>>>> gets the blobs from, and have you add a colliding object locally,\n>>>> and the usual \"are we recording the same object as existing one?\"\n>>>> check is bypassed.\n>>>\n>>> If I understand this correctly, what you mean is the situation where\n>>> the hook adds an object to the local repo, overriding another object\n>>> of the same name?\n>>\n>> No.\n>>\n>> write_sha1_file() pays attention to objects already in the local\n>> object store to avoid hash collisions that can be used to replace a\n>> known-to-be-good object and that is done as a security measure.\n>> What I am reading in your response was that this new mechanism\n>> bypasses that, and I was wondering if that is a good thing.\n>\n> Oh, what I meant was that write_sha1_file() bypasses the new\n> mechanism, not that the new mechanism bypasses the checks in\n> write_sha1_file().\n>\n> To be clear, here's what happens when write_sha1_file() is invoked\n> (before and after this patch - this patch does not affect\n> write_sha1_file at all):\n> 1. (some details omitted)\n> 2. call freshen_packed_object\n> 3, call freshen_loose_object if necessary\n> 4. write object (if freshen_packed_object and freshen_loose_object do\n> not both return 0)\n>\n> Nothing changes in this patch (whether a hook is defined or not).\n\nBut don't the semantics change in the sense that before\ncore.missingBlobCommand you couldn't write a new blob SHA1 that was\nalready part of your history, whereas with this change\nwrite_sha1_file() might write what it considers to be a new blob, but\nit's actually colliding with an existing blob, but write_sha1_file()\ndoesn't know that because knowing would involve asking the hook to\nfetch the blob?\n\n> And here's what happens when has_sha1_file (or another function listed\n> in the commit message) is invoked:\n> 1. check for existence of packed object of the requested name\n> 2. check for existence of loose object of the requested name\n> 3. check again for existence of packed object of the requested name\n> 4. if a hook is defined, invoke the hook and repeat 1-3\n>\n> Here, in step 4, the hook could do whatever it wants to the repository.\n\nThis might be a bit of early bikeshedding, but then again the lack of\nearly bikeshedding tends to turn into standards.\n\nWouldn't it be better to name this core.missingObjectCommand & have\nthe hook take a list on stdin like:\n\n    <id> <TAB> <object_type> <TAB> <object_id> <TAB> <request_type> <TAB> [....]\n\nAnd have the hook respond:\n\n    <id> <TAB> <status> <TAB> [....]\n\nI.e. what you'd do now is send this to the hook:\n\n    1 <TAB> blob <TAB> <sha1> <TAB> missing\n\nAnd the hook would respond:\n\n    1 <TAB> ok\n\nBut this leaves open the door addressing this potential edge case with\nwriting new blobs in the future, i.e. write_sha1_file() could call it\nas:\n\n    1 <TAB> blob <TAB> <sha1> <TAB> new\n\nAnd the hook could either respond immediately as:\n\n    1 <TAB> ok\n\nIf it's in some #YOLO mode where it's not going to check for colliding\nblobs over the network, or alternatively & ask the parent repo if it\nhas those blobs, and if so print:\n\n    1 <TAB> collision\n\nOr something like that.\n\nThis also enables future lazy loading of trees/commits from the same\nAPI, and for the hook to respond out-of-order to the input it gets as\nit can, since each request is prefixed with an incrementing request\nid.\n"},{"id":"318568","messageId":"daed427f-b39e-c3d9-76ee-9c37203e1988@google.com","threadId":"45810","inReplyTo":"CACBZZX6jQtO_3zYjnvq0dhtWvUxb7vYLtQUWpFHLw1v-SteHcQ@mail.gmail.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-02T21:45:06Z","receivedAt":"2017-05-02T21:45:14Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 05/02/2017 11:32 AM, Ævar Arnfjörð Bjarmason wrote:\n> On Tue, May 2, 2017 at 7:21 PM, Jonathan Tan <jonathantanmy@google.com> wrote:\n>> On Mon, May 1, 2017 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Jonathan Tan <jonathantanmy@google.com> writes:\n>>>\n>>>> On 05/01/2017 04:29 PM, Junio C Hamano wrote:\n>>>>> Jonathan Tan <jonathantanmy@google.com> writes:\n>>>>>\n>>>>>> Thanks for your comments. If you're referring to the codepath\n>>>>>> involving write_sha1_file() (for example, builtin/hash-object ->\n>>>>>> index_fd or builtin/unpack-objects), that is fine because\n>>>>>> write_sha1_file() invokes freshen_packed_object() and\n>>>>>> freshen_loose_object() directly to check if the object already exists\n>>>>>> (and thus does not invoke the new mechanism in this patch).\n>>>>>\n>>>>> Is that a good thing, though?  It means that you an attacker can\n>>>>> feed one version to the remote object store your \"grab blob\" hook\n>>>>> gets the blobs from, and have you add a colliding object locally,\n>>>>> and the usual \"are we recording the same object as existing one?\"\n>>>>> check is bypassed.\n>>>>\n>>>> If I understand this correctly, what you mean is the situation where\n>>>> the hook adds an object to the local repo, overriding another object\n>>>> of the same name?\n>>>\n>>> No.\n>>>\n>>> write_sha1_file() pays attention to objects already in the local\n>>> object store to avoid hash collisions that can be used to replace a\n>>> known-to-be-good object and that is done as a security measure.\n>>> What I am reading in your response was that this new mechanism\n>>> bypasses that, and I was wondering if that is a good thing.\n>>\n>> Oh, what I meant was that write_sha1_file() bypasses the new\n>> mechanism, not that the new mechanism bypasses the checks in\n>> write_sha1_file().\n>>\n>> To be clear, here's what happens when write_sha1_file() is invoked\n>> (before and after this patch - this patch does not affect\n>> write_sha1_file at all):\n>> 1. (some details omitted)\n>> 2. call freshen_packed_object\n>> 3, call freshen_loose_object if necessary\n>> 4. write object (if freshen_packed_object and freshen_loose_object do\n>> not both return 0)\n>>\n>> Nothing changes in this patch (whether a hook is defined or not).\n>\n> But don't the semantics change in the sense that before\n> core.missingBlobCommand you couldn't write a new blob SHA1 that was\n> already part of your history,\n\nStrictly speaking, you can already do this if you don't have the blob in \nyour local repo (for example, with shallow clones - you likely wouldn't \nhave blobs pointed to by historical commits outside whatever depth is set).\n\n > whereas with this change\n> write_sha1_file() might write what it considers to be a new blob, but\n> it's actually colliding with an existing blob, but write_sha1_file()\n> doesn't know that because knowing would involve asking the hook to\n> fetch the blob?\n\nYes, this might happen.\n\nI see the semantics as \"don't write what you already have\", where \"have\" \nmeans what you have in local storage, but if you extend \"have\" to what \nupstream has, then yes, you're right that this changes (ignoring shallow \nclones).\n\nThis does remove a resistance that we have against hash collision (in \nthat normally we would have the correct object for a given hash and can \nresist other servers trying to introduce a wrong object, but now that is \nno longer the case), but I think it's better than consulting the hook \nwhenever you want to write anything (which is also a change in semantics \nin that you're consulting an external source whenever you're writing an \nobject, besides the performance implications).\n\n>> And here's what happens when has_sha1_file (or another function listed\n>> in the commit message) is invoked:\n>> 1. check for existence of packed object of the requested name\n>> 2. check for existence of loose object of the requested name\n>> 3. check again for existence of packed object of the requested name\n>> 4. if a hook is defined, invoke the hook and repeat 1-3\n>>\n>> Here, in step 4, the hook could do whatever it wants to the repository.\n>\n> This might be a bit of early bikeshedding, but then again the lack of\n> early bikeshedding tends to turn into standards.\n>\n> Wouldn't it be better to name this core.missingObjectCommand & have\n> the hook take a list on stdin like:\n>\n>     <id> <TAB> <object_type> <TAB> <object_id> <TAB> <request_type> <TAB> [....]\n>\n> And have the hook respond:\n>\n>     <id> <TAB> <status> <TAB> [....]\n>\n> I.e. what you'd do now is send this to the hook:\n>\n>     1 <TAB> blob <TAB> <sha1> <TAB> missing\n>\n> And the hook would respond:\n>\n>     1 <TAB> ok\n>\n> But this leaves open the door addressing this potential edge case with\n> writing new blobs in the future, i.e. write_sha1_file() could call it\n> as:\n>\n>     1 <TAB> blob <TAB> <sha1> <TAB> new\n>\n> And the hook could either respond immediately as:\n>\n>     1 <TAB> ok\n>\n> If it's in some #YOLO mode where it's not going to check for colliding\n> blobs over the network, or alternatively & ask the parent repo if it\n> has those blobs, and if so print:\n>\n>     1 <TAB> collision\n>\n> Or something like that.\n>\n> This also enables future lazy loading of trees/commits from the same\n> API, and for the hook to respond out-of-order to the input it gets as\n> it can, since each request is prefixed with an incrementing request\n> id.\n\nMy initial thought is that it would be better to extend hook support by \nadding configuration options for separate hooks instead of extending an \nexisting protocol. For example, with this, the hook would probably need \nto declare capabilities so that Git knows what the hook supports; also, \neven if the user and the hook have no issue with Git writing any object, \nGit would have to invoke the hook since it wouldn't know.\n\nHaving said that, I'm not too attached to the existing protocol and it \ncan change to something like what you suggested if necessary.\n"},{"id":"318721","messageId":"xmqq1ss5b5xh.fsf@gitster.mtv.corp.google.com","threadId":"45810","inReplyTo":"daed427f-b39e-c3d9-76ee-9c37203e1988@google.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-04T04:29:46Z","receivedAt":"2017-05-04T04:29:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> I see the semantics as \"don't write what you already have\", where\n> \"have\" means what you have in local storage, but if you extend \"have\"\n> to what upstream has, then yes, you're right that this changes\n> (ignoring shallow clones).\n>\n> This does remove a resistance that we have against hash collision (in\n> that normally we would have the correct object for a given hash and\n> can resist other servers trying to introduce a wrong object, but now\n> that is no longer the case), but I think it's better than consulting\n> the hook whenever you want to write anything (which is also a change\n> in semantics in that you're consulting an external source whenever\n> you're writing an object, besides the performance implications).\n\nAs long as the above pros-and-cons analysis is understood and we are\nstriking a balance between performance and strictness with such an\nunderstanding of the implications, I am perfectly fine with the\nproposal.  That is why my comment has never been \"I think that is\nwrong\" but consistently was \"I wonder if that is a good thing.\"\n\nThanks.\n"},{"id":"318794","messageId":"3abf3663-b09d-0196-e719-6cef02f01482@google.com","threadId":"45810","inReplyTo":"xmqq1ss5b5xh.fsf@gitster.mtv.corp.google.com","subject":"Re: Proposal for missing blob support in Git repos","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-04T17:09:34Z","receivedAt":"2017-05-04T17:09:41Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 05/03/2017 09:29 PM, Junio C Hamano wrote:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>> I see the semantics as \"don't write what you already have\", where\n>> \"have\" means what you have in local storage, but if you extend \"have\"\n>> to what upstream has, then yes, you're right that this changes\n>> (ignoring shallow clones).\n>>\n>> This does remove a resistance that we have against hash collision (in\n>> that normally we would have the correct object for a given hash and\n>> can resist other servers trying to introduce a wrong object, but now\n>> that is no longer the case), but I think it's better than consulting\n>> the hook whenever you want to write anything (which is also a change\n>> in semantics in that you're consulting an external source whenever\n>> you're writing an object, besides the performance implications).\n>\n> As long as the above pros-and-cons analysis is understood and we are\n> striking a balance between performance and strictness with such an\n> understanding of the implications, I am perfectly fine with the\n> proposal.  That is why my comment has never been \"I think that is\n> wrong\" but consistently was \"I wonder if that is a good thing.\"\n>\n> Thanks.\n\nNoted - if/when I update the patch, I'll include this information in.\n"}]}