feat(xds): Implement CheckRequestBuilder for external authorization - #12493
feat(xds): Implement CheckRequestBuilder for external authorization #12493sauravzg wants to merge 3 commits into
Conversation
Sorry, something went wrong.
| /** | ||
| * 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
| } | ||
| } | ||
| } 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.
Sorry, something went wrong.
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?
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
@ejona86 PTAL. I've addressed comments. I think it should be up to spec, but I am slightly worried about
|
Sorry, something went wrong.
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.
| /** | ||
| * 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.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
This PR sits on top of #12492, so only the last commit + any fixups need to be reviewed.
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:
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.
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.