Bump Go version 1.22.2 -> 1.22.3

jraddaoui commented 5 months ago

@djjuhasz I was just chatting with Sevein yesterday and he suggested that we relax the go.mod version in this project to adhere to the Go release support cycle. Similar to what he did on the gotools package (https://github.com/artefactual-labs/gotools/pull/18).

djjuhasz commented 5 months ago

@djjuhasz I was just chatting with Sevein yesterday and he suggested that we relax the go.mod version in this project to adhere to the Go release support cycle. Similar to what he did on the gotools package (artefactual-labs/gotools#18).

I don't understand what you mean by adhering to the Go release support cycle? It looks to me like https://github.com/artefactual-labs/gotools/pull/18 is out of date because Go 1.20 is no longer under support (Go 1.22 has been released).

jraddaoui commented 5 months ago

Each major Go release is supported until there are two newer major releases. For example, Go 1.5 was supported until the Go 1.7 release, and Go 1.6 was supported until the Go 1.8 release. We fix critical problems, including critical security problems, in supported releases as needed by issuing minor revisions (for example, Go 1.6.1, Go 1.6.2, and so on).

I think you are right now that I read that, @sevein ^^

In any case, should we use 1.21 here?

djjuhasz commented 5 months ago

@jraddaoui I changed go.mod to use:

go 1.21

Then I ran go mod tidy and got:

go 1.21.4

toolchain go1.22.1

And then govulncheck returns warnings:

❯ govulncheck ./...
Scanning your code and 406 packages across 47 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
  Standard library
    Found in: net/http@go1.22.1
    Fixed in: net/http@go1.22.2

I'm not really sure what the downgrade is supposed to get us?

sevein commented 5 months ago

I have a limited understanding of what's happening with Go here, but it seems like there are few conflated issues:

The issue that govulncheck reported for you is likely due to the version of go (go1.22.1) that you were using locally when generated the report.
The go directive used to be advisory, but it became a mandatory requirement starting with go1.21. I'm going to explore this aspect further below because I feel it's also something I need to understand better.
I read the release policy yesterday and mistakenly interpreted that go1.20 is still maintained, misguided by the fact that a lot of public modules are still using go1.20 as the minimum version required.

As a publisher of a few modules that has the potential to be widely used (wishful thinking), it's reasonable to set go1.20 as the minimum required version recognizing that software teams may wish to continue using the latest versions of my module while they are still due for an upgrade to go1.21 for various reasons, e.g. it may be unreasonable to expect teams to upgrade to go1.21.x immediately following the release of go1.22.0.

It looks like that's a common practice among widely-adopted modules, e.g. github.com/gorilla/mux or github.com/prometheus/client_golang only require go1.20, although it's not always the case, e.g. go.opentelemetry.io/otel is already on go1.21. But github.com/spf13/cobra, which is actively maintained, chooses to set go1.15 as the minimum verison! For example, see what aws-sdk-go-v2 reads in their README:

The v2 SDK follows the upstream release policy with an additional six months of support for the most recently deprecated language version. AWS reserves the right to drop support for unsupported Go versions earlier to address critical security issues.

It may be good to adopt this same policy across all projects listed under https://go.artefactual.dev/, but obviously temporal-activities doesn't have to do that since it has a limited scope.

On the other hand, I think that go mod tidy wanted you to use go1.21.4 because you have a dependency that requires go@1.21.1:

$ git reset --hard eae3871dc67b8ebf4c1c63ea941ef5b2f81baeab # Before this PR14 is merged.
HEAD is now at eae3871 Adjust codecov test coverage targets

$ go mod tidy -go=1.20
go: go.artefactual.dev/tools@v0.8.0 requires go@1.21.1, but 1.20 is requested

I can't tell why it chooses go1.21.4 specifically but in any case, I think the culprit is in an old version of go.artefactual.dev/tools, which is my fault because I had the directive set to go 1.21.1, but I've relaxed that constraint in recent releases, go.artefactual.dev/tools@v0.9.0 started using the directive go 1.21 and go.artefactual.dev/tools@v0.12.0 uses go 1.20. If you get the updated module:

$ go get -u go.artefactual.dev/tools
go: upgraded github.com/davecgh/go-spew v1.1.1 => v1.1.2-0.20180830191138-d8f796af33cc
go: upgraded github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 => v1.4.0
go: upgraded github.com/pmezard/go-difflib v1.0.0 => v1.0.1-0.20181226105442-5d4384ee4fb2
go: upgraded go.artefactual.dev/tools v0.8.0 => v0.12.0
go: upgraded golang.org/x/exp v0.0.0-20231127185646-65229373498e => v0.0.0-20231219180239-dc181d75b848
go: upgraded golang.org/x/time v0.3.0 => v0.5.0

$ go mod tidy -go=1.20
go: github.com/google/safeopen@v0.0.0-20240125081138-66b54d5181c6 requires go@1.21, but 1.20 is requested

Bummer! I guess at this point we realize that we'll have to require 1.21 unless you want to give up on using safeopen, so I run go mod tidy -go=1.21 and I finally get:

-go 1.22.2
+go 1.21
+
+toolchain go1.22.3

The toolchain directive is only informative afaik and you can omit it.

Overall, I think it's probably for the best to require go 1.21 as the minimum version on this module like Radda was suggesting. I think that in go.artefactual.dev/* modules I'll default to the more conservative approach like aws-sdk-go-v2.

Please let me know if this reasoning makes sense to you! For me the conclusion is that libraries, as opposed to applications, should use loose constraints to allow for a greater variety of scenarios, e.g. an application developer that can't afford bumping the version of Go for other reasons (limited human resources, unsolved runtime bugs, performance regressions, internal policies, regressions, etc), who knows maybe they were using Go in an embedded appliance without network access and at that point they won't care about a bug in net/http.

jraddaoui commented 5 months ago

Makes a lot of sense to me, thanks for the research and update @sevein!

djjuhasz commented 5 months ago

@sevein @jraddaoui so it sounds to me like the unstated goal here is to make the go version required to run this (and other) modules as low as possible so it can be imported into other projects that are running on older toolchains. Is that accurate?

@sevein I followed your lead:

❯ go get -u go.artefactual.dev/tools
go: downloading go.artefactual.dev/tools v0.12.0
go: upgraded go 1.20 => 1.21.1
go: added toolchain go1.22.1
go: upgraded github.com/davecgh/go-spew v1.1.1 => v1.1.2-0.20180830191138-d8f796af33cc
go: upgraded github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 => v1.4.0
go: upgraded github.com/pmezard/go-difflib v1.0.0 => v1.0.1-0.20181226105442-5d4384ee4fb2
go: upgraded go.artefactual.dev/tools v0.8.0 => v0.12.0
go: upgraded golang.org/x/exp v0.0.0-20231127185646-65229373498e => v0.0.0-20231219180239-dc181d75b848
go: upgraded golang.org/x/time v0.3.0 => v0.5.0
❯ go mod tidy -go=1.21

But when I run govulncheck with the 1.21.0 toolchain I get security warnings:

❯ GOTOOLCHAIN=go1.21.0 govulncheck ./...
Scanning your code and 404 packages across 46 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
  Standard library
    Found in: net/http@go1.21
    Fixed in: net/http@go1.21.9
    Example traces found:
      #1: removefiles/activity.go:12:2: removefiles.init calls temporal.init, which eventually calls http.CanonicalHeaderKey
      #2: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.Header.Del
      #3: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.Header.Get
      #4: archive/extract_activity.go:162:19: archive.writeFileHandler calls io.Copy, which eventually calls http.bodyEOFSignal.Read
      #5: archive/extract_activity.go:162:19: archive.writeFileHandler calls io.Copy, which eventually calls http.bodyLocked.Read
      #6: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2ConnectionError.Error
      #7: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2ErrCode.String
      #8: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2FrameHeader.String
      #9: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2FrameType.String
      #10: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2FrameWriteRequest.String
      #11: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2Setting.String
      #12: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2SettingID.String
      #13: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2StreamError.Error
      #14: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2chunkWriter.Write
      #15: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2connError.Error
      #16: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2duplicatePseudoHeaderError.Error
      #17: archive/extract_activity.go:162:19: archive.writeFileHandler calls io.Copy, which eventually calls http.http2gzipReader.Read
      #18: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2headerFieldNameError.Error
      #19: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2headerFieldValueError.Error
      #20: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2pseudoHeaderError.Error
      #21: archive/extract_activity.go:162:19: archive.writeFileHandler calls io.Copy, which eventually calls http.http2requestBody.Read
      #22: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2responseWriter.Write
      #23: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2responseWriter.WriteString
      #24: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2stickyErrWriter.Write
      #25: archive/extract_activity.go:162:19: archive.writeFileHandler calls io.Copy, which eventually calls http.http2transportResponseBody.Read
      #26: removefiles/activity.go:63:25: removefiles.Activity.Execute calls fmt.Errorf, which eventually calls http.http2writeData.String
      #27: archive/extract_activity.go:162:19: archive.writeFileHandler calls io.Copy, which eventually calls http.maxBytesReader.Read
      #28: archive/extract_activity.go:116:23: archive.ExtractActivity.extract calls archiver.Rar.Extract, which eventually calls http.response.Write

Vulnerability #2: GO-2024-2599
    Memory exhaustion in multipart form parsing in net/textproto and net/http
  More info: https://pkg.go.dev/vuln/GO-2024-2599
  Standard library
    Found in: net/textproto@go1.21
    Fixed in: net/textproto@go1.21.8
    Example traces found:
      #1: archive/extract_activity.go:162:19: archive.writeFileHandler calls io.Copy, which eventually calls textproto.Reader.ReadMIMEHeader

Vulnerability #3: GO-2024-2598
    Verify panics on certificates with an unknown public key algorithm in
    crypto/x509
  More info: https://pkg.go.dev/vuln/GO-2024-2598
  Standard library
    Found in: crypto/x509@go1.21
    Fixed in: crypto/x509@go1.21.8
    Example traces found:
      #1: archive/extract_activity.go:162:19: archive.writeFileHandler calls io.Copy, which eventually calls x509.Certificate.Verify

Vulnerability #4: GO-2023-2186
    Incorrect detection of reserved device names on Windows in path/filepath
  More info: https://pkg.go.dev/vuln/GO-2023-2186
  Standard library
    Found in: path/filepath@go1.21
    Fixed in: path/filepath@go1.21.4
    Example traces found:
      #1: archive/extract_activity.go:116:23: archive.ExtractActivity.extract calls archiver.Tar.Extract, which eventually calls filepath.IsLocal

Vulnerability #5: GO-2023-2185
    Insecure parsing of Windows paths with a \??\ prefix in path/filepath
  More info: https://pkg.go.dev/vuln/GO-2023-2185
  Standard library
    Found in: path/filepath@go1.21
    Fixed in: path/filepath@go1.21.4
    Platforms: windows
    Example traces found:
      #1: archive/extract_activity.go:146:36: archive.writeFileHandler calls safeopen.CreateBeneath, which eventually calls filepath.Clean
      #2: archive/extract_activity.go:146:36: archive.writeFileHandler calls safeopen.CreateBeneath, which eventually calls filepath.Clean
      #3: archive/extract_activity.go:176:22: archive.extractPath calls filepath.Dir
      #4: archive/extract_activity.go:176:22: archive.extractPath calls filepath.Dir
      #5: archive/extract_activity.go:116:23: archive.ExtractActivity.extract calls archiver.Tar.Extract, which eventually calls filepath.IsLocal
      #6: archive/extract_activity.go:116:23: archive.ExtractActivity.extract calls archiver.Tar.Extract, which eventually calls filepath.IsLocal
      #7: archive/extract_activity.go:200:22: archive.skipTopLevelDir calls filepath.Join
      #8: archive/extract_activity.go:200:22: archive.skipTopLevelDir calls filepath.Join
      #9: removefiles/activity.go:88:24: removefiles.removeByNames calls filepath.WalkDir
      #10: removefiles/activity.go:88:24: removefiles.removeByNames calls filepath.WalkDir

Your code is affected by 5 vulnerabilities from the Go standard library.
This scan also found 0 vulnerabilities in packages you import and 9
vulnerabilities in modules you require, but your code doesn't appear to call
these vulnerabilities.
Use '-show verbose' for more details.

So it seems to me we should require at least go1.21.9 to avoid the security issues in the standard library.

djjuhasz commented 5 months ago

❯ GOTOOLCHAIN=go1.21.9 govulncheck ./...
Scanning your code and 404 packages across 46 dependent modules for known vulnerabilities...

No vulnerabilities found.

sevein commented 5 months ago

@sevein @jraddaoui so it sounds to me like the unstated goal here is to make the go version required to run this (and other) modules as low as possible so it can be imported into other projects that are running on older toolchains. Is that accurate?

Yes, well put!

artefactual-sdps / temporal-activities

Bump Go version 1.22.2 -> 1.22.3 #14