alts: AuthorizationUtil.clientAuthorizationCheck compares the peer service account case-sensitively

Abierto Apto para principiantes
#13,156 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
2/5
Tiempo estimado
1-3 horas
Aptitud para principiantes
85/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
java

Línea de trabajo

La comparación se encuentra en clientAuthorizationCheck en alts/src/main/java/io/grpc/alts/AuthorizationUtil.java, que actualmente usa Collection.contains, una comprobación de coincidencia exacta. Lee ese método y el AuthorizationUtilTest existente, y luego añade el caso de prueba de regresión sin distinción de mayúsculas y minúsculas del issue. Está terminado cuando la nueva prueba pasa, las pruebas existentes siguen pasando con ./gradlew :grpc-alts:test y el Javadoc indica que la coincidencia no distingue mayúsculas y minúsculas.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

alts: AuthorizationUtil.clientAuthorizationCheck compares the peer service account case-sensitively


**正文(Markdown 原文)**

`````markdown
### What version of gRPC-Java are you using?

`master` at [`15d64a4`](https://github.com/grpc/grpc-java/commit/15d64a4678450991aa5bf7af23066d36b3391351) (`1.86.0-SNAPSHOT`); `AuthorizationUtil` 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?

`AuthorizationUtil.clientAuthorizationCheck(call, expectedServiceAccounts)` accepts a client whose ALTS peer service account differs from an allowed entry only in letter case (for example peer `alice`, allowed `ALICE`), as grpc-go's equivalent `alts.ClientAuthorizationCheck` does.

### What did you see instead?

It returns `PERMISSION_DENIED` ("Client alice is not authorized").

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

- [`alts/src/main/java/io/grpc/alts/AuthorizationUtil.java:35-47`](https://github.com/grpc/grpc-java/blob/15d64a4678450991aa5bf7af23066d36b3391351/alts/src/main/java/io/grpc/alts/AuthorizationUtil.java#L35-L47)

```java
  public static Status clientAuthorizationCheck(
      ServerCall<?, ?> call, Collection<String> expectedServiceAccounts) {
    ...
    if (expectedServiceAccounts.contains(altsContext.getPeerServiceAccount())) {
      return Status.OK;
    }
    return Status.PERMISSION_DENIED.withDescription(
        "Client " + altsContext.getPeerServiceAccount() + " is not authorized");
  }

Problem

Collection.contains() uses String.equals(), so the check is exact-match. Service account identifiers are e-mail style names that people often write in mixed case in config. grpc-go changed its helper for exactly this reason ("expectedServiceAccounts are commonly mixed-case, while the PeerServiceAccount is lower-case"). With the Java helper, an allow-list entry that differs only in case rejects a legitimate client, and the two languages give different answers for the same allow-list. This only concerns letter case; any other difference in the name is still rejected.

Steps to reproduce the bug

Reproduction

Regression test added to AuthorizationUtilTest (reuses its FakeServerCall, which sets the ALTS peer service account):

diff --git a/alts/src/test/java/io/grpc/alts/AuthorizationUtilTest.java b/alts/src/test/java/io/grpc/alts/AuthorizationUtilTest.java
--- a/alts/src/test/java/io/grpc/alts/AuthorizationUtilTest.java
+++ b/alts/src/test/java/io/grpc/alts/AuthorizationUtilTest.java
@@ -55,6 +55,14 @@ public final class AuthorizationUtilTest {
     assertThat(status.getDescription()).endsWith("not authorized");
   }
 
