-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Redact passwords from tasks fetched from the TaskQueue #16182
Redact passwords from tasks fetched from the TaskQueue #16182
Conversation
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java
Outdated
Show resolved
Hide resolved
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.
Thanks for the fix.
I wonder if it would be better to just do the redaction while putting an item in the tasks
map. That way the serde would have to be done only once.
Edit: On second thought, doing the redaction while putting in the map is not a viable option as this same Task
instance is sent across the wire to MMs/indexers.
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java
Outdated
Show resolved
Hide resolved
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java
Outdated
Show resolved
Hide resolved
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java
Outdated
Show resolved
Hide resolved
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java
Outdated
Show resolved
Hide resolved
@@ -122,6 +121,7 @@ public class TaskQueue | |||
private final TaskActionClientFactory taskActionClientFactory; | |||
private final TaskLockbox taskLockbox; | |||
private final ServiceEmitter emitter; | |||
private final ObjectMapper PASSWORD_REDACTING_MAPPER; |
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.
Rename since it is not a constant anymore.
private final ObjectMapper PASSWORD_REDACTING_MAPPER; | |
private final ObjectMapper passwordRedactingMapper; |
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
throw DruidException.forPersona(DruidException.Persona.USER) | ||
.ofCategory(DruidException.Category.RUNTIME_FAILURE) | ||
.build(e, "Failed to serialize or deserialize task."); |
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 this exception should be targeted either at developer or operator, not the user persona.
throw DruidException.forPersona(DruidException.Persona.USER) | |
.ofCategory(DruidException.Category.RUNTIME_FAILURE) | |
.build(e, "Failed to serialize or deserialize task."); | |
throw DruidException.forPersona(DruidException.Persona.OPERATOR) | |
.ofCategory(DruidException.Category.RUNTIME_FAILURE) | |
.build(e, "Failed to serialize or deserialize task[%s].", task.getId()); |
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
@@ -114,7 +114,7 @@ public Optional<Task> getTask(final String taskid) | |||
return activeTask; | |||
} | |||
} | |||
catch (JsonProcessingException e) { | |||
catch (DruidException e) { |
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.
Why are we swallowing this exception?
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 thought it might be helpful to fallback to the metadata store (TaskStorage) in such a case
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.
We have to do one of two things:
Either (1) Throw JsonProcessingException
from TaskQueue.getActiveTask()
, catch it here, log it and fallback to using TaskStorage
(catch block should have a comment to this effect). In this case, TaskQueue.getActiveTask
need not log the exception.
Or (2) Throw DruidException
from TaskQueue.getActiveTask()
and don't catch it anywhere.
I would prefer the latter as exception during serde shouldn't happen, and it is best to notify the operator about it as soon as possible.
Also if there is an issue deserializing the task, the task storage would be likely to encounter the same issue as it is the same mapper and the same Task object.
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
taskQueue.start(); | ||
taskQueue.add(taskWithPassword); | ||
|
||
final Optional<Task> taskFromTaskStorage = taskStorage.getTask(taskWithPassword.getId()); |
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.
final Optional<Task> taskFromTaskStorage = taskStorage.getTask(taskWithPassword.getId()); | |
final Optional<Task> taskInStorage = taskStorage.getTask(taskWithPassword.getId()); |
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
Assert.assertFalse(mapper.writeValueAsString(taskFromTaskStorage.get()).contains(password)); | ||
|
||
|
||
final Optional<Task> taskFromTaskQueue = taskQueue.getActiveTask(taskWithPassword.getId()); |
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.
final Optional<Task> taskFromTaskQueue = taskQueue.getActiveTask(taskWithPassword.getId()); | |
final Optional<Task> taskInQueue = taskQueue.getActiveTask(taskWithPassword.getId()); |
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
|
||
final Optional<Task> taskFromTaskQueue = taskQueue.getActiveTask(taskWithPassword.getId()); | ||
Assert.assertTrue(taskFromTaskQueue.isPresent()); | ||
Assert.assertNotNull(taskFromTaskQueue.get()); |
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.
Assign the result of optional.get()
to a new variable and then do the assertions.
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
indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskQueueTest.java
Show resolved
Hide resolved
indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskQueueTest.java
Outdated
Show resolved
Hide resolved
@@ -152,6 +157,7 @@ public class TaskQueue | |||
private final AtomicInteger statusUpdatesInQueue = new AtomicInteger(); | |||
private final AtomicInteger handledStatusUpdates = new AtomicInteger(); | |||
|
|||
|
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.
Nit: extra line?
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.
Removed
Fixes #15882
Description
Active tasks fetched from TaskQueue's in memory state may reveal sensitive information.
This doesn't happen for payloads fetched from the metadata store, as they are read using a json mapper with an added mixin for redacting credentials.
This PR leans on a similar change in the TaskQueue where a payload is serialized and then deserialized.
Release note
Key changed/added classes in this PR
TaskQueue
This PR has: