Skip to content

Commit

Permalink
Fix maps/lists with sensitive members not redacted (#3765)
Browse files Browse the repository at this point in the history
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
Lists and maps with sensitive members were not being properly redacted.
Closes #3757

## Description
<!--- Describe your changes in detail -->
Updated `Shape.shouldRedact` method to properly check for list and map
shapes with sensitive members. Also updated the test definition so it
would actually run since previously the test code was generated in a
nested function inside a no-op function and never run.

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
Added test cases for lists with sensitive members, maps with sensitive
keys, and maps with sensitive values.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates


----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
  • Loading branch information
landonxjames authored Jul 17, 2024
1 parent c63e792 commit c39792b
Show file tree
Hide file tree
Showing 6 changed files with 122 additions and 31 deletions.
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

0 comments on commit c39792b

Please sign in to comment.