From ba55ab4b12bcd63141911627bca1c80f6505c7f4 Mon Sep 17 00:00:00 2001 From: Arvind Nadendla Date: Tue, 28 Jul 2015 23:01:25 -0700 Subject: [PATCH] Fix bug where password information is printed in logs in case of exceptions --- .../v2_0/binders/BindAuthToJsonPayload.java | 28 +++++----- .../v2_0/handlers/KeystoneErrorHandler.java | 18 ++++-- core/src/main/java/org/jclouds/Constants.java | 7 +++ .../jclouds/http/HttpResponseException.java | 35 ++++++++---- .../org/jclouds/http/internal/HttpWire.java | 17 +++++- .../src/main/java/org/jclouds/io/Payload.java | 14 ++++- .../org/jclouds/io/payloads/BasePayload.java | 21 +++++-- .../io/payloads/DelegatingPayload.java | 20 +++++-- .../org/jclouds/logging/internal/Wire.java | 52 +++++++++++------ .../org/jclouds/http/internal/WireTest.java | 56 +++++++++++++++++-- 10 files changed, 203 insertions(+), 65 deletions(-) diff --git a/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/binders/BindAuthToJsonPayload.java b/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/binders/BindAuthToJsonPayload.java index 0ede3a125e..6dda128b84 100644 --- a/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/binders/BindAuthToJsonPayload.java +++ b/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/binders/BindAuthToJsonPayload.java @@ -16,14 +16,11 @@ */ package org.jclouds.openstack.keystone.v2_0.binders; -import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.base.Preconditions.checkNotNull; - -import java.util.Map; - -import javax.inject.Inject; -import javax.inject.Singleton; - +import com.google.common.base.Predicates; +import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableMap.Builder; +import com.google.common.collect.Iterables; import org.jclouds.http.HttpRequest; import org.jclouds.json.Json; import org.jclouds.openstack.keystone.v2_0.config.CredentialType; @@ -31,11 +28,12 @@ import org.jclouds.rest.MapBinder; import org.jclouds.rest.binders.BindToJsonPayload; import org.jclouds.rest.internal.GeneratedHttpRequest; -import com.google.common.base.Predicates; -import com.google.common.base.Strings; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableMap.Builder; -import com.google.common.collect.Iterables; +import javax.inject.Inject; +import javax.inject.Singleton; +import java.util.Map; + +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; @Singleton public class BindAuthToJsonPayload extends BindToJsonPayload implements MapBinder { @@ -71,7 +69,9 @@ public class BindAuthToJsonPayload extends BindToJsonPayload implements MapBinde builder.put("tenantName", postParams.get("tenantName")); else if (!Strings.isNullOrEmpty((String) postParams.get("tenantId"))) builder.put("tenantId", postParams.get("tenantId")); - return super.bindToRequest(request, ImmutableMap.of("auth", builder.build())); + R authRequest = super.bindToRequest(request, ImmutableMap.of("auth", builder.build())); + authRequest.getPayload().setSensitive(true); + return authRequest; } } diff --git a/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/handlers/KeystoneErrorHandler.java b/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/handlers/KeystoneErrorHandler.java index 8d80840208..520e3268c3 100644 --- a/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/handlers/KeystoneErrorHandler.java +++ b/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/handlers/KeystoneErrorHandler.java @@ -16,10 +16,8 @@ */ package org.jclouds.openstack.keystone.v2_0.handlers; -import static org.jclouds.http.HttpUtils.closeClientButKeepContentStream; - -import javax.inject.Singleton; - +import com.google.inject.Inject; +import org.jclouds.Constants; import org.jclouds.http.HttpCommand; import org.jclouds.http.HttpErrorHandler; import org.jclouds.http.HttpResponse; @@ -27,19 +25,29 @@ import org.jclouds.http.HttpResponseException; import org.jclouds.rest.AuthorizationException; import org.jclouds.rest.ResourceNotFoundException; +import javax.inject.Named; +import javax.inject.Singleton; + +import static org.jclouds.http.HttpUtils.closeClientButKeepContentStream; + /** * This will parse and set an appropriate exception on the command object. */ // TODO: is there error spec someplace? let's type errors, etc. @Singleton public class KeystoneErrorHandler implements HttpErrorHandler { + + @Inject(optional = true) + @Named(Constants.PROPERTY_LOGGER_WIRE_LOG_SENSITIVE_INFO) + private boolean logSensitiveInformation = false; + public void handleError(HttpCommand command, HttpResponse response) { // it is important to always read fully and close streams byte[] data = closeClientButKeepContentStream(response); String message = data != null ? new String(data) : null; Exception exception = message != null ? new HttpResponseException(command, response, message) - : new HttpResponseException(command, response); + : new HttpResponseException(command, response, logSensitiveInformation); message = message != null ? message : String.format("%s -> %s", command.getCurrentRequest().getRequestLine(), response.getStatusLine()); switch (response.getStatusCode()) { diff --git a/core/src/main/java/org/jclouds/Constants.java b/core/src/main/java/org/jclouds/Constants.java index b58551192e..a658026cf7 100644 --- a/core/src/main/java/org/jclouds/Constants.java +++ b/core/src/main/java/org/jclouds/Constants.java @@ -191,6 +191,13 @@ public final class Constants { * trust self-signed certs */ public static final String PROPERTY_TRUST_ALL_CERTS = "jclouds.trust-all-certs"; + /** + * Boolean property. + *

+ * true to allow logging of sensitive information like passwords in the wire log + * default value is false + */ + public static final String PROPERTY_LOGGER_WIRE_LOG_SENSITIVE_INFO = "jclouds.wire.log.sensitive"; /** * Name of the logger that records all http headers from the client and the server. */ diff --git a/core/src/main/java/org/jclouds/http/HttpResponseException.java b/core/src/main/java/org/jclouds/http/HttpResponseException.java index eb71002624..583e759a5b 100644 --- a/core/src/main/java/org/jclouds/http/HttpResponseException.java +++ b/core/src/main/java/org/jclouds/http/HttpResponseException.java @@ -16,12 +16,13 @@ */ package org.jclouds.http; -import java.io.IOException; - +import org.jclouds.io.Payload; import org.jclouds.io.payloads.StringPayload; import org.jclouds.javax.annotation.Nullable; import org.jclouds.util.Strings2; +import java.io.IOException; + /** * Represents an error obtained from an HttpResponse. */ @@ -71,20 +72,34 @@ public class HttpResponseException extends RuntimeException { } public HttpResponseException(HttpCommand command, HttpResponse response) { + this(command, response, false); + } + public HttpResponseException(HttpCommand command, HttpResponse response, boolean logSensitiveInformation) { this(String.format("request: %s %sfailed with response: %s", command.getCurrentRequest().getRequestLine(), - requestPayloadIfStringOrFormIfNotReturnEmptyString(command.getCurrentRequest()), + requestPayloadIfStringOrFormIfNotReturnEmptyString(command.getCurrentRequest(), logSensitiveInformation), response.getStatusLine()), command, response); } static String requestPayloadIfStringOrFormIfNotReturnEmptyString(HttpRequest request) { - if (request.getPayload() != null - && ("application/x-www-form-urlencoded".equals(request.getPayload().getContentMetadata().getContentType()) || request - .getPayload() instanceof StringPayload) - && request.getPayload().getContentMetadata().getContentLength() != null - && request.getPayload().getContentMetadata().getContentLength() < 1024) { + return requestPayloadIfStringOrFormIfNotReturnEmptyString(request, false); + } + + static String requestPayloadIfStringOrFormIfNotReturnEmptyString(HttpRequest request, boolean logSensitiveInformation) { + Payload payload = request.getPayload(); + if (payload != null + && ("application/x-www-form-urlencoded".equals(payload.getContentMetadata().getContentType()) || payload instanceof StringPayload) + && payload.getContentMetadata().getContentLength() != null + && payload.getContentMetadata().getContentLength() < 1024) { try { - return String.format(" [%s] ", request.getPayload() instanceof StringPayload ? request.getPayload() - .getRawContent() : Strings2.toStringAndClose(request.getPayload().openStream())); + String logStatement; + if (payload.isSensitive() && !logSensitiveInformation) { + logStatement = "Sensitive data in payload, use PROPERTY_LOGGER_WIRE_LOG_SENSITIVE_INFO override to enable logging this data."; + } else if (payload instanceof StringPayload) { + logStatement = payload.getRawContent().toString(); + } else { + logStatement = Strings2.toStringAndClose(payload.openStream()); + } + return String.format(" [%s] ", logStatement); } catch (IOException e) { } } diff --git a/core/src/main/java/org/jclouds/http/internal/HttpWire.java b/core/src/main/java/org/jclouds/http/internal/HttpWire.java index 439f1721b0..e8aba2e744 100644 --- a/core/src/main/java/org/jclouds/http/internal/HttpWire.java +++ b/core/src/main/java/org/jclouds/http/internal/HttpWire.java @@ -16,21 +16,32 @@ */ package org.jclouds.http.internal; -import javax.annotation.Resource; -import javax.inject.Named; - +import com.google.common.annotations.VisibleForTesting; +import com.google.inject.Inject; import org.jclouds.Constants; import org.jclouds.logging.Logger; import org.jclouds.logging.internal.Wire; +import javax.annotation.Resource; +import javax.inject.Named; + public class HttpWire extends Wire { @Resource @Named(Constants.LOGGER_HTTP_WIRE) Logger wireLog = Logger.NULL; + @VisibleForTesting + @Inject(optional = true) + @Named(Constants.PROPERTY_LOGGER_WIRE_LOG_SENSITIVE_INFO) + boolean logSensitiveInformation = false; + public Logger getWireLog() { return wireLog; } + @Override + protected boolean isLogSensitiveInformation() { + return logSensitiveInformation; + } } diff --git a/core/src/main/java/org/jclouds/io/Payload.java b/core/src/main/java/org/jclouds/io/Payload.java index 5316d6cb9d..4754f33134 100644 --- a/core/src/main/java/org/jclouds/io/Payload.java +++ b/core/src/main/java/org/jclouds/io/Payload.java @@ -17,8 +17,8 @@ package org.jclouds.io; import java.io.Closeable; -import java.io.InputStream; import java.io.IOException; +import java.io.InputStream; public interface Payload extends Closeable { @@ -54,4 +54,16 @@ public interface Payload extends Closeable { void setContentMetadata(MutableContentMetadata in); + /** + * Sets whether the payload contains sensitive information. This is used when trying to decide whether to print out the + * payload information or not in logs + */ + void setSensitive(boolean isSensitive); + + /** + * Returns whether the payload contains sensitive information. This is used when trying to decide whether to print out the + * payload information or not in logs + */ + boolean isSensitive(); + } diff --git a/core/src/main/java/org/jclouds/io/payloads/BasePayload.java b/core/src/main/java/org/jclouds/io/payloads/BasePayload.java index 338bb312db..a25a38d97c 100644 --- a/core/src/main/java/org/jclouds/io/payloads/BasePayload.java +++ b/core/src/main/java/org/jclouds/io/payloads/BasePayload.java @@ -16,20 +16,20 @@ */ package org.jclouds.io.payloads; -import static com.google.common.base.Preconditions.checkNotNull; +import com.google.common.base.Throwables; +import org.jclouds.io.MutableContentMetadata; +import org.jclouds.io.Payload; import java.io.IOException; import java.io.InputStream; -import com.google.common.base.Throwables; - -import org.jclouds.io.MutableContentMetadata; -import org.jclouds.io.Payload; +import static com.google.common.base.Preconditions.checkNotNull; public abstract class BasePayload implements Payload { protected final V content; protected transient volatile boolean written; protected MutableContentMetadata contentMetadata; + private boolean isSensitive; protected BasePayload(V content) { this(content, new BaseMutableContentMetadata()); @@ -84,7 +84,7 @@ public abstract class BasePayload implements Payload { @Override public String toString() { - return "[content=" + (content != null) + ", contentMetadata=" + contentMetadata + ", written=" + written + "]"; + return "[content=" + (content != null) + ", contentMetadata=" + contentMetadata + ", written=" + written + ", isSensitive=" + isSensitive + "]"; } /** @@ -128,4 +128,13 @@ public abstract class BasePayload implements Payload { this.contentMetadata = in; } + @Override + public void setSensitive(boolean isSensitive) { + this.isSensitive = isSensitive; + } + + @Override + public boolean isSensitive() { + return this.isSensitive; + } } diff --git a/core/src/main/java/org/jclouds/io/payloads/DelegatingPayload.java b/core/src/main/java/org/jclouds/io/payloads/DelegatingPayload.java index df1b06b992..14558b107b 100644 --- a/core/src/main/java/org/jclouds/io/payloads/DelegatingPayload.java +++ b/core/src/main/java/org/jclouds/io/payloads/DelegatingPayload.java @@ -16,19 +16,19 @@ */ package org.jclouds.io.payloads; -import static com.google.common.base.Preconditions.checkNotNull; +import com.google.common.base.Throwables; +import org.jclouds.io.MutableContentMetadata; +import org.jclouds.io.Payload; import java.io.IOException; import java.io.InputStream; -import com.google.common.base.Throwables; - -import org.jclouds.io.MutableContentMetadata; -import org.jclouds.io.Payload; +import static com.google.common.base.Preconditions.checkNotNull; public class DelegatingPayload implements Payload { private final Payload delegate; + private boolean isSensitive; public DelegatingPayload(Payload delegate) { this.delegate = checkNotNull(delegate, "delegate"); @@ -110,4 +110,14 @@ public class DelegatingPayload implements Payload { public void setContentMetadata(MutableContentMetadata in) { delegate.setContentMetadata(in); } + + @Override + public void setSensitive(boolean isSensitive) { + this.isSensitive = isSensitive; + } + + @Override + public boolean isSensitive() { + return this.isSensitive; + } } diff --git a/core/src/main/java/org/jclouds/logging/internal/Wire.java b/core/src/main/java/org/jclouds/logging/internal/Wire.java index 64f61ceedf..8c20c2d618 100644 --- a/core/src/main/java/org/jclouds/logging/internal/Wire.java +++ b/core/src/main/java/org/jclouds/logging/internal/Wire.java @@ -16,10 +16,14 @@ */ package org.jclouds.logging.internal; -import static com.google.common.base.Preconditions.checkNotNull; -import static org.jclouds.io.Payloads.newPayload; -import static org.jclouds.util.Closeables2.closeQuietly; +import com.google.common.io.ByteStreams; +import com.google.common.io.FileBackedOutputStream; +import org.jclouds.io.MutableContentMetadata; +import org.jclouds.io.Payload; +import org.jclouds.io.PayloadEnclosing; +import org.jclouds.logging.Logger; +import javax.annotation.Resource; import java.io.ByteArrayInputStream; import java.io.File; import java.io.FileInputStream; @@ -28,15 +32,9 @@ import java.io.FilterInputStream; import java.io.IOException; import java.io.InputStream; -import javax.annotation.Resource; - -import org.jclouds.io.MutableContentMetadata; -import org.jclouds.io.Payload; -import org.jclouds.io.PayloadEnclosing; -import org.jclouds.logging.Logger; - -import com.google.common.io.ByteStreams; -import com.google.common.io.FileBackedOutputStream; +import static com.google.common.base.Preconditions.checkNotNull; +import static org.jclouds.io.Payloads.newPayload; +import static org.jclouds.util.Closeables2.closeQuietly; /** * Logs data to the wire LOG, similar to {@code org.apache.HttpWire.impl.conn.Wire} @@ -48,6 +46,10 @@ public abstract class Wire { protected abstract Logger getWireLog(); + protected boolean isLogSensitiveInformation() { + return false; + } + private void wire(String header, InputStream instream) { StringBuilder buffer = new StringBuilder(); int ch; @@ -119,7 +121,13 @@ public abstract class Wire { public void input(PayloadEnclosing request) { Payload oldContent = request.getPayload(); - Payload wiredPayload = newPayload(input(oldContent.getInput())); + Payload wiredPayload; + if (!oldContent.isSensitive() || isLogSensitiveInformation()) { + wiredPayload = newPayload(input(oldContent.getInput())); + } else { + wiredPayload = newPayload(oldContent.getInput()); + } + wiredPayload.setSensitive(oldContent.isSensitive()); copyPayloadMetadata(oldContent, wiredPayload); request.setPayload(wiredPayload); } @@ -127,11 +135,21 @@ public abstract class Wire { public void output(PayloadEnclosing request) { Payload oldContent = request.getPayload(); Payload wiredPayload; - try { - wiredPayload = newPayload(output(oldContent.getRawContent())); - } catch (UnsupportedOperationException e) { - wiredPayload = newPayload(output(oldContent.getInput())); + if (!oldContent.isSensitive() || isLogSensitiveInformation()) { + try { + wiredPayload = newPayload(output(oldContent.getRawContent())); + } catch (UnsupportedOperationException e) { + wiredPayload = newPayload(output(oldContent.getInput())); + } + } else { + try { + wiredPayload = newPayload(oldContent.getRawContent()); + } catch (UnsupportedOperationException e) { + wiredPayload = newPayload(oldContent.getInput()); + } + output("Sensitive data in payload, use PROPERTY_LOGGER_WIRE_LOG_SENSITIVE_INFO override to enable logging this data."); } + wiredPayload.setSensitive(oldContent.isSensitive()); copyPayloadMetadata(oldContent, wiredPayload); request.setPayload(wiredPayload); } diff --git a/core/src/test/java/org/jclouds/http/internal/WireTest.java b/core/src/test/java/org/jclouds/http/internal/WireTest.java index b3d2fb952b..9057fd5aed 100644 --- a/core/src/test/java/org/jclouds/http/internal/WireTest.java +++ b/core/src/test/java/org/jclouds/http/internal/WireTest.java @@ -16,14 +16,18 @@ */ package org.jclouds.http.internal; -import static org.testng.Assert.assertEquals; +import org.jclouds.http.HttpRequest; +import org.jclouds.io.PayloadEnclosing; +import org.jclouds.io.payloads.StringPayload; +import org.jclouds.logging.Logger; +import org.jclouds.util.Strings2; +import org.testng.annotations.Test; import java.io.ByteArrayInputStream; import java.io.InputStream; -import org.jclouds.logging.Logger; -import org.jclouds.util.Strings2; -import org.testng.annotations.Test; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNotEquals; @Test(groups = "unit", sequential = true) public class WireTest { @@ -130,4 +134,48 @@ public class WireTest { wire.output("foo"); assertEquals(((BufferLogger) wire.getWireLog()).buff.toString(), ">> \"foo\""); } + + @Test + public void testInputPayload() throws Exception { + HttpWire wire = setUp(); + StringPayload payload = new StringPayload("foo"); + PayloadEnclosing request = HttpRequest.builder().method("foo").endpoint("http://foo").payload(payload).build(); + wire.input(request); + BufferLogger wireLog = (BufferLogger) wire.getWireLog(); + assertEquals(wireLog.buff.toString(), "<< \"foo\"", "Expected payload to be printed in logs"); + wireLog.buff.setLength(0); + + payload.setSensitive(true); + request = HttpRequest.builder().method("foo").endpoint("http://foo").payload(payload).build(); + wire.input(request); + assertNotEquals(wireLog.buff.toString(), "<< \"foo\"", "Expected payload to NOT be printed in logs"); + wireLog.buff.setLength(0); + + wire.logSensitiveInformation = true; + request = HttpRequest.builder().method("foo").endpoint("http://foo").payload(payload).build(); + wire.input(request); + assertEquals(wireLog.buff.toString(), "<< \"foo\"", "Expected payload to be printed in logs"); + } + + @Test + public void testOutputPayload() throws Exception { + HttpWire wire = setUp(); + StringPayload payload = new StringPayload("foo"); + PayloadEnclosing request = HttpRequest.builder().method("foo").endpoint("http://foo").payload(payload).build(); + wire.output(request); + BufferLogger wireLog = (BufferLogger) wire.getWireLog(); + assertEquals(wireLog.buff.toString(), ">> \"foo\"", "Expected payload to be printed in logs"); + wireLog.buff.setLength(0); + + payload.setSensitive(true); + request = HttpRequest.builder().method("foo").endpoint("http://foo").payload(payload).build(); + wire.output(request); + assertNotEquals(wireLog.buff.toString(), ">> \"foo\"", "Expected payload to NOT be printed in logs"); + wireLog.buff.setLength(0); + + wire.logSensitiveInformation = true; + request = HttpRequest.builder().method("foo").endpoint("http://foo").payload(payload).build(); + wire.output(request); + assertEquals(wireLog.buff.toString(), ">> \"foo\"", "Expected payload to be printed in logs"); + } }