[APP 2911] Step Towards Removal of Legacy Robot API Keys (#4374)

viamrobotics · Sep 25, 2024 · 9ca4cd4 · 9ca4cd4
1 parent 77cbddd
commit 9ca4cd4
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 71 deletions.
diff --git a/config/config.go b/config/config.go
@@ -899,7 +899,7 @@ type AuthHandlerConfig struct {
 //					}
 //				}
 //			],
-//		"external_auth_config": {}
+//		    "external_auth_config": {}
 //	}
 func (config *AuthConfig) Validate(path string) error {
 	seenTypes := make(map[string]struct{}, len(config.Handlers))
@@ -928,8 +928,8 @@ func (config *AuthHandlerConfig) Validate(path string) error {
 	}
 	switch config.Type {
 	case rpc.CredentialsTypeAPIKey:
-		if config.Config.String("key") == "" && len(config.Config.StringSlice("keys")) == 0 {
-			return resource.NewConfigValidationError(fmt.Sprintf("%s.config", path), errors.New("key or keys is required"))
+		if len(config.Config.StringSlice("keys")) == 0 {
+			return resource.NewConfigValidationError(fmt.Sprintf("%s.config", path), errors.New("keys is required"))
 		}
 	case rpc.CredentialsTypeExternal:
 		return errors.New("robot cannot issue external auth tokens")

diff --git a/config/config_test.go b/config/config_test.go
@@ -390,7 +390,8 @@ func TestConfigEnsure(t *testing.T) {
 	validAPIKeyHandler := config.AuthHandlerConfig{
 		Type: rpc.CredentialsTypeAPIKey,
 		Config: rutils.AttributeMap{
-			"key": "foo",
+			"key":  "foo",
+			"keys": []string{"key"},
 		},
 	}
 
@@ -429,7 +430,7 @@ func TestConfigEnsure(t *testing.T) {
 	test.That(t, err, test.ShouldNotBeNil)
 	test.That(t, err.Error(), test.ShouldContainSubstring, `auth.handlers.0`)
 	test.That(t, err.Error(), test.ShouldContainSubstring, `required`)
-	test.That(t, err.Error(), test.ShouldContainSubstring, `key`)
+	test.That(t, err.Error(), test.ShouldContainSubstring, `keys`)
 
 	validAPIKeyHandler.Config = rutils.AttributeMap{
 		"keys": []string{"one", "two"},
@@ -609,7 +610,8 @@ func TestConfigEnsurePartialStart(t *testing.T) {
 	validAPIKeyHandler := config.AuthHandlerConfig{
 		Type: rpc.CredentialsTypeAPIKey,
 		Config: rutils.AttributeMap{
-			"key": "foo",
+			"key":  "foo",
+			"keys": []string{"key"},
 		},
 	}
 
@@ -648,7 +650,7 @@ func TestConfigEnsurePartialStart(t *testing.T) {
 	test.That(t, err, test.ShouldNotBeNil)
 	test.That(t, err.Error(), test.ShouldContainSubstring, `auth.handlers.0`)
 	test.That(t, err.Error(), test.ShouldContainSubstring, `required`)
-	test.That(t, err.Error(), test.ShouldContainSubstring, `key`)
+	test.That(t, err.Error(), test.ShouldContainSubstring, `keys`)
 
 	validAPIKeyHandler.Config = rutils.AttributeMap{
 		"keys": []string{"one", "two"},
@@ -928,8 +930,11 @@ func TestAuthConfigEnsure(t *testing.T) {
 			Auth: config.AuthConfig{
 				Handlers: []config.AuthHandlerConfig{
 					{
-						Type:   rpc.CredentialsTypeAPIKey,
-						Config: rutils.AttributeMap{"key": "abc123"},
+						Type: rpc.CredentialsTypeAPIKey,
+						Config: rutils.AttributeMap{
+							"abc123": "abc123",
+							"keys":   []string{"abc123"},
+						},
 					},
 				},
 			},

diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go
@@ -318,14 +318,16 @@ func TestConfigRemoteWithAuth(t *testing.T) {
 			options.Managed = tc.Managed
 			options.FQDN = tc.EntityName
 			options.LocalFQDN = primitive.NewObjectID().Hex()
+			apiKeyID := "sosecretID"
 			apiKey := "sosecret"
 			locationSecret := "locsosecret"
 
 			options.Auth.Handlers = []config.AuthHandlerConfig{
 				{
 					Type: rpc.CredentialsTypeAPIKey,
 					Config: rutils.AttributeMap{
-						"key": apiKey,
+						apiKeyID: apiKey,
+						"keys":   []string{apiKeyID},
 					},
 				},
 				{
@@ -391,7 +393,7 @@ func TestConfigRemoteWithAuth(t *testing.T) {
 				test.That(t, ok, test.ShouldBeFalse)
 				test.That(t, remoteBot, test.ShouldBeNil)
 
-				remoteConfig.Remotes[0].Auth.Entity = entityName
+				remoteConfig.Remotes[0].Auth.Entity = apiKeyID
 				remoteConfig.Remotes[1].Auth.Entity = entityName
 				test.That(t, setupLocalRobot(t, context.Background(), remoteConfig, logger).Close(context.Background()), test.ShouldBeNil)
 
@@ -408,6 +410,8 @@ func TestConfigRemoteWithAuth(t *testing.T) {
 
 				test.That(t, setupLocalRobot(t, context.Background(), remoteConfig, logger).Close(context.Background()), test.ShouldBeNil)
 
+				remoteConfig.Remotes[0].Auth.Entity = apiKeyID
+
 				ctx2 := context.Background()
 				remoteConfig.Remotes[0].Address = options.LocalFQDN
 				r2 = setupLocalRobot(t, ctx2, remoteConfig, logger)

diff --git a/robot/web/web.go b/robot/web/web.go
@@ -742,23 +742,12 @@ func (svc *webService) initAuthHandlers(listenerTCPAddr *net.TCPAddr, options we
 			switch handler.Type {
 			case rpc.CredentialsTypeAPIKey:
 				apiKeys := parseAPIKeys(handler)
-				legacyAPIKeys := parseLegacyAPIKeys(handler, apiKeys)
-				hasAPIKeys := len(apiKeys) != 0
-				hasLegacyAPIKeys := len(legacyAPIKeys) != 0
-
-				switch {
-				case !hasLegacyAPIKeys && !hasAPIKeys:
-					return nil, errors.Errorf("%q handler requires non-empty API key or keys", handler.Type)
-				case hasLegacyAPIKeys && !hasAPIKeys:
-					rpcOpts = append(rpcOpts, rpc.WithAuthHandler(
-						handler.Type,
-						rpc.MakeSimpleMultiAuthHandler(authEntities, legacyAPIKeys),
-					))
-				case !hasLegacyAPIKeys && hasAPIKeys:
-					rpcOpts = append(rpcOpts, rpc.WithAuthHandler(handler.Type, rpc.MakeSimpleMultiAuthPairHandler(apiKeys)))
-				default:
-					rpcOpts = append(rpcOpts, rpc.WithAuthHandler(handler.Type, makeMultiStepAPIKeyAuthHandler(authEntities, legacyAPIKeys, apiKeys)))
+
+				if len(apiKeys) == 0 {
+					return nil, errors.Errorf("%q handler requires non-empty API keys", handler.Type)
 				}
+
+				rpcOpts = append(rpcOpts, rpc.WithAuthHandler(handler.Type, rpc.MakeSimpleMultiAuthPairHandler(apiKeys)))
 			case rutils.CredentialsTypeRobotLocationSecret:
 				locationSecrets := handler.Config.StringSlice("secrets")
 				if len(locationSecrets) == 0 {
@@ -789,28 +778,6 @@ func (svc *webService) initAuthHandlers(listenerTCPAddr *net.TCPAddr, options we
 	return rpcOpts, nil
 }
 
-func parseLegacyAPIKeys(handler config.AuthHandlerConfig, nonLegacyAPIKeys map[string]string) []string {
-	apiKeys := handler.Config.StringSlice("keys")
-	var filteredAPIKeys []string
-
-	// filter out new api keys from keys array to ensure we're left with only legacy keys
-	for _, apiKey := range apiKeys {
-		if _, ok := nonLegacyAPIKeys[apiKey]; !ok {
-			filteredAPIKeys = append(filteredAPIKeys, apiKey)
-		}
-	}
-
-	if len(filteredAPIKeys) == 0 {
-		apiKey := handler.Config.String("key")
-		if apiKey == "" {
-			return []string{}
-		}
-		filteredAPIKeys = []string{apiKey}
-	}
-
-	return filteredAPIKeys
-}
-
 func parseAPIKeys(handler config.AuthHandlerConfig) map[string]string {
 	apiKeys := map[string]string{}
 	for k := range handler.Config {
@@ -823,21 +790,6 @@ func parseAPIKeys(handler config.AuthHandlerConfig) map[string]string {
 	return apiKeys
 }
 
-// makeMultiStepAPIKeyAuthHandler supports auth handlers for both legacy and non-legacy api keys for backwards compatibility.
-func makeMultiStepAPIKeyAuthHandler(legacyEntities, legacyExpectedAPIKeys []string, apiKeys map[string]string) rpc.AuthHandler {
-	legacyAuthHandler := rpc.MakeSimpleMultiAuthHandler(legacyEntities, legacyExpectedAPIKeys)
-	currentAuthHandler := rpc.MakeSimpleMultiAuthPairHandler(apiKeys)
-	return rpc.AuthHandlerFunc(func(ctx context.Context, entity, payload string) (map[string]string, error) {
-		result, err := legacyAuthHandler.Authenticate(ctx, entity, payload)
-		if err == nil {
-			return result, nil
-		}
-
-		// if legacy API key authentication fails, try a new API key authentication
-		return currentAuthHandler.Authenticate(ctx, entity, payload)
-	})
-}
-
 // Register every API resource grpc service here.
 func (svc *webService) initAPIResourceCollections(ctx context.Context, mod bool) error {
 	// TODO (RSDK-144): only register necessary services

diff --git a/robot/web/web_test.go b/robot/web/web_test.go
@@ -202,7 +202,6 @@ func TestWebWithAuth(t *testing.T) {
 			options.Managed = tc.Managed
 			options.FQDN = tc.EntityName
 			options.LocalFQDN = primitive.NewObjectID().Hex()
-			legacyAPIKey := "sosecret"
 			apiKeyID1 := uuid.New().String()
 			apiKey1 := utils.RandomAlphaString(32)
 			apiKeyID2 := uuid.New().String()
@@ -212,7 +211,6 @@ func TestWebWithAuth(t *testing.T) {
 				{
 					Type: rpc.CredentialsTypeAPIKey,
 					Config: rutils.AttributeMap{
-						"key":     legacyAPIKey,
 						apiKeyID1: apiKey1,
 						apiKeyID2: apiKey2,
 						"keys":    []string{apiKeyID1, apiKeyID2},
@@ -244,7 +242,7 @@ func TestWebWithAuth(t *testing.T) {
 				_, err = rgrpc.Dial(context.Background(), addr, logger, rpc.WithAllowInsecureWithCredentialsDowngrade(),
 					rutils.WithEntityCredentials("wrong", rpc.Credentials{
 						Type:    rpc.CredentialsTypeAPIKey,
-						Payload: legacyAPIKey,
+						Payload: apiKey1,
 					}))
 				test.That(t, err, test.ShouldNotBeNil)
 				test.That(t, err.Error(), test.ShouldContainSubstring, "invalid credentials")
@@ -268,9 +266,9 @@ func TestWebWithAuth(t *testing.T) {
 				// WebRTC connections across unix sockets can create deadlock in CI.
 				conn, err := rgrpc.Dial(context.Background(), addr, logger,
 					rpc.WithAllowInsecureWithCredentialsDowngrade(),
-					rpc.WithEntityCredentials(entityName, rpc.Credentials{
+					rpc.WithEntityCredentials(apiKeyID1, rpc.Credentials{
 						Type:    rpc.CredentialsTypeAPIKey,
-						Payload: legacyAPIKey,
+						Payload: apiKey1,
 					}),
 					rpc.WithForceDirectGRPC(),
 				)
@@ -407,9 +405,9 @@ func TestWebWithAuth(t *testing.T) {
 				// WebRTC connections across unix sockets can create deadlock in CI.
 				conn, err := rgrpc.Dial(context.Background(), addr, logger,
 					rpc.WithAllowInsecureWithCredentialsDowngrade(),
-					rpc.WithCredentials(rpc.Credentials{
+					rpc.WithEntityCredentials(apiKeyID1, rpc.Credentials{
 						Type:    rpc.CredentialsTypeAPIKey,
-						Payload: legacyAPIKey,
+						Payload: apiKey1,
 					}),
 					rpc.WithForceDirectGRPC(),
 				)