Authentication, Authorization, HTTPS support #773

lpabon · 2018-12-04T20:21:24Z

What this PR does / why we need it:
OpenStorage SDK will be the method of accessing any internal driver or cluster APIs. This PR provides a foundation for auth work.

Which issue(s) this PR fixes (optional)
Part of #448

Special notes for your reviewer:
It also has the following enhancements:

HTTPS support for gRPC
- HTTPS is now used in all unit tests
Unix Domain Socket for the SDK
Interceptors
- Token based authentication and validation
- Audit
- Authorization
- Logging

ggriffiths

Mostly looks good, just a few minor suggestions and questions

ggriffiths · 2018-12-04T21:54:14Z

api/server/sdk/server.go

+	}
+
+	// Create REST Gateway and connect it to the unix domain socket server
+	restGeteway, err := newSdkRestGateway(config, udsServer)


typo: restGeteway -> restGateway

ggriffiths · 2018-12-04T22:01:01Z

api/server/sdk/rest_gateway.go

+)
+
+type sdkRestGateway struct {
+	config     ServerConfig


Any reason for not making this a pointer? Just to ensure we make a copy of the config?

ggriffiths · 2018-12-04T22:06:05Z

api/server/sdk/rest_gateway.go

+				err.Error())
+		}
+	}()
+	<-ready


Is the purpose of this just to block until we know the goroutine has started? I suppose you could use a sync.WaitGroup to be a little more clear, but that may be overkill for just one goroutine.

correct 👍

Also, this code is copied from the older server.go

ggriffiths · 2018-12-04T22:14:26Z

api/server/sdk/server_interceptors.go

+type InterceptorContextkey string
+
+const (
+	InterceptorContextTokenKey InterceptorContextkey = "tokenclaims"


Maybe a comment here about how InterceptorContextkey and/or InterceptorContextTokenKey are for those unfamiliar with gRPC interceptors or our design

Good point.

ggriffiths · 2018-12-04T22:19:05Z

api/server/sdk/server_interceptors.go

+	}
+
+	// Setup auditor log
+	claimsJSON, _ := json.Marshal(claims)


Any reason for ignoring this error?

Ah, i shouldn't just in case

ggriffiths · 2018-12-04T22:22:38Z

api/server/sdk/server_interceptors.go

