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

Make nobackend.go build constraint an inverse of the other backends (and add test) #953

Closed
dagood opened this issue Jun 17, 2023 · 1 comment · Fixed by #965
Closed

Make nobackend.go build constraint an inverse of the other backends (and add test) #953

dagood opened this issue Jun 17, 2023 · 1 comment · Fixed by #965

Comments

@dagood
Copy link
Member

dagood commented Jun 17, 2023

In #950 I wrote a test/generator that parses the build constraints for each backend and constructs a simple inverse. We could use that code to generate the constraint for nobackend.go so that it is always enabled when none of the backends' conditions are met.

This could allow "silent" unintended usage of Go crypto. GOEXPERIMENT=cngcrypto GOOS=linux go build would not end up using a backend. However, this is in line with some other behavior: GOEXPERIMENT=opensslcrypto GOOS=linux CGO_ENABLED=0 go build behaves similarly today.

It seems to me we should still make sure GOEXPERIMENT=systemcrypto never silently uses Go crypto, though. Based on the issues I saw in #950, this might mean GOEXPERIMENT=systemcrypto go tool dist test won't work (unless we make more test changes), but since systemcrypto is a fairly simple alias, this might be ok.

@dagood
Copy link
Member Author

dagood commented Jun 20, 2023

@qmuntal and I discussed this in Go sync:

  • We think that generating/testing the nobackend.go build constraint makes sense no matter what, simply to make sure the condition is always correct. There's no intentional mismatch.
  • In the past we've favored compatibility--seamlessly using Go crypto when the conditions aren't right to use system crypto. With the new crypto policy that we're trying to help people conform to, we think we should switch to more proactive errors, instead.

To fix the test suite, a few ideas:

  • Add an "opt out of proactive errors" tag. We could set it in CI (but then go dist test "in the wild" may not work) or set it in go dist test as part of the patch.
  • Individually fix test issues. For the errors I spotted in Rework systemcrypto to support tags; add better configuration errors #950 (comment), it might be enough to detect and remove crypto GOEXPERIMENTs when running those go build commands.
  • Now that I've thought about it: both. I think I like this the most.
    • Add an opt out, and enable it only in tests that have issues. (Patch each test individually. Only 2 are known.)
    • It's easier to add an opt-out tag than remove an experiment, adding it in individual tests means most tests are testing the proactive errors, and we might find some inherent value in knowing what scenarios interact poorly with the proactive errors.
    • In theory, if someone using our toolset needs the opt out for some reason (overwhelmingly simplifies their build process?), it will also be easy for them to enable. I don't think we should advertise that it exists, though, because it's certainly not the right thing to do in any normal case.

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 a pull request may close this issue.

1 participant