Skip to content

Add ECS compliant option for username field #54051

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Add ECS compliant option for username field #54051

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6685b6a
Add esc compliant option for username field
ywangd Mar 24, 2020
61486b7
Tighten escCompliant option to enforce parent field name to be user
ywangd Mar 24, 2020
1562bdf
ensure ecs name consistency
ywangd Mar 26, 2020
f71af96
Merge remote-tracking branch 'origin/master' into es-51799-username-e…
ywangd Mar 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.function.Supplier;

import static org.elasticsearch.ingest.ConfigurationUtils.newConfigurationException;
import static org.elasticsearch.ingest.ConfigurationUtils.readBooleanProperty;
import static org.elasticsearch.ingest.ConfigurationUtils.readOptionalList;
import static org.elasticsearch.ingest.ConfigurationUtils.readStringProperty;

Expand All @@ -43,10 +44,14 @@ public final class SetSecurityUserProcessor extends AbstractProcessor {
private final XPackLicenseState licenseState;
private final String field;
private final Set<Property> properties;
private final boolean escCompliant;

public SetSecurityUserProcessor(String tag, SecurityContext securityContext, XPackLicenseState licenseState, String field,
Set<Property> properties) {
Set<Property> properties, boolean ecsCompliant) {
super(tag);
if (ecsCompliant && false == "user".equals(field)) {
throw newConfigurationException(TYPE, tag, "ecs_compliant", "ESC compliance requires [field] value to be 'user'");
}
this.securityContext = securityContext;
this.licenseState = Objects.requireNonNull(licenseState, "license state cannot be null");
if (licenseState.isAuthAllowed() == false) {
Expand All @@ -57,6 +62,7 @@ public SetSecurityUserProcessor(String tag, SecurityContext securityContext, XPa
}
this.field = field;
this.properties = properties;
this.escCompliant = ecsCompliant;
Copy link
Contributor

Choose a reason for hiding this comment

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

there are many places with the esc -> ecs inversion (including the PR title).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed most of them other than the one in the PR branch name. Thanks for spotting this.

}

@Override
Expand Down Expand Up @@ -93,7 +99,11 @@ public IngestDocument execute(IngestDocument ingestDocument) throws Exception {
switch (property) {
case USERNAME:
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes it so that the user can specify the username property when defining the pipeline, but the field is actually named name (in 8 when ecs_compliat is true by default). This is mildly confusing. I would suggest that we add a new property name with the same value as username.

if (user.principal() != null) {
userObject.put("username", user.principal());
if (escCompliant) {
userObject.put("name", user.principal());
} else {
userObject.put("username", user.principal());
}
}
break;
case FULL_NAME:
Expand Down Expand Up @@ -179,6 +189,10 @@ Set<Property> getProperties() {
return properties;
}

boolean isEscCompliant() {
return escCompliant;
}

public static final class Factory implements Processor.Factory {

private final Supplier<SecurityContext> securityContext;
Expand All @@ -193,6 +207,7 @@ public Factory(Supplier<SecurityContext> securityContext, Supplier<XPackLicenseS
public SetSecurityUserProcessor create(Map<String, Processor.Factory> processorFactories, String tag,
Map<String, Object> config) throws Exception {
String field = readStringProperty(TYPE, tag, config, "field");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the field have a default value of user for 8?

final Boolean ecsCompliant = readBooleanProperty(TYPE, tag, config, "ecs_compliant", true);
List<String> propertyNames = readOptionalList(TYPE, tag, config, "properties");
Set<Property> properties;
if (propertyNames != null) {
Expand All @@ -203,7 +218,8 @@ public SetSecurityUserProcessor create(Map<String, Processor.Factory> processorF
} else {
properties = EnumSet.allOf(Property.class);
}
return new SetSecurityUserProcessor(tag, securityContext.get(), licenseState.get(), field, properties);
return new SetSecurityUserProcessor(tag, securityContext.get(), licenseState.get(), field, properties,
ecsCompliant);
}
}

Expand Down
22 changes: 22 additions & 0 deletions ...st/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessorFactoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.security.ingest;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
Expand All @@ -20,6 +21,7 @@
import java.util.HashMap;
import java.util.Map;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;
import static org.mockito.Mockito.when;
Expand All @@ -40,6 +42,7 @@ public void testProcessor() throws Exception {
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext, () -> licenseState);
Map<String, Object> config = new HashMap<>();
config.put("field", "_field");
config.put("ecs_compliant", false);
SetSecurityUserProcessor processor = factory.create(null, "_tag", config);
assertThat(processor.getField(), equalTo("_field"));
assertThat(processor.getProperties(), equalTo(EnumSet.allOf(Property.class)));
Expand All @@ -58,6 +61,7 @@ public void testProcessor_validProperties() throws Exception {
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext, () -> licenseState);
Map<String, Object> config = new HashMap<>();
config.put("field", "_field");
config.put("ecs_compliant", false);
config.put("properties", Arrays.asList(Property.USERNAME.name(), Property.ROLES.name()));
SetSecurityUserProcessor processor = factory.create(null, "_tag", config);
assertThat(processor.getField(), equalTo("_field"));
Expand All @@ -80,8 +84,26 @@ public void testCanConstructorProcessorWithoutSecurityEnabled() throws Exception
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> null, () -> licenseState);
Map<String, Object> config = new HashMap<>();
config.put("field", "_field");
config.put("ecs_compliant", false);
final SetSecurityUserProcessor processor = factory.create(null, "_tag", config);
assertThat(processor, notNullValue());
}

public void testEscCompliantDefaultToTrue() throws Exception {
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext, () -> licenseState);
Map<String, Object> config = new HashMap<>();
config.put("field", "user");
SetSecurityUserProcessor processor = factory.create(null, "_tag", config);
assertTrue(processor.isEscCompliant());
}

public void testEscCompliantRequiresParentFieldToBeUser() throws Exception {
SetSecurityUserProcessor.Factory factory = new SetSecurityUserProcessor.Factory(() -> securityContext, () -> licenseState);
Map<String, Object> config = new HashMap<>();
config.put("field", "_field");
config.put("ecs_compliant", true);
final ElasticsearchException e = expectThrows(ElasticsearchException.class, () -> factory.create(null, "_tag", config));
assertThat(e.getMessage(), containsString("[ecs_compliant] ESC compliance requires [field] value to be 'user'"));
}

}
57 changes: 44 additions & 13 deletions .../src/test/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.security.ingest;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.Version;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
Expand All @@ -27,7 +28,10 @@
import java.util.HashMap;
import java.util.Map;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.nullValue;
import static org.mockito.Mockito.when;

public class SetSecurityUserProcessorTests extends ESTestCase {
Expand All @@ -52,7 +56,7 @@ public void testProcessorWithData() throws Exception {

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class));
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class), false);
processor.execute(ingestDocument);

