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.

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

Jump to bottom

Validate role templates before saving role mapping #52636

Merged
merged 21 commits into from
Mar 24, 2020
Merged

Validate role templates before saving role mapping #52636

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
3cce84d
Validate role templates before save
ywangd Feb 21, 2020
07c7819
No double wrap for illegal argument exception
ywangd Feb 21, 2020
a593def
Fix doc tests
ywangd Feb 21, 2020
3e2e4b4
Merge remote-tracking branch 'origin/master' into es-48773-role-mappi…
ywangd Feb 21, 2020
46cc8d3
Merge remote-tracking branch 'origin/master' into es-48773-role-mappi…
ywangd Feb 26, 2020
e07b243
Address feedback
ywangd Feb 26, 2020
51d9c20
Revert change to mustacheTemplateEvaluator
ywangd Feb 26, 2020
18333bb
Add more tests for role template validation
ywangd Feb 28, 2020
e4f8ec2
Merge remote-tracking branch 'origin/master' into es-48773-role-mappi…
ywangd Feb 28, 2020
d58a4c8
Fix tests and add doc
ywangd Feb 28, 2020
4587f14
Update x-pack/docs/en/rest-api/security/create-role-mappings.asciidoc
ywangd Mar 1, 2020
7ed5322
Merge remote-tracking branch 'origin/master' into es-48773-role-mappi…
ywangd Mar 2, 2020
59b8405
Add example of using stored script for role templates
ywangd Mar 2, 2020
2f44946
Update x-pack/docs/en/rest-api/security/create-role-mappings.asciidoc
ywangd Mar 2, 2020
2f84c1e
Update x-pack/docs/en/rest-api/security/create-role-mappings.asciidoc
ywangd Mar 2, 2020
3f2512f
Update x-pack/docs/en/rest-api/security/create-role-mappings.asciidoc
ywangd Mar 2, 2020
f9c0b48
Merge remote-tracking branch 'origin/master' into es-48773-role-mappi…
ywangd Mar 3, 2020
b6f68ba
[DOCS] Moves role templates into API description
lcawl Mar 3, 2020
c86d09b
Update x-pack/docs/en/rest-api/security/create-role-mappings.asciidoc
lcawl Mar 3, 2020
d464d73
Merge remote-tracking branch 'origin/master' into es-48773-role-mappi…
ywangd Mar 24, 2020
fe0da67
Remove doc of using stored script for role template since we do not r…
ywangd Mar 24, 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
41 changes: 38 additions & 3 deletions x-pack/docs/en/rest-api/security/create-role-mappings.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Creates and updates role mappings.
[[security-api-put-role-mapping-desc]]
==== {api-description-title}

Role mappings define which roles are assigned to each user. Each mapping has
Role mappings define which roles are assigned to each user. Each mapping has
_rules_ that identify users and a list of _roles_ that are granted to those users.

The role mapping APIs are generally the preferred way to manage role mappings
Expand Down Expand Up @@ -77,10 +77,14 @@ _Exactly one of `roles` or `role_templates` must be specified_.
`rules`::
(Required, object) The rules that determine which users should be matched by the
mapping. A rule is a logical condition that is expressed by using a JSON DSL.
See <<role-mapping-resources>>.
See <<role-mapping-resources>>.

==== Role Templates

NOTE: Role templates require the relevant scripting feature to be enabled,
Copy link
Contributor

@lcawl lcawl Mar 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this NOTE should move below the introductory paragraph, since at this point we haven't defined role templates yet. I actually also think this whole "Role templates" subsection should be moved into the "Description" section. If you're willing to let me add that change to this PR, let me know and I'll push a commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Lisa. Please make changes as you see fit. Thanks a lot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done, thanks!

ywangd marked this conversation as resolved.
Show resolved Hide resolved
otherwise the creation of a role mapping with role templates will fail.
ywangd marked this conversation as resolved.
Show resolved Hide resolved
See <<allowed-script-types-setting>>.

The most common use for role mappings is to create a mapping from a known value
on the user to a fixed role name.
For example, all users in the `cn=admin,dc=example,dc=com` LDAP group should be
Expand Down Expand Up @@ -233,6 +237,38 @@ POST /_security/role_mapping/mapping5
<2> Because the template produces a JSON array, the format must be
set to `json`.

In addition to inline script, we can also use stored script for the
ywangd marked this conversation as resolved.
Show resolved Hide resolved
`template` field. Above role mapping can also be created with the follows:

[source,console]
------------------------------------------------------------
POST /_scripts/derive-roles
{
"script": {
"lang": "mustache",
"source" : "{{#tojson}}groups{{/tojson}}"
}
}
------------------------------------------------------------

[source,console]
------------------------------------------------------------
POST /_security/role_mapping/mapping5
{
"role_templates": [
{
"template": { "id": "derive-roles" },
"format" : "json"
}
],
"rules": {
"field" : { "realm.name" : "saml1" }
},
"enabled": true
}
------------------------------------------------------------
// TEST[continued]

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we think carefully about whether we really want to document & support this?
What's the use case? Why do we want to elevate this to "officially supported"?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm good question. One issue of using stored script is consistency, i.e. the script can change without the role-mapping being aware of it. This could potentially creates confusion and support time frustration. Is this the reason why we may not want to "officially" support it? (I assume adding it to official docs means offical support).

Copy link
Member

Choose a reason for hiding this comment

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

What's the use case? Why do we want to elevate this to "officially supported"?

Can I flip the question ? Why do we "unofficially' support this already ? Why not disallow it instead ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I undersetand there are cases where we intentionally left them as a grey area, i.e. "unofficially supported". But I feel this is not the case here because:

  1. Stored script support is not an unique feature for this API. It is available and documented elsewhere. So in some sense, it is already officially supported. There could be issues with stored scripts in general, which should be a separate discussion.
  2. If we decide to disable the support here, it will be a breaking change even it is "unofficially" supported. So keeping it "unofficial" does not really buy us anything from this perspective.
  3. General gain of clarity when something is clearly documented

So overall, I'd prefer to have it documented. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced.
What reason do we have for people to do this? Why should we spend time and effort maintaining the docs for this, when it has no clear value to anyone?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there some context that I am not aware of, e.g. any past discussions about not having this documented in the first place? I admit that I don't have a strong reason other than "completeness" to add this in. But are there strong reasons to not add it? I can think of a few candidates:

  1. We don't want people to use stored script for authentication related stuff since a missing script is more devastating
  2. We don't encourage stored script usage in general
  3. We may want disable stored script for role template in future and by not documenting it we could avoid a breaking change
  4. No user requires it
  5. We don't spend the extra time and effort because of 4

I'd be happy to go with item 1, 2, 3, but not particularly convinced by 4 and 5. I cannot say user would see value in this, but cannot say they would not either, unless there are past evidents of this being no value. Also the time and effort for this additional docs are not really significant.

I am not really attached to this snippet of docs. I am happy to either remove or keep it. Just like to clear my thought process. I'd appreicate if you could elaborate it a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Less is more.
There's a temptation toward more docs (and lots of places where our docs are too brief) but it isn't always better.

One of our recurring experiences is that people try and configure ES security based on blogs because they seem simpler. The official docs have "more steps" so they follow a blog and then get stuck because those steps were actually necessarily and the blog skipped over them (or made assumptions that allowed them to ignore the steps) to keep it short.

Documenting something, tells people "this might be helpful to you" and also "you should read this if you want to understand how to use this feature". But I don't see any argument for why either of those are true here. So what's the reason to do it?

Or, put another way, if a customer raises a support ticket saying "I'm trying to use a role mapping with a stored script as the template" my immediate reaction would be "Why on earth would you do that? That's a terrible idea!"
And their (entirely reasonable) answer will be because you documented it.

I cannot say user would see value in this

Then don't do it.
It's easy - if we cannot work out a reason why users would want to do it, then don't recommend it to them. Don't waste time writing and maintaining the docs. Don't waste their time by asking them to read those docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

if a customer raises a support ticket saying "I'm trying to use a role mapping with a stored script as the template" my immediate reaction would be "Why on earth would you do that? That's a terrible idea!"

I can resonate with this argument. I do agree it's not a good idea to make a role template depend on external stuff. Since we cannot or don't want to recommend this usage ourselves, it makes sense to leave it out. Will update accordingly. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

For what it’s worth, the role mapping UI in Kibana attempts to support the entire API surface. In other words, any valid role mapping created via the API should render and be editable in the UI. We of course default to inline scripts for these templates in the interface, but since stored scripts are technically available, we have to know how to render those as well, so there are UI controls to edit them.

I personally agree that these stored templates don’t make a lot of sense. Even if they aren’t documented, they’re more discoverable than they used to be because we now have a dedicated UI. For some folks, the UI will become the documentation, so like it or not, I fear we end up (currently) with the appearance that this is a first-class feature of role mappings.

We can consider changes to the UI which only surface these controls when editing an exiting mapping which already contains a stored script, but not without additional complexity

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the insights @legrego
You are right that the UI could be considered as documentation or even source of truth for some users. The UI also has a link to the doco ... it is possible that users would notice the difference and ask "why is there no document for this feature".

For now, we are trying to not actively promote it. I wonder whether we could afford to disable it in 8.0. But I suspect that the solution could end up being adding more machinery to make this feature more robust, e.g. reference counting a stored script.

The following example matches users within a specific LDAP sub-tree:

[source,console]
Expand Down Expand Up @@ -339,4 +375,3 @@ POST /_security/role_mapping/mapping9
<1> Because it is not possible to specify both `roles` and `role_templates` in
the same role mapping, we can apply a "fixed name" role by using a template
that has no substitutions.

12 changes: 12 additions & 0 deletions ...ain/java/org/elasticsearch/xpack/core/security/authc/support/mapper/TemplateRoleName.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,18 @@ public List<String> getRoleNames(ScriptService scriptService, ExpressionModel mo
}
}

public void validate(ScriptService scriptService) {
try {
parseTemplate(scriptService, Collections.emptyMap());
} catch (IllegalArgumentException e) {
throw e;
} catch (IOException e) {
throw new UncheckedIOException(e);
} catch (Exception e) {
throw new IllegalArgumentException(e);
}
}

private List<String> convertJsonToList(String evaluation) throws IOException {
final XContentParser parser = XContentFactory.xContent(XContentType.JSON).createParser(NamedXContentRegistry.EMPTY,
LoggingDeprecationHandler.INSTANCE, evaluation);
Expand Down
117 changes: 117 additions & 0 deletions ...ava/org/elasticsearch/xpack/core/security/authc/support/mapper/TemplateRoleNameTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

package org.elasticsearch.xpack.core.security.authc.support.mapper;

import org.elasticsearch.cluster.ClusterChangedEvent;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
Expand All @@ -17,8 +20,11 @@
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.script.ScriptException;
import org.elasticsearch.script.ScriptMetaData;
import org.elasticsearch.script.ScriptModule;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.script.StoredScriptSource;
import org.elasticsearch.script.mustache.MustacheScriptEngine;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.EqualsHashCodeTestUtils;
Expand All @@ -31,8 +37,11 @@
import java.util.Collections;

import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class TemplateRoleNameTests extends ESTestCase {

Expand Down Expand Up @@ -116,4 +125,112 @@ public void tryEquals(TemplateRoleName original) {
};
EqualsHashCodeTestUtils.checkEqualsAndHashCode(original, copy, mutate);
}

public void testValidate() {
final ScriptService scriptService = new ScriptService(Settings.EMPTY,
Collections.singletonMap(MustacheScriptEngine.NAME, new MustacheScriptEngine()), ScriptModule.CORE_CONTEXTS);

final TemplateRoleName plainString = new TemplateRoleName(new BytesArray("{ \"source\":\"heroes\" }"), Format.STRING);
plainString.validate(scriptService);

final TemplateRoleName user = new TemplateRoleName(new BytesArray("{ \"source\":\"_user_{{username}}\" }"), Format.STRING);
user.validate(scriptService);

final TemplateRoleName groups = new TemplateRoleName(new BytesArray("{ \"source\":\"{{#tojson}}groups{{/tojson}}\" }"),
Format.JSON);
groups.validate(scriptService);

final TemplateRoleName notObject = new TemplateRoleName(new BytesArray("heroes"), Format.STRING);
expectThrows(IllegalArgumentException.class, () -> notObject.validate(scriptService));

final TemplateRoleName invalidField = new TemplateRoleName(new BytesArray("{ \"foo\":\"heroes\" }"), Format.STRING);
expectThrows(IllegalArgumentException.class, () -> invalidField.validate(scriptService));
}

public void testValidateWillPassWithEmptyContext() {
final ScriptService scriptService = new ScriptService(Settings.EMPTY,
Collections.singletonMap(MustacheScriptEngine.NAME, new MustacheScriptEngine()), ScriptModule.CORE_CONTEXTS);

final BytesReference template = new BytesArray("{ \"source\":\"" +
"{{username}}/{{dn}}/{{realm}}/{{metadata}}" +
"{{#realm}}" +
" {{name}}/{{type}}" +
"{{/realm}}" +
"{{#toJson}}groups{{/toJson}}" +
"{{^groups}}{{.}}{{/groups}}" +
"{{#metadata}}" +
" {{#first}}" +
" <li><strong>{{name}}</strong></li>" +
" {{/first}}" +
" {{#link}}" +
" <li><a href=\\\"{{url}}\\\">{{name}}</a></li>" +
" {{/link}}" +
" {{#toJson}}subgroups{{/toJson}}" +
" {{something-else}}" +
"{{/metadata}}\" }");
final TemplateRoleName templateRoleName = new TemplateRoleName(template, Format.STRING);
templateRoleName.validate(scriptService);
}

public void testValidateWillFailForSyntaxError() {
final ScriptService scriptService = new ScriptService(Settings.EMPTY,
Collections.singletonMap(MustacheScriptEngine.NAME, new MustacheScriptEngine()), ScriptModule.CORE_CONTEXTS);

final BytesReference template = new BytesArray("{ \"source\":\" {{#not-closed}} {{other-variable}} \" }");

final IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> new TemplateRoleName(template, Format.STRING).validate(scriptService));
assertTrue(e.getCause() instanceof ScriptException);
}

