Skip to content

Conversation

kpango
Copy link
Contributor

@kpango kpango commented Sep 30, 2025

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

  • Refactored GRPCHandler in handler/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

  • Added AccessTokenAuthHeader to the AccessToken config struct in config/config.go to allow specifying the request header key for extracting access tokens. Also clarified the documentation for RoleAuthHeader.

Dependency Updates

  • Updated Go version to 1.25.1 and refreshed multiple dependencies in go.mod and go.mod.default to newer versions for improved compatibility, performance, and security. [1] [2] [3]

Build and Formatting Improvements

  • Enhanced the Makefile with new variables, Docker build targets, and formatting commands to streamline the build process and code formatting. [1] [2]

Utility Functions

  • Added a wildcardMatch utility function in handler/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

  • Bug fix
  • New feature
  • Refactoring (no functional changes, no api changes)
  • Non-code changes (update documentation, pipeline, etc.)

Flags

  • Breaks backward compatibility
  • Requires a documentation update
  • Has untestable code

Checklist

  • Followed the guidelines in the CONTRIBUTING document
  • Added prefix [skip ci]/[ci skip]/[no ci]/[skip actions]/[actions skip] in the PR title if necessary
  • Commented the code
  • Made corresponding changes to the documentation
  • Passed all pipeline checking

Checklist for maintainer

  • Use Squash and merge
  • Double-confirm the merge message has prefix [skip ci]/[ci skip]/[no ci]/[skip actions]/[actions skip]
  • Delete the branch after merge

@kpango kpango force-pushed the feature/add/access-token-authorization-for-grpc-proxy branch 6 times, most recently from a9d139c to 5583a05 Compare September 30, 2025 05:09
@kpango kpango changed the title [WIP] [Feature] add Access Token Authorization for gRPC Proxy Handler [Feature] add Access Token Authorization for gRPC Proxy Handler Sep 30, 2025
@kpango kpango force-pushed the feature/add/access-token-authorization-for-grpc-proxy branch 3 times, most recently from 847fa27 to e74604f Compare September 30, 2025 08:02
@kpango kpango mentioned this pull request Sep 30, 2025
10 tasks
@mlajkim mlajkim requested a review from Copilot September 30, 2025 08:46
Copy link

@Copilot Copilot AI left a 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.

@kpango kpango force-pushed the feature/add/access-token-authorization-for-grpc-proxy branch 3 times, most recently from 42e7024 to 0e1e0cf Compare September 30, 2025 09:13
@kpango kpango requested a review from Copilot September 30, 2025 09:18
Copy link

@Copilot Copilot AI left a 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.

@kpango kpango requested a review from Copilot September 30, 2025 09:20
Copy link

@Copilot Copilot AI left a 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.

@fsul7o
Copy link
Contributor

fsul7o commented Sep 30, 2025

@kpango

Thank you for creating the PR!!

Could you please split the PR into the following five sections so changes are clear?

  • gRPC Authorization Logic Enhancements
  • Configuration Improvements
  • Dependency Updates
  • Build and Formatting Improvements
  • Utility Functions

@kpango
Copy link
Contributor Author

kpango commented Sep 30, 2025

@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.
Additionally, I had to use new dependencies like the grpc/peer package, so I'm updating those as well, that's why I've updated go modules.

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 build format utility only affects the Makefile and does not touch any CI, so the impact is minimal.
If you still think splitting the PR is necessary, I will remove it rather than split it, as it was prepared as a useful build script for this implementation.

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:

  • gRPC Authorization Logic Enhancements: Cannot be split
  • Configuration Improvements: Cannot be split
  • Dependency Updates: Cannot be split
  • Build and Formatting Improvements: Can be removed
  • Utility Functions: Ignored for over a year in a separate PR, so please review together

@fsul7o
Copy link
Contributor

fsul7o commented Oct 1, 2025

@kpango

I understand.

Could you please split this into the following two PRs? Even if I split them, I'll review both.

  • PR: 1
    • gRPC Authorization Logic Enhancements
    • Configuration Improvements
    • Dependency Updates
    • Build and Formatting Improvements
  • PR: 2
    • Utility Functions

@fsul7o
Copy link
Contributor

