Binary log config: `-service/*` exclusion is accepted and silently turned into a per-service rule, so the service is still logged

Die Konfigurationsanalyse befindet sich in FactoryImpl in services/src/main/java/io/grpc/protobuf/services/BinlogHelper.java (etwa Zeilen 645-669), und isServiceGlob steht etwa in den Zeilen 781-783. Beginne damit, den gemeldeten Test zu BinlogHelperTest hinzuzufügen und ihn auszuführen, um die erste fälschlicherweise akzeptierte Konfiguration zu finden. Fertig ist es, wenn *,-p.s/*, -p.s/*, -* und -p.s/m{h} jeweils Illegal log config pattern auslösen und die bestehenden BinlogHelperTest-Fälle weiterhin bestehen.

What version of gRPC-Java are you using?

master at 15d64a4 (1.86.0-SNAPSHOT); the parsing code is unchanged on current master.

What is your environment?

Linux x86_64, OpenJDK 21.0.2, Gradle 8.14.5 (wrapper).

What did you expect to see?

A binary log config (from GRPC_BINARY_LOG_CONFIG or BinaryLogs.createBinaryLog(sink, configStr)) that contains an exclusion with a wildcard (-p.s/*) or with a {h|m} suffix (-p.s/m{h}) is rejected with IllegalArgumentException("Illegal log config pattern: ..."), like other malformed patterns. gRFC A16 says:

If the pattern is -<service>/<method>, then the specified method must not be logged. The "*" wildcard must not be used for negations, i.e. -<service>/* is not supported.
...
If a pattern is malformed, the gRPC process must refuse to start up

and its grammar defines Excluded-Method -> "-" Full-Method-Name, with no Pattern-Config suffix.

What did you see instead?

Both configs are accepted.

Location (grpc/grpc-java @ 15d64a4):

• services/src/main/java/io/grpc/protobuf/services/BinlogHelper.java:645-669: FactoryImpl checks isServiceGlob() before startsWith("-"), and the exclusion branch ignores binlogOptionStr.
• services/src/main/java/io/grpc/protobuf/services/BinlogHelper.java:781-783: isServiceGlob only checks endsWith("/*").

          } else if (isServiceGlob(methodOrSvc)) {
            // parse config for a service, e.g. "service/*"
            String service = MethodDescriptor.extractFullServiceName(methodOrSvc);
            ...
            perServiceLogs.put(service, createBinaryLog(sink, binlogOptionStr));
            ...
          } else if (methodOrSvc.startsWith("-")) {
            // parse config for a method, e.g. "-service/method"
            String blacklistedMethod = methodOrSvc.substring(1);
            ...
            blacklistedMethods.add(blacklistedMethod);

Problem

• -p.s/* ends with /*, so it takes the service-glob branch and registers a per-service log for a service literally named -p.s. No exclusion is recorded, so with *,-p.s/* every method of p.s is still logged through the global * rule. The user asked for the service not to be logged, and its headers and messages end up in the binary log without any warning.
• -* is stored as an exclusion for a method literally named *, which never matches anything.
• -p.s/m{h} excludes p.s/m as intended, but the {h} suffix (invalid for an exclusion) is silently dropped.

Only the first case changes what gets logged. The other two are malformed configs that are accepted without an error.

Steps to reproduce the bug

Reproduction

Regression test added to BinlogHelperTest:

diff --git a/services/src/test/java/io/grpc/protobuf/services/BinlogHelperTest.java b/services/src/test/java/io/grpc/protobuf/services/BinlogHelperTest.java
--- a/services/src/test/java/io/grpc/protobuf/services/BinlogHelperTest.java
+++ b/services/src/test/java/io/grpc/protobuf/services/BinlogHelperTest.java
@@ -341,6 +341,18 @@ public final class BinlogHelperTest {
     assertNotNull(makeLog("-p.s/blacklisted,p.s/*", "p.s/allowed"));
   }
 
+  @Test
+  public void configBinLog_blacklist_wildcardOrOptionsRejected() {
+    for (String configStr : new String[] {"*,-p.s/*", "-p.s/*", "-*", "-p.s/m{h}"}) {
+      try {
+        new FactoryImpl(sink, configStr);
+        fail("Expected IllegalArgumentException for " + configStr);
+      } catch (IllegalArgumentException expected) {
+        assertThat(expected).hasMessageThat().startsWith("Illegal log config pattern");
+      }
+    }
+  }
+
   private void assertDuplicatelPatternDetected(String factoryConfigStr) {
     try {
       new BinlogHelper.FactoryImpl(sink, factoryConfigStr);

./gradlew :grpc-services:test --tests io.grpc.protobuf.services.BinlogHelperTest -PskipCodegen=true -PskipAndroid=true

BinlogHelperTest > configBinLog_blacklist_wildcardOrOptionsRejected FAILED
    java.lang.AssertionError: Expected IllegalArgumentException for *,-p.s/*
        at org.junit.Assert.fail(Assert.java:89)
        at io.grpc.protobuf.services.BinlogHelperTest.configBinLog_blacklist_wildcardOrOptionsRejected(BinlogHelperTest.java:349)

172 tests completed, 1 failed

Environment:

• Commit: 15d64a4678450991aa5bf7af23066d36b3391351
• Toolchain: OpenJDK 21.0.2, Gradle 8.14.5

Already fixed in grpc/grpc-go

PR: https://github.com/grpc/grpc-go/pull/2299 ("binarylog: implement env variable config parsing", merged 2018-09-26T20:03:55Z, merge commit a338994886e). grpc-go's parser for the same A16 config string handles a leading - first and rejects both a * method and a {...} suffix in exclusions.

How grpc-go fixed it (relevant hunks, verbatim):

--- a/internal/binarylog/env_config.go
+++ b/internal/binarylog/env_config.go
@@ -0,0 +1,206 @@
...
+	// "-service/method", blacklist, no * or {} allowed.
+	if config[0] == '-' {
+		s, m, suffix, err := parseMethodConfigAndSuffix(config[1:])
+		if err != nil {
+			return fmt.Errorf("invalid config: %q, %v", config, err)
+		}
+		if m == "*" {
+			return fmt.Errorf("invalid config: %q, %v", config, "* not allowd in blacklist config")
+		}
+		if suffix != "" {
+			return fmt.Errorf("invalid config: %q, %v", config, "header/message limit not allowed in blacklist config")
+		}
+		if err := l.setBlacklist(s + "/" + m); err != nil {
+			return fmt.Errorf("invalid config: %v", err)
+		}
+		return nil
+	}
...

Suggested fix

Reject an exclusion that contains * or has a {...} suffix before the pattern is classified, using the same Illegal log config pattern error as other malformed entries:

diff --git a/services/src/main/java/io/grpc/protobuf/services/BinlogHelper.java b/services/src/main/java/io/grpc/protobuf/services/BinlogHelper.java
--- a/services/src/main/java/io/grpc/protobuf/services/BinlogHelper.java
+++ b/services/src/main/java/io/grpc/protobuf/services/BinlogHelper.java
@@ -635,6 +635,11 @@ final class BinlogHelper {
           if (methodOrSvc.isEmpty()) {
             throw new IllegalArgumentException("Illegal log config pattern: " + configuration);
           }
+          if (methodOrSvc.startsWith("-")
+              && (binlogOptionStr != null || methodOrSvc.contains("*"))) {
+            // An exclusion names exactly one method; '*' and '{...}' are not allowed
+            throw new IllegalArgumentException("Illegal log config pattern: " + configuration);
+          }
           if (methodOrSvc.equals("*")) {
             // parse config for "*"
             checkState(

Verified on 15d64a4 (OpenJDK 21.0.2, Gradle 8.14.5): the test above fails without this patch and passes with it; no other test results change (:grpc-services:test).

I can submit a PR for this.

Checked: searches for binlog blacklist/exclusion wildcard, BinlogHelper config pattern and GRPC_BINARY_LOG_CONFIG exclusions found no existing issue or PR about this.