search

agola-io / agola

Agola: CI/CD Redefined
https://agola.io
Apache License 2.0
1.46k stars 116 forks source link

update golang.org/x/net to v0.23.0 #509

Closed katexochen closed 4 weeks ago

katexochen commented 1 month ago

Fixing https://pkg.go.dev/vuln/GO-2024-2687

Full scan output of govulncheck:

Scanning your code and 817 packages across 113 dependent modules for known vulnerabilities...

=== Symbol Results ===

Vulnerability #1: GO-2024-2687
    HTTP/2 CONTINUATION flood in net/http
  More info: https://pkg.go.dev/vuln/GO-2024-2687
  Module: golang.org/x/net
    Found in: golang.org/x/net@v0.8.0
    Fixed in: golang.org/x/net@v0.23.0
    Example traces found:
      #1: internal/services/executor/driver/k8s.go:107:41: driver.NewK8sDriver calls kubernetes.NewForConfig, which eventually calls http2.ConfigureTransports
      #2: internal/services/runservice/api/api.go:896:26: api.RunTaskActionsHandler.ServeHTTP calls http2.ConnectionError.Error
      #3: internal/services/executor/executor.go:1473:31: executor.NewExecutor calls fmt.Sprintf, which eventually calls http2.ErrCode.String
      #4: internal/services/executor/executor.go:1473:31: executor.NewExecutor calls fmt.Sprintf, which eventually calls http2.FrameHeader.String
      #5: internal/services/executor/executor.go:1473:31: executor.NewExecutor calls fmt.Sprintf, which eventually calls http2.FrameType.String
      #6: internal/services/runservice/api/api.go:896:26: api.RunTaskActionsHandler.ServeHTTP calls http2.GoAwayError.Error
      #7: internal/services/executor/executor.go:1473:31: executor.NewExecutor calls fmt.Sprintf, which eventually calls http2.Setting.String
      #8: internal/services/executor/executor.go:1473:31: executor.NewExecutor calls fmt.Sprintf, which eventually calls http2.SettingID.String
      #9: internal/services/runservice/api/api.go:896:26: api.RunTaskActionsHandler.ServeHTTP calls http2.StreamError.Error
      #10: internal/gitsources/github/github.go:106:23: github.TokenTransport.RoundTrip calls http.Transport.RoundTrip, which eventually calls http2.Transport.NewClientConn
      #11: internal/gitsources/github/github.go:106:23: github.TokenTransport.RoundTrip calls http.Transport.RoundTrip, which eventually calls http2.Transport.RoundTrip
      #12: internal/sqlg/manager/export.go:54:34: manager.DBManager.Export calls bufio.Writer.Flush, which calls http2.chunkWriter.Write
      #13: internal/services/runservice/api/api.go:896:26: api.RunTaskActionsHandler.ServeHTTP calls http2.connError.Error
      #14: internal/services/runservice/api/api.go:896:26: api.RunTaskActionsHandler.ServeHTTP calls http2.duplicatePseudoHeaderError.Error
      #15: internal/services/gateway/api/run.go:624:2: api.LogsHandler.ServeHTTP calls http2.gzipReader.Close
      #16: internal/gitsources/github/github.go:209:22: github.Client.GetFile calls io.ReadAll, which calls http2.gzipReader.Read
      #17: internal/services/runservice/api/api.go:896:26: api.RunTaskActionsHandler.ServeHTTP calls http2.headerFieldNameError.Error
      #18: internal/services/runservice/api/api.go:896:26: api.RunTaskActionsHandler.ServeHTTP calls http2.headerFieldValueError.Error
      #19: internal/gitsources/github/github.go:106:23: github.TokenTransport.RoundTrip calls http.Transport.RoundTrip, which eventually calls http2.noDialH2RoundTripper.RoundTrip
      #20: internal/services/runservice/api/api.go:896:26: api.RunTaskActionsHandler.ServeHTTP calls http2.pseudoHeaderError.Error
      #21: internal/sqlg/manager/export.go:54:34: manager.DBManager.Export calls bufio.Writer.Flush, which calls http2.stickyErrWriter.Write
      #22: internal/services/gateway/api/run.go:624:2: api.LogsHandler.ServeHTTP calls http2.transportResponseBody.Close
      #23: internal/gitsources/github/github.go:209:22: github.Client.GetFile calls io.ReadAll, which calls http2.transportResponseBody.Read
      #24: internal/services/executor/executor.go:1473:31: executor.NewExecutor calls fmt.Sprintf, which eventually calls http2.writeData.String

