From 57dba6990c3a28ecc14439b399a7c20370e9bd68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20=C3=81lvarez=20=C3=81lvarez?= Date: Tue, 28 May 2024 09:51:18 +0200 Subject: [PATCH 1/5] Add grpc.server.method to WAF addresses with FQN of the grpc method --- .../appsec/event/data/KnownAddresses.java | 4 +++ .../datadog/appsec/gateway/GatewayBridge.java | 27 +++++++++++++++++++ .../data/KnownAddressesSpecification.groovy | 3 ++- .../gateway/GatewayBridgeSpecification.groovy | 18 +++++++++++++ .../grpc/server/TracingServerInterceptor.java | 12 +++++++++ .../src/test/groovy/ArmeriaGrpcTest.groovy | 7 +++++ .../grpc/server/TracingServerInterceptor.java | 12 +++++++++ .../grpc-1.5/src/test/groovy/GrpcTest.groovy | 7 +++++ .../datadog/trace/api/gateway/Events.java | 11 ++++++++ .../api/gateway/InstrumentationGateway.java | 2 ++ .../gateway/InstrumentationGatewayTest.java | 6 +++++ 11 files changed, 108 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/event/data/KnownAddresses.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/event/data/KnownAddresses.java index f6c25e242b7..7171c2281de 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/event/data/KnownAddresses.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/event/data/KnownAddresses.java @@ -93,6 +93,8 @@ public interface KnownAddresses { Address>> HEADERS_NO_COOKIES = new Address<>("server.request.headers.no_cookies"); + Address GRPC_SERVER_METHOD = new Address<>("grpc.server.method"); + Address GRPC_SERVER_REQUEST_MESSAGE = new Address<>("grpc.server.request.message"); // XXX: Not really used yet, but it's a known address and we should not treat it as unknown. @@ -159,6 +161,8 @@ static Address forName(String name) { return REQUEST_QUERY; case "server.request.headers.no_cookies": return HEADERS_NO_COOKIES; + case "grpc.server.method": + return GRPC_SERVER_METHOD; case "grpc.server.request.message": return GRPC_SERVER_REQUEST_MESSAGE; case "grpc.server.request.metadata": diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java index 837b96f6628..fd0ded3a0b4 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java @@ -79,6 +79,7 @@ public class GatewayBridge { private volatile DataSubscriberInfo requestBodySubInfo; private volatile DataSubscriberInfo pathParamsSubInfo; private volatile DataSubscriberInfo respDataSubInfo; + private volatile DataSubscriberInfo grpcServerMethodSubInfo; private volatile DataSubscriberInfo grpcServerRequestMsgSubInfo; private volatile DataSubscriberInfo graphqlServerRequestMsgSubInfo; private volatile DataSubscriberInfo requestEndSubInfo; @@ -361,6 +362,32 @@ public void init() { return maybePublishResponseData(ctx); }); + subscriptionService.registerCallback( + EVENTS.grpcServerMethod(), + (ctx_, method) -> { + AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC); + if (ctx == null) { + return NoopFlow.INSTANCE; + } + while (true) { + DataSubscriberInfo subInfo = grpcServerMethodSubInfo; + if (subInfo == null) { + subInfo = producerService.getDataSubscribers(KnownAddresses.GRPC_SERVER_METHOD); + grpcServerMethodSubInfo = subInfo; + } + if (subInfo == null || subInfo.isEmpty()) { + return NoopFlow.INSTANCE; + } + DataBundle bundle = + new SingletonDataBundle<>(KnownAddresses.GRPC_SERVER_METHOD, method); + try { + return producerService.publishDataEvent(subInfo, ctx, bundle, true); + } catch (ExpiredSubscriberInfoException e) { + grpcServerMethodSubInfo = null; + } + } + }); + subscriptionService.registerCallback( EVENTS.grpcServerRequestMessage(), (ctx_, obj) -> { diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/event/data/KnownAddressesSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/event/data/KnownAddressesSpecification.groovy index 39f236076c2..63ead1b4ca3 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/event/data/KnownAddressesSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/event/data/KnownAddressesSpecification.groovy @@ -28,6 +28,7 @@ class KnownAddressesSpecification extends Specification { 'server.request.body.combined_file_size', 'server.request.query', 'server.request.headers.no_cookies', + 'grpc.server.method', 'grpc.server.request.message', 'grpc.server.request.metadata', 'graphql.server.all_resolvers', @@ -41,7 +42,7 @@ class KnownAddressesSpecification extends Specification { void 'number of known addresses is expected number'() { expect: - Address.instanceCount() == 29 + Address.instanceCount() == 30 KnownAddresses.WAF_CONTEXT_PROCESSOR.serial == Address.instanceCount() - 1 } } diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy index cb86d18668f..2f2b4d8a594 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy @@ -77,6 +77,7 @@ class GatewayBridgeSpecification extends DDSpecification { BiFunction> responseStartedCB TriConsumer respHeaderCB Function> respHeadersDoneCB + BiFunction> grpcServerMethodCB BiFunction> grpcServerRequestMessageCB BiFunction, Flow> graphqlServerRequestMessageCB BiConsumer databaseConnectionCB @@ -413,6 +414,7 @@ class GatewayBridgeSpecification extends DDSpecification { 1 * ig.registerCallback(EVENTS.responseStarted(), _) >> { responseStartedCB = it[1]; null } 1 * ig.registerCallback(EVENTS.responseHeader(), _) >> { respHeaderCB = it[1]; null } 1 * ig.registerCallback(EVENTS.responseHeaderDone(), _) >> { respHeadersDoneCB = it[1]; null } + 1 * ig.registerCallback(EVENTS.grpcServerMethod(), _) >> { grpcServerMethodCB = it[1]; null } 1 * ig.registerCallback(EVENTS.grpcServerRequestMessage(), _) >> { grpcServerRequestMessageCB = it[1]; null } 1 * ig.registerCallback(EVENTS.graphqlServerRequestMessage(), _) >> { graphqlServerRequestMessageCB = it[1]; null } 1 * ig.registerCallback(EVENTS.databaseConnection(), _) >> { databaseConnectionCB = it[1]; null } @@ -710,6 +712,22 @@ class GatewayBridgeSpecification extends DDSpecification { flow.action == Flow.Action.Noop.INSTANCE } + void 'grpc server method publishes'() { + setup: + eventDispatcher.getDataSubscribers(KnownAddresses.GRPC_SERVER_METHOD) >> nonEmptyDsInfo + DataBundle bundle + + when: + Flow flow = grpcServerMethodCB.apply(ctx, '/my.package.Greeter/SayHello') + + then: + 1 * eventDispatcher.publishDataEvent(nonEmptyDsInfo, ctx.data, _ as DataBundle, true) >> + { args -> bundle = args[2]; NoopFlow.INSTANCE } + bundle.get(KnownAddresses.GRPC_SERVER_METHOD) == '/my.package.Greeter/SayHello' + flow.result == null + flow.action == Flow.Action.Noop.INSTANCE + } + void 'calls trace segment post processor'() { setup: AgentSpan span = Stub() diff --git a/dd-java-agent/instrumentation/armeria-grpc/src/main/java/datadog/trace/instrumentation/armeria/grpc/server/TracingServerInterceptor.java b/dd-java-agent/instrumentation/armeria-grpc/src/main/java/datadog/trace/instrumentation/armeria/grpc/server/TracingServerInterceptor.java index 158c7c72d01..8e60fecaf30 100644 --- a/dd-java-agent/instrumentation/armeria-grpc/src/main/java/datadog/trace/instrumentation/armeria/grpc/server/TracingServerInterceptor.java +++ b/dd-java-agent/instrumentation/armeria-grpc/src/main/java/datadog/trace/instrumentation/armeria/grpc/server/TracingServerInterceptor.java @@ -27,6 +27,7 @@ import io.grpc.ForwardingServerCallListener; import io.grpc.Grpc; import io.grpc.Metadata; +import io.grpc.MethodDescriptor; import io.grpc.ServerCall; import io.grpc.ServerCallHandler; import io.grpc.ServerInterceptor; @@ -76,6 +77,7 @@ public ServerCall.Listener interceptCall( if (reqContext != null) { callIGCallbackClientAddress(cbp, reqContext, call); callIGCallbackHeaders(cbp, reqContext, headers); + callIGCallbackGrpcServerMethod(cbp, reqContext, call.getMethodDescriptor()); } DECORATE.afterStart(span); @@ -315,6 +317,16 @@ private static void callIGCallbackRequestEnded(@Nonnull final AgentSpan span) { } } + private static void callIGCallbackGrpcServerMethod( + CallbackProvider cbp, RequestContext ctx, MethodDescriptor methodDescriptor) { + String method = methodDescriptor.getFullMethodName(); + BiFunction> cb = cbp.getCallback(EVENTS.grpcServerMethod()); + if (method == null || cb == null) { + return; + } + cb.apply(ctx, method); + } + private static void callIGCallbackGrpcMessage(@Nonnull final AgentSpan span, Object obj) { if (obj == null) { return; diff --git a/dd-java-agent/instrumentation/armeria-grpc/src/test/groovy/ArmeriaGrpcTest.groovy b/dd-java-agent/instrumentation/armeria-grpc/src/test/groovy/ArmeriaGrpcTest.groovy index 4f93259bedb..9aa49c386d8 100644 --- a/dd-java-agent/instrumentation/armeria-grpc/src/test/groovy/ArmeriaGrpcTest.groovy +++ b/dd-java-agent/instrumentation/armeria-grpc/src/test/groovy/ArmeriaGrpcTest.groovy @@ -45,6 +45,7 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase { def collectedAppSecHeaders = [:] boolean appSecHeaderDone = false + def collectedAppSecServerMethods = [] def collectedAppSecReqMsgs = [] final Duration timeoutDuration() { @@ -97,6 +98,10 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase { collectedAppSecReqMsgs << obj Flow.ResultFlow.empty() } as BiFunction>) + ig.registerCallback(EVENTS.grpcServerMethod(), { reqCtx, method -> + collectedAppSecServerMethods << method + Flow.ResultFlow.empty() + } as BiFunction>) } def cleanup() { @@ -230,6 +235,8 @@ abstract class ArmeriaGrpcTest extends VersionedNamingTestBase { traceId.toLong() as String == collectedAppSecHeaders['x-datadog-trace-id'] collectedAppSecReqMsgs.size() == 1 collectedAppSecReqMsgs.first().name == name + collectedAppSecServerMethods.size() == 1 + collectedAppSecServerMethods.first() == 'example.Greeter/SayHello' and: if (isDataStreamsEnabled()) { diff --git a/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/server/TracingServerInterceptor.java b/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/server/TracingServerInterceptor.java index a238d7ac5f6..276524a098a 100644 --- a/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/server/TracingServerInterceptor.java +++ b/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/server/TracingServerInterceptor.java @@ -27,6 +27,7 @@ import io.grpc.ForwardingServerCallListener; import io.grpc.Grpc; import io.grpc.Metadata; +import io.grpc.MethodDescriptor; import io.grpc.ServerCall; import io.grpc.ServerCallHandler; import io.grpc.ServerInterceptor; @@ -75,6 +76,7 @@ public ServerCall.Listener interceptCall( if (reqContext != null) { callIGCallbackClientAddress(cbp, reqContext, call); callIGCallbackHeaders(cbp, reqContext, headers); + callIGCallbackGrpcServerMethod(cbp, reqContext, call.getMethodDescriptor()); } DECORATE.afterStart(span); @@ -314,6 +316,16 @@ private static void callIGCallbackRequestEnded(@Nonnull final AgentSpan span) { } } + private static void callIGCallbackGrpcServerMethod( + CallbackProvider cbp, RequestContext ctx, MethodDescriptor methodDescriptor) { + String method = methodDescriptor.getFullMethodName(); + BiFunction> cb = cbp.getCallback(EVENTS.grpcServerMethod()); + if (method == null || cb == null) { + return; + } + cb.apply(ctx, method); + } + private static void callIGCallbackGrpcMessage(@Nonnull final AgentSpan span, Object obj) { if (obj == null) { return; diff --git a/dd-java-agent/instrumentation/grpc-1.5/src/test/groovy/GrpcTest.groovy b/dd-java-agent/instrumentation/grpc-1.5/src/test/groovy/GrpcTest.groovy index 7a5dac37102..49f76f430a4 100644 --- a/dd-java-agent/instrumentation/grpc-1.5/src/test/groovy/GrpcTest.groovy +++ b/dd-java-agent/instrumentation/grpc-1.5/src/test/groovy/GrpcTest.groovy @@ -47,6 +47,7 @@ abstract class GrpcTest extends VersionedNamingTestBase { def collectedAppSecHeaders = [:] boolean appSecHeaderDone = false def collectedAppSecReqMsgs = [] + def collectedAppSecServerMethods = [] @Override final String service() { @@ -89,6 +90,10 @@ abstract class GrpcTest extends VersionedNamingTestBase { collectedAppSecReqMsgs << obj Flow.ResultFlow.empty() } as BiFunction>) + ig.registerCallback(EVENTS.grpcServerMethod(), { reqCtx, method -> + collectedAppSecServerMethods << method + Flow.ResultFlow.empty() + } as BiFunction>) } def cleanup() { @@ -217,6 +222,8 @@ abstract class GrpcTest extends VersionedNamingTestBase { traceId.toLong() as String == collectedAppSecHeaders['x-datadog-trace-id'] collectedAppSecReqMsgs.size() == 1 collectedAppSecReqMsgs.first().name == name + collectedAppSecServerMethods.size() == 1 + collectedAppSecServerMethods.first() == 'example.Greeter/SayHello' and: if (isDataStreamsEnabled()) { diff --git a/internal-api/src/main/java/datadog/trace/api/gateway/Events.java b/internal-api/src/main/java/datadog/trace/api/gateway/Events.java index e7265c238d2..99412ab7143 100644 --- a/internal-api/src/main/java/datadog/trace/api/gateway/Events.java +++ b/internal-api/src/main/java/datadog/trace/api/gateway/Events.java @@ -225,6 +225,17 @@ public EventType> databaseSqlQuery() { return (EventType>) DATABASE_SQL_QUERY; } + static final int GRPC_SERVER_METHOD_ID = 18; + + @SuppressWarnings("rawtypes") + private static final EventType GRPC_SERVER_METHOD = + new ET<>("grpc.server.method", GRPC_SERVER_METHOD_ID); + + @SuppressWarnings("unchecked") + public EventType>> grpcServerMethod() { + return (EventType>>) GRPC_SERVER_METHOD; + } + static final int MAX_EVENTS = nextId.get(); private static final class ET extends EventType { diff --git a/internal-api/src/main/java/datadog/trace/api/gateway/InstrumentationGateway.java b/internal-api/src/main/java/datadog/trace/api/gateway/InstrumentationGateway.java index c4babc85f07..970583460e7 100644 --- a/internal-api/src/main/java/datadog/trace/api/gateway/InstrumentationGateway.java +++ b/internal-api/src/main/java/datadog/trace/api/gateway/InstrumentationGateway.java @@ -3,6 +3,7 @@ import static datadog.trace.api.gateway.Events.DATABASE_CONNECTION_ID; import static datadog.trace.api.gateway.Events.DATABASE_SQL_QUERY_ID; import static datadog.trace.api.gateway.Events.GRAPHQL_SERVER_REQUEST_MESSAGE_ID; +import static datadog.trace.api.gateway.Events.GRPC_SERVER_METHOD_ID; import static datadog.trace.api.gateway.Events.GRPC_SERVER_REQUEST_MESSAGE_ID; import static datadog.trace.api.gateway.Events.MAX_EVENTS; import static datadog.trace.api.gateway.Events.REQUEST_BODY_CONVERTED_ID; @@ -287,6 +288,7 @@ public boolean equals(Object obj) { return callback.equals(obj); } }; + case GRPC_SERVER_METHOD_ID: case REQUEST_INFERRED_CLIENT_ADDRESS_ID: return (C) new BiFunction>() { diff --git a/internal-api/src/test/java/datadog/trace/api/gateway/InstrumentationGatewayTest.java b/internal-api/src/test/java/datadog/trace/api/gateway/InstrumentationGatewayTest.java index 0d9c30719ee..69bc77a4887 100644 --- a/internal-api/src/test/java/datadog/trace/api/gateway/InstrumentationGatewayTest.java +++ b/internal-api/src/test/java/datadog/trace/api/gateway/InstrumentationGatewayTest.java @@ -185,6 +185,9 @@ public void testNormalCalls() { ss.registerCallback(events.requestBodyProcessed(), callback); assertThat(cbp.getCallback(events.requestBodyProcessed()).apply(null, null).getAction()) .isEqualTo(Flow.Action.Noop.INSTANCE); + ss.registerCallback(events.grpcServerMethod(), callback); + assertThat(cbp.getCallback(events.grpcServerMethod()).apply(null, null).getAction()) + .isEqualTo(Flow.Action.Noop.INSTANCE); ss.registerCallback(events.grpcServerRequestMessage(), callback); assertThat(cbp.getCallback(events.grpcServerRequestMessage()).apply(null, null).getAction()) .isEqualTo(Flow.Action.Noop.INSTANCE); @@ -238,6 +241,9 @@ public void testThrowableBlocking() { ss.registerCallback(events.requestBodyProcessed(), throwback); assertThat(cbp.getCallback(events.requestBodyProcessed()).apply(null, null).getAction()) .isEqualTo(Flow.Action.Noop.INSTANCE); + ss.registerCallback(events.grpcServerMethod(), throwback); + assertThat(cbp.getCallback(events.grpcServerMethod()).apply(null, null).getAction()) + .isEqualTo(Flow.Action.Noop.INSTANCE); ss.registerCallback(events.grpcServerRequestMessage(), throwback); assertThat(cbp.getCallback(events.grpcServerRequestMessage()).apply(null, null).getAction()) .isEqualTo(Flow.Action.Noop.INSTANCE); From 1b74acc8f05f145a1448dd828f3f4a252d2bcd07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20=C3=81lvarez=20=C3=81lvarez?= Date: Tue, 28 May 2024 12:07:30 +0200 Subject: [PATCH 2/5] Add smoke tests for grpc.server.method --- ...bstractSpringBootWithGRPCAppSecTest.groovy | 24 +++++++ .../smoketest/appsec/ServerMethodTest.groovy | 67 +++++++++++++++++++ ...groovy => ServerRequestMessageTest.groovy} | 24 +------ .../appsec/SpringBootSmokeTest.groovy | 52 ++++++-------- .../AbstractAppSecServerSmokeTest.groovy | 23 +++++++ 5 files changed, 137 insertions(+), 53 deletions(-) create mode 100644 dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/AbstractSpringBootWithGRPCAppSecTest.groovy create mode 100644 dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy rename dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/{SpringBootWithGRPCAppSecTest.groovy => ServerRequestMessageTest.groovy} (54%) diff --git a/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/AbstractSpringBootWithGRPCAppSecTest.groovy b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/AbstractSpringBootWithGRPCAppSecTest.groovy new file mode 100644 index 00000000000..d2ebff968da --- /dev/null +++ b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/AbstractSpringBootWithGRPCAppSecTest.groovy @@ -0,0 +1,24 @@ +package datadog.smoketest.appsec + +abstract class AbstractSpringBootWithGRPCAppSecTest extends AbstractAppSecServerSmokeTest { + + @Override + ProcessBuilder createProcessBuilder() { + String springBootShadowJar = System.getProperty("datadog.smoketest.appsec.springboot-grpc.shadowJar.path") + assert springBootShadowJar != null + + List command = [ + javaPath(), + *defaultJavaProperties, + *defaultAppSecProperties, + "-jar", + springBootShadowJar, + "--server.port=${httpPort}" + ].collect { it as String } + + ProcessBuilder processBuilder = new ProcessBuilder(command) + processBuilder.directory(new File(buildDirectory)) + } + + static final String ROUTE = 'async_annotation_greeting' +} diff --git a/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy new file mode 100644 index 00000000000..67b2312abf5 --- /dev/null +++ b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy @@ -0,0 +1,67 @@ +package datadog.smoketest.appsec + + +import okhttp3.Request +import spock.lang.Shared + +class ServerMethodTest extends AbstractSpringBootWithGRPCAppSecTest { + + @Shared + String buildDir = new File(System.getProperty("datadog.smoketest.builddir")).absolutePath + @Shared + String customRulesPath = "${buildDir}/appsec_custom_rules.json" + + @Override + ProcessBuilder createProcessBuilder() { + // We run this here to ensure it runs before starting the process. Child setupSpec runs after parent setupSpec, + // so it is not a valid location. + appendRules(customRulesPath, [ + [ + id : '__test_server_method_bock', + name : 'test rule to block on server method', + tags : [ + type : 'test', + category : 'test', + confidence: '1', + ], + conditions : [ + [ + parameters: [ + inputs: [[address: 'grpc.server.method']], + regex : '^smoketest', + ], + operator : 'match_regex', + ] + ], + transformers: [], + on_match : ['block'] + ] + ]) + return super.createProcessBuilder() + } + + void 'test grpc.server.method address'() { + setup: + String url = "http://localhost:${httpPort}/${ROUTE}" + def request = new Request.Builder() + .url("${url}?message=${'Hello!'.bytes.encodeBase64()}") + .get().build() + + when: + def response = client.newCall(request).execute() + + then: + def responseBodyStr = response.body().string() + responseBodyStr != null + responseBodyStr.contains("bye") + response.body().contentType().toString().contains("text/plain") + response.code() == 200 + + and: + waitForTraceCount(2) == 2 + rootSpans.size() == 2 + def grpcRootSpan = rootSpans.find { it.triggers } + grpcRootSpan != null + grpcRootSpan.triggers[0]['rule_matches'][0]['parameters']['address'][0] == 'grpc.server.method' + } +} diff --git a/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/SpringBootWithGRPCAppSecTest.groovy b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerRequestMessageTest.groovy similarity index 54% rename from dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/SpringBootWithGRPCAppSecTest.groovy rename to dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerRequestMessageTest.groovy index e9bcfbed4ed..72e36cf1bd6 100644 --- a/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/SpringBootWithGRPCAppSecTest.groovy +++ b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerRequestMessageTest.groovy @@ -3,29 +3,9 @@ package datadog.smoketest.appsec import okhttp3.Request -class SpringBootWithGRPCAppSecTest extends AbstractAppSecServerSmokeTest { +class ServerRequestMessageTest extends AbstractSpringBootWithGRPCAppSecTest { - @Override - ProcessBuilder createProcessBuilder() { - String springBootShadowJar = System.getProperty("datadog.smoketest.appsec.springboot-grpc.shadowJar.path") - assert springBootShadowJar != null - - List command = [ - javaPath(), - *defaultJavaProperties, - *defaultAppSecProperties, - "-jar", - springBootShadowJar, - "--server.port=${httpPort}" - ].collect { it as String } - - ProcessBuilder processBuilder = new ProcessBuilder(command) - processBuilder.directory(new File(buildDirectory)) - } - - static final String ROUTE = 'async_annotation_greeting' - - def greeter() { + void 'test grpc.server.request.message address'() { setup: String url = "http://localhost:${httpPort}/${ROUTE}" def request = new Request.Builder() diff --git a/dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/SpringBootSmokeTest.groovy b/dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/SpringBootSmokeTest.groovy index a129f481c55..85cbdd9046e 100644 --- a/dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/SpringBootSmokeTest.groovy +++ b/dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/SpringBootSmokeTest.groovy @@ -1,17 +1,11 @@ package datadog.smoketest.appsec import datadog.trace.agent.test.utils.ThreadUtils -import groovy.json.JsonGenerator -import groovy.json.JsonSlurper import okhttp3.MediaType import okhttp3.Request import okhttp3.RequestBody -import org.apache.commons.io.IOUtils import spock.lang.Shared -import java.nio.charset.StandardCharsets -import java.util.jar.JarFile - class SpringBootSmokeTest extends AbstractAppSecServerSmokeTest { @Shared @@ -21,33 +15,30 @@ class SpringBootSmokeTest extends AbstractAppSecServerSmokeTest { def prepareCustomRules() { // Prepare ruleset with additional test rules - final jarFile = new JarFile(shadowJarPath) - final zipEntry = jarFile.getEntry("appsec/default_config.json") - final content = IOUtils.toString(jarFile.getInputStream(zipEntry), StandardCharsets.UTF_8) - final json = new JsonSlurper().parseText(content) as Map - final rules = json.rules as List> - rules.add([ - id: '__test_request_body_block', - name: 'test rule to block on request body', - tags: [ - type: 'test', - category: 'test', - confidence: '1', - ], - conditions: [ + appendRules( + customRulesPath, + [ [ - parameters: [ - inputs: [ [ address: 'server.request.body' ] ], - regex: 'dd-test-request-body-block', + id : '__test_request_body_block', + name : 'test rule to block on request body', + tags : [ + type : 'test', + category : 'test', + confidence: '1', + ], + conditions : [ + [ + parameters: [ + inputs: [[address: 'server.request.body']], + regex : 'dd-test-request-body-block', + ], + operator : 'match_regex', + ] ], - operator: 'match_regex', + transformers: [], + on_match : ['block'] ] - ], - transformers: [], - on_match: [ 'block' ] - ]) - final gen = new JsonGenerator.Options().build() - IOUtils.write(gen.toJson(json), new FileOutputStream(customRulesPath, false), StandardCharsets.UTF_8) + ]) } @Override @@ -62,7 +53,6 @@ class SpringBootSmokeTest extends AbstractAppSecServerSmokeTest { command.add(javaPath()) command.addAll(defaultJavaProperties) command.addAll(defaultAppSecProperties) - command.add("-Ddd.appsec.rules=${customRulesPath}" as String) command.addAll((String[]) ["-jar", springBootShadowJar, "--server.port=${httpPort}"]) ProcessBuilder processBuilder = new ProcessBuilder(command) diff --git a/dd-smoke-tests/appsec/src/main/groovy/datadog/smoketest/appsec/AbstractAppSecServerSmokeTest.groovy b/dd-smoke-tests/appsec/src/main/groovy/datadog/smoketest/appsec/AbstractAppSecServerSmokeTest.groovy index 4fe9b858f83..f0bc62fa660 100644 --- a/dd-smoke-tests/appsec/src/main/groovy/datadog/smoketest/appsec/AbstractAppSecServerSmokeTest.groovy +++ b/dd-smoke-tests/appsec/src/main/groovy/datadog/smoketest/appsec/AbstractAppSecServerSmokeTest.groovy @@ -4,11 +4,15 @@ import datadog.smoketest.AbstractServerSmokeTest import datadog.trace.test.agent.decoder.DecodedSpan import datadog.trace.test.agent.decoder.DecodedTrace import datadog.trace.test.agent.decoder.Decoder +import groovy.json.JsonGenerator import groovy.json.JsonSlurper +import org.apache.commons.io.IOUtils import spock.lang.Shared +import java.nio.charset.StandardCharsets import java.util.concurrent.BlockingQueue import java.util.concurrent.LinkedBlockingQueue +import java.util.jar.JarFile abstract class AbstractAppSecServerSmokeTest extends AbstractServerSmokeTest { @@ -78,4 +82,23 @@ abstract class AbstractAppSecServerSmokeTest extends AbstractServerSmokeTest { } rootSpans.collectMany {it.triggers }.forEach closure } + + /** + * This method fetches default ruleset included in the agent and appends the selected rules, then it points + * the {@code dd.appsec.rules} variable to the new file + */ + void appendRules(final String path, final List> customRules) { + // Prepare a file with the new rules + final jarFile = new JarFile(shadowJarPath) + final zipEntry = jarFile.getEntry("appsec/default_config.json") + final content = IOUtils.toString(jarFile.getInputStream(zipEntry), StandardCharsets.UTF_8) + final json = new JsonSlurper().parseText(content) as Map + final rules = json.rules as List> + rules.addAll(customRules) + final gen = new JsonGenerator.Options().build() + IOUtils.write(gen.toJson(json), new FileOutputStream(path, false), StandardCharsets.UTF_8) + + // Add a new property pointing to the new ruleset + defaultAppSecProperties += "-Ddd.appsec.rules=${path}" as String + } } From 5b72a59f2c644420e9eb12755ac05a12940f7f8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20=C3=81lvarez=20=C3=81lvarez?= Date: Wed, 29 May 2024 11:40:42 +0200 Subject: [PATCH 3/5] Ensure that the value of grpc.server.method sent to the WAF starts with / --- .../java/com/datadog/appsec/gateway/GatewayBridge.java | 6 +++++- .../datadog/smoketest/appsec/ServerMethodTest.groovy | 9 ++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java index fd0ded3a0b4..a7bcf29c89a 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java @@ -61,6 +61,7 @@ public class GatewayBridge { private static final Pattern QUERY_PARAM_VALUE_SPLITTER = Pattern.compile("="); private static final Pattern QUERY_PARAM_SPLITTER = Pattern.compile("&"); private static final Map> EMPTY_QUERY_PARAMS = Collections.emptyMap(); + private static final char GRPC_METHOD_START = '/'; /** User tracking tags that will force the collection of request headers */ private static final String[] USER_TRACKING_TAGS = { @@ -366,7 +367,7 @@ public void init() { EVENTS.grpcServerMethod(), (ctx_, method) -> { AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC); - if (ctx == null) { + if (ctx == null || method == null || method.isEmpty()) { return NoopFlow.INSTANCE; } while (true) { @@ -378,6 +379,9 @@ public void init() { if (subInfo == null || subInfo.isEmpty()) { return NoopFlow.INSTANCE; } + if (method.charAt(0) != GRPC_METHOD_START) { + method = GRPC_METHOD_START + method; + } DataBundle bundle = new SingletonDataBundle<>(KnownAddresses.GRPC_SERVER_METHOD, method); try { diff --git a/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy index 67b2312abf5..a621e57de25 100644 --- a/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy +++ b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy @@ -28,9 +28,9 @@ class ServerMethodTest extends AbstractSpringBootWithGRPCAppSecTest { [ parameters: [ inputs: [[address: 'grpc.server.method']], - regex : '^smoketest', + list : ['/smoketest.Greeter/Hello'], ], - operator : 'match_regex', + operator : 'phrase_match', ] ], transformers: [], @@ -62,6 +62,9 @@ class ServerMethodTest extends AbstractSpringBootWithGRPCAppSecTest { rootSpans.size() == 2 def grpcRootSpan = rootSpans.find { it.triggers } grpcRootSpan != null - grpcRootSpan.triggers[0]['rule_matches'][0]['parameters']['address'][0] == 'grpc.server.method' + def match = grpcRootSpan.triggers[0]['rule_matches'][0] + match != null + match['parameters'][0]['address'] == 'grpc.server.method' + match['parameters'][0]['value'] == '/smoketest.Greeter/Hello' } } From a8ca4fe8c56d241b1ea053fc3d1cae001bc53970 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20=C3=81lvarez=20=C3=81lvarez?= Date: Thu, 30 May 2024 12:16:24 +0200 Subject: [PATCH 4/5] Add rpc.grpc.full_method tag to be used by the backend --- .../java/com/datadog/appsec/gateway/GatewayBridge.java | 8 ++++---- .../datadog/smoketest/appsec/ServerMethodTest.groovy | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java index a7bcf29c89a..937eb17e25e 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java @@ -61,7 +61,7 @@ public class GatewayBridge { private static final Pattern QUERY_PARAM_VALUE_SPLITTER = Pattern.compile("="); private static final Pattern QUERY_PARAM_SPLITTER = Pattern.compile("&"); private static final Map> EMPTY_QUERY_PARAMS = Collections.emptyMap(); - private static final char GRPC_METHOD_START = '/'; + private static final String GRPC_FULL_METHOD_NAME_TAG = "rpc.grpc.full_method"; /** User tracking tags that will force the collection of request headers */ private static final String[] USER_TRACKING_TAGS = { @@ -370,6 +370,9 @@ public void init() { if (ctx == null || method == null || method.isEmpty()) { return NoopFlow.INSTANCE; } + // set the tag used by the backend to generate the grpc pass-lists + TraceSegment segment = ctx_.getTraceSegment(); + segment.setTagCurrent(GRPC_FULL_METHOD_NAME_TAG, method); while (true) { DataSubscriberInfo subInfo = grpcServerMethodSubInfo; if (subInfo == null) { @@ -379,9 +382,6 @@ public void init() { if (subInfo == null || subInfo.isEmpty()) { return NoopFlow.INSTANCE; } - if (method.charAt(0) != GRPC_METHOD_START) { - method = GRPC_METHOD_START + method; - } DataBundle bundle = new SingletonDataBundle<>(KnownAddresses.GRPC_SERVER_METHOD, method); try { diff --git a/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy index a621e57de25..71b9c8a4f0f 100644 --- a/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy +++ b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy @@ -65,6 +65,7 @@ class ServerMethodTest extends AbstractSpringBootWithGRPCAppSecTest { def match = grpcRootSpan.triggers[0]['rule_matches'][0] match != null match['parameters'][0]['address'] == 'grpc.server.method' - match['parameters'][0]['value'] == '/smoketest.Greeter/Hello' + match['parameters'][0]['value'] == 'smoketest.Greeter/Hello' + grpcRootSpan.meta['rpc.grpc.full_method'] == 'smoketest.Greeter/Hello' } } From df37e7acb3151f3c7ba466ca47d2d4022aa84128 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20=C3=81lvarez=20=C3=81lvarez?= Date: Wed, 5 Jun 2024 09:30:14 +0200 Subject: [PATCH 5/5] Remove rpc.grpc.full_method tag --- .../main/java/com/datadog/appsec/gateway/GatewayBridge.java | 4 ---- .../groovy/datadog/smoketest/appsec/ServerMethodTest.groovy | 5 ++--- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java index 937eb17e25e..a32bddd4236 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java @@ -61,7 +61,6 @@ public class GatewayBridge { private static final Pattern QUERY_PARAM_VALUE_SPLITTER = Pattern.compile("="); private static final Pattern QUERY_PARAM_SPLITTER = Pattern.compile("&"); private static final Map> EMPTY_QUERY_PARAMS = Collections.emptyMap(); - private static final String GRPC_FULL_METHOD_NAME_TAG = "rpc.grpc.full_method"; /** User tracking tags that will force the collection of request headers */ private static final String[] USER_TRACKING_TAGS = { @@ -370,9 +369,6 @@ public void init() { if (ctx == null || method == null || method.isEmpty()) { return NoopFlow.INSTANCE; } - // set the tag used by the backend to generate the grpc pass-lists - TraceSegment segment = ctx_.getTraceSegment(); - segment.setTagCurrent(GRPC_FULL_METHOD_NAME_TAG, method); while (true) { DataSubscriberInfo subInfo = grpcServerMethodSubInfo; if (subInfo == null) { diff --git a/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy index 71b9c8a4f0f..a9b7c8525c7 100644 --- a/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy +++ b/dd-smoke-tests/appsec/springboot-grpc/src/test/groovy/datadog/smoketest/appsec/ServerMethodTest.groovy @@ -28,9 +28,9 @@ class ServerMethodTest extends AbstractSpringBootWithGRPCAppSecTest { [ parameters: [ inputs: [[address: 'grpc.server.method']], - list : ['/smoketest.Greeter/Hello'], + regex : 'Greeter', ], - operator : 'phrase_match', + operator : 'match_regex', ] ], transformers: [], @@ -66,6 +66,5 @@ class ServerMethodTest extends AbstractSpringBootWithGRPCAppSecTest { match != null match['parameters'][0]['address'] == 'grpc.server.method' match['parameters'][0]['value'] == 'smoketest.Greeter/Hello' - grpcRootSpan.meta['rpc.grpc.full_method'] == 'smoketest.Greeter/Hello' } }