Dependency fixes and updates to development process

[gaby]

1. What does this change do, exactly?

• Fix issues with dependencies
• Use gotestsum for running tests.
• Run tests with shuffle enabled
• Add Makefile to ease development
• Bump min go version to 1.22
• This PR will close half of the current open PR's

[mholt]

I'm not a fan of Makefiles in Go projects :sweat_smile: but as long as it's optional and just your preference I guess that's OK :man_shrugging: :+1:

Any reason in particular to bump min version to Go 1.22 and to bump the Caddy version?

[gaby]

@mholt Removed the Makefile. Given this is a secury related project it made sense to the latest release. I can change it to 1.21 if needed.

Several of the dependencies needed to be updated because govulncheck was complaining about them.

[mholt]

Well, the versions in go.mod of libraries don't really matter (AFAIK), since, in the case of Caddy plugins, the versions used are determined by Caddy's go.mod. @francislavoie and @mohammed90 understand better than I do why needlessly upgrading versions in plugins' go.mod can be problematic, but I know we ran into some troubles in the recent past.

As far as govulncheck, we also have problems with lots of false positives in vulnerability scanners. Again, maybe Francis and Mohammed can elaborate as to what is happening but I think our new advice to plugin authors will soon be to only upgrade go.mod versions if really needed to compile.

[francislavoie]

Yeah all that will do is make it impossible to build the latest version of this plugin with anything lower than Caddy 2.8.4. If you're trying to build the latest version of Caddy with this plugin, then it will already use the correct versions. You can confirm by looking at caddy build-info after building.

There's no need to touch go.mod unless you actually need it due to an API change in some dependency.

One reason to do it would be to use a new httpcaddyfile.RegisterDirectiveOrder() function so that users don't need to set an explicit order in their config (via the order global option or with the route directive).

[mohammed90]

The rule of thumb is

Only care of your direct dependencies. Don't touch stuff that's not yours. Worry about yourself! (https://www.youtube.com/watch?v=3Jn4XEmSvC8)

In this case, you have caddy and x/net (zap is brought in by caddy). As Francis said, you don't need to upgrade Caddy unless you need new API. The go.mod file specifies the minimum version needed for your module to work. Bumping it up for no reason only brings pain.

As far as govulncheck, we also have problems with lots of false positives in vulnerability scanners.

The govulncheck tool isn't like others! It analyzes the execution paths and only warns of vulns that are actually affect the project. See this blog post by the Go team: https://go.dev/blog/vuln#vulnerability-detection-using-govulncheck.

[gaby]

I believe the one that was breaking the CI was x/net. I will update the PR this weekend to only update that dependency.

[gaby]

@mholt Caddy v2.7.x uses a vulnerable version of quic-go

Vulnerability #1: GO-2024-2682
    Denial of service via connection starvation in github.com/quic-go/quic-go
  More info: https://pkg.go.dev/vuln/GO-2024-2682
  Module: github.com/quic-go/quic-go
    Found in: github.com/quic-go/[email protected]
    Fixed in: github.com/quic-go/[email protected]

[mholt]

2.7 is an old version though. The go.mod in this repo doesn't dictate what version of quic-go gets used (unless it needs an old version, AFAIK). It's the go.mod in Caddy that dictates which version of quic-go is used, since Caddy is the main.

[francislavoie]

Like I said, do xcaddy build --with github.com/caddyserver/forwardproxy then ./caddy build-info and you'll see that it uses the quic-go version from latest Caddy, not the one from this plugin.

If we change the go.mod here to Caddy latest, then all it will do is prevent xcaddy build v2.7.6 --with github.com/caddyserver/forwardproxy from working, unless we have a good reason to bump it like using a new API only available in the latest version.

I still suggest adding httpcaddyfile.RegisterDirectiveOrder(), and then I'm okay with bumping deps in here.

[francislavoie]

If you want to rebase, I made some dep changed in my branch which I merged.

[gaby]

If you want to rebase, I made some dep changed in my branch which I merged.

Will do