Add extension point for file-based role validation #107177
Conversation
This commit adds an extension point for Security extensions to add a file-based role validator, which will be used to apply rules to the roles provided in `roles.yml`. There is no change to the default behavior of Elasticsearch.
gwbrown
added
>non-issue
:Security/Authorization
|
Pinging @elastic/es-security (Team:Security) |
|
@n1v0lg I came around to your way of thinking and didn't change the plumbing we discussed around the file roles store and name checker - it turned out much less awkward to break this out in a different way than I had when we last spoke. |
There was a problem hiding this comment.
LGTM! One clarification about the PR description just to make sure I understand the intended behavior correctly:
If this validation fails, the system will treat it like the YAML failed to parse: if this happens at startup, the node will fail to start, while if the roles.yml file is changed at runtime and fails validation, the failure will be logged and the active roles not updated.
Before this change when the YAML fails to parse, we simply skip defining roles from file, instead of the node failing to start.
With this PR, that doesn't change. We only fail if the new validator returns an exception, during node start. I think that's indeed the behavior we want but just want to double-check with you since the PR description above makes it sound like parsing failures also result in node start failure (and I don't think that's true).
| @@ -374,6 +382,10 @@ private static RoleDescriptor checkDescriptor( | |||
| } | |||
| } | |||
| } | |||
| ActionRequestValidationException ex = roleValidator.validatePredefinedRole(descriptor); | |||
There was a problem hiding this comment.
Might add a test to FileRoleStoreTests with a mocked validator that throws
| * Provides a check which will be applied to roles in the file-based roles store. | ||
| */ | ||
| @FunctionalInterface | ||
| public interface FileRoleValidator { |
There was a problem hiding this comment.
Nice, I think this came out quite clean!
| ) { | ||
| return parseFile(path, logger, true, settings, licenseState, xContentRegistry); | ||
| // EMPTY & default role validator are safe here because we never use namedObject as we are just parsing role names | ||
| return parseRoleDescriptors(path, logger, false, Settings.EMPTY, new FileRoleValidator.Default(), NamedXContentRegistry.EMPTY) |
There was a problem hiding this comment.
Optional nit: parseRoleDescriptors with the above invocation is only used in one place now, so might as well inline it.
|
@n1v0lg I dug into the behavior a bit and you're right, most other times we only skip parsing the role that's broken, unless the file is severely mashed. I've updated the PR description to account for that, as I agree that the current behavior of this PR is what we want. Good catch, thanks! |
…idation-extension
…idation-extension
…idation-extension
|
Opened #107387 for the failure - it's been popping up in multiple PRs. |
|
@elasticmachine update branch |
|
Failure is #105331. Someday, CI will pass, and on that day, I will leap with joy and shout from the rooftops, lo, CI has passed. |
|
@elasticmachine test this please |
This commit adds an extension point for Security extensions to add a file-based role validator, which will be used to apply rules to the roles provided in
roles.yml. If this validation fails, the system will treat it like the roles YAML file could not be read: if this happens at startup, the node will fail to start, while if theroles.ymlfile is changed at runtime and fails validation, the failure will be logged and the active roles not updated. There is no change to the default behavior of Elasticsearch.