Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For Apple M1, binary substitution breaks codesign #5650

Closed
bobot opened this issue May 3, 2022 · 27 comments · Fixed by #6137 or ocaml/opam-repository#22317
Closed

For Apple M1, binary substitution breaks codesign #5650

bobot opened this issue May 3, 2022 · 27 comments · Fixed by #6137 or ocaml/opam-repository#22317

Comments

@bobot
Copy link
Collaborator

bobot commented May 3, 2022

In #4389 and #4135 problem between dune substitutions and codesign has been raised. But unfortunately both times another bug was also the culprit. It is time for dune to call codesign -s - $binary automatically after substitution.

  • applying only for architecture = arm64 and system = macosx

However I don't know on which files it should be applied:

  • should we do it only for substituted files for which the source is verfied (codesign -v <path>)
  • for all executables?

Also the CI doesn't check that? Could we test on M1?

@correnson

@bobot
Copy link
Collaborator Author

bobot commented May 3, 2022

@correnson
Copy link

correnson commented May 4, 2022

I can confirm the bug:

  • the freshly build or promoted binary is always correctly signed;
  • if no relocation is performed by dune, the installed binary is OK;
  • the installed relocated binary is no more signed for macOS M1, be it promoted or not.

I think the esy methods is OK for determining whether to sign or not: they check the platform by running uname, which returns Darwin for macOS and the architecture by running uname -m, which returns x86_64 on i86 and arm64 on M1.

@bobot
Copy link
Collaborator Author

bobot commented May 4, 2022

What is returned by ocamlopt -config | grep -e system -e architecture on both system?

@correnson
Copy link

Using ocamlopt -config seems also ok, relevant fields are architecture and system:

architecture: amd64 | arm64
system: macosx

@bobot
Copy link
Collaborator Author

bobot commented May 7, 2022

However I don't know on which files it should be applied:

  • should we do it only for substituted files for which the source is verfied (codesign -v <path>)
  • for all executables?

@correnson
Copy link

I think only (and systematically) for executables that are rewritten on-the-fly by dune. Others are correctly signed by the compiler.

@toots
Copy link
Contributor

toots commented May 21, 2022

+1 here, got hit by the issue today.

@Blaisorblade
Copy link
Contributor

I think I'm hit by this issue as well, and it interferers with developing dune itself...

@toots
Copy link
Contributor

toots commented Aug 31, 2022

Any update on this? This hit us again in dev and will be problem releasing our next major version. Thanks y'all!

@emillon
Copy link
Collaborator

emillon commented Sep 7, 2022

I don't have a mac to test this, but here are some notes if someone wants to give it a go (or verify a PR):

  • I think that the piece of code to modify is Artifact_substitution.copy_file: after the call to copy_file_non_atomic, insert a call that signs the temporary file
  • signing is a noop unless we're on arm and at least big sur (technically this applies to the target toolchain, but we don't have a way codesign when crosscompiling anyway).
  • esy's test is good enough to start but I wonder if there's a platform API we can call to determine whether codesigning is mandatory on a given version of mac os
  • the codesign commandline used by esy is here, we can use that
  • my understanding is that the error message described in that file applies when rewriting is done in-place. in that case the kernel's cache mapping inodes to signatures gets confused. we're not affected since we're already doing the rewriting to a temporary file
  • there's the question of which identity to use for signing. esy uses - which corresponds to ad-hoc signing. this works I wonder if we can use the same identity as the one in the original file. What is the identity used in the pristine binaries? does codesign -v (or -vv) display it?

@Et7f3
Copy link
Contributor

Et7f3 commented Sep 7, 2022

Is their a test/repoduction that show the failure ?

@emillon
Copy link
Collaborator

emillon commented Sep 7, 2022

I don't think so. We don't have M1 CI so the test would not fail in CI, but it's a good first step if there's no test that attempts to run a substituted binary for now.

@Et7f3
Copy link
Contributor

Et7f3 commented Sep 7, 2022

M1 CI doesn't seem to be a thing yet actions/runner-images#2187 so the first step is to add a substitute binary test ?

@emillon
Copy link
Collaborator

emillon commented Sep 7, 2022

Yes I think that this would be important to add.

@Blaisorblade
Copy link
Contributor

Blaisorblade commented Sep 7, 2022

but it's a good first step if there's no test that attempts to run a substituted binary for now.

Not to dissuade you from your plans, but I think such tests must already exist — last I tried, quite a few tests segfaulted on generated binaries; I confirmed that manually running codesign* on those binaries let them run correctly.

*It must have been something simple like codesign -s -, as I didn't setup any custom identity.

@correnson
Copy link

correnson commented Sep 7, 2022

  • Don't need a CI here, using codesign - definitely works (this is what we do manually to fix the issue)
  • For identification via commands, uname = Darwin and uname -m = arm64 identifies a mac M1.
  • For identification via ocaml config, system: macos and architecture: arm64 identifies a mac M1.