Map<String, Object> result = ingestDocument.getFieldValue("_field", Map.class);
Expand Down Expand Up @@ -82,7 +86,7 @@ public void testProcessorWithEmptyUserData() throws Exception {

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class));
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class), false);
processor.execute(ingestDocument);
Map<String, Object> result = ingestDocument.getFieldValue("_field", Map.class);
// Still holds data for realm and authentication type
Expand All @@ -95,7 +99,7 @@ public void testProcessorWithEmptyUserData() throws Exception {
public void testNoCurrentUser() throws Exception {
IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class));
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class), false);
IllegalStateException e = expectThrows(IllegalStateException.class, () -> processor.execute(ingestDocument));
assertThat(e.getMessage(),
equalTo("There is no authenticated user - the [set_security_user] processor requires an authenticated user"));
Expand All @@ -105,7 +109,7 @@ public void testSecurityDisabled() throws Exception {
when(licenseState.isAuthAllowed()).thenReturn(false);
IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class));
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class), false);
IllegalStateException e = expectThrows(IllegalStateException.class, () -> processor.execute(ingestDocument));
assertThat(e.getMessage(), equalTo("Security (authentication) is not enabled on this cluster, so there is no active user" +
" - the [set_security_user] processor cannot be used without security"));
Expand All @@ -118,7 +122,7 @@ public void testUsernameProperties() throws Exception {

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.of(Property.USERNAME));
"_tag", securityContext, licenseState, "_field", EnumSet.of(Property.USERNAME), false);
processor.execute(ingestDocument);

@SuppressWarnings("unchecked")
Expand All @@ -134,7 +138,7 @@ public void testRolesProperties() throws Exception {

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.of(Property.ROLES));
"_tag", securityContext, licenseState, "_field", EnumSet.of(Property.ROLES), false);
processor.execute(ingestDocument);

@SuppressWarnings("unchecked")
Expand All @@ -150,7 +154,7 @@ public void testFullNameProperties() throws Exception {

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor
= new SetSecurityUserProcessor("_tag", securityContext, licenseState, "_field", EnumSet.of(Property.FULL_NAME));
= new SetSecurityUserProcessor("_tag", securityContext, licenseState, "_field", EnumSet.of(Property.FULL_NAME), false);
processor.execute(ingestDocument);

@SuppressWarnings("unchecked")
Expand All @@ -166,7 +170,7 @@ public void testEmailProperties() throws Exception {

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.of(Property.EMAIL));
"_tag", securityContext, licenseState, "_field", EnumSet.of(Property.EMAIL), false);
processor.execute(ingestDocument);

@SuppressWarnings("unchecked")
Expand All @@ -182,7 +186,7 @@ public void testMetadataProperties() throws Exception {

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.of(Property.METADATA));
"_tag", securityContext, licenseState, "_field", EnumSet.of(Property.METADATA), false);
processor.execute(ingestDocument);

@SuppressWarnings("unchecked")
Expand All @@ -198,7 +202,7 @@ public void testOverwriteExistingField() throws Exception {
new Authentication(user, realmRef, null).writeToContext(threadContext);

SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.of(Property.USERNAME));
"_tag", securityContext, licenseState, "_field", EnumSet.of(Property.USERNAME), false);

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
ingestDocument.setFieldValue("_field", "test");
Expand Down Expand Up @@ -238,7 +242,7 @@ public void testApiKeyPopulation() throws Exception {

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class));
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class), false);
processor.execute(ingestDocument);

Map<String, Object> result = ingestDocument.getFieldValue("_field", Map.class);
Expand Down Expand Up @@ -269,7 +273,7 @@ public void testWillNotOverwriteExistingApiKeyAndRealm() throws Exception {
"_field", Map.of("api_key", Map.of("version", 42), "realm", Map.of("id", 7))
)), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class));
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class), false);
processor.execute(ingestDocument);

Map<String, Object> result = ingestDocument.getFieldValue("_field", Map.class);
Expand All @@ -292,7 +296,7 @@ public void testWillSetRunAsRealmForNonApiAuth() throws Exception {

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class));
"_tag", securityContext, licenseState, "_field", EnumSet.allOf(Property.class), false);
processor.execute(ingestDocument);

Map<String, Object> result = ingestDocument.getFieldValue("_field", Map.class);
Expand All @@ -301,4 +305,31 @@ public void testWillSetRunAsRealmForNonApiAuth() throws Exception {
assertThat(((Map) result.get("realm")).get("type"), equalTo(lookedUpRealmRef.getType()));
}

public void testWillEnsureEscComplianceWhenParentFieldIsUser() throws Exception {
User user = new User("_username", null, null);
Authentication.RealmRef realmRef = new Authentication.RealmRef("_name", "_type", "_node_name");
new Authentication(user, realmRef, null).writeToContext(threadContext);

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "user", EnumSet.of(Property.USERNAME), true);
processor.execute(ingestDocument);

@SuppressWarnings("unchecked")
Map<String, Object> result = ingestDocument.getFieldValue("user", Map.class);
assertThat(result.size(), equalTo(1));
assertThat(result.get("name"), equalTo("_username"));
assertThat(result.get("username"), is(nullValue()));
}

public void testWillThrowWhenEscComplianceIsSetButParentFieldIsNotUser() throws Exception {
User user = new User("_username", null, null);
Authentication.RealmRef realmRef = new Authentication.RealmRef("_name", "_type", "_node_name");
new Authentication(user, realmRef, null).writeToContext(threadContext);

IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>());
ElasticsearchException e = expectThrows(ElasticsearchException.class, () -> new SetSecurityUserProcessor(
"_tag", securityContext, licenseState, "auth", EnumSet.of(Property.USERNAME), true));
assertThat(e.getMessage(), containsString("[ecs_compliant] ESC compliance requires [field] value to be 'user'"));
}
}
4 changes: 2 additions & 2 deletions ...ugin/src/test/resources/rest-api-spec/test/set_security_user/10_small_users_one_index.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ teardown:
index: shared_logs
body: { "query" : { "match_all" : {} } }
- match: { hits.total: 1}
- match: { hits.hits.0._source.user.username: joe}
- match: { hits.hits.0._source.user.name: joe}
- match: { hits.hits.0._source.user.roles.0: company_x_logs_role}

# John searches:
Expand All @@ -139,5 +139,5 @@ teardown:
index: shared_logs
body: { "query" : { "match_all" : {} } }
- match: { hits.total: 1}
- match: { hits.hits.0._source.user.username: john}
- match: { hits.hits.0._source.user.name: john}
- match: { hits.hits.0._source.user.roles.0: company_y_logs_role}
10 changes: 5 additions & 5 deletions ...security-with-mustache/src/test/resources/rest-api-spec/test/20_small_users_one_index.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ teardown:
"query" : {
"template" : {
"source" : {
"term" : { "user.username" : "{{_user.username}}" }
"term" : { "user.name" : "{{_user.username}}" }
}
}
}
Expand Down Expand Up @@ -116,7 +116,7 @@ teardown:
index: shared_logs
body: { "query" : { "match_all" : {} } }
- match: { hits.total: 1}
- match: { hits.hits.0._source.user.username: joe}
- match: { hits.hits.0._source.user.name: joe}

# John searches:
- do:
Expand All @@ -127,7 +127,7 @@ teardown:
index: shared_logs
body: { "query" : { "match_all" : {} } }
- match: { hits.total: 1}
- match: { hits.hits.0._source.user.username: john}
- match: { hits.hits.0._source.user.name: john}

---
"Test shared index separating user by using DLS role query with user's metadata":
Expand Down Expand Up @@ -184,7 +184,7 @@ teardown:
index: shared_logs
body: { "query" : { "match_all" : {} } }
- match: { hits.total: 1}
- match: { hits.hits.0._source.user.username: joe}
- match: { hits.hits.0._source.user.name: joe}

# John searches:
- do:
Expand All @@ -195,4 +195,4 @@ teardown:
index: shared_logs
body: { "query" : { "match_all" : {} } }
- match: { hits.total: 1}
- match: { hits.hits.0._source.user.username: john}
- match: { hits.hits.0._source.user.name: john}