xds: RBAC `Authenticated` principal without `principal_name` is rejected ("Unknown StringMatcher match pattern: MATCHPATTERN_NOT_SET")
Mantenedores costumam responder em até 1 dia
Avaliação
- Dificuldade
- 2/5
- Tempo estimado
- 1-3 horas
- Facilidade para iniciantes
- 78/100
- Tipo de issue
- Bug
- Clareza
- Claramente especificada
- Status de atividade
- Ativa
- Stack de tecnologia
- java
- Domínio
- authorization, security
Direção de pesquisa
Comece em parseAuthenticatedMatcher em xds/src/main/java/io/grpc/xds/RbacFilter.java, que sempre passa proto.getPrincipalName() para MatcherParser.parseStringMatcher. Verifique como AuthenticatedMatcher.create(null) é tratado em GrpcAuthorizationEngine.java. Está concluído quando os dois testes de RbacFilterAuthenticatedMatcherTest da issue fazem parse e match, e os testes RBAC existentes continuam passando.
Escrita pelo modelo de indexação a partir do texto da issue.
Descrição
What version of gRPC-Java are you using?
master at 15d64a4 (1.86.0-SNAPSHOT). The code involved is unchanged on the current master.
What is your environment?
Linux 5.15, OpenJDK 21.0.2, Gradle 8.14.5 (wrapper).
What did you expect to see?
An RBAC policy whose principal is authenticated: {} (no principal_name) should be accepted and should match any TLS connection. The Envoy Principal.Authenticated docs say "If unset, it applies to any user that is authenticated", and gRFC A41 says:
If
principal_nameis unset, thenAuthenticatedis said to match if the connection uses TLS; a client certificate is not necessary.
What did you see instead?
The RBAC filter config fails to parse with Encountered error parsing policy: java.lang.IllegalArgumentException: Unknown StringMatcher match pattern: MATCHPATTERN_NOT_SET.
Location (grpc/grpc-java @ 15d64a4):
xds/src/main/java/io/grpc/xds/RbacFilter.java:335-339:parseAuthenticatedMatcheralways parsesproto.getPrincipalName().xds/src/main/java/io/grpc/xds/internal/MatcherParser.java:105-108:parseStringMatcherthrows for the default (unset)StringMatcher.xds/src/main/java/io/grpc/xds/RbacFilter.java:159-171:parseRbacConfigturns the exception into a config error.xds/src/main/java/io/grpc/xds/internal/rbac/engine/GrpcAuthorizationEngine.java:149-168:AuthenticatedMatcher.create(null)is already documented and implemented as "match all authenticated", but the parser never passesnull.
private static AuthenticatedMatcher parseAuthenticatedMatcher(
Principal.Authenticated proto) {
Matchers.StringMatcher matcher = MatcherParser.parseStringMatcher(proto.getPrincipalName());
return AuthenticatedMatcher.create(matcher);
}
Problem
When principal_name is unset, proto.getPrincipalName() returns the default StringMatcher instance, whose match pattern is MATCHPATTERN_NOT_SET. MatcherParser.parseStringMatcher throws for that case, so the whole RBAC config (RbacFilter.Provider.parseRbacConfig) is rejected, even though the engine already supports a null delegate that means "any authenticated connection". Only Authenticated principals without principal_name are affected. Matching with a principal_name set does not change.
Reproduction
RbacFilterAuthenticatedMatcherTest.java (xds/src/test/java/io/grpc/xds)
/*
* Copyright 2021 The gRPC Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.grpc.xds;
import static com.google.common.truth.Truth.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import io.envoyproxy.envoy.config.rbac.v3.Permission;
import io.envoyproxy.envoy.config.rbac.v3.Policy;
import io.envoyproxy.envoy.config.rbac.v3.Principal;
import io.envoyproxy.envoy.config.rbac.v3.Principal.Authenticated;
import io.envoyproxy.envoy.config.rbac.v3.RBAC;
import io.envoyproxy.envoy.config.rbac.v3.RBAC.Action;
import io.grpc.Attributes;
import io.grpc.Grpc;
import io.grpc.Metadata;
import io.grpc.MethodDescriptor;
import io.grpc.MethodDescriptor.MethodType;
import io.grpc.ServerCall;
import io.grpc.testing.TestMethodDescriptors;
import io.grpc.xds.internal.rbac.engine.GrpcAuthorizationEngine;
import io.grpc.xds.internal.rbac.engine.GrpcAuthorizationEngine.AuthDecision;
import java.security.cert.X509Certificate;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import javax.net.ssl.SSLSession;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
/**
* Tests for {@link RbacFilter} authenticated matcher with unset PrincipalName.
*/
@RunWith(JUnit4.class)
public class RbacFilterAuthenticatedMatcherTest {
private static final RbacFilter.Provider FILTER_PROVIDER = new RbacFilter.Provider();
/**
* Per the RBAC spec, an Authenticated principal with no PrincipalName set should match
* any authenticated user (i.e. any connection with a valid TLS certificate).
* This test verifies that parsing such a configuration succeeds.
*/
@Test
public void authenticatedMatcherWithoutPrincipalName_shouldParseSuccessfully() {
// Create an Authenticated principal without setting PrincipalName.
// Per the RBAC spec: "If unset, it applies to any user that is authenticated."
List<Permission> permissionList = Arrays.asList(
Permission.newBuilder().setAny(true).build());
List<Principal> principalList = Arrays.asList(
Principal.newBuilder().setAuthenticated(
Authenticated.newBuilder().build()).build());
io.envoyproxy.envoy.extensions.filters.http.rbac.v3.RBAC rbac =
io.envoyproxy.envoy.extensions.filters.http.rbac.v3.RBAC.newBuilder()
.setRules(RBAC.newBuilder().setAction(Action.ALLOW)
.putPolicies("any-authenticated-user",
Policy.newBuilder()
.addAllPermissions(permissionList)
.addAllPrincipals(principalList).build())
.build())
.build();
ConfigOrError<RbacConfig> result = RbacFilter.Provider.parseRbacConfig(rbac);
// The config should parse successfully without error.
assertThat(result.errorDetail).isNull();
assertThat(result.config).isNotNull();
assertThat(result.config.authConfig()).isNotNull();
}
/**
* An Authenticated principal with no PrincipalName should match any authenticated
* connection (one with an SSL session and peer certificates).
*/
@Test
@SuppressWarnings("unchecked")
public void authenticatedMatcherWithoutPrincipalName_shouldMatchAuthenticatedConnection()
throws Exception {
List<Permission> permissionList = Arrays.asList(
Permission.newBuilder().setAny(true).build());
List<Principal> principalList = Arrays.asList(
Principal.newBuilder().setAuthenticated(
Authenticated.newBuilder().build()).build());
io.envoyproxy.envoy.extensions.filters.http.rbac.v3.RBAC rbac =
io.envoyproxy.envoy.extensions.filters.http.rbac.v3.RBAC.newBuilder()
.setRules(RBAC.newBuilder().setAction(Action.ALLOW)
.putPolicies("any-authenticated-user",
Policy.newBuilder()
.addAllPermissions(permissionList)
.addAllPrincipals(principalList).build())
.build())
.build();
ConfigOrError<RbacConfig> result = RbacFilter.Provider.parseRbacConfig(rbac);
// Parsing must succeed first.
assertThat(result.errorDetail).isNull();
// Set up an authenticated connection with a mock SSL session and certificate.
SSLSession sslSession = mock(SSLSession.class);
X509Certificate mockCert = mock(X509Certificate.class);
when(sslSession.getPeerCertificates()).thenReturn(new X509Certificate[]{mockCert});
when(mockCert.getSubjectAlternativeNames()).thenReturn(
Collections.<List<?>>singletonList(Arrays.asList(6, "spiffe://example.com/service")));
ServerCall<Void, Void> serverCall = mock(ServerCall.class);
Attributes attributes = Attributes.newBuilder()
.set(Grpc.TRANSPORT_ATTR_SSL_SESSION, sslSession)
.build();
when(serverCall.getAttributes()).thenReturn(attributes);
when(serverCall.getMethodDescriptor()).thenReturn(
MethodDescriptor.<Void, Void>newBuilder()
.setType(MethodType.BIDI_STREAMING)
.setFullMethodName("test/method")
.setRequestMarshaller(TestMethodDescriptors.voidMarshaller())
.setResponseMarshaller(TestMethodDescriptors.voidMarshaller())
.build());
GrpcAuthorizationEngine engine =
new GrpcAuthorizationEngine(result.config.authConfig());
AuthDecision decision = engine.evaluate(new Metadata(), serverCall);
// An authenticated connection should be allowed by this policy.
assertThat(decision.decision()).isEqualTo(GrpcAuthorizationEngine.Action.ALLOW);
}
}
Run: ./gradlew :grpc-xds:test --tests io.grpc.xds.RbacFilterAuthenticatedMatcherTest -PskipCodegen=true -PskipAndroid=true
RbacFilterAuthenticatedMatcherTest > authenticatedMatcherWithoutPrincipalName_shouldMatchAuthenticatedConnection FAILED
expected: null
but was : Encountered error parsing policy: java.lang.IllegalArgumentException: Unknown StringMatcher match pattern: MATCHPATTERN_NOT_SET
at app//io.grpc.xds.RbacFilterAuthenticatedMatcherTest.authenticatedMatcherWithoutPrincipalName_shouldMatchAuthenticatedConnection(RbacFilterAuthenticatedMatcherTest.java:113)
RbacFilterAuthenticatedMatcherTest > authenticatedMatcherWithoutPrincipalName_shouldParseSuccessfully FAILED
expected: null
but was : Encountered error parsing policy: java.lang.IllegalArgumentException: Unknown StringMatcher match pattern: MATCHPATTERN_NOT_SET
at app//io.grpc.xds.RbacFilterAuthenticatedMatcherTest.authenticatedMatcherWithoutPrincipalName_shouldParseSuccessfully(RbacFilterAuthenticatedMatcherTest.java:82)
Environment: commit 15d64a4, OpenJDK 21.0.2, Gradle 8.14.5.
Already fixed in grpc/grpc-go
PR: https://github.com/grpc/grpc-go/pull/4576, "Add extra layer on top of RBAC Engine" (merged 2021-07-22T02:42:38Z, merge commit c513103bee3). grpc-go is another implementation of the same xDS RBAC spec. As part of that PR, newAuthenticatedMatcher treats a nil PrincipalName as "any authenticated user" instead of passing it to the string matcher parser.
How grpc-go fixed it (relevant hunks, verbatim):
--- a/internal/xds/rbac/matchers.go
+++ b/internal/xds/rbac/matchers.go
@@ -361,26 +361,56 @@ func newPortMatcher(destinationPort uint32) *portMatcher {
...
type authenticatedMatcher struct {
- stringMatcher internalmatcher.StringMatcher
+ stringMatcher *internalmatcher.StringMatcher
}
func newAuthenticatedMatcher(authenticatedMatcherConfig *v3rbacpb.Principal_Authenticated) (*authenticatedMatcher, error) {
+ // Represents this line in the RBAC documentation = "If unset, it applies to
+ // any user that is authenticated" (see package-level comments).
+ if authenticatedMatcherConfig.PrincipalName == nil {
+ return &authenticatedMatcher{}, nil
+ }
stringMatcher, err := internalmatcher.StringMatcherFromProto(authenticatedMatcherConfig.PrincipalName)
if err != nil {
return nil, err
}
- return &authenticatedMatcher{stringMatcher: stringMatcher}, nil
+ return &authenticatedMatcher{stringMatcher: &stringMatcher}, nil
}
...
+ if am.stringMatcher == nil {
+ return len(data.certs) != 0
+ }
...
Suggested fix
When principal_name is unset, create the AuthenticatedMatcher with a null delegate, which the engine already treats as "any authenticated connection":
diff --git a/xds/src/main/java/io/grpc/xds/RbacFilter.java b/xds/src/main/java/io/grpc/xds/RbacFilter.java
--- a/xds/src/main/java/io/grpc/xds/RbacFilter.java
+++ b/xds/src/main/java/io/grpc/xds/RbacFilter.java
@@ -334,6 +334,10 @@ final class RbacFilter implements Filter {
private static AuthenticatedMatcher parseAuthenticatedMatcher(
Principal.Authenticated proto) {
+ // An unset principal_name matches any authenticated (TLS) connection.
+ if (!proto.hasPrincipalName()) {
+ return AuthenticatedMatcher.create(null);
+ }
Matchers.StringMatcher matcher = MatcherParser.parseStringMatcher(proto.getPrincipalName());
return AuthenticatedMatcher.create(matcher);
}
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-xds:test: 4473 tests; FakeControlPlaneXdsIntegrationTest.changeClusterForRoute[enableRfc3986UrisParam=true] fails with and without the patch).
I can submit a PR for this.
I searched issues and PRs for RBAC Authenticated/principal_name handling and for the MATCHPATTERN_NOT_SET error, and found no existing report.
- Linguagem predominante
- Java
- Estrelas
- 12.1k
- Forks
- 4k
- Merge médio
- 2d 3h
- PRs com merge (30d)
- 41
Preparar o ambiente
- Sem Dockerfile nem arquivo Docker Compose
- Sem modelo de pull request
- Ler o guia de contribuição
Primeiros passos
- Leia a issue inteira e depois o guia de contribuição do projeto.
- Comente na issue dizendo que vai assumir — evita que duas pessoas façam o mesmo trabalho.
- Faça um fork do repositório e trabalhe em uma branch.
- Abra um pull request que referencie o número da issue.
Mais de grpc/grpc-java
-
Outlier detection: unset `failure_percent_ejection` results in 100% instead of 0%Talvez já em andamento @ahmadmaaz assumiu há 1 dia. Aberta
Dificuldade 1/5 Menos de uma hora Facilidade para iniciantes 88/100
grpc/grpc-java#13165 · 1 comentário ·
Mantenedores costumam responder em até 1 dia
-
Outlier detection can eject one too many endpointsTalvez já em andamento @goutamadwant assumiu hoje. Aberta
Dificuldade 2/5 1-3 horas Facilidade para iniciantes 82/100
grpc/grpc-java#13164 · 1 comentário ·
Mantenedores costumam responder em até 1 dia
-
alts: AuthorizationUtil.clientAuthorizationCheck compares the peer service account case-sensitivelyTalvez já em andamento @hidingmyname assumiu há 4 dias. Aberta
Dificuldade 2/5 1-3 horas Facilidade para iniciantes 85/100
grpc/grpc-java#13156 · 1 comentário ·
Mantenedores costumam responder em até 1 dia
-
Binary log config: `-service/*` exclusion is accepted and silently turned into a per-service rule, so the service is still loggedTalvez já em andamento @hidingmyname assumiu há 4 dias. Abertabug
Dificuldade 2/5 1-3 horas Facilidade para iniciantes 82/100
grpc/grpc-java#13155 · 1 comentário ·
Mantenedores costumam responder em até 1 dia
-
SpiffeUtil.loadTrustBundleFromFile accepts invalid trust domain names (e.g. `UPPER.example.com`, `invalid#character`) as bundle map keysTalvez já em andamento @hidingmyname assumiu há 4 dias. Abertabug
Dificuldade 2/5 1-3 horas Facilidade para iniciantes 82/100
grpc/grpc-java#13154 · 1 comentário ·
Mantenedores costumam responder em até 1 dia
Todas as issues de grpc/grpc-java
Issues semelhantes
-
Document and enforce pull request review guidelinesTalvez já em andamento @dashorst assumiu hoje. Aberta
Dificuldade 2/5 1-3 horas Facilidade para iniciantes 76/100
Mantenedores costumam responder em até 1 dia
-
`Time.at(t, in: nil)` returns UTC instead of the local time (or the time zone of `t`), unlike CRubyAberta
Dificuldade 2/5 1-3 horas Facilidade para iniciantes 78/100
Mantenedores costumam responder em até 1 dia
-
hermes-queue
Dificuldade 1/5 Menos de uma hora Facilidade para iniciantes 72/100
chrisalunlloyd2-sudo/mindpalace#349 ·
Mantenedores costumam responder em até 1 dia
-
Retry-After parser accepts invalid delay-seconds valuesTalvez já em andamento @Dhruvraj-Rana assumiu hoje. AbertaBug
Dificuldade 2/5 1-3 horas Facilidade para iniciantes 82/100
open-telemetry/opentelemetry-java#9018 ·
Mantenedores costumam responder em até 2 dias
-
dropwizard-testing: DAOTest.inTransaction leaves the transaction open when an Error is thrownAberta
Dificuldade 2/5 1-3 horas Facilidade para iniciantes 72/100
dropwizard/dropwizard#11410 ·
Mantenedores costumam responder em até 1 dia