{"thread":{"id":"45506","subject":"[PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","startedAt":"2017-03-24T23:24:59Z","lastAt":"2017-04-18T11:30:56Z","messageCount":23,"participants":["Johannes Schindelin","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"315347","messageId":"cover.1490397869.git.johannes.schindelin@gmx.de","threadId":"45506","inReplyTo":null,"subject":"[PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-24T23:24:42Z","receivedAt":"2017-03-24T23:24:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"As I pointed out several times in the past, the performance hit of\nenabling SHA1DC globally is not acceptable. This patch series not only\ndemonstrates that clearly in the perf test it adds (it is the last patch\nin the current series, and its commit message has some numbers), it also\nshows an early glimpse of what I will be proposing to fix the situation.\n\nObviously, this patch series is not complete yet:\n\n- the first patch is an obvious bug fix that I sent out separately, but\n it is needed to unbreak almost all tests on Windows.\n\n- the most important part will be the patch turning core.enableSHA1DC\n into a tristate: \"externalOnly\" or \"smart\" or \"auto\" or something\n indicating that it switches on collision detection only for commands\n that accept objects from an outside source into the local repository,\n such as fetch, fast-import, etc\n\n\nJohannes Schindelin (7):\n sha1dc: safeguard against outside definitions of BIGENDIAN\n Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL\n config: add the core.enablesha1dc setting\n t0013: do not skip the entire file wholesale without DC_SHA1\n t0013: test DC_AND_OPENSSL_SHA1, too\n mingw: enable DC_AND_OPENSSL_SHA1 by default\n p0013: new test to compare SHA1DC vs OpenSSL\n\n Makefile | 13 +++++++++++++\n config.c | 8 ++++++++\n config.mak.uname | 1 +\n hash.h | 2 +-\n sha1dc/sha1.c | 23 ++++++++++++++++++++++-\n sha1dc/sha1.h | 16 ++++++++++++++++\n t/helper/test-sha1.c | 10 ++++++++++\n t/perf/p0013-sha1dc.sh | 23 +++++++++++++++++++++++\n t/t0013-sha1dc.sh | 18 ++++++++++++------\n 9 files changed, 106 insertions(+), 8 deletions(-)\n create mode 100644 t/perf/p0013-sha1dc.sh\n\n\nbase-commit: 063fe858b89ef8ee27965115fd6b1ed12e42e793\nPublished-As: https://github.com/dscho/git/releases/tag/sha1dc-knob-v1\nFetch-It-Via: git fetch https://github.com/dscho/git sha1dc-knob-v1\n\n-- \n2.12.1.windows.1\n\n"},{"id":"315348","messageId":"cb494e68de5875b34fab1a0163e85c85c5dcb263.1490397869.git.johannes.schindelin@gmx.de","threadId":"45506","inReplyTo":"cover.1490397869.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/7] sha1dc: safeguard against outside definitions of BIGENDIAN","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-24T23:24:47Z","receivedAt":"2017-03-24T23:25:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In sha1dc/sha1.c, we #define BIGENDIAN under certain circumstances, and\nobviously leave the door open for scenarios where our conditions do not\ncatch and that constant is #defined elsewhere.\n\nHowever, we did not expect that anybody would possibly #define BIGENDIAN\nto 0, indicating that the current platform is *not* big endian.\n\nThis is not just a theoretical consideration: On Windows, the winsock2.h\nheader file (which is used to allow Git to communicate via network) does\nindeed do this.\n\nLet's test for that circumstance, too, and byte-swap as intended in that\ncase.\n\nThis fixes a massive breakage on Windows where current `pu` (having\nswitched on DC_SHA1 by default) breaks pretty much every single test\ncase.\n\nSigned-off-by: Johannes Schindelin \n---\n sha1dc/sha1.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\nindex 6dd0da36084..d99db4f2e1b 100644\n--- a/sha1dc/sha1.c\n+++ b/sha1dc/sha1.c\n@@ -35,7 +35,7 @@\n \n #define sha1_mix(W, t) (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14] ^ W[t - 16], 1))\n \n-#if defined(BIGENDIAN)\n+#if defined(BIGENDIAN) && BIGENDIAN != 0\n \t#define sha1_load(m, t, temp) { temp = m[t]; }\n #else\n \t#define sha1_load(m, t, temp) { temp = m[t]; sha1_bswap32(temp); }\n-- \n2.12.1.windows.1\n\n\n"},{"id":"315349","messageId":"7a2444f08dea1b2fe497ae7498eba44626414d29.1490397869.git.johannes.schindelin@gmx.de","threadId":"45506","inReplyTo":"cover.1490397869.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/7] Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-24T23:24:52Z","receivedAt":"2017-03-24T23:25:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Nowadays, there are practical collision attacks on the SHA-1 algorithm.\nFor that reason, Git integrated code that detects attempts to sneak in\nobjects using those known attack vectors and enabled it by default.\n\nThe collision detection is not for free, though: when using the SHA1DC\ncode, calculating the SHA-1 takes substantially longer than using\nOpenSSL's (in some case hardware-accelerated) SHA1-routines, and this\nhappens even when switching off the collision detection in SHA1DC's code.\n\nTherefore, it makes sense to limit the use of the collision-detecting\nSHA1DC to the cases where objects are introduced from any outside source,\nand use the fast OpenSSL code instead when working on implicitly trusted\ndata (such as when the user calls `git add`).\n\nThis patch introduces the DC_AND_OPENSSL_SHA1 Makefile knob to compile\nwith both SHA1DC and OpenSSL's SHA-1 routines, defaulting to SHA1DC. A\nlater patch will add the ability to switch between them.\n\nSigned-off-by: Johannes Schindelin \n---\n Makefile | 12 ++++++++++++\n hash.h | 2 +-\n sha1dc/sha1.c | 21 +++++++++++++++++++++\n sha1dc/sha1.h | 16 ++++++++++++++++\n 4 files changed, 50 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9f8b35ad41f..3e181d2f0e2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -147,6 +147,11 @@ all::\n # Define OPENSSL_SHA1 environment variable when running make to link\n # with the SHA1 routine from openssl library.\n #\n+# Define DC_AND_OPENSSL_SHA1 environment variable when running make to use\n+# the collision-detecting sha1 algorithm by default, configurable via the\n+# core.enableSHA1DC setting, falling back to OpenSSL's faster algorithm when\n+# the collision detection is disabled.\n+#\n # Define SHA1_MAX_BLOCK_SIZE to limit the amount of data that will be hashed\n # in one call to the platform's SHA1_Update(). e.g. APPLE_COMMON_CRYPTO\n # wants 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined.\n@@ -1391,6 +1396,12 @@ ifdef APPLE_COMMON_CRYPTO\n \tSHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L\n endif\n \n+ifdef DC_AND_OPENSSL_SHA1\n+\tEXTLIBS += $(LIB_4_CRYPTO)\n+\tLIB_OBJS += sha1dc/sha1.o\n+\tLIB_OBJS += sha1dc/ubc_check.o\n+\tBASIC_CFLAGS += -DSHA1_DC_AND_OPENSSL\n+else\n ifdef OPENSSL_SHA1\n \tEXTLIBS += $(LIB_4_CRYPTO)\n \tBASIC_CFLAGS += -DSHA1_OPENSSL\n@@ -1415,6 +1426,7 @@ endif\n endif\n endif\n endif\n+endif\n \n ifdef SHA1_MAX_BLOCK_SIZE\n \tLIB_OBJS += compat/sha1-chunked.o\ndiff --git a/hash.h b/hash.h\nindex a11fc9233fc..3a09343270d 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -7,7 +7,7 @@\n #include \n #elif defined(SHA1_OPENSSL)\n #include \n-#elif defined(SHA1_DC)\n+#elif defined(SHA1_DC) || defined(SHA1_DC_AND_OPENSSL)\n #include \"sha1dc/sha1.h\"\n #else /* SHA1_BLK */\n #include \"block-sha1/sha1.h\"\ndiff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\nindex d99db4f2e1b..e6bcf0ffa86 100644\n--- a/sha1dc/sha1.c\n+++ b/sha1dc/sha1.c\n@@ -1806,3 +1806,24 @@ void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *vdata, unsigned long len)\n \t}\n \tSHA1DCUpdate(ctx, data, len);\n }\n+\n+#ifdef SHA1_DC_AND_OPENSSL\n+void (*SHA1_Init_func)(SHA_CTX_union *ctx) = (void *)SHA1DCInit;\n+void (*SHA1_Update_func)(SHA_CTX_union *ctx, const void *pointer, size_t size) =\n+\t(void *)git_SHA1DCUpdate;\n+int (*SHA1_Final_func)(unsigned char sha1[20], SHA_CTX_union *ctx) =\n+\t(void *)git_SHA1DCFinal;\n+\n+void toggle_sha1dc(int enable)\n+{\n+\tif (enable) {\n+\t\tSHA1_Init_func = (void *)SHA1DCInit;\n+\t\tSHA1_Update_func = (void *)git_SHA1DCUpdate;\n+\t\tSHA1_Final_func = (void *)git_SHA1DCFinal;\n+\t} else {\n+\t\tSHA1_Init_func = (void *)SHA1_Init;\n+\t\tSHA1_Update_func = (void *)SHA1_Update;\n+\t\tSHA1_Final_func = (void *)SHA1_Final;\n+\t}\n+}\n+#endif\ndiff --git a/sha1dc/sha1.h b/sha1dc/sha1.h\nindex bd8bd928fb3..243c2fe0b6b 100644\n--- a/sha1dc/sha1.h\n+++ b/sha1dc/sha1.h\n@@ -110,10 +110,26 @@ void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);\n */\n void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);\n \n+#ifdef SHA1_DC_AND_OPENSSL\n+extern void toggle_sha1dc(int enable);\n+\n+typedef union {\n+\tSHA1_CTX dc;\n+\tSHA_CTX openssl;\n+} SHA_CTX_union;\n+extern void (*SHA1_Init_func)(SHA_CTX_union *ctx);\n+extern void (*SHA1_Update_func)(SHA_CTX_union *ctx, const void *data, size_t len);\n+extern int (*SHA1_Final_func)(unsigned char sha1[20], SHA_CTX_union *ctx);\n+#define platform_SHA_CTX SHA_CTX_union\n+#define platform_SHA1_Init SHA1_Init_func\n+#define platform_SHA1_Update SHA1_Update_func\n+#define platform_SHA1_Final SHA1_Final_func\n+#else\n #define platform_SHA_CTX SHA1_CTX\n #define platform_SHA1_Init SHA1DCInit\n #define platform_SHA1_Update git_SHA1DCUpdate\n #define platform_SHA1_Final git_SHA1DCFinal\n+#endif\n \n #if defined(__cplusplus)\n }\n-- \n2.12.1.windows.1\n\n\n"},{"id":"315350","messageId":"7505e660d461f2a000bdb4d540346345186f42a8.1490397869.git.johannes.schindelin@gmx.de","threadId":"45506","inReplyTo":"cover.1490397869.git.johannes.schindelin@gmx.de","subject":"[PATCH 3/7] config: add the core.enablesha1dc setting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-24T23:24:57Z","receivedAt":"2017-03-24T23:25:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When compiled with DC_AND_OPENSSL_SHA1, this new config setting allows to\nswitch from the collision-detecting SHA-1 routines (SHA1DC) to the\nnoticeably faster OpenSSL ones.\n\nThe default is still to detect collisions.\n\nSigned-off-by: Johannes Schindelin \n---\n config.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 1a4d85537b3..f94e2963e57 100644\n--- a/config.c\n+++ b/config.c\n@@ -1205,6 +1205,14 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.enablesha1dc\")) {\n+#ifdef DC_AND_OPENSSL_SHA1\n+\t\ttoggle_sha1dc(git_config_bool(var, value));\n+#else\n+\t\twarning(\"Ignoring core.enablesha1dc='%s'\", value);\n+#endif\n+\t}\n+\n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn 0;\n }\n-- \n2.12.1.windows.1\n\n\n"},{"id":"315351","messageId":"1f26e0320db1182d68663812378937e438d5c887.1490397869.git.johannes.schindelin@gmx.de","threadId":"45506","inReplyTo":"cover.1490397869.git.johannes.schindelin@gmx.de","subject":"[PATCH 4/7] t0013: do not skip the entire file wholesale without DC_SHA1","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-24T23:25:00Z","receivedAt":"2017-03-24T23:25:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"So far, there is only one test case in that script, and that case indeed\nrequires that the code was compiled with with the DC_SHA1 flag.\n\nHowever, we are about to add another test case to verify that the\nDC_AND_OPENSSL_SHA1 flag works correctly, too.\n\nSo let's refactor the code a little.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0013-sha1dc.sh | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t0013-sha1dc.sh b/t/t0013-sha1dc.sh\nindex 6d655cb161b..435a96d6108 100755\n--- a/t/t0013-sha1dc.sh\n+++ b/t/t0013-sha1dc.sh\n@@ -4,13 +4,9 @@ test_description='test sha1 collision detection'\n . ./test-lib.sh\n TEST_DATA=\"$TEST_DIRECTORY/t0013\"\n \n-if test -z \"$DC_SHA1\"\n-then\n-\tskip_all='skipping sha1 collision tests, DC_SHA1 not set'\n-\ttest_done\n-fi\n+test -z \"$DC_SHA1\" || test_set_prereq DC_SHA1\n \n-test_expect_success 'test-sha1 detects shattered pdf' '\n+test_expect_success DC_SHA1 'test-sha1 detects shattered pdf' '\n \ttest_must_fail test-sha1 <\"$TEST_DATA/shattered-1.pdf\" 2>err &&\n \ttest_i18ngrep collision err &&\n \tgrep 38762cf7f55934b34d179ae6a4c80cadccbb7f0a err\n-- \n2.12.1.windows.1\n\n\n"},{"id":"315352","messageId":"2187bcec8b1e2c61b801a04e05f58d87f683cbcf.1490397869.git.johannes.schindelin@gmx.de","threadId":"45506","inReplyTo":"cover.1490397869.git.johannes.schindelin@gmx.de","subject":"[PATCH 5/7] t0013: test DC_AND_OPENSSL_SHA1, too","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-24T23:25:04Z","receivedAt":"2017-03-24T23:25:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Signed-off-by: Johannes Schindelin \n---\n Makefile | 1 +\n t/helper/test-sha1.c | 10 ++++++++++\n t/t0013-sha1dc.sh | 10 ++++++++++\n 3 files changed, 21 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 3e181d2f0e2..0b581357625 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2251,6 +2251,7 @@ GIT-BUILD-OPTIONS: FORCE\n \t@echo NO_UNIX_SOCKETS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_UNIX_SOCKETS)))'\\' >>$@+\n \t@echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n \t@echo DC_SHA1=\\''$(subst ','\\'',$(subst ','\\'',$(DC_SHA1)))'\\' >>$@+\n+\t@echo DC_AND_OPENSSL_SHA1=\\''$(subst ','\\'',$(subst ','\\'',$(DC_AND_OPENSSL_SHA1)))'\\' >>$@+\n ifdef TEST_OUTPUT_DIRECTORY\n \t@echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n endif\ndiff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c\nindex a1c13f54eca..27ce8869e51 100644\n--- a/t/helper/test-sha1.c\n+++ b/t/helper/test-sha1.c\n@@ -8,6 +8,16 @@ int cmd_main(int ac, const char **av)\n \tint binary = 0;\n \tchar *buffer;\n \n+\tif (ac > 1 && !strcmp(av[1], \"--disable-sha1dc\")) {\n+#ifdef SHA1_DC_AND_OPENSSL\n+\t\ttoggle_sha1dc(0);\n+#else\n+\t\tdie(\"Not compiled with DC_AND_OPENSSL_SHA1\");\n+#endif\n+\t\tac--;\n+\t\tav++;\n+\t}\n+\n \tif (ac == 2) {\n \t\tif (!strcmp(av[1], \"-b\"))\n \t\t\tbinary = 1;\ndiff --git a/t/t0013-sha1dc.sh b/t/t0013-sha1dc.sh\nindex 435a96d6108..2b529b31b4e 100755\n--- a/t/t0013-sha1dc.sh\n+++ b/t/t0013-sha1dc.sh\n@@ -5,6 +5,10 @@ test_description='test sha1 collision detection'\n TEST_DATA=\"$TEST_DIRECTORY/t0013\"\n \n test -z \"$DC_SHA1\" || test_set_prereq DC_SHA1\n+test -z \"$DC_AND_OPENSSL_SHA1\" || {\n+\ttest_set_prereq DC_AND_OPENSSL_SHA1\n+\ttest_set_prereq DC_SHA1\n+}\n \n test_expect_success DC_SHA1 'test-sha1 detects shattered pdf' '\n \ttest_must_fail test-sha1 <\"$TEST_DATA/shattered-1.pdf\" 2>err &&\n@@ -12,4 +16,10 @@ test_expect_success DC_SHA1 'test-sha1 detects shattered pdf' '\n \tgrep 38762cf7f55934b34d179ae6a4c80cadccbb7f0a err\n '\n \n+test_expect_success DC_AND_OPENSSL_SHA1 'sha1dc can be turned off' '\n+\ttest-sha1 --disable-sha1dc <\"$TEST_DATA/shattered-1.pdf\" 2>err &&\n+\t! test_i18ngrep collision err &&\n+\t! grep 38762cf7f55934b34d179ae6a4c80cadccbb7f0a err\n+'\n+\n test_done\n-- \n2.12.1.windows.1\n\n\n"},{"id":"315354","messageId":"6582a2da7a322dd309733933803d714db8e8fff9.1490397869.git.johannes.schindelin@gmx.de","threadId":"45506","inReplyTo":"cover.1490397869.git.johannes.schindelin@gmx.de","subject":"[PATCH 6/7] mingw: enable DC_AND_OPENSSL_SHA1 by default","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-24T23:28:39Z","receivedAt":"2017-03-24T23:28:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Signed-off-by: Johannes Schindelin \n---\n config.mak.uname | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 399fe192719..a16e9ef0551 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -552,6 +552,7 @@ else\n \t\tUSE_LIBPCRE= YesPlease\n \t\tNO_CURL =\n \t\tUSE_NED_ALLOCATOR = YesPlease\n+\t\tDC_AND_OPENSSL_SHA1 = YesPlease\n \telse\n \t\tCOMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO\n \t\tNO_CURL = YesPlease\n-- \n2.12.1.windows.1\n\n\n"},{"id":"315355","messageId":"4c2e3b2abc84a4b4eb7b41b424f76defda550646.1490397869.git.johannes.schindelin@gmx.de","threadId":"45506","inReplyTo":"cover.1490397869.git.johannes.schindelin@gmx.de","subject":"[PATCH 7/7] p0013: new test to compare SHA1DC vs OpenSSL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-24T23:28:44Z","receivedAt":"2017-03-24T23:28:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To demonstrate the need for the core.enableSHA1DC knob, this test\ncompares the performance of the SHA-1 algorithms with collision\ndetection vs OpenSSL's (that does not detect attempted collision\nattacks).\n\nThe payload size of 300MB was actually not concocted from thin air, but\nis based on the massive Windows monorepo, whose index file weighs in\nwith roughly that size (and which is SHA-1'ed upon every single read and\nwrite).\n\nOn this developer's machine, this comparison shows a hefty difference:\n\n\t0013.1: calculate SHA-1 for 300MB (SHA1DC) 3.03(0.03+0.17)\n\t0013.2: calculate SHA-1 for 300MB (OpenSSL) 0.58(0.06+0.16)\n\nIt is not only that ~6x slower performance is a pretty tall order, the\nabsolute numbers themselves speak a very clear language: having to wait\none second every time a file is `git add`ed is noticeable, but one can\nhandwave it away. Having to wait six seconds (3 to read the index, a\nfraction of a millisecond to hash the new contents and update the\nin-memory index, then 3 seconds to write out the index) is outright\nannoying. And unnecessary, too: the content of the index is never\ncrafted to cause SHA-1 collisions.\n\nObviously, this test requires that Git was built with the new\nDC_AND_OPENSSL_SHA1 make flag; it is skipped otherwise.\n\nSigned-off-by: Johannes Schindelin \n---\n t/perf/p0013-sha1dc.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n create mode 100644 t/perf/p0013-sha1dc.sh\n\ndiff --git a/t/perf/p0013-sha1dc.sh b/t/perf/p0013-sha1dc.sh\nnew file mode 100644\nindex 00000000000..e08473ac969\n--- /dev/null\n+++ b/t/perf/p0013-sha1dc.sh\n@@ -0,0 +1,23 @@\n+#!/bin/sh\n+\n+test_description=\"Tests performance of SHA1DC vs OpenSSL\"\n+\n+. ./perf-lib.sh\n+\n+test -n \"$DC_AND_OPENSSL_SHA1\" || {\n+\tskip_all='DC_AND_OPENSSL_SHA1 required for this test'\n+\ttest_done\n+}\n+\n+test_perf 'calculate SHA-1 for 300MB (SHA1DC)' '\n+\tdd if=/dev/zero bs=1M count=300 |\n+\ttest-sha1\n+'\n+\n+test_perf 'calculate SHA-1 for 300MB (OpenSSL)' '\n+\tdd if=/dev/zero bs=1M count=300 |\n+\ttest-sha1 --disable-sha1dc\n+'\n+\n+test_done\n+\n-- \n2.12.1.windows.1\n"},{"id":"315376","messageId":"xmqq7f3d6ev1.fsf@gitster.mtv.corp.google.com","threadId":"45506","inReplyTo":"cover.1490397869.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-25T06:37:54Z","receivedAt":"2017-03-25T06:38:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> - the most important part will be the patch turning core.enableSHA1DC\n> into a tristate: \"externalOnly\" or \"smart\" or \"auto\" or something\n> indicating that it switches on collision detection only for commands\n> that accept objects from an outside source into the local repository,\n> such as fetch, fast-import, etc\n\nThere are different uses of SHA-1 hashing in Git, and I do agree\nthat depending on the use, some of them do not need the overhead for\nthe collision-attack detection. As DC_SHA1 with attack detection\ndisabled may still be slower than other implementations, it may make\nsense to have a compile-time option to use DC_SHA1 for places that\nneeds protection, and another implementation [*1*] for places that\ndon't.\n\nI think the places that MUST use DC variant is anything that hashes\ncontent for a single object to compute its object name. \"git add\"\n[*2*] that creates a new loose object, \"git index-pack\" that reads\nincoming pack stream over the wire, reconstitutes each object data\nwhile resolving delta and hash them to get their names to construct\nthe .idx file and \"git unpack-objects\" that does the same but\nexplodes the pack contents into loose objects, \"git write-tree\" that\ncreates a new tree object given the contents of the index, etc.\n\nOne notable exception of the above is \"update-index --refresh\". We\nalready have contents in the index and in the object store, and we\nare hashing the contents in the working tree to see if it hashes to\nthe same value. When the hash does not match, it won't go in to the\nobject store. When the hash does match, it either is indeed the\nsame content (i.e. no collision), in which case we earlier must have\ndone the collision-attack detecting hash when we added the object to\nthe object store. Or the object we have in the object store and\nwhat is in the working tree are different contents that hash to the\nsame name, in which case the user already has colliding pair and it\nis too late to invoke collision-attack detecting variant ;-)\n\nThe running checksum over the whole file csum-file.c computes does\nnot have to be the collision-attack detecting kind. This is the\nhash at the end of various files like the index, .pack .idx, etc.\nThese are used to protect us against bit-flipping disks and we are\nnot fighting with a clever disk that can do collision attack. For\nthat matter, some of these checksums do not even have to be SHA-1.\nIf one hacks his own Git to replace SHA-1 checksum at the end of the\nindex file with something faster and weaker and use it in one's\nrepository, nobody else would notice nor care. The same thing can\nbe said for the .idx file. The one at the end of .pack does get\nchecked at the receiving end when it comes over the wire, so it MUST\nbe SHA-1, but it does not have to be hashed with collision-attack\ndetection.\n\nThe rerere database is indexed with SHA-1 of (normalized) conflict\ntext. This does not have to be hashed with the collision-attack\ndetection logic. Thanks to recent update that allows multiple pairs\nof conflict and its resolution, the subsystem is prepared to see two\nconflicts that share the same hash already (for completely different\nreasons).\n\nThe hash that names a packfile is constructed by sorting all the\nnames of the objects contained in the packfile and running SHA-1\nhash over it. I think this MUST be hashed with collision-attack\ndetection. A malicious site can feed you a packfile that contains\nobjects the site crafts so that the sorted object names would result\nin a collision-attack, ending up with one pack that contains a sets\nof objects different from another pack that happens to have the same\npackname, causing Git to say \"Ah, this new pack must have the same\nset of objects as the pack we already have\" and discard it,\nresulting in lost objects and a corrupt repository with missing\nobjects.\n\n\n[Footnote]\n\n*1* I think this PREVIEW hardcodes OpenSSL only for illustration and\n that is OK for a preview. Given the recent news on licensing,\n however, if we want to pursue this dual hashing scheme, we must\n consider allowing other implementations as well in the final\n form.\n\n*2* In this paragraph, whenever \"git\" command is named, I mean both\n the command and its underlying machinery. When I say \"git\n write-tree\", for example, write_index_as_tree() obviously is\n included.\n"},{"id":"315398","messageId":"xmqq1stl5m4l.fsf@gitster.mtv.corp.google.com","threadId":"45506","inReplyTo":"xmqq7f3d6ev1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-25T16:58:34Z","receivedAt":"2017-03-25T16:58:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The hash that names a packfile is constructed by sorting all the\n> names of the objects contained in the packfile and running SHA-1\n> hash over it.\n\nSorry, but I need to make a correction here.\n\nThis \"SHA-1 over sorted object names\" is a description of an ancient\nbehaviour before 1190a1ac (\"pack-objects: name pack files after\ntrailer hash\", 2013-12-05) happened. These days the pack name is\nthe same as the csum-file checksum of the .pack contents.\n\nThis however does not change the fact that the site that feeds us a\npackfile is in control of the hash, hence the name we give to the\nresulting packfile. Unlike the use of csum-file for the trailing\nhash for the index file, which is only to protect against bit\nflipping, \"SHA-1 over .pack contents\" done here is used to come up\nwith a unique name used for identification and deduplication (of the\npackfile, not of individual objects), and the need for protection\nagainst collision attack attempts does not change between the\nimplementation before 1190a1ac and after that commit.\n"},{"id":"315405","messageId":"CACBZZX64wCFFG9WFEmAiB7vtAfP1uwcgMx8HwA4Hx8jtF5pKOQ@mail.gmail.com","threadId":"45506","inReplyTo":"7a2444f08dea1b2fe497ae7498eba44626414d29.1490397869.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/7] Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-25T19:51:39Z","receivedAt":"2017-03-25T19:52:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Mar 25, 2017 at 12:24 AM, Johannes Schindelin\n wrote:\n> The collision detection is not for free, though: when using the SHA1DC\n> code, calculating the SHA-1 takes substantially longer than using\n> OpenSSL's (in some case hardware-accelerated) SHA1-routines, and this\n> happens even when switching off the collision detection in SHA1DC's code.\n\n[...]in some case*s*[...]\n"},{"id":"315451","messageId":"20170326061826.yx6nh3k2ps6uyyz6@sigill.intra.peff.net","threadId":"45506","inReplyTo":"xmqq7f3d6ev1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-26T06:18:26Z","receivedAt":"2017-03-26T06:25:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2017 at 11:37:54PM -0700, Junio C Hamano wrote:\n\n> The hash that names a packfile is constructed by sorting all the\n> names of the objects contained in the packfile and running SHA-1\n> hash over it. I think this MUST be hashed with collision-attack\n> detection. A malicious site can feed you a packfile that contains\n> objects the site crafts so that the sorted object names would result\n> in a collision-attack, ending up with one pack that contains a sets\n> of objects different from another pack that happens to have the same\n> packname, causing Git to say \"Ah, this new pack must have the same\n> set of objects as the pack we already have\" and discard it,\n> resulting in lost objects and a corrupt repository with missing\n> objects.\n\nI don't think this case really matters for collision detection. What's\nimportant is what Git does when it receives a brand-new packfile that\nwould overwrite an existing one. It _should_ keep the old one, under the\nusual \"existing data wins\" rule.\n\nIt should be easy to test, though:\n\n $ git init tmp && cd tmp\n $ git commit --allow-empty -m foo\n $ git gc\n\n $ touch -d yesterday .git/objects/pack/*\n $ ls -l .git/objects/pack\n -r--r--r-- 1 peff peff 1128 Mar 25 02:10 pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.idx\n -r--r--r-- 1 peff peff 153 Mar 25 02:10 pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.pack\n\n $ git index-pack --stdin <.git/objects/pack/*.pack\n pack\t7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b\n\n $ ls -l .git/objects/pack\n -r--r--r-- 1 peff peff 1128 Mar 25 02:10 pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.idx\n -r--r--r-- 1 peff peff 153 Mar 25 02:10 pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.pack\n\nLooks like the timestamps were retained. And if we use strace, we\ncan see what happens:\n\n $ strace git index-pac k--stdin <.git/objects/pack/*.pack\n link(\".git/objects/pack/tmp_pack_YSrdWU\", \".git/objects/pack/pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.pack\") = -1 EEXIST (File exists)\n unlink(\".git/objects/pack/tmp_pack_YSrdWU\") = 0\n link(\".git/objects/pack/tmp_idx_O94NNU\", \".git/objects/pack/pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.idx\") = -1 EEXIST (File exists)\n unlink(\".git/objects/pack/tmp_idx_O94NNU\") = 0\n\nThis is due to the link()/EEXIST handling in finalize_object_file. It\nhas a FIXME for a collision check, so we could actually detect at that\npoint whether we have a real collision, or if the other side just\nhappened to send us the same pack.\n\nI wouldn't be surprised if the dumb-http walker is not so careful,\nthough (but the solution is to make it careful, not to worry about\na weak hash algorithm).\n\nThe rest of your email all made sense to me.\n\n-Peff\n"},{"id":"315489","messageId":"xmqqinmv4ojt.fsf@gitster.mtv.corp.google.com","threadId":"45506","inReplyTo":"20170326061826.yx6nh3k2ps6uyyz6@sigill.intra.peff.net","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-26T23:16:06Z","receivedAt":"2017-03-26T23:23:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 24, 2017 at 11:37:54PM -0700, Junio C Hamano wrote:\n>\n>> The hash that names a packfile is constructed by sorting all the\n>> names of the objects contained in the packfile and running SHA-1\n>> hash over it. I think this MUST be hashed with collision-attack\n>> detection. A malicious site can feed you a packfile that contains\n>> objects the site crafts so that the sorted object names would result\n>> in a collision-attack, ending up with one pack that contains a sets\n>> of objects different from another pack that happens to have the same\n>> packname, causing Git to say \"Ah, this new pack must have the same\n>> set of objects as the pack we already have\" and discard it,\n>> resulting in lost objects and a corrupt repository with missing\n>> objects.\n>\n> I don't think this case really matters for collision detection. What's\n> important is what Git does when it receives a brand-new packfile that\n> would overwrite an existing one. It _should_ keep the old one, under the\n> usual \"existing data wins\" rule.\n\nIf a malicious site can craft two packfiles that hash to the same,\nthen it can first feed one against a fetch request, then feed the\nother one when a later fetch request comes, and then the later pack\nis discarded by the \"existing data wins\" rule. Even though this\nlater pack may have all the objects necessary to complete the refs\nthis later fetch request asked for, and an earlier pack that\nhappened to have the same pack trailer hash doesn't have these\nnecessary objects, the receiver ends up discarding this necessary\npack. The end result is that the receiver's repository is now\ncorrupt, lacking some objects.\n\nIt is a different matter if such an \"attack\" is a useful one to\nconduct from attacker's point of view. \n\nI highlighted this case as notable because the way the trailing hash\nis also used as the name of packfile makes this fall into the same\ncategory as object hash, in that the hash is used for identification\nand deduplication (because we have a rule that says there can be\nonly one _thing_ that hashes to one value), for which we do want to\nuse the collision-attack detecting kind of hashing, even though it\notherwise should fall into the other category (i.e. use of csum-file\nAPI to compute trailer hash), where the hash is used merely for\nbit-flip detection (we are perfectly OK if you have multiple index\nfiles with different contents that happen to have the same hash in\nthe trailer, because the hash is not used for identificaiton and\ndeduplication).\n"},{"id":"315495","messageId":"20170327011140.icqfc4lqlarvae6l@sigill.intra.peff.net","threadId":"45506","inReplyTo":"xmqqinmv4ojt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-27T01:11:40Z","receivedAt":"2017-03-27T01:11:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 26, 2017 at 04:16:06PM -0700, Junio C Hamano wrote:\n\n> > I don't think this case really matters for collision detection. What's\n> > important is what Git does when it receives a brand-new packfile that\n> > would overwrite an existing one. It _should_ keep the old one, under the\n> > usual \"existing data wins\" rule.\n> \n> If a malicious site can craft two packfiles that hash to the same,\n> then it can first feed one against a fetch request, then feed the\n> other one when a later fetch request comes, and then the later pack\n> is discarded by the \"existing data wins\" rule. Even though this\n> later pack may have all the objects necessary to complete the refs\n> this later fetch request asked for, and an earlier pack that\n> happened to have the same pack trailer hash doesn't have these\n> necessary objects, the receiver ends up discarding this necessary\n> pack. The end result is that the receiver's repository is now\n> corrupt, lacking some objects.\n\nNo, I don't think so. We don't trust the trailer hash for anything to do\nwith corruption; we actually inflate the objects and see which ones we\ngot. So the victim will notice immediately that what the attacker sent\nit is insufficient to complete the fetch (or push), and will refuse to\nupdate the refs. The fetch transfer, but nobody gets corrupted.\n\nOr another way to think about it: we don't care what the trailer hash\nis. It comes from the untrusted attacker in the first place. So this\nattack is no different than the other side just sending a bogus pack\nthat omits an object (but has a valid checksum). The fact that it\nhappens to use the same on-disk name as something we already have is\nirrelevant. We'll still notice that we don't have everything we need to\nupdate the refs.\n\nSo the best you could do is probably a mini-DoS. Something like:\n\n 0. Attacker generates two colliding packs, A and B. Only pack B has\n object X.\n\n 1. Somehow, the attacker convinces upstream to take history with X\n _and_ to repack such that serving a fetch will yield pack B.\n\n 2. You fetch from the attacker, who feeds you A.\n\n 3. Now you fetch from upstream, who tries to send you B. You refuse\n it, thinking you already have it (but you really have A). The fetch\n fails because you are missing X.\n\nThat situation persists until either you repack (at which point you no\nlonger have A, unless your repack happens to generate the exact same\npack again!), or upstream history moves (or is repacked), causing them\nto send a different pack.\n\nI think arranging for (1) is exceedingly unlikely, and the fact that\nyour investment in (0) is lost the moment either the victim or the\nupstream changes a single bit makes it an unlikely attack.\n\n> I highlighted this case as notable because the way the trailing hash\n> is also used as the name of packfile makes this fall into the same\n> category as object hash, in that the hash is used for identification\n> and deduplication (because we have a rule that says there can be\n> only one _thing_ that hashes to one value), for which we do want to\n> use the collision-attack detecting kind of hashing, even though it\n> otherwise should fall into the other category (i.e. use of csum-file\n> API to compute trailer hash), where the hash is used merely for\n> bit-flip detection (we are perfectly OK if you have multiple index\n> files with different contents that happen to have the same hash in\n> the trailer, because the hash is not used for identificaiton and\n> deduplication).\n\nIf we really wanted to address this (and I remain unconvinced that it's\nworth caring about), the simplest thing is not to do collision\ndetection, but simply to give the packs a less predictable name. There\nis nothing at all that cares about the filenames beyond the syntactic\n\"pack-[0-9a-f]{40}.pack\", and it does not need to match the trailer\nchecksum (and as you know, we switched it recently without ill effect).\n\nSo we could literally just switch to writing out pack-$x.pack, where $x\nis something like the 160-bit truncated sha-256, and the readers would\nbe none the wiser.\n\n-Peff\n"},{"id":"315502","messageId":"xmqqbmsn2qyh.fsf@gitster.mtv.corp.google.com","threadId":"45506","inReplyTo":"20170327011140.icqfc4lqlarvae6l@sigill.intra.peff.net","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-27T06:07:02Z","receivedAt":"2017-03-27T06:07:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> If a malicious site can craft two packfiles that hash to the same,\n>> then it can first feed one against a fetch request, then feed the\n>> other one when a later fetch request comes, and then the later pack\n>> is discarded by the \"existing data wins\" rule. Even though this\n>> later pack may have all the objects necessary to complete the refs\n>> this later fetch request asked for, and an earlier pack that\n>> happened to have the same pack trailer hash doesn't have these\n>> necessary objects, the receiver ends up discarding this necessary\n>> pack. The end result is that the receiver's repository is now\n>> corrupt, lacking some objects.\n>\n> No, I don't think so. We don't trust the trailer hash for anything to do\n> with corruption; we actually inflate the objects and see which ones we\n> got. So the victim will notice immediately that what the attacker sent\n> it is insufficient to complete the fetch (or push), and will refuse to\n> update the refs. The fetch transfer, but nobody gets corrupted.\n\nIn the scenario I was presenting, both the original fetch that gives\none packdata and the later fetch that gives another packdata (which\nhappens to share the csum-file trailing checksum) satisfy the \"does\nthe new pack give us enough objects to really complete the tips of\nrefs?\" check. The second fetch transfers, we validate the packdata\nusing index-pack (we may pass --check-self-contained-and-connected\nand we would pass --strict if transfer-fsck is set), we perhaps even\nstore it in quarantine area while adding it to the list of in-core\npacks, make sure everything is now connected from the refs using\npre-existing packs and this new pack. The index-pack may see\neverything is good and then would report the resulting pack name\nback to index_pack_lockfile() called by fetch-pack.c::get_pack().\n\nThat was the scenario I had in mind. Not \"bogus sender throws a\ncorrupt pack at you\" case, where we check the integrity and\nconnectivity of the objects ourselves.\n\nAnd the trailer hash the sender added at the end of the pack data\nstream does not have to come into the picture. When we compute that\nourselves for the received pack data, because the sender is trying a\nsuccessful collision attack (they gave us one pack that hashes to\ncertain value earlier; now they are giving us the other one), we\nwould end up computing the same.\n\nBut even though both of these packs _are_ otherwise valid (in the\nsense that they satisfactorily transfer objects necessary to make\nthe refs that were fetched complete), because we name the packs\nafter the trailer hash and we cannot have two files with the same\nname, we end up throwing away the later one.\n\nAs I said, it is a totally different matter if this attack scenario\nis a practical threat. For one thing, it is probably harder than\njust applying the straight \"shattered\" attack to create a single\nobject collision--you have to make two packs share the same trailing\nhash _and_ make sure that both of them record data for valid\nobjects. But I am not convinced that it would be much harder\n(e.g. I understand that zlib deflate can be told not to attempt\ncompression at all, and the crafted garbage used in the middle part\nof the \"shattered\" attack can be such a blob object expressed as a\nbase object--once the attacker has two such packfiles that hash the\nsame, two object names for these garbage blobs can be used to\npresent two versions of the values for a ref to be fetched by these\ntwo fetch requests).\n"},{"id":"315504","messageId":"20170327070909.26ojhkhagf6pq3wp@sigill.intra.peff.net","threadId":"45506","inReplyTo":"xmqqbmsn2qyh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-27T07:09:09Z","receivedAt":"2017-03-27T07:10:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 26, 2017 at 11:07:02PM -0700, Junio C Hamano wrote:\n\n> > No, I don't think so. We don't trust the trailer hash for anything to do\n> > with corruption; we actually inflate the objects and see which ones we\n> > got. So the victim will notice immediately that what the attacker sent\n> > it is insufficient to complete the fetch (or push), and will refuse to\n> > update the refs. The fetch transfer, but nobody gets corrupted.\n> \n> In the scenario I was presenting, both the original fetch that gives\n> one packdata and the later fetch that gives another packdata (which\n> happens to share the csum-file trailing checksum) satisfy the \"does\n> the new pack give us enough objects to really complete the tips of\n> refs?\" check.\n\nRight, my point was that we do that check _after_ throwing away the\nduplicate-named pack. So you cannot fool that check, update the ref, and\nthen throw away the pack to get a corrupt receiver. The receiver throws\naway the pack first, then says \"hey, I don't have all the objects\" and\naborts.\n\nThat said...\n\n> The second fetch transfers, we validate the packdata using index-pack\n> (we may pass --check-self-contained-and-connected and we would pass\n> --strict if transfer-fsck is set), we perhaps even store it in\n> quarantine area while adding it to the list of in-core packs, make\n> sure everything is now connected from the refs using pre-existing\n> packs and this new pack. The index-pack may see everything is good\n> and then would report the resulting pack name back to\n> index_pack_lockfile() called by fetch-pack.c::get_pack().\n\nThese are interesting corner cases. We only use\n--check-self-contained-and-connected with clones, but you may still have\npacks from an alternate during a clone (although I think the two packs\nwould be allowed to co-exist indefinitely, then).\n\nThe quarantine case is more interesting. The two packs _do_ co-exist\nwhile we do the connectivity check there, and then afterwards we can\nhave only one. So that reversal of operations introduces a problem, and\nyou could end up with a lasting corruption as a result.\n\n> But even though both of these packs _are_ otherwise valid (in the\n> sense that they satisfactorily transfer objects necessary to make\n> the refs that were fetched complete), because we name the packs\n> after the trailer hash and we cannot have two files with the same\n> name, we end up throwing away the later one.\n\nI kind of wonder if we should simply allow potential duplicates to\nco-exist. The pack names really aren't used for duplicate suppression in\nany meaningful sense. We effectively use them as UUIDs so that each new\npack gets a unique name without having to do any locking or other\ncoordination. It would not be unreasonable to say \"oops, 1234abcd\nalready exists; I'll just increment and call this new one 1234abce\". The\ntwo presumably-the-same packs would then co-exist until the new \"repack\n-a\" removes duplicates (not just at the pack level, but at the object\nlevel).\n\nThe biggest problem there is that \"claiming\" a pack name is not\ncurrently atomic. We just do it blindly. So switching to some other\npresumed-unique UUID might actually be easier (whether SHA-256 of the\npack contents or some other method).\n\n> As I said, it is a totally different matter if this attack scenario\n> is a practical threat. For one thing, it is probably harder than\n> just applying the straight \"shattered\" attack to create a single\n> object collision--you have to make two packs share the same trailing\n> hash _and_ make sure that both of them record data for valid\n> objects. But I am not convinced that it would be much harder\n> (e.g. I understand that zlib deflate can be told not to attempt\n> compression at all, and the crafted garbage used in the middle part\n> of the \"shattered\" attack can be such a blob object expressed as a\n> base object--once the attacker has two such packfiles that hash the\n> same, two object names for these garbage blobs can be used to\n> present two versions of the values for a ref to be fetched by these\n> two fetch requests).\n\nYeah, I think we can assume it will be possible with SHAttered levels of\neffort. An attacker can use it to create a persistent corruption by\nhaving somebody fetch from them twice. So not really that interesting an\nattack, but it is something. I still think that ditching SHA-1 for the\nnaming is probably a better fix than worrying about SHA-1 collisions.\n\n-Peff\n"},{"id":"315520","messageId":"xmqqy3vq1w10.fsf@gitster.mtv.corp.google.com","threadId":"45506","inReplyTo":"20170327070909.26ojhkhagf6pq3wp@sigill.intra.peff.net","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-27T17:15:07Z","receivedAt":"2017-03-27T17:15:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I think we can assume it will be possible with SHAttered levels of\n> effort. An attacker can use it to create a persistent corruption by\n> having somebody fetch from them twice. So not really that interesting an\n> attack, but it is something. I still think that ditching SHA-1 for the\n> naming is probably a better fix than worrying about SHA-1 collisions.\n\nYes, I agree with that part. \n\nOur trailer checksum happens to be SHA-1 mostly because the code was\navailable, not because they need to be a crypto-strong hash. It can\nsafely be changed to something other than SHA-1 that is much faster,\nif that is desired, when it is used only for bit-flip detection of\nlocal files like the index file.\n\nI also agree that changing the naming scheme (e.g. use the \"hash\" as\na hash to choose hash-bucket but accept the fact that hashes can\ncollide) is a better solution, if this \"packname can collide\" were\nto become real problem.\n\nThanks.\n\n"},{"id":"315702","messageId":"alpine.DEB.2.20.1703291509450.48202@virtualbox","threadId":"45506","inReplyTo":"xmqq7f3d6ev1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-29T20:02:17Z","receivedAt":"2017-03-29T20:04:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 24 Mar 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > - the most important part will be the patch turning core.enableSHA1DC\n> > into a tristate: \"externalOnly\" or \"smart\" or \"auto\" or something\n> > indicating that it switches on collision detection only for commands\n> > that accept objects from an outside source into the local repository,\n> > such as fetch, fast-import, etc\n> \n> There are different uses of SHA-1 hashing in Git, and I do agree\n> that depending on the use, some of them do not need the overhead for\n> the collision-attack detection.\n\nIndeed.\n\nI guess I should have clarified very clearly what I intended to accomplish\nwith this preview.\n\nLet me first summarize the background:\n\n- Git originally used SHA-1 as a convenient catch-all hashing algorithm,\n with the security of the hash merely being a nice afterthought.\n Unfortunately, SHA-1 was hardcoded. Mistakes were made. They always are.\n\n- After the SHAttered blog post became public, Linus first made the case\n that it matters not all that much: the development of the Linux kernel\n is based on trust, and nobody would pull from a person they do not trust.\n This approach does obviously not extend to most other projects.\n\n- By switching the default to DC_SHA1 already in `master`, you now took\n the exact opposite position: it *always* matters, even when you trust\n people, and the 6x slowdown demonstrated by my perf test is something that\n everybody needs to accept, even if it is spent for no benefit in return.\n\nBetween these two extremes (\"collision attacks do not matter if you run\nyour project based on trust\" vs \"we always mistrust everybody, including\nthe user's own source code that they stage for commit\"), I think there are\nmany shades of green, and I think it would be delusional to believe that\nwe can predict the trust model for each and every Git user [*1*], baking\nit into a single compile time setting.\n\nThat is why I wanted to implement a tristate config so that users can\nadapt Git to their particular scenario. That way, maintainers of\nprecompiled Git packages do not have to dictate to Git users what trust\nmodel they should use.\n\nOne scenario seems to be common, and it is certainly one that I have a\ndirect interest in supporting: inside a company, where the server as well\nas the developers are implicitly trusted not to fiddle with collision\nattacks (because there are much easier ways to hide malicious code, let's\nbe frank). And in this scenario, slowing down the SHA-1 computation half\nan order of magnitude by trying to detect collision attacks is simply\nunacceptable, because there is no benefit, only cost to it.\n\nAn even more common scenario is when a developer works on a local\nrepository, adds a couple of files, then runs rebase, then merges, etc. In\n*none* of these cases does the developer distrust any of the objects\nflying about. Like above, forcing the developer to accept half an order of\nmagnitude slow down of the SHA-1 computation is something I would consider\ndisrespectful of those developers' time. Note: in this scenario, any\nobject coming from elsewhere would most likely be subject to the collision\ndetection, as the developer may not trust anybody but themselves.\n\nIn other words: I disagree that, say, `git add` should use the collision\ndetecting SHA-1.\n\nI also suspect that you had a much more elaborate (maybe even fragile)\nstrategy than mine in mind when you tried to determine which code paths\nwould need collision detection and which ones would not: we have *no*\ncontext in the object-oriented sense whenever we call the object hashing\nfunctions. Meaning that you would have to introduce such a context, or to\nadd some sort of thread local state. I have to admit that I do not like\neither way.\n\nThe approach I chose instead was to make the switch global, per command.\nObviously, the next step is to identify the Git commands which accept\nobjects from external sources (`clone`, `fetch`, `fast-import` and all the\nremote helpers come to mind) and let them set a global flag before asking\ngit_default_config() to parse the core.enableSHA1DC setting, so that the\nspecial value `external` could trigger the collision detecting code for\nthose, and only those, commands. That would still err on the safe side if\nthese commands are used to hash objects originating from the same machine,\nbut that is a price I would be willing to pay for the simplicity of this\napproach.\n\nDoes my explanation manage to illustrate in a plausible way why I chose\nthe approach that I did?\n\nCiao,\nJohannes\n\nFootnote *1*: It is equally delusional, of course, to claim that every Git\nuser can and should configure and compile Git for themselves.\n"},{"id":"315726","messageId":"xmqqr31f7gh8.fsf@gitster.mtv.corp.google.com","threadId":"45506","inReplyTo":"alpine.DEB.2.20.1703291509450.48202@virtualbox","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-30T00:31:15Z","receivedAt":"2017-03-30T00:31:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> - After the SHAttered blog post became public, Linus first made the case\n> that it matters not all that much: the development of the Linux kernel\n> is based on trust, and nobody would pull from a person they do not trust.\n> This approach does obviously not extend to most other projects.\n>\n> - By switching the default to DC_SHA1 already in `master`, you now took\n> the exact opposite position: it *always* matters, even when you trust\n> people, and the 6x slowdown demonstrated by my perf test is something that\n> everybody needs to accept, even if it is spent for no benefit in return.\n\nYou seem to be trying very hard to make it look like as if Linus\nsaid one thing and I decided to do something else, but if one digs\ninto the archive, it should be clear that that is not how the\ncurrent line of development has happened. Even from the same\nperson, you can hear that \"the social aspect of how the project is\nstructured makes the kernel less susceptible to the shattered\nattack\" [*1*] and that \"mitigation at Git level is easy\" [*2*].\n\nThe kernel as a project may not have to do much, but the world of\nprojects using Git is wider than the kernel alone, and mitigation\nthat applies to all was available and is easy to make it the\ndefault.\n\n> The approach I chose instead was to make the switch global, per command.\n> Obviously, the next step is to identify the Git commands which accept\n> objects from external sources (`clone`, `fetch`, `fast-import` and all the\n> remote helpers come to mind) and let them set a global flag before asking\n> git_default_config() to parse the core.enableSHA1DC setting, so that the\n> special value `external` could trigger the collision detecting code for\n> those, and only those, commands. That would still err on the safe side if\n> these commands are used to hash objects originating from the same machine,\n> but that is a price I would be willing to pay for the simplicity of this\n> approach.\n>\n> Does my explanation manage to illustrate in a plausible way why I chose\n> the approach that I did?\n\nI agree that there may be rooms to tweak the degree of paranoia per\ncodepath (I said that already in the message you are responding to),\nbut as Linus and Peff already said in the old discussion thread\n[*3*], I doubt that it needs to be runtime configurable.\n\nIn any case, I do not think you should blindly trust what end-users\nadd to the repository locally. A vendor may send in a binary driver\nupdate via a pull-request, and you would want to protect \"clone\" and\n\"fetch\" client because of that. The same driver update however may\ncome as a patch that is accepted by running \"git am\". Or you may\nget a vendor tarball ftp'ed over and \"git add .\" the whole thing.\n\nThese \"local\" operations still need the same degree of paranoia as\nyour \"git fetch\"; \"per command\" may not be such a good criterion\nbecause of that.\n\nThat is why I mentioned \"you want to be paranoid any time you add\nnew data to the object database\" as a rule of thumb in the message\nyou are responding to. It may be overly broad, but would be a\nbetter starting point than \"'git add' is always safe as it is\nlocal\".\n\nSo in short, I do agree with your idea of using \"faster\" hashing for\nsome codepaths and \"paranoid\" ones for others. I think \"slower but\ncollision-attack detecting\" one (aka DC_SHA1) plus a choice of\n\"faster\" one at compile time would be a sensible way to go, and if\none is truly paranoid, the \"faster\" one _could_ be sha1dc.c with\ncollision detection disabled. On the other hand, if one is in a\nclosed environment, one may want both hashes configurable and let\nOpenSSL implementation be chosen for both \"slower but DC\" and\n\"faster\" hash. And I am OK with that, too.\n\n\n[Reference]\n\n*1* https://public-inbox.org/git/CA+55aFz98r7NC_3BW_1HU9-0C-HcrFou3=0gmRcS38=-x8dmVw@mail.gmail.com/\n\n*2* https://public-inbox.org/git/CA+55aFxmr6ntWGbJDa8tOyxXDX3H-yd4TQthgV_Tn1u91yyT8w@mail.gmail.com/\n\n*3* https://public-inbox.org/git/20170323174750.xyucxmfhuc6dbrzc@sigill.intra.peff.net/\n"},{"id":"315790","messageId":"xmqq37du7n9p.fsf@gitster.mtv.corp.google.com","threadId":"45506","inReplyTo":"7a2444f08dea1b2fe497ae7498eba44626414d29.1490397869.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/7] Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-30T16:16:50Z","receivedAt":"2017-03-30T16:17:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> +#ifdef SHA1_DC_AND_OPENSSL\n> +void (*SHA1_Init_func)(SHA_CTX_union *ctx) = (void *)SHA1DCInit;\n> +void (*SHA1_Update_func)(SHA_CTX_union *ctx, const void *pointer, size_t size) =\n> +\t(void *)git_SHA1DCUpdate;\n> +int (*SHA1_Final_func)(unsigned char sha1[20], SHA_CTX_union *ctx) =\n> +\t(void *)git_SHA1DCFinal;\n> +\n> +void toggle_sha1dc(int enable)\n> +{\n> +\tif (enable) {\n> +\t\tSHA1_Init_func = (void *)SHA1DCInit;\n> +\t\tSHA1_Update_func = (void *)git_SHA1DCUpdate;\n> +\t\tSHA1_Final_func = (void *)git_SHA1DCFinal;\n> +\t} else {\n> +\t\tSHA1_Init_func = (void *)SHA1_Init;\n> +\t\tSHA1_Update_func = (void *)SHA1_Update;\n> +\t\tSHA1_Final_func = (void *)SHA1_Final;\n> +\t}\n> +}\n> +#endif\n\nAs I understand that this is a demonstration series, the approach\nabove is OK as an expedite way to illustrate one way how run-time\nswitching could be done. The approach however is not very thread\nfriendly, though.\n\n> diff --git a/sha1dc/sha1.h b/sha1dc/sha1.h\n> index bd8bd928fb3..243c2fe0b6b 100644\n> --- a/sha1dc/sha1.h\n> +++ b/sha1dc/sha1.h\n> @@ -110,10 +110,26 @@ void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);\n> */\n> void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);\n> \n> +#ifdef SHA1_DC_AND_OPENSSL\n> +extern void toggle_sha1dc(int enable);\n> +\n> +typedef union {\n> +\tSHA1_CTX dc;\n> +\tSHA_CTX openssl;\n> +} SHA_CTX_union;\n\nThe use of union is a good ingredient for a solution. I would have\nchosen to do this slightly differently if I were doing it.\n\n typedef struct {\n int safe;\n union {\n SHA1_CTX_SAFE safe;\n SHA1_CTX_FAST fast;\n } u;\n } git_SHA_CTX;\n\n void git_SHA1_Init(git_SHA_CTX *ctx, int safe);\n\tvoid git_SHA1_Update(git_SHA_CTX *ctx, const void *, unsigned long);\n\tgit_SHA1_Final(uchar [20], git_SHA_CTX *ctx);\n\nwhere SHA1_CTX_FAST may be chosen from the Makefile just like we\ncurrently choose platform_SHA_CTX. SHA1_CTX_SAFE could also be made\nconfigurable but it may be OK to hardcode it to refer to SHA1_CTX of\nDC's.\n\nAs you already know, I am assuming that each codepath pretty much\nknows if it needs safe or fast one (e.g. the one used in csum-file.c\nknows it does not have to), so each git_SHA_CTX is told which one to\nuse when it gets initialized.\n"},{"id":"315791","messageId":"xmqqvaqq67am.fsf@gitster.mtv.corp.google.com","threadId":"45506","inReplyTo":"xmqq37du7n9p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/7] Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-30T16:47:13Z","receivedAt":"2017-03-30T16:47:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ...\n> The use of union is a good ingredient for a solution. I would have\n> chosen to do this slightly differently if I were doing it.\n>\n> typedef struct {\n> int safe;\n> union {\n> SHA1_CTX_SAFE safe;\n> SHA1_CTX_FAST fast;\n> } u;\n> } git_SHA_CTX;\n>\n> void git_SHA1_Init(git_SHA_CTX *ctx, int safe);\n> \tvoid git_SHA1_Update(git_SHA_CTX *ctx, const void *, unsigned long);\n> \tgit_SHA1_Final(uchar [20], git_SHA_CTX *ctx);\n>\n> where SHA1_CTX_FAST may be chosen from the Makefile just like we\n> currently choose platform_SHA_CTX. SHA1_CTX_SAFE could also be made\n> configurable but it may be OK to hardcode it to refer to SHA1_CTX of\n> DC's.\n>\n> As you already know, I am assuming that each codepath pretty much\n> knows if it needs safe or fast one (e.g. the one used in csum-file.c\n> knows it does not have to), so each git_SHA_CTX is told which one to\n> use when it gets initialized.\n\nAnd if we wanted to declare \"git add\" is always safe, we could still\ndo\n\n int sha1_safety_global_override = -1; /* unspecified */\n\n void git_SHA1_Init(git_SHA_CTX *ctx, int safe)\n {\n\tif (sha1_safety_global_override >= 0)\n\t ctx->safe = sha1_safety_global_override;\n else\n\t ctx->safe = safe;\n\n\tif (ctx->safe)\n\t SHA1DCInit(&(ctx->u.safe));\n\telse\n\t platform_SHA1_Init(&(ctx->u.fast));\n }\n\nand then have cmd_add() in builtin/add.c to flip that global\noverride bit to say \"this does not have to be safe\". I personally\ndo not think it is a good idea, but I am showing that it is still\ndoable. \n\nAnd as long as assignment to sha1_safety_global_override is done in\na thread-friendly way, such a scheme would be more thread-friendly\nas a whole compared to the \"toggle_sha1dc()\" approach where each CTX\ninstance does not know which side of the union it is being used us\n(which, if mixed-up, of course would lead to a funny behaviour).\n\n"},{"id":"317097","messageId":"alpine.DEB.2.20.1704181325390.8522@virtualbox","threadId":"45506","inReplyTo":"xmqq37du7n9p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/7] Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-18T11:28:32Z","receivedAt":"2017-04-18T11:29:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 30 Mar 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > +#ifdef SHA1_DC_AND_OPENSSL\n> > +void (*SHA1_Init_func)(SHA_CTX_union *ctx) = (void *)SHA1DCInit;\n> > +void (*SHA1_Update_func)(SHA_CTX_union *ctx, const void *pointer, size_t size) =\n> > +\t(void *)git_SHA1DCUpdate;\n> > +int (*SHA1_Final_func)(unsigned char sha1[20], SHA_CTX_union *ctx) =\n> > +\t(void *)git_SHA1DCFinal;\n> > +\n> > +void toggle_sha1dc(int enable)\n> > +{\n> > +\tif (enable) {\n> > +\t\tSHA1_Init_func = (void *)SHA1DCInit;\n> > +\t\tSHA1_Update_func = (void *)git_SHA1DCUpdate;\n> > +\t\tSHA1_Final_func = (void *)git_SHA1DCFinal;\n> > +\t} else {\n> > +\t\tSHA1_Init_func = (void *)SHA1_Init;\n> > +\t\tSHA1_Update_func = (void *)SHA1_Update;\n> > +\t\tSHA1_Final_func = (void *)SHA1_Final;\n> > +\t}\n> > +}\n> > +#endif\n> \n> As I understand that this is a demonstration series, the approach\n> above is OK as an expedite way to illustrate one way how run-time\n> switching could be done. The approach however is not very thread\n> friendly, though.\n\nIndeed. However, the toggle is meant to be coarse, heavy-handed. I have to\nprotect my time as Git for Windows maintainer, so I have to keep this as\nsimple to maintain as possible while also benefitting my users. This\ntoggle is intended to be run very, very early in the cmd_main() functions.\nAs in: right when the config is read.\n\n> > diff --git a/sha1dc/sha1.h b/sha1dc/sha1.h\n> > index bd8bd928fb3..243c2fe0b6b 100644\n> > --- a/sha1dc/sha1.h\n> > +++ b/sha1dc/sha1.h\n> > @@ -110,10 +110,26 @@ void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);\n> > */\n> > void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);\n> > \n> > +#ifdef SHA1_DC_AND_OPENSSL\n> > +extern void toggle_sha1dc(int enable);\n> > +\n> > +typedef union {\n> > +\tSHA1_CTX dc;\n> > +\tSHA_CTX openssl;\n> > +} SHA_CTX_union;\n> \n> The use of union is a good ingredient for a solution. I would have\n> chosen to do this slightly differently if I were doing it.\n> \n> typedef struct {\n> int safe;\n> union {\n> SHA1_CTX_SAFE safe;\n> SHA1_CTX_FAST fast;\n> } u;\n> } git_SHA_CTX;\n> \n> void git_SHA1_Init(git_SHA_CTX *ctx, int safe);\n> \tvoid git_SHA1_Update(git_SHA_CTX *ctx, const void *, unsigned long);\n> \tgit_SHA1_Final(uchar [20], git_SHA_CTX *ctx);\n> \n> where SHA1_CTX_FAST may be chosen from the Makefile just like we\n> currently choose platform_SHA_CTX. SHA1_CTX_SAFE could also be made\n> configurable but it may be OK to hardcode it to refer to SHA1_CTX of\n> DC's.\n> \n> As you already know, I am assuming that each codepath pretty much\n> knows if it needs safe or fast one (e.g. the one used in csum-file.c\n> knows it does not have to), so each git_SHA_CTX is told which one to\n> use when it gets initialized.\n\nThanks, at this stage it is pretty clear, though, that I will have to\nmaintain this. I am not willing to make this as configurable as you\nsuggested, as these patches will have to stay in git-for-windows/git.\n\nCiao,\nDscho\n"},{"id":"317098","messageId":"alpine.DEB.2.20.1704181320010.8522@virtualbox","threadId":"45506","inReplyTo":"xmqqr31f7gh8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-18T11:30:45Z","receivedAt":"2017-04-18T11:30:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 29 Mar 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > The approach I chose instead was to make the switch global, per\n> > command. Obviously, the next step is to identify the Git commands\n> > which accept objects from external sources (`clone`, `fetch`,\n> > `fast-import` and all the remote helpers come to mind) and let them\n> > set a global flag before asking git_default_config() to parse the\n> > core.enableSHA1DC setting, so that the special value `external` could\n> > trigger the collision detecting code for those, and only those,\n> > commands. That would still err on the safe side if these commands are\n> > used to hash objects originating from the same machine, but that is a\n> > price I would be willing to pay for the simplicity of this approach.\n> >\n> > Does my explanation manage to illustrate in a plausible way why I\n> > chose the approach that I did?\n> \n> I agree that there may be rooms to tweak the degree of paranoia per\n> codepath (I said that already in the message you are responding to),\n> but as Linus and Peff already said in the old discussion thread\n> [*3*], I doubt that it needs to be runtime configurable.\n\nMy real world scenario here was intended to cast a spell on your doubts:\nthe very real slow down is very inacceptable.\n\nGiven your reluctance to accept that the performance impact is large\nenough to be painful, and that there are situations where the trust in\nyour infrastructure and your colleagues makes that pain unnecessary, you\nleave me no other option than to make this a Git for Windows-only feature.\n\nI really tried to reconcile your concerns with the needs of Git for\nWindows' users, to demonstrate that the compile-time switch is\nunacceptable, but it is time to accept defeat and the newly acquired\nmaintenance burden.\n\nMoving on,\nDscho\n\n"}]}