add option to configure externally supplied implementation for sensitive data masking

[JohnMTu]

Current feature to hide sensitive data is not good enough for us.
E.g. we have interfaces containing key-value like structures and we want to hide only values of element where key value is some concrete value etc.
Hence would like to make it possible to plug in own implementation.

[reta]

@JohnMTu thanks for the pull request (apologies for the delay), I have a few comments:

• could you please create a JIRA issue for the feature in question?
• the MaskSensitiveHelper is intended to do exactly what the suggested MaskSensitiveMasker abstraction does, I think this is not needed and could be solved with the ability to provide custom MaskSensitiveHelper instance

[JohnMTu]

@reta : This is ticket i just made https://issues.apache.org/jira/browse/CXF-8831

Having option to provide own instance of MaskSensitiveHelper subclass would be also fine for me.
Perhaps it can be even refactored to interface.

Only not so nice thing might be, that method setSensitiveElementNames is invoked from AbstractLoggingInterceptor#setSensitiveElementNames and LoggingFeature and LoggingFeature.Portable.
Similar for setSensitiveProtocolHeaderNames.
In case of our implementation such method would have no use. But for backward compatibility we would need to keep them.

Do you want to do stuff yourself, or should i provide it?
If i should do it, do you prefer to keep MaskSensitiveHelper a class or should be interface refactored out of it?

thanks

[reta]

@reta : This is ticket i just made https://issues.apache.org/jira/browse/CXF-8831

Thank you @JohnMTu !

Having option to provide own instance of MaskSensitiveHelper subclass would be also fine for me. Perhaps it can be even refactored to interface.

I think extracting interface would be cleaner in general

Only not so nice thing might be, that method setSensitiveElementNames is invoked from AbstractLoggingInterceptor#setSensitiveElementNames and LoggingFeature and LoggingFeature.Portable. Similar for setSensitiveProtocolHeaderNames. In case of our implementation such method would have no use. But for backward compatibility we would need to keep them.

Yes, the delegation model could be used instead (in this regards, subclassing would be simpler than interface extraction), but preserving backward compatibility is necessary.

Do you want to do stuff yourself, or should i provide it? If i should do it, do you prefer to keep MaskSensitiveHelper a class or should be interface refactored out of it?

Up to you, would be great if you finish the change since this is your idea and most of the work is done already. I am not sure you have time though (no pressure).

thanks

Thanks to you!

[JohnMTu]

i didn't forget, just no time atm. But is in my list.

[reta]

@JohnMTu could you please rebase against latest 3.5.x-fixes upstream? thank you

[JohnMTu]

hello,

@reta: so far i removed "final" from maskSensitiveHelper field and added setters. Hence subclasses can be passed.

In next step i tried to extract interface. Here I realized there is some inconsistency, as configuration used for hiding data is in once case on Interceptor org.apache.cxf.ext.logging.AbstractLoggingInterceptor#sensitiveProtocolHeaderNames while in other case it's near to implementation in org.apache.cxf.ext.logging.DefaultMaskSensitiveHelper#replacementsJSON (and replacementsXML).

Best would be to cleanup it and keep it next to implementation, but unfortunately that would mean to change to some public methods, e.g.
org.apache.cxf.ext.logging.event.DefaultLogEventMapper#map(org.apache.cxf.message.Message, java.util.Set<java.lang.String>) would not need 2nd argument anymore.
And also org.apache.cxf.ext.logging.MaskSensitiveHelper#maskHeaders would not need it.

Anyway, i'm not sure, if even change MaskSensitiveHelper to from class to interface is allowed in terms of backward compatibility. Please give me some hints,

So here is my attempt:
https://github.com/JohnMTu/cxf/compare/3.5.x-fixes...JohnMTu:cxf:CXF-8831_extract_interface?expand=1

Unfortunately github doesn't show rename of MaskSensitiveHelper to DefaultMaskSensitiveHelper and instead shows diff against extracted interface .. but when checking commit by commit, rename is visible. Not sure how it would look like, if squash would be used.

Please check.
Thanks you.

[reta]

Hey @JohnMTu

@reta: so far i removed "final" from maskSensitiveHelper field and added setters. Hence subclasses can be passed.

I think this is great!

Anyway, i'm not sure, if even change MaskSensitiveHelper to from class to interface is allowed in terms of backward compatibility. Please give me some hints,

That would be a breaking change sadly, if subclassing works, that would be the best option (for now)

[JohnMTu]

then lets stick to subclassing.

And for rest, if you want, put come cleanup TODO for next major version or so.

[JohnMTu]

So i suggest, from https://github.com/JohnMTu/cxf/compare/3.5.x-fixes...JohnMTu:cxf:CXF-8831_extract_interface?expand=1 i would take at least changes in rt/features/logging/src/main/java/org/apache/cxf/ext/logging/event/DefaultLogEventMapper.java

and in rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java extend setSensitiveDataHelper with call to
this.eventMapper.setSensitiveDataHelper(maskSensitiveHelper);

hence also 2nd usage can be replaced with subclass.

[reta]

would take at least changes in rt/features/logging/src/main/java/org/apache/cxf/ext/logging/event/DefaultLogEventMapper.java and in rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java extend setSensitiveDataHelper with call to
this.eventMapper.setSensitiveDataHelper(maskSensitiveHelper);

Sure, would you like to make this changes in scope of this pull request or open another one?

[JohnMTu]

i can do it here, but only in 2 weeks. What is better for you?

[reta]

i can do it here, but only in 2 weeks. What is better for you?

Let me merge this one and you will be able to work on the next improvements at your own pace, thank you