Skip to content

[FIPS] Ensure that Agent cannot present a client-side TLS certificate created with a RSA keypair of less than 2048 bits length#7912

Merged
ycombinator merged 30 commits into
elastic:mainfrom
ycombinator:fips-remote-client-tests-insecure-key
Apr 29, 2025
Merged

[FIPS] Ensure that Agent cannot present a client-side TLS certificate created with a RSA keypair of less than 2048 bits length#7912
ycombinator merged 30 commits into
elastic:mainfrom
ycombinator:fips-remote-client-tests-insecure-key

Conversation

@ycombinator
Copy link
Copy Markdown
Contributor

@ycombinator ycombinator commented Apr 18, 2025

What does this PR do?

This PR ensures that remote.Clients created with a configuration containing a client-side TLS certificate generated from an RSA keypair < 2048 bits in length cannot be created in FIPS mode.

Why is it important?

FIPS-140 does not allow use of RSA with keylengths < 2048 bits.

Checklist

  • I have read and understood the pull request guidelines of this project.
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool
  • I have added an integration test or an E2E test

Disruptive User Impact

Users using FIPS-capable Elastic Agents with client-side TLS certificates that are generated with RSA, must generate the RSA keypairs with a minimum of 2048 bits key lengths.

@ycombinator ycombinator added enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team skip-changelog backport-8.x Automated backport to the 8.x branch with mergify labels Apr 18, 2025
@ycombinator ycombinator requested a review from a team as a code owner April 18, 2025 22:15
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

@ycombinator ycombinator marked this pull request as draft April 18, 2025 22:15
@ycombinator ycombinator added backport-8.19 Automated backport to the 8.19 branch and removed backport-8.x Automated backport to the 8.x branch with mergify labels Apr 22, 2025
Comment thread internal/pkg/remote/client_fips_test.go Outdated
@ycombinator ycombinator force-pushed the fips-remote-client-tests-insecure-key branch from 5714883 to 506b629 Compare April 22, 2025 22:54
@ycombinator ycombinator marked this pull request as ready for review April 22, 2025 23:17
@ycombinator
Copy link
Copy Markdown
Contributor Author

This PR is failing CI in the Unit tests - Ubuntu 22.04 with requirefips build tag step like so:

=== FAIL: internal/pkg/remote TestClientWithCertificate/insecure_key (0.02s)
--
  | client_fips_test.go:156:
  | Error Trace:	/opt/buildkite-agent/builds/bk-agent-prod-gcp-1745365321736020269/elastic/elastic-agent/internal/pkg/remote/client_fips_test.go:156
  | Error:      	Expected nil, but got: &http.Response{Status:"200 OK", StatusCode:200, Proto:"HTTP/1.1", ProtoMajor:1, ProtoMinor:1, Header:http.Header{"Content-Length":[]string{"19"}, "Content-Type":[]string{"text/plain; charset=utf-8"}, "Date":[]string{"Wed, 23 Apr 2025 00:01:55 GMT"}}, Body:(*http.bodyEOFSignal)(0xc00007c440), ContentLength:19, TransferEncoding:[]string(nil), Close:false, Uncompressed:false, Trailer:http.Header(nil), Request:(*http.Request)(0xc0002a9900), TLS:(*tls.ConnectionState)(0xc0002c8cc0)}
  | Test:       	TestClientWithCertificate/insecure_key
  |  
  | === FAIL: internal/pkg/remote TestClientWithCertificate (0.05s)

@michel-laterman Aren't these tests supposed to run on a machine with the OpenSSL FIPS provider configured and with GOEXPERIMENT=systemcrypto?

Copy link
Copy Markdown
Contributor

@michel-laterman michel-laterman left a comment

Choose a reason for hiding this comment

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

nitpick: rename agent_secure to fips_valid and agent_insecure to fips_invalid so the purpose of the keys are clearer

@michel-laterman
Copy link
Copy Markdown
Contributor

@ycombinator yes, if we are relying on FIPS enforcement we would either need to run this test with a FIPS provider and the microsoft/go options, or only when 1GODEBUG=fips140=only(or maybeon`?)

@ycombinator
Copy link
Copy Markdown
Contributor Author

ycombinator commented Apr 23, 2025

Okay, so it looks like the TestClientWithCertificate/insecure_key unit test case fails in four different ways depending on the Go distribution and whether GODEBUG=fips140=only is set or not. 🤦

Specifically, the error message returned from the HTTPS server in the test during TLS handshake is different in each of these four combinations:

Go distribution GODEBUG=fips140=only set? Error message from TLS handshake
microsoft Yes EOF
microsoft No tls: failed to sign handshake: EVP_PKEY_sign_init failed\nopenssl error(s):\nerror:1C800069:Provider routines::invalid key length\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\n\tproviders/common/securitycheck.c:65
upstream Yes tls: failed to sign handshake: crypto/rsa: use of keys smaller than 2048 bits is not allowed in FIPS 140-only mode
upstream No No error is returned.

In the face of these disparities, I'm not really sure how to write this unit test. Any thoughts? @michel-laterman @kruskall @simitt.

Ideally, I think our CI environment should run the microsoft/go toolchain, at least for the two steps where it's running FIPS tests, one with just go test --tags=requirefips ... and one with GODEBUG=fips140=only go test --tags=requirefips .... That would eliminate at least the last two rows in the table above and perhaps the test can then conditionally do assertions based on the value of crypto/fips140.Enabled. WDYT?

@michel-laterman
Copy link
Copy Markdown
Contributor

Can we test the behaviour of using microsoft/go with GOEXPERIMENT=systemcrypto go test -tags=requirefips ./...?
This may require a FIPS provider in order to run, but would produce the same results as the binaries we ship

@ycombinator
Copy link
Copy Markdown
Contributor Author

Can we test the behaviour of using microsoft/go with GOEXPERIMENT=systemcrypto go test -tags=requirefips ./...? This may require a FIPS provider in order to run, but would produce the same results as the binaries we ship

Sure. It means this PR will fail CI until we have a Buildkite image with microsoft/go and OpenSSL + FIPS provider, which I understand is itself currently waiting on microsoft/go#1654 to be merged.

With that, we'd eliminate the last two rows from the table in #7912 (comment), which is helpful. That leaves us with the difference in error from the TLS handshake due to the value of the GODEBUG=fips140= environment variable, which is shown in the first two rows in that table. For that, I will add a conditional to the test based on the value of crypto/fips140.Enabled.

@qmuntal
Copy link
Copy Markdown

qmuntal commented Apr 28, 2025

Okay, so it looks like the TestClientWithCertificate/insecure_key unit test case fails in four different ways depending on the Go distribution and whether GODEBUG=fips140=only is set or not. 🤦

Hi @ycombinator, Microsoft Go dev here. Can you fill an issue explaining this issue in https://github.com/microsoft/go? I'm sure we can improve this situation.

@ycombinator ycombinator force-pushed the fips-remote-client-tests-insecure-key branch from 68d7b5c to 3a9c2ae Compare April 29, 2025 14:04
@elastic-sonarqube
Copy link
Copy Markdown

Quality Gate failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

@ycombinator ycombinator enabled auto-merge (squash) April 29, 2025 19:53
@ycombinator ycombinator merged commit 0eb4f0f into elastic:main Apr 29, 2025
11 of 12 checks passed
mergify Bot pushed a commit that referenced this pull request Apr 29, 2025
… created with a RSA keypair of less than 2048 bits length (#7912)

* Adding test for unsupported TLS versions sent by client

* Add TODO for second test

* Add test case for multiple versions

* Removing unused code

* Add test for config hosts validation

* Remove hosts validation

* Refactoring test cases to take in any TLS configuration, not just versions

* Add test for RSA keypair with < 2048 key length

* Updating comment

* Add test case for using certificate with RSA keypair < 2048 bits

* Adding test data files

* Revert test scope

* Update test to fail on handshake

* Rename agent key and cert for clarity

* Adding test case with secure Agent certificate

* Adding README to testdata folder to explain manual generation of keys+certs

* Reverting unintended changes from conflict resolution

* Removing irrelevant integration test

* Remove unused variables

* Remove CA private key

* Renaming root -> CA for clarity

* Rename files and variable to make purpose clearer

* Adding .gitignore for CA private key file

* Introduce GoDebugFIPS140() function

* Separate test cases based on GODEBUG=fips140= value

* Rename test cases for clarity

* Remove test cases for GODEBUG=fips140=on

* Adjust test cases to assume upstream Go instead of Microsoft Go

* Be explicit in all constant types

* Fix data race

(cherry picked from commit 0eb4f0f)
@ycombinator ycombinator deleted the fips-remote-client-tests-insecure-key branch April 29, 2025 20:42
ycombinator added a commit that referenced this pull request Apr 29, 2025
… created with a RSA keypair of less than 2048 bits length (#7912) (#8040)

* Adding test for unsupported TLS versions sent by client

* Add TODO for second test

* Add test case for multiple versions

* Removing unused code

* Add test for config hosts validation

* Remove hosts validation

* Refactoring test cases to take in any TLS configuration, not just versions

* Add test for RSA keypair with < 2048 key length

* Updating comment

* Add test case for using certificate with RSA keypair < 2048 bits

* Adding test data files

* Revert test scope

* Update test to fail on handshake

* Rename agent key and cert for clarity

* Adding test case with secure Agent certificate

* Adding README to testdata folder to explain manual generation of keys+certs

* Reverting unintended changes from conflict resolution

* Removing irrelevant integration test

* Remove unused variables

* Remove CA private key

* Renaming root -> CA for clarity

* Rename files and variable to make purpose clearer

* Adding .gitignore for CA private key file

* Introduce GoDebugFIPS140() function

* Separate test cases based on GODEBUG=fips140= value

* Rename test cases for clarity

* Remove test cases for GODEBUG=fips140=on

* Adjust test cases to assume upstream Go instead of Microsoft Go

* Be explicit in all constant types

* Fix data race

(cherry picked from commit 0eb4f0f)

Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-8.19 Automated backport to the 8.19 branch enhancement New feature or request skip-changelog Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants