From 856a551e02c4e1645ef78e0a4c7ebfdd270a9b52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Mon, 3 Jan 2022 21:51:13 +0100 Subject: [PATCH 1/3] chore: improves auth plugin resolution. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently when aiming to use a Plugin in credentials section, if the plugin is known then it will be resolved, if it isn't, it will be passed to the supported credentials and tried to be cast as HTTPAuthPlugin which ends up in a casting issue without further feedback on what was the plugin string. Signed-off-by: José Carlos Chávez --- plugins/rest/rest.go | 12 +++++++++++- plugins/rest/rest_test.go | 10 ++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/plugins/rest/rest.go b/plugins/rest/rest.go index 385601c9af..c0778ba9fe 100644 --- a/plugins/rest/rest.go +++ b/plugins/rest/rest.go @@ -67,19 +67,29 @@ func (c *Config) authPlugin(authPluginLookup func(string) HTTPAuthPlugin) (HTTPA var candidate HTTPAuthPlugin if c.Credentials.Plugin != nil && authPluginLookup != nil { candidate := authPluginLookup(*c.Credentials.Plugin) - if candidate != nil { + if candidate == nil { + c.logger.WithFields(map[string]interface{}{ + "plugin": *c.Credentials.Plugin, + }).Warn("Could not resolve candidate auth plugin") + } else { return candidate, nil } } // reflection avoids need for this code to change as auth plugins are added s := reflect.ValueOf(c.Credentials) for i := 0; i < s.NumField(); i++ { + if s.Field(i).Type().String() == "*string" { + continue + } + if s.Field(i).IsNil() { continue } + if candidate != nil { return nil, errors.New("a maximum one credential method must be specified") } + candidate = s.Field(i).Interface().(HTTPAuthPlugin) } if candidate == nil { diff --git a/plugins/rest/rest_test.go b/plugins/rest/rest_test.go index 002e9f08bc..2d5010a2fd 100644 --- a/plugins/rest/rest_test.go +++ b/plugins/rest/rest_test.go @@ -620,6 +620,16 @@ func TestNew(t *testing.T) { "url": "http://localhost", "credentials": { "plugin": "my_plugin" + } + }`, + }, + { + name: "Unknown plugin", + input: `{ + "name": "foo", + "url": "http://localhost", + "credentials": { + "plugin": "my_other_plugin" } }`, }, From ba55904ecf3a45e156721e6f9676b2b46c4f4bc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Mon, 3 Jan 2022 21:54:32 +0100 Subject: [PATCH 2/3] docs: improves explanation. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Carlos Chávez --- plugins/rest/rest.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugins/rest/rest.go b/plugins/rest/rest.go index c0778ba9fe..0228414202 100644 --- a/plugins/rest/rest.go +++ b/plugins/rest/rest.go @@ -79,6 +79,9 @@ func (c *Config) authPlugin(authPluginLookup func(string) HTTPAuthPlugin) (HTTPA s := reflect.ValueOf(c.Credentials) for i := 0; i < s.NumField(); i++ { if s.Field(i).Type().String() == "*string" { + // if the field is `Plugin` we should have resolved that above, if we didn't + // we won't do it here, hence we avoid this check (which will fail anyways in casting) + // and let the rest of the code below handle it continue } @@ -92,6 +95,7 @@ func (c *Config) authPlugin(authPluginLookup func(string) HTTPAuthPlugin) (HTTPA candidate = s.Field(i).Interface().(HTTPAuthPlugin) } + if candidate == nil { return &defaultAuthPlugin{}, nil } From 80619c11df70b28ab0d241b796bc48ff209a975c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Wed, 5 Jan 2022 11:43:06 +0100 Subject: [PATCH 3/3] chore: returns error when either credentials can't be resolved or resolver is nil. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Carlos Chávez --- plugins/rest/rest.go | 23 ++++++++++------------- plugins/rest/rest_test.go | 28 ++++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/plugins/rest/rest.go b/plugins/rest/rest.go index 0228414202..11fd3e2977 100644 --- a/plugins/rest/rest.go +++ b/plugins/rest/rest.go @@ -10,6 +10,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "reflect" @@ -65,26 +66,22 @@ func (c *Config) Equal(other *Config) bool { func (c *Config) authPlugin(authPluginLookup func(string) HTTPAuthPlugin) (HTTPAuthPlugin, error) { var candidate HTTPAuthPlugin - if c.Credentials.Plugin != nil && authPluginLookup != nil { + if c.Credentials.Plugin != nil { + if authPluginLookup == nil { + // if no authPluginLookup function is passed we can't resolve the plugin + return nil, errors.New("missing auth plugin lookup function") + } + candidate := authPluginLookup(*c.Credentials.Plugin) if candidate == nil { - c.logger.WithFields(map[string]interface{}{ - "plugin": *c.Credentials.Plugin, - }).Warn("Could not resolve candidate auth plugin") - } else { - return candidate, nil + return nil, fmt.Errorf("auth plugin %q not found", *c.Credentials.Plugin) } + + return candidate, nil } // reflection avoids need for this code to change as auth plugins are added s := reflect.ValueOf(c.Credentials) for i := 0; i < s.NumField(); i++ { - if s.Field(i).Type().String() == "*string" { - // if the field is `Plugin` we should have resolved that above, if we didn't - // we won't do it here, hence we avoid this check (which will fail anyways in casting) - // and let the rest of the code below handle it - continue - } - if s.Field(i).IsNil() { continue } diff --git a/plugins/rest/rest_test.go b/plugins/rest/rest_test.go index 2d5010a2fd..37592fecca 100644 --- a/plugins/rest/rest_test.go +++ b/plugins/rest/rest_test.go @@ -41,8 +41,31 @@ import ( const keyID = "key1" -func TestNew(t *testing.T) { +func TestAuthPluginWithNoAuthPluginLookup(t *testing.T) { + authPlugin := "anything" + cfg := Config{ + Credentials: struct { + Bearer *bearerAuthPlugin `json:"bearer,omitempty"` + OAuth2 *oauth2ClientCredentialsAuthPlugin `json:"oauth2,omitempty"` + ClientTLS *clientTLSAuthPlugin `json:"client_tls,omitempty"` + S3Signing *awsSigningAuthPlugin `json:"s3_signing,omitempty"` + GCPMetadata *gcpMetadataAuthPlugin `json:"gcp_metadata,omitempty"` + AzureManagedIdentity *azureManagedIdentitiesAuthPlugin `json:"azure_managed_identity,omitempty"` + Plugin *string `json:"plugin,omitempty"` + }{ + Plugin: &authPlugin, + }, + } + _, err := cfg.authPlugin(nil) + if err == nil { + t.Error("Expected error but got nil") + } + if want, have := "missing auth plugin lookup function", err.Error(); want != have { + t.Errorf("Unexpected error, want %q, have %q", want, have) + } +} +func TestNew(t *testing.T) { tests := []struct { name string input string @@ -629,9 +652,10 @@ func TestNew(t *testing.T) { "name": "foo", "url": "http://localhost", "credentials": { - "plugin": "my_other_plugin" + "plugin": "unknown_plugin" } }`, + wantErr: true, }, }