-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Add New Security Script Settings #24637
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
Conversation
@elasticmachine please test this |
@rjernst If you're feeling up to it, I would appreciate a review on this :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I left a few minor comments.
|
||
/** | ||
* @return <tt>true</tt> if the provided {@link ScriptContext} is supported, <tt>false</tt> otherwise | ||
*/ | ||
boolean isSupportedContext(ScriptContext scriptContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the callers of isSupportedContext to call getKey() so there is only one variant of this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. There were only two callers and one was a test.
HashMap<String, Boolean> scriptModes = new HashMap<>(); | ||
for (Setting<Boolean> scriptModeSetting : scriptSettings.getScriptLanguageSettings()) { | ||
scriptModes.put(scriptModeSetting.getKey(), scriptModeSetting.get(settings)); | ||
} | ||
this.scriptEnabled = Collections.unmodifiableMap(scriptModes); | ||
|
||
typesAllowed = TYPES_ALLOWED_SETTING.exists(settings) ? new HashSet<>() : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the future we can simplify this a bit, so the Setting value is not a List, but that can be in future work as I know this class will probably go away in master with context work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree completely on this.
[float] | ||
=== Allowed script types setting | ||
|
||
By default all script types are allowed to be executed. This can modified using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can -> can be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
=== Allowed script types setting | ||
|
||
By default all script types are allowed to be executed. This can modified using the | ||
setting `script.allowed_types`. Should this setting be used, only the types specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove the "Should this setting be used" part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@rjernst Thanks for the review! As soon as the CI build passes I will commit. |
Settings are simplified to allowed_types and allowed_contexts. If a setting is not specified the default is to enable all for that setting.
Adds allowed_types and allowed_contexts as new security settings for scripts described in detail in the issue #24532.
Closes #24532