Add support for multiple preshared keys #537

josephschorr · 2022-04-18T22:15:15Z

jzelinskie · 2022-04-18T22:20:30Z

pkg/cmd/server/server.go

@@ -339,7 +352,7 @@ type completedServerConfig struct {

 	unaryMiddleware     []grpc.UnaryServerInterceptor
 	streamingMiddleware []grpc.StreamServerInterceptor
-	presharedKey        string
+	presharedKey        []string


presharedKey => presharedKeys

jzelinskie · 2022-04-18T22:33:43Z

pkg/cmd/server/server.go

@@ -103,7 +103,11 @@ func (c *Config) Complete() (RunnableServer, error) {
 	}

 	if c.GRPCAuthFunc == nil {
-		log.Trace().Int("preshared-key-length", len(c.PresharedKey)).Msg("using gRPC auth with preshared key")
+		log.Trace().Int("preshared-keys", len(c.PresharedKey)).Msg("using gRPC auth with preshared key(s)")


preshared-keys-count

jzelinskie · 2022-04-18T22:41:50Z

pkg/cmd/server/server.go

@@ -141,10 +145,15 @@ func (c *Config) Complete() (RunnableServer, error) {
 			return nil, fmt.Errorf("failed to create dispatcher: %w", cerr)
 		}

+		presharedKey := ""


presharedKey => dispatchKey

used dispatchPresharedKey

jzelinskie · 2022-04-18T22:42:59Z

pkg/cmd/server/server.go

@@ -141,10 +145,15 @@ func (c *Config) Complete() (RunnableServer, error) {
 			return nil, fmt.Errorf("failed to create dispatcher: %w", cerr)
 		}

+		presharedKey := ""
+		if len(c.PresharedKey) > 0 {


Why check this? Shouldn't it error before here if len(c.PresharedKeys) == 0?

Not if no keys were given; remember: preshared keys are ignored if a custom auth function is specified

Oh, gotcha.

I wonder if it makes more sense to hoist this up to where all of the other preshared key logic is.

pkg/cmd/server/server.go

jzelinskie · 2022-04-18T22:47:34Z

cmd/spicedb/serve_integration_test.go

+			require.NoError(err)
+			require.Equal(healthpb.HealthCheckResponse_SERVING, resp.GetStatus())
+
+			client := v1alpha1.NewSchemaServiceClient(conn)


why not use v1?

jzelinskie · 2022-04-18T22:51:47Z

cmd/spicedb/serve_integration_test.go

+			var conn *grpc.ClientConn
+			var err error
+			if key == "" {
+				conn, err = grpc.Dial(fmt.Sprintf("localhost:%s", tester.port), grpc.WithTransportCredentials(insecure.NewCredentials()))
+			} else {
+				conn, err = grpc.Dial(fmt.Sprintf("localhost:%s", tester.port),
+					grpc.WithInsecure(),
+					grpcutil.WithInsecureBearerToken(key),
+				)
+			}


opts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())} if key != "" { opts = append(opts, grpcutil.WithInsecureBearerToken(key)) } conn, err := grpc.Dial(fmt.Sprintf("localhost:%s", tester.port), opts...)

jzelinskie · 2022-04-19T18:11:26Z

.github/workflows/build-test.yaml

+      - uses: "docker/build-push-action@v1"
+        with:
+          push: false
+          tags: "currentpr"


make this dev instead of currentpr

dev has a risk of overloading with a, well, development image. I wanted to be explicitly different from all other contexts

jzelinskie · 2022-04-19T18:11:48Z

.github/workflows/build-test.yaml

@@ -50,7 +50,10 @@ jobs:
      - uses: "actions/setup-go@v3"
        with:
          go-version: "^1.17"
-      - uses: "authzed/actions/docker-build@main"
+      - uses: "docker/build-push-action@v1"


probably need to rename this job

image-build => docker-tests
Build Container Image => Build & Test Container Image

If we do, it'll break the CI submit checks, so I left it as is (unless we should change those too)

It's really easy to change them. I'd rather spend two seconds in the GitHub settings than live with names and descriptions of things that don't make sense.

jzelinskie · 2022-04-19T18:12:29Z

cmd/spicedb/serve_integration_test.go

+	tester, err := newTester(t,
+		&dockertest.RunOptions{
+			Repository:   "authzed/spicedb",
+			Tag:          "currentpr",


cmd/spicedb/serve_integration_test.go

jzelinskie · 2022-04-19T18:15:13Z

cmd/spicedb/serve_integration_test.go

+				opts = append(opts, grpcutil.WithInsecureBearerToken(key))
+			}
+			conn, err := grpc.Dial(fmt.Sprintf("localhost:%s", tester.port), opts...)
+


why the new line here?

jzelinskie · 2022-04-19T18:17:01Z

cmd/spicedb/serve_integration_test.go

+			require.NoError(err)
+			defer conn.Close()
+
+			resp, err := healthpb.NewHealthClient(conn).Check(context.Background(), &healthpb.HealthCheckRequest{Service: "authzed.api.v1alpha1.SchemaService"})


This is against v1alpha1, but then you use v1

Its mostly to make sure the service is running at all, but I can change it

jzelinskie · 2022-04-19T18:20:20Z

cmd/spicedb/serve_integration_test.go

+			} else {
+				s, ok := status.FromError(err)
+				require.True(ok)
+				require.Equal(codes.Unknown, s.Code())


Shouldn't this be codes.Unauthenticated?

Currently, we don't return that code if the key does not match

that seems... like a bug

jzelinskie · 2022-04-19T18:20:29Z

cmd/spicedb/servetesting_integration_test.go

@@ -29,7 +29,7 @@ func TestTestServer(t *testing.T) {
 	tester, err := newTester(t,
 		&dockertest.RunOptions{
 			Repository:   "authzed/spicedb",
-			Tag:          "latest",
+			Tag:          "currentpr",


jzelinskie · 2022-04-19T18:24:17Z

cmd/spicedb/serve_integration_test.go

+			client := v1.NewSchemaServiceClient(conn)
+			_, err = client.WriteSchema(context.Background(), &v1.WriteSchemaRequest{
+				Schema: `
+				definition user {}


Why not use a simpler schema? It's not really relevant to the test

Copy-pasted it; I can make it simpler, but it wasn't a big deal to change either way

jzelinskie · 2022-04-19T18:27:38Z

pkg/cmd/server/server.go

@@ -141,10 +145,15 @@ func (c *Config) Complete() (RunnableServer, error) {
 			return nil, fmt.Errorf("failed to create dispatcher: %w", cerr)
 		}

+		presharedKey := ""
+		if len(c.PresharedKey) > 0 {


Oh, gotcha.

I wonder if it makes more sense to hoist this up to where all of the other preshared key logic is.

Fixes AUTHZ-500

… prevent pulling the normal image accidentally

josephschorr requested a review from jzelinskie April 18, 2022 22:15

josephschorr requested a review from a team as a code owner April 18, 2022 22:15

github-actions bot added area/CLI Affects the command line area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Apr 18, 2022

josephschorr force-pushed the preshared-keys branch from 8586263 to 6448574 Compare April 18, 2022 22:17

jzelinskie requested changes Apr 18, 2022

View reviewed changes

josephschorr force-pushed the preshared-keys branch 3 times, most recently from 9439f23 to caa0998 Compare April 19, 2022 16:41

josephschorr marked this pull request as draft April 19, 2022 16:58

josephschorr force-pushed the preshared-keys branch 3 times, most recently from 6a8a912 to a8bad15 Compare April 19, 2022 17:54

josephschorr marked this pull request as ready for review April 19, 2022 17:55

josephschorr enabled auto-merge April 19, 2022 17:58

jzelinskie requested changes Apr 19, 2022

View reviewed changes

josephschorr force-pushed the preshared-keys branch 3 times, most recently from 0d2d311 to 0652856 Compare April 19, 2022 20:36

jzelinskie approved these changes Apr 20, 2022

View reviewed changes

josephschorr added 3 commits April 20, 2022 10:58

Add support for multiple preshared keys

8a17522

Fixes AUTHZ-500

Change build-test to use a local docker build with a different tag to…

8b65da2

… prevent pulling the normal image accidentally

Update error code for invalid preshared keys to Unauthenticated

8c0659f

jzelinskie force-pushed the preshared-keys branch from 0652856 to 8c0659f Compare April 20, 2022 14:58

josephschorr merged commit afa8057 into authzed:main Apr 20, 2022

github-actions bot locked and limited conversation to collaborators Apr 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple preshared keys #537

Add support for multiple preshared keys #537

josephschorr commented Apr 18, 2022 •

edited by ecordell

jzelinskie Apr 18, 2022

jzelinskie Apr 18, 2022

jzelinskie Apr 18, 2022

josephschorr Apr 19, 2022

jzelinskie Apr 18, 2022

josephschorr Apr 19, 2022

jzelinskie Apr 19, 2022

jzelinskie Apr 18, 2022

josephschorr Apr 19, 2022

jzelinskie Apr 18, 2022

jzelinskie Apr 19, 2022

josephschorr Apr 19, 2022

jzelinskie Apr 19, 2022

josephschorr Apr 19, 2022

jzelinskie Apr 19, 2022

jzelinskie Apr 19, 2022

jzelinskie Apr 19, 2022

jzelinskie Apr 19, 2022

josephschorr Apr 19, 2022

jzelinskie Apr 19, 2022

josephschorr Apr 19, 2022

jzelinskie Apr 19, 2022

jzelinskie Apr 19, 2022

jzelinskie Apr 19, 2022

josephschorr Apr 19, 2022

jzelinskie Apr 19, 2022

Add support for multiple preshared keys #537

Add support for multiple preshared keys #537

Conversation

josephschorr commented Apr 18, 2022 • edited by ecordell

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

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

josephschorr commented Apr 18, 2022 •

edited by ecordell