feat(xds): Implement CheckRequestBuilder for external authorization #12493
feat(xds): Implement CheckRequestBuilder for external authorization #12493sauravzg wants to merge 3 commits into
Conversation
sauravzg
changed the title
sauravzg
force-pushed
the
feat/request-builder
branch
5 times, most recently
from
7b5cd3c to
2d08a46
Compare
| /** | ||
| * Interface for building external authorization check requests. | ||
| */ | ||
| public interface CheckRequestBuilder { |
There was a problem hiding this comment.
What is the point of this interface existing? It doesn't seem to be for injection. The only time it is used with an implementation other than CheckRequestBuilderImpl seems to be as a mock... that has no calls to it within the tests. So that means it isn't used for verification or for injection of behavior.
The Factory is even more baffling.
There was a problem hiding this comment.
Removed. Had a discussion with Kannan on moving the responsibility of injection to the caller instead of the callee which somewhat made sense.
To anwer the question about FooFactory, Foo which will be a recurring pattern in the child PRs.
The idea is analogous to FilterProvider vs Filter.
Inject factories into the FilterProvider, so that when creating the interceptor these factories can return mock instances and so on. The sole advantage being unit test coverage of Interceptor construction.
| } | ||
| } | ||
| } catch (java.security.cert.CertificateParsingException e) { | ||
| logger.log(Level.WARNING, "Error parsing certificate SANs. " + "This is not expected," |
There was a problem hiding this comment.
Remove the unnecessary string concatenation.
There was a problem hiding this comment.
Have reduced the log message size. But for my education, what's the correct way to avoid checkstyle errors for long strings without concatenation?
| Iterable<byte[]> binaryValues = | ||
| headers.getAll(Metadata.Key.of(key, Metadata.BINARY_BYTE_MARSHALLER)); | ||
| if (binaryValues == null) { | ||
| // Unreachable code, since we iterate over the keys. Exists for defensive programming. |
There was a problem hiding this comment.
Delete this. It means there is a bug. You've gone out of your way to use Optional for something impossible. If Optional was needed for other reasons such that you'd be reusing the error handling, I could understand it. But there's no point here, and it makes the code more likely to have bugs. Delete the Optional.
There was a problem hiding this comment.
Fixed.
But wouldn't best practices dictate to defensively program and not make any assumptions about your caller situation and make sure the method works well in isolation?
There are two cases in which the current function is incorrect in isolation now
- A new method decides to call this method without ensuring the key's presence.
- We assume "key being present" guarantees non null, which may or may not hold true depending on Metadata's implementation in theory, in which case our assumption is invalid.
sauravzg
force-pushed
the
feat/request-builder
branch
2 times, most recently
from
b97c3be to
7afce33
Compare
|
@ejona86 PTAL. I've addressed comments. I think it should be up to spec, but I am slightly worried about
|
sauravzg
force-pushed
the
feat/request-builder
branch
3 times, most recently
from
5669413 to
0e70dee
Compare
This commit introduces the `CheckRequestBuilder` library, which is responsible for constructing the `CheckRequest` message sent to the external authorization service. The `CheckRequestBuilder` gathers information from various sources, including: - `ServerCall` attributes (local and remote addresses, SSL session). - `MethodDescriptor` (full method name). - Request headers. It uses this information to populate the `AttributeContext` of the `CheckRequest` message, which provides the authorization service with the necessary context to make an authorization decision. This commit also introduces the `ExtAuthzCertificateProvider`, a helper class for extracting certificate information, such as the principal and PEM-encoded certificate. Unit tests for the new components are also included.
sauravzg
force-pushed
the
feat/request-builder
branch
from
0e70dee to
ea26e76
Compare
| /** | ||
| * A utility class for certificate-related information. | ||
| */ | ||
| final class CertificateUtils { |
There was a problem hiding this comment.
@kannanjgithub Would like a closer look at this particular file, since my familiarity with certificates, their specifications and existing battle tested utilities in grpc java or java stdlib is very limited.
2 participants
This PR sits on top of #12492, so only the last commit + any fixups need to be reviewed.
This commit introduces the
CheckRequestBuilderlibrary, which is responsible for constructing theCheckRequestmessage sent to the external authorization service.The
CheckRequestBuildergathers information from various sources, including:ServerCallattributes (local and remote addresses, SSL session).MethodDescriptor(full method name).It uses this information to populate the
AttributeContextof theCheckRequestmessage, which provides the authorization service with the necessary context to make an authorization decision.This commit also introduces the
ExtAuthzCertificateProvider, a helper class for extracting certificate information, such as the principal and PEM-encoded certificate.The relevant section of the spec is: https://github.com/grpc/proposal/pull/481/files#diff-6bb76a24aa142cc33db9218509688f01b30c8885d2fd8849f164244e68cd54eaR196-R250
Unit tests for the new components are also included.