Hi Erik, Magnus, Thank you for reviewing this. Please let me know what's the next step and if you'd like me to help with backporting this change or anything.
Thanks, Junyuan From: Erik Joelsson <[email protected]> Sent: Friday, February 28, 2020 6:26 AM To: Magnus Ihse Bursie <[email protected]>; Junyuan Zheng <[email protected]>; [email protected] <[email protected]> Subject: [EXTERNAL] Re: macOS build success but codesign fail on macOS 10.13.5 or older On 2020-02-28 01:04, Magnus Ihse Bursie wrote: > On 2020-02-28 09:59, Magnus Ihse Bursie wrote: >> On 2020-02-27 16:07, Erik Joelsson wrote: >>> On 2020-02-27 06:16, Magnus Ihse Bursie wrote: >>>> I don't think it should be a fatal error. If you have a codesign >>>> binary on your path that does not support --option runtime, you >>>> should still be able to build, but not sign. Change it to a >>>> warning, and let the user continue without CODESIGN. >>>> >>> My interpretation of this patch is that the new check is only >>> performed if a valid --with-macosx-codesign-identity was provided, >>> so the user has clearly requested signing to be performed. In that >>> case I agree that it should error out. >> >> I'm sorry Erik, but that is not open to "interpretation". Look at the >> code: >> >> UTIL_PATH_PROGS(CODESIGN, codesign) >> >> if test "x$CODESIGN" != "x"; then >> # Check for user provided code signing identity. >> # If no identity was provided, fall back to "openjdk_codesign". >> AC_ARG_WITH([macosx-codesign-identity], >> [AS_HELP_STRING([--with-macosx-codesign-identity], >> [specify the code signing identity])], >> [MACOSX_CODESIGN_IDENTITY=$with_macosx_codesign_identity], >> [MACOSX_CODESIGN_IDENTITY=openjdk_codesign] >> ) >> >> AC_SUBST(MACOSX_CODESIGN_IDENTITY) >> >> # Verify that the codesign certificate is present >> AC_MSG_CHECKING([if codesign certificate is present]) >> $RM codesign-testfile >> $TOUCH codesign-testfile >> $CODESIGN -s "$MACOSX_CODESIGN_IDENTITY" codesign-testfile >> 2>&AS_MESSAGE_LOG_FD >&AS_MESSAGE_LOG_FD || CODESIGN= >> $RM codesign-testfile >> if test "x$CODESIGN" = x; then >> AC_MSG_RESULT([no]) >> else >> AC_MSG_RESULT([yes]) >> # Verify that the codesign has --option runtime >> AC_MSG_CHECKING([if codesign has --option runtime]) >> $RM codesign-testfile >> $TOUCH codesign-testfile >> $CODESIGN --option runtime -s "$MACOSX_CODESIGN_IDENTITY" >> codesign-testfile 2>&AS_MESSAGE_LOG_FD >&AS_MESSAGE_LOG_FD || CODESIGN= >> $RM codesign-testfile >> if test "x$CODESIGN" = x; then >> AC_MSG_ERROR([codesign does not have --option runtime. macOS >> 10.13.6 and above is required.]) >> else >> AC_MSG_RESULT([yes]) >> fi >> fi >> fi >> >> This means that if you have a binary named "codesign" on your path, >> and it does not accept the '--option runtime' argument, configure >> will fail. > Sorry, my bad: configure will fail if you have codesign, and > openjdk_codesign is a valid codesign identity, but --option runtime is > not supported. This does indeed limit the impact of this patch. > Nevertheless, I still think this is bad design. If the code would e.g. > check that --with-macosx-codesign-identity was explicitly given on the > command line, then it would be OK to fail. > I agree that an explicit check that --with-macosx-codesign-identity was set would make the code clearer. The point is that if you have a valid identity defined, but you are building on an older MacOS version so the --option runtime is not supported, the build will unconditionally fail at the first signing attempt anyway. This is the proper fail fast mechanism. On a side note, there is also no point in supporting signing on older macs without the --option runtime because such signatures are basically useless today. /Erik > /Magnus > >> >> This is not acceptable. >> >> However, I understand that there is a need to be able to *enforce* >> signing. I'm actually currently working with a patch that will add >> --enable-jdk-feature-codesign, and if that is enabled, configure will >> fail unless a working codesign binary and certificate is present. It >> will be easy to adapt this change as well. But in the meantime, the >> AC_MSG_ERROR must be changed to a warning. >> >> /Magnus >
