-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[fips140][configurablehttpprovider.TestFunctionalityDownloadFileHTTPS] Skip test if GODEBUG=fips140=only is set
#14076
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #14076 +/- ##
==========================================
- Coverage 92.24% 92.24% -0.01%
==========================================
Files 657 658 +1
Lines 41068 41072 +4
==========================================
+ Hits 37884 37885 +1
- Misses 2179 2182 +3
Partials 1005 1005 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // NOTE: This only checks env var; at the time of writing fips140 can only be set via env | ||
| // other GODEBUG settings can be set via embedded comments or in go.mod, we may need to account for this in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own education: is the GOFIPS140 env variable only used for deciding the version but not for enabling/disabling then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the GOFIPS140 environment variable is used to select the version of the Go Cryptographic Module to use but, as of now, none of these versions are validated as being FIPS-compliant (mentioned further down in the same doc). So, IMO, there is no benefit to setting this variable, again, as of now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move the test util to confmap/provider/internal? That way we avoid creating more public API. Another alternative is to create a module for testing utils
configurablehttpprovider.TestFunctionalityDownloadFileHTTPS] Skip test if GODEBUG=fips140=only is setconfigurablehttpprovider.TestFunctionalityDownloadFileHTTPS] Skip test if GODEBUG=fips140=only is set
configurablehttpprovider.TestFunctionalityDownloadFileHTTPS] Skip test if GODEBUG=fips140=only is setconfigurablehttpprovider.TestFunctionalityDownloadFileHTTPS] Skip test if GODEBUG=fips140=only is set
959fd1b to
87349a7
Compare
Thanks for the suggestion, @mx-psi. I decided to make a new module for testing utilities; see 87349a7. However, now CI is failing on this PR like so: I'm guessing I need to make a separate PR first that introduces the |
|
This shouldn't be a public module. If we expect there will be more tests skipped on FIPS, it makes sense to have this as a module global to the collector. But it should be under |
|
It is. I think to fix the linter, you need to run |
Thanks. I ran Do you think I need to publish the |
|
You need a replace statement so the testutil package properly points to the local path, not to a released package. diff --git a/confmap/go.mod b/confmap/go.mod
index 35d3f3f81..426bbf6ba 100644
--- a/confmap/go.mod
+++ b/confmap/go.mod
@@ -10,6 +10,7 @@ require (
github.com/knadh/koanf/v2 v2.3.0
github.com/stretchr/testify v1.11.1
go.opentelemetry.io/collector/featuregate v1.44.0
+ go.opentelemetry.io/collector/internal/testutil v0.0.0-00010101000000-000000000000
go.uber.org/goleak v1.3.0
go.uber.org/multierr v1.11.0
go.uber.org/zap v1.27.0
@@ -30,4 +31,6 @@ retract (
v0.69.0 // Release failed, use v0.69.1
)
+replace go.opentelemetry.io/collector/internal/testutil => ../internal/testutil
+ |
e6d6ad6 to
f133008
Compare
f60c7d6 to
cc93336
Compare
Description
When #13926 is merged, all unit tests in this repository will be run with
GODEBUG=fips140=onlyto help surface any FIPS-140 violations in all the Go modules in this repository.One such unit test that fails in these circumstances is
confmap/provider/internal/configurablehttpprovider.TestFunctionalityDownloadFileHTTPS. However, the FIPS violation surfaced by this test is from the test code itself (as opposed to from OpenTelemetry Collector core code that the test is exercising), specifically when this call is made:opentelemetry-collector/confmap/provider/internal/configurablehttpprovider/provider_test.go
Line 81 in 0f3b0c9
As such, this is not a FIPS violation we need to worry about when running the OpenTelemetry Collector. To prevent the unit test from failing when it's run with
GODEBUG=fips140=only, we skip it.Link to tracking issue
Fixes #13998
Testing
Run the
configurablehttpprovider.TestFunctionalityDownloadFileHTTPSunit test withGODEBUG=fips140=only.