Your code is affected by 1 vulnerability from 1 module.
This scan also found 3 vulnerabilities in packages you import and 3
vulnerabilities in modules you require, but your code doesn't appear to call
these vulnerabilities.
Use '-show verbose' for more details.
sgotti commented 4 weeks ago

I don't think we are using http2 from golang.org/x/net but from the stdlib. So it's fixed by using an updated go version.

katexochen commented 4 weeks ago

This is NOT fixed by upgrading the stdlib. It is a vulnerability in a transitive dependency of your project.

govulncheck still finds this vulnerability on the latest commit (20f80cd29814e66e69b5b57f8fe33d037e4789eb) of your master branch, running with the latest available Go version. You can run that locally in your dev environment and will get the exact same error.

Check out your go.mod, which states that x/net is a transitive dependency of yours:

https://github.com/agola-io/agola/blob/20f80cd29814e66e69b5b57f8fe33d037e4789eb/go.mod#L131

You can use go mod graph | grep 'x/net' | grep -v -E '^golang.org/x/net' to figure out which of your dependencies is requiring x/net:

agola.io/agola golang.org/x/net@v0.17.0
github.com/containerd/containerd@v1.7.7 golang.org/x/net@v0.13.0
github.com/go-git/go-billy/v5@v5.5.0 golang.org/x/net@v0.15.0
github.com/go-git/go-git/v5@v5.9.0 golang.org/x/net@v0.15.0
github.com/google/go-containerregistry@v0.16.1 golang.org/x/net@v0.10.0
github.com/minio/minio-go/v6@v6.0.57 golang.org/x/net@v0.0.0-20190522155817-f3200d17e092
github.com/opencontainers/runc@v1.1.9 golang.org/x/net@v0.8.0
github.com/xanzy/go-gitlab@v0.93.1 golang.org/x/net@v0.8.0
golang.org/x/crypto@v0.14.0 golang.org/x/net@v0.10.0
golang.org/x/oauth2@v0.13.0 golang.org/x/net@v0.16.0
golang.org/x/tools@v0.14.0 golang.org/x/net@v0.16.0
k8s.io/api@v0.28.2 golang.org/x/net@v0.13.0
k8s.io/apimachinery@v0.28.2 golang.org/x/net@v0.13.0
k8s.io/client-go@v0.28.2 golang.org/x/net@v0.13.0
k8s.io/kube-openapi@v0.0.0-20231010175941-2dd684a91f00 golang.org/x/net@v0.7.0
golang.org/x/crypto@v0.0.0-20220525230936-793ad666bf5e golang.org/x/net@v0.0.0-20211112202133-69e39bad7dc2
golang.org/x/crypto@v0.3.0 golang.org/x/net@v0.2.0
golang.org/x/tools@v0.6.0 golang.org/x/net@v0.6.0
golang.org/x/crypto@v0.7.0 golang.org/x/net@v0.8.0
golang.org/x/crypto@v0.3.1-0.20221117191849-2c476679df9a golang.org/x/net@v0.2.0
golang.org/x/crypto@v0.0.0-20210513164829-c07d793c2f9a golang.org/x/net@v0.0.0-20210226172049-e18ecbb05110
golang.org/x/crypto@v0.0.0-20200622213623-75b288015ac9 golang.org/x/net@v0.0.0-20190404232315-eb5bcb51f2a3
golang.org/x/tools@v0.0.0-20210106214847-113979e3529a golang.org/x/net@v0.0.0-20201021035429-f5854403a974
golang.org/x/crypto@v0.0.0-20190513172903-22d7a77e9e5f golang.org/x/net@v0.0.0-20190404232315-eb5bcb51f2a3
golang.org/x/crypto@v0.0.0-20220622213112-05595931fe9d golang.org/x/net@v0.0.0-20211112202133-69e39bad7dc2
golang.org/x/tools@v0.1.12 golang.org/x/net@v0.0.0-20220722155237-a158d28d115b
golang.org/x/tools@v0.0.0-20200619180055-7c47624df98f golang.org/x/net@v0.0.0-20200226121028-0de0cce0169b
golang.org/x/tools@v0.0.0-20190328211700-ab21143f2384 golang.org/x/net@v0.0.0-20190311183353-d8887717615a
golang.org/x/crypto@v0.0.0-20211215165025-cf75a172585e golang.org/x/net@v0.0.0-20210226172049-e18ecbb05110
golang.org/x/tools@v0.1.7 golang.org/x/net@v0.0.0-20210805182204-aaa1db679c0d
golang.org/x/crypto@v0.0.0-20191011191535-87dc89f01550 golang.org/x/net@v0.0.0-20190404232315-eb5bcb51f2a3
golang.org/x/tools@v0.0.0-20191119224855-298f0cb1881e golang.org/x/net@v0.0.0-20190620200207-3b0461eec859
golang.org/x/crypto@v0.0.0-20210921155107-089bfa567519 golang.org/x/net@v0.0.0-20210226172049-e18ecbb05110
sgotti commented 4 weeks ago

