From af0126e545390d4b4fd0db2472173a931213cc0c Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 25 Mar 2020 16:31:19 -0700 Subject: [PATCH 1/4] surface errs from responses --- builtin/logical/rabbitmq/path_role_create.go | 108 ++++++++++++++----- 1 file changed, 79 insertions(+), 29 deletions(-) diff --git a/builtin/logical/rabbitmq/path_role_create.go b/builtin/logical/rabbitmq/path_role_create.go index 71b211f14b804..15357bd7814ec 100644 --- a/builtin/logical/rabbitmq/path_role_create.go +++ b/builtin/logical/rabbitmq/path_role_create.go @@ -3,10 +3,9 @@ package rabbitmq import ( "context" "fmt" + "io/ioutil" - "github.com/hashicorp/errwrap" - multierror "github.com/hashicorp/go-multierror" - uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" rabbithole "github.com/michaelklishin/rabbit-hole" @@ -69,27 +68,63 @@ func (b *backend) pathCredsRead(ctx context.Context, req *logical.Request, d *fr } // Register the generated credentials in the backend, with the RabbitMQ server - if _, err = client.PutUser(username, rabbithole.UserSettings{ + resp, err := client.PutUser(username, rabbithole.UserSettings{ Password: password, Tags: role.Tags, - }); err != nil { + }) + if err != nil { return nil, fmt.Errorf("failed to create a new user with the generated credentials") } + defer func() { + if err := resp.Body.Close(); err != nil { + b.Logger().Error(fmt.Sprintf("unable to close response body: %s", err)) + } + }() + if !isIn200s(resp.StatusCode) { + body, _ := ioutil.ReadAll(resp.Body) + return nil, fmt.Errorf("error putting user %s - %d: %s", username, resp.StatusCode, body) + } + + success := false + defer func() { + if success { + return + } + // Delete the user because it's in an unknown state. + resp, err := client.DeleteUser(username) + if err != nil { + b.Logger().Error(fmt.Sprintf("failed to delete %s: %s", username, err)) + } + if !isIn200s(resp.StatusCode) { + body, _ := ioutil.ReadAll(resp.Body) + b.Logger().Error(fmt.Sprintf("error deleting %s - %d: %s", username, resp.StatusCode, body)) + } + }() // If the role had vhost permissions specified, assign those permissions // to the created username for respective vhosts. for vhost, permission := range role.VHosts { - if _, err := client.UpdatePermissionsIn(vhost, username, rabbithole.Permissions{ - Configure: permission.Configure, - Write: permission.Write, - Read: permission.Read, - }); err != nil { - outerErr := errwrap.Wrapf(fmt.Sprintf("failed to update permissions to the %q user: {{err}}", username), err) - // Delete the user because it's in an unknown state - if _, rmErr := client.DeleteUser(username); rmErr != nil { - return nil, multierror.Append(errwrap.Wrapf("failed to delete user: {{err}}", rmErr), outerErr) + if err := func() error { + resp, err := client.UpdatePermissionsIn(vhost, username, rabbithole.Permissions{ + Configure: permission.Configure, + Write: permission.Write, + Read: permission.Read, + }) + if err != nil { + return err + } + defer func() { + if err := resp.Body.Close(); err != nil { + b.Logger().Error(fmt.Sprintf("unable to close response body: %s", err)) + } + }() + if !isIn200s(resp.StatusCode) { + body, _ := ioutil.ReadAll(resp.Body) + return fmt.Errorf("error updating vhost permissions for %s - %d: %s", vhost, resp.StatusCode, body) } - return nil, outerErr + return nil + }(); err != nil { + return nil, err } } @@ -97,23 +132,34 @@ func (b *backend) pathCredsRead(ctx context.Context, req *logical.Request, d *fr // to the created username for respective vhosts and exchange. for vhost, permissions := range role.VHostTopics { for exchange, permission := range permissions { - if _, err := client.UpdateTopicPermissionsIn(vhost, username, rabbithole.TopicPermissions{ - Exchange: exchange, - Write: permission.Write, - Read: permission.Read, - }); err != nil { - outerErr := errwrap.Wrapf(fmt.Sprintf("failed to update topic permissions to the %q user: {{err}}", username), err) - // Delete the user because it's in an unknown state - if _, rmErr := client.DeleteUser(username); rmErr != nil { - return nil, multierror.Append(errwrap.Wrapf("failed to delete user: {{err}}", rmErr), outerErr) + if err := func() error { + resp, err := client.UpdateTopicPermissionsIn(vhost, username, rabbithole.TopicPermissions{ + Exchange: exchange, + Write: permission.Write, + Read: permission.Read, + }) + if err != nil { + return err } - return nil, outerErr + defer func() { + if err := resp.Body.Close(); err != nil { + b.Logger().Error(fmt.Sprintf("unable to close response body: %s", err)) + } + }() + if !isIn200s(resp.StatusCode) { + body, _ := ioutil.ReadAll(resp.Body) + return fmt.Errorf("error updating vhost permissions for %s - %d: %s", vhost, resp.StatusCode, body) + } + return nil + }(); err != nil { + return nil, err } } } + success = true // Return the secret - resp := b.Secret(SecretCredsType).Response(map[string]interface{}{ + response := b.Secret(SecretCredsType).Response(map[string]interface{}{ "username": username, "password": password, }, map[string]interface{}{ @@ -127,11 +173,15 @@ func (b *backend) pathCredsRead(ctx context.Context, req *logical.Request, d *fr } if lease != nil { - resp.Secret.TTL = lease.TTL - resp.Secret.MaxTTL = lease.MaxTTL + response.Secret.TTL = lease.TTL + response.Secret.MaxTTL = lease.MaxTTL } - return resp, nil + return response, nil +} + +func isIn200s(respStatus int) bool { + return respStatus >= 200 && respStatus < 300 } const pathRoleCreateReadHelpSyn = ` From 0c12db27496e741062ebb98d82fb0aa474304ef0 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 25 Mar 2020 16:51:05 -0700 Subject: [PATCH 2/4] add test --- builtin/logical/rabbitmq/backend_test.go | 33 ++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/builtin/logical/rabbitmq/backend_test.go b/builtin/logical/rabbitmq/backend_test.go index ac76267f87852..552f5ca704b63 100644 --- a/builtin/logical/rabbitmq/backend_test.go +++ b/builtin/logical/rabbitmq/backend_test.go @@ -97,6 +97,39 @@ func TestBackend_basic(t *testing.T) { } +func TestBackend_returnsErrs(t *testing.T) { + if os.Getenv(logicaltest.TestEnvVar) == "" { + t.Skip(fmt.Sprintf("Acceptance tests skipped unless env '%s' set", logicaltest.TestEnvVar)) + return + } + b, _ := Factory(context.Background(), logical.TestBackendConfig()) + + cleanup, uri, _ := prepareRabbitMQTestContainer(t) + defer cleanup() + + logicaltest.Test(t, logicaltest.TestCase{ + PreCheck: testAccPreCheckFunc(t, uri), + LogicalBackend: b, + Steps: []logicaltest.TestStep{ + testAccStepConfig(t, uri), + { + Operation: logical.CreateOperation, + Path: "roles/web", + Data: map[string]interface{}{ + "tags": testTags, + "vhosts": `{"invalid":{"write": ".*", "read": ".*"}}`, + "vhost_topics": testVHostTopics, + }, + }, + { + Operation: logical.ReadOperation, + Path: "creds/web", + ErrorOk: true, + }, + }, + }) +} + func TestBackend_roleCrud(t *testing.T) { if os.Getenv(logicaltest.TestEnvVar) == "" { t.Skip(fmt.Sprintf("Acceptance tests skipped unless env '%s' set", logicaltest.TestEnvVar)) From a18ad24c586b01daf450d080b23731d65647dc53 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Fri, 27 Mar 2020 14:05:39 -0700 Subject: [PATCH 3/4] Update builtin/logical/rabbitmq/path_role_create.go Co-Authored-By: Vishal Nayak --- builtin/logical/rabbitmq/path_role_create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/rabbitmq/path_role_create.go b/builtin/logical/rabbitmq/path_role_create.go index 15357bd7814ec..a8cbe08c1fbc6 100644 --- a/builtin/logical/rabbitmq/path_role_create.go +++ b/builtin/logical/rabbitmq/path_role_create.go @@ -82,7 +82,7 @@ func (b *backend) pathCredsRead(ctx context.Context, req *logical.Request, d *fr }() if !isIn200s(resp.StatusCode) { body, _ := ioutil.ReadAll(resp.Body) - return nil, fmt.Errorf("error putting user %s - %d: %s", username, resp.StatusCode, body) + return nil, fmt.Errorf("error creating user %s - %d: %s", username, resp.StatusCode, body) } success := false From 976a6f60ce6c666a2681c12265570e35d3544e11 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Fri, 27 Mar 2020 14:14:36 -0700 Subject: [PATCH 4/4] improve error message --- builtin/logical/rabbitmq/path_role_create.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/logical/rabbitmq/path_role_create.go b/builtin/logical/rabbitmq/path_role_create.go index a8cbe08c1fbc6..63c09f6d6d4eb 100644 --- a/builtin/logical/rabbitmq/path_role_create.go +++ b/builtin/logical/rabbitmq/path_role_create.go @@ -93,11 +93,11 @@ func (b *backend) pathCredsRead(ctx context.Context, req *logical.Request, d *fr // Delete the user because it's in an unknown state. resp, err := client.DeleteUser(username) if err != nil { - b.Logger().Error(fmt.Sprintf("failed to delete %s: %s", username, err)) + b.Logger().Error(fmt.Sprintf("deleting %s due to permissions being in an unknown state, but failed: %s", username, err)) } if !isIn200s(resp.StatusCode) { body, _ := ioutil.ReadAll(resp.Body) - b.Logger().Error(fmt.Sprintf("error deleting %s - %d: %s", username, resp.StatusCode, body)) + b.Logger().Error(fmt.Sprintf("deleting %s due to permissions being in an unknown state, but error deleting: %d: %s", username, resp.StatusCode, body)) } }()