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

build: Use go install for linter and add cache. #3162

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Jul 22, 2023

This is rebased on #3161.

This modifies the GitHub Action workflow to install golangci-lint from source with go install instead of using the separate install script and adds ~/.cache/golangci-lint to the saved cache for faster future runs.

The goal is to pin the dependency for the same reason the GitHub actions are pinned by hash. Namely, it reduces potential security risks such as compromised dependencies and dependency substitution attacks.

Using go install serves to pin the dependency because go verifies the downloaded module contents against the original checksum hashes they were first created with.

@davecgh davecgh added this to the 1.9.0 milestone Jul 22, 2023
@jholdstock
Copy link
Member

jholdstock commented Jul 22, 2023

The project doco advises against doing this:

Note: such go install/go get installation aren't guaranteed to work. We recommend using binary installation. go install/go get installation isn't recommended because of the following points:

  • some users use -u flag for go get, which upgrades our dependencies. Resulting configuration wasn't tested and isn't guaranteed to work.
  • go.mod replacement directive doesn't apply. It means a user will be using patched version of golangci-lint if we use such replacements.
  • it's stability depends on a user's Go version (e.g. on this compiler Go <= 1.12 bug).
  • we've encountered a lot of issues with Go modules hashes.
  • it allows installation from master branch which can't be considered stable.
  • it's slower than binary installation

@davecgh
Copy link
Member Author

davecgh commented Jul 22, 2023

I saw that, but I'm fairly confident that documentation is mostly outdated. While it used to be a valid assessment, it really isn't anymore. At least 4 of them definitely don't apply to us and go install was changed back around Go 1.18 to actually allow this new usage.

The one about being slower than binary installation is still mostly true, but that is mitigated by the fact we're now caching the go module cache and making use of it. With the cache, it only takes 4 seconds versus 2 seconds from the binary.

The only remaining case that potentially applies is the fact that go.mod replacements are not applied, but we only use release versions and replacements should never be in a release version. Moreover, that is exactly what we don't want to be able to happen as it's a common way that dependency substitution attacks arise.

EDIT: I should also note that we were, correctly, docked points on our open source security foundation scorecard because of this too. The recommended method is, in fact, at risk for such attacks.

If the golangci-lint team were to allow the install script to target a specific release by hash as opposed to a mutable tagged version (as the action does) then it would be safe, but they don't support that and the action doesn't support multi-module repos from a single invocation, so this is the only reasonable remaining option.

This modifies the GitHub Action workflow to install golangci-lint from
source with go install instead of using the separate install script and
adds ~/.cache/golangci-lint to the saved cache for faster future runs.

The goal is to pin the dependency for the same reason the GitHub actions
are pinned by hash.  Namely, it reduces potential security risks such as
compromised dependencies and dependency substitution attacks.

Using go install serves to pin the dependency because go verifies the
downloaded module contents against the original checksum hashes they
were first created with.
@davecgh davecgh force-pushed the build_install_linter_from_source branch from 7310d2a to 304568b Compare July 24, 2023 17:33
@davecgh davecgh merged commit 304568b into decred:master Jul 24, 2023
2 checks passed
@davecgh davecgh deleted the build_install_linter_from_source branch July 24, 2023 17:34
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