Revise PropertySourcesPropertyResolver's default logging and customizability [SPR-14370]

[spring-projects-issues]

Antonio Anzivino opened SPR-14370 and commented

PropertySourcesPropertyResolver may, under some circumstances, leak information that should not be normally logged. On line 90 it will log the key and the value of the property it finds if debug logging is enabled.

The following is the scenario in which the unwanted information is logged:

In order to store sensitive information (e.g. database password) in property files, we chose to encrypt them using a proprietary class that extends PropertySorucesPropertyResolver (in particular getPropertyAsRawString method).
Since the class extends PropertySourcesPropertyResolver, the logger will be named according to our package (com.acme.EncryptedPropertyResolver).
Unfortunately for us, we have enabled debug logging in UAT environment, resulting in the following

2015-07-17 10:57:13,683 DEBUG [localhost-startStop-1] com.acme.EncryptedPropertyResolver - getProperty - Found key 'database.password' in [file [/path/to/app.properties]] with type [String] and value '[redacted encrypted string]'

The string that is output to the log is encrypted, but our customer is still upset and reported this to us after an automated security scan. We had two options: 1) override the entire method and avoid logging value of property, 2) set logging level to > DEBUG for com.acme.EncryptedPropertyResolver and leave the rest intact.

Security consideration: the string output to the logs is encrypted as I said, so the password is not clearly printed to the logs, however this extends the attack surface as a malicious user with access to the code and application has more chances to obtain the key and the final plaintext password. Of course the key must be protected too, but that is another issue.
The fact is that while property files are protected (by technical and administrative policies) on customer's UAT instance, log files are normally not part of the security policy and are easily sent via email to interested parties when a problem occurs.

I might suggest, since dropping the entire value logging in the official Spring release might be excessive for debug purposes, implementing a blacklist-based value obfuscation. Like if the key contains word such as "password", "secret", "key" (key, really???) Spring may drop the value in the logging statement. Just to give an example.

This, in my personal experience, is done by Atlassian Bamboo logs too (e.g BAM-14475 or Answers

This issue is marked trivial by OP because it can be addressed with additional logging configuration.

Ref:
https://github.com/spring-projects/spring-framework/blob/master/spring-core/src/main/java/org/springframework/core/env/PropertySourcesPropertyResolver.java#L90

---

Affects: 3.2.17, 4.2.3

Issue Links:

• Accepting null as default value for a property [SPR-14896] #19462 Accepting null as default value for a property
• Don't log property values in PropertySourcesPropertyResolver by default [SPR-14709] #19274 Don't log property values in PropertySourcesPropertyResolver by default
• Consistent logging in Environment and PropertySource implementations [SPR-15825] #20380 Consistent logging in Environment and PropertySource implementations

[spring-projects-issues]

Juergen Hoeller commented

Aside from some of the logging to be revised there in any case (it's a bit too excessive), we could factor out a protected method which does the value logging... to be overridden in your custom subclass with a no-op?

[spring-projects-issues]

Antonio Anzivino commented

Actually I would prefer a different refactoring. The entire getProperty() being able to be overriden by copy&paste.

If I copy/paste the code in that method to my subclass I can't access private members of the subclass. In my opinion, from the OO point of view, all non-public members of non-final/sealed classes should be protected unless a good reason exists.

I can prepare an alternate PR easily for displaying

[spring-projects-issues]

Antonio Anzivino commented

Ok made #1088. It's very, very trivial now.

Now that I think about it, propertySources is a constructor argument. The subclass may and should save a copy of that object which is not currently accessible.

This doesn't change my opinion about giving more visibility to members of non-final classes.

You can take a look at it. And obviously adding a protected getter doesn't change the result

[spring-projects-issues]

Juergen Hoeller commented

We're generally fans of the template method pattern, so I'd rather expose several methods for optional overriding while still keeping the actual PropertySources handling to the base class (i.e. not exposing the field to subclasses). If you're actually overriding everything, why not simply extend from AbstractPropertyResolver directly, manually handling all PropertySources access? I could imagine that the various overloaded methods are a bit painful, so I'll handle more of those in AbstractPropertyResolver as a part of my ongoing revision here.

FWIW, I'm also deprecating getPropertyAsClass while being at it. We don't use that method anywhere, its implementation is somewhat quirky, and we force AbstractPropertyResolver subclasses to implement it.

I'll push the result of my efforts tomorrow. Let's have a discussion about it once available.

[spring-projects-issues]

Juergen Hoeller commented

Aside from the general revision that I committed last week, PropertySourcesPropertyResolver also provides a logKeyFound template method for your particular purpose now.

[spring-projects-issues]

Christoffer Sawicki commented

I know this is not a support forum, but I've run into exactly the same with passwords getting logged (at a DEBUG level) in an environment where that is not acceptable. I understand how to override logKeyFound in PropertySourcesPropertyResolver but injecting the customized PropertySourcesPropertyResolver seems awfully tricky. Is there any easier way to do that than replacing the entire PropertySourcesPlaceholderConfigurer (that has a new PropertySourcesPropertyResolver(…)) with my own implementation?

[spring-projects-issues]

Juergen Hoeller commented

Christoffer Sawicki, could you please create a follow-up issue to this one - about dropping PropertySourcesPropertyResolver's value logging by default? We might do that for 4.3.3 then, effectively turning the logKeyFound method into a hook for logging the value if you need it...