feat(fips): Use nop keystore implementation #279
Conversation
Keystore.Factory and other constuctor functions will return a NopKeystore if the requirefips build tag is set. The NopKeystore will will return ErrNotWritable for any retrievals, and ErrKeystoreUnsupported for create/save/store operations. The NopKeystore uses the same existing constructor name as the regular file keystore so it can be used in-place by callers.
michel-laterman
added
enhancement
michel-laterman
requested review from
rdner and
mauri870
and removed request for
a team
| @@ -37,8 +39,24 @@ var ( | |||
|
|
|||
| // ErrNotWritable is returned when the keystore is not writable | |||
| ErrNotListing = errors.New("the configured keystore is not listing") | |||
|
|
|||
| // ErrKeystoreDisabled is returned on create, save, and store operations if built with the requirefips flag | |||
| ErrKeystoreDisabled = fmt.Errorf("keystore disabled in FIPS mode: %w", errors.ErrUnsupported) | |||
There was a problem hiding this comment.
We could also use ErrNotWritable in place of this error
jlind23
requested review from
a team,
michalpristas,
pkoutsovasilis and
pchila
and removed request for
a team,
rdner,
mauri870 and
michalpristas
|
Tests will only succeed once #281 is merged |
There was a problem hiding this comment.
@michel-laterman please tell me if I am missing something, but as far as I can tell, the current keystore implementation should be FIPS compliant:
- pbkdf2 with SHA512 is approved
- AES with GCM is approved
I can see all the crypto related calls that we use in the code here.
With all that said, is the keystore implementation not FIPS compliant? 🤔
There was a problem hiding this comment.
@michel-laterman can this unit-test failure happen because FIPS enabled code is not decrypting correctly the blocks of both PKCS1 and PKCS8 keys? 🤔
There was a problem hiding this comment.
I would love this change to be as simple as returning always an error from NewFileKeystoreWithPasswordAndStrictPerms when it is compiled for FIPS. However I understand that due to external dependencies that do not handle appropriately an error there an alternative approach, such as the one of this PR is needed. Thus LGTM
There was a problem hiding this comment.
@michel-laterman instead of implementing a noop keystore, have you thought of removing the command from the fips compliant binaries?
Something similar to what was done for the APM Server elastic/apm-server#15545?
💚 Build Succeeded
History
|
There was a problem hiding this comment.
I don't think we should add a noop keystore. The implementation itself is ok, the issue is that we're passing an empty password.
Downstream clients should not create a keystore with an empty password in fips mode (e.g. by disabling the keystore)
EDIT: sorry, I missed @simitt comment above, fully agree with what was said about leaving the keystore impl (it would return an error anyway with an empty password in fips mode).
|
I have an alternate approach (as @kruskall suggested) that needs reviews: elastic/beats#42846 |
What does this PR do?
Keystore.Factory and other constuctor functions will return a NopKeystore if the requirefips build tag is set. The NopKeystore will will return ErrKeyDoesntExists for any retrievals, and ErrKeystoreUnsupported for create/save/store operations. The NopKeystore uses the same existing constructor name as the regular file keystore so it can be used in-place by callers.
Minor refactoring to move unchanged constructors, and the
Factorymethod tokeystore.go, renamedfile_keystore.gotofile_keystore_nofips.goto give an addition indicator to developers.Why is it important?
Current keystore implementation is not FIPS compliant.
Checklist
I have added tests that prove my fix is effective or that my feature works