credentials, transport, grpc : add a call option to override the :authority header on a per-RPC basis #8068

eshitachandwani · 2025-02-06T06:50:16Z

Fixes: #5361

RELEASE NOTES:

Added a CallAuthority callOption that can be used to overwrite the http :authority header on per-RPC basis.
Introduces an option AuthorityValidator interface which needs to be implemented by credentials that want to allow authority overwrite.
Adds implementations of AuthorityValidator interface for Insecure and TLS credentials.

codecov · 2025-02-06T07:06:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.36%. Comparing base (0003b4f) to head (b1583a6).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8068      +/-   ##
==========================================
+ Coverage   82.32%   82.36%   +0.04%     
==========================================
  Files         387      387              
  Lines       39064    39091      +27     
==========================================
+ Hits        32159    32197      +38     
+ Misses       5593     5584       -9     
+ Partials     1312     1310       -2

Files with missing lines	Coverage Δ
credentials/credentials.go	`87.87% <ø> (ø)`
credentials/insecure/insecure.go	`83.33% <100.00%> (+1.51%)`	⬆️
credentials/tls.go	`88.46% <100.00%> (+1.88%)`	⬆️
internal/transport/http2_client.go	`91.77% <100.00%> (+0.06%)`	⬆️
internal/transport/transport.go	`91.56% <ø> (ø)`
rpc_util.go	`80.97% <100.00%> (+0.29%)`	⬆️
stream.go	`81.54% <100.00%> (-0.24%)`	⬇️

... and 19 files with indirect coverage changes

purnesh42H

Overall lgtm. Just few minor comments. I think we can combine some individual into t-tests.

purnesh42H · 2025-02-07T05:51:05Z

credentials/credentials.go

@@ -120,6 +120,14 @@ type AuthInfo interface {
 	AuthType() string
 }

+// AuthorityValidator defines an interface for validating the authority used to


no need to mention "defines an interface". Can just say "AuthorityValidator validates the authority....."

purnesh42H · 2025-02-07T06:42:08Z

internal/transport/http2_client.go

+	// precedence to determine the :authority header. Any value in Host field of
+	// CallHdr is overwritten. But before overriding, we validate the authority
+	// string against the peer certificates and fail the RPC with `UNAVAILABLE`
+	// status code if eirther of the condition fails.


typo: either

purnesh42H · 2025-02-07T06:44:29Z

internal/transport/http2_client.go

+		if !ok {
+			return nil, &NewStreamError{Err: status.Error(codes.Unavailable, fmt.Sprintf("credentials type %s does not implement the AuthorityValidator interface", t.authInfo.AuthType())), AllowTransparentRetry: false}
+		}
+		err := auth.ValidateAuthority(callHdr.Authority)


can combine this with if

purnesh42H · 2025-02-07T06:49:43Z

rpc_util.go

+//
+// Notice: This API is EXPERIMENTAL and may be changed or removed in a
+// later release.
+func CallAuthority(auth string) CallOption {


should we call it PerRPCAuthority? Similar to PerRPCCredentials?

We went through this in the design doc. We had consensus on CallAuthority.

purnesh42H · 2025-02-07T06:50:47Z

rpc_util.go

+//
+// Notice: This type is EXPERIMENTAL and may be changed or removed in a
+// later release.
+type AuthorityOverrideCallOption struct {


If we name above PerRPCAuthority, AuthorityOverrideCallOption can change to PerRPCAuthorityCallOption

purnesh42H · 2025-02-07T06:55:32Z

credentials/credentials_ext_test.go

+			defer cancel()
+
+			_, err = testgrpc.NewTestServiceClient(cc).EmptyCall(ctx, &testpb.Empty{}, grpc.CallAuthority(tt.expectedAuth))
+			if tt.expectRPCError {


s/expect/want

purnesh42H · 2025-02-07T06:58:48Z

credentials/credentials_ext_test.go

+	}
+}
+
+func (s) TestTLSCredsWithNoAuthorityOverride(t *testing.T) {


the tests tls_ext_test.go should already be testing this right? Does this need to be here? or it should just be another test case in the above TestAuthorityCallOptionsWithTLSCreds

purnesh42H · 2025-02-07T07:03:23Z

credentials/credentials_ext_test.go

+			// Perform a test RPC with a specified call authority.
+			ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
+			defer cancel()
+


nix new line

purnesh42H · 2025-02-07T07:04:08Z

credentials/credentials_ext_test.go

+}
+
+// FakeCredsNoAuthValidator is a test credential that does not implement AuthorityValidator.
+type FakeCredsNoAuthValidator struct {


the FakeCreds can be at the top

Please rename this testCreds. We use the term fake when we use a test double that actually provides a fake implementation of something that the code under test depends on.

Also, the AuthInfo types need to be named such that it is clear from their names that they either implement the AuthorityValidator interface or not.

purnesh42H · 2025-02-07T07:05:52Z

credentials/credentials_ext_test.go

+// TestCallOptionWithNoAuthorityValidator tests the CallAuthority call option
+// with custom credentials that do not implement AuthorityValidator and verifies
+// that it fails with `UNAVAILABLE` status code.
+func (s) TestCallOptionWithNoAuthorityValidator(t *testing.T) {


Same. This can be another test case in test with CustomCreds

purnesh42H · 2025-02-07T07:08:32Z

Also, don't feel strongly but may be we can utilize the setup in tls_ext_test or move the tls tests to tls_ext_test.go

easwars · 2025-02-11T19:37:15Z

credentials/insecure/insecure.go

@@ -71,6 +71,10 @@ func (info) AuthType() string {
 	return "insecure"
 }

+func (info) ValidateAuthority(_ string) error {


Nit: Can get rid of the _ variable name as this method contains only one parameter.

easwars · 2025-02-11T19:39:15Z

credentials/tls.go

+			return nil
+		}
+	}
+	return fmt.Errorf("credentials: failed to verify authority %s", err)


The err variable will only contain the last error here. Should we instead include all errors?

Nit: Also, the error message could be something like
fmt.Errorf(credentials: invalid authority %q: %s", authority, err)

easwars · 2025-02-11T19:39:37Z

credentials/credentials.go

@@ -120,6 +120,13 @@ type AuthInfo interface {
 	AuthType() string
 }

+// AuthorityValidator validates the authority used to override the `:authority`
+// header. A struct implementing AuthInfo should also implement


s/A struct implementing AuthInfo should/Implementations of AuthInfo should/

easwars · 2025-02-11T19:40:04Z

credentials/credentials.go

+// header. A struct implementing AuthInfo should also implement
+// AuthorityValidator if the credentials need to support per-RPC authority overrides.
+type AuthorityValidator interface {
+	ValidateAuthority(authority string) error


Please document what this authority means and what the return value indicates.

easwars · 2025-02-14T00:06:09Z

credentials/credentials.go

+// AuthorityValidator validates the authority used to override the `:authority`
+// header. A struct implementing AuthInfo should also implement
+// AuthorityValidator if the credentials need to support per-RPC authority overrides.
+type AuthorityValidator interface {


Also, I think we should focus on documenting a couple of things here:

that this is an option interface

will be invoked only when the appropriate call option is specified by the application

Something to this effect could work (might need some cleanup)

// AuthorityValidator validates `:authority` header overrides. This is an // optional interface for transport credentials to implement. type AuthorityValidator interface { // ValidateAuthority validates the authority used to override the // `:authority` header. If implemented by the AuthInfo type returned by the // transport credentials, this method will be invoked if the client // application attempts to override the `:authority` header by using the // CallAuthority call option. ValidateAuthority(authority string) error }

easwars · 2025-02-14T00:08:05Z

credentials/tls.go

+			return nil
+		}
+	}
+	return fmt.Errorf("credentials: failed to verify authority %s", err)


Nit: Also, the error message could be something like
fmt.Errorf(credentials: invalid authority %q: %s", authority, err)

easwars · 2025-02-14T00:09:15Z

rpc_util.go

+//
+// Notice: This API is EXPERIMENTAL and may be changed or removed in a
+// later release.
+func CallAuthority(auth string) CallOption {


We went through this in the design doc. We had consensus on CallAuthority.

easwars · 2025-02-14T00:09:59Z

rpc_util.go

+//
+// Notice: This API is EXPERIMENTAL and may be changed or removed in a
+// later release.
+func CallAuthority(auth string) CallOption {


Nit: s/auth/authority/

Please note that this is part of our public API. Parameter names also serve as documentation. So using good parameter names is critical.

easwars · 2025-02-14T00:10:55Z

rpc_util.go

@@ -365,6 +366,36 @@ func (o MaxRecvMsgSizeCallOption) before(c *callInfo) error {
 }
 func (o MaxRecvMsgSizeCallOption) after(*callInfo, *csAttempt) {}

+// CallAuthority returns a CallOption which sets the authority to override the
+// `:authority` pseudoheader in a RPC. If this is set, the RPC will use only


This is the only place where we use the term pseudoheader. Either use it everywhere (like in the dosctrings of the AuthorityValidator interface), or don't use it here. Thanks.

How about something more simpler and direct

"CallAuthority creates a CallOption that sets the HTTP/2 :authority header of an RPC to the specified value."

easwars · 2025-02-14T00:45:43Z

credentials/credentials_ext_test.go

+}
+
+// FakeCredsNoAuthValidator is a test credential that does not implement AuthorityValidator.
+type FakeCredsNoAuthValidator struct {


Please rename this testCreds. We use the term fake when we use a test double that actually provides a fake implementation of something that the code under test depends on.

Also, the AuthInfo types need to be named such that it is clear from their names that they either implement the AuthorityValidator interface or not.

easwars · 2025-02-14T00:46:49Z

credentials/credentials_ext_test.go

+// TestAuthInfo implements the AuthInfo interface.
+type TestAuthInfo struct{}
+
+// AuthType returns the authentication type.
+func (TestAuthInfo) AuthType() string { return "test" }


Nit: The definition of this type is sandwiched between the implementation of the test creds type. Please move it to be separate.

easwars · 2025-02-14T00:46:58Z

credentials/credentials_ext_test.go

+}
+
+// TestAuthInfo implements the AuthInfo interface.
+type TestAuthInfo struct{}


Does this have to be exported?

easwars · 2025-02-14T00:49:46Z

credentials/credentials_ext_test.go

+
+// ServerHandshake performs the server-side handshake.
+// Returns a test AuthInfo object to satisfy the interface requirements.
+func (c *FakeCredsWithAuthValidator) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) {


Basically, we can have a single testCreds type that contains a boolean field that indicates whether or not it should return an AuthInfo that implements AuthorityValidator or not. We don't need two test credentials types here.

easwars · 2025-02-14T00:50:21Z

credentials/credentials_ext_test.go

+		name         string
+		creds        credentials.TransportCredentials
+		expectedAuth string
+		wantRPCError bool


Same here. You can directly use the status that you want the RPC to return.

eshitachandwani and others added 4 commits January 24, 2025 10:52

regenrate protos

6840baf

revert

11d96c1

authority overwrite

d5b5af1

Merge branch 'grpc:master' into auth_override

34175aa

eshitachandwani added Type: Feature New features or improvements in behavior Area: Auth Includes regular credentials API and implementation. Also includes advancedtls, authz, rbac etc. labels Feb 6, 2025

eshitachandwani added this to the 1.71 Release milestone Feb 6, 2025

eshitachandwani added 2 commits February 6, 2025 12:25

update callInfo

4791ac7

update callInfo

ac318de

eshitachandwani requested a review from purnesh42H February 6, 2025 07:09

eshitachandwani assigned purnesh42H Feb 6, 2025

purnesh42H reviewed Feb 7, 2025

View reviewed changes

purnesh42H assigned eshitachandwani and unassigned purnesh42H Feb 7, 2025

eshitachandwani added 2 commits February 10, 2025 14:34

efn

d1ea438

address comments

72d132a

eshitachandwani requested a review from easwars February 11, 2025 07:19

eshitachandwani assigned easwars Feb 11, 2025

Merge branch 'grpc:master' into auth_override

b1583a6

easwars reviewed Feb 13, 2025

View reviewed changes

easwars reviewed Feb 14, 2025

View reviewed changes

easwars removed their assignment Feb 14, 2025

arjan-bal modified the milestones: 1.71 Release, 1.72 Release Feb 19, 2025

credentials, transport, grpc : add a call option to override the :authority header on a per-RPC basis #8068

Are you sure you want to change the base?

credentials, transport, grpc : add a call option to override the :authority header on a per-RPC basis #8068

Conversation

eshitachandwani commented Feb 6, 2025

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

purnesh42H left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2025 •

edited

Loading