Add FIPS TLS encrypted private key tests#281
Conversation
Add a test to ensure that when if FIPS mode attempting to decrypt an encrypted private key will result in errors.ErrUnsuported. Change the ReadPEM method to return a joined error so that an encrypted block will return an error instead of just having the "no PEM blocks" error.
d660946 to
952398d
Compare
| //go:build !requirefips | ||
|
|
There was a problem hiding this comment.
The only thing I didn't understand is why most of the tests won't run when FIPS is enabled.
Do they break with FIPS enabled?
There was a problem hiding this comment.
Some of them break, but this is mostly so we can limit the requirefips tests to only test the different behaviour
There was a problem hiding this comment.
@michel-laterman I don't think that we should generally exclude non-system tests from running when in fips mode. What we had discussed was to exclude the expensive system tests that might test certain specifics that aren't focused on FIPS relevant parts; instead we want to have some focused system tests for FIPS covering TLS, crypto, pgp,..
Would be worth looking into why these tests are failing rather than disabling them.
There was a problem hiding this comment.
I agree with @simitt, because FIPS changes some cryptographic libraries it is better to have the full suit of tests dealing with crypto running and only add build tags to the ones with different behaviour.
Also, IIRC the elastic-agent-libs CI is rather fast, so I don't see any issues with running the tests twice, one for each build.
simitt
left a comment
There was a problem hiding this comment.
@michel-laterman and I discussed to replace hardcoded test certs with generated ones as a follow up #282 (out of scope for currently planned work).
… for non-fips mode testing
simitt
left a comment
There was a problem hiding this comment.
@michel-laterman I created a PR against your branch to get rid of the hardcoded certificates michel-laterman#1. Please review and let me know if you are ok with merging this into your branch before moving on with this PR.
(fips): create test TLS certs and run max number of tests in fips mode
💚 Build Succeeded
History
|
belimawr
left a comment
There was a problem hiding this comment.
The code looks good, but there is a TODO that was left behind. It would be better to remove it before merging.
| block, err = x509.EncryptPEMBlock(rand.Reader, "RSA PRIVATE KEY", x509.MarshalPKCS1PrivateKey(key), []byte(password), x509.PEMCipherAES256) //nolint:staticcheck // we need to support encrypted private keys | ||
| require.NoError(t, err) | ||
| case blockTypePKCS8Encrypted: | ||
| //TODO: this uses an elastic implementation of pkcs8 as the stdlib does not support password protected pkcs8 |
There was a problem hiding this comment.
Why a TODO comment? Shouldn't it be just the comment explaining why we use our custom implementation?
| //TODO: this uses an elastic implementation of pkcs8 as the stdlib does not support password protected pkcs8 | |
| // This uses an elastic implementation of pkcs8 as the stdlib does not support password protected pkcs8 |
What does this PR do?
Add a test to ensure that when if FIPS mode attempting to decrypt an encrypted private key will result in errors.ErrUnsuported. Change the ReadPEM method to return a joined error so that an encrypted block will return an error instead of just having the "no PEM blocks" error.
Why is it important?
FIPS functionality needs tests.
Checklist
Example test run
Related issues