Skip to content

[fips] do not expliclity set the default distro #91101

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

Merged
merged 11 commits into from
Oct 26, 2022

Conversation

jakelandis
Copy link
Contributor

@jakelandis jakelandis commented Oct 24, 2022

This commit no longer explicitly sets the default configuration for FIPS tests.
This allows each project's tests to run in FIPS mode with out deviation (other
than the FIPS mode).

A side product of this change is that any REST test
can now enable security if they so choose without needing to use the default
distribution. This allows for additional usage of the integ_test distribution
which can help with testing modularization.

This only possible now that the security plugin is always included
with the integ_test distribution via #77632

fixes: #70005
related: #77632


note - I ran a full test suite with FIPS hard coded as enabled to sucess. (It would be nice to have a GH label to do that as well - edit: neat that already exists ! added the label)
note - #71664 can be fixed with this change via https://github.com/elastic/elasticsearch/compare/main...jakelandis%3Afix_71664?body=&expand=1

@@ -114,7 +114,8 @@ public class ElasticsearchNode implements TestClusterConfiguration {
"path.repo",
"discovery.seed_providers",
"cluster.deprecation_indexing.enabled",
"cluster.initial_master_nodes"
"cluster.initial_master_nodes",
"xpack.security.enabled"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has the effect that allows you to enable security via the integ_test distribution (in x-pack or core). I think that is a good thing especially if we want to start using the integ_test distro for all module/plugin REST tests.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that security is disabled by default for INTEG_TEST distribution.

if (node.getTestDistribution().equals(TestDistribution.INTEG_TEST)) {
node.defaultConfig.put("xpack.security.enabled", "false");

I think that means most FIPS related stuff will not be excercised in tests with INTEG_TEST distribution even the fips.gradle file configures a bunch of settings, e.g. xpack.security.fips_mode.enabled. Theoretically, this is probably a reduced coverage for FIPS since we are currently forcing DEFAULT distribution for FIPS tests?

That said, there are still tons of tests that use the DEFAULT distribution so that coverage is not really an issue. And we can later look into whether it is necessary and how to enable security for INTEG_TEST.

Copy link
Contributor Author

@jakelandis jakelandis Oct 26, 2022

Choose a reason for hiding this comment

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

Theoretically, this is probably a reduced coverage for FIPS since we are currently forcing DEFAULT distribution for FIPS tests?

I don't think this will reduce coverage since security is also disabled for FIPS tests with the DEFAULT distribution. Individual tests can enable security. Before this change they could only enable security if they used the default distribution, but with this change they can override for any REST test.

setting 'xpack.security.enabled', 'false'

Copy link
Member

Choose a reason for hiding this comment

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

I missed that. Thanks!

@jakelandis jakelandis added :Delivery/Build Build or test infrastructure >non-issue labels Oct 25, 2022
@jakelandis jakelandis marked this pull request as ready for review October 25, 2022 21:27
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@elasticsearchmachine elasticsearchmachine added the Team:Delivery Meta label for Delivery team label Oct 25, 2022
@jakelandis jakelandis added the :Security/FIPS Running ES in FIPS 140-2 mode label Oct 25, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Oct 25, 2022
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for working on this.

@@ -114,7 +114,8 @@ public class ElasticsearchNode implements TestClusterConfiguration {
"path.repo",
"discovery.seed_providers",
"cluster.deprecation_indexing.enabled",
"cluster.initial_master_nodes"
"cluster.initial_master_nodes",
"xpack.security.enabled"
Copy link
Member

Choose a reason for hiding this comment

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

It seems that security is disabled by default for INTEG_TEST distribution.

if (node.getTestDistribution().equals(TestDistribution.INTEG_TEST)) {
node.defaultConfig.put("xpack.security.enabled", "false");

I think that means most FIPS related stuff will not be excercised in tests with INTEG_TEST distribution even the fips.gradle file configures a bunch of settings, e.g. xpack.security.fips_mode.enabled. Theoretically, this is probably a reduced coverage for FIPS since we are currently forcing DEFAULT distribution for FIPS tests?

That said, there are still tons of tests that use the DEFAULT distribution so that coverage is not really an issue. And we can later look into whether it is necessary and how to enable security for INTEG_TEST.

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

👍 We want to make it easier for folks to test against the integ test distribution.

@jakelandis jakelandis merged commit 4d27313 into elastic:main Oct 26, 2022
@jakelandis jakelandis deleted the fips_no_default_distro branch October 26, 2022 15:19
jakelandis added a commit that referenced this pull request Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure >non-issue :Security/FIPS Running ES in FIPS 140-2 mode Team:Delivery Meta label for Delivery team Team:Security Meta label for security team v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Use INTEG_TEST distribution in FIPS 140 testing when possible
4 participants