diff --git a/build-logic/tag-registry/src/test/kotlin/datadog/buildlogic/tagRegistry/TagRegistryGeneratorTest.kt b/build-logic/tag-registry/src/test/kotlin/datadog/buildlogic/tagRegistry/TagRegistryGeneratorTest.kt index a342886371c..c2628558f25 100644 --- a/build-logic/tag-registry/src/test/kotlin/datadog/buildlogic/tagRegistry/TagRegistryGeneratorTest.kt +++ b/build-logic/tag-registry/src/test/kotlin/datadog/buildlogic/tagRegistry/TagRegistryGeneratorTest.kt @@ -799,6 +799,50 @@ class TagRegistryGeneratorTest { .withMessageContaining("coloring needs more than 63 slots") } + @Test + fun `a tag declared on two sibling span types that share a parent direction is rejected`() { + // Two concrete siblings extend one parent, so they share its direction. Declaring the same tag + // on each is two same-direction declarations of one name, which has no single identity. + val yaml = directory.conventionsFile( + """ + span_types: + rpc.client: + abstract: true + span-kind: client + rpc.a: + extends: rpc.client + tags: [{dd-name: account, required: optional}] + rpc.b: + extends: rpc.client + tags: [{dd-name: account, required: optional}] + """ + ) + + assertThatIllegalArgumentException() + .isThrownBy { TagRegistryGenerator.generate(yaml, File(directory, "generated")) } + .withMessageContaining("declared in both 'rpc.a' and 'rpc.b'") + } + + @Test + fun `declaring the shared tag once on the parent lets both siblings inherit one identity`() { + val conventions = tagConventions( + """ + span_types: + rpc.client: + abstract: true + span-kind: client + tags: [{dd-name: account, required: optional}] + rpc.a: {extends: rpc.client} + rpc.b: {extends: rpc.client} + """ + ) + + assertThat(conventions.resolve("rpc.a").map { it.name }).contains("account") + assertThat(conventions.resolve("rpc.b").map { it.name }).contains("account") + // One declaration, so one tag identity shared by both siblings. + assertThat(TagRegistry.build(conventions).tags.filter { it.name == "account" }).hasSize(1) + } + private fun tagConventions(@Language("yaml") yamlText: String) = TagConventions.parse( ObjectMapper(YAMLFactory()).readValue( directory.conventionsFile(yamlText), diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-common/src/main/java/datadog/trace/instrumentation/aws/AwsAccountIdentity.java b/dd-java-agent/instrumentation/aws-java/aws-java-common/src/main/java/datadog/trace/instrumentation/aws/AwsAccountIdentity.java new file mode 100644 index 00000000000..3316f203569 --- /dev/null +++ b/dd-java-agent/instrumentation/aws-java/aws-java-common/src/main/java/datadog/trace/instrumentation/aws/AwsAccountIdentity.java @@ -0,0 +1,39 @@ +package datadog.trace.instrumentation.aws; + +/** + * Account identity of resources addressed by AWS SDK requests. + * + *

The {@code aws_account} span tag always identifies the owner of the addressed resource, not + * the caller in general. The account that owns the signing credentials may be used only when the + * service contract guarantees that it also owns the addressed resource. + * + *

Two sources are trustworthy and used by the SDK instrumentations: + * + *

+ */ +public final class AwsAccountIdentity { + + private AwsAccountIdentity() {} + + /** {@code true} for exactly twelve ASCII digits. */ + public static boolean isAccountId(final String value) { + if (value == null || value.length() != 12) { + return false; + } + for (int i = 0; i < 12; i++) { + char c = value.charAt(i); + if (c < '0' || c > '9') { + return false; + } + } + return true; + } +} diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-common/src/main/java/datadog/trace/instrumentation/aws/AwsArn.java b/dd-java-agent/instrumentation/aws-java/aws-java-common/src/main/java/datadog/trace/instrumentation/aws/AwsArn.java new file mode 100644 index 00000000000..0bb3f5f62ea --- /dev/null +++ b/dd-java-agent/instrumentation/aws-java/aws-java-common/src/main/java/datadog/trace/instrumentation/aws/AwsArn.java @@ -0,0 +1,107 @@ +package datadog.trace.instrumentation.aws; + +/** + * A parsed Amazon Resource Name: {@code arn:::::}. + * + *

Parsed once so callers that need several fields do not re-scan the string. The AWS SDK ships + * its own parser ({@code software.amazon.awssdk.arns.Arn}) but it lives in a module that neither + * the SDK v2 2.2.0 floor these instrumentations compile against nor SDK v1 provide, so a minimal + * one is kept here. + */ +public final class AwsArn { + + private final String raw; + private final String partition; + private final String service; + private final String region; + private final String account; + private final String resource; + + private AwsArn( + final String raw, + final String partition, + final String service, + final String region, + final String account, + final String resource) { + this.raw = raw; + this.partition = partition; + this.service = service; + this.region = region; + this.account = account; + this.resource = resource; + } + + /** + * Parses an ARN. + * + * @return the parsed ARN, or {@code null} when the value is not of the form {@code + * arn:partition:service:region:account:resource} (partition and service non-empty). + */ + public static AwsArn parse(final String value) { + if (value == null || !value.startsWith("arn:")) { + return null; + } + int c1 = value.indexOf(':', 4); + if (c1 < 0) { + return null; + } + int c2 = value.indexOf(':', c1 + 1); + if (c2 < 0) { + return null; + } + int c3 = value.indexOf(':', c2 + 1); + if (c3 < 0) { + return null; + } + int c4 = value.indexOf(':', c3 + 1); + if (c4 < 0) { + return null; + } + if (c1 == 4 || c2 == c1 + 1 || c4 == value.length() - 1) { + // empty partition, empty service or empty resource + return null; + } + return new AwsArn( + value, + value.substring(4, c1), + value.substring(c1 + 1, c2), + emptyToNull(value.substring(c2 + 1, c3)), + emptyToNull(value.substring(c3 + 1, c4)), + value.substring(c4 + 1)); + } + + /** The original string. */ + public String raw() { + return raw; + } + + /** {@code aws}, {@code aws-cn}, {@code aws-us-gov}, ... */ + public String partition() { + return partition; + } + + /** {@code dynamodb}, {@code sns}, {@code s3}, ... */ + public String service() { + return service; + } + + /** The Region, or {@code null} for global services (S3, IAM). */ + public String region() { + return region; + } + + /** The 12-digit account, or {@code null} when absent (S3 ARNs) or not exactly twelve digits. */ + public String account() { + return AwsAccountIdentity.isAccountId(account) ? account : null; + } + + /** Everything after the fifth colon. Never empty. */ + public String resource() { + return resource; + } + + private static String emptyToNull(final String value) { + return value.isEmpty() ? null : value; + } +} diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-common/src/test/java/datadog/trace/instrumentation/aws/AwsAccountIdentityTest.java b/dd-java-agent/instrumentation/aws-java/aws-java-common/src/test/java/datadog/trace/instrumentation/aws/AwsAccountIdentityTest.java new file mode 100644 index 00000000000..6f53271b95e --- /dev/null +++ b/dd-java-agent/instrumentation/aws-java/aws-java-common/src/test/java/datadog/trace/instrumentation/aws/AwsAccountIdentityTest.java @@ -0,0 +1,25 @@ +package datadog.trace.instrumentation.aws; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; + +class AwsAccountIdentityTest { + + @ParameterizedTest(name = "{0} is an account id") + @ValueSource(strings = {"123456789012", "000000000000", "999999999999"}) + void acceptsTwelveDigits(String value) { + assertTrue(AwsAccountIdentity.isAccountId(value)); + } + + @ParameterizedTest(name = "{0} is not an account id") + @NullAndEmptySource + @ValueSource( + strings = {"12345", "1234567890123", "12345678901a", " 23456789012", "arn:aws:s3:::b"}) + void rejectsNonAccountIds(String value) { + assertFalse(AwsAccountIdentity.isAccountId(value)); + } +} diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-common/src/test/java/datadog/trace/instrumentation/aws/AwsArnTest.java b/dd-java-agent/instrumentation/aws-java/aws-java-common/src/test/java/datadog/trace/instrumentation/aws/AwsArnTest.java new file mode 100644 index 00000000000..7a123c84441 --- /dev/null +++ b/dd-java-agent/instrumentation/aws-java/aws-java-common/src/test/java/datadog/trace/instrumentation/aws/AwsArnTest.java @@ -0,0 +1,75 @@ +package datadog.trace.instrumentation.aws; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; + +class AwsArnTest { + + @ParameterizedTest(name = "parses {0}") + @CsvSource( + nullValues = "NULL", + value = { + "arn:aws:dynamodb:us-east-1:123456789012:table/orders, aws, dynamodb, us-east-1, 123456789012, table/orders", + "arn:aws:dynamodb:us-east-1:123456789012:table/orders/index/by-user, aws, dynamodb, us-east-1, 123456789012, table/orders/index/by-user", + "arn:aws-cn:dynamodb:cn-north-1:123456789012:table/orders, aws-cn, dynamodb, cn-north-1, 123456789012, table/orders", + "arn:aws-us-gov:sns:us-gov-west-1:123456789012:alerts, aws-us-gov, sns, us-gov-west-1, 123456789012, alerts", + "arn:aws:s3:::my-bucket, aws, s3, NULL, NULL, my-bucket", + "arn:aws:iam::123456789012:role/app, aws, iam, NULL, 123456789012, role/app", + "arn:aws:states:us-east-1:123456789012:stateMachine:orders:extra:colons, aws, states, us-east-1, 123456789012, stateMachine:orders:extra:colons", + }) + void parsesFields( + String value, + String partition, + String service, + String region, + String account, + String resource) { + AwsArn arn = AwsArn.parse(value); + assertNotNull(arn); + assertEquals(value, arn.raw()); + assertEquals(partition, arn.partition()); + assertEquals(service, arn.service()); + assertEquals(region, arn.region()); + assertEquals(account, arn.account()); + assertEquals(resource, arn.resource()); + } + + @ParameterizedTest(name = "rejects {0}") + @NullAndEmptySource + @ValueSource( + strings = { + "orders", + "arn:", + "arn:aws", + "arn:aws:dynamodb", + "arn:aws:dynamodb:us-east-1", + "arn:aws:dynamodb:us-east-1:123456789012", + "arn:aws:dynamodb:us-east-1:123456789012:", + "arn::dynamodb:us-east-1:123456789012:table/orders", + "arn:aws::us-east-1:123456789012:table/orders", + "table/orders", + "https://sqs.us-east-1.amazonaws.com/123456789012/q", + }) + void rejectsNonArn(String value) { + assertNull(AwsArn.parse(value)); + } + + @ParameterizedTest(name = "account of {0} is null") + @ValueSource( + strings = { + "arn:aws:dynamodb:us-east-1:12345:table/orders", + "arn:aws:dynamodb:us-east-1:12345678901a:table/orders", + "arn:aws:dynamodb:us-east-1::table/orders", + }) + void accountMustBeTwelveDigits(String value) { + AwsArn arn = AwsArn.parse(value); + assertNotNull(arn); + assertNull(arn.account()); + } +} diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/build.gradle b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/build.gradle index 54b2075ef14..a03273a4961 100644 --- a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/build.gradle +++ b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/build.gradle @@ -38,6 +38,7 @@ addTestSuiteExtendingForDir('latestDsmForkedTest', 'latestDsmTest', 'dsmTest') dependencies { compileOnly group: 'com.amazonaws', name: 'aws-java-sdk-core', version: '1.11.0' + implementation project(':dd-java-agent:instrumentation:aws-java:aws-java-common') // Include httpclient instrumentation for testing because it is a dependency for aws-sdk. testImplementation project(':dd-java-agent:instrumentation:apache-httpclient:apache-httpclient-4.0') diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/AwsSdkClientDecorator.java b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/AwsSdkClientDecorator.java index 5e0e388c1ed..61aaccdf40d 100644 --- a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/AwsSdkClientDecorator.java +++ b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/AwsSdkClientDecorator.java @@ -26,6 +26,8 @@ import datadog.trace.bootstrap.instrumentation.api.Tags; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; import datadog.trace.bootstrap.instrumentation.decorator.HttpClientDecorator; +import datadog.trace.instrumentation.aws.AwsAccountIdentity; +import datadog.trace.instrumentation.aws.AwsArn; import java.net.URI; import java.util.List; import java.util.Locale; @@ -182,6 +184,21 @@ protected void doOnRequest(final AgentSpan span, final Request request) { } String tableName = access.getTableName(originalRequest); if (null != tableName) { + // A DynamoDB TableName given as an ARN carries the owning account. SDK v1 does not expose + // the signing credentials to request handlers, so a bare name yields no account here. The + // existing table name tags keep the value as given, ARN or not. Other services with a + // TableName member (Timestream, Keyspaces, ...) only get the plain table name tags. + AwsArn arn = + awsSimplifiedServiceName != null && awsSimplifiedServiceName.startsWith("dynamodb") + ? AwsArn.parse(tableName) + : null; + if (arn != null) { + span.setTag(InstrumentationTags.AWS_TABLE_ARN, arn.raw()); + String account = arn.account(); + if (account != null) { + span.setTag(InstrumentationTags.AWS_ACCOUNT, account); + } + } span.setTag(InstrumentationTags.AWS_TABLE_NAME, tableName); span.setTag(InstrumentationTags.TABLE_NAME, tableName); bestPrecursor = InstrumentationTags.AWS_TABLE_NAME; @@ -264,6 +281,23 @@ protected void doOnRequest(final AgentSpan span, final Request request) { } } + /** + * Tags derived from the request that are only trustworthy once the service accepted it. S3 + * enforces {@code ExpectedBucketOwner} (403 on mismatch), so the asserted owner names the bucket + * owner only on a successful response. + */ + public void onSuccessfulRequest(final AgentSpan span, final Request request) { + if (!"s3".equalsIgnoreCase(simplifyServiceName(request.getServiceName()))) { + return; + } + final AmazonWebServiceRequest originalRequest = request.getOriginalRequest(); + final String expectedBucketOwner = + GetterAccess.of(originalRequest).getExpectedBucketOwner(originalRequest); + if (AwsAccountIdentity.isAccountId(expectedBucketOwner)) { + span.setTag(InstrumentationTags.AWS_ACCOUNT, expectedBucketOwner); + } + } + /** * Normalizes a Lambda {@code FunctionName} request value. {@code aws.function.name} is always the * bare function name (account, {@code function:} prefix and {@code :qualifier} suffix stripped), diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/GetterAccess.java b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/GetterAccess.java index 8ff4ff87972..e14ad193ab2 100644 --- a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/GetterAccess.java +++ b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/GetterAccess.java @@ -39,6 +39,7 @@ static GetterAccess of(final Object request) { private final MethodHandle getPublishBatchRequestEntries; private final MethodHandle getApproximateArrivalTimestamp; private final MethodHandle getTableName; + private final MethodHandle getExpectedBucketOwner; private final MethodHandle getStateMachineArn; private final MethodHandle getExecutionArn; private final MethodHandle getFunctionName; @@ -58,6 +59,7 @@ private GetterAccess(final Class objectType) { getApproximateArrivalTimestamp = findGetter(objectType, "getApproximateArrivalTimestamp", Date.class); getTableName = findStringGetter(objectType, "getTableName"); + getExpectedBucketOwner = findStringGetter(objectType, "getExpectedBucketOwner"); getStateMachineArn = findStringGetter(objectType, "getStateMachineArn"); getExecutionArn = findStringGetter(objectType, "getExecutionArn"); getFunctionName = findStringGetter(objectType, "getFunctionName"); @@ -107,6 +109,10 @@ String getTableName(final Object object) { return invokeForString(getTableName, object); } + String getExpectedBucketOwner(final Object object) { + return invokeForString(getExpectedBucketOwner, object); + } + String getStateMachineArn(final Object object) { return invokeForString(getStateMachineArn, object); } diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/TracingRequestHandler.java b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/TracingRequestHandler.java index 3282f0f9a7d..b6057cfcd31 100644 --- a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/TracingRequestHandler.java +++ b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/main/java/datadog/trace/instrumentation/aws/v0/TracingRequestHandler.java @@ -96,6 +96,7 @@ public void afterResponse(final Request request, final Response response) span = AgentSpan.fromContext(context); if (span != null) { DECORATE.onResponse(span, response); + DECORATE.onSuccessfulRequest(span, request); DECORATE.onServiceResponse(span, request.getServiceName(), response); DECORATE.beforeFinish(span); span.finish(); diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/test/groovy/AWS1ClientTest.groovy b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/test/groovy/AWS1ClientTest.groovy index 31dfa891461..c5c54d690cc 100644 --- a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/test/groovy/AWS1ClientTest.groovy +++ b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-1.11/src/test/groovy/AWS1ClientTest.groovy @@ -215,6 +215,7 @@ abstract class AWS1ClientTest extends VersionedNamingTestBase { "S3" | "GetObject" | "GET" | "/someBucket/someKey" | AmazonS3ClientBuilder.standard().withPathStyleAccessEnabled(true).withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.getObject("someBucket", "someKey") } | ["aws.bucket.name": "someBucket", "bucketname": "someBucket", "aws.object.key": "someKey"] | "" | "aws.bucket.name" | null "DynamoDBv2" | "CreateTable" | "POST" | "/" | AmazonDynamoDBClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createTable(new CreateTableRequest("sometable", null)) } | ["aws.table.name": "sometable", "tablename": "sometable"] | "" | "aws.table.name" | null "DynamoDBv2" | "GetItem" | "POST" | "/" | AmazonDynamoDBClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.getItem(new GetItemRequest("sometable", ["attribute": new AttributeValue("somevalue")])) } | ["aws.table.name": "sometable", "tablename": "sometable"] | "" | "aws.table.name" | null + "DynamoDBv2" | "GetItem" | "POST" | "/" | AmazonDynamoDBClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.getItem(new GetItemRequest("arn:aws:dynamodb:us-east-1:123456789012:table/sometable", ["attribute": new AttributeValue("somevalue")])) } | ["aws.table.name": "arn:aws:dynamodb:us-east-1:123456789012:table/sometable", "tablename": "arn:aws:dynamodb:us-east-1:123456789012:table/sometable", "aws.table.arn": "arn:aws:dynamodb:us-east-1:123456789012:table/sometable", "aws_account": "123456789012"] | "" | "aws.table.name" | null "Kinesis" | "DeleteStream" | "POST" | "/" | AmazonKinesisClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.deleteStream(new DeleteStreamRequest().withStreamName("somestream")) } | ["aws.stream.name": "somestream", "streamname": "somestream"] | "" | "aws.stream.name" | null "AWSStepFunctions" | "StartExecution" | "POST" | "/" | AWSStepFunctionsClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.startExecution(new StartExecutionRequest().withStateMachineArn("arn:aws:states:us-east-1:123456789012:stateMachine:somestatemachine")) } | ["aws.state_machine.arn": "arn:aws:states:us-east-1:123456789012:stateMachine:somestatemachine"] | "" | null | null "AWSStepFunctions" | "DescribeExecution" | "POST" | "/" | AWSStepFunctionsClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.describeExecution(new DescribeExecutionRequest().withExecutionArn("arn:aws:states:us-east-1:123456789012:execution:somestatemachine:someexecution")) } | ["aws.execution.arn": "arn:aws:states:us-east-1:123456789012:execution:somestatemachine:someexecution"] | "" | null | null diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/build.gradle b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/build.gradle index b4809fa9e3b..d55f704b223 100644 --- a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/build.gradle +++ b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/build.gradle @@ -36,7 +36,7 @@ def fixedSdkVersion = '2.20.33' // 2.20.34 is missing and breaks IDEA import dependencies { compileOnly group: 'software.amazon.awssdk', name: 'aws-core', version: '2.2.0' payloadTaggingTestContainerImage(image('localstack/localstack:4.2.0', 'test.localstack.image')) - testImplementation project(':dd-java-agent:instrumentation:aws-java:aws-java-common') + implementation project(':dd-java-agent:instrumentation:aws-java:aws-java-common') // Include httpclient instrumentation for testing because it is a dependency for aws-sdk. testImplementation project(':dd-java-agent:instrumentation:apache-httpclient:apache-httpclient-4.0') diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/main/java/datadog/trace/instrumentation/aws/v2/AwsSdkClientDecorator.java b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/main/java/datadog/trace/instrumentation/aws/v2/AwsSdkClientDecorator.java index 78034de3f20..f481b2fc261 100644 --- a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/main/java/datadog/trace/instrumentation/aws/v2/AwsSdkClientDecorator.java +++ b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/main/java/datadog/trace/instrumentation/aws/v2/AwsSdkClientDecorator.java @@ -14,6 +14,7 @@ import datadog.trace.api.Config; import datadog.trace.api.ConfigDefaults; import datadog.trace.api.DDTags; +import datadog.trace.api.GenericClassValue; import datadog.trace.api.cache.DDCache; import datadog.trace.api.cache.DDCaches; import datadog.trace.api.datastreams.AgentDataStreamsMonitoring; @@ -28,7 +29,12 @@ import datadog.trace.bootstrap.instrumentation.api.Tags; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; import datadog.trace.bootstrap.instrumentation.decorator.HttpClientDecorator; +import datadog.trace.instrumentation.aws.AwsAccountIdentity; +import datadog.trace.instrumentation.aws.AwsArn; import datadog.trace.payloadtags.PayloadTagsData; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; import java.net.URI; import java.time.Instant; import java.util.ArrayList; @@ -41,6 +47,8 @@ import java.util.Set; import javax.annotation.Nonnull; import javax.annotation.ParametersAreNonnullByDefault; +import software.amazon.awssdk.auth.credentials.AwsCredentials; +import software.amazon.awssdk.auth.signer.AwsSignerExecutionAttribute; import software.amazon.awssdk.awscore.AwsResponse; import software.amazon.awssdk.core.SdkBytes; import software.amazon.awssdk.core.SdkField; @@ -97,6 +105,14 @@ public class AwsSdkClientDecorator extends HttpClientDecorator new ExecutionAttribute<>("KinesisStreamArn")); + // ExpectedBucketOwner is carried from the request to the response so the account is only tagged + // once S3 has accepted it (a mismatch is rejected with 403). + public static final ExecutionAttribute EXPECTED_BUCKET_OWNER_ATTRIBUTE = + InstanceStore.of(ExecutionAttribute.class) + .getOrCreate( + "DatadogExpectedBucketOwner", + () -> new ExecutionAttribute<>("DatadogExpectedBucketOwner")); + // not static because this object would be ClassLoader specific if multiple SDK instances were // loaded by different loaders private SdkField kinesisApproximateArrivalTimestampField = null; @@ -136,6 +152,12 @@ public void onSdkRequest( // S3 request.getValueForField("Bucket", String.class).ifPresent(name -> setBucketName(span, name)); if ("s3".equalsIgnoreCase(awsServiceName)) { + // S3 enforces ExpectedBucketOwner (403 on mismatch), so it names the owner only once the + // request has succeeded. Remember it here and tag it from the successful response. + request + .getValueForField("ExpectedBucketOwner", String.class) + .filter(AwsAccountIdentity::isAccountId) + .ifPresent(owner -> attributes.putAttribute(EXPECTED_BUCKET_OWNER_ATTRIBUTE, owner)); // gate "Key" extraction to S3 — DynamoDB's Key is Map, would CCE request.getValueForField("Key", String.class).ifPresent(key -> setObjectKey(span, key)); if (traceConfig().isDataStreamsEnabled()) { @@ -188,8 +210,18 @@ public void onSdkRequest( } }); - // DynamoDB - request.getValueForField("TableName", String.class).ifPresent(name -> setTableName(span, name)); + // DynamoDB. Other services (Timestream, Keyspaces, Athena, ...) also have TableName members; + // only DynamoDB resolves a bare name in the caller's account, so only it gets the ownership + // enrichment. Everything else keeps the plain table name tags. + if ("dynamodb".equalsIgnoreCase(awsServiceName)) { + request + .getValueForField("TableName", String.class) + .ifPresent(name -> onDynamoDbTable(span, name, attributes)); + } else { + request + .getValueForField("TableName", String.class) + .ifPresent(name -> setTableName(span, name)); + } // Step Functions. getValueForField matches the API model's member name verbatim, and the // Step Functions model declares its members in lowerCamelCase (unlike SNS "TopicArn" or @@ -298,6 +330,90 @@ private static void setPeerService( } } + /** + * Tags the owning account of the table. A TableName given as an ARN carries it (and is tagged as + * aws.table.arn). A bare name is, by DynamoDB's documented contract, resolved in the requestor's + * own account, so the account owning the signing credentials is the table owner. The existing + * aws.table.name, tablename and peer.service tags keep the TableName value as given, ARN or not, + * so nothing changes for spans that already carry an ARN there. + */ + private static void onDynamoDbTable( + final AgentSpan span, final String tableName, final ExecutionAttributes attributes) { + setTableName(span, tableName); + AwsArn arn = AwsArn.parse(tableName); + String account; + if (arn != null) { + account = arn.account(); + span.setTag(InstrumentationTags.AWS_TABLE_ARN, arn.raw()); + } else { + account = dynamoDbOwnerAccountFromCredentials(attributes); + } + if (account != null) { + span.setTag(InstrumentationTags.AWS_ACCOUNT, account); + } + } + + /** + * Owner of a bare-name DynamoDB table, obtained from the signing credentials only because + * DynamoDB guarantees that a bare name addresses a table in the requestor's own account. This is + * not a general caller-account source and must not be reused for services that support + * cross-account resource addressing. The account is absent on older SDKs and for providers that + * do not expose {@code AwsCredentialsIdentity.accountId()}. + */ + private static String dynamoDbOwnerAccountFromCredentials(final ExecutionAttributes attributes) { + AwsCredentials credentials = + attributes.getAttribute(AwsSignerExecutionAttribute.AWS_CREDENTIALS); + return credentials == null ? null : credentialsAccountId(credentials); + } + + // Optional accountId() was introduced on AwsCredentialsIdentity after 2.2.0, an interface + // that does not exist at the 2.2.0 floor, so it is looked up by name per credentials class. The + // handle is kept on the class itself (ClassValue) rather than in a static map: a credentials + // class from an application class loader must not be pinned by instrumentation state. A missing + // method is recorded as this sentinel. + private static final MethodHandle NO_ACCOUNT_ID_GETTER = + MethodHandles.constant(Optional.class, Optional.empty()); + private static final ClassValue ACCOUNT_ID_GETTERS = + GenericClassValue.of(AwsSdkClientDecorator::accountIdGetter); + + private static MethodHandle accountIdGetter(final Class type) { + try { + return MethodHandles.publicLookup() + .findVirtual(type, "accountId", MethodType.methodType(Optional.class)); + } catch (NoSuchMethodException | IllegalAccessException e) { + return NO_ACCOUNT_ID_GETTER; + } + } + + static String credentialsAccountId(final AwsCredentials credentials) { + MethodHandle getter = ACCOUNT_ID_GETTERS.get(credentials.getClass()); + if (getter == NO_ACCOUNT_ID_GETTER) { + return null; + } + try { + Object value = getter.invoke(credentials); + if (value instanceof Optional) { + Object account = ((Optional) value).orElse(null); + if (account instanceof String && AwsAccountIdentity.isAccountId((String) account)) { + return (String) account; + } + } + } catch (VirtualMachineError | ThreadDeath fatal) { + // OutOfMemoryError, StackOverflowError, ...: never hide these from the application. + throw fatal; + } catch (Throwable ignored) { + // Anything else (a provider failing to resolve, a linkage problem in the lookup) means the + // account is not available for this request; it must not fail the application's call. + } + return null; + } + + private static void setBucketOwner(AgentSpan span, String owner) { + // aws_account only: the Agent's credit card obfuscator redacts 12-digit values under keys it + // does not know, and aws_account is on its allow list. + span.setTag(InstrumentationTags.AWS_ACCOUNT, owner); + } + private static void setBucketName(AgentSpan span, String name) { span.setTag(InstrumentationTags.AWS_BUCKET_NAME, name); span.setTag(InstrumentationTags.BUCKET_NAME, name); @@ -396,6 +512,13 @@ public void onSdkResponse( final AgentSpan span = fromContext(context); Config config = Config.get(); String serviceName = attributes.getAttribute(SdkExecutionAttribute.SERVICE_NAME); + + // Only a successful response proves the asserted ExpectedBucketOwner is the bucket owner. + String expectedBucketOwner = attributes.getAttribute(EXPECTED_BUCKET_OWNER_ATTRIBUTE); + if (expectedBucketOwner != null && httpResponse != null && httpResponse.isSuccessful()) { + setBucketOwner(span, expectedBucketOwner); + } + if (config.isCloudResponsePayloadTaggingEnabled() && config.isCloudPayloadTaggingEnabledFor(serviceName)) { awsPojoToTags(span, ConfigDefaults.DEFAULT_TRACE_CLOUD_PAYLOAD_RESPONSE_TAG, response); diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/payloadTaggingTest/java/datadog/trace/instrumentation/aws/v2/S3BucketOwnerForkedTest.java b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/payloadTaggingTest/java/datadog/trace/instrumentation/aws/v2/S3BucketOwnerForkedTest.java new file mode 100644 index 00000000000..cb7347b8738 --- /dev/null +++ b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/payloadTaggingTest/java/datadog/trace/instrumentation/aws/v2/S3BucketOwnerForkedTest.java @@ -0,0 +1,122 @@ +package datadog.trace.instrumentation.aws.v2; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; + +import com.sun.net.httpserver.HttpServer; +import datadog.trace.agent.test.AbstractInstrumentationTest; +import datadog.trace.core.DDSpan; +import java.io.IOException; +import java.net.InetSocketAddress; +import java.net.URI; +import java.util.List; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.S3Configuration; +import software.amazon.awssdk.services.s3.model.GetObjectRequest; +import software.amazon.awssdk.services.s3.model.S3Exception; + +/** + * Lives in this source set because its S3 model (2.18.40) has {@code ExpectedBucketOwner}; the base + * test suite pins s3 2.2.0 which predates the field. + */ +class S3BucketOwnerForkedTest extends AbstractInstrumentationTest { + + private static HttpServer server; + private static S3Client client; + + @BeforeAll + static void startServer() throws IOException { + server = HttpServer.create(new InetSocketAddress("localhost", 0), 0); + server.createContext( + "/", + exchange -> { + // Keys under "forbidden" simulate S3 rejecting the asserted ExpectedBucketOwner. + int status = exchange.getRequestURI().getPath().contains("forbidden") ? 403 : 200; + exchange.sendResponseHeaders(status, -1); + exchange.close(); + }); + server.start(); + client = + S3Client.builder() + .endpointOverride(URI.create("http://localhost:" + server.getAddress().getPort())) + // Path-style addressing keeps the bucket out of the host name. With the virtual-hosted + // default the SDK would call somebucket.localhost, which only resolves on machines + // whose resolver maps *.localhost to loopback (it did locally, not on CI runners). + .serviceConfiguration(S3Configuration.builder().pathStyleAccessEnabled(true).build()) + .region(Region.US_EAST_1) + .credentialsProvider( + StaticCredentialsProvider.create(AwsBasicCredentials.create("test", "test"))) + .build(); + } + + @AfterAll + static void stopServer() { + client.close(); + server.stop(0); + } + + @Test + void expectedBucketOwnerTagsTheOwningAccount() throws Exception { + client.getObject( + GetObjectRequest.builder() + .bucket("somebucket") + .key("somekey") + .expectedBucketOwner("123456789012") + .build()); + + DDSpan span = firstSpan(); + assertEquals("somebucket", span.getTag("aws.bucket.name")); + assertEquals("123456789012", span.getTag("aws_account")); + } + + @Test + void noExpectedBucketOwnerMeansNoAccountTag() throws Exception { + client.getObject(GetObjectRequest.builder().bucket("somebucket").key("somekey").build()); + + DDSpan span = firstSpan(); + assertEquals("somebucket", span.getTag("aws.bucket.name")); + assertFalse(span.getTags().containsKey("aws_account")); + } + + @Test + void rejectedExpectedBucketOwnerIsNotTagged() throws Exception { + try { + client.getObject( + GetObjectRequest.builder() + .bucket("somebucket") + .key("forbidden") + .expectedBucketOwner("123456789012") + .build()); + } catch (S3Exception expected) { + // S3 rejects a wrong owner with 403; the asserted owner must not be tagged as the account + } + + DDSpan span = firstSpan(); + assertEquals("somebucket", span.getTag("aws.bucket.name")); + assertFalse(span.getTags().containsKey("aws_account")); + } + + @Test + void malformedExpectedBucketOwnerIsIgnored() throws Exception { + client.getObject( + GetObjectRequest.builder() + .bucket("somebucket") + .key("somekey") + .expectedBucketOwner("not-an-account") + .build()); + + assertFalse(firstSpan().getTags().containsKey("aws_account")); + } + + private static DDSpan firstSpan() throws Exception { + writer.waitForTraces(1); + List trace = writer.firstTrace(); + return trace.get(0); + } +} diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/test/groovy/Aws2ClientTest.groovy b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/test/groovy/Aws2ClientTest.groovy index bab61052a0c..24a8d7a7f6b 100644 --- a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/test/groovy/Aws2ClientTest.groovy +++ b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/test/groovy/Aws2ClientTest.groovy @@ -563,6 +563,78 @@ abstract class Aws2ClientTest extends VersionedNamingTestBase { """ } + def "DynamoDb request with a table ARN tags the owning account and table ARN"() { + setup: + def client = DynamoDbClient.builder() + .endpointOverride(server.address) + .region(Region.AP_NORTHEAST_1) + .credentialsProvider(CREDENTIALS_PROVIDER) + .build() + responseBody.set("") + def tableArn = "arn:aws:dynamodb:ap-northeast-1:123456789012:table/sometable" + + when: + client.getItem(GetItemRequest.builder().tableName(tableArn).key(["attribute": AttributeValue.builder().s("somevalue").build()]).build()) + TEST_WRITER.waitForTraces(1) + + then: + assertTraces(1) { + trace(1) { + span { + serviceName expectedService("DynamoDb", "GetItem") + operationName expectedOperation("DynamoDb", "GetItem") + resourceName "DynamoDb.GetItem" + spanType DDSpanTypes.HTTP_CLIENT + errored false + measured true + parent() + tags { + "$Tags.COMPONENT" "java-aws-sdk" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT + "$Tags.PEER_HOSTNAME" "localhost" + "$Tags.PEER_PORT" server.address.port + "$Tags.HTTP_METHOD" "POST" + "$Tags.HTTP_STATUS" 200 + "aws.service" "DynamoDb" + "aws_service" "DynamoDb" + "aws.operation" "GetItem" + "aws.agent" "java-aws-sdk" + "aws.requestId" "UNKNOWN" + // the name tags (and so peer.service) keep the TableName as given; the ARN and + // account are additions, not replacements + "aws.table.name" tableArn + "tablename" tableArn + "aws.table.arn" tableArn + "aws_account" "123456789012" + peerServiceFrom("aws.table.name") + urlTags("${server.address}/", ExpectedQueryParams.getExpectedQueryParams("GetItem")) + defaultTags(false, true) + } + } + } + } + } + + def "DynamoDb request with a bare table name has no account when the credentials carry none"() { + setup: + def client = DynamoDbClient.builder() + .endpointOverride(server.address) + .region(Region.AP_NORTHEAST_1) + .credentialsProvider(CREDENTIALS_PROVIDER) + .build() + responseBody.set("") + + when: + client.getItem(GetItemRequest.builder().tableName("sometable").key(["attribute": AttributeValue.builder().s("somevalue").build()]).build()) + TEST_WRITER.waitForTraces(1) + + then: + def tags = TEST_WRITER.firstTrace().first().tags + tags["aws.table.name"] == "sometable" + !tags.containsKey("aws_account") + !tags.containsKey("aws.table.arn") + } + def "timeout and retry errors captured"() { setup: def server = httpServer { diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/test/java/datadog/trace/instrumentation/aws/v2/AwsSdkClientDecoratorCredentialsTest.java b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/test/java/datadog/trace/instrumentation/aws/v2/AwsSdkClientDecoratorCredentialsTest.java new file mode 100644 index 00000000000..69074cf8dfe --- /dev/null +++ b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/test/java/datadog/trace/instrumentation/aws/v2/AwsSdkClientDecoratorCredentialsTest.java @@ -0,0 +1,111 @@ +package datadog.trace.instrumentation.aws.v2; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import datadog.trace.agent.test.AbstractInstrumentationTest; +import java.util.Optional; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.AwsCredentials; + +/** + * {@code AwsCredentialsIdentity.accountId()} does not exist in the SDK this suite compiles against, + * so the lookup is exercised with credentials classes that declare the same method. + */ +public class AwsSdkClientDecoratorCredentialsTest extends AbstractInstrumentationTest { + + @Test + void readsTheAccountFromCredentialsThatExposeIt() { + assertEquals( + "123456789012", + AwsSdkClientDecorator.credentialsAccountId(new AccountCredentials("123456789012"))); + } + + @Test + void ignoresCredentialsWithoutAnAccountIdMethod() { + // AwsBasicCredentials at 2.2.0 predates accountId() + assertNull( + AwsSdkClientDecorator.credentialsAccountId(AwsBasicCredentials.create("key", "secret"))); + } + + @Test + void ignoresAnEmptyOrMalformedAccount() { + assertNull(AwsSdkClientDecorator.credentialsAccountId(new AccountCredentials(null))); + assertNull(AwsSdkClientDecorator.credentialsAccountId(new AccountCredentials("12345"))); + assertNull( + AwsSdkClientDecorator.credentialsAccountId(new AccountCredentials("arn:aws:iam::1:root"))); + } + + @Test + void treatsAFailingLookupAsAbsent() { + assertNull(AwsSdkClientDecorator.credentialsAccountId(new FailingCredentials())); + } + + @Test + void doesNotSwallowFatalErrors() { + OutOfMemoryError error = + assertThrows( + OutOfMemoryError.class, + () -> AwsSdkClientDecorator.credentialsAccountId(new ExhaustedCredentials())); + assertSame(ExhaustedCredentials.ERROR, error); + } + + public static final class AccountCredentials implements AwsCredentials { + private final String account; + + AccountCredentials(String account) { + this.account = account; + } + + public Optional accountId() { + return Optional.ofNullable(account); + } + + @Override + public String accessKeyId() { + return "key"; + } + + @Override + public String secretAccessKey() { + return "secret"; + } + } + + public static final class FailingCredentials implements AwsCredentials { + public Optional accountId() { + throw new IllegalStateException("provider could not resolve the account"); + } + + @Override + public String accessKeyId() { + return "key"; + } + + @Override + public String secretAccessKey() { + return "secret"; + } + } + + public static final class ExhaustedCredentials implements AwsCredentials { + static final OutOfMemoryError ERROR = new OutOfMemoryError("simulated"); + + public Optional accountId() { + throw ERROR; + } + + @Override + public String accessKeyId() { + return "key"; + } + + @Override + public String secretAccessKey() { + return "secret"; + } + } +} diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/InstrumentationTags.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/InstrumentationTags.java index 1971a551f88..7f7305b3f67 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/InstrumentationTags.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/InstrumentationTags.java @@ -37,6 +37,11 @@ public class InstrumentationTags { public static final String AWS_REQUEST_ID = "aws.requestId"; public static final String AWS_STORAGE_CLASS = "aws.storage.class"; + // Owning account of the addressed resource. aws_account matches the tag dd-trace-py sets and the + // dimension tag on the AWS integration metrics. + public static final String AWS_ACCOUNT = "aws_account"; + public static final String AWS_TABLE_ARN = "aws.table.arn"; + // Complete resource identity carried by the request. public static final String AWS_TOPIC_ARN = "aws.topic.arn"; public static final String AWS_STREAM_ARN = "aws.stream.arn"; diff --git a/tag-conventions.yaml b/tag-conventions.yaml index 3f13f0fbd49..7f30e12c00c 100644 --- a/tag-conventions.yaml +++ b/tag-conventions.yaml @@ -156,6 +156,11 @@ span_types: # consumer spans are the aws_request mixin. aws.client: extends: http.client + tags: + # Owning AWS account of the called resource, resolved per service (e.g. S3 from a successful + # ExpectedBucketOwner, DynamoDB from the table owner). Declared once here so every service + # type inherits the same tag identity. + - { dd-name: aws_account, type: string, required: optional } aws.s3: extends: aws.client @@ -191,6 +196,7 @@ span_types: extends: aws.client tags: - { dd-name: aws.table.name, type: string, required: optional } + - { dd-name: aws.table.arn, type: string, required: optional } # when the table is named by ARN - { dd-name: tablename, type: string, required: optional } # Span pointer input, removed before serialization (SpanPointersProcessor). - { dd-name: dynamodb.primary_key_1, type: string, required: optional }