@emillon
Copy link
Collaborator

emillon commented Sep 7, 2022

thanks, it's good to know that the current test suite already catches that on M1. can anyone paste the output of codesign -v and codesign -vv on a pristine binary produced by ocamlopt?

@Alizter
Copy link
Collaborator

Alizter commented Sep 7, 2022

Regarding M1 runners, Github does have support for them you just have to provide them yourself I think. We could ask OCamllabs or whoever appropriate for some. cc @rgrinberg

@toots
Copy link
Contributor

toots commented Sep 7, 2022

Thanks for looking at this y'all. I'll try to give it a pass when I get some time back on the M1.

I believe that binaries generated during a local opam build are not really intended to be distributed outside of the local machine at this would require much more work with dynamically linked libraries, binary hardening, etc. so self signing sounds like a pretty acceptable solution.

I'd also caution that this might not be only for M1 lacs only in the future but any Mac with a recent enough macos.

@emillon
Copy link
Collaborator

emillon commented Sep 8, 2022

Can anyone try #6137 to see if it works? Also still interested in the output of codesign -v / -vv for a normal ocaml binary. Thanks!

@samoht
Copy link
Member

samoht commented Sep 8, 2022

Without #6137

❯ dune exec -- cohttp-lwt-unix/bin/cohttp_curl_lwt.exe --help
❯ codesign -v _build/default/cohttp-lwt-unix/bin/cohttp_curl_lwt.exe
❯ codesign -vv _build/default/cohttp-lwt-unix/bin/cohttp_curl_lwt.exe
_build/default/cohttp-lwt-unix/bin/cohttp_curl_lwt.exe: valid on disk
_build/default/cohttp-lwt-unix/bin/cohttp_curl_lwt.exe: satisfies its Designated Requirement

@samoht
Copy link
Member

samoht commented Sep 8, 2022

@emillon is this what you are looking after?

❯ ocamlopt foo.ml -o foo
❯ ./foo
❯ codesign -v ./foo
❯ codesign -vv ./foo
./foo: valid on disk
./foo: satisfies its Designated Requirement
❯ cat foo.ml
let () = ()

@emillon
Copy link
Collaborator

emillon commented Sep 8, 2022

thanks. can you attach the output of codesign -vvvv as well? I'd like to see if it can print the identity it's been signed with

@samoht
Copy link
Member

samoht commented Sep 8, 2022

It was tricky to find but the magic argument is --dispay :-)

❯ codesign --verbose=4 --display foo
Executable=/Users/thomas/git/cohttp/foo
Identifier=foo
Format=Mach-O thin (arm64)
CodeDirectory v=20400 size=3324 flags=0x20002(adhoc,linker-signed) hashes=101+0 location=embedded
VersionPlatform=1
VersionMin=786432
VersionSDK=787200
Hash type=sha256 size=32
CandidateCDHash sha256=9ac8f2b102c6f2b8dafc777574099063f0e209f9
CandidateCDHashFull sha256=9ac8f2b102c6f2b8dafc777574099063f0e209f97f21d7afe4986451a0c30ab3
Hash choices=sha256
CMSDigest=9ac8f2b102c6f2b8dafc777574099063f0e209f97f21d7afe4986451a0c30ab3
CMSDigestType=2
Executable Segment base=0
Executable Segment limit=196608
Executable Segment flags=0x1
Page size=4096
CDHash=9ac8f2b102c6f2b8dafc777574099063f0e209f9
Signature=adhoc
Info.plist=not bound
TeamIdentifier=not set
Sealed Resources=none
Internal requirements=none

@emillon
Copy link
Collaborator

emillon commented Sep 8, 2022

Great! so this confirms that this is an adhoc signature as well. so we don't have to be more clever about that part. thanks!

@toots
Copy link
Contributor

toots commented Sep 13, 2022

Can anyone try #6137 to see if it works?

This seems to work here!

emillon added a commit to emillon/dune that referenced this issue Oct 5, 2022
@toots
Copy link
Contributor

toots commented Oct 5, 2022

Dope, thank you all for working on this!

emillon added a commit to emillon/opam-repository that referenced this issue Oct 11, 2022
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0~alpha1)

CHANGES:

- Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg)

- Add a `runtime_deps` field in the `cinaps` stanza to specify runtime
  dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg)

- Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg)

- Extend dune describe to include the root path of the workspace and the
  relative path to the build directory. (ocaml/dune#6136, @reubenrowe)

- Allow dune describe workspace to accept directories as arguments.
  The provided directories restrict the worskpace description to those
  directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope)

- Add a terminal persistence mode that attempts to clear the terminal history.
  It is enabled by setting terminal persistence to
  `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg)

- Disallow generating targets in sub direcories in inferred rules. The check to
  forbid this was accidentally done only for manually specified targets (ocaml/dune#6031,
  @rgrinberg)

- Do not ignore rules marked `(promote (until-clean))` when
  `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon)

