Ease of hidding sensitive properties from /status/proper…

[zemin-piao]

…ties endpoint

Description

Fixes #12063.

This PR has:

• been self-reviewed.
• added documentation for new or modified features or behaviors.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.

[FrankChen021]

Thanks for your contribution.

I'm wondering why not combine hiddenProperties and the proposed hiddenPropertiesContain together?

hiddenProperties = Sets.newHashSet(
"druid.s3.accessKey", 
"druid.s3.secretKey", 
"druid.metadata.storage.connector.password", 
"password", 
"key", 
"token", 
"pwd");

I think this won't introduce another property for users to config.

[zemin-piao]

Thanks for your contribution.

I'm wondering why not combine hiddenProperties and the proposed hiddenPropertiesContain together?

hiddenProperties = Sets.newHashSet(
"druid.s3.accessKey", 
"druid.s3.secretKey", 
"druid.metadata.storage.connector.password", 
"password", 
"key", 
"token", 
"pwd");

I think this won't introduce another property for users to config.

Thanks a lot for your advice. If I understand correctly, your proposal is to have only hiddenProperties property list, and always check whether element of hiddenProperties is substring of all property keys. (Correct me if I misunderstood)

Think with this approach indeed we can make it one configurable property, just by default it will be a bit slow to hide these properties since it checks substrings. Would this be something acceptable?

[capistrant]

seems to me that it would make most sense to change behavior of existing config to your proposed behavior of hiddenPropertiesContain. The existing behavior should still match and exclude exact matches so clusters who leverage the config already won't see a difference on upgrade. This way there is only one configuration to manage. Unless I'm missing some issue that this would introduce?

also, if this config is still not documented as the issue states, we should add the config to documentation for clarity.

[paul-rogers]

Because of a recent Travis change that introduced a "silent" merge conflict, you will need to rebase this PR on the latest master to pick up the newest Travis file.

[zemin-piao]

seems to me that it would make most sense to change behavior of existing config to your proposed behavior of hiddenPropertiesContain. The existing behavior should still match and exclude exact matches so clusters who leverage the config already won't see a difference on upgrade. This way there is only one configuration to manage. Unless I'm missing some issue that this would introduce?

also, if this config is still not documented as the issue states, we should add the config to documentation for clarity.

Indeed using one field approach is elegant and users don't need to remember many properties. If we directly adjust the behavior for hiddenProperties field, upgrading won't be a problem. Regarding backward compatibility, there is only one scenario where someone sets the property to ['pwd', 'key'], and after downgrading this won't work. For this scenario, should I include in the documentation? Or any other advices on this point?

[zemin-piao]

seems to me that it would make most sense to change behavior of existing config to your proposed behavior of hiddenPropertiesContain. The existing behavior should still match and exclude exact matches so clusters who leverage the config already won't see a difference on upgrade. This way there is only one configuration to manage. Unless I'm missing some issue that this would introduce?
also, if this config is still not documented as the issue states, we should add the config to documentation for clarity.

Indeed using one field approach is elegant and users don't need to remember many properties. If we directly adjust the behavior for hiddenProperties field, upgrading won't be a problem. Regarding backward compatibility, there is only one scenario where someone sets the property to ['pwd', 'key'], and after downgrading this won't work. For this scenario, should I include in the documentation? Or any other advices on this point?

Already put a note at the index.md regarding the change of behavior on this property

[FrankChen021]

Think with this approach indeed we can make it one configurable property, just by default it will be a bit slow to hide these properties since it checks substrings. Would this be something acceptable?

The performance is acceptable since it's only used in a RESTful API which is accessed in low frequency.

[capistrant]

few nits and one idea about a new default list to be more aggressive at redacting properties by default

[capistrant]

LGTM 👍

[zemin-piao]

May I ask anything still to be done before merging it?

[capistrant]

May I ask anything still to be done before merging it?

No changes expected to be needed. I usually leave a PR open for a bit after approving to make sure the others who have participated in the review have some time to circle back and add thoughts if they have them so we can avoid having to create a follow on PR if more changes are needed. I will plan on merging either this afternoon or tomorrow morning depending on my availability

[capistrant]

I'm actually going to email the dev list to see if we want to backport this into Druid 24 branch because I fear that many live environments are unknowingly exposing secrets in plaintext. Hopefully /status/properties is an authenticated endpoint for most production deployments. But even so, it's still not good to have sensitive data in plaintext regardless!

[santosh-d3vpl3x]

I'm actually going to email the dev list to see if we want to backport this into Druid 24 branch

That'd be great! Thanks @capistrant!

[capistrant]

@zemin-piao Thank you for the contribution! This will definitely help shore up the the security of many live clusters

[abhishekagarwal87]

with the new behaviour, couldn't we just set the default to ["password", "key", "token", "pwd"]? That should also cover ["druid.s3.accessKey","druid.s3.secretKey","druid.metadata.storage.connector.password"]

[zemin-piao]

with the new behaviour, couldn't we just set the default to ["password", "key", "token", "pwd"]? That should also cover ["druid.s3.accessKey","druid.s3.secretKey","druid.metadata.storage.connector.password"]

In case of downgrading the version, we also want to make sure the property file still contains ["druid.s3.accessKey","druid.s3.secretKey","druid.metadata.storage.connector.password"] to keep the old behavior.

[abhishekagarwal87]

I have also backported the change to the 24.0.0 branch.

[zemin-piao]

I have also backported the change to the 24.0.0 branch.

Awesome! Thanks a lot

[capistrant]

Unfortunately, I found this patch to be invalid after deploying it to a cluster of mine. #13045 is posted to fix in master and will also need to be backported to 24.0.0

[zemin-piao]

Unfortunately, I found this patch to be invalid after deploying it to a cluster of mine. #13045 is posted to fix in master and will also need to be backported to 24.0.0

ooops yeah Splitter.on(",").split(returnedProperties.get("druid.server.hiddenProperties")).forEach(hiddenProperties::add); does not work properly with the property array with '[]'. I reused this part from the existing test but should have caught this bug sooner indeed ...

Thanks a bunch for spotting it and fix it!