feat(gateway): add gRPC server reflection - #3161
Conversation
87b3006 to
150824f
Compare
letv1nnn
left a comment
There was a problem hiding this comment.
Reviewed against #3058 acceptance criteria — all met: reflection v1 on the primary listener, descriptor set filtered to the public services via a transitive-import closure, callback listeners reject reflection, no OIDC/mTLS user auth (transport TLS still applies), integration test against a running gateway, and grpcurl docs. Filtering by reachability (allowlist, not denylist) is the right call, and reuse of the pre-existing /grpc.reflection. unauthenticated bypass means no new auth hole is opened.
A few things I checked that are fine as-is:
- Rate limiting covers reflection.
GrpcRateLimiteris a global counter wrapping the router above auth, so anonymous reflection requests are already counted — no reflection-specific DoS gap. - Rename guard.
tonic-reflectionbuild does not validate explicitwith_service_nameagainst the descriptor pool, so a renamed root proto could advertise a phantom service. Thereflection_descriptor_excludes_internal_service_protostest asserts the root protos are present, so a rename fails CI rather than shipping silently. Adebug_assert!on the retained set ingateway_reflection_descriptor_setwould make that intent local, but the test already covers it.
One conscious accept to confirm: the full public API surface — including admin/operator RPC names and message shapes — is now discoverable unauthenticated on the primary listener. That is the documented intent of #3058; flagging only so it is a deliberate decision.
LGTM.
politerealism
left a comment
There was a problem hiding this comment.
Minor Concern: The auth exemption prefix is broader than the reflection service actually registered.
auth/oidc.rs:33's UNAUTHENTICATED_PREFIXES already exempts the general prefix "/grpc.reflection." (not version-scoped) from OIDC/mTLS-user auth . This is an entry that predates this PR. This PR wires up a real service behind REFLECTION_PATH_PREFIX = "/grpc.reflection.v1." in multiplex.rs, but doesn't tighten the auth exemption to match it.
The Risk: A future reflection variant (e.g., a v1alpha or v2 service) registered on the router would automatically inherit this blanket auth exemption without requiring changes to auth/oidc.rs, making the security implications invisible in that future diff.
Suggested Fixes:
- Tighten the string directly: Change the UNAUTHENTICATED_PREFIXES entry from "/grpc.reflection." to "/grpc.reflection.v1.". This fixes today's gap, but the two strings remain independently maintained and could drift apart.
- Share a single source of truth (Preferred): Define the reflection prefix once — e.g., pub const REFLECTION_PATH_PREFIX in multiplex.rs — and reference that same constant inside oidc.rs instead of using a hardcoded literal. That way, a future reflection variant must explicitly extend or reuse the shared constant, making its auth exposure fully visible in code review.
(I recommend number 2)
Signed-off-by: Krzysztof Malczuk <kmalczuk@redhat.com>
150824f to
c947f5e
Compare
|
Good catch @politerealism! I went with option 2, the concern should be addressed now. |
Summary
This PR adds functionality to preform reflection calls to get a list of available gRPC endpoints. The reflection call bypasses auth and can be preformed to any gateway without a mTLS or OIDC.
Related Issue
Closes #3058
Changes
Testing
cargo test -p openshell-serverDeployed a local Podman gateway and verified:
grpcurl ... listenumerates the public services.mise run pre-commitpassesUnit tests added/updated
E2E tests added/updated (if applicable)
Checklist