+  @Test
+  public void altsAuthorizationCheck_ignoresCase() throws Exception {
+    Status status =
+        AuthorizationUtil.clientAuthorizationCheck(
+            new FakeServerCall("alice"), Lists.newArrayList("ALICE", "Bob"));
+    assertThat(status.getCode()).isEqualTo(Status.Code.OK);
+  }
+
   private static class FakeServerCall extends ServerCall<String, String> {
     final Attributes attrs;
 

./gradlew :grpc-alts:test --tests io.grpc.alts.AuthorizationUtilTest -PskipCodegen=true -PskipAndroid=true

AuthorizationUtilTest > altsAuthorizationCheck_ignoresCase FAILED
    value of: getCode()
    expected: OK
    but was : PERMISSION_DENIED
        at app//io.grpc.alts.AuthorizationUtilTest.altsAuthorizationCheck_ignoresCase(AuthorizationUtilTest.java:63)

152 tests completed, 1 failed

Environment:

Already fixed in grpc/grpc-go

PR: https://github.com/grpc/grpc-go/pull/3792 ("credentials/alts: ClientAuthorizationCheck to case-fold compare of peer SA", merged 2020-09-01T23:23:56Z, merge commit 48bf772d0c2). It switched the peer service account comparison to strings.EqualFold.

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

--- a/credentials/alts/utils.go
+++ b/credentials/alts/utils.go
@@ -152,12 +152,13 @@ func AuthInfoFromPeer(p *peer.Peer) (AuthInfo, error) {
 func ClientAuthorizationCheck(ctx context.Context, expectedServiceAccounts []string) error {
 	authInfo, err := AuthInfoFromContext(ctx)
 	if err != nil {
-		return status.Newf(codes.PermissionDenied, "The context is not an ALTS-compatible context: %v", err).Err()
+		return status.Errorf(codes.PermissionDenied, "The context is not an ALTS-compatible context: %v", err)
 	}
+	peer := authInfo.PeerServiceAccount()
 	for _, sa := range expectedServiceAccounts {
-		if authInfo.PeerServiceAccount() == sa {
+		if strings.EqualFold(peer, sa) {
 			return nil
 		}
 	}
-	return status.Newf(codes.PermissionDenied, "Client %v is not authorized", authInfo.PeerServiceAccount()).Err()
+	return status.Errorf(codes.PermissionDenied, "Client %v is not authorized", peer)
 }

Suggested fix

Compare the peer service account to each expected entry with equalsIgnoreCase, and document it:

diff --git a/alts/src/main/java/io/grpc/alts/AuthorizationUtil.java b/alts/src/main/java/io/grpc/alts/AuthorizationUtil.java
--- a/alts/src/main/java/io/grpc/alts/AuthorizationUtil.java
+++ b/alts/src/main/java/io/grpc/alts/AuthorizationUtil.java
@@ -29,8 +29,8 @@ public final class AuthorizationUtil {
 
   /**
    * Given a server call, performs client authorization check, i.e., checks if the client service
-   * account matches one of the expected service accounts. It returns OK if client is authorized and
-   * an error otherwise.
+   * account matches one of the expected service accounts, ignoring case. It returns OK if client
+   * is authorized and an error otherwise.
    */
   public static Status clientAuthorizationCheck(
       ServerCall<?, ?> call, Collection<String> expectedServiceAccounts) {
@@ -39,10 +39,13 @@ public final class AuthorizationUtil {
     if (altsContext == null) {
       return Status.PERMISSION_DENIED.withDescription("Peer ALTS AuthContext not found");
     }
-    if (expectedServiceAccounts.contains(altsContext.getPeerServiceAccount())) {
-      return Status.OK;
+    String peerServiceAccount = altsContext.getPeerServiceAccount();
+    for (String serviceAccount : expectedServiceAccounts) {
+      if (peerServiceAccount.equalsIgnoreCase(serviceAccount)) {
+        return Status.OK;
+      }
     }
     return Status.PERMISSION_DENIED.withDescription(
-        "Client " + altsContext.getPeerServiceAccount() + " is not authorized");
+        "Client " + peerServiceAccount + " is not authorized");
   }
 }

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-alts:test).

I can submit a PR for this.

Checked: searches for clientAuthorizationCheck case, ALTS service account case-insensitive and AuthorizationUtil found only #11042 and #7840 (ALTS context API changes), not this comparison.

Lenguaje dominante
Java
Estrellas
12.1k
Forks
4k
Merge medio
1 d 13 h
PR fusionados (30 d)
42

Preparar el entorno

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de grpc/grpc-java

Todos los issues de grpc/grpc-java

Issues similares

Más issues de Java

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.