{"thread":{"id":"58661","subject":"[PATCH] Makefile: use sha1collisiondetection by default on OSX and Darwin","startedAt":"2022-10-20T23:01:53Z","lastAt":"2022-12-15T21:17:24Z","messageCount":5,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465369","messageId":"patch-1.1-1f4e39be97b-20221020T225305Z-avarab@gmail.com","threadId":"58661","inReplyTo":null,"subject":"[PATCH] Makefile: use sha1collisiondetection by default on OSX and Darwin","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-20T23:01:20Z","receivedAt":"2022-10-20T23:01:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the sha1collisiondetection library was added and made the default\nin [1] the interaction with APPLE_COMMON_CRYPTO added in [2] and [3]\nseems to have been missed. On modern OSX and Darwin we are able to use\nApple's CommonCrypto both for SHA-1, and as a generic (but partial)\nOpenSSL replacement.\n\nThis left OSX and Darwin without protection against the SHAttered\nattack when building Git in its default configuration.\n\nLet's also use sha1collisiondetection on OSX, to do so we'll need to\nsplit up the \"APPLE_COMMON_CRYPTO\" flag into that flag and a new\n\"APPLE_COMMON_CRYPTO_SHA1\".\n\nLet's also change the CI for \"osx-clang\" to test with the new\nAPPLE_COMMON_CRYPTO_SHA1 knob (\"osx-gcc\" uses the new\nsha1collisiondetection default).\n\nIn practice this will spot issues like the one noted in [4], as\ntesting with just two backends should be enough to spot unportable\ncode. Ideally we'd have other CI jobs to test the various SHA-1\ncombinations, but for now we have better CI coverage than before.\n\n1. e6b07da2780 (Makefile: make DC_SHA1 the default, 2017-03-17)\n2. 4dcd7732db0 (Makefile: add support for Apple CommonCrypto facility, 2013-05-19)\n3. 61067954ce1 (cache.h: eliminate SHA-1 deprecation warnings on Mac OS X, 2013-05-19)\n4. 32205655dc7 (fsmonitor OSX: compile with DC_SHA1=YesPlease, 2022-10-19)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nThe 1st hunk here depends on the \"base\" topic which prepares the docs\nfor this small change:\nhttps://lore.kernel.org/git/cover-v3-0.9-00000000000-20221020T223946Z-avarab@gmail.com/\n\nBut otherwise this applies on \"master\".\n\nJunio: I see in the meantime you've queued your own\nhttps://lore.kernel.org/git/cover-v3-0.9-00000000000-20221020T223946Z-avarab@gmail.com/;\nwhich is currently in \"seen\".\n\nThat patch will merge smoothly with this one, both textually and\nsemantically, but if we have this it's all that's needed to flip the\ndefault and give us pretty much the same OSX CI coverage.\n\n\"Pretty much\" because an unstated effect of your patch is to disable\nall use of the \"Apple Common Crypto\" library on \"osx-clang\", which\nincludes (but is not limited to) giving us another SHA-1 backend.\n\nRange-diff:\n1: 392fabdb456 < -: ----------- fsmonitor OSX: compile with DC_SHA1=YesPlease\n2: 7ae22276aa7 < -: ----------- Makefile: create and use sections for \"define\" flag listing\n3: 78ef8636c57 < -: ----------- Makefile: really use and document sha1collisiondetection by default\n4: f1fb9775b33 < -: ----------- Makefile: rephrase the discussion of *_SHA1 knobs\n-: ----------- > 1: 1f4e39be97b Makefile: use sha1collisiondetection by default on OSX and Darwin\n\n Makefile | 10 ++++------\n ci/lib.sh | 3 +++\n 2 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 36d6bffd1f1..fb4f240e28b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -529,10 +529,8 @@ include shared.mak\n # Define BLK_SHA1 to make use of optimized C SHA-1 routines bundled\n # with git (in the block-sha1/ directory).\n #\n-# Define NO_APPLE_COMMON_CRYPTO on OSX to opt-out of using the\n-# \"APPLE_COMMON_CRYPTO\" backend for SHA-1, which is currently the\n-# default on that OS. We'll define NO_APPLE_COMMON_CRYPTO on Mac OS\n-# 10.4 or older (\"Tiger\", released in early 2005).\n+# Define APPLE_COMMON_CRYPTO_SHA1 to use Apple's CommonCrypto for\n+# SHA-1.\n #\n # === SHA-256 backend ===\n #\n@@ -1873,7 +1871,7 @@ ifdef NO_POSIX_GOODIES\n \tBASIC_CFLAGS += -DNO_POSIX_GOODIES\n endif\n \n-ifdef APPLE_COMMON_CRYPTO\n+ifdef APPLE_COMMON_CRYPTO_SHA1\n \t# Apple CommonCrypto requires chunking\n \tSHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L\n endif\n@@ -1890,7 +1888,7 @@ ifdef BLK_SHA1\n \tLIB_OBJS += block-sha1/sha1.o\n \tBASIC_CFLAGS += -DSHA1_BLK\n else\n-ifdef APPLE_COMMON_CRYPTO\n+ifdef APPLE_COMMON_CRYPTO_SHA1\n \tCOMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL\n \tBASIC_CFLAGS += -DSHA1_APPLE\n else\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 1b0cc2b57db..fda7e008f26 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -264,6 +264,9 @@ macos-latest)\n esac\n \n case \"$jobname\" in\n+osx-clang)\n+\tMAKEFLAGS=\"$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes\"\n+\t;;\n linux32)\n \tCC=gcc\n \t;;\n-- \n2.38.0.1178.g509f5fa8ce0\n\n"},{"id":"465387","messageId":"xmqq4jvyqdof.fsf@gitster.g","threadId":"58661","inReplyTo":"patch-1.1-1f4e39be97b-20221020T225305Z-avarab@gmail.com","subject":"Re: [PATCH] Makefile: use sha1collisiondetection by default on OSX and Darwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-20T23:26:56Z","receivedAt":"2022-10-20T23:27:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Junio: I see in the meantime you've queued your own\n> https://lore.kernel.org/git/cover-v3-0.9-00000000000-20221020T223946Z-avarab@gmail.com/;\n> which is currently in \"seen\".\n\nYes, as I said, I intend to merge it to 'next' in tomorrow's\npushout, and then fast track all three topics (Peff's \"-O0\",\nEric/Ævar's \"use git_SHA_CTX abstraction\", and \"osx-clan uses\nsha1dc\") down to 'master'. As you chose to make this topic hostage\nto the other multi-part topic, which is likely to be slowed down and\nrequire rerolls for typofixes and possible bikeshedding, by the time\nthis topic becomes ready, it is likely that it would already be in\n'master' and you'd have to rebase on that.\n\nIsn't this step of much higher importance than the other multi-part\ntopic? I do not see why you chose to take it a hostage to the other\none. Let's all learn to give priorities to produce sufficiently\nfocused fixes that also sufficiently cover important issues. Frills\nand niceties can come on top later.\n"},{"id":"465392","messageId":"221021.86tu3yf27g.gmgdl@evledraar.gmail.com","threadId":"58661","inReplyTo":"xmqq4jvyqdof.fsf@gitster.g","subject":"Re: [PATCH] Makefile: use sha1collisiondetection by default on OSX and Darwin","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-20T23:52:39Z","receivedAt":"2022-10-21T00:30:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 20 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Junio: I see in the meantime you've queued your own\n>> https://lore.kernel.org/git/cover-v3-0.9-00000000000-20221020T223946Z-avarab@gmail.com/;\n>> which is currently in \"seen\".\n\n[B.t.w. that link is wrong, I meant\nhttps://lore.kernel.org/git/xmqq35bitooc.fsf@gitster.g/]\n\n> Yes, as I said, I intend to merge it to 'next' in tomorrow's\n> pushout, and then fast track all three topics (Peff's \"-O0\",\n> Eric/Ævar's \"use git_SHA_CTX abstraction\", and \"osx-clan uses\n> sha1dc\") down to 'master'. As you chose to make this topic hostage\n> to the other multi-part topic, which is likely to be slowed down and\n> require rerolls for typofixes and possible bikeshedding, by the time\n> this topic becomes ready, it is likely that it would already be in\n> 'master' and you'd have to rebase on that.\n>\n> Isn't this step of much higher importance than the other multi-part\n> topic? I do not see why you chose to take it a hostage to the other\n> one. Let's all learn to give priorities to produce sufficiently\n> focused fixes that also sufficiently cover important issues. Frills\n> and niceties can come on top later.\n\nI don't see why we're in any rush to get this change down to \"master\"\n(either this [1], or the base [2]), nor your jc/ci-osx-with-sha1dc[3].\n\nYour list in [4] has two fixes for issues on \"master\" which we should\nget there sooner than later.\n\nBut the 3rd is just addressing a CI blind spot that we've had for at\nleast 2 years, and since 2017 if we're talking about the default SHA-1\nbackend on OSX.\n\nYes, we had some unportable code sneak in recently, but there's no\nreason I can see for why we'd expect that to happen tomorrow. It's been\none such issue from 2017 until now, so at this rate we should expect the\nnext one in 2027, not next week :)\n\nIn any case, I figured you might want to fast-track it anyway, or\nwhatever, which is why I crafted this series to give you easy options\ngiven your [4]. Namely (and in no particular order):\n\nA. You can queue the base topic[2] and this [1] on top of \"seen\" and\n your jc/ci-osx-with-sha1dc and they won't conflict.\n\nB. This applies directly on \"master\" if you peel off the first hunk. If\n you wanted this faster than the \"doc\" change it could be queued like\n that, and we could fix the docs later.\n\nC. You could eject your [3] and we could let this simmer at the normal\n rate, but that's assuming the \"no rush\" outlined above.\n\nD. You could proceed with your [3], and I can eventually rebase on top\n of it (we'll probably want to undo the nothing-to-do-with-SHA-1\n change, presumably?)\n\nE. You don't need to pick this up at all at this time. Nothing's broken\n now that hasn't been broken for the years by us not using DC_SHA1 on\n OSX by default.\n\n1. https://lore.kernel.org/git/patch-1.1-1f4e39be97b-20221020T225305Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/cover-v3-0.9-00000000000-20221020T223946Z-avarab@gmail.com/\n3. https://lore.kernel.org/git/xmqq35bitooc.fsf@gitster.g/\n4. https://lore.kernel.org/git/xmqqtu3yqhm2.fsf@gitster.g/\n"},{"id":"469067","messageId":"patch-v2-1.1-3de7cdbd260-20221215T084129Z-avarab@gmail.com","threadId":"58661","inReplyTo":"patch-1.1-1f4e39be97b-20221020T225305Z-avarab@gmail.com","subject":"[PATCH v2] Makefile: use sha1collisiondetection by default on OSX and Darwin","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-15T08:43:05Z","receivedAt":"2022-12-15T08:43:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the sha1collisiondetection library was added and made the default\nin [1] the interaction with APPLE_COMMON_CRYPTO added in [2] and [3]\nseems to have been missed. On modern OSX and Darwin we are able to use\nApple's CommonCrypto both for SHA-1, and as a generic (but partial)\nOpenSSL replacement.\n\nThis left OSX and Darwin without protection against the SHAttered\nattack when building Git in its default configuration.\n\nLet's also use sha1collisiondetection on OSX, to do so we'll need to\nsplit up the \"APPLE_COMMON_CRYPTO\" flag into that flag and a new\n\"APPLE_COMMON_CRYPTO_SHA1\".\n\nBecause of this we can stop conflating whether we want to use Apple's\nCommonCrypto at all, and whether we want to use it for SHA-1. This\nmakes the CI recipe added in [4] simpler.\n\n1. e6b07da2780 (Makefile: make DC_SHA1 the default, 2017-03-17)\n2. 4dcd7732db0 (Makefile: add support for Apple CommonCrypto facility, 2013-05-19)\n3. 61067954ce1 (cache.h: eliminate SHA-1 deprecation warnings on Mac OS X, 2013-05-19)\n4. 1ad5c3df35a (ci: use DC_SHA1=YesPlease on osx-clang job for CI,\n 2022-10-20)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\nRange-diff against v1:\n1: 1f4e39be97b ! 1: 3de7cdbd260 Makefile: use sha1collisiondetection by default on OSX and Darwin\n @@ Commit message\n split up the \"APPLE_COMMON_CRYPTO\" flag into that flag and a new\n \"APPLE_COMMON_CRYPTO_SHA1\".\n \n - Let's also change the CI for \"osx-clang\" to test with the new\n - APPLE_COMMON_CRYPTO_SHA1 knob (\"osx-gcc\" uses the new\n - sha1collisiondetection default).\n -\n - In practice this will spot issues like the one noted in [4], as\n - testing with just two backends should be enough to spot unportable\n - code. Ideally we'd have other CI jobs to test the various SHA-1\n - combinations, but for now we have better CI coverage than before.\n + Because of this we can stop conflating whether we want to use Apple's\n + CommonCrypto at all, and whether we want to use it for SHA-1. This\n + makes the CI recipe added in [4] simpler.\n \n 1. e6b07da2780 (Makefile: make DC_SHA1 the default, 2017-03-17)\n 2. 4dcd7732db0 (Makefile: add support for Apple CommonCrypto facility, 2013-05-19)\n 3. 61067954ce1 (cache.h: eliminate SHA-1 deprecation warnings on Mac OS X, 2013-05-19)\n - 4. 32205655dc7 (fsmonitor OSX: compile with DC_SHA1=YesPlease, 2022-10-19)\n + 4. 1ad5c3df35a (ci: use DC_SHA1=YesPlease on osx-clang job for CI,\n + 2022-10-20)\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ Makefile: include shared.mak\n #\n -# Define NO_APPLE_COMMON_CRYPTO on OSX to opt-out of using the\n -# \"APPLE_COMMON_CRYPTO\" backend for SHA-1, which is currently the\n --# default on that OS. We'll define NO_APPLE_COMMON_CRYPTO on Mac OS\n --# 10.4 or older (\"Tiger\", released in early 2005).\n +-# default on that OS. On macOS 01.4 (Tiger) or older,\n +-# NO_APPLE_COMMON_CRYPTO is defined by default.\n +# Define APPLE_COMMON_CRYPTO_SHA1 to use Apple's CommonCrypto for\n +# SHA-1.\n #\n - # === SHA-256 backend ===\n - #\n + # If don't enable any of the *_SHA1 settings in this section, Git will\n + # default to its built-in sha1collisiondetection library, which is a\n @@ Makefile: ifdef NO_POSIX_GOODIES\n \tBASIC_CFLAGS += -DNO_POSIX_GOODIES\n endif\n @@ Makefile: ifdef BLK_SHA1\n else\n \n ## ci/lib.sh ##\n -@@ ci/lib.sh: macos-latest)\n - esac\n - \n - case \"$jobname\" in\n -+osx-clang)\n -+\tMAKEFLAGS=\"$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes\"\n -+\t;;\n - linux32)\n - \tCC=gcc\n +@@ ci/lib.sh: macos-*)\n + \t\tMAKEFLAGS=\"$MAKEFLAGS PYTHON_PATH=$(which python3)\"\n + \telse\n + \t\tMAKEFLAGS=\"$MAKEFLAGS PYTHON_PATH=$(which python2)\"\n +-\t\tMAKEFLAGS=\"$MAKEFLAGS NO_APPLE_COMMON_CRYPTO=NoThanks\"\n +-\t\tMAKEFLAGS=\"$MAKEFLAGS NO_OPENSSL=NoThanks\"\n ++\t\tMAKEFLAGS=\"$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes\"\n + \tfi\n \t;;\n + esac\n\n Makefile | 10 ++++------\n ci/lib.sh | 3 +--\n 2 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 0f7d7ab1fd2..db447d07383 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -511,10 +511,8 @@ include shared.mak\n # Define BLK_SHA1 to make use of optimized C SHA-1 routines bundled\n # with git (in the block-sha1/ directory).\n #\n-# Define NO_APPLE_COMMON_CRYPTO on OSX to opt-out of using the\n-# \"APPLE_COMMON_CRYPTO\" backend for SHA-1, which is currently the\n-# default on that OS. On macOS 01.4 (Tiger) or older,\n-# NO_APPLE_COMMON_CRYPTO is defined by default.\n+# Define APPLE_COMMON_CRYPTO_SHA1 to use Apple's CommonCrypto for\n+# SHA-1.\n #\n # If don't enable any of the *_SHA1 settings in this section, Git will\n # default to its built-in sha1collisiondetection library, which is a\n@@ -1911,7 +1909,7 @@ ifdef NO_POSIX_GOODIES\n \tBASIC_CFLAGS += -DNO_POSIX_GOODIES\n endif\n \n-ifdef APPLE_COMMON_CRYPTO\n+ifdef APPLE_COMMON_CRYPTO_SHA1\n \t# Apple CommonCrypto requires chunking\n \tSHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L\n endif\n@@ -1928,7 +1926,7 @@ ifdef BLK_SHA1\n \tLIB_OBJS += block-sha1/sha1.o\n \tBASIC_CFLAGS += -DSHA1_BLK\n else\n-ifdef APPLE_COMMON_CRYPTO\n+ifdef APPLE_COMMON_CRYPTO_SHA1\n \tCOMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL\n \tBASIC_CFLAGS += -DSHA1_APPLE\n else\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 706e3ba7e93..db7105e8a8d 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -258,8 +258,7 @@ macos-*)\n \t\tMAKEFLAGS=\"$MAKEFLAGS PYTHON_PATH=$(which python3)\"\n \telse\n \t\tMAKEFLAGS=\"$MAKEFLAGS PYTHON_PATH=$(which python2)\"\n-\t\tMAKEFLAGS=\"$MAKEFLAGS NO_APPLE_COMMON_CRYPTO=NoThanks\"\n-\t\tMAKEFLAGS=\"$MAKEFLAGS NO_OPENSSL=NoThanks\"\n+\t\tMAKEFLAGS=\"$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes\"\n \tfi\n \t;;\n esac\n-- \n2.39.0.rc2.1048.g0e5493b8d5b\n\n"},{"id":"469142","messageId":"xmqqlen8s6xv.fsf@gitster.g","threadId":"58661","inReplyTo":"patch-v2-1.1-3de7cdbd260-20221215T084129Z-avarab@gmail.com","subject":"Re: [PATCH v2] Makefile: use sha1collisiondetection by default on OSX and Darwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-15T21:17:16Z","receivedAt":"2022-12-15T21:17:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Let's also use sha1collisiondetection on OSX, to do so we'll need to\n> split up the \"APPLE_COMMON_CRYPTO\" flag into that flag and a new\n> \"APPLE_COMMON_CRYPTO_SHA1\".\n\nThat's well reasoned\n\nThis leaves these in Makefile:\n\n # Define NO_APPLE_COMMON_CRYPTO if you are building on Darwin/Mac OS X\n # and do not want to use Apple's CommonCrypto library. This allows you\n # to provide your own OpenSSL library, for example from MacPorts.\n\n ifndef NO_APPLE_COMMON_CRYPTO\n NO_OPENSSL = YesPlease\n APPLE_COMMON_CRYPTO = YesPlease\n COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO\n endif\n\nBecause we only mention what it does to use NO_APPLE_COMMON_CRYPTO\nhere, without promising anything about what happens otherwise, we\ncan do this change without breaking any promises ;-)\n\nHowever there is this bit:\n\n ifdef APPLE_COMMON_CRYPTO\n LIB_4_CRYPTO += -framework Security -framework CoreFoundation\n endif\n\nSo, if one says \n\n make NO_APPLE_COMMON_CRYPTO=NoThanks APPLE_COMMON_CRYPTO_SHA1=YesPlease\n\npresumably the end result will fail to link? I _think_ that falls\ninto \"if it hurts, don't do that\", and \n\n (1) automatically disabling the latter when the former is set would\n be more confusing than helpful.\n\n (2) explicitly detecting the situation and issue an error message\n from the Makefile might appear nicer, but if the linker does\n the failing with a messaage fine, that would be sufficient.\n\nso in the end, the posted patch as-is should be fine, I think.\n"}]}