-
-
Notifications
You must be signed in to change notification settings - Fork 21
data protection #615
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
base: main
Are you sure you want to change the base?
data protection #615
Conversation
packages/DataProtection/src/OutboundDecryptionChannelInterceptor.php
Outdated
Show resolved
Hide resolved
packages/DataProtection/src/Configuration/DataProtectionModule.php
Outdated
Show resolved
Hide resolved
| ); | ||
| $messagingConfiguration->registerChannelInterceptor( | ||
| new OutboundDecryptionChannelBuilder($pollableMessageChannel->getMessageChannelName()) | ||
| ); |
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.
What is the reason to do the hooking via Channel Interceptors?
As this approach will have implications:
-
So far we only deserialize the message payload when it's actually being used (not after it's fetched from channel). This is to ensure higher performance.
As some Event Handler could for example use only Headers, so there is no need to deserialize payload. The other case is that outbox may be pushing message from db channel to rabbit channel. In this scenario there is no need to deserialize message when it's fetched, only to serialize it again to push it to other channel -
With current implementation, we do inside round trips, meaning:
- we get serialized message before it's pushed to channel,
- deserialize it to encrypt,
- serialize again with encryption encrypted.
- How you give a thought about enriching our current internal Converter/Conversion mechanism? As this would work earlier the in flow, so potentially it should have above drawbacks, and would work naturally with different media types.
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.
@dgafka I've been struggling with using Conversion mechanism for that. There are few problems with that approach that I don't see how to resolve.
- Encryption require source to be
string. - Converters do not chain nor can't be wrapped. First applicable converter will be used.
To use Conversion mechanism it will require a lot of ground refactoring and even then I expect it will be heavy and complicated as sensitive properties can have custom converters already which do not have to convert to/from string. Hooking into Channel Interceptors is easier as message is already serialized into expected media type.
Also, behavior for encryption depends more on endpoint itself rather than message as for some endpoints (as You've already noticed) may use only headers I see it possible to define endpoint as sensitive which with introduced approach allow for following definitions:
#[CommandHandler|EventHandler]
#[UsingSensitiveData(encryptionKeyName: 'gcp-key')]
public function doStuff(#[Payload] Message $message): void
{
// ...
}#[CommandHandler|EventHandler]
#[UsingSensitiveData]
#[WithSensitiveHeaders(headers: ['foo', 'bar'])]
public function doStuff(#[Header('foo')] string $foo): void
{
// ...
}#[CommandHandler|EventHandler]
#[UsingSensitiveData]
#[WithSensitiveHeader(header: 'foo')]
public function doStuff(#[Header('foo')] string $foo): void
{
// ...
}Again, when hooking into channel interceptor it is possible to configure it based on endpoint definition and to see if payload actually needs to be encrypted. I think when using converters there is no way to do that. Or at least it is not easy which will only complicate whole implementation.
732ba0d to
ff23dab
Compare
- use `Ecotone\DataProtection\Attribute\UsingSensitiveData` to define messages with sensitive data - use `Ecotone\DataProtection\Attribute\Sensitive` to mark properties with sensitive data - define encryption keys with `Ecotone\DataProtection\Configuration\DataProtectionConfiguration` - sensitive data will be encrypted right before its sended to queue and decrypted right after message is being retrieved from queue - data protection require JMSModule to be enabled
- allow to define sensitive headers
- message - endpoint - channel
7460dca to
dac7750
Compare
dgafka
left a comment
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.
Just some more coverage comments and potential API improvements.
I do think we can start with this and iterate over that (e.g. discuss how we want to handle Event Streams) :)
| use Ecotone\Messaging\Support\Assert; | ||
|
|
||
| #[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_CLASS)] | ||
| class WithSensitiveHeaders |
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.
WithSensitiveHeader can be used multiple times, so we can drop this one?
That would also follow DX of other features like being able to put multiple CommandHandler attribute
| ->withKey('secondary', $this->secondaryKey), | ||
| SimpleMessageChannelBuilder::create('test', $messageChannel), | ||
| JMSConverterConfiguration::createWithDefaults()->withDefaultEnumSupport(true), | ||
| ]) |
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.
can we add test case that using data protection without licence key fails?
| SimpleMessageChannelBuilder::create('test', $messageChannel), | ||
| JMSConverterConfiguration::createWithDefaults()->withDefaultEnumSupport(true), | ||
| ]) | ||
| ); |
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.
Can we add test case where header is set to be sensitive, yet it's missing in message (I guess we should simply skip)
| self::assertEquals($metadataSent['bar'], Crypto::decrypt(base64_decode($messageHeaders->get('bar')), $this->primaryKey)); | ||
| self::assertEquals($metadataSent['baz'], $messageHeaders->get('baz')); | ||
| } | ||
|
|
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.
Can we add test case that it fails on setting up channel protection on non pollable channel?
|
|
||
| foreach ($this->sortedChannelInterceptors as $channelInterceptor) { | ||
| $channelInterceptor->postReceive($message, $this->messageChannel); | ||
| $message = $channelInterceptor->postReceive($message, $this->messageChannel); |
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.
Can we add single test case for such interceptor modifying result in main Ecotone package?
Just to be aware if we break this functionality without the need to run whole test suite.
| self::assertEquals($metadataSent['bar'], Crypto::decrypt(base64_decode($messageHeaders->get('bar')), $this->primaryKey)); | ||
| self::assertEquals($metadataSent['baz'], $messageHeaders->get('baz')); | ||
| } | ||
|
|
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.
Can we add a test case where we encrypt only headers without payload?
| $routingSlip = $message->getHeaders()->get(MessageHeaders::ROUTING_SLIP); | ||
|
|
||
| foreach ($this->messageObfuscators as $routing => $obfuscator) { | ||
| if (str_starts_with($routingSlip, $routing)) { |
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.
That may fail if we have something like 'stocks', 'stocks_public'.
There should be method hasPrecedingRoutingSlip or something like in MessageHeaders to compare equally (if not we can add)
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.
But we are targetting current channel, so maybe we can just inject channel name here and compare instead?
|
|
||
| if ($routingObfuscator = $this->findRoutingObfuscator($message)) { | ||
| return $routingObfuscator->encrypt($message); | ||
| } |
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.
Can we have test where we define sensitive data on channel and message to define the expected behaviour?
I would suspect we either throw exception, or we put precedence on message configuration
| #[UsingSensitiveData] | ||
| #[WithSensitiveHeaders(['foo', 'bar'])] | ||
| #[WithSensitiveHeader('fos')] | ||
| public function handleFullyObfuscatedMessage( |
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.
a) Can you explain the use case we are trying to solve with encryption on the level of command handler, if we have this on level of Message?
b) If Message would come from different Service and our use case would be to decrypt headers, then we could make the api more explicit here
#[Sensitive] #[Header('foo')] $someThere 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.
Not a blocker for this PR, we can follow up with potential changes with next PRs
| use Attribute; | ||
|
|
||
| #[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_CLASS)] | ||
| class UsingSensitiveData |
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.
| class UsingSensitiveData | |
| class UsingSensitivePayload |
We define Header, and in documentation we do mention that Message contains of Headers and Payload, therefore that would be in relation to our domain language
Why is this change proposed?
Sensitive data should be obfuscated when leaving application via transport channels.
Description of Changes
Ecotone\DataProtection\Attribute\UsingSensitiveDatato define messages with sensitive datamessage,endpoint, or globally forchannelEcotone\DataProtection\Configuration\DataProtectionConfigurationPull Request Contribution Terms