Skip to content

Conversation

@SarahFrench
Copy link
Member

Fixes this vulnerability, which doesn't necessarily impact users of Terraform but will almost certainly cause our release pipelines to block our upcoming release 🫠

Target Release

1.14.x

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@SarahFrench SarahFrench added no-changelog-needed Add this to your PR if the change does not require a changelog entry 1.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Nov 14, 2025
@SarahFrench SarahFrench marked this pull request as ready for review November 14, 2025 21:35
@SarahFrench SarahFrench requested review from a team as code owners November 14, 2025 21:35
@SarahFrench
Copy link
Member Author

SarahFrench commented Nov 17, 2025

==> Checking that code complies with static analysis requirements...
Error: internal/backend/backendrun/operationtype_string.go:23:5: no value of type uint is less than 0 (SA4003)
Error: internal/terraform/walkoperation_string.go:27:5: no value of type byte is less than 0 (SA4003)
make: *** [Makefile:28: staticcheck] Error 1
Error: Process completed with exit code 2.

These code consistency errors are related to use of go generate with a stringer tool:

//go:generate go tool golang.org/x/tools/cmd/stringer -type=OperationType operation_type.go

I'm unsure why these decided to appear on this PR, but here we are

Edit: the static analysis passes on ✨ my machine

@hashicorp hashicorp deleted a comment from github-actions bot Nov 17, 2025
@SarahFrench
Copy link
Member Author

==> Checking that code complies with static analysis requirements...
Error: internal/backend/backendrun/operationtype_string.go:23:5: no value of type uint is less than 0 (SA4003)
Error: internal/terraform/walkoperation_string.go:27:5: no value of type byte is less than 0 (SA4003)
make: *** [Makefile:28: staticcheck] Error 1
Error: Process completed with exit code 2.

These code consistency errors are related to use of go generate with a stringer tool:

//go:generate go tool golang.org/x/tools/cmd/stringer -type=OperationType operation_type.go

I'm unsure why these decided to appear on this PR, but here we are

Edit: the static analysis passes on ✨ my machine

I think the solution is to stop this is:

-type OperationType uint
+type OperationType int

as that'll result in different generated code that doesn't trigger this static check. The other types that we use go generate stringer in the codebase are either int, rune, or byte. That's why this static analysis issue only affects the two types listed.

Alternatively, we can disable the SA4003 check via staticcheck.conf files in the sub directories holding those two uint types. That feels like a better short-term solution to help get this PR merged faster and unblock the release.

…s/cmd/stringer` is used with custom types that have unsigned integers as underlying types
@hashicorp hashicorp deleted a comment from github-actions bot Nov 17, 2025
@hashicorp hashicorp deleted a comment from github-actions bot Nov 17, 2025
@github-actions
Copy link
Contributor

Backported dependency change

This PR makes changes to dependencies in go.mod file(s) and is labelled for backport.

Notice to the maintainer: Before merging the backport of this PR please follow our security scanning processes.

…s/cmd/stringer` is used with custom types that have byte as the underlying type

byte values also cannot take negative values
@SarahFrench
Copy link
Member Author

SarahFrench commented Nov 17, 2025

Now I've suppressed the linting errors the code consistency check is now progressing further and telling me:

ERROR: Generated files are inconsistent. Run 'make generate' and 'make protobuf' locally and then commit the updated files.

And this makes the linter errors above make more sense; the new generated code has if i < 0 checks on unsigned types.

@dbanck
Copy link
Member

dbanck commented Nov 17, 2025

Oh, what an adventure 😅 Thanks for looking into this

Looks like the x/tools bump to 0.38.0 includes this change golang/tools@82041a1 which updates the stringer logic

What do you think about adding -SA4003 to our global staticcheck config, so it's easier to find and revisit?

Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment on lines -75 to +82
golang.org/x/crypto v0.42.0
golang.org/x/mod v0.27.0
golang.org/x/net v0.44.0
golang.org/x/crypto v0.44.0
golang.org/x/mod v0.29.0
golang.org/x/net v0.46.0
golang.org/x/oauth2 v0.27.0
golang.org/x/sys v0.36.0
golang.org/x/term v0.35.0
golang.org/x/text v0.29.0
golang.org/x/tools v0.36.0
golang.org/x/sys v0.38.0
golang.org/x/term v0.37.0
golang.org/x/text v0.31.0
golang.org/x/tools v0.38.0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of these upgrades, the majority of the minor versions are just the dependency updating their internal dependencies to latest versions.

The more relevant changes are:

@SarahFrench
Copy link
Member Author

SarahFrench commented Nov 17, 2025

Testing the s3 backend:

go test github.com/hashicorp/terraform/internal/backend/remote-state/s3 
ok      github.com/hashicorp/terraform/internal/backend/remote-state/s3 315.086s

Testing the gcs backend:

go test github.com/hashicorp/terraform/internal/backend/remote-state/gcs
ok      github.com/hashicorp/terraform/internal/backend/remote-state/gcs        91.383s

I'm currently provisioning the resources needed to test the azure backend...

@SarahFrench SarahFrench changed the title Bump golang.org/x/crypto dependency chore: Bump golang.org/x/crypto dependency, ignore SA4003 linting errors globally Nov 17, 2025
# We are skipping:
# -ST*: Style-related checks, since terraform intentionally breaks some of these.
# -SA1019: Function deprecation checks because our policy is to update deprecated calls locally while making other nearby changes, rather than to make cross-cutting changes to update them all.
# -SA4003: Comparing unsigned values against negative values checks; we generate code using golang.org/x/tools/cmd/stringer that introduces this issues when used with custom types that have unsigned underlying types.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# -SA4003: Comparing unsigned values against negative values checks; we generate code using golang.org/x/tools/cmd/stringer that introduces this issues when used with custom types that have unsigned underlying types.
# -SA4003: Comparing unsigned values against negative values checks; we generate code using golang.org/x/tools/cmd/stringer that introduces these issues when used with custom types that have unsigned underlying types.

@SarahFrench
Copy link
Member Author

Unable to test the azure backend due to problems provisioning subscriptions, but we're assuming that this change is just as benign as it was in the other two main backends.

@SarahFrench SarahFrench merged commit 1dd8e60 into main Nov 17, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants