grpc: optional interface to provide channel authority #6752

Aditya-Sood · 2023-10-27T16:18:23Z

Also implements the same for unix resolver's builder

RELEASE NOTES:

resolver: provide method for resolver to implement to override default channel authority (EXPERIMENTAL).

Also implements the same for unix resolver's builder

codecov · 2023-10-27T16:21:32Z

Codecov Report

Merging #6752 (1dd2231) into master (b046cca) will increase coverage by 0.12%.
Report is 39 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 1dd2231 differs from pull request most recent head fb65458. Consider uploading reports for the commit fb65458 to get more accurate results

Additional details and impacted files

Aditya-Sood · 2023-10-27T16:44:24Z

hi @dfawley, I have a couple queries:

I changed the name of the interface method to GetServiceAuthority() to keep the same name as the java method - is that fine?
The java method also places a couple more requirements on the implementation:

It must be from a trusted source, because if the authority is tampered with, RPCs may be sent to the attackers which may leak sensitive user data.
An implementation must generate it without blocking, typically in line, and must keep it unchanged. NameResolvers created from the same factory with the same argument must return the same authority.

Should we mention the same in our comments?

Regarding your comment about "escapeAuthority", I checked the java codebase and it seems to expect the percent-encoding from the name resolver like you mentioned (link to file):

assertThat(resolver.getServiceAuthority()).isEqualTo("path%2Fto%2Fservice");

I guess we could add a line in the comments for GetServiceAuthority() asking users to do the encoding as part of their concrete method (for the sake of conforming to existing implementations), and add a check at the time of assigning the value to cc.authority for our verification?

dfawley

Thank you for the PR! A few comments inline...

dfawley · 2023-11-06T22:27:33Z

clientconn.go

-		// return channel authority.
-		cc.authority = "localhost"
+	case isAuthOverrider:
+		cc.authority = auth.GetServiceAuthority(cc.parsedTarget)
 	case strings.HasPrefix(endpoint, ":"):
 		cc.authority = "localhost" + endpoint
 	default:


Please delete the TODO below.

dfawley · 2023-11-06T22:27:41Z

clientconn.go

@@ -1981,16 +1981,14 @@ func (cc *ClientConn) determineAuthority() error {
 	}

 	endpoint := cc.parsedTarget.Endpoint()
-	target := cc.target
+	auth, isAuthOverrider := any(cc.resolverBuilder).(resolver.AuthorityOverrider)


Why cast it to any first? That shouldn't be needed.

my bad, cc.resolverBuilder is already an interface type
thanks

dfawley · 2023-11-06T22:34:34Z

resolver/resolver.go

+type AuthorityOverrider interface {
+	// GetServiceAuthority returns the authority to use for a ClientConn with the
+	// given target. It must not perform I/O or any other blocking operations.
+	GetServiceAuthority(t Target) string


Go style generally doesn't like GetXYZ. OverrideAuthority makes more sense given the name of the interface; otherwise AuthorityForTarget or something like that could be okay.

dfawley · 2023-11-06T22:35:22Z

resolver/resolver.go

+type AuthorityOverrider interface {
+	// GetServiceAuthority returns the authority to use for a ClientConn with the
+	// given target. It must not perform I/O or any other blocking operations.
+	GetServiceAuthority(t Target) string


Please name the parameter target to be explicit, or don't name it at all, as t doesn't really add documentation value here, and it's not necessary to name.

dfawley · 2023-11-06T22:36:36Z

clientconn.go

@@ -1981,16 +1981,14 @@ func (cc *ClientConn) determineAuthority() error {
 	}

 	endpoint := cc.parsedTarget.Endpoint()
-	target := cc.target
+	auth, isAuthOverrider := any(cc.resolverBuilder).(resolver.AuthorityOverrider)
 	switch {


Optional: maybe just convert this into a few ifs and do the auth overrider as a compound if.

Aditya-Sood · 2023-11-08T06:04:12Z

@dfawley thank you for the review, I have pushed the review changes to the PR
could you also weigh-in on points (2) and (3) in #6752 (comment) and let me know if any further work is required?

github-actions · 2023-11-14T06:45:47Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

dfawley · 2023-11-14T16:55:34Z

clientconn.go

-	case isAuthOverrider:
-		cc.authority = auth.GetServiceAuthority(cc.parsedTarget)
-	case strings.HasPrefix(endpoint, ":"):
+	} else if auth, isAuthOverrider := cc.resolverBuilder.(resolver.AuthorityOverrider); isAuthOverrider {


Go style: please name this ok instead of isAuthOverrider.

dfawley · 2023-11-14T17:10:31Z

internal/resolver/unix/unix.go

@@ -61,6 +61,10 @@ func (b *builder) Scheme() string {
 	return b.scheme
 }

+func (b *builder) OverrideAuthority(t resolver.Target) string {


Since t is unused, you can omit it entirely here.

dfawley · 2023-11-14T17:10:56Z

+@arvindbr8 for a second pass

github-actions · 2023-11-22T04:45:50Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

Aditya-Sood · 2023-11-27T02:47:00Z

hi @arvindbr8, could you please review the PR and remove the reporter clarification status?

resolver/resolver.go

clientconn.go

easwars · 2023-12-05T20:10:22Z

@Aditya-Sood : Thank you for your contribution!

Increment: Adds AuthorityOverrider interface

eab9b33

Also implements the same for unix resolver's builder

Fix typo in comments

5cc6bab

ginayeh requested a review from dfawley October 31, 2023 16:20

ginayeh added the Hacktoberfest label Oct 31, 2023

ginayeh assigned dfawley Oct 31, 2023

ginayeh added P2 Type: Feature New features or improvements in behavior and removed P2 Hacktoberfest labels Oct 31, 2023

ginayeh added this to the 1.60 Release milestone Oct 31, 2023

dfawley reviewed Nov 6, 2023

View reviewed changes

dfawley assigned Aditya-Sood Nov 6, 2023

dfawley added the Status: Requires Reporter Clarification label Nov 6, 2023

Increment: Code review changes

fa15301

github-actions bot added the stale label Nov 14, 2023

dfawley removed Status: Requires Reporter Clarification stale labels Nov 14, 2023

dfawley approved these changes Nov 14, 2023

View reviewed changes

dfawley reviewed Nov 14, 2023

View reviewed changes

dfawley assigned arvindbr8 and unassigned Aditya-Sood Nov 14, 2023

dfawley assigned Aditya-Sood and unassigned dfawley Nov 14, 2023

dfawley added the Status: Requires Reporter Clarification label Nov 14, 2023

arvindbr8 removed this from the 1.60 Release milestone Nov 14, 2023

arvindbr8 added this to the 1.61 Release milestone Nov 14, 2023

Code review refactors

b6617ec

github-actions bot added the stale label Nov 22, 2023

github-actions bot removed the stale label Nov 27, 2023

dfawley assigned ginayeh and unassigned Aditya-Sood and arvindbr8 Nov 27, 2023

dfawley removed the Status: Requires Reporter Clarification label Nov 27, 2023

Aditya-Sood mentioned this pull request Nov 28, 2023

grpc: add optional interface to return channel authority #5360

Closed

7 tasks

dfawley assigned easwars and unassigned ginayeh Nov 28, 2023

easwars reviewed Nov 29, 2023

View reviewed changes

resolver/resolver.go Outdated Show resolved Hide resolved

resolver/resolver.go Outdated Show resolved Hide resolved

clientconn.go Outdated Show resolved Hide resolved

easwars assigned dfawley and Aditya-Sood Nov 29, 2023

easwars added the Status: Requires Reporter Clarification label Nov 30, 2023

Code review changes

1dd2231

dfawley removed their assignment Dec 4, 2023

Increment: Modifies description of OverrideAuthority()

fb65458

easwars removed the Status: Requires Reporter Clarification label Dec 5, 2023

easwars approved these changes Dec 5, 2023

View reviewed changes

easwars merged commit 0866ce0 into grpc:master Dec 5, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc: optional interface to provide channel authority #6752

grpc: optional interface to provide channel authority #6752

Aditya-Sood commented Oct 27, 2023 •

edited by dfawley

codecov bot commented Oct 27, 2023 •

edited

Aditya-Sood commented Oct 27, 2023

dfawley left a comment

dfawley Nov 6, 2023

dfawley Nov 6, 2023

Aditya-Sood Nov 7, 2023

dfawley Nov 6, 2023

dfawley Nov 6, 2023

dfawley Nov 6, 2023

Aditya-Sood commented Nov 8, 2023

github-actions bot commented Nov 14, 2023

dfawley Nov 14, 2023

dfawley Nov 14, 2023

dfawley commented Nov 14, 2023

github-actions bot commented Nov 22, 2023

Aditya-Sood commented Nov 27, 2023

easwars commented Dec 5, 2023

grpc: optional interface to provide channel authority #6752

grpc: optional interface to provide channel authority #6752

Conversation

Aditya-Sood commented Oct 27, 2023 • edited by dfawley

codecov bot commented Oct 27, 2023 • edited

Codecov Report

Aditya-Sood commented Oct 27, 2023

dfawley 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

Aditya-Sood commented Nov 8, 2023

github-actions bot commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley commented Nov 14, 2023

github-actions bot commented Nov 22, 2023

Aditya-Sood commented Nov 27, 2023

easwars commented Dec 5, 2023

Aditya-Sood commented Oct 27, 2023 •

edited by dfawley

codecov bot commented Oct 27, 2023 •

edited