- Dune no longer considers .aux files as targets during Coq compilation. This
  means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter)

- Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg)

- Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056,
  @rgrinberg)

- Introduce a `dirs` field in the `install` stanza to install entire
  directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg)

- Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg)

- Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes
  ocaml/dune#5945, @rgrinberg)

- Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg)

- Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine)

- Add an `(include <file>)` term to the `include_dirs` field for adding
  directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993,
  @gridbugs)

- Support `(extra_objects ...)` field in `(executable ...)` and `(library
  ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs)

- Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb)

- Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114,
  fixes ocaml/dune#6103, @emillon)

- odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes
  ocaml/dune#1117, @emillon)

- dune install: copy files in an atomic way (ocaml/dune#6150, @emillon)

- Add `%{coq:...}` macro for accessing data about the configuration about Coq.
  For instance `%{coq:version}` (ocaml/dune#6049, @Alizter)

- update vendored copy of cmdliner to 1.1.1. This improves the built-in
  documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon,
  ocaml/dune#6169, @shonfeder)

- The test suite for Coq now requires Coq >= 8.16 due to changes in the
  plugin loading mechanism upstream (which now uses findlib).

- Starting with Coq build language 0.6, theories can be built without importing
  Coq's standard library by including `(stdlib no)`.
  (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek)

- on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, fixes
  ocaml/dune#5650, @emillon)

- Added an (aliases ...) field to the (rules ...) stanza which allows the
  specification of multiple aliases per rule (ocaml/dune#6194, @Alizter)
emillon added a commit to emillon/opam-repository that referenced this issue Oct 19, 2022
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0)

CHANGES:

- macOS: Handle unknown fsevents without crashing (ocaml/dune#6217, @rgrinberg)

- Enable file watching on MacOS SDK < 10.13. (ocaml/dune#6218, @rgrinberg)

- Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg)

- Add a `runtime_deps` field in the `cinaps` stanza to specify runtime
  dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg)

- Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg)

- Extend dune describe to include the root path of the workspace and the
  relative path to the build directory. (ocaml/dune#6136, @reubenrowe)

- Allow dune describe workspace to accept directories as arguments.
  The provided directories restrict the worskpace description to those
  directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope)

- Add a terminal persistence mode that attempts to clear the terminal history.
  It is enabled by setting terminal persistence to
  `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg)

- Disallow generating targets in sub direcories in inferred rules. The check to
  forbid this was accidentally done only for manually specified targets (ocaml/dune#6031,
  @rgrinberg)

- Do not ignore rules marked `(promote (until-clean))` when
  `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon)

- Dune no longer considers .aux files as targets during Coq compilation. This
  means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter)

- Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg)

- Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056,
  @rgrinberg)

- Introduce a `dirs` field in the `install` stanza to install entire
  directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg)

- Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg)

- Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes
  ocaml/dune#5945, @rgrinberg)

- Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg)

- Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine)

- Add an `(include <file>)` term to the `include_dirs` field for adding
  directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993,
  @gridbugs)

- Support `(extra_objects ...)` field in `(executable ...)` and `(library
  ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs)

- Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb)

- Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114,
  fixes ocaml/dune#6103, @emillon)

- odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes
  ocaml/dune#1117, @emillon)

- dune install: copy files in an atomic way (ocaml/dune#6150, @emillon)

- Add `%{coq:...}` macro for accessing data about the configuration about Coq.
  For instance `%{coq:version}` (ocaml/dune#6049, @Alizter)

- update vendored copy of cmdliner to 1.1.1. This improves the built-in
  documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon,
  ocaml/dune#6169, @shonfeder)

- The test suite for Coq now requires Coq >= 8.16 due to changes in the
  plugin loading mechanism upstream (which now uses `Findlib`).

- Starting with Coq build language 0.6, theories can be built without importing
  Coq's standard library by including `(stdlib no)`.
  (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek)

- on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, ocaml/dune#6231,
  fixes ocaml/dune#5650, fixes ocaml/dune#6226, @emillon)

- Added an (aliases ...) field to the (rules ...) stanza which allows the
  specification of multiple aliases per rule (ocaml/dune#6194, @Alizter)

- The `(coq.theory ...)` stanza will now ensure that for each declared `(plugin
 ...)`, the `META` file for it is built before calling `coqdep`. This enables
 the use of the new `Findlib`-based loading method in Coq 8.16; however as of
 Coq 8.16.0, Coq itself has some bugs preventing this to work yet. (ocaml/dune#6167 ,
 workarounds ocaml/dune#5767, @ejgallego)

- Allow include statement in install stanza (ocaml/dune#6139, fixes ocaml/dune#256, @gridbugs)

- Handle CSI n K code in ANSI escape codes from commands. (ocaml/dune#6214, fixes ocaml/dune#5528,
  @emillon)

- Add a new experimental feature `mode_specific_stubs` that allows the
  specification of different flags and sources for foreign stubs depending on
  the build mode (ocaml/dune#5649, @voodoos)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment