Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: gate logging intermediate signing results which may contain sensitive information #984

Merged
merged 5 commits into from
Oct 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changes/b1a1bee4-e3e4-4520-ae8e-48d3b0e94ae9.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"id": "b1a1bee4-e3e4-4520-ae8e-48d3b0e94ae9",
"type": "bugfix",
"description": "Do not log intermediate signature calculations without explicit opt-in via `LogMode.LogSigning`."
}
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ kotlin.native.ignoreDisabledTargets=true
sdkVersion=0.27.8-SNAPSHOT

# kotlin
kotlinVersion=1.8.22
kotlinVersion=1.8.22
3 changes: 3 additions & 0 deletions runtime/auth/aws-signing-common/api/aws-signing-common.api
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public final class aws/smithy/kotlin/runtime/auth/awssigning/AwsSigningConfig {
public final fun getCredentials ()Laws/smithy/kotlin/runtime/auth/awscredentials/Credentials;
public final fun getExpiresAfter-FghU774 ()Lkotlin/time/Duration;
public final fun getHashSpecification ()Laws/smithy/kotlin/runtime/auth/awssigning/HashSpecification;
public final fun getLogRequest ()Z
public final fun getNormalizeUriPath ()Z
public final fun getOmitSessionToken ()Z
public final fun getRegion ()Ljava/lang/String;
Expand All @@ -69,6 +70,7 @@ public final class aws/smithy/kotlin/runtime/auth/awssigning/AwsSigningConfig$Bu
public final fun getCredentials ()Laws/smithy/kotlin/runtime/auth/awscredentials/Credentials;
public final fun getExpiresAfter-FghU774 ()Lkotlin/time/Duration;
public final fun getHashSpecification ()Laws/smithy/kotlin/runtime/auth/awssigning/HashSpecification;
public final fun getLogRequest ()Z
public final fun getNormalizeUriPath ()Z
public final fun getOmitSessionToken ()Z
public final fun getRegion ()Ljava/lang/String;
Expand All @@ -82,6 +84,7 @@ public final class aws/smithy/kotlin/runtime/auth/awssigning/AwsSigningConfig$Bu
public final fun setCredentials (Laws/smithy/kotlin/runtime/auth/awscredentials/Credentials;)V
public final fun setExpiresAfter-BwNAW2A (Lkotlin/time/Duration;)V
public final fun setHashSpecification (Laws/smithy/kotlin/runtime/auth/awssigning/HashSpecification;)V
public final fun setLogRequest (Z)V
public final fun setNormalizeUriPath (Z)V
public final fun setOmitSessionToken (Z)V
public final fun setRegion (Ljava/lang/String;)V
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 at the trace level.
* This is an opt-in configuration because these intermediate outputs may contain sensitive fields bound
* to headers, URI, or query parameters.
*/
public val logRequest: Boolean = builder.logRequest

public fun toBuilder(): Builder = Builder().also {
it.region = region
it.service = service
Expand All @@ -178,6 +186,7 @@ public class AwsSigningConfig(builder: Builder) {
it.signedBodyHeader = signedBodyHeader
it.credentials = credentials
it.expiresAfter = expiresAfter
it.logRequest = logRequest
}

public class Builder {
Expand All @@ -194,6 +203,7 @@ public class AwsSigningConfig(builder: Builder) {
public var signedBodyHeader: AwsSignedBodyHeader = AwsSignedBodyHeader.NONE
public var credentials: Credentials? = null
public var expiresAfter: Duration? = null
public var logRequest: Boolean = false

public fun build(): AwsSigningConfig = AwsSigningConfig(this)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ internal class DefaultAwsSignerImpl(
require(config.algorithm == AwsSigningAlgorithm.SIGV4) { "${config.algorithm} support is not yet implemented" }

val canonical = canonicalizer.canonicalRequest(request, config)
logger.trace { "Canonical request:\n${canonical.requestString}" }
if (config.logRequest) {
logger.trace { "Canonical request:\n${canonical.requestString}" }
}

val stringToSign = signatureCalculator.stringToSign(canonical.requestString, config)
logger.trace { "String to sign:\n$stringToSign" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import aws.smithy.kotlin.runtime.auth.awssigning.internal.isEligibleForAwsChunke
import aws.smithy.kotlin.runtime.auth.awssigning.internal.setAwsChunkedBody
import aws.smithy.kotlin.runtime.auth.awssigning.internal.setAwsChunkedHeaders
import aws.smithy.kotlin.runtime.auth.awssigning.internal.useAwsChunkedEncoding
import aws.smithy.kotlin.runtime.client.LogMode
import aws.smithy.kotlin.runtime.client.SdkClientOption
import aws.smithy.kotlin.runtime.http.HttpBody
import aws.smithy.kotlin.runtime.http.operation.HttpOperationContext
import aws.smithy.kotlin.runtime.http.request.HttpRequest
Expand Down Expand Up @@ -138,6 +140,7 @@ public class AwsHttpSigner(private val config: Config) : HttpSigner {
shouldSignHeader = config.shouldSignHeader

signedBodyHeader = contextSignedBodyHeader ?: config.signedBodyHeader
logRequest = attributes.getOrNull(SdkClientOption.LogMode)?.isEnabled(LogMode.LogRequest) == true

// SDKs are supposed to default to signed payload _always_ when possible (and when `unsignedPayload` trait
// isn't present). The only exception is when the customer explicitly disables signed payloads (via Config.isUnsignedPayload).
Expand Down
Loading