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 maps/lists with sensitive members not redacted #3765

Merged
merged 5 commits into from
Jul 17, 2024
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
38 changes: 37 additions & 1 deletion CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,40 @@
# message = "Fix typos in module documentation for generated crates"
# references = ["smithy-rs#920"]
# meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"}
# author = "rcoh"
# author = "rcoh"

[[smithy-rs]]
message = "Support `stringArray` type in endpoints params"
references = ["smithy-rs#3742"]
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client"}
author = "landonxjames"

[[smithy-rs]]
message = "Add support for `operationContextParams` Endpoints trait"
references = ["smithy-rs#3755"]
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client"}
author = "landonxjames"

[[aws-sdk-rust]]
message = "`aws_smithy_runtime_api::client::orchestrator::HttpRequest` and `aws_smithy_runtime_api::client::orchestrator::HttpResponse` are now re-exported in AWS SDK clients so that using these types does not require directly depending on `aws-smithy-runtime-api`."
references = ["smithy-rs#3591"]
meta = { "breaking" = false, "tada" = false, "bug" = false }
author = "ysaito1001"

[[smithy-rs]]
message = "`aws_smithy_runtime_api::client::orchestrator::HttpRequest` and `aws_smithy_runtime_api::client::orchestrator::HttpResponse` are now re-exported in generated clients so that using these types does not require directly depending on `aws-smithy-runtime-api`."
references = ["smithy-rs#3591"]
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client" }
author = "ysaito1001"

[[aws-sdk-rust]]
message = "Fix incorrect redaction of `@sensitive` types in maps and lists."
references = ["smithy-rs#3765", "smithy-rs#3757"]
meta = { "breaking" = false, "tada" = false, "bug" = true }
author = "landonxjames"

[[smithy-rs]]
message = "Fix incorrect redaction of `@sensitive` types in maps and lists."
references = ["smithy-rs#3765", "smithy-rs#3757"]
meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client" }
author = "landonxjames"
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,12 @@ import software.amazon.smithy.rust.codegen.core.smithy.isOptional
import software.amazon.smithy.rust.codegen.core.smithy.makeOptional
import software.amazon.smithy.rust.codegen.core.smithy.rustType
import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticInputTrait
import software.amazon.smithy.rust.codegen.core.util.REDACTION
import software.amazon.smithy.rust.codegen.core.util.dq
import software.amazon.smithy.rust.codegen.core.util.hasTrait
import software.amazon.smithy.rust.codegen.core.util.letIf
import software.amazon.smithy.rust.codegen.core.util.redactIfNecessary
import software.amazon.smithy.rust.codegen.core.util.shouldRedact
import software.amazon.smithy.rust.codegen.core.util.toSnakeCase

// TODO(https://github.com/smithy-lang/smithy-rs/issues/1401) This builder generator is only used by the client.
Expand Down Expand Up @@ -353,7 +355,16 @@ class BuilderGenerator(
rust("""let mut formatter = f.debug_struct(${builderName.dq()});""")
members.forEach { member ->
val memberName = symbolProvider.toMemberName(member)
val fieldValue = member.redactIfNecessary(model, "self.$memberName")
// If the struct is marked sensitive all fields get redacted, otherwise each field is determined on its own
val fieldValue =
if (shape.shouldRedact(model)) {
REDACTION
} else {
member.redactIfNecessary(
model,
"self.$memberName",
)
}

rust(
"formatter.field(${memberName.dq()}, &$fieldValue);",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,11 @@ import software.amazon.smithy.rust.codegen.core.smithy.customize.writeCustomizat
import software.amazon.smithy.rust.codegen.core.smithy.expectRustMetadata
import software.amazon.smithy.rust.codegen.core.smithy.renamedFrom
import software.amazon.smithy.rust.codegen.core.smithy.rustType
import software.amazon.smithy.rust.codegen.core.util.REDACTION
import software.amazon.smithy.rust.codegen.core.util.dq
import software.amazon.smithy.rust.codegen.core.util.getTrait
import software.amazon.smithy.rust.codegen.core.util.redactIfNecessary
import software.amazon.smithy.rust.codegen.core.util.shouldRedact

/** StructureGenerator customization sections */
sealed class StructureSection(name: String) : Section(name) {
Expand Down Expand Up @@ -102,9 +104,19 @@ open class StructureGenerator(
) {
writer.rustBlock("fn fmt(&self, f: &mut #1T::Formatter<'_>) -> #1T::Result", RuntimeType.stdFmt) {
rust("""let mut formatter = f.debug_struct(${name.dq()});""")

members.forEach { member ->
val memberName = symbolProvider.toMemberName(member)
val fieldValue = member.redactIfNecessary(model, "self.$memberName")
// If the struct is marked sensitive all fields get redacted, otherwise each field is determined on its own
val fieldValue =
if (shape.shouldRedact(model)) {
REDACTION
} else {
member.redactIfNecessary(
model,
"self.$memberName",
)
}

rust(
"formatter.field(${memberName.dq()}, &$fieldValue);",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import software.amazon.smithy.aws.traits.ServiceTrait
import software.amazon.smithy.codegen.core.CodegenException
import software.amazon.smithy.model.Model
import software.amazon.smithy.model.shapes.BooleanShape
import software.amazon.smithy.model.shapes.ListShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.MemberShape
import software.amazon.smithy.model.shapes.NumberShape
import software.amazon.smithy.model.shapes.OperationShape
Expand Down Expand Up @@ -84,13 +86,12 @@ fun ServiceShape.hasEventStreamOperations(model: Model): Boolean =
}

fun Shape.shouldRedact(model: Model): Boolean =
when (this) {
is MemberShape ->
model.expectShape(target).shouldRedact(model) ||
model.expectShape(container)
.shouldRedact(model)

else -> this.hasTrait<SensitiveTrait>()
when {
hasTrait<SensitiveTrait>() -> true
this is MemberShape -> model.expectShape(target).shouldRedact(model)
this is ListShape -> member.shouldRedact(model)
this is MapShape -> key.shouldRedact(model) || value.shouldRedact(model)
else -> false
}

const val REDACTION = "\"*** Sensitive Data Redacted ***\""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ internal class BuilderGeneratorTest {
.username("admin")
.password("pswd")
.secret_key("12345");
assert_eq!(format!("{:?}", builder), "Builder { username: Some(\"admin\"), password: \"*** Sensitive Data Redacted ***\", secret_key: \"*** Sensitive Data Redacted ***\" }");
assert_eq!(format!("{:?}", builder),
"Builder { username: Some(\"admin\"), password: \"*** Sensitive Data Redacted ***\", secret_key: \"*** Sensitive Data Redacted ***\", secret_value_map: \"*** Sensitive Data Redacted ***\", secret_key_map: \"*** Sensitive Data Redacted ***\", secret_list: \"*** Sensitive Data Redacted ***\" }");
""",
)
}
Expand Down Expand Up @@ -278,18 +279,21 @@ internal class BuilderGeneratorTest {
implBlock(provider.toSymbol(inner)) {
BuilderGenerator.renderConvenienceMethod(this, provider, inner)
}
unitTest("test", additionalAttributes = listOf(Attribute.DenyDeprecated), block = {
rust(
// Notice that the builder is instantiated directly, and not through the Inner::builder() method.
// This is because Inner is marked with `deprecated`, so any usage of `Inner` inside the test will
// fail the compilation.
//
// This piece of code would fail though if the Builder inherits the attributes from Inner.
"""
let _ = test_inner::Builder::default();
""",
)
})
unitTest(
"test", additionalAttributes = listOf(Attribute.DenyDeprecated),
block = {
rust(
// Notice that the builder is instantiated directly, and not through the Inner::builder() method.
// This is because Inner is marked with `deprecated`, so any usage of `Inner` inside the test will
// fail the compilation.
//
// This piece of code would fail though if the Builder inherits the attributes from Inner.
"""
let _ = test_inner::Builder::default();
""",
)
},
)
}
project.withModule(provider.moduleForBuilder(inner)) {
BuilderGenerator(model, provider, inner, emptyList()).render(this)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,10 @@ class StructureGeneratorTest {
structure Credentials {
username: String,
password: Password,

secretKey: SecretKey
secretKey: SecretKey,
secretValueMap: MapThatContainsSecretValues,
secretKeyMap: MapThatContainsSecretKeys,
secretList: ListThatContainsSecrets
}

structure StructWithDoc {
Expand All @@ -79,6 +81,20 @@ class StructureGeneratorTest {
public: String,
private: SecretStructure,
}

map MapThatContainsSecretKeys {
key: SecretKey
value: String
}

map MapThatContainsSecretValues {
key: String
value: SecretKey
}

list ListThatContainsSecrets {
member: Password
}
""".asSmithyModel()
val struct = model.lookup<StructureShape>("com.test#MyStruct")
val structWithDoc = model.lookup<StructureShape>("com.test#StructWithDoc")
Expand Down Expand Up @@ -159,15 +175,27 @@ class StructureGeneratorTest {
TestWorkspace.testProject().unitTest {
structureGenerator(model, provider, this, credentials).render()

this.unitTest(
"sensitive_fields_redacted",
rust(
"""
use std::collections::HashMap;

let mut secret_map = HashMap::new();
secret_map.insert("FirstSecret".to_string(), "don't leak me".to_string());
secret_map.insert("SecondSecret".to_string(), "don't leak me".to_string());

let secret_list = vec!["don't leak me".to_string()];

let creds = Credentials {
username: Some("not_redacted".to_owned()),
password: Some("don't leak me".to_owned()),
secret_key: Some("don't leak me".to_owned())
secret_key: Some("don't leak me".to_owned()),
secret_key_map: Some(secret_map.clone()),
secret_value_map: Some(secret_map),
secret_list: Some(secret_list),
};
assert_eq!(format!("{:?}", creds), "Credentials { username: Some(\"not_redacted\"), password: \"*** Sensitive Data Redacted ***\", secret_key: \"*** Sensitive Data Redacted ***\" }");

assert_eq!(format!("{:?}", creds),
"Credentials { username: Some(\"not_redacted\"), password: \"*** Sensitive Data Redacted ***\", secret_key: \"*** Sensitive Data Redacted ***\", secret_value_map: \"*** Sensitive Data Redacted ***\", secret_key_map: \"*** Sensitive Data Redacted ***\", secret_list: \"*** Sensitive Data Redacted ***\" }");
""",
)
}.compileAndTest()
Expand All @@ -179,8 +207,7 @@ class StructureGeneratorTest {
TestWorkspace.testProject().unitTest {
structureGenerator(model, provider, this, secretStructure).render()

this.unitTest(
"sensitive_structure_redacted",
rust(
"""
let secret_structure = SecretStructure {
secret_field: Some("secret".to_owned()),
Expand Down