Skip to content
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

Fix the REST FIPS tests #61001

Merged
merged 5 commits into from
Aug 13, 2020
Merged

Fix the REST FIPS tests #61001

merged 5 commits into from
Aug 13, 2020

Conversation

jakelandis
Copy link
Contributor

@jakelandis jakelandis commented Aug 12, 2020

When run with the -Dtests.fips.enabled=true most all of the REST tests fail.
This fixes most (all?) of the REST test failures in FIPS mode.

Specifically:

  • The bouncy castle library was no longer on the classpath for the tests for
    [yaml/java]RestTests since they explicitly set the classpath to only that of
    the test sourceset. The fix for this to ensure that bouncy castle is added to
    classpath for these (and all) types of tests when running with FIPS.

  • The elasticsearch.java plugin is not applied to every standalone test project.
    This mainly impacts the specialized tests that setup custom test clusters such as CCR.
    The fix for this is to hook into the java plugin instead of the elasticsearch.java plugin.

  • One or more project would throw an error about could not change dependency after
    evaluation (it did so without any of the changes here). The fix for this was to move
    the FIPS config out of the TestBasePlugin and just use the classpath addition via
    gradle/fips.gradle.

  • Ingest attachment was conditionally disabling integTest, now it conditionally
    disables yamlRestTest

fixes #60990

@jakelandis jakelandis changed the title Fix the FIPS tests Fix the REST FIPS tests Aug 12, 2020
@jakelandis jakelandis marked this pull request as ready for review August 12, 2020 13:36
@jakelandis jakelandis added :Delivery/Build Build or test infrastructure >test Issues or PRs that are addressing/adding tests v7.10.0 v7.8.2 v8.0.0 labels Aug 12, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Build)

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Aug 12, 2020
@rjernst
Copy link
Member

rjernst commented Aug 12, 2020

The elasticsearch.java plugin is not applied to every standalone test project.
This mainly impacts the specialized tests that setup custom test clusters such as CCR.
The fix for this is to ensure that the bouncy castle classpath addition is not tied
directly to the elasticsearch.java plugin.

It seems like the testclusters configuration block is still guarded by use of elasticsearch.java. I see that we are now adding the bcfips jar files, but without the necessary jvm args for Tests that are nested underneath the testclusters block, won't we be actually executing the tests without fips?

@jakelandis
Copy link
Contributor Author

won't we be actually executing the tests without fips?

I expected some failures once run on CI for the reasons you mention, but hadn't considered false positives where it passes but not actually running in FIPS. I will keep iterating on this ...

@jakelandis jakelandis marked this pull request as draft August 12, 2020 19:12
@jakelandis jakelandis marked this pull request as ready for review August 13, 2020 18:56
@jakelandis
Copy link
Contributor Author

@rjernst this should be ready for review, the additional change is pretty minor but seems to do the trick 5714e6d and pretty confident we won't get false positives. A scan of the filesystem and it appears that the bc jars are landing for all the specialized tests. I still have a couple failures locally, but don't think they are related... in short I am confident that this won't have false positives and fixes it for yaml/java REST tests, but not 100% sure that all tests are passing...need to commit to get CI to run the FIPS tests.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@rjernst rjernst merged commit d7dc93b into elastic:master Aug 13, 2020
rjernst pushed a commit that referenced this pull request Aug 13, 2020
Adds bouncycastle to classpath for tests and testclusters
rjernst pushed a commit that referenced this pull request Aug 14, 2020
Adds bouncycastle to classpath for tests and testclusters
@rjernst rjernst removed the v7.8.2 label Aug 14, 2020
@mark-vieira mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020
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 Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests v7.9.1 v7.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: FIPS builds in 7.8 and 7.9 failing due to Crypto provider not installed: BCFIPS
4 participants