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

Mask sensitive data in logs with the MessageAccessor #35

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

flexponsive
Copy link

@flexponsive flexponsive commented Oct 2, 2024

Description

This PR improves the handling of sensitive data, like passwords and auth tokens, by making it possible to easily configure the replacing of sensitive values, headers etc. in the package configuration. It does this by integrating the existing MessageAccessor (#13) with the `PsrMessageToStringConverter``

The configuration file template now includes detailed documentation on replacement options available for sensitive data, including:

  • Replace anywhere in the message
  • Replace values
  • Replace headers
  • Replace JSON path

Backward Compatibility: By default, this update does not alter the package’s behavior, ensuring compatibility for existing users. However, since logging sensitive data is such a widespread problem, it may be worthwhile to consider in a future release to mask the Authentication and Authorization headers by default.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update (documentation change included)

Checklist

  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@flexponsive flexponsive marked this pull request as ready for review October 2, 2024 13:00
…w exception if this is attempt; docs + test improvements
@bilfeldt
Copy link
Owner

bilfeldt commented Oct 28, 2024

@flexponsive thanks so much for this PR. This is definitely a nice feature.

This is a breaking change because of the below, but that should not stop us from implementing it:

  1. The parameters jsonFilers (misspelled) and replace are renamed in MessageAccessor
  2. The constructor of MessageAccessor is changed

The current implementation makes a number of change to the MessageAccessor class. I am wondering if instead we should add an array of middleware to HttpLogger. Each of these middleware can modify the request/response before it gets logged and we can then push a default Obfuscator class to this stack which will do the filtering?

@flexponsive
Copy link
Author

Thank you for reviewing this PR and creating such a useful package!

One of the great strengths of laravel-http-client-logger is its flexibility through ad-hoc configuration. This is especially valuable in larger applications with multiple APIs and log channels.

I appreciate the suggestion to add middleware support for modifying requests/responses before logging. However, I have concerns that this added complexity could detract from the package's simplicity. That said, I recognize the original PR had a limitation: ad-hoc configuration of MessageAccessor wasn't possible.

To address this, I made two key changes:

  1. Support for Ad-Hoc Configuration: Updated the implementation to allow MessageAccessor parameters to be set within ad-hoc configurations, enabling more precise data masking based on each scenario.

  2. Custom Message Accessor Support: Added a configuration parameter for custom MessageAccessor implementations. This provides flexibility for specialized use cases (e.g., XML-based APIs) while keeping the default configuration simple.

This approach provides full customization where needed while maintaining simplicity for most use cases. What do you think about this direction?

@bilfeldt
Copy link
Owner

bilfeldt commented Nov 3, 2024

I have been thinking about this for some time, because this is a very nice addition.

If we go for a middleware driven approach, then it could look like this:

Dedicated masking middleware

Http::log()
  ->pushLogMiddleware(
      new SentryMasker
  )
   ->post(...);

where the SentryMasker looks something like this:

class SentryMasker implements LogMiddlewareInterface
{
    /**
     * Mask sensitive data in the request and response.
     *
     * @param RequestInterface $request
     * @param ResponseInterface $response
     * @return array{0: RequestInterface, 1: ResponseInterface}
     */
    public function handle(
        RequestInterface $request,
        ResponseInterface $response
    ): array
    {
        return [
            PsrRequestProcessor::replaceHeader($request, 'Authorization', '***'),
            PsrResponseProcessor::replaceBody($response, '3566002020360505', '************0505'),
        ];
    }
}

Config parameter

If config is passed to the log function like so:

Http::log(config: [
       'replace' => ['3566002020360505' => '************0505'],
       'replace_headers' => ['Authorization'],
   ])
   ->post(...);

then all that we do it that we push a new middleware MaskMiddleware to the middleware stack:

class SentryMasker implements LogMiddlewareInterface
{
    public function __construct(
        private array $replace = [],
        private array $replaceHeaders = [],
    ) {}

    ....
}

I think the above implementation is more flexible and can be implemented without breaking backwards compatibility because this is simply a matter of implementing a middleware stack.

Let me know your thoughts @flexponsive ?

@flexponsive
Copy link
Author

Thank you, @bilfeldt, for the detailed feedback and for expanding on the middleware concept—this certainly adds a lot of flexibility!

One way forward would be to implement a DefaultLogMasker which implements the LogMiddlewareInterface and uses the MessageAccessor to do the actual masking. This default masker could then be pushed onto the middleware stack with $config, allowing global masking settings, such as removing the Authorization header. If no replace settings are configured, it would act as a no-op.

Another potential enhancement made possible by the middleware approach could be setting the log level based on conditions. For instance, some APIs respond with a 200 status but include an error field in the JSON payload. By slightly adjusting the middleware interface, middlewares could enable custom log level settings in cases like these if you think it would be beneficial.

Let me know what you think!

@bilfeldt
Copy link
Owner

bilfeldt commented Nov 6, 2024

One way forward would be to implement a DefaultLogMasker which implements the LogMiddlewareInterface and uses the MessageAccessor to do the actual masking. This default masker could then be pushed onto the middleware stack with $config, allowing global masking settings, such as removing the Authorization header. If no replace settings are configured, it would act as a no-op.

Exactly.

Http::log() // The DefaultLogMasker is pushed to the stack with the global masking settings
   ->post(...);

Http::log(config: [
   'replace' => ['3566002020360505' => '************0505'],
   'replace_headers' => ['Authorization'],
]) // The DefaultLogMasker is pushed to the stack with a merger of the global settings and those provided above
   ->post(...);

Another potential enhancement made possible by the middleware approach could be setting the log level based on conditions. For instance, some APIs respond with a 200 status but include an error field in the JSON payload. By slightly adjusting the middleware interface, middlewares could enable custom log level settings in cases like these if you think it would be beneficial.

I get that some APIs do not use proper HTTP status codes like your example. But I am not sure that I completely understand how you intend to use middleware to do this?

I would expect one to do:

Http::log(filter: new SentryFilter)->post(...)

but now that I think about it, it would be cool to offer a syntax like:

Http::logWhen(fn ($response, $request) => $response->json('error'))->post(...)

But that is an entirely new PR :)

PsrMessages or Illuminate\Http\Client\Request

Note that there is a good chance that I will convert this package to use the "new" ResponseReceived event (see #2 (comment)) so it would probably make more sense that the middleware build on the Illuminate\Http\Client\Request and Illuminate\Http\Client\Response classes instead of the Psr counterparts for full flexibility and access to the nice methods like ->json('field').

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants