Check for ml privilege when using the Inference Aggregation #59530
Conversation
|
Pinging @elastic/ml-core (:ml) |
|
ping @szabosteve this change adds a security permission check to the inference pipeline. Where should this be documented? |
|
@davidkyle Thanks for pinging. I'll add this info to the inference pipeline docs and add you as a reviewer. |
| final String username = securityContext.getUser().principal(); | ||
| final HasPrivilegesRequest privRequest = new HasPrivilegesRequest(); | ||
| privRequest.username(username); | ||
| privRequest.clusterPrivileges("manage_ml", "monitor_ml"); |
There was a problem hiding this comment.
Does this stop really high privilege users from using inference? For example, the actions permitted by manage_ml are a subset of those permitted by manage (which means manage everything except security), but literally checking for manage_ml might not return true for a user who has manage. I may be wrong as I haven't tested it, but it seems likely that the security code will resolve high level cluster privilege names to sets of actions they allow, but not to other high level cluster privilege names.
You could check for just GetTrainedModelsAction.NAME here instead to solve that potential problem and also make the later code simpler as you'll only be testing for one privilege:
| privRequest.clusterPrivileges("manage_ml", "monitor_ml"); | |
| privRequest.clusterPrivileges(GetTrainedModelsAction.NAME); |
| "user [" + username + "] is not an ml user and does not have sufficient privilege " + | ||
| "to use ml inference")); |
There was a problem hiding this comment.
| "user [" + username + "] is not an ml user and does not have sufficient privilege " + | |
| "to use ml inference")); | |
| "user [" + username + "] does not have the privilege to get trained models " + | |
| "so cannot use ml inference")); |
| }); | ||
| return new InferencePipelineAggregationBuilder(name, bucketPathMap, loadedModel::get, modelId, inferenceConfig, licenseState); | ||
| } | ||
|
|
||
| static boolean hasMlPrivilege(HasPrivilegesResponse privilegesResponse) { |
There was a problem hiding this comment.
This method won't be needed if just checking one privilege.
| context.registerAsyncAction((client, listener) -> { | ||
| if (licenseState.isSecurityEnabled()) { | ||
| // check the user has ml privileges | ||
| SecurityContext securityContext =new SecurityContext(Settings.EMPTY, client.threadPool().getThreadContext()); |
There was a problem hiding this comment.
nit: space after equals
| SetOnce<LocalModel> loadedModel = new SetOnce<>(); | ||
| context.registerAsyncAction((client, listener) -> { | ||
| BiConsumer<Client, ActionListener<?>> modelLoadAction = (client, listener) -> |
There was a problem hiding this comment.
Ah, nice! That's what to use when in need to return more than a single thing. I'll use that onwards!
| final HasPrivilegesRequest privRequest = new HasPrivilegesRequest(); | ||
| privRequest.username(username); | ||
| privRequest.clusterPrivileges(GetTrainedModelsAction.NAME); | ||
| privRequest.indexPrivileges(new RoleDescriptor.IndicesPrivileges[]{}); |
There was a problem hiding this comment.
Do we need to set index and application privileges even though they're empty?
There was a problem hiding this comment.
| @SuppressWarnings("unchecked") | ||
| static boolean containsKey(String key, Map<String, Object> mapOfMaps) { | ||
| Set<String> keys = mapOfMaps.keySet(); | ||
| for (String keyEntry : keys) { |
There was a problem hiding this comment.
Isn't this the same as doing if (mapOfMaps.contains(key)) return true; ?
There was a problem hiding this comment.
Ha yeah that might be a simpler way of doing it
|
run elasticsearch-ci/packaging-sample-windows |
Before running the inference pipeline aggregation check the user has permission to access the Get trained models endpoint (
_ml/inference/)If the user does not have sufficient privilege a
security_exceptionis thrown with the messageuser [dave] is not an ml user and does not have sufficient privilege to use ml inference. Any suggestions for an alternative message are welcome.Non issue as the change comes under #58193