Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support for FIPS compliance mode #14912
base: main
Are you sure you want to change the base?
Support for FIPS compliance mode #14912
Changes from all commits
64b5506
1020171
e83d7b3
3241678
cce7eb7
f0ee7c4
71da5d5
bca541b
f78254c
eea51be
38a9e60
8cda2ce
6db7f0a
f775677
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Let's use
BuildParams
consistently please: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.
I believe this is optional dependency and should not be included by default (this is build tooling which mostly every single plugin will depend upon:
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.
(we sadly cannot use
BuildParams
here)Check warning on line 168 in buildSrc/src/main/java/org/opensearch/gradle/OpenSearchTestBasePlugin.java
Codecov / codecov/patch
buildSrc/src/main/java/org/opensearch/gradle/OpenSearchTestBasePlugin.java#L168
Check warning on line 170 in buildSrc/src/main/java/org/opensearch/gradle/OpenSearchTestBasePlugin.java
Codecov / codecov/patch
buildSrc/src/main/java/org/opensearch/gradle/OpenSearchTestBasePlugin.java#L170
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.
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.
Check warning on line 253 in buildSrc/src/main/java/org/opensearch/gradle/http/WaitForHttpResource.java
Codecov / codecov/patch
buildSrc/src/main/java/org/opensearch/gradle/http/WaitForHttpResource.java#L253
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.
Since FIPS dependency has to be optional, we need to make its usage so as well:
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.
I think it would be useful to extract this into
FipsBuild
helper class (or similar), so we could consolidateOPENSEARCH_CRYPTO_STANDARD
usage in 1 place: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.
(we could use it here and https://github.com/opensearch-project/OpenSearch/pull/14912/files#r1913283753)
Check warning on line 169 in buildSrc/src/main/java/org/opensearch/gradle/info/GlobalBuildInfoPlugin.java
Codecov / codecov/patch
buildSrc/src/main/java/org/opensearch/gradle/info/GlobalBuildInfoPlugin.java#L169
Check warning on line 187 in buildSrc/src/main/java/org/opensearch/gradle/info/GlobalBuildInfoPlugin.java
Codecov / codecov/patch
buildSrc/src/main/java/org/opensearch/gradle/info/GlobalBuildInfoPlugin.java#L187
Check warning on line 189 in buildSrc/src/main/java/org/opensearch/gradle/info/GlobalBuildInfoPlugin.java
Codecov / codecov/patch
buildSrc/src/main/java/org/opensearch/gradle/info/GlobalBuildInfoPlugin.java#L189
Check warning on line 551 in buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
Codecov / codecov/patch
buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L551
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.
historically opensearch-keystore tool didn't know how to correctly load BC-FJA module (and associated jars) nor able to create a keystore in
bcfks
format. Is this tool now able to require long enough password (14 characters) and create keystore in bcfks format? Note one may need to setJAVA_TOOL_OPTIONS" : "--module-path=/path/to/dir/with/bouncycastle-fips/jars/"
or similarly use maven extract plugin to uber-jar all bcfips jars; and then extract them to a temp location.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.
I agree - this circumstance is a big issue with the current branch.
KeyStoreWrapper
states that internal keystore has built-in FIPS compliance starting from ES 6.3. The keystore uses the AES/GCM/NoPadding algorithm with the PBKDF2WithHmacSHA512 key derivation function, and the maximum key length is limited to 128 chars in UTF-8.However, when the keystore is created using opensearch-cli, it does not run in FIPS mode with the BC libraries on the classpath. As a result
KeyStoreWrapper
falls back to the built-in Sun/Oracle security providers. This situation causes issues in the current integration tests because the$OPENSEARCH_CRYPTO_STANDARD
environment variable does not propagate in this scenario.To address this, as defined in
opensearch-cli
, the$OPENSEARCH_ADDITIONAL_CLASSPATH_DIRECTORIES
variable can be used to include BC libraries.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.
Note otherprojects have hit this too; it would be really nice if opensearch-cli could query the default keystore type; and really try hard to use it; and fail if it is incapable of doing it.
At chainguard we do set default keystore type to bcfks, but lots of tools fail to honor it; or have ability to specify additional
--module-path
or--classpath
to their java invocations, and/or otherwise vendor-in (uber-jar'ed-in) the bcfips jars to ensure the bcfks keystore creation "just works" upon launch as "normal".I do question the many places that currently uses the deprecated, even in Java8, keystore type of JKS. As imho all references to JKS should be eliminated towards requesting default keystore type which in most contexts today would be pkcs12, nss/pkcs11/nssdb-fips for the likes of rhel-ubi, and bcks / bcfks for the bcfips based jdks (like the ones from chainguard).
I guess I also should finally make a public docker image of a universal keytool that "just works" and has support for all keystore types, properly, out of the box. For efficient keystore creation / import / export / migration. To avoid need for individual projects of implementing that, yet again, themselves.
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.
I see where you're coming from and thank you for explaining the background.
The internal KeyStore is not intended to store certificate chains; instead, it acts like a key/value map with specific requirements that should operate w/o a FIPS context. I believe this is why it was designed in that manner. At least I don't see the request for possible migration to BCFKS standard as part of this PR.
Regarding Key- & TrustStores that are used for TLS communication, their type is usually predefined. If not, it is inferred from the file extension (see
KeyStoreType
).KeyStoreFactory
prevents non-secure KeyStore types from being instantiated. For example, if the application is running in a FIPS context but the TrustStore type has a JKS extension, a runtime exception will be thrown.Check warning on line 559 in buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
Codecov / codecov/patch
buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L559
Check warning on line 743 in buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
Codecov / codecov/patch
buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L743
Check warning on line 745 in buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
Codecov / codecov/patch
buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L745
This file was deleted.
This file was deleted.