Add a smoke test for security realms #68881

tvernum · 2021-02-11T04:11:37Z

This changes adds a new QA test that runs a smoke test on a node that
has been configured with one realm of each type.

Not all of the realms work, because some of them would depend on
external fixtures (LDAP, SAML, etc) and this particularly test suite
is intended to be as stable as possible and have no external
dependencies.

The primary purpose of this test is to catch any issues that prevent
a node from starting with particular realms configured (e.g. security
manager or classpath issues). We don't depend on external fixtures
because we want this to be a smoke test that clearly indicates when a
(seemingly unrelated) change in Elasticsearch has unintended
consequences on realms. The use of external dependencies would
increase the number of things that could go wrong and move this from a
smoke test to a potentially noisy integration test.

Relates: #68838, #68872

This changes adds a new QA test that runs a smoke test on a node that has been configured with one realm of each type. Not all of the realms work, because some of them would depend on external fixtures (LDAP, SAML, etc) and this particularly test suite is intended to be as stable as possible and have no external dependencies. The primary purpose of this test is to catch any issues that prevent a node from starting with particular realms configurd (e.g. security manager or classpath issues). We don't depend on external fixtures becaused we want this to be a smoke test that clearly indicates when a (seemingly unrelated) change in Elasticsearch has unintended consequences on realms. The use of external dependencies would increase the number of things that could go wrong and move this from a smoke test to a potentially noisy integration test.

elasticmachine · 2021-02-11T04:11:40Z

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

tvernum · 2021-02-11T04:27:36Z

For verification purposes here is a gradle scan where this test passes (because the bug is fixed).

If I cherry pick this test on top of the last commit before the LDAP bug was fixed (683368c), then we get this gradle scan that has the expected classloading failure.

ywangd

LGTM.
Just to be paranoid, we could test the reserved realm as well.

ywangd · 2021-02-11T04:59:38Z

...-all-realms/src/javaRestTest/java/org/elasticsearch/xpack/security/authc/PkiRealmAuthIT.java

+
+        assertUsername(authenticate, USERNAME);
+        assertRealm(authenticate, "pki", "pki4");
+        assertRoles(authenticate, new String[0]);


The new String[0] argument is redundant since the method takes varargs. Or did you intentionally add it for more clarity?

It is for clarity. I find

assertRoles(authenticate);

a bit weird. Technically it does the same thing, but it doesn't read well.

The method normally reads like:

Assert the the authentication response has roles foo

With the empty array it reads

Assert the the authentication response has roles empty

But with no argument at all it reads

Assert the the authentication response has roles ...

ywangd · 2021-02-11T05:13:24Z

...src/javaRestTest/java/org/elasticsearch/xpack/security/authc/SecurityRealmSmokeTestCase.java

+    protected void createRole(String name, Collection<String> clusterPrivileges) throws IOException {
+        final RestHighLevelClient client = getHighLevelAdminClient();
+        final Role role = Role.builder().name(name).clusterPrivileges(clusterPrivileges).build();
+        client.security().putRole(new PutRoleRequest(role, null), RequestOptions.DEFAULT);


Nit: I'd also use RefreshPolicy.WAIT_UNTIL for PutRoleRequest even though it is not necessary for existing use cases. The performance gain with null is trivial and it feels more future proof to use WAIT_UNTIL or even IMMEDIATE.

ywangd · 2021-02-11T05:28:37Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

+        } else if (clientCertificatePath != null) {
+            throw new IllegalStateException("Client certificates are currently only support when using a custom CA");


Why do we have this restriction? Is it just so we don't need to bother writing code for this branch (no test would be setup like this anyway)?

I didn't want to write yet another block of code if no one was going to use it (and therefore it had a good chance of not actually working).

But I also didn't want to leave it in a state where someone would try and use it (e.g. a truststore and a client cert in PEM) and then have to debug why their code didn't do what they thought it should do.

ywangd · 2021-02-11T05:39:34Z

For verification purposes here is a gradle scan where this test passes (because the bug is fixed).

If I cherry pick this test on top of the last commit before the LDAP bug was fixed (683368c), then we get this gradle scan that has the expected classloading failure.

It is also FIPS worthy!

jkakavas

LGTM. I guess there can still be things that we miss ( i.e. because a class can be loaded in the process of authenticating to an external system and not realm initialization - but hopefully realm integ tests can catch these? ) but I fully agree with the idea of having this smoke test be stripped of all external dependencies.

jkakavas · 2021-02-11T07:07:47Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

                throw new RuntimeException("Error setting up ssl", e);
            }