public void testValidationWillFailWhenInlineScriptIsNotEnabled() {
final Settings settings = Settings.builder().put("script.allowed_types", ScriptService.ALLOW_NONE).build();
final ScriptService scriptService = new ScriptService(settings,
Collections.singletonMap(MustacheScriptEngine.NAME, new MustacheScriptEngine()), ScriptModule.CORE_CONTEXTS);
final BytesReference inlineScript = new BytesArray("{ \"source\":\"\" }");
final IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> new TemplateRoleName(inlineScript, Format.STRING).validate(scriptService));
assertThat(e.getMessage(), containsString("[inline]"));
}

public void testValidateWillFailWhenStoredScriptIsNotEnabled() {
final Settings settings = Settings.builder().put("script.allowed_types", ScriptService.ALLOW_NONE).build();
final ScriptService scriptService = new ScriptService(settings,
Collections.singletonMap(MustacheScriptEngine.NAME, new MustacheScriptEngine()), ScriptModule.CORE_CONTEXTS);
final ClusterChangedEvent clusterChangedEvent = mock(ClusterChangedEvent.class);
final ClusterState clusterState = mock(ClusterState.class);
final MetaData metaData = mock(MetaData.class);
final StoredScriptSource storedScriptSource = mock(StoredScriptSource.class);
final ScriptMetaData scriptMetaData = new ScriptMetaData.Builder(null).storeScript("foo", storedScriptSource).build();
when(clusterChangedEvent.state()).thenReturn(clusterState);
when(clusterState.metaData()).thenReturn(metaData);
when(metaData.custom(ScriptMetaData.TYPE)).thenReturn(scriptMetaData);
when(storedScriptSource.getLang()).thenReturn("mustache");
when(storedScriptSource.getSource()).thenReturn("");
when(storedScriptSource.getOptions()).thenReturn(Collections.emptyMap());
scriptService.applyClusterState(clusterChangedEvent);

final BytesReference storedScript = new BytesArray("{ \"id\":\"foo\" }");
final IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> new TemplateRoleName(storedScript, Format.STRING).validate(scriptService));
assertThat(e.getMessage(), containsString("[stored]"));
}

