Skip to content

feat(gateway): add gRPC server reflection - #3161

Open
2000krysztof wants to merge 1 commit into
NVIDIA:mainfrom
2000krysztof:feat/gRpcReflection
Open

feat(gateway): add gRPC server reflection#3161
2000krysztof wants to merge 1 commit into
NVIDIA:mainfrom
2000krysztof:feat/gRpcReflection

Conversation

@2000krysztof

@2000krysztof 2000krysztof commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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

  • Added tonic-reflection 0.14 and registered gRPC reflection v1 on the primary gateway listener.
  • Extended the custom gRPC router to dispatch reflection requests.
  • Exposed only the public OpenShell and Inference schemas.
  • Excluded internal driver, interceptor, credential, and middleware descriptors.
  • Kept reflection exempt from OIDC and mTLS user authentication while preserving transport-level TLS requirements.
  • Preserved reflection rejection on callback-only compute-driver listeners.
  • Added coverage for routing, service enumeration, descriptor filtering, authentication bypass, and the complete primary-listener path.
  • Documented grpcurl discovery, authentication, and listener behavior in the README and gateway documentation.

Testing

  • cargo test -p openshell-server

    • 1,461 passed; 7 ignored
  • Deployed a local Podman gateway and verified:

    • grpcurl ... list enumerates the public services.
    • The health RPC succeeds through reflection.
    • Internal service symbols cannot be resolved.
  • mise run pre-commit passes

  • Unit tests added/updated

  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@copy-pr-bot

copy-pr-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@letv1nnn letv1nnn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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. GrpcRateLimiter is a global counter wrapping the router above auth, so anonymous reflection requests are already counted — no reflection-specific DoS gap.
  • Rename guard. tonic-reflection build does not validate explicit with_service_name against the descriptor pool, so a renamed root proto could advertise a phantom service. The reflection_descriptor_excludes_internal_service_protos test asserts the root protos are present, so a rename fails CI rather than shipping silently. A debug_assert! on the retained set in gateway_reflection_descriptor_set would 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 politerealism left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. 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>
@2000krysztof

Copy link
Copy Markdown
Contributor Author

Good catch @politerealism! I went with option 2, the concern should be addressed now.
Thanks for the feedback :)

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.

feat(server): register standard gRPC reflection on the primary listener

3 participants