+        } else if (clientCertificatePath != null) {
+            throw new IllegalStateException("Client certificates are currently only support when using a custom CA");


Suggested change

throw new IllegalStateException("Client certificates are currently only support when using a custom CA");

throw new IllegalStateException("Client certificates are currently only supported when using a custom CA");

This ended up being the only comment, I wouldn't necessarily respin all CI checks just to change this now.

jkakavas · 2021-02-11T07:11:43Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

                assertThat("License must exist", map.containsKey("license"), equalTo(true));
-                @SuppressWarnings("unchecked")
-                final Map<String, ?> license = (Map<String, ?>) map.get("license");
+                @SuppressWarnings("unchecked") final Map<String, ?> license = (Map<String, ?>) map.get("license");


Is this the IDE doing autoformatting or our spotless plugin being applied on changes? Keeping the diff to reflect actual changes helps a lot in reviewing ( "hide whitespace changes" is not enough.. )

Writing this mostly as a mental note so that we can discuss it at some point

This was IDE. I'm not sure what the official style guide is - I didn't even notice the change this time.

tvernum · 2021-02-11T12:06:46Z

@elasticmachine update branch

This changes adds a new QA test that runs a smoke test on a node that has been configured with one realm of each type. Not all of the realms work, because some of them would depend on external fixtures (LDAP, SAML, etc) and this particularly test suite is intended to be as stable as possible and have no external dependencies. The primary purpose of this test is to catch any issues that prevent a node from starting with particular realms configurd (e.g. security manager or classpath issues). We don't depend on external fixtures becaused we want this to be a smoke test that clearly indicates when a (seemingly unrelated) change in Elasticsearch has unintended consequences on realms. The use of external dependencies would increase the number of things that could go wrong and move this from a smoke test to a potentially noisy integration test. Backport of: elastic#68881

This changes adds a new QA test that runs a smoke test on a node that has been configured with one realm of each type. Not all of the realms work, because some of them would depend on external fixtures (LDAP, SAML, etc) and this particularly test suite is intended to be as stable as possible and have no external dependencies. The primary purpose of this test is to catch any issues that prevent a node from starting with particular realms configurd (e.g. security manager or classpath issues). We don't depend on external fixtures becaused we want this to be a smoke test that clearly indicates when a (seemingly unrelated) change in Elasticsearch has unintended consequences on realms. The use of external dependencies would increase the number of things that could go wrong and move this from a smoke test to a potentially noisy integration test. Backport of: #68881

tvernum added >test Issues or PRs that are addressing/adding tests :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.12.0 v7.11.1 labels Feb 11, 2021

tvernum requested review from jkakavas and ywangd February 11, 2021 04:11

elasticmachine added the Team:Security Meta label for security team label Feb 11, 2021

ywangd approved these changes Feb 11, 2021

View reviewed changes

tvernum added 2 commits February 11, 2021 17:32

Merge branch 'master' into all-realm-smoke-test

668d723

Address feedback: Reserved realm, WAIT_UNTIL

2c2db80

jkakavas approved these changes Feb 11, 2021

View reviewed changes

pugnascotia added v7.11.2 and removed v7.11.1 labels Feb 11, 2021

Revert formatting changes in ESRestTestCase

b90bf09

Merge branch 'master' into all-realm-smoke-test

c72eaeb

tvernum merged commit 4d9ebca into elastic:master Feb 12, 2021

tvernum added the backport pending label Feb 12, 2021

tvernum deleted the all-realm-smoke-test branch February 12, 2021 01:43

tvernum mentioned this pull request Feb 12, 2021

[Backport] Add a smoke test for security realms #68950

Merged

tvernum mentioned this pull request Feb 12, 2021

[Backport 7.11] Add a smoke test for security realms #68951

Merged

tvernum removed the backport pending label Feb 16, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

		} else if (clientCertificatePath != null) {
		throw new IllegalStateException("Client certificates are currently only support when using a custom CA");

Add a smoke test for security realms #68881

Add a smoke test for security realms #68881

Uh oh!

Conversation

tvernum commented Feb 11, 2021

Uh oh!

elasticmachine commented Feb 11, 2021

Uh oh!

tvernum commented Feb 11, 2021

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd commented Feb 11, 2021

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvernum commented Feb 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants