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

build broken on windows #479

Closed
c-cube opened this issue Apr 14, 2022 · 16 comments · Fixed by #482 or #483
Closed

build broken on windows #479

c-cube opened this issue Apr 14, 2022 · 16 comments · Fixed by #482 or #483
Labels
bug Something isn't working

Comments

@c-cube
Copy link

c-cube commented Apr 14, 2022

e.g. https://github.com/c-cube/ocaml-containers/runs/6017282430?check_suite_focus=true , it's recent (was working fine a week ago I think).

edit: after disabling windows, turns out it's also broken on linux (was hidden by failfast):
https://github.com/c-cube/linol/runs/6017287696?check_suite_focus=true

@tatchi
Copy link

tatchi commented Apr 14, 2022

@dra27
Copy link
Member

dra27 commented Apr 14, 2022

The problem is with git - something has changed with git 2.35.2 which appears to be affecting opam. Pinning it to the previous release in dra27/setup-ocaml@pin-git appears to fix opam - see @c-cube's run on my fork in https://github.com/dra27/ocaml-containers/runs/6022886774?check_suite_focus=true

@dra27
Copy link
Member

dra27 commented Apr 14, 2022

Git 2.35.2 was updated in Cygwin 2 days ago (see https://cygwin.com/packages/summary/git.html)

@c-cube
Copy link
Author

c-cube commented Apr 14, 2022

wow, that's surprising. Could setup-ocaml use a lockfile to avoid this?

@dra27
Copy link
Member

dra27 commented Apr 14, 2022

This is all a consequence of CVE-2022-24765 (MITRE), I think. I haven't looked at the full gory details yet, but I think that while the problem principally affects Git for Windows, the core issue affects all OSes, and I think the fix is universal (which might be why Cygwin's git is suddenly doing this too)

@JasonGross
Copy link

Probably related to actions/checkout#760 and actions/checkout#767

@dra27
Copy link
Member

dra27 commented Apr 16, 2022

Your workaround looks sensible, @JasonGross (see similar change in ocaml/opam#5119).

I’m not at this stage certain exactly what (if anything) opam itself should do about this. For a CI runner, this whole security check seems to be something that just wants completely disabling; for a local user, opam should absolutely not be setting global git configuration, so it may (painfully) be that Windows users have to get used to this.

@JasonGross
Copy link

actions/checkout#762 fixed the issue by making a backup of the global config, then editing it, then restoring the backup. The issue only shows up when the owner of the git directory is not the current user, I believe, so Windows users in general probably won't run into this problem so much? Probably opam should check for this and emit a better warning / error message in general.

@smorimoto
Copy link
Member

As long as you're using checkout v3, it looks like this has already been fixed in the upstream. If the issue still persists, let us know.

@c-cube
Copy link
Author

c-cube commented Apr 17, 2022

https://github.com/c-cube/ocaml-containers/runs/6051443798?check_suite_focus=true still seems broken to me (I just tried to update checkout to v3)

@JasonGross
Copy link

As long as you're using checkout v3, it looks like this has already been fixed in the upstream. If the issue still persists, let us know.

The upstream fix is only a fix for the checkout action, and does not fix uses of git in other steps. Please reopen this issue

@dra27 dra27 added bug Something isn't working and removed upstream labels Apr 17, 2022
@dra27 dra27 reopened this Apr 17, 2022
@c-cube
Copy link
Author

c-cube commented Apr 17, 2022

Do we need a new release to observe the fix?

@dra27
Copy link
Member

dra27 commented Apr 17, 2022

Yes, unless you temporarily use setup-ocaml@master (you don’t need to test it, though, as I used my fork of ocaml-containers to test the fix 🙂)

@c-cube
Copy link
Author

c-cube commented Apr 17, 2022

Yes, unless you temporarily use setup-ocaml@master (you don’t need to test it, though, as I used my fork of ocaml-containers to test the fix slightly_smiling_face)

Awesome, I'll just wait for a release then 🙂 . Thank you and @smorimoto for the quick fixes.

@smorimoto
Copy link
Member

Okay, the release just arrived!
https://github.com/ocaml/setup-ocaml/releases/tag/v2.0.3

@smorimoto
Copy link
Member

As always, the v2 tag has already moved to that point!

@smorimoto smorimoto linked a pull request Apr 18, 2022 that will close this issue
@c-cube c-cube mentioned this issue Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
5 participants