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

Dependency fixes and updates to development process #137

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gaby
Copy link
Collaborator

@gaby gaby commented Jul 16, 2024

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

@gaby gaby requested a review from mholt July 16, 2024 03:19
@mholt
Copy link
Member

mholt commented Jul 18, 2024

I'm not a fan of Makefiles in Go projects 😅 but as long as it's optional and just your preference I guess that's OK 🤷‍♂️ 👍

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

@gaby
Copy link
Collaborator Author

gaby commented Jul 19, 2024

@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
Copy link
Member

mholt commented Jul 19, 2024

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
Copy link
Member

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
Copy link
Member

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.

README.md Show resolved Hide resolved
@gaby
Copy link
Collaborator Author

gaby commented Jul 19, 2024

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
Copy link
Collaborator Author

gaby commented Jul 21, 2024

@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
Copy link
Member

mholt commented Jul 22, 2024

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
Copy link
Member

francislavoie commented Jul 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants