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

Test adjustments for FIPS 140 #56427

Closed
wants to merge 3 commits into from
Closed

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented May 8, 2020

This change replaces #52211. Instead of explicitly setting
xpack.security.ssl.diagnose.trust to false in all or our tests,
we set xpack.security.fips_mode.enabled to true. This
implicitly disables the diagnostic trust manager and at the
same time mimics more closely the settings that we
expect users to set when running in a FIPS 140 JVM.
We need to disable the diagnostic trust manager
because we run our tests in Java 8 with the SunJSSE
in FIPS 140 mode and it explicitly disallows wrapping
the X509TrustManager.

The change seems large but it mostly consists of two
change categories:

  • Set xpack.security.fips_mode.enabled to true when we
    run tests in FIPS 140 mode.
  • Set the password hashing algorithm to PBKDF2 when
    we run tests in FIPS 140 mode (disallowing bcrypt where
    we already set it explicitly or setting it explicitly now so that
    the default ( bcrypt ) is not used.

The major problematic case in this PR is that the settings
above need to be set every time we create an
SSLService object with some settings, otherwise the
diagnostic trust manager will be enabled and the tests will
fail in Java 8. This is currently done by setting the setting
manually in test methods or providing a getSettingsBuilder
method for test classes where we create a Settings object
more than once. This covers the case now, but is in no
way future proof. Nothing stops one of us from adding
a test that needs a SSLServive object and creating a
Settings object manually without setting fips_mode.enabled
to true. This will pass in the PR CI as we don't run in FIPS
mode but it will fail for the next periodic FIPS job and it
will need to be triaged and fixed.

Alternatives I considered:

  • Adding the Settings.builder() to the forbiddent APIs for the
    security plugin. This seems like using a sledgehammer to
    drive a nail and it will be very confusing and annoying for
    developers, for little cost.
  • Adding a Base class for the security plugin test classes
    offering the getSettingsBuilder() method. That could be
    useful but it doesn't forbid anyone from adding a new
    test suite with class that doesn't extend that base class
  • Stop running our tests with the SunJSSE provider in
    Java 8 and use BCJSSE as we do with Java 11 onwards.
    I refrained from doing so I as I know that SunJSSE with
    BC as the crypto provider in FIPS mode is pretty popular
    and I would expect many organizations to run with that
    in Java 8. I could be swayed if we think the current
    solution is too brittle/ugly.
  • Adding a runtime check to disable the Diagnostic Trust
    Manager in FIPS mode in Java 8 when SunJSSE is used. This
    could be done but I'm not positive on how reliably we can
    check this and what's more important I don't feel
    comfortable adding production code for handling CI
    issues.

I don't necessarily love the approach taken here but it's
no use keeping the PR in my drawer for longer, so
I open it up for discussion and I;m really keen on
feedback/suggestions or ideas.

This commit adjusts our tests so that we set
`xpack.security.fips_mode.enabled` to true in all testclusters and
in the settings that we use to build SSLService objects and spin
up nodes otherwise.
This has the intent of running the tests in an environment and
configuration that is closer to what we expect users will run
elasticsearch in fips 140 mode and as a bonus it implicitly
disables the Diagnostic TrustManager that cannot be used when
SunJSSE is used in fips 140 mode.
@jkakavas jkakavas added >test Issues or PRs that are addressing/adding tests :Delivery/Build Build or test infrastructure :Security/Security Security issues without another label labels May 8, 2020
@jkakavas jkakavas requested review from tvernum and ywangd May 8, 2020 14:30
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Security Meta label for security team label May 8, 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 May 8, 2020
@tvernum
Copy link
Contributor

tvernum commented May 8, 2020

Sorry, I saw you discussed this at a meeting I didn't attend, and I planned to circle back, but forgot.
Did we consider a system property to disable the DiagnosticTrustManager? It's not a great option, but it's not completely without precedent.

@jkakavas
Copy link
Member Author

jkakavas commented May 8, 2020

Did we consider a system property to disable the DiagnosticTrustManager? It's not a great option, but it's not completely without precedent.

No, not really. Can you elaborate ? Do you mean like a system property that would force the DiagnosticTrustManager to be false even if it is configured to be true in the settings ? Or a system property that would set xpack.security.ssl.diagnose.trust to false ?

I haven't seen any precedent for the former, can you point me to it ? It does feel a bit strange ( but the existing solution is not great either ). If the latter, we do have a system property for our CI that we use ( tests.fips.enabled ) and this is pretty much what #52211 attempted to do.

@ywangd
Copy link
Member

ywangd commented May 11, 2020

IIUC, Tim's suggestion is to have something like the follows:

private static final Setting<Boolean> DIAGNOSE_TRUST_EXCEPTIONS_SETTING =
    Setting.boolSetting(
        "xpack.security.ssl.diagnose.trust", 
        Boolean.parseBoolean(System.getProperty("xpack.security.ssl.diagnose.trust", "true")),
        Setting.Property.NodeScope);

It's still a change to the production code, but much smaller footprint than dynamically detecting FIPS and SunJSSE. I cannot find an example of this approach in the existing code (master branch). But it feels reasonable to me.

@@ -48,7 +48,8 @@
public class TransportChangePasswordActionTests extends ESTestCase {

public void testAnonymousUser() {
final String hashingAlgorithm = randomFrom("pbkdf2", "pbkdf2_1000", "bcrypt", "bcrypt9");
final String hashingAlgorithm = inFipsJvm() ?
randomFrom("pbkdf2", "pbkdf2_1000") : randomFrom("pbkdf2", "pbkdf2_1000", "bcrypt", "bcrypt9");
Copy link
Contributor

Choose a reason for hiding this comment

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

Not essential, but would it be worth putting the choice of hashing algorithm in its own utility method?

jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request May 11, 2020
This change aims to fix our setup in CI so that we can run 7.x in
FIPS 140 mode. The major issue that we have in 7.x and did not
have in master is that we can't use the diagnostic trust manager
in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it
explicitly disallows the wrapping of X509TrustManager.

Previous attempts like elastic#56427 and elastic#52211 focused on disabling the
setting in all of our tests when creating a Settings object or
on setting fips_mode.enabled accordingly (which implicitly disables
the diagnostic trust manager). The attempts weren't future proof
though as nothing would forbid someone to add new tests without
setting the necessary setting and forcing this would be very
inconvenient for any other case ( see
elastic#56427 (comment)
for the full argumentation).

This change introduces a system property that effectively bypasses
the configuration value of xpack.security.ssl.diagnose.trust and
disables the diagnostic trust manager. We will then set this
system property in our periodic CI jobs for Java 8.
@jkakavas
Copy link
Member Author

Closing this for now in favor of #56526 ( which is much much cleaner )

@jkakavas jkakavas closed this May 11, 2020
jkakavas added a commit that referenced this pull request May 15, 2020
This change aims to fix our setup in CI so that we can run 7.x in
FIPS 140 mode. The major issue that we have in 7.x and did not
have in master is that we can't use the diagnostic trust manager
in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it
explicitly disallows the wrapping of X509TrustManager.

Previous attempts like #56427 and #52211 focused on disabling the
setting in all of our tests when creating a Settings object or
on setting fips_mode.enabled accordingly (which implicitly disables
the diagnostic trust manager). The attempts weren't future proof
though as nothing would forbid someone to add new tests without
setting the necessary setting and forcing this would be very
inconvenient for any other case ( see
#56427 (comment) for the full argumentation).

This change introduces a runtime check in SSLService that overrides
the configuration value of xpack.security.ssl.diagnose.trust and
disables the diagnostic trust manager when we are running in Java 8
and the SunJSSE provider is set in FIPS mode.
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Jun 5, 2020
This change aims to fix our setup in CI so that we can run 7.x in
FIPS 140 mode. The major issue that we have in 7.x and did not
have in master is that we can't use the diagnostic trust manager
in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it
explicitly disallows the wrapping of X509TrustManager.

Previous attempts like elastic#56427 and elastic#52211 focused on disabling the
setting in all of our tests when creating a Settings object or
on setting fips_mode.enabled accordingly (which implicitly disables
the diagnostic trust manager). The attempts weren't future proof
though as nothing would forbid someone to add new tests without
setting the necessary setting and forcing this would be very
inconvenient for any other case ( see
elastic#56427 (comment) for the full argumentation).

This change introduces a runtime check in SSLService that overrides
the configuration value of xpack.security.ssl.diagnose.trust and
disables the diagnostic trust manager when we are running in Java 8
and the SunJSSE provider is set in FIPS mode.
jkakavas added a commit that referenced this pull request Jun 8, 2020
This change aims to fix our setup in CI so that we can run 7.x in
FIPS 140 mode. The major issue that we have in 7.x and did not
have in master is that we can't use the diagnostic trust manager
in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it
explicitly disallows the wrapping of X509TrustManager.

Previous attempts like #56427 and #52211 focused on disabling the
setting in all of our tests when creating a Settings object or
on setting fips_mode.enabled accordingly (which implicitly disables
the diagnostic trust manager). The attempts weren't future proof
though as nothing would forbid someone to add new tests without
setting the necessary setting and forcing this would be very
inconvenient for any other case ( see
#56427 (comment) for the full argumentation).

This change introduces a runtime check in SSLService that overrides
the configuration value of xpack.security.ssl.diagnose.trust and
disables the diagnostic trust manager when we are running in Java 8
and the SunJSSE provider is set in FIPS mode.
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Jun 9, 2020
This change aims to fix our setup in CI so that we can run 7.x in
FIPS 140 mode. The major issue that we have in 7.x and did not
have in master is that we can't use the diagnostic trust manager
in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it
explicitly disallows the wrapping of X509TrustManager.

Previous attempts like elastic#56427 and elastic#52211 focused on disabling the
setting in all of our tests when creating a Settings object or
on setting fips_mode.enabled accordingly (which implicitly disables
the diagnostic trust manager). The attempts weren't future proof
though as nothing would forbid someone to add new tests without
setting the necessary setting and forcing this would be very
inconvenient for any other case ( see

This change introduces a runtime check in SSLService that overrides
the configuration value of xpack.security.ssl.diagnose.trust and
disables the diagnostic trust manager when we are running in Java 8
and the SunJSSE provider is set in FIPS mode.
jkakavas added a commit that referenced this pull request Jun 16, 2020
This change aims to fix our setup in CI so that we can run 7.7 in
FIPS 140 mode. The major issue that we have in 7.x and did not
have in master is that we can't use the diagnostic trust manager
in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it
explicitly disallows the wrapping of X509TrustManager.

Previous attempts like #56427 and #52211 focused on disabling the
setting in all of our tests when creating a Settings object or
on setting fips_mode.enabled accordingly (which implicitly disables
the diagnostic trust manager). The attempts weren't future proof
though as nothing would forbid someone to add new tests without
setting the necessary setting and forcing this would be very
inconvenient for any other case ( see

This change introduces a runtime check in SSLService that overrides
the configuration value of xpack.security.ssl.diagnose.trust and
disables the diagnostic trust manager when we are running in Java 8
and the SunJSSE provider is set in FIPS mode.
@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 :Security/Security Security issues without another label Team:Delivery Meta label for Delivery team Team:Security Meta label for security team >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants