From 94ae2415a3cfb88816d83fc3b4e0a3af66d74050 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 19 Sep 2024 11:49:32 -0400 Subject: [PATCH 1/7] make config changes --- config/config.go | 6 +++--- config/config_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 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..bbda607d66b 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -429,7 +429,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"}, @@ -648,7 +648,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"}, From e15bb559e4c937370f5235f418e692b1f067a855 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 19 Sep 2024 11:54:01 -0400 Subject: [PATCH 2/7] make web changes --- robot/web/web.go | 21 +++++---------------- robot/web/web_test.go | 12 +++++------- 2 files changed, 10 insertions(+), 23 deletions(-) diff --git a/robot/web/web.go b/robot/web/web.go index d778b77ff34..311a7947ea4 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 { 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(), ) From de0b3fc88aafc84ef8e780b2603cb262d89176f0 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 19 Sep 2024 11:56:00 -0400 Subject: [PATCH 3/7] remove uncalled functions --- robot/web/web.go | 37 ------------------------------------- 1 file changed, 37 deletions(-) diff --git a/robot/web/web.go b/robot/web/web.go index 311a7947ea4..b93eadad7d8 100644 --- a/robot/web/web.go +++ b/robot/web/web.go @@ -778,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 { @@ -812,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 From d24ac120cb0f813f29df6fa96167fb4f98893dd4 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 19 Sep 2024 13:15:07 -0400 Subject: [PATCH 4/7] refactor some tests to match auth config sent by app --- config/config_test.go | 10 +++++++--- robot/impl/local_robot_test.go | 4 +++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index bbda607d66b..428f97d497e 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"}, }, } @@ -928,8 +929,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 47be7d5d8b7..77aea53a66a 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{apiKey}, }, }, { From aa7266f51cfec8ab29f863523ffd29c3eb8005ac Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 19 Sep 2024 13:54:46 -0400 Subject: [PATCH 5/7] reformat test auth config for test --- config/config_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config/config_test.go b/config/config_test.go index 428f97d497e..2486d050f49 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -610,7 +610,8 @@ func TestConfigEnsurePartialStart(t *testing.T) { validAPIKeyHandler := config.AuthHandlerConfig{ Type: rpc.CredentialsTypeAPIKey, Config: rutils.AttributeMap{ - "key": "foo", + "key": "foo", + "keys": []string{"key"}, }, } From 8c8660a6d3d0900f77ea2798d3df375e1a31e0b2 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 19 Sep 2024 14:22:29 -0400 Subject: [PATCH 6/7] change entityName --- robot/impl/local_robot_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 77aea53a66a..6aca71cedd8 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -327,7 +327,7 @@ func TestConfigRemoteWithAuth(t *testing.T) { Type: rpc.CredentialsTypeAPIKey, Config: rutils.AttributeMap{ apiKeyID: apiKey, - "keys": []string{apiKey}, + "keys": []string{apiKeyID}, }, }, { @@ -393,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) From dbbd40f3982931b8a7e1dc3692ddc62e20d9de17 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 20 Sep 2024 17:31:39 -0400 Subject: [PATCH 7/7] change entity for managed and unmanaged cases --- robot/impl/local_robot_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 6aca71cedd8..030397f2d3a 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -410,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)