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

Add FIPS switch #19179

Merged
merged 45 commits into from
Dec 26, 2024
Merged

Add FIPS switch #19179

merged 45 commits into from
Dec 26, 2024

Conversation

dkirov-dd
Copy link
Contributor

@dkirov-dd dkirov-dd commented Dec 3, 2024

What does this PR do?

This PR adds the following functionality to the AgentCheck base class:

  • When the GOFIPS environment variable is set to 1, the OPENSSL_CONF and OPENSSL_MODULES environment variables are modified in order to point all OpenSSL binaries to the desired configuration and modules.
  • This will enable FIPS by default for all calls made with the OpenSSL binaries, as documented here.

Motivation

We want to ensure that all integrations can function in a FIPS compliant mode if it is necessary.

Implementation decisions

The GOFIPS environment variable was set as the FIPS toggle indicator by the ASC team.
Environment variables were preferred to C function calls for two main reasons for simplicity and reliability. The limitation of environment variables is that they have no effect on already loaded OpenSSL configurations. However, since there is no requirement for a toggle to FIPS mode during runtime, this is not an issue as we can load the correct configuration on startup.
The reason why the implementation should live in the base check is to be as close to the start of the Python process as possible, to prevent any OpenSSL configuration loading before the env vars are set.

Testing

Two types of automated tests were attempted:

  • E2E tests using the TLS check: we are checking if the TLS check can communicate with a custom server that restricts its accepted ciphers to a single one. When that cipher is not FIPS approved and we are running in FIPS mode, communication should fail. It should succeed in all other cases.
  • (WIP - Will be added in follow-up PR) Integration tests: testing our enable_fips outside of the Agent container environment. These tests are verify that our FIPS switch has an effect on cryptographic libraries like ssl and cryptography. They are run in the test-fips workflow which is very much experimental.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
  • If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

@dkirov-dd dkirov-dd force-pushed the david.kirov/fips-switch branch 5 times, most recently from 83f3a46 to 7811081 Compare December 3, 2024 12:55
@dkirov-dd dkirov-dd force-pushed the david.kirov/fips-switch branch 11 times, most recently from 4b47e1d to f7bc5be Compare December 3, 2024 14:30
@dkirov-dd dkirov-dd force-pushed the david.kirov/fips-switch branch 6 times, most recently from 7f98a36 to 3b6c7e2 Compare December 3, 2024 15:35
@dkirov-dd dkirov-dd force-pushed the david.kirov/fips-switch branch 3 times, most recently from d140e29 to c25c9f7 Compare December 20, 2024 15:34
@dkirov-dd dkirov-dd force-pushed the david.kirov/fips-switch branch from c25c9f7 to 77508be Compare December 20, 2024 15:43
@dkirov-dd dkirov-dd force-pushed the david.kirov/fips-switch branch 3 times, most recently from 44cd2bc to ef6c3e0 Compare December 20, 2024 21:42
@dkirov-dd dkirov-dd force-pushed the david.kirov/fips-switch branch from ef6c3e0 to fda181f Compare December 20, 2024 21:46
@dkirov-dd dkirov-dd force-pushed the david.kirov/fips-switch branch from 837237f to ea88fce Compare December 23, 2024 09:39
@dkirov-dd dkirov-dd force-pushed the david.kirov/fips-switch branch from 402ce03 to 610bcb7 Compare December 23, 2024 11:01
@dkirov-dd dkirov-dd requested a review from steveny91 December 23, 2024 11:28
@dkirov-dd dkirov-dd added this pull request to the merge queue Dec 26, 2024
Merged via the queue into master with commit c0d1c42 Dec 26, 2024
393 of 396 checks passed
@dkirov-dd dkirov-dd deleted the david.kirov/fips-switch branch December 26, 2024 15:18
github-actions bot pushed a commit that referenced this pull request Dec 26, 2024
* Add FIPS workflow file

* Add Windows steps

* Experiment with download from S3 for Windows

* Revert to building openssl

* Switch Windows steps to download from S3

* Remove unnecessary steps

* Add FIPS_MODULE_VERSION for Linux

* Finish handling Python in setup

* Remove unnecessary steps

* Add md5 tests

* Make md5 tests pass

* Try separating FIPS and non-FIPS md5 tests

* Add e2e tests for TLS FIPS

* Make TLS E2E tests pass

* Switch from env vars to C bindings

* Revert to using env vars

* Add option for e2e env vars in workflow

* Remove unnecessary comments from start-server.sh

* Rework enable_fips for user env var overwrite

* Disable FIPS tests by default in master

* Add changelogs

* Fix license headers

* Remove unfinished tests

* Remove openssl.cnf workaround

* Remove unused compose file

* Fix license headers

* Bring back integration tests

* Experiment with integration tests

* Remove integration test files

* Restore pr.yml and test-target.yml

* Move FIPS workflow to test-fips.yml

* Fix pytest "not fips" args

* Update test-fips.yml

* Fix unvalid workflow

* Modify JOB_NAME env var

* Re-introduce experimental integration tests

* Merge e2e tests and clean test-fips workflow

* Merge integration tests and use monkeypatch in setup fixture

* Attemp to fix experimental workflow

* Replace ddev with pytest in experimental workflow

* Revert "Replace ddev with pytest in experimental workflow"

This reverts commit fda181f.

* Remove experimental tests from PR

* Add unit tests for env var logic

* Switch to using marks to exclude fips from test-target

* Revert "Switch to using marks to exclude fips from test-target"

This reverts commit 3e3e51a. c0d1c42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants