-
Notifications
You must be signed in to change notification settings - Fork 303
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
update to a newer version of git #535
base: master
Are you sure you want to change the base?
Conversation
fadb9cb
to
997bfdd
Compare
With this, we are forcing Doodba users to have such git version in their host system? Can you check CI? |
Sorry, wrong button. |
As far as i known this would not force users to have such a version on their host system. I intended to document why i chose the arbitrary version of git in this commit. I will check why the CI is failing for this. |
997bfdd
to
3d44b08
Compare
Fixed the tests by making sure the image is built before inspecting the layers. On our CI this didn't fail (last week) because the image from a previous run was inspected instead (but i would have failed at some point). |
Thanks for that. And finally, what is the reason for changing the git version? How it impacts on Doodba (for what is used this git)? |
The reason is that the version of git in the doodba images is affected by the following security issue: Basically it could be possible to achieve RCE via .gitattributes (using integer overflow in git). Fixes exist for git, git (ubuntu), gitlab, github but not for the debian version of git. There seem to be to few maintainers in debian that can backport the git security fixes (only applied to 2.30+) to the version used in debian (2.20). |
OK, I see. Now I'm a bit worried on the increase on the build time, having to compile git, and the need of the full build environment. Have you measured how much more time is needed? |
Unfortunately no i cannot do any reliable performance tests. Our build systems are too different and also the amount of work done in the build differs a lot run by run. From the runtimes of the runs with the git build and without i would say it takes a few minutes (3-5) longer on our CI Systems (Ryzen 1700X, Ryzen 3900, Xeon E-2176). The image sizes on our registry increased by about 50MB (11.0 + 12.0) or 30MB (13.0+). |
Not sure about the trade-off, as exploiting such bug in this environment is very unlikely if I understand it correctly. You need at least permission to push to the scaffolding repo, and deploy such changes. Don't we expect any binary soon on the distribution? What is your opinion on this, @yajo? |
@pedrobaeza the only thing that needs to happen is for someone using a not 100% trusted repo in their repos.yaml and someone changing the .gitattributes in one of the repos. By now debian has a patch for the maintained versions (not stretch used in the 11.0 and 12.0 images) so if someone triggers to build new images theoretically (please verify manually) the version should be patched. |
So this is only needed in 11.0 and 12.0? |
Well that depends, i do find it easier to have the same git version in all Odoo versions when scripting stuff (like gitaggregate but also our custom scripts), but for security reasons, yes it is now only needed in 11.0 and 12.0 it seems. |
Then given the trade-off, I prefer to just patch 11.0 and 12.0. The scripts you mention will be very rare that needs a specific feature introduced in the new versions and the command line is very unlikely to change. |
I agree on fixing the vulnerability. Also on applying the patch only to 11.0 and 12.0 if the other versions are unaffected. If you're worried about the build time increase, maybe one option would be to directly download the newer |
Let's go then for now with the 11.0/12.0 option compiling it for not causing you more work, but if the deb works, it can be also interesting. |
3d44b08
to
c3eea7e
Compare
Isn't this still compiling GIT for all the versions? |
I would prefer to have one git version in all versions of doodba as it makes scripting/maintaining way easier. It would feel strange/unexpected to me for the older versions to have a newer git than the newer versions. This version of the patch can be maintained with new patch versions of git with minimal maintenance effort whereas i fear that a split up version is not as easy to create merge requests from our merge requests for. That's why i would prefer to add it in the current state. But feel free to fix it any other way. |
Git version 2.37.5 was chosen because it matches Ubuntu 22.10 2.37.2 + patches the closest (that we use on our workstation for Ryzen 7700x support) and includes the latest fixes.
As we couldn't see any movement in debian in either https://security-tracker.debian.org/tracker/CVE-2022-23521 nor https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1029114 we fixed the git version on our systems this way.
Info @wt-io-it