+
+func authorizeClaims(rules []sdk_auth.Rule, fullmethod string) error {
+
+	var (


Can just do a single line var reqService, reqApi string here

ggriffiths · 2018-12-04T22:29:01Z

cmd/osd/main.go

@@ -337,12 +339,18 @@ func start(c *cli.Context) error {
 		csiServer.Start()

 		// Start SDK Server for this driver
+		sdksocket := fmt.Sprintf("/var/lib/osd/driver/%s-sdk.sock", d)
+		os.Remove(sdksocket)


Should we check for an error from os.Remove? I'm not sure under what circumstances this would fail, but maybe to be safe

no need, imho, if it fails, we will not be able to create it and the starting of the service will show the correct error.

ggriffiths · 2018-12-04T22:34:19Z

pkg/auth/auth.go

+	requiredClaims = []string{"exp", "iat", "name", "email"}
+)
+
+type Authenticator interface {


Not sure if we follow golint too closely, but these exported functions & could use comments

Good point. I will add some

ggriffiths · 2018-12-04T22:35:58Z

api/server/sdk/server_interceptors.go

+			info.FullMethod)
+	}
+
+	logger.Info("Authorized")


Will this include the request timestamp in the logs? From the design doc:

An SDK interceptor will log all successful and denied accesses. Information will include:

Time of request

All latest claims information

API call requested

Success/Denied status

yes, the logrus will take care of that.

IMO these should be logged into a separate audit file or else it is going to flood the storage provider logs with request information.

I will add an issue to have this done. In the mean time I will disable this from the interceptors.

ggriffiths · 2018-12-04T23:00:55Z

pkg/auth/auth.go

+func (j *JwtAuthenticator) AuthenticateToken(rawtoken string) (*sdk_auth.Claims, error) {
+
+	// Parse token
+	token, err := jwt.Parse(rawtoken, func(token *jwt.Token) (interface{}, error) {


Is this where we check if the token has expired? If so, is token.Valid set to false?

If not, where do we check for token expiry? w/ token.exp

Yeah, that's it. the jwt package sets that boolean value and the caller is supposed to check it.

ggriffiths · 2018-12-05T17:54:57Z

lgtm 👍

adityadani · 2018-12-05T18:58:10Z

api/server/sdk/rest_gateway.go

+
+	// REST Gateway Handlers
+	handlers := []func(context.Context, *runtime.ServeMux, *grpc.ClientConn) (err error){
+		api.RegisterOpenStorageClusterHandler,


How about this list of functions is a global variable. So that we can add tests, to ensure if anyone adds a new handler it gets added to this list.

This is a good idea. Do you mind if we add this as a PR after this?

yes that works.

Issue #776 created

adityadani · 2018-12-05T19:07:39Z

api/server/sdk/server.go

+	}
+
+	// Create REST Gateway and connect it to the unix domain socket server
+	restGateway, err := newSdkRestGateway(config, udsServer)


we should error out if config.RestPort (required by rest gateway) is not provided.

adityadani · 2018-12-05T19:07:56Z

api/server/sdk/server.go

+		return err
+	}
+
+	if len(s.config.RestPort) != 0 {


if we add this check in New(), it will not be needed here.

adityadani · 2018-12-05T19:10:15Z

api/server/sdk/rest_gateway.go

+	return nil
+
+}
+


We should also add a Stop routine for the REST gateway.

That's much harder to do. It has to do with http server in golang. If it is supports it in later golang versions, I will add it.

This is how we can do it

Suggested change

restGatewayServer := &http.Server{Addr: address, Handler: mux}

restGatewayServer.ListenAndServeTLS(s.config.Tls.CertFile, s.config.Tls.KeyFile)

// for stopping

restGatewayServer.Close()

adityadani · 2018-12-05T19:10:37Z

api/server/sdk/server.go

+
+func (s *Server) Stop() {
+	s.netServer.Stop()
+	s.udsServer.Stop()


Stop the REST gateway as well.

adityadani · 2018-12-05T19:42:39Z

api/server/sdk/server_interceptors.go

+
+// Authenticate user and add authorization information back in the context
+func (s *sdkGrpcServer) auth(ctx context.Context) (context.Context, error) {
+	token, err := grpc_auth.AuthFromMD(ctx, "bearer")


nit: bearer could be a constant, with some comment explaining what it is and why we are not using basic

Will do. I will add that it is just standard JWT style

adityadani · 2018-12-05T19:48:33Z

api/server/sdk/server_interceptors.go

+			info.FullMethod)
+	}
+
+	logger.Info("Authorized")


IMO these should be logged into a separate audit file or else it is going to flood the storage provider logs with request information.

adityadani · 2018-12-05T19:54:08Z

pkg/auth/auth.go

+func New(config *JwtAuthConfig) (*JwtAuthenticator, error) {
+
+	// Check at least one is set
+	if len(config.SharedSecret) == 0 &&


nit: add a check for config is not nil

adityadani · 2018-12-05T19:57:36Z

pkg/auth/auth.go

+)
+
+var (
+	requiredClaims = []string{"exp", "iat", "name", "email"}


nit: doc would be helpful here.

adityadani · 2018-12-05T19:58:16Z

api/server/sdk/server_interceptors.go

+}
+
+// Authenticate user and add authorization information back in the context
+func (s *sdkGrpcServer) auth(ctx context.Context) (context.Context, error) {


When does this auth(ctx) function get invoked?

by the interceptor which is setup during registration and setup of the gRPC server.

Signed-off-by: Luis Pabón <luis@portworx.com>

Previously the REST Gateway would directly connect to the gRPC port. Now with HTTPS/TLS support, this is more complicated since the CERTS would have a CN not labeled 'localhost'. For simplicity the SDK now also has a unix domain socket. This UDS is used for local access and for the REST Gateway to communicate with the gRPC server. All models support token authentication and authorization based on JWTs. All SDK unit tests run with HTTPS enabled. Signed-off-by: Luis Pabón <luis@portworx.com>

adityadani

lgtm

lpabon · 2018-12-07T16:08:24Z

🎉 😄

ggriffiths suggested changes Dec 4, 2018

View reviewed changes

lpabon force-pushed the sdk-auth-4 branch from f194747 to 9331c2a Compare December 5, 2018 05:40

ggriffiths approved these changes Dec 5, 2018

View reviewed changes

adityadani reviewed Dec 5, 2018

View reviewed changes

lpabon mentioned this pull request Dec 5, 2018

Make list of rest gateway functions global to add for tests #776

Open

lpabon force-pushed the sdk-auth-4 branch 6 times, most recently from f13f445 to b48bc38 Compare December 6, 2018 16:40

lpabon added 2 commits December 6, 2018 13:47

Vendor additions for Auth/HTTPS support

b841eef

Signed-off-by: Luis Pabón <luis@portworx.com>

lpabon force-pushed the sdk-auth-4 branch from b48bc38 to b121d99 Compare December 6, 2018 18:47

adityadani approved these changes Dec 7, 2018

View reviewed changes

lpabon merged commit 7ad3786 into libopenstorage:master Dec 7, 2018

lpabon deleted the sdk-auth-4 branch December 7, 2018 16:08


		func authorizeClaims(rules []sdk_auth.Rule, fullmethod string) error {

		var (

+ restGatewayServer := &http.Server{Addr: address, Handler: mux}
+ restGatewayServer.ListenAndServeTLS(s.config.Tls.CertFile, s.config.Tls.KeyFile)
+ // for stopping
+ restGatewayServer.Close()

Authentication, Authorization, HTTPS support #773

Authentication, Authorization, HTTPS support #773

Conversation

lpabon commented Dec 4, 2018 • edited Loading

ggriffiths 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

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

ggriffiths Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggriffiths commented Dec 5, 2018

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

adityadani Dec 5, 2018 • edited Loading

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

lpabon Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

adityadani left a comment

Choose a reason for hiding this comment

lpabon commented Dec 7, 2018

lpabon commented Dec 4, 2018 •

edited

Loading

ggriffiths Dec 4, 2018 •

edited

Loading

adityadani Dec 5, 2018 •

edited

Loading

lpabon Dec 5, 2018 •

edited

Loading