AMBARI-24742. Encrypting/decrypting PASSWORD type properties when inserting them into the DB/using them #2458

smolnar82 · 2018-10-15T09:28:26Z

What changes were proposed in this pull request?

Sensitive service configuration values should be encrypted in the Ambari server DB, if enabled.

Sensitive service configuration values are defined by a service's configuration metadata. Properties are defined in XML files under the service's definition directory and contain attributes that Ambari may use to determine whether they should be encrypted or not.

Currently, Ambari uses the property-type attribute to determine the type of property. If the value of this attribute is PASSWORD, than the value is considered sensitive and should be encrypted.

The Ambari server should encrypt sensitive configuration values if the following has been met:

A master key has been setup using the ambari-server setup-security CLI (using option Fixed missing } in configuration.md #2 - Encrypt passwords stored in ambari.properties file)
The Ambari server configuration property named server.security.encrypt_sensitive_data is set to true

How was this patch tested?

In addition to unit testing I executed several E2E tests:
After creating a new ssl-server configuration:

Checking my changes using Ambari API (modified the code a little bit temporarily to avoid secret reference replacement):

asfgit · 2018-10-15T10:01:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4271/
Test FAILed.
Test FAILured.

smolnar82 · 2018-10-15T10:49:49Z

retest this please

asfgit · 2018-10-15T11:23:00Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4273/
Test PASSed.

rlevas · 2018-10-15T15:55:35Z

.../src/main/java/org/apache/ambari/server/security/encryption/PasswordPropertiesEncryptor.java

Can you document what the additional keys are for this implementation?

rlevas · 2018-10-15T16:49:52Z

.../src/main/java/org/apache/ambari/server/security/encryption/PasswordPropertiesEncryptor.java

I think we tend to use CollectionUtils.isNotEmpty(passwordProperties) instead of the long form here.

nice catch; I'll change it

rlevas · 2018-10-15T17:13:55Z

.../src/main/java/org/apache/ambari/server/security/encryption/PasswordPropertiesEncryptor.java

How will a stack upgrade affect this?

nice catch; I'll handle that situation like this: subscribing to org.apache.ambari.server.events.StackUpgradeFinishEvent and clean the map when this happened.

Any objection?

During a stack upgrade, new properties are added and possibly properties change types. How will this cache behave?

Upon stack upgrade finish the cache will be purged. So that proceeding encryption actions will re-build it.
See the latest patch I submitted (grep for onStackUpgradeFinished)

asfgit · 2018-10-15T21:23:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4277/
Test FAILed.
Test FAILured.

smolnar82 · 2018-10-16T07:29:00Z

retest this please

asfgit · 2018-10-16T08:01:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4278/
Test FAILed.
Test FAILured.

asfgit · 2018-10-16T09:31:00Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4279/
Test FAILed.
Test FAILured.

asfgit · 2018-10-16T09:57:05Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4280/
Test PASSed.

zeroflag · 2018-10-16T14:51:17Z

.../src/main/java/org/apache/ambari/server/security/encryption/PasswordPropertiesEncryptor.java

Any reason why we pass these arguments as a Object[] ? I see it comes from the interface, but the caller still needs to know which is the underlying implementation in use to fill these params correctly.

But the caller should not be down casted to the implementation.

ghost · 2018-10-16T16:03:30Z

.../src/main/java/org/apache/ambari/server/security/encryption/PasswordPropertiesEncryptor.java

I think it may be replaced with java.util.concurrent.ConcurrentHashMap#computeIfAbsent . As a bonus, we would get atomic check/assigment

Thanks for this hint (I've to admit I did not know about this option)

ghost · 2018-10-16T16:10:41Z

.../src/main/java/org/apache/ambari/server/security/encryption/PasswordPropertiesEncryptor.java

Are not we replacing here an instance of ConcurrentHashMap with an instance of a plain HashMap (that is unsafe for concurrent use)? I think we access and modify cluster properties from different threads

Those properties are generated by ConfigFactory after they have been parsed out from the request: https://github.com/apache/ambari/blob/trunk/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ConfigurationResourceProvider.java#L139
As you can see this is a simple HashMap. Which is ok since we guard the actual Config creation with locks and can not be happening that we have multiple properties map for the same version.

Moreover, those properties are being queried in ConfigImpl.getProperties(), which did simply wrapped the internal properties map in a HashMap in previous versions and does it here too.

smolnar82 · 2018-10-18T20:53:00Z

Tested the following use cases (sensitive data encryption is turned on):

updated config before upgrade
did a stack upgrade (added a new PASSWORD type property to cover that case too)
updated config after upgrade

Everything went fine.

asfgit · 2018-10-18T21:20:14Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4329/
Test PASSed.

asfgit · 2018-10-19T07:45:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4335/
Test PASSed.

smolnar82 · 2018-10-19T08:14:47Z

I had to rebase locally and send a new patch to be able to merge with trunk

asfgit · 2018-10-19T08:40:57Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4336/
Test FAILed.
Test FAILured.

smolnar82 · 2018-10-19T09:18:11Z

retest this please

asfgit · 2018-10-19T09:44:47Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4338/
Test PASSed.

asfgit · 2018-10-19T11:37:50Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4340/
Test FAILed.
Test FAILured.

asfgit · 2018-10-19T11:43:48Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4341/
Test FAILed.
Test FAILured.

asfgit · 2018-10-19T11:45:51Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4342/
Test FAILed.
Test FAILured.

asfgit · 2018-10-19T12:19:48Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4343/
Test FAILed.
Test FAILured.

…erting them into the DB/using them

…ed additional information we need for encryption and using 3rd party to check if password property set of a config type is netiher null nor empty

…ies cache

…encryptor

smolnar82 · 2018-10-19T13:20:34Z

Rebased locally with a new change to avoid Swagger issue (unrelated to my changes)

rlevas · 2018-10-19T13:38:49Z

...er/src/main/java/org/apache/ambari/server/security/encryption/ConfigPropertiesEncryptor.java

+
+  private Set<String> getPasswordProperties(Cluster cluster, StackId stackId, String configType) {
+    final long clusterId = cluster.getClusterId();
+    clusterPasswordProperties.computeIfAbsent(clusterId, v -> new ConcurrentHashMap<>()).computeIfAbsent(stackId, v -> new ConcurrentHashMap<>())


Why bother caching using the cluster id at all. Even if Ambari was managing multiple clusters, the stack definitions all would be the same for the same stack id.

hm..that's true. I may remove that layer next week.

asfgit · 2018-10-19T13:57:07Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/4345/
Test PASSed.

smolnar82 requested review from a user, mpapirkovskyy, rlevas and zeroflag October 15, 2018 09:28

smolnar82 self-assigned this Oct 15, 2018

smolnar82 added security encryption labels Oct 15, 2018

rlevas reviewed Oct 15, 2018

View reviewed changes

smolnar82 force-pushed the AMBARI-24742_part_2 branch from b70f463 to 16c8446 Compare October 15, 2018 20:40

smolnar82 force-pushed the AMBARI-24742_part_2 branch 2 times, most recently from 1541a16 to 633d988 Compare October 16, 2018 09:17

zeroflag reviewed Oct 16, 2018

View reviewed changes

ghost reviewed Oct 16, 2018

View reviewed changes

smolnar82 force-pushed the AMBARI-24742_part_2 branch from 19edde1 to fe0b822 Compare October 19, 2018 07:15

smolnar82 force-pushed the AMBARI-24742_part_2 branch from fe0b822 to d3d49ae Compare October 19, 2018 08:14

smolnar82 force-pushed the AMBARI-24742_part_2 branch 2 times, most recently from 33465d7 to 466013c Compare October 19, 2018 11:37

zeroflag approved these changes Oct 19, 2018

View reviewed changes

ghost approved these changes Oct 19, 2018

View reviewed changes

smolnar82 added 6 commits October 19, 2018 15:19

AMBARI-24742. Encrypting/decrypting PASSWORD type properties when ins…

3d0b7c6

…erting them into the DB/using them

AMBARI-24742. Handling the situation when stack is upgraded; document…

0e2dc79

…ed additional information we need for encryption and using 3rd party to check if password property set of a config type is netiher null nor empty

AMBARI-24472. Using a more effective way to populate password propert…

751189b

…ies cache

AMBARI-24742. Caching properties types by stacks

64812db

AMBARI-24742. To make encryptor really generic we implement a Config …

bd579c4

…encryptor

AMBARI-24742. Missing decryption when loading data from DB

587d796

smolnar82 force-pushed the AMBARI-24742_part_2 branch from bc2d699 to 587d796 Compare October 19, 2018 13:19

rlevas approved these changes Oct 19, 2018

View reviewed changes

smolnar82 merged commit 9c61584 into apache:trunk Oct 19, 2018

smolnar82 deleted the AMBARI-24742_part_2 branch October 19, 2018 15:56

AMBARI-24742. Encrypting/decrypting PASSWORD type properties when inserting them into the DB/using them #2458

AMBARI-24742. Encrypting/decrypting PASSWORD type properties when inserting them into the DB/using them #2458

Uh oh!

Conversation

smolnar82 commented Oct 15, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

asfgit commented Oct 15, 2018

Uh oh!

smolnar82 commented Oct 15, 2018

Uh oh!

asfgit commented Oct 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smolnar82 Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smolnar82 Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Oct 15, 2018

Uh oh!

smolnar82 commented Oct 16, 2018

Uh oh!

asfgit commented Oct 16, 2018

Uh oh!

asfgit commented Oct 16, 2018

Uh oh!

asfgit commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smolnar82 Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smolnar82 commented Oct 18, 2018

Uh oh!

asfgit commented Oct 18, 2018

Uh oh!

asfgit commented Oct 19, 2018

Uh oh!

smolnar82 commented Oct 19, 2018

Uh oh!

asfgit commented Oct 19, 2018

Uh oh!

smolnar82 commented Oct 19, 2018

Uh oh!

asfgit commented Oct 19, 2018

Uh oh!

asfgit commented Oct 19, 2018

Uh oh!

asfgit commented Oct 19, 2018

Uh oh!

asfgit commented Oct 19, 2018

Uh oh!

asfgit commented Oct 19, 2018

Uh oh!

smolnar82 Oct 15, 2018 •

edited

Loading

smolnar82 Oct 16, 2018 •

edited

Loading

smolnar82 Oct 16, 2018 •

edited

Loading