Skip to content
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

Add an option to disable logging for jobs with sensitive arguments #37660

Merged
merged 1 commit into from Nov 8, 2019

Conversation

@rafaelfranca
Copy link
Member

rafaelfranca commented Nov 7, 2019

class SensitiveJob < ApplicationJob
  self.log_arguments = false

  def perform(my_sensitive_argument)
  end
end

When dealing with sensitive arguments as password and tokens it is now possible to configure the job to not put the sensitive argument in the logs.

Closes #34438.

@rafaelfranca rafaelfranca requested review from georgeclaghorn and kaspth Nov 7, 2019
@rails-bot rails-bot bot added the activejob label Nov 7, 2019
@rafaelfranca rafaelfranca force-pushed the rm-add-way-to-disable-argument-logging-jobs branch 3 times, most recently from 25a034b to ab8eab1 Nov 7, 2019
@kaspth
kaspth approved these changes Nov 7, 2019
Copy link
Member

kaspth left a comment

I'm fine with it, but I thought we already had ported over the parameter filters to also be able to filter out sensitive Active Job args? Or maybe I'm thinking of another case where we ported them?

activejob/CHANGELOG.md Outdated Show resolved Hide resolved
activejob/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -77,6 +77,8 @@ def queue_name(event)
end

def args_info(job)
return "" if job.arguments_logging_disabled?

if job.arguments.any?

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 7, 2019

Member

What about flipping to job.log_arguments? and use it as if job.log_arguments? && job.arguments.any??

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Nov 7, 2019

Ahhh, it was in the issue you linked to 👍. I don't like that users would have to remember to disable logging for each new job that happens to have sensitive data in there, but I guess there's not much to do about that.

Unless we can somehow regex the password/token digest format and auto-disable logging in that case? Not sure if that's worth it.

@rafaelfranca rafaelfranca force-pushed the rm-add-way-to-disable-argument-logging-jobs branch from ab8eab1 to 8e8f735 Nov 8, 2019
@rails-bot rails-bot bot added the docs label Nov 8, 2019
@rafaelfranca

This comment has been minimized.

Copy link
Member Author

rafaelfranca commented Nov 8, 2019

Unless we can somehow regex the password/token digest format and auto-disable logging in that case? Not sure if that's worth it.

The problem is, what is a password format. I think we can be very effective on that path.

I don't like that users would have to remember to disable logging for each new job that happens to have sensitive data in there, but I guess there's not much to do about that.

I don't like that as well, but I changed the implementation a little bit, now we can for example default that to false, so users will have to enable logging.

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Nov 8, 2019

I don't like that as well, but I changed the implementation a little bit, now we can for example default that to false, so users will have to enable logging.

Hm, yeah, that's not super great either. But it's better to have the option for this case.

    class SensitiveJob < ApplicationJob
      self.log_arguments = false

      def perform(my_sensitive_argument)
      end
    end

When dealing with sensitive arugments as password and tokens it is
now possible to configure the job to not put the sensitive argument
in the logs.

Closes #34438.
@rafaelfranca rafaelfranca force-pushed the rm-add-way-to-disable-argument-logging-jobs branch from 8e8f735 to ce085f6 Nov 8, 2019
@rafaelfranca rafaelfranca merged commit 986d3bf into master Nov 8, 2019
2 of 3 checks passed
2 of 3 checks passed
build
Details
build
Details
buildkite/rails Build #64827 failed (43 minutes, 22 seconds)
Details
@rafaelfranca rafaelfranca deleted the rm-add-way-to-disable-argument-logging-jobs branch Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.