You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by arvindn05 <no...@github.com> on 2015/07/29 08:04:43 UTC
[jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Fix bug where password information is printed in logs in case of exceptions in keystone
fixes bug https://issues.apache.org/jira/browse/JCLOUDS-958
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/830
-- Commit Summary --
* Fix bug where password information is printed in logs in case of exceptions
-- File Changes --
M apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/binders/BindAuthToJsonPayload.java (28)
M core/src/main/java/org/jclouds/http/HttpResponseException.java (13)
M core/src/main/java/org/jclouds/io/Payload.java (14)
M core/src/main/java/org/jclouds/io/payloads/BasePayload.java (19)
M core/src/main/java/org/jclouds/io/payloads/DelegatingPayload.java (20)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/830.patch
https://github.com/jclouds/jclouds/pull/830.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by Zack Shoylev <no...@github.com>.
merged and backported
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126412541
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by Ignasi Barrera <no...@github.com>.
I'd agree if the fix just masked the sensitive part of the payload just like the "toString" does. But we are removing the entire payload.
Wouldn't a note in the release notes/release announcement saying the issue is fixed and it can be turned on by setting the property be enough for those worried about clear text sensitive payloads?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126567632
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by Zack Shoylev <no...@github.com>.
Hmm I thought that the backport should be fine because that is what the original PR targeted (it targeted the 1.9.x branch), which is why it was backported. Sorry for any confusion.
Instead of reverting the change, we should just merge a PR to 1.9.x specifically with the default targeted behavior that we want, but keep the option in.
Please link such a PR here so we can keep track.
As for the acceptable solutions, I really would prefer to err on the side of more security. I am OK with the code as is; but is there a way to filter just sensitive data from the payload? Like search-and-replace passwords? Or is the whole payload stream considered sensitive?
I am not really sure what parts of the payload are considered sensitive, so let me know if I'm missing something.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126695049
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by Zack Shoylev <no...@github.com>.
Looks good to merge/backport, is there any work left on this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126402014
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by arvindn05 <no...@github.com>.
> @@ -83,8 +83,13 @@ static String requestPayloadIfStringOrFormIfNotReturnEmptyString(HttpRequest req
> && request.getPayload().getContentMetadata().getContentLength() != null
> && request.getPayload().getContentMetadata().getContentLength() < 1024) {
> try {
> - return String.format(" [%s] ", request.getPayload() instanceof StringPayload ? request.getPayload()
> - .getRawContent() : Strings2.toStringAndClose(request.getPayload().openStream()));
> + String logStatement;
> + if (request.getPayload() instanceof StringPayload) {
> + logStatement = request.getPayload().isSensitive() ? "---- SENSITIVE INFORMATION ----" : request.getPayload().getRawContent().toString();
will make the change
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830/files#r35839307
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by Zack Shoylev <no...@github.com>.
Closed #830.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#event-369711853
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by arvindn05 <no...@github.com>.
I dont have any other work left on this.
@zack-shoylev we should be able to merge this to master. Is there anything i need to do for helping you merge this to master?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126404352
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by arvindn05 <no...@github.com>.
we do add this message in its place instead of removing the payload "Sensitive data in payload, use PROPERTY_LOGGER_WIRE_LOG_SENSITIVE_INFO override to enable logging this data." so its going to be obvious i hope.
like i said i am fine either way, though i prefer keeping the authentication payload out of the logs until explicitly requested with an ovveride.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126587215
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by Ignasi Barrera <no...@github.com>.
I'm ok with the merge but not with the backport. In 1.9.x the defsult for the property should be set to true.
We can't just remove the payload logs in a bugfix release. There might be tooling, etc, that process the logs and we don't want to break any integration. We should provide the feature but default to the current behavior in 1.9.x.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126439968
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @arvindn05! The approach looks pretty clean.
You will also want to check the sensitive flag [in the Wire class](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/logging/internal/Wire.java#L120-L137), so the sensitive information does not get printed in the logs with the wire logs are enabled.
Regarding the change, this is something users might want to be able to turn on/off. I mean, this fix does not only mask the sensitive information, but removes the entire payload from the logs, and that might not be ideal/wanted by everyone. We should provide a way to disable this (I'm OK having it enabled by default), such a property one can configure when creating the context.
/cc @andrewgaul @demobox
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-125863131
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -83,8 +83,13 @@ static String requestPayloadIfStringOrFormIfNotReturnEmptyString(HttpRequest req
> && request.getPayload().getContentMetadata().getContentLength() != null
> && request.getPayload().getContentMetadata().getContentLength() < 1024) {
> try {
> - return String.format(" [%s] ", request.getPayload() instanceof StringPayload ? request.getPayload()
> - .getRawContent() : Strings2.toStringAndClose(request.getPayload().openStream()));
> + String logStatement;
> + if (request.getPayload() instanceof StringPayload) {
> + logStatement = request.getPayload().isSensitive() ? "---- SENSITIVE INFORMATION ----" : request.getPayload().getRawContent().toString();
The sensitive flag should apply to all payload types, not just string ones.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830/files#r35732027
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by arvindn05 <no...@github.com>.
@nacx I am fine if you want the default behavior to be that you print out the password information in plaintext for 1.9.x
But since that will cause a security issue i would suggest to not do that. I understand your concern about tooling and tools which process logs but they should not be relying on security issue being present in jclouds. I think its good to break their tooling in this case.
maybe we can add a release note to say this issue has been fixed and might require updates to tooling.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126537798
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by Zack Shoylev <no...@github.com>.
Also I see this is against 1.9.x, can it be merged to master as well?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126402176
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by Zack Shoylev <no...@github.com>.
I will merge and run tests, then backport
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126406658
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by arvindn05 <no...@github.com>.
>>but is there a way to filter just sensitive data from the payload? Like search-and-replace passwords?
by the time we get the payload data, it has been converted into JSON and the only way to selectively mask the sensitive payload, we would have to go with a search and replace solution.
I dont like using search and replace because it will only handle one off cases and not be a generic solution.
Regardless, created a PR to change default logging behavior in 1.9.x
see https://github.com/jclouds/jclouds/pull/834
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126821196
Re: [jclouds] Fix bug where password information is printed in logs in case of exce… (#830)
Posted by arvindn05 <no...@github.com>.
thanks
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/830#issuecomment-126407782