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

golang: update to v1.19.2 #19652

Merged
merged 2 commits into from
Nov 24, 2022
Merged

Conversation

wormi4ok
Copy link
Contributor

@wormi4ok wormi4ok commented Oct 20, 2022

Maintainer: @jefferyto
Compile tested: -
Run tested: -

Description:
Bump up the Golang version to the latest stable.

Includes fixes for security vulnerabilities:

  • CVE-2022-27664 net/http: handle server errors after sending GOAWAY
  • CVE-2022-32190 net/url: JoinPath does not strip relative path components in all circumstances
  • CVE-2022-2879 archive/tar: unbounded memory consumption when reading headers
  • CVE-2022-2880 net/http/httputil: ReverseProxy should not forward unparseable query parameters
  • CVE-2022-41715 regexp/syntax: limit memory used by parsing regexps

Addresses the build failure:

Signed-off-by: Stanislav Petrashov s@petrashov.ru

@BKPepe
Copy link
Member

BKPepe commented Oct 20, 2022

This is a huge update and it needs to be compile and run tested.

@BKPepe
Copy link
Member

BKPepe commented Oct 29, 2022

Kind reminder here, please.

@wormi4ok
Copy link
Contributor Author

I've tried to reproduce this problem locally using nektos/act, but no luck. The original CI build fails with an error message Dirty patches detected, please refresh and review the diff, so the error is related to the custom patch, but it's hard to troubleshoot without reproducing. Is there any good article on how to set up a similar CI environment?

@1715173329
Copy link
Member

This package was recently updated, please rebase it.
You can run with make package/golang/refresh V=s to refresh patches.

I just made a simple compile test, it did compile and run.
However, from the release note, there were some breaking changes and I'm not sure if we need adjust the makefiles.

Includes fixes for security vulnerabilities:
 * [CVE-2022-27664](GHSA-69cg-p879-7622) net/http: handle server errors after sending GOAWAY
 * [CVE-2022-32190](golang/go#54385) net/url: JoinPath does not strip relative path components in all circumstances
 * [CVE-2022-2879](golang/go#54853) archive/tar: unbounded memory consumption when reading headers
 * [CVE-2022-2880](golang/go#54663) net/http/httputil: ReverseProxy should not forward unparseable query parameters
 * [CVE-2022-41715](golang/go#55949) regexp/syntax: limit memory used by parsing regexps

Addresses the build failure:
* openwrt#19613

Signed-off-by: Stanislav Petrashov <s@petrashov.ru>
@wormi4ok
Copy link
Contributor Author

Thanks a lot for the advice, @1715173329! I've advanced in setting up OpenWRT buildroot locally and could get the patch updated. But I still haven't run-tested this new version.

@BKPepe
Copy link
Member

BKPepe commented Nov 14, 2022

Unfortunately, we can not merge this unless this is confirmed that it works on the real device.

@CAFxX
Copy link
Contributor

CAFxX commented Nov 15, 2022

(Total newbie here, so may be missing something obvious... if so just consider it a drive-by comment about contribution friction)

Unfortunately, we can not merge this unless this is confirmed that it works on the real device.

What is considered the "real device" in this case? AFAICT a change like this potentially affects every architecture/device and every package that uses golang as compiler... but testing all of them (and all their combinations) is almost certainly impossible. If I were to guess I would say it means any golang-built package on any "real device" that is not x86/amd64/arm/aarch64 based1, but I can't find it described anywhere:

If I did not miss it, maybe having a page describing how to effectively do real device testing, and ideally that also offers the prepackaged tools to get started, would help lower the barrier to entry. (If I did miss it, then maybe making it more prominent/visible would help?)

Footnotes

  1. as golang support for these architectures is heavily tested/used elsewhere, so we're unlikely to hit bugs on these; golang support for other archs like mips is instead much less widely tested, so it makes sense to focus testing there

GO_VERSION_MAJOR_MINOR:=1.18
GO_VERSION_PATCH:=8
GO_VERSION_MAJOR_MINOR:=1.19
GO_VERSION_PATCH:=2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, .3 is already out so maybe if testing has not been done yet it makes sense to target .3 instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@1715173329
Copy link
Member

I have applied this commit to our downstream project, and so far so good.
Only ooniprobe failed to build1 due to outdated dependency, I believe this can be resolved by bumping to recent version.

Footnotes

  1. https://downloads.immortalwrt.org/snapshots/faillogs/aarch64_cortex-a53/packages/ooniprobe/compile.txt

@1715173329
Copy link
Member

I think this is ready to go. But before merging it, please update to the latest version 1.19.3.

Signed-off-by: Stanislav Petrashov <s@petrashov.ru>
@1715173329 1715173329 merged commit 98e12e5 into openwrt:master Nov 24, 2022
@1715173329
Copy link
Member

Merged, thanks!

@jefferyto
Copy link
Member

@BKPepe why did you cherry pick this into 22.03? And why did you cherry pick Go 1.18 into 21.02?

@BKPepe
Copy link
Member

BKPepe commented Mar 10, 2023

Hey @jefferyto,

It's nice to hear that you are back! 👏 We missed you here.

But get back to your questions, yes. I backported Golang updates in both stable releases because several people asked me if I could update tailscale to its newest version. Unfortunately, it required updating Go and also some packages as they could not be compiled with that version. On the other hand, users got updated Golang, which brings multiple security fixes as their old version of Go was no longer supported.

@jefferyto
Copy link
Member

@BKPepe

several people asked me if I could update tailscale to its newest version

The answer should have been no and:

  • Compile your own packages
  • Run snapshots on your devices
  • Harass the core developers to make releases on a faster, more predictable schedule

On the other hand, users got updated Golang, which brings multiple security fixes as their old version of Go was no longer supported.

By this logic we should upgrade all packages in all release branches all the time.

The reasons to have release branches are many:

  • Developers get to focus their limited resources on the next release. By putting a newer Go into previous branches, we have to continue updating those packages for longer than they were suppose to be updated. That is time and effort that could have gone into doing something else.

  • Users get releases that are known, to the best of our knowledge, to be stable and working. By putting a newer Go into previous branches, all of the other Go programs are suddenly compiled with a newer version of Go. How do we know this won't cause new problems? (Just because there are no build-time errors doesn't mean there will be no run-time errors.)

    The place to make these kinds of changes is the "master" branch, run-tested by people who install snapshots and have the expectation that things may break.

If there are security fixes that are missing from previous branches, then those fixes should be backported as patches.

I understand users want things but that doesn't mean we just do what the few want without considering what is best for all users, in particular the many thousands, if not millions, of users who don't speak up here and just want their routers to be stable and run without problems.

OpenWrt is a major, if not the main, Linux distribution for routers and other devices. This wouldn't have been allowed to happen in Debian or Fedora or any other major distro. I wish people here would start acting like they are in charge of a major distro.

1715173329 referenced this pull request Apr 17, 2023
Included fixes for:
- CVE-2023-24534
- CVE-2023-24536
- CVE-2023-24537
- CVE-2023-24538

Refreshed patches.

Signed-off-by: Tianling Shen <cnsztl@immortalwrt.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants