xds: server-side security: server integration #4092

easwars · 2020-12-09T01:18:05Z

Summary of changes:

Add security integration to xds.GRPCServer:
- Retrieve configured transport credentials from the underlying grpc.Server
- Handle security configuration in received Listener resource
  - create appropriate certificate providers
  - setup HandshakeInfo to be passed to xds.Credentials
  - wrap the underlying net.Conn
    - support for setting and getting deadline
    - support for retrieving the HandshakeInfo
e2e test which exercises xDS security with a file watcher plugin.
A way to retrieve the transport credentials configured on a grpc.Server through the internal package.

easwars · 2020-12-14T17:25:05Z

@dfawley : This is now ready to be looked at. Thanks.

dfawley · 2020-12-15T16:42:49Z

internal/internal.go

+	// GetServerCredentials returns the transport credentials configured on a
+	// gRPC server. An xDS-enabled server needs to know what type of credentials
+	// is configured on the underlying gRPC server. This is set by server.go.
+	GetServerCredentials interface{} // func (s interface{}) *credentials.TransportCredentials


Why not func (s *grpc.Server) ...?

Also it seems the convention is mostly to not give names to the parameters, just list their types.

We cannot have this package depend on the grpc package, right. That would lead to a cyclic dependency.

Removed the parameter names.

This was referring to the comment. The type would still be interface{}

internal/internal.go

xds/server.go

dfawley · 2020-12-15T17:18:08Z

xds/server.go

+		// config and makes sure that it is acceptable to the plugin. Still, it
+		// is possible that the plugin parses the config successfully, but its
+		// Build() method errors out.
+		return nil, fmt.Errorf("xds: failed to get security plugin instance (%+v): %v", cfg, err)


Is this xds going to be redundant?

The convention seems to be that we add the component prefix to errors that we generate. The same or closely related prefix might also get added by the prefix logger if the returned error is logged at some point. I still haven't looked at any real logs to see how useful/bad these prefixes are getting.

This isn't a convention we should have, unless the error is leaving the xds component. Let's be careful about these so our logs are clear and not cluttered.

Can I leave it as-is for now? We should probably discuss this and come up with a convention that we can follow.

I don't understand the pushback. If we're producing an error solely to log it later, it's equivalent to directly writing:

logger.Warning("Error doing whatever: xds: couldn't do that thing")

...which would result in the output:

[xds] [xds-server 0xf34a89d] Error doing whatever: xds: couldn't do that thing

...which has "xds" in it 3 times!

I've removed the xds prefix from the errors returned from here.

I didn't mean to pushback on the change you are suggesting here. I'm only saying that this is widespread and that we should talk about it and decide how we want to go about structuring error messages.

I'm all in favor of discussing and fixing the other areas! But I think we should not make things worse in the meantime.

xds/server.go

easwars

Thanks for the review. PTAL.

easwars · 2020-12-15T17:41:47Z

internal/internal.go

+	// GetServerCredentials returns the transport credentials configured on a
+	// gRPC server. An xDS-enabled server needs to know what type of credentials
+	// is configured on the underlying gRPC server. This is set by server.go.
+	GetServerCredentials interface{} // func (s interface{}) *credentials.TransportCredentials


We cannot have this package depend on the grpc package, right. That would lead to a cyclic dependency.

Removed the parameter names.

internal/internal.go

xds/server.go

easwars · 2020-12-15T18:01:25Z

xds/server.go

+		// config and makes sure that it is acceptable to the plugin. Still, it
+		// is possible that the plugin parses the config successfully, but its
+		// Build() method errors out.
+		return nil, fmt.Errorf("xds: failed to get security plugin instance (%+v): %v", cfg, err)


The convention seems to be that we add the component prefix to errors that we generate. The same or closely related prefix might also get added by the prefix logger if the returned error is logged at some point. I still haven't looked at any real logs to see how useful/bad these prefixes are getting.

xds/server.go

dfawley · 2020-12-15T23:59:26Z

internal/internal.go

+	// GetServerCredentials returns the transport credentials configured on a
+	// gRPC server. An xDS-enabled server needs to know what type of credentials
+	// is configured on the underlying gRPC server. This is set by server.go.
+	GetServerCredentials interface{} // func (s interface{}) *credentials.TransportCredentials


This was referring to the comment. The type would still be interface{}

xds/server.go

dfawley · 2020-12-16T00:08:05Z

xds/server.go

+		// config and makes sure that it is acceptable to the plugin. Still, it
+		// is possible that the plugin parses the config successfully, but its
+		// Build() method errors out.
+		return nil, fmt.Errorf("xds: failed to get security plugin instance (%+v): %v", cfg, err)


This isn't a convention we should have, unless the error is leaving the xds component. Let's be careful about these so our logs are clear and not cluttered.

xds/server.go

xds/internal/test/xds_server_integration_test.go

easwars · 2020-12-16T00:48:52Z

Had to rebase because of a merge conflict and the comments are now all over the place :(

easwars · 2020-12-16T00:52:44Z

For comments where I'm not able to reply inline:

Added the bc == nil check back
As for polluting the logs with our prefixes: can I leave it as-is for now. We should discuss this and come to a consensus on what would be the best strategy forward. It might require some wholesale changes
As for the type in the comment in the internal package: server.go which implements internal.GetServerCredentials accepts an interface{} and not a *grpc.Server. It then goes ahead and type asserts to a *grpc.Server. I did this so that tests which fake out the underlying grpc.Server will continue to work without extensive changes. Let me know if you still want me to change the type in the comment.

Thanks.

dfawley · 2020-12-16T00:53:41Z

internal/internal.go

@@ -64,7 +64,7 @@ var (
 	// GetServerCredentials returns the transport credentials configured on a
 	// gRPC server. An xDS-enabled server needs to know what type of credentials
 	// is configured on the underlying gRPC server. This is set by server.go.
-	GetServerCredentials interface{} // func (interface{}) credentials.TransportCredentials


We're still type asserting this to interface{} so now the comment doesn't match reality.

Fixed it now.

dfawley · 2020-12-16T00:56:34Z

xds/server.go

+		// config and makes sure that it is acceptable to the plugin. Still, it
+		// is possible that the plugin parses the config successfully, but its
+		// Build() method errors out.
+		return nil, fmt.Errorf("xds: failed to get security plugin instance (%+v): %v", cfg, err)


I don't understand the pushback. If we're producing an error solely to log it later, it's equivalent to directly writing:

logger.Warning("Error doing whatever: xds: couldn't do that thing")

...which would result in the output:

[xds] [xds-server 0xf34a89d] Error doing whatever: xds: couldn't do that thing

...which has "xds" in it 3 times!

dfawley · 2020-12-16T01:06:18Z

As for the type in the comment in the internal package: server.go which implements internal.GetServerCredentials accepts an interface{} and not a *grpc.Server. It then goes ahead and type asserts to a *grpc.Server. I did this so that tests which fake out the underlying grpc.Server will continue to work without extensive changes. Let me know if you still want me to change the type in the comment.

Which tests do this?

easwars

As for the type in the comment in the internal package: server.go which implements internal.GetServerCredentials accepts an interface{} and not a *grpc.Server. It then goes ahead and type asserts to a *grpc.Server. I did this so that tests which fake out the underlying grpc.Server will continue to work without extensive changes. Let me know if you still want me to change the type in the comment.

Which tests do this?

Tests in xds/server_test.go use a fakeGRPCServer to make sure that things like Serve(), Stop() etc on the xds.GRPCServer ends up calling the appropriate methods on the grpc.Server.

easwars · 2020-12-16T01:20:13Z

xds/server.go

+		// config and makes sure that it is acceptable to the plugin. Still, it
+		// is possible that the plugin parses the config successfully, but its
+		// Build() method errors out.
+		return nil, fmt.Errorf("xds: failed to get security plugin instance (%+v): %v", cfg, err)


I've removed the xds prefix from the errors returned from here.

I didn't mean to pushback on the change you are suggesting here. I'm only saying that this is widespread and that we should talk about it and decide how we want to go about structuring error messages.

dfawley

Tests in xds/server_test.go use a fakeGRPCServer to make sure that things like Serve(), Stop() etc on the xds.GRPCServer ends up calling the appropriate methods on the grpc.Server.

Okay, I get what's happening now, thanks.

How about if we do the type assertion in the xds code instead, since that's where the interface is used instead of a direct *grpc.Server? It seems like a minor change (it is), but I think it's a little cleaner if the grpc package and its (semi-)exported functions don't have to do type assertions like this.

Also, conceptually, it's more appropriate to determine what to do if the server isn't a *grpc.Server closer to the code that knows what else it might be. E.g., the fake could support this feature as well, but doing the type assertion in grpc makes that harder.

dfawley · 2020-12-16T16:47:51Z

xds/server.go

+		// config and makes sure that it is acceptable to the plugin. Still, it
+		// is possible that the plugin parses the config successfully, but its
+		// Build() method errors out.
+		return nil, fmt.Errorf("xds: failed to get security plugin instance (%+v): %v", cfg, err)


I'm all in favor of discussing and fixing the other areas! But I think we should not make things worse in the meantime.

easwars · 2020-12-16T17:49:45Z

Tests in xds/server_test.go use a fakeGRPCServer to make sure that things like Serve(), Stop() etc on the xds.GRPCServer ends up calling the appropriate methods on the grpc.Server.

Okay, I get what's happening now, thanks.

How about if we do the type assertion in the xds code instead, since that's where the interface is used instead of a direct *grpc.Server? It seems like a minor change (it is), but I think it's a little cleaner if the grpc package and its (semi-)exported functions don't have to do type assertions like this.

Also, conceptually, it's more appropriate to determine what to do if the server isn't a *grpc.Server closer to the code that knows what else it might be. E.g., the fake could support this feature as well, but doing the type assertion in grpc makes that harder.

Makes sense. Moved the type assertion to the xds code. Thanks. It does look cleaner now.

dfawley · 2020-12-16T18:11:34Z

xds/server.go

+	// provides the implementation for internal.GetServerCredentials, and allows
+	// us to use a fake gRPC server in tests.


For future consideration: moving this to a function would allow it to be stubbed for testing:

var usesXDS = func(s *GRPCServer) bool { ... }

in tests:

usesXDS = func(s *GRPCServer) bool { return s.gs.(*fakeGRPCServer).whatever }

easwars added no release notes Type: Feature New features or improvements in behavior labels Dec 9, 2020

easwars mentioned this pull request Dec 9, 2020

xds: server-side security: server creds implementation #4089

Merged

easwars force-pushed the xds_server_creds branch from d245a49 to 8a35d0e Compare December 14, 2020 17:14

easwars requested a review from dfawley December 14, 2020 17:22

easwars assigned dfawley Dec 14, 2020

dfawley requested changes Dec 15, 2020

View reviewed changes

easwars commented Dec 15, 2020

View reviewed changes

easwars assigned dfawley and unassigned dfawley Dec 15, 2020

dfawley requested changes Dec 16, 2020

View reviewed changes

dfawley assigned easwars and unassigned dfawley Dec 16, 2020

easwars added 3 commits December 15, 2020 16:46

xds: Implement server-side security

c23173d

review comments

ff6516e

review comments

7c03bb3

easwars force-pushed the xds_server_creds branch from 8cd3fd4 to 7c03bb3 Compare December 16, 2020 00:46

merge conflicts

9aa7abd

revert to interface{} in comment in internal.go

6bfb47b

dfawley reviewed Dec 16, 2020

View reviewed changes

easwars commented Dec 16, 2020

View reviewed changes

remove xds prefix from error strings

3d36074

dfawley reviewed Dec 16, 2020

View reviewed changes

move type assertion to xds code

dae639a

easwars removed their assignment Dec 16, 2020

easwars assigned dfawley Dec 16, 2020

dfawley approved these changes Dec 16, 2020

View reviewed changes

easwars merged commit 2fad6bf into grpc:master Dec 16, 2020

easwars deleted the xds_server_creds branch December 16, 2020 18:27

easwars changed the title ~~xds: Implement server-side security~~ xds: server-side security: server integration Mar 4, 2021

github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: server-side security: server integration #4092

xds: server-side security: server integration #4092

easwars commented Dec 9, 2020 •

edited

Loading

easwars commented Dec 14, 2020

dfawley Dec 15, 2020

easwars Dec 15, 2020

dfawley Dec 15, 2020

dfawley Dec 15, 2020

easwars Dec 15, 2020

dfawley Dec 16, 2020

easwars Dec 16, 2020

dfawley Dec 16, 2020

easwars Dec 16, 2020

dfawley Dec 16, 2020

easwars left a comment

easwars Dec 15, 2020

easwars Dec 15, 2020

dfawley Dec 15, 2020

dfawley Dec 16, 2020

easwars commented Dec 16, 2020 •

edited

Loading

easwars commented Dec 16, 2020

dfawley Dec 16, 2020

easwars Dec 16, 2020

dfawley Dec 16, 2020

dfawley commented Dec 16, 2020

easwars left a comment

easwars Dec 16, 2020

dfawley left a comment

dfawley Dec 16, 2020

easwars commented Dec 16, 2020

dfawley Dec 16, 2020

easwars Dec 16, 2020

		// provides the implementation for internal.GetServerCredentials, and allows
		// us to use a fake gRPC server in tests.

xds: server-side security: server integration #4092

xds: server-side security: server integration #4092

Conversation

easwars commented Dec 9, 2020 • edited Loading

easwars commented Dec 14, 2020

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

easwars 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

easwars commented Dec 16, 2020 • edited Loading

easwars commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley commented Dec 16, 2020

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Dec 9, 2020 •

edited

Loading

easwars commented Dec 16, 2020 •

edited

Loading