ING-1339 Finish implementing client cert auth #319
Conversation
Westwooo
force-pushed
the
ING-1339-client-cert-auth-2
branch
6 times, most recently
from
23614a1 to
3788521
Compare
Westwooo
changed the base branch from
master
to
ING-1329-client-cert-testing
Westwooo
changed the base branch from
ING-1329-client-cert-testing
to
master
Westwooo
force-pushed
the
ING-1339-client-cert-auth-2
branch
4 times, most recently
from
f66d45f to
ce69388
Compare
Westwooo
changed the title
chvck
left a comment
chvck
left a comment
There was a problem hiding this comment.
Looks good to me, will leave +2 to Brett.
| p.writeError(w, err, "failed to validate cetificate") | ||
| return | ||
| } else if errors.Is(err, cbauthx.ErrNoCert) { | ||
| p.writeErrorWithStatus(w, err, "authorization header or client cert are required", 401) |
There was a problem hiding this comment.
Based on the ticket about concrete errors, we may want to bring that over to data API as well.
There was a problem hiding this comment.
Though in hindsight, perhaps we might want to rule out that applying to the passthrough APIs since we are not in control of those.
| // We only set the on behalf of and auth headers if there is a user, if | ||
| // we set the onbehalf of header to an empty string and use the admin | ||
| // creds then the server seems to ignore the obo header. | ||
| if oboUser != "" { |
There was a problem hiding this comment.
I think this may run into problems if CNG is authenticating to the cluster with certificate authentication, and then attempts to perform OBO. It may be worth filing a ticket and mentioning this location in it such that when we ultimately add support for CNG itself using Cert Auth, there will be a way to identify this.
There was a problem hiding this comment.
Good point have raised ING-1371 and tagged here with that.
Westwooo
force-pushed
the
ING-1339-client-cert-auth-2
branch
2 times, most recently
from
ab40603 to
491ae68
Compare
There was a problem hiding this comment.
Pull request overview
This PR completes the implementation of client certificate authentication support for non-KV services and the Data API. The changes enable mutual TLS (mTLS) authentication as an alternative to username/password authentication across gRPC services (Query, QueryMgmt) and HTTP endpoints (Data API CRUD operations, proxy endpoints, and tools).
Key changes include:
- Extended client cert auth support to Query, QueryMgmt, and other gRPC services beyond KV
- Added mTLS support to Data API endpoints via TLS connection state handling
- Updated the Data API specification to make the Authorization header optional (supporting cert-based auth)
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| gateway/test/mtls_test.go | Refactored to test multiple gRPC services (KV, Query, QueryMgmt) with table-driven tests and improved permission propagation handling |
| gateway/test/dapi_mtls_test.go | New comprehensive test suite for Data API client cert auth covering Tools, Proxy, and CRUD operations |
| gateway/test/dapi_utils_test.go | Refactored HTTP request helper to support custom HTTP clients for mTLS testing |
| gateway/test/dapi_proxy_test.go | Updated expected status code from Forbidden to Unauthorized for missing auth |
| gateway/test/dapi_crud_test.go | Updated expected status code for missing auth header |
| gateway/system/system.go | Added TLS connection state handler middleware to Data API middleware chain |
| gateway/gateway.go | Configured TLS client cert verification and added username/password to proxy services |
| gateway/dataimpl/server_v1/authhandler.go | Extended gRPC auth handler to support client certificate validation for OBO operations |
| gateway/dapiimpl/tlsconnstate.go | New middleware to extract TLS connection state and add it to request context |
| gateway/dapiimpl/server_v1/errorhandler.go | Added error handlers for invalid certificates and unexpected auth types |
| gateway/dapiimpl/server_v1/authhandler.go | Updated HTTP auth handler to support client certificates alongside username/password |
| gateway/dapiimpl/proxy/proxy.go | Added client cert authentication logic to proxy with fallback to admin credentials |
| gateway/dapiimpl/dapiimpl.go | Passed admin credentials to proxy for on-behalf-of operations |
| gateway/auth/cbauthauthenticator.go | Improved error wrapping for better error chain handling |
| dataapiv1/spec.yaml | Made Authorization header optional to support client cert auth |
| .github/workflows/client_cert_auth_test.yml | Updated test filter pattern to match both gRPC and Data API mTLS tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Westwooo
force-pushed
the
ING-1339-client-cert-auth-2
branch
4 times, most recently
from
d387ad2 to
cdd4591
Compare
Westwooo
force-pushed
the
ING-1339-client-cert-auth-2
branch
from
cdd4591 to
ba35479
Compare
This PR adds support for client cert auth to the non KV services and Data API.