Pull cross-project authorization forward #136164
Conversation
slobodanadamovic added :Security/Authorization 
elasticsearchmachine added the serverless-linked 
| ProjectResolver projectResolver, | ||
| AuthorizedProjectsResolver authorizedProjectsResolver | ||
| AuthorizedProjectsResolver authorizedProjectsResolver, | ||
| CrossProjectModeDecider crossProjectModeDecider |
There was a problem hiding this comment.
Providing CrossProjectModeDecider via constructor to allow easier testing of project authorization code-paths.
in regards to the error handling
| * @param <Response> the type of the response | ||
| * @return a listener that executes the appropriate consumer when the response (or failure) is received. | ||
| */ | ||
| private static <Response> ActionListener<Response> actionListenerWithErrorHandling( |
There was a problem hiding this comment.
We need this to maintain the bwc error handling behaviour (e.g. returning 403 ElasticsearchSecurityException with IllegalStateException as a root cause).
It's debatable if the behaviour is correct or not in all cases, but I'd like to avoid opening that can of worms at this point. I'd prefer to keep the same error and status behaviour and not have to deal with potential breaking changes now.
There was a problem hiding this comment.
It is not immediately obvious to me where IllegalStateException is thrown? I guess it is likely from authzEngine.authorizeIndexAction? If that's the case, we can wrap it with ActionListener.run? That is, something like
ActionListener.run( listener, l -> authzEngine.authorizeIndexAction(requestInfo, authzInfo, resolvedIndicesAsyncSupplier, projectMetadata) .addListener( wrapPreservingContext( new AuthorizationResultListener<>( result -> handleIndexActionAuthorizationResult( result, requestInfo, requestId, authzInfo, authzEngine, resolvedIndicesAsyncSupplier, projectMetadata, l ), l::onFailure, requestInfo, requestId, authzInfo ), threadContext ) ) );and maybe with a comment explaining why wrapping is needed?
There was a problem hiding this comment.
Yes. The behaviour is that all unexpected exceptions thrown by authzEngine.authorizeIndexAction should not go through onAuthorizedResourceLoadFailure. They should be handled by original listener and result in 500 responses. On the other hand, the unexpected exception thrown by IndicesAndAliasesResolver do go through onAuthorizedResourceLoadFailure, and end up audited and wrapped in security exception. This is something I'd really like to fix but it's not straightforward since the exceptions thrown by resolver are not consistent. Also, I'd really like us to threat all unexpected exceptions consistently when security is enabled and only return a generic 500 message with unique identifier back to callers with logging an actual error (and cause) that includes unique identifier so it can be easily traced down.
There was a problem hiding this comment.
That is, something like
ActionListener.run( listener, l -> authzEngine.authorizeIndexAction(requestInfo, authzInfo, resolvedIndicesAsyncSupplier, projectMetadata) .addListener( wrapPreservingContext( new AuthorizationResultListener<>( result -> handleIndexActionAuthorizationResult( result, requestInfo, requestId, authzInfo, authzEngine, resolvedIndicesAsyncSupplier, projectMetadata, l ), l::onFailure, requestInfo, requestId, authzInfo ), threadContext ) ) );and maybe with a comment explaining why wrapping is needed?
TIL Nice! I was looking for a suitable wrapper method but did not find it.
…ect-authorization
| Pinging @elastic/es-security (Team:Security) |
| final var targetProjectListener = new SubscribableListener<TargetProjects>(); | ||
| if (indicesAndAliasesResolver.resolvesCrossProject(request)) { | ||
| authorizedProjectsResolver.resolveAuthorizedProjects(targetProjectListener); | ||
| } else { | ||
| targetProjectListener.onResponse(TargetProjects.NOT_CROSS_PROJECT); | ||
| } |
There was a problem hiding this comment.
Nit: a little more efficient
| final var targetProjectListener = new SubscribableListener<TargetProjects>(); | |
| if (indicesAndAliasesResolver.resolvesCrossProject(request)) { | |
| authorizedProjectsResolver.resolveAuthorizedProjects(targetProjectListener); | |
| } else { | |
| targetProjectListener.onResponse(TargetProjects.NOT_CROSS_PROJECT); | |
| } | |
| final SubscribableListener<TargetProjects> targetProjectListener; | |
| if (indicesAndAliasesResolver.resolvesCrossProject(request)) { | |
| targetProjectListener = new SubscribableListener<>(); | |
| authorizedProjectsResolver.resolveAuthorizedProjects(targetProjectListener); | |
| } else { | |
| targetProjectListener = SubscribableListener.newSucceeded(TargetProjects.NOT_CROSS_PROJECT); | |
| } |
| * @param <Response> the type of the response | ||
| * @return a listener that executes the appropriate consumer when the response (or failure) is received. | ||
| */ | ||
| private static <Response> ActionListener<Response> actionListenerWithErrorHandling( |
There was a problem hiding this comment.
It is not immediately obvious to me where IllegalStateException is thrown? I guess it is likely from authzEngine.authorizeIndexAction? If that's the case, we can wrap it with ActionListener.run? That is, something like
ActionListener.run( listener, l -> authzEngine.authorizeIndexAction(requestInfo, authzInfo, resolvedIndicesAsyncSupplier, projectMetadata) .addListener( wrapPreservingContext( new AuthorizationResultListener<>( result -> handleIndexActionAuthorizationResult( result, requestInfo, requestId, authzInfo, authzEngine, resolvedIndicesAsyncSupplier, projectMetadata, l ), l::onFailure, requestInfo, requestId, authzInfo ), threadContext ) ) );and maybe with a comment explaining why wrapping is needed?
slobodanadamovic added the auto-merge-without-approval 3 participants
Fixing the wrong order of listener calls.
Note: This fix is only relevant when project authorization is used and cross-project enabled.
Labeling as
>refactoringsince the feature is under development and not affecting production code.Kudos to @ywangd for tracking the issue down.