Take a look at the vuln report and how go vendors in the stdlib the golang.org/x/net http2 part. So you'll discover that a package using the net/http package uses the code provided by the stdlib and not the golang.org/x/net module code.

Anyway I'll update the dependencies in a future PR.

katexochen commented 4 weeks ago

Take a look at the vuln report and how go vendors in the stdlib the golang.org/x/net http2 part. So you'll discover that a package using the net/http package uses the code provided by the stdlib and not the golang.org/x/net module code.

Anyway I'll update the dependencies in a future PR.

I can't find x/net/http2 vendored in my 1.22.2 stdlib. There is x/net/http2/hpack, but that's not the package in question.

https://github.com/golang/go/blob/dddf0ae40fa0c1223aba191d73a44425a08e1035/src/vendor/modules.txt

sgotti commented 4 weeks ago

h2_bundle.go is a bundle file generated from x/net source files: https://github.com/golang/go/blob/a3eb55cea95b5894e97f311be981cda0e87bd465/src/net/http/http.go#L5

This is the backport of the fix for go 1.22: https://github.com/golang/go/commit/e55d7cf8435ba4e58d4a5694e63b391821d4ee9b

As you can see h2_bundle.go is updated.

This is the fix in x/net/http2: https://github.com/golang/net/commit/ba872109ef2dc8f1da778651bd1fd3792d0e4587

As you can see this is the same code but a different files that is used to generate the h2_bundle.go.

I created a PR #512 to update all dependencies, BUT it's important to know that the built binaries won't be vulnerable only when compiled with an updated go version.

katexochen commented 4 weeks ago

h2_bundle.go is a bundle file generated from x/net source files: https://github.com/golang/go/blob/a3eb55cea95b5894e97f311be981cda0e87bd465/src/net/http/http.go#L5

Thanks for the explanation! Just to understand this right, how do you know that none of your transitive dependencies is doing manual configuration of http2 transport, which would lead to x/net/http2 being used instead of the bundled http2 support?

https://pkg.go.dev/net/http#hdr-HTTP_2:

Manually configuring HTTP/2 via the golang.org/x/net/http2 package takes precedence over the net/http package's built-in HTTP/2 support.

I created a PR #512 to update all dependencies, BUT it's important to know that the built binaries won't be vulnerable only when compiled with an updated go version.

So would you say this is a false positive of govulncheck, which should be aware of the toolchain version?

sgotti commented 4 weeks ago

Thanks for the explanation! Just to understand this right, how do you know that none of your transitive dependencies is doing manual configuration of http2 transport, which would lead to x/net/http2 being used instead of the bundled http2 support?

It's a vulnerability on the server side. And only agola packages will start a listening http server and use its own handlers.

So would you say this is a false positive of govulncheck, which should be aware of the toolchain version?

It's not a false positive since the issue is in that package, but the way the http2 code is bundled in the go stdlib means that it's not enough to update golang.org/x/net.

To be clearer: I'm not against updating golang.org/x/net dependency version, but this vulnerability is not fixed by this action.