Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94ae241
make config changes
bashar-515 Sep 19, 2024
e15bb55
make web changes
bashar-515 Sep 19, 2024
de0b3fc
remove uncalled functions
bashar-515 Sep 19, 2024
d24ac12
refactor some tests to match auth config sent by app
bashar-515 Sep 19, 2024
aa7266f
reformat test auth config for test
bashar-515 Sep 19, 2024
8c8660a
change entityName
bashar-515 Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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")
Expand Down
17 changes: 11 additions & 6 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
},
}

Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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"},
},
}

Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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"},
},
},
},
},
Expand Down
6 changes: 4 additions & 2 deletions robot/impl/local_robot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
},
},
{
Expand Down Expand Up @@ -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)

Expand Down
58 changes: 5 additions & 53 deletions robot/web/web.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down
12 changes: 5 additions & 7 deletions robot/web/web_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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},
Expand Down Expand Up @@ -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")
Expand All @@ -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(),
)
Expand Down Expand Up @@ -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(),
)
Expand Down
Loading