public void testValidateWillFailWhenStoredScriptIsNotFound() {
final ScriptService scriptService = new ScriptService(Settings.EMPTY,
Collections.singletonMap(MustacheScriptEngine.NAME, new MustacheScriptEngine()), ScriptModule.CORE_CONTEXTS);
final ClusterChangedEvent clusterChangedEvent = mock(ClusterChangedEvent.class);
final ClusterState clusterState = mock(ClusterState.class);
final MetaData metaData = mock(MetaData.class);
final ScriptMetaData scriptMetaData = new ScriptMetaData.Builder(null).build();
when(clusterChangedEvent.state()).thenReturn(clusterState);
when(clusterState.metaData()).thenReturn(metaData);
when(metaData.custom(ScriptMetaData.TYPE)).thenReturn(scriptMetaData);
scriptService.applyClusterState(clusterChangedEvent);

final BytesReference storedScript = new BytesArray("{ \"id\":\"foo\" }");
final IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> new TemplateRoleName(storedScript, Format.STRING).validate(scriptService));
assertThat(e.getMessage(), containsString("unable to find script"));
}
}
5 changes: 5 additions & 0 deletions ...in/java/org/elasticsearch/xpack/security/authc/support/mapper/NativeRoleMappingStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.elasticsearch.xpack.core.security.action.rolemapping.DeleteRoleMappingRequest;
import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingRequest;
import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping;
import org.elasticsearch.xpack.core.security.authc.support.mapper.TemplateRoleName;
import org.elasticsearch.xpack.core.security.authc.support.mapper.expressiondsl.ExpressionModel;
import org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames;
import org.elasticsearch.xpack.core.security.authc.support.CachingRealm;
Expand Down Expand Up @@ -165,6 +166,10 @@ protected ExpressionRoleMapping buildMapping(String id, BytesReference source) {
* Stores (create or update) a single mapping in the index
*/
public void putRoleMapping(PutRoleMappingRequest request, ActionListener<Boolean> listener) {
// Validate all templates before storing the role mapping
for (TemplateRoleName templateRoleName : request.getRoleTemplates()) {
templateRoleName.validate(scriptService);
}
modifyMapping(request.getName(), this::innerPutMapping, request, listener);
}

Expand Down
15 changes: 15 additions & 0 deletions ...va/org/elasticsearch/xpack/security/authc/support/mapper/NativeRoleMappingStoreTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.xpack.core.security.action.realm.ClearRealmCacheAction;
import org.elasticsearch.xpack.core.security.action.realm.ClearRealmCacheRequest;
import org.elasticsearch.xpack.core.security.action.realm.ClearRealmCacheResponse;
import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingRequest;
import org.elasticsearch.xpack.core.security.authc.AuthenticationResult;
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
import org.elasticsearch.xpack.core.security.authc.RealmSettings;
Expand Down Expand Up @@ -210,6 +211,20 @@ public void testCacheIsNotClearedIfNoRealmsAreAttached() {
assertEquals(0, numInvalidation.get());
}

public void testPutRoleMappingWillValidateTemplateRoleNamesBeforeSave() {
final PutRoleMappingRequest putRoleMappingRequest = mock(PutRoleMappingRequest.class);
final TemplateRoleName templateRoleName = mock(TemplateRoleName.class);
final ScriptService scriptService = mock(ScriptService.class);
when(putRoleMappingRequest.getRoleTemplates()).thenReturn(Collections.singletonList(templateRoleName));
doAnswer(invocationOnMock -> {
throw new IllegalArgumentException();
}).when(templateRoleName).validate(scriptService);

final NativeRoleMappingStore nativeRoleMappingStore =
new NativeRoleMappingStore(Settings.EMPTY, mock(Client.class), mock(SecurityIndexManager.class), scriptService);
expectThrows(IllegalArgumentException.class, () -> nativeRoleMappingStore.putRoleMapping(putRoleMappingRequest, null));
}

private NativeRoleMappingStore buildRoleMappingStoreForInvalidationTesting(AtomicInteger invalidationCounter, boolean attachRealm) {
final Settings settings = Settings.builder().put("path.home", createTempDir()).build();

Expand Down