From 9ca4cd4d5bd1e5fec697a509a2b246f8ce9a463c Mon Sep 17 00:00:00 2001 From: bashar-515 Date: Wed, 25 Sep 2024 11:00:03 -0400 Subject: [PATCH] [APP 2911] Step Towards Removal of Legacy Robot API Keys (#4374) --- config/config.go | 6 ++-- config/config_test.go | 17 ++++++---- robot/impl/local_robot_test.go | 8 +++-- robot/web/web.go | 58 +++------------------------------- robot/web/web_test.go | 12 +++---- 5 files changed, 30 insertions(+), 71 deletions(-) diff --git a/config/config.go b/config/config.go index 8a06be48946..c2ba258ae51 100644 --- 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 index 5eb6090ad21..2486d050f49 100644 --- 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 index d284654255a..54f87b7c346 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -318,6 +318,7 @@ func TestConfigRemoteWithAuth(t *testing.T) { options.Managed = tc.Managed options.FQDN = tc.EntityName options.LocalFQDN = primitive.NewObjectID().Hex() + apiKeyID := "sosecretID" apiKey := "sosecret" locationSecret := "locsosecret" @@ -325,7 +326,8 @@ func TestConfigRemoteWithAuth(t *testing.T) { { 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 index 4c5a46d7fe6..65a534cbba6 100644 --- 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 index a6c1ba66ad9..f7bd83700e6 100644 --- 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(), )