fsul7o commented Oct 1, 2025

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.

@kpango
Copy link
Contributor Author

kpango commented Oct 1, 2025

@fsul7o Regarding the Go version, since my environment is always kept up to date, the upgrade happened automatically when I organized the dependencies.
On the other hand, although Athenz is a security platform, it still relies on an end-of-life (EOL) version of Go—apparently prioritizing the avoidance of backward-compatibility issues over keeping up with Go’s version updates.

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.

@fsul7o
Copy link
Contributor

fsul7o commented Oct 1, 2025

@kpango

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.
(We are concerned that the same issues that occurred during a previous Go version upgrade might happen again.)

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.

@kpango
Copy link
Contributor Author

kpango commented Oct 1, 2025

@fsul7o Thank you for the feedback and for sharing your concerns about potential regressions when upgrading Go.
I fully understand the need for caution, but staying on an end-of-life Go version in a security-critical project like AthenZ authorization-proxy exposes us to unpatched vulnerabilities and unsupported toolchains.
Go 1.25 already contains important security fixes and performance improvements for TLS, crypto, and HTTP/2/3 that 1.23 no longer receives.

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.
If we defer the upgrade again, the eventual jump will only become larger and riskier.
The AthenZ team has historically postponed dependency and Go version upgrades for a long time; therefore, this PR presents a valuable opportunity to break that pattern. Even if review and merge take some extra time, we should use this occasion to complete the Go version upgrade now rather than deferring it again.

Finally, since authorization-proxy follows a semantic versioned release process, merging this PR will not immediately affect end users.
Any remaining concerns about regressions can be thoroughly addressed during the pre-release validation phase before an official release is cut.
This approach enables us to proceed with the upgrade while maintaining safety and compatibility for our users.


Thank you also for allowing me not to split the PR.

@kpango kpango force-pushed the feature/add/access-token-authorization-for-grpc-proxy branch from e443ead to e0c490d Compare October 1, 2025 23:16
@kpango kpango force-pushed the feature/add/access-token-authorization-for-grpc-proxy branch from e0c490d to 028e940 Compare October 1, 2025 23:16
@kpango kpango requested a review from Copilot October 1, 2025 23:24
Copy link

@Copilot Copilot AI left a 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.

@fsul7o
Copy link
Contributor

fsul7o commented Oct 3, 2025

@kpango

I apologize for the delayed response.
Thank you for your various explanations and suggestions.

Our team has also discussed this matter. We would like to proceed as follows:

  1. Our team will complete the go1.25 compatibility work for authorization-proxy.
  2. We will review and merge this PR.

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.

@kpango
Copy link
Contributor Author

kpango commented Oct 3, 2025

@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.

Copy link
Contributor

@fsul7o fsul7o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpango

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>
@kpango kpango force-pushed the feature/add/access-token-authorization-for-grpc-proxy branch from 7a37b45 to 3d15ef5 Compare October 6, 2025 07:02
Signed-off-by: kpango <kpango@vdaas.org>
@kpango kpango force-pushed the feature/add/access-token-authorization-for-grpc-proxy branch from 3d15ef5 to 6ee8395 Compare October 6, 2025 08:12
kpango added 2 commits October 6, 2025 18:05
Signed-off-by: kpango <kpango@vdaas.org>
Signed-off-by: kpango <kpango@vdaas.org>
@kpango kpango requested a review from fsul7o October 6, 2025 09:17
Co-authored-by: fsul7o <75571344+fsul7o@users.noreply.github.com>
Signed-off-by: Yusuke Kato <kpango@vdaas.org>
@kpango
Copy link
Contributor Author

kpango commented Oct 6, 2025

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.

@fsul7o
Copy link
Contributor

fsul7o commented Oct 6, 2025

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.

@fsul7o
Copy link
Contributor

fsul7o commented Oct 7, 2025

@kpango

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:

  1. Go 1.25 Compatibility Release (upgrade go1.25 from go1.23 #58)
    • First, merge upgrade go1.25 from go1.23 #58 into the master branch. This is a minimal release to verify there are no issues arising from the update to Go 1.25.
  2. Release of gRPC Reverse Proxy with Access Token Support (Merge pull request #57 from the fork into the master branch #59)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants