feat: support user identity forwarding to tasks via TaskAuthContext#19236
feat: support user identity forwarding to tasks via TaskAuthContext#19236jtuglu1 wants to merge 1 commit intoapache:masterfrom
Conversation
f68a5ba to
3255f0b
Compare
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java
Dismissed
Show dismissed
Hide dismissed
...ib/druid-iceberg-extensions/src/main/java/org/apache/druid/iceberg/input/IcebergCatalog.java
Fixed
Show fixed
Hide fixed
...ruid-iceberg-extensions/src/main/java/org/apache/druid/iceberg/input/IcebergInputSource.java
Fixed
Show fixed
Hide fixed
f2012ec to
99c006c
Compare
| * @return TaskAuthContext to inject into the task, or null to skip injection | ||
| */ | ||
| @Nullable | ||
| TaskAuthContext createTaskAuthContext(AuthenticationResult authenticationResult, Task task); |
Check notice
Code scanning / CodeQL
Useless parameter Note
| * @return TaskAuthContext to inject into the task, or null to skip injection | ||
| */ | ||
| @Nullable | ||
| TaskAuthContext createTaskAuthContext(AuthenticationResult authenticationResult, Task task); |
Check notice
Code scanning / CodeQL
Useless parameter Note
...e/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceAuthContextTest.java
Fixed
Show fixed
Hide fixed
1f4e9ff to
09f57a7
Compare
33be042 to
81fae04
Compare
81fae04 to
6ac3139
Compare
|
@jtuglu1 whats the status for this PR? should we move this to 38? |
This one I wanted to get into v37 – I was hoping to get this merged today. |
6ac3139 to
74d8369
Compare
gianm
left a comment
There was a problem hiding this comment.
I reviewed the core changes only, focusing on trying to understand the security properties of the changes.
I wonder what your thoughts are on an alternate design that keeps the vended credentials inside the input source itself:
- Add a new method to
InputSourcelikescopeForUser(AuthenticationResult authResult). Default implementation isreturn this - Whenever a task is submitted, call
scopeForUseron all of its input sources at whichever service initially accepts the task (either Broker [for SQL DML] or Overlord [for anything else]). - The
IcebergInputSourcewould implementscopeForUserto fetch the vended credentials and transform itself into an input source that bakes in the vended credentials. It would use aPasswordProviderso it is redactable.
The idea would be to avoid the need for a new credential vending system in core, putting most of the changes inside the Iceberg extension instead.
|
|
||
| /** | ||
| * Returns sensitive credentials (e.g., OAuth tokens, API keys). | ||
| * This method MUST be redacted during serialization via {@link TaskAuthContextRedactionMixIn}. |
There was a problem hiding this comment.
This is over-eager, isn't it? The credentials must be serialized in some cases. When we send a task spec from Overlord to the server that will actually run the task, the credentials must be included. In some cases the nonredacted task file will need to end up on disk, so it can be run. Perhaps this should say "MUST be redacted when written to log files, the metadata store, or returned from user-facing APIs".
There was a problem hiding this comment.
Yeah the comment here is a bit aggressive, but that's currently what is already done. The field is not redacted when being submitted from Overlord to MM/Peon/Indexer. It is only redacted currently on logging, persistence (to disk or db).
| * | ||
| * <p><b>Credential lifetime:</b> Vended credentials (OAuth tokens, STS session tokens) have | ||
| * limited lifetimes, typically 1-12 hours. There is currently no credential refresh mechanism, | ||
| * so long-running tasks may fail when credentials expire. Task restarts are also not supported |
There was a problem hiding this comment.
Any thoughts on how to handle long-running tasks? I expect it will be a problem. It's not uncommon for tasks to take hours.
There was a problem hiding this comment.
I was going to add this in a separate change. Using a task auth context, inputSource could conceivably just use that with some implementable revend() method. The only issue I would want to avoid is having the subtasks refresh the credentials (and not the driver task), since this might cause thundering herd effect where many subtasks all attempt to refresh at once. Spark does a better job of this by letting executor handle credential refresh on the driver, then propagate that to the executors.
| * | ||
| * @return the identity string | ||
| */ | ||
| String getIdentity(); |
There was a problem hiding this comment.
The auth context is a @JsonProperty so I believe that means people can set it explicitly when they submit tasks. Does anything bad happen if someone sets a context and sets the identity to someone else? What do you think about clearing the user-provided auth context, if any, in OverlordResource?
There was a problem hiding this comment.
Does anything bad happen if someone sets a context and sets the identity to someone else?
If we want to scope the auth context to Druid-settable only, then yes we should. IMO, I think that's a safer option but I could see people wanting to use this apart from Authorization result.
|
|
||
| FutureUtils.getUnchecked(overlordClient.runTask(taskId, controllerTask), true); | ||
| // Propagate auth context headers to Overlord for consumption | ||
| if (plannerContext.getAuthenticationResult() != null && plannerContext.getAuthenticationResult().getContext() != null) { |
There was a problem hiding this comment.
What is the purpose of this? Authentication context is meant to be an arbitrary extension-defined in-memory map. It may not in general take well to being stuffed into a header. It may also include sensitive information that shouldn't be sent in a header.
There was a problem hiding this comment.
We need a way to propagate AuthenticationResult from the broker to the overlord. I could make an overridable serialization method/class for AuthenticationResult to ensure we only pass-through what's needed (and let the user specify that), but propagating the headers here was the least intrusive approach for that change. We still need to pass the authentication result somehow.
|
|
||
| // Inject auth context if provider is configured | ||
| if (taskAuthContextProvider != null) { | ||
| final AuthenticationResult authenticationResult = AuthorizationUtils.authenticationResultFromRequest(req); |
There was a problem hiding this comment.
How will this work in the SQL DML path, where the user submits a task to /druid/v2/sql/task/ and the Broker then submits the task using its own credentials? The current design is that the Broker authenticates the user, authorizes the DML operation, and then submits it to the Overlord using a service account (not the user's own credentials).
IMO it would make more sense in this case for the Broker to get the vended credentials and pass them along to the Overlord.
There was a problem hiding this comment.
How will this work in the SQL DML path, where the user submits a task to /druid/v2/sql/task/ and the Broker then submits the task using its own credentials?
That's the thing. In our implementation, the broker passes through the user auth context and not its own credentials, so there's no use of the Broker credentials in the task payload (only used for validating the request came from the Brokers).
| */ | ||
| @ExtensionPoint | ||
| @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type") | ||
| public interface TaskAuthContext |
There was a problem hiding this comment.
I didn't see an implementation of TaskAuthContext or TaskAuthContextProvider in this PR. Are they meant to be added later? What would they look like? I was wondering in particular what taskAuthContextProvider.createTaskAuthContext would do exactly with the authenticationResult that is passed in.
There was a problem hiding this comment.
They are meant for users to implement. I have internal versions of these classes which take our internal identity tokens and propagate them through to the Iceberg input source, to be then used for vending S3 credentials to read data from Iceberg.
Are you proposing pushing the vending of credentials using an identity to the broker/overlord prior to task submission? I'd ideally like to propagate the auth context to the task and have it vend the credentials at runtime, not at submit time. Currently, the way this works is:
|
Closes #18957.
Description
Create a mechanism to propagate user identity context from authenticator to tasks. This allows tasks to safely operate with a sort of per-task identity, allowing tasks to access information dynamically that would be hard to setup in the current state of things. This allows for auth mechanisms like catalog credential vending (through Iceberg Catalog, etc.) and allows Druid to operate more like a standalone engine that can integrate with other things within a larger data ecosystem.
Design
Constraints
This currently does NOT support task restarts.
Release note
Create a mechanism to propagate user identity context from authenticator to tasks to support things like credential vending, etc.
This PR has: