-
-
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
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
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) :)
packages/DataProtection/tests/Integration/ObfuscateAnnotatedMessagesTest.php
Outdated
Show resolved
Hide resolved
packages/DataProtection/tests/Integration/ObfuscateAnnotatedMessagesTest.php
Outdated
Show resolved
Hide resolved
packages/Ecotone/src/Messaging/Channel/PollableChannelInterceptorAdapter.php
Show resolved
Hide resolved
packages/DataProtection/src/OutboundEncryptionChannelInterceptor.php
Outdated
Show resolved
Hide resolved
packages/DataProtection/src/OutboundEncryptionChannelInterceptor.php
Outdated
Show resolved
Hide resolved
packages/DataProtection/tests/Fixture/ObfuscateAnnotatedEndpoints/TestCommandHandler.php
Outdated
Show resolved
Hide resolved
- 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
0e82dbf to
167e1d8
Compare
- solution provides obfuscation either for payload: either via Domain Message or entire channel - annotated Message will have precedence over channel configuration - obfuscating headers will be additional for message or default for channel as headers are not derivative from domain messages - annotating single endpoint, Ecotone will try to configure obfuscator for Message based on Payload
167e1d8 to
647a732
Compare
check license ensure data protection is applied on pollable channels ensure ChannelInterceptor::postReceive() modifies message when defined
add DataProtection test suite to phpunit.xml.dist file
|
|
||
| use Attribute; | ||
|
|
||
| #[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_CLASS | Attribute::IS_REPEATABLE)] |
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.
| #[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_CLASS | Attribute::IS_REPEATABLE)] | |
| Attribute::TARGET_CLASS | Attribute::IS_REPEATABLE)] |
same suggestion as for Sensitive, to keep it on the param level. There is also no point in decoding metadata, if we are not using that in param, which with current design would be possible.
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 would keep this one as it is. just because header is not used in method parameters it can still be sent and if considered as sensitive, it should be obfuscated in queue.
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.
Hmm, I think we should define the use case for those, because now they feel lacking intention, therefore being too generic,
- If someone want's full encryption / decryption, then I would consider him going with attribute on top of class. (inbound/outbound scenarios)
- And if someone would only handle decryption - Message coming from other system, Service Bus for example (Inbound scenarios)
If that's for inbound only, then it make to sense to keep it only for param, as we don't really care about the other ones.
wdyt?
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 think right now it makes no sense to differentiate whether message would come from other system or it was sent by the same application.
Whether this attribute should be omitted for TARGET_METHOD is a case for combining configuration from all sources - message, channel and endpoint - which would be trickier to understand why something was encrypted or not.
Right now, if Message is annotated, any other configuration will be skipped as Message will always have a priority - I'm thinking about future converter integration.
Lets keep it as it is. We can check how it is mostly used and adapt.
| use Ecotone\Messaging\Support\Assert; | ||
| use Ecotone\Messaging\Support\MessageBuilder; | ||
|
|
||
| readonly class Obfuscator |
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 are using different naming: Obfuscator, Sensitive, Data Protection.
Let's align it to Sensitive + Data Protection, dropping Obfuscator from Docs and Code?
| readonly class Obfuscator | |
| readonly class DataProtector |
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 obfuscation is one of the methods data can be protected. I remember we've been taking also about forgetable payloads. DataProtector sounds too general and doesn't describe what is actually done.
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.
Hmm, I see. It feel to me that Obfuscator is naming that confuses, we are stating here about data protection using encryption and decryption. Like in here:

End user defines encryption key, but yet we describe the feature as obfuscation. This doesn't make things fully obvious for end user: is it actually related or something different (I personally got confused, when I started to review PR, and yet I knew the intention).
If we consider that data-protection may for example provide forgetable payloads too as a feature, then we this feature could be considered as Data Encryption (which is also searchable seo keyword, and rather widely used within community).
wdyt?
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 is this change proposed?
Sensitive data should be obfuscated when leaving application via transport channels.
Description of Changes
Ecotone\DataProtection\Attribute\Sensitiveto define messages with sensitive datamessage,endpoint, or globally forchannelEcotone\DataProtection\Configuration\DataProtectionConfigurationPull Request Contribution Terms