fix!: gate logging intermediate signing results which may contain sensitive information #984
Conversation
…sitive information
| * level. This is an opt-in configuration because these intermediate outputs may contain sensitive fields bound | ||
| * to headers, URI, or query parmaeters. | ||
| */ | ||
| public val enableTraceLogging: Boolean = builder.enableTraceLogging |
There was a problem hiding this comment.
Not sure what we wanted to call this, let the 🚲 🏚️ commence!
There was a problem hiding this comment.
I like this name if we were controlling all logger.trace calls with it, but we always log the "string to sign" at the trace level regardless of this flag. I think we should either rename this to better describe exactly what gets logged, or block the string-to-sign logging with this flag too.
There was a problem hiding this comment.
Yeah this is why I wanted to bikeshed over it because I had a similar feeling. I don't want to call it something too specific like logCanonicalRequests in the event there are other signing steps or calculations that we may want to put behind this flag.
Open to suggestions
There was a problem hiding this comment.
enableSensitiveTraceLogging?
There was a problem hiding this comment.
In tandem with my feedback above about LogSigning controlling all signing logging, I think this should just be called enableLogging.
| { | ||
| "id": "b1a1bee4-e3e4-4520-ae8e-48d3b0e94ae9", | ||
| "type": "bugfix", | ||
| "description": "Do not log intermediate signature calculations without explicit opt-in via `LogMode.LogSignging`." |
| @@ -164,6 +164,14 @@ public class AwsSigningConfig(builder: Builder) { | |||
| */ | |||
| public val expiresAfter: Duration? = builder.expiresAfter | |||
|
|
|||
| /** | |||
| * Flag enabling whether detailed trace logging is enabled (if the signer implementation supports it). When true | |||
| * signers should emit intermediate logging details such as the canonical request and string to sign at the trace | |||
There was a problem hiding this comment.
and string to sign
It looks the string to sign always gets logged (which I think is what we agreed to do). So should we only reference the canonical request in these docs?
There was a problem hiding this comment.
I'd go the other way, actually. The SdkLogMode option is named LogSigning. At a glance, that indicates to me that all signing logging will be enabled/disabled by that flag. (The KDocs add some nuance but I think they actually undermine the name.)
I think we should just make that the case: canonical request, string to sign, and final signature should be gated by LogSigning.
| * level. This is an opt-in configuration because these intermediate outputs may contain sensitive fields bound | ||
| * to headers, URI, or query parmaeters. | ||
| */ | ||
| public val enableTraceLogging: Boolean = builder.enableTraceLogging |
There was a problem hiding this comment.
I like this name if we were controlling all logger.trace calls with it, but we always log the "string to sign" at the trace level regardless of this flag. I think we should either rename this to better describe exactly what gets logged, or block the string-to-sign logging with this flag too.
| @@ -33,7 +33,7 @@ class LogModeTest { | |||
| fun testToString() { | |||
| val mode = LogMode.allModes().reduce { acc, curr -> acc + curr } | |||
| assertTrue { LogMode.allModes().all { mode.isEnabled(it) } } | |||
| val expected = "LogRequest|LogRequestWithBody|LogResponse|LogResponseWithBody" | |||
| val expected = "LogRequest|LogRequestWithBody|LogResponse|LogResponseWithBody|LogSigning" | |||
There was a problem hiding this comment.
Could you also add this value to the composite string test on L46? (tests the reverse of this, String->LogMode)
gradle.properties
Outdated
| sdkVersion=0.27.8-SNAPSHOT | ||
| sdkVersion=0.28.0-SNAPSHOT |
There was a problem hiding this comment.
Question: Why did this necessitate a major version bump?
There was a problem hiding this comment.
It didn't, it fixes a previous commit where it should have been bumped: 681379d
There was a problem hiding this comment.
I was expecting the nullability PR to land with everything else which bumped the version, that won't land until next week (hopefully with the name changes which would have also bumped it)
There was a problem hiding this comment.
Blerg, it's causing downstream CI issues though so I'm reverting. @lauzadis we just need to remember to bump this before release.
| @@ -164,6 +164,14 @@ public class AwsSigningConfig(builder: Builder) { | |||
| */ | |||
| public val expiresAfter: Duration? = builder.expiresAfter | |||
|
|
|||
| /** | |||
| * Flag enabling whether detailed trace logging is enabled (if the signer implementation supports it). When true | |||
| * signers should emit intermediate logging details such as the canonical request and string to sign at the trace | |||
There was a problem hiding this comment.
I'd go the other way, actually. The SdkLogMode option is named LogSigning. At a glance, that indicates to me that all signing logging will be enabled/disabled by that flag. (The KDocs add some nuance but I think they actually undermine the name.)
I think we should just make that the case: canonical request, string to sign, and final signature should be gated by LogSigning.
| * level. This is an opt-in configuration because these intermediate outputs may contain sensitive fields bound | ||
| * to headers, URI, or query parmaeters. | ||
| */ | ||
| public val enableTraceLogging: Boolean = builder.enableTraceLogging |
There was a problem hiding this comment.
In tandem with my feedback above about LogSigning controlling all signing logging, I think this should just be called enableLogging.
|
Kudos, SonarCloud Quality Gate passed!
|
Issue #
n/a
Description of changes
We gate logging HTTP requests and responses because headers and query parameters (and the body) could have sensitive information bound to them from the model. We currently log canonical request during signing which may contain many of the same details (headers, query params, etc). This PR adds
LogMode.LogSigningwhich gates logging these details unless explicitly opted into.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.