-
Notifications
You must be signed in to change notification settings - Fork 6
[Feature] add Access Token Authorization for gRPC Proxy Handler #57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[Feature] add Access Token Authorization for gRPC Proxy Handler #57
Conversation
a9d139c
to
5583a05
Compare
847fa27
to
e74604f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces access token authorization support for the gRPC proxy handler, enhancing the authorization system to support both role token and access token authentication methods. The changes include significant refactoring of the gRPC handler, configuration improvements, and dependency updates.
- Refactored gRPC handler to support dual authorization methods (role tokens and access tokens)
- Added configuration options for access token header specification
- Updated Go version to 1.25.1 and modernized dependencies
Reviewed Changes
Copilot reviewed 15 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
handler/grpc.go | Major refactoring to support both role token and access token authorization with improved error handling |
handler/grpc_option.go | Added new configuration option for access token settings |
handler/error.go | Added new error constants for access token authorization |
config/config.go | Added AccessTokenAuthHeader field and improved documentation |
usecase/authz_proxyd.go | Added access token configuration to handler initialization |
handler/handler.go | Added wildcardMatch utility function for path matching |
handler/transport.go | Enhanced health check path matching with wildcard support |
go.mod | Updated Go version and dependencies |
Makefile | Added Docker build targets and formatting commands |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
42e7024
to
0e1e0cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 15 out of 18 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 15 out of 18 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Thank you for creating the PR!! Could you please split the PR into the following five sections so changes are clear?
|
@fsul7o Thank you for your quick review. The config package needs to be modified when adding gRPC access token authorization logic, so I'm updating it simultaneously. While the build and formatting improvements could be separated, I didn't consider them significant enough to warrant a separate review, so I left them as-is. The wildcardMatch and OriginHealthCheckPath for gRPC feature hadn't been merged for over a year in another PR, so I've included it here. I'd appreciate it if you could review this together with the other changes. To summarize:
|
I understand. Could you please split this into the following two PRs? Even if I split them, I'll review both.
|
Also, regarding the Go version, could you revert it from 1.25 back to 1.23? This is because we haven't fully assessed the impact of upgrading to go1.25, and there may be unintended consequences beyond the addition of gRPC functionality. |
@fsul7o Regarding the Go version, since my environment is always kept up to date, the upgrade happened automatically when I organized the dependencies. While it’s certainly possible to split this PR into two, do you really think it’s worth reviewing two separate PRs for just about 30 lines of code? Finally, this patch (including the Go version and utility changes) has already been tested and is running successfully on the Internal Platform. |
We plan to update the Go version of authorization-proxy to 1.25. However, since we haven't investigated the impact of Go version updates on authorization-proxy, we want to perform that investigation before raising the Go version. Updating the Go version of authorization-proxy requires multiple steps, such as updating related libraries (e.g., athenz-authorizer) to newer Go versions and investigating potential impacts. Therefore, if we proceed with the version upgrade in this PR, it will require time for review and merging. If we use Go 1.23, we can review and merge this PR quickly. -- Also, splitting the PR is not necessary. We apologize for the inconvenience. |
@fsul7o Thank you for the feedback and for sharing your concerns about potential regressions when upgrading Go. It’s also important to clarify that Go modules do not require us to upgrade athenz-authorizer to Go 1.25 first. The go directive in a module’s go.mod defines the minimum language/toolchain features used by that module; it still builds fine under newer Go compilers. In other words, athenz-authorizer and authorization-proxy do not need to run on the same Go version—aligning versions is optional, not a requirement. We’ve already verified that authorization-proxy and its dependent modules (including athenz-authorizer) build and pass all tests(unit tests by make test and E2E using private cloud platform) under Go 1.25.1, so the impact investigation you’re concerned about has effectively been performed. This means authorization-proxy can move forward independently without waiting for other components. Because AthenZ is positioned as a security platform, upgrading Go proactively will strengthen our security posture, reduce supply-chain risk, and keep us aligned with the Go ecosystem. Finally, since authorization-proxy follows a semantic versioned release process, merging this PR will not immediately affect end users. Thank you also for allowing me not to split the PR. |
e443ead
to
e0c490d
Compare
Signed-off-by: kpango <kpango@vdaas.org>
e0c490d
to
028e940
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 16 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
handler/grpc.go:1
- The removal of grpc.WithCodec(proxy.Codec()) might break proxy functionality. The proxy codec is typically required for transparent gRPC proxying to work correctly.
// Copyright 2023 LY Corporation
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
I apologize for the delayed response. Our team has also discussed this matter. We would like to proceed as follows:
The go1.25 compatibility work will be carried out between this week and next week. Once it is complete, we will review this PR and merge it if there are no issues. We expect to release the version of authorization-proxy incorporating this PR by the end of October. |
@fsul7o Thank you for the update and for discussing this internally. I appreciate your plan to complete the Go 1.25 compatibility work for authorization-proxy and to review and merge this PR afterward. That schedule sounds good to me, and I’m happy to assist or provide any additional information if needed during the compatibility work. I look forward to seeing the version including this PR released by the end of October. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed it, so please check it.
Co-authored-by: fsul7o <75571344+fsul7o@users.noreply.github.com> Signed-off-by: Yusuke Kato <kpango@vdaas.org>
7a37b45
to
3d15ef5
Compare
Signed-off-by: kpango <kpango@vdaas.org>
3d15ef5
to
6ee8395
Compare
Signed-off-by: kpango <kpango@vdaas.org>
Signed-off-by: kpango <kpango@vdaas.org>
Co-authored-by: fsul7o <75571344+fsul7o@users.noreply.github.com> Signed-off-by: Yusuke Kato <kpango@vdaas.org>
The account settings for Docker Build in pull requests from external CI contributors are not configured correctly, causing this PR's Docker Build to fail at the login stage. However, since we have performed local build checks and functionality verification, we believe this is not an issue. |
I am the same understanding. Currently, we are considering the merging method. Please wait a moment. |
I created PR #59 as a pull request that should allow CI/CD to function correctly. Could you please change the merge target for this PR to the grpc-access-token branch of #59 and merge it? (I believe the reason CI/CD fails with this PR is because it was created from a forked repository.) Additionally, The planned release schedule moving forward is as follows:
We plan to space these two releases about one week apart. The release of the gRPC reverse proxy with access token support is scheduled to be completed by the end of October. |
Description
This pull request introduces several significant improvements and refactoring to the authorization proxy, focusing on enhanced gRPC authorization logic, configuration flexibility, and modernization of dependencies. The most impactful changes include refactoring the gRPC handler to support both role and access token authorization, adding configuration options for access token headers, and updating dependencies for better compatibility and security.
gRPC Authorization Logic Enhancements
GRPCHandler
inhandler/grpc.go
to support both role token and access token authorization, including new methods for token extraction and validation, improved error handling, and client certificate support for access token validation. [1] [2] [3] [4] [5]Configuration Improvements
AccessTokenAuthHeader
to theAccessToken
config struct inconfig/config.go
to allow specifying the request header key for extracting access tokens. Also clarified the documentation forRoleAuthHeader
.Dependency Updates
go.mod
andgo.mod.default
to newer versions for improved compatibility, performance, and security. [1] [2] [3]Build and Formatting Improvements
Makefile
with new variables, Docker build targets, and formatting commands to streamline the build process and code formatting. [1] [2]Utility Functions
wildcardMatch
utility function inhandler/handler.go
to support wildcard matching for health check paths and authorization skipping logic.These changes collectively improve the flexibility, maintainability, and security of the authorization proxy.
Type of change
Flags
Checklist
[skip ci]
/[ci skip]
/[no ci]
/[skip actions]
/[actions skip]
in the PR title if necessaryChecklist for maintainer
Squash and merge
[skip ci]
/[ci skip]
/[no ci]
/[skip actions]
/[actions skip]