xds: implement RBAC gRFC misc cases #8518
Conversation
YifeiZhuang
added
the
TODO:backport
xds/src/main/java/io/grpc/xds/internal/rbac/engine/GrpcAuthorizationEngine.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Don't talk about the checklist doc for this PR; those are not authoritative, isn't widely shared, and is a living document so probably won't be useful very long after this is submitted. Instead, talk about the gRFCs. Everything here is in the gRFCs. If it is just "misc special cases," that's fine.
xds/src/main/java/io/grpc/xds/internal/rbac/engine/GrpcAuthorizationEngine.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/io/grpc/xds/internal/rbac/engine/GrpcAuthorizationEngine.java
Outdated
Show resolved
Hide resolved
| headerName = "authority"; | ||
| } | ||
| String value = deserializeHeader(headerName); | ||
| if (value == null && ":method".equals(headerName)) { |
There was a problem hiding this comment.
There's no code for :path here. If you are aware and were going to do it later, that's fine.
xds/src/test/java/io/grpc/xds/internal/rbac/engine/GrpcAuthorizationEngineTest.java
Outdated
Show resolved
Hide resolved
xds/src/test/java/io/grpc/xds/internal/rbac/engine/GrpcAuthorizationEngineTest.java
Outdated
Show resolved
Hide resolved
YifeiZhuang
changed the title
xds/src/main/java/io/grpc/xds/internal/rbac/engine/GrpcAuthorizationEngine.java
Outdated
Show resolved
Hide resolved
| } | ||
| String value = deserializeHeader(headerName); | ||
| if (value == null && ":method".equals(headerName)) { | ||
| return "POST"; | ||
| String apiMethod = serverCall.getMethodDescriptor().getFullMethodName(); |
There was a problem hiding this comment.
No, no, no. This is the HTTP method, like GET/PUT/POST. It has no relation to the grpc method. This was correct before.
There was a problem hiding this comment.
lol, okay, I keep misunderstand it. It might help if no :method in the first sentence. How about ":path" just added.
For this design, headers can include :method, :authority, and :path matchers and they should match the values received on-the-wire independent of whether they are stored in Metadata or in separate APIs. :method can be hard-coded to POST if unavailable and a code audit confirms the server denies requests for all other method types.
.
There was a problem hiding this comment.
What do you mean, "no :method in the first sentence"? It absolutely needs to be there, otherwise you wouldn't have this code here.
Seems you are missing :authority still as well.
Seems you should remove the value == null part for :method and move next to :path, now that I can compare to your intentions for :path. :method will always be on-the-wire. The "if unavailable" was "if you don't know the value", and it will never be returned from deserializeHeader() as creating the Key will always throw IllegalArgumentException.
There was a problem hiding this comment.
Right.
I meant that headers can include :method....independent of whether they are stored in Metadata or in separate APIs makes me think that if header matcher has a name :method then they are stored in API: serverCall.getMethodDescriptor().getFullMethodName(), similar to :authority -> serverCall.getAuthority(), or :path. But that was wrong.
There was a problem hiding this comment.
Remember, this is :path:
If you saw that inline instead of "hidden" behind the getPath() method, it would have probably been more obvious that something was wrong.
| for (byte[] v : values) { | ||
| String encoded = new String(v, US_ASCII); | ||
| decoded.add(new String(BaseEncoding.base64().decode(encoded), US_ASCII)); | ||
| encoded.add(BaseEncoding.base64().encode(v)); |
There was a problem hiding this comment.
We need omitPadding() as well, per https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md
xds/src/test/java/io/grpc/xds/internal/rbac/engine/GrpcAuthorizationEngineTest.java
Outdated
Show resolved
Hide resolved
YifeiZhuang
removed
the
TODO:backport
YifeiZhuang
added
the
TODO:backport
ejona86
removed
the
TODO:backport
No description provided.