From 5783b02e36c0a9c5f0e3890ef43f47a1ba58cd6d Mon Sep 17 00:00:00 2001 From: Kevin Pike Date: Fri, 20 May 2016 22:51:09 -0700 Subject: [PATCH] Address feedback --- builtin/logical/rabbitmq/backend.go | 10 +-- builtin/logical/rabbitmq/backend_test.go | 4 +- .../rabbitmq/path_config_connection.go | 4 +- builtin/logical/rabbitmq/path_config_lease.go | 89 +++++++++++-------- .../rabbitmq/path_config_lease_test.go | 53 +++++++++++ builtin/logical/rabbitmq/path_role_create.go | 28 +++--- builtin/logical/rabbitmq/path_roles.go | 69 +++++++++----- builtin/logical/rabbitmq/path_roles_test.go | 42 +++++++++ builtin/logical/rabbitmq/secret_creds.go | 10 +-- 9 files changed, 222 insertions(+), 87 deletions(-) create mode 100644 builtin/logical/rabbitmq/path_config_lease_test.go create mode 100644 builtin/logical/rabbitmq/path_roles_test.go diff --git a/builtin/logical/rabbitmq/backend.go b/builtin/logical/rabbitmq/backend.go index f806798d0..ab50833e6 100644 --- a/builtin/logical/rabbitmq/backend.go +++ b/builtin/logical/rabbitmq/backend.go @@ -21,17 +21,11 @@ func Backend() *framework.Backend { b.Backend = &framework.Backend{ Help: strings.TrimSpace(backendHelp), - PathsSpecial: &logical.Paths{ - Root: []string{ - "config/*", - }, - }, - Paths: []*framework.Path{ pathConfigConnection(&b), pathConfigLease(&b), - pathRoles(&b), pathRoleCreate(&b), + pathRoles(&b), }, Secrets: []*framework.Secret{ @@ -94,7 +88,7 @@ func (b *backend) ResetClient() { // Lease returns the lease information func (b *backend) Lease(s logical.Storage) (*configLease, error) { - entry, err := s.Get("config/lease") + entry, err := s.Get(leasePatternLabel) if err != nil { return nil, err } diff --git a/builtin/logical/rabbitmq/backend_test.go b/builtin/logical/rabbitmq/backend_test.go index bbaf2abd9..8adddb3ed 100644 --- a/builtin/logical/rabbitmq/backend_test.go +++ b/builtin/logical/rabbitmq/backend_test.go @@ -58,7 +58,7 @@ func testAccPreCheck(t *testing.T) { func testAccStepConfig(t *testing.T) logicaltest.TestStep { return logicaltest.TestStep{ - Operation: logical.WriteOperation, + Operation: logical.UpdateOperation, Path: "config/connection", Data: map[string]interface{}{ "uri": os.Getenv("RABBITMQ_MG_URI"), @@ -70,7 +70,7 @@ func testAccStepConfig(t *testing.T) logicaltest.TestStep { func testAccStepRole(t *testing.T) logicaltest.TestStep { return logicaltest.TestStep{ - Operation: logical.WriteOperation, + Operation: logical.UpdateOperation, Path: "roles/web", Data: map[string]interface{}{ "tags": "administrator", diff --git a/builtin/logical/rabbitmq/path_config_connection.go b/builtin/logical/rabbitmq/path_config_connection.go index e814ee37e..39c1c00a1 100644 --- a/builtin/logical/rabbitmq/path_config_connection.go +++ b/builtin/logical/rabbitmq/path_config_connection.go @@ -27,7 +27,7 @@ func pathConfigConnection(b *backend) *framework.Path { }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.WriteOperation: b.pathConnectionWrite, + logical.UpdateOperation: b.pathConnectionUpdate, }, HelpSynopsis: pathConfigConnectionHelpSyn, @@ -35,7 +35,7 @@ func pathConfigConnection(b *backend) *framework.Path { } } -func (b *backend) pathConnectionWrite(req *logical.Request, data *framework.FieldData) (*logical.Response, error) { +func (b *backend) pathConnectionUpdate(req *logical.Request, data *framework.FieldData) (*logical.Response, error) { uri := data.Get("uri").(string) username := data.Get("username").(string) password := data.Get("password").(string) diff --git a/builtin/logical/rabbitmq/path_config_lease.go b/builtin/logical/rabbitmq/path_config_lease.go index ec3bb22f0..b512773cd 100644 --- a/builtin/logical/rabbitmq/path_config_lease.go +++ b/builtin/logical/rabbitmq/path_config_lease.go @@ -1,6 +1,7 @@ package rabbitmq import ( + "errors" "fmt" "time" @@ -8,24 +9,34 @@ import ( "github.com/hashicorp/vault/logical/framework" ) -func pathConfigLease(b *backend) *framework.Path { - return &framework.Path{ - Pattern: "config/lease", - Fields: map[string]*framework.FieldSchema{ - "lease": &framework.FieldSchema{ - Type: framework.TypeString, - Description: "Default lease for roles.", - }, +const ( + leaseLabel = "ttl" + leaseMaxLabel = "ttl_max" + leasePatternLabel = "config/" + leaseLabel +) - "lease_max": &framework.FieldSchema{ - Type: framework.TypeString, - Description: "Maximum time a credential is valid for.", - }, +func configFields() map[string]*framework.FieldSchema { + return map[string]*framework.FieldSchema{ + leaseLabel: &framework.FieldSchema{ + Type: framework.TypeDurationSecond, + Description: "Default " + leaseLabel + " for roles.", }, + leaseMaxLabel: &framework.FieldSchema{ + Type: framework.TypeDurationSecond, + Description: "Maximum time a credential is valid for.", + }, + } +} + +func pathConfigLease(b *backend) *framework.Path { + return &framework.Path{ + Pattern: leasePatternLabel, + Fields: configFields(), + Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.ReadOperation: b.pathLeaseRead, - logical.WriteOperation: b.pathLeaseWrite, + logical.ReadOperation: b.pathLeaseRead, + logical.UpdateOperation: b.pathLeastUpdate, }, HelpSynopsis: pathConfigLeaseHelpSyn, @@ -33,24 +44,15 @@ func pathConfigLease(b *backend) *framework.Path { } } -func (b *backend) pathLeaseWrite( +func (b *backend) pathLeastUpdate( req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - leaseRaw := d.Get("lease").(string) - leaseMaxRaw := d.Get("lease_max").(string) - - lease, err := time.ParseDuration(leaseRaw) + lease, leaseMax, err := validateLeases(d) if err != nil { - return logical.ErrorResponse(fmt.Sprintf( - "Invalid lease: %s", err)), nil - } - leaseMax, err := time.ParseDuration(leaseMaxRaw) - if err != nil { - return logical.ErrorResponse(fmt.Sprintf( - "Invalid lease: %s", err)), nil + return nil, err } // Store it - entry, err := logical.StorageEntryJSON("config/lease", &configLease{ + entry, err := logical.StorageEntryJSON(leasePatternLabel, &configLease{ Lease: lease, LeaseMax: leaseMax, }) @@ -77,8 +79,8 @@ func (b *backend) pathLeaseRead( return &logical.Response{ Data: map[string]interface{}{ - "lease": lease.Lease.String(), - "lease_max": lease.LeaseMax.String(), + leaseLabel: lease.Lease.String(), + leaseMaxLabel: lease.LeaseMax.String(), }, }, nil } @@ -88,16 +90,29 @@ type configLease struct { LeaseMax time.Duration } -const pathConfigLeaseHelpSyn = ` -Configure the default lease information for generated credentials. -` +func validateLeases(data *framework.FieldData) (lease, leaseMax time.Duration, err error) { -const pathConfigLeaseHelpDesc = ` -This configures the default lease information used for credentials -generated by this backend. The lease specifies the duration that a + leaseRaw := data.Get(leaseLabel).(int) + leaseMaxRaw := data.Get(leaseMaxLabel).(int) + + if leaseRaw == 0 && leaseMaxRaw == 0 { + err = errors.New(leaseLabel + " or " + leaseMaxLabel + " must have a value") + return + } + + return time.Duration(leaseRaw) * time.Second, time.Duration(leaseMaxRaw) * time.Second, nil +} + +var pathConfigLeaseHelpSyn = fmt.Sprintf(` +Configure the default %s information for generated credentials. +`, leaseLabel) + +var pathConfigLeaseHelpDesc = fmt.Sprintf(` +This configures the default %s information used for credentials +generated by this backend. The %s specifies the duration that a credential will be valid for, as well as the maximum session for a set of credentials. -The format for the lease is "1h" or integer and then unit. The longest +The format for the %s is "1h" or integer and then unit. The longest unit is hour. -` +`, leaseLabel, leaseLabel, leaseLabel) diff --git a/builtin/logical/rabbitmq/path_config_lease_test.go b/builtin/logical/rabbitmq/path_config_lease_test.go new file mode 100644 index 000000000..6d71faa57 --- /dev/null +++ b/builtin/logical/rabbitmq/path_config_lease_test.go @@ -0,0 +1,53 @@ +package rabbitmq + +import ( + "testing" + + "github.com/hashicorp/vault/logical/framework" +) + +type validateLeasesTestCase struct { + Lease int + LeaseMax int + Fail bool +} + +func TestConfigLease_validateLeases(t *testing.T) { + cases := map[string]validateLeasesTestCase{ + "Both lease and lease max": { + Lease: 60 * 60, + LeaseMax: 60 * 60, + }, + "Just lease": { + Lease: 60 * 60, + LeaseMax: 0, + }, + "No lease nor lease max": { + Lease: 0, + LeaseMax: 0, + Fail: true, + }, + } + + data := &framework.FieldData{ + Schema: configFields(), + } + for name, c := range cases { + data.Raw = map[string]interface{}{ + leaseLabel: c.Lease, + leaseMaxLabel: c.LeaseMax, + } + + _, _, err := validateLeases(data) + if err != nil && c.Fail { + // This was expected + continue + } else if err != nil { + // This was unexpected + t.Errorf("Failed: %s", name) + } else if err == nil && c.Fail { + // This was unexpected + t.Errorf("Failed to fail: %s", name) + } + } +} diff --git a/builtin/logical/rabbitmq/path_role_create.go b/builtin/logical/rabbitmq/path_role_create.go index aafab011e..aa83acb92 100644 --- a/builtin/logical/rabbitmq/path_role_create.go +++ b/builtin/logical/rabbitmq/path_role_create.go @@ -31,7 +31,11 @@ func pathRoleCreate(b *backend) *framework.Path { func (b *backend) pathRoleCreateRead( req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - name := data.Get("name").(string) + // Validate name + name, err := validateName(data) + if err != nil { + return nil, err + } // Get the role role, err := b.Role(req.Storage, name) @@ -51,15 +55,8 @@ func (b *backend) pathRoleCreateRead( lease = &configLease{Lease: 1 * time.Hour} } - // Generate the username, password and expiration. PG limits user to 63 characters - displayName := req.DisplayName - if len(displayName) > 26 { - displayName = displayName[:26] - } - username := fmt.Sprintf("%s-%s", displayName, uuid.GenerateUUID()) - if len(username) > 63 { - username = username[:63] - } + // Ensure username is unique + username := fmt.Sprintf("%s-%s", req.DisplayName, uuid.GenerateUUID()) password := uuid.GenerateUUID() // Get our connection @@ -68,6 +65,10 @@ func (b *backend) pathRoleCreateRead( return nil, err } + if client == nil { + return logical.ErrorResponse("unable to get client"), nil + } + // Create the user _, err = client.PutUser(username, rabbithole.UserSettings{ Password: password, @@ -86,7 +87,12 @@ func (b *backend) pathRoleCreateRead( }) if err != nil { - return nil, err + // Delete the user because it's in an unknown state + _, rmErr := client.DeleteUser(username) + if rmErr != nil { + return logical.ErrorResponse(fmt.Sprintf("failed to update user: %s, failed to delete user: %s, user: %s", err, rmErr, username)), rmErr + } + return logical.ErrorResponse(fmt.Sprintf("failed to update user: %s, user: %s", err, username)), err } } diff --git a/builtin/logical/rabbitmq/path_roles.go b/builtin/logical/rabbitmq/path_roles.go index 8c8631297..4760e1ce3 100644 --- a/builtin/logical/rabbitmq/path_roles.go +++ b/builtin/logical/rabbitmq/path_roles.go @@ -2,35 +2,40 @@ package rabbitmq import ( "encoding/json" + "errors" "fmt" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" ) +func rolesFields() map[string]*framework.FieldSchema { + return map[string]*framework.FieldSchema{ + "name": &framework.FieldSchema{ + Type: framework.TypeString, + Description: "Name of the role.", + }, + + "tags": &framework.FieldSchema{ + Type: framework.TypeString, + Description: "Comma-separated list of tags for this role.", + }, + + "vhosts": &framework.FieldSchema{ + Type: framework.TypeString, + Description: "A map of virtual hosts to permissions.", + }, + } +} + func pathRoles(b *backend) *framework.Path { return &framework.Path{ Pattern: "roles/" + framework.GenericNameRegex("name"), - Fields: map[string]*framework.FieldSchema{ - "name": &framework.FieldSchema{ - Type: framework.TypeString, - Description: "Name of the role.", - }, - - "tags": &framework.FieldSchema{ - Type: framework.TypeString, - Description: "Comma-separated list of tags for this role.", - }, - - "vhosts": &framework.FieldSchema{ - Type: framework.TypeString, - Description: "A map of virtual hosts to permissions.", - }, - }, + Fields: rolesFields(), Callbacks: map[logical.Operation]framework.OperationFunc{ logical.ReadOperation: b.pathRoleRead, - logical.WriteOperation: b.pathRoleCreate, + logical.CreateOperation: b.pathRoleCreate, logical.DeleteOperation: b.pathRoleDelete, }, @@ -58,7 +63,13 @@ func (b *backend) Role(s logical.Storage, n string) (*roleEntry, error) { func (b *backend) pathRoleDelete( req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - err := req.Storage.Delete("role/" + data.Get("name").(string)) + + name, err := validateName(data) + if err != nil { + return nil, err + } + + err = req.Storage.Delete("role/" + name) if err != nil { return nil, err } @@ -68,7 +79,13 @@ func (b *backend) pathRoleDelete( func (b *backend) pathRoleRead( req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - role, err := b.Role(req.Storage, data.Get("name").(string)) + + name, err := validateName(data) + if err != nil { + return nil, err + } + + role, err := b.Role(req.Storage, name) if err != nil { return nil, err } @@ -86,7 +103,10 @@ func (b *backend) pathRoleRead( func (b *backend) pathRoleCreate( req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - name := data.Get("name").(string) + name, err := validateName(data) + if err != nil { + return nil, err + } tags := data.Get("tags").(string) rawVHosts := data.Get("vhosts").(string) @@ -124,6 +144,15 @@ type vhostPermission struct { Read string `json:"read"` } +func validateName(data *framework.FieldData) (string, error) { + name := data.Get("name").(string) + if len(name) == 0 { + return "", errors.New("name is required") + } + + return name, nil +} + const pathRoleHelpSyn = ` Manage the roles that can be created with this backend. ` diff --git a/builtin/logical/rabbitmq/path_roles_test.go b/builtin/logical/rabbitmq/path_roles_test.go new file mode 100644 index 000000000..7a9aa4579 --- /dev/null +++ b/builtin/logical/rabbitmq/path_roles_test.go @@ -0,0 +1,42 @@ +package rabbitmq + +import ( + "testing" + + "github.com/hashicorp/vault/logical/framework" +) + +type validateNameTestCase struct { + Name string + Fail bool +} + +func TestRoles_validateName(t *testing.T) { + cases := map[string]validateNameTestCase{ + "test name": { + Name: "test", + }, + "empty name": { + Name: "", + Fail: true, + }, + } + + data := &framework.FieldData{ + Schema: rolesFields(), + } + for name, c := range cases { + data.Raw = map[string]interface{}{ + "name": c.Name, + } + + actual, err := validateName(data) + if err != nil && !c.Fail { + t.Error(err) + } + + if c.Name != actual { + t.Errorf("Fail: %s: expected %s, got %s", name, c.Name, actual) + } + } +} diff --git a/builtin/logical/rabbitmq/secret_creds.go b/builtin/logical/rabbitmq/secret_creds.go index 151ef6978..6183ab2ee 100644 --- a/builtin/logical/rabbitmq/secret_creds.go +++ b/builtin/logical/rabbitmq/secret_creds.go @@ -2,7 +2,6 @@ package rabbitmq import ( "fmt" - "time" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" @@ -26,9 +25,6 @@ func secretCreds(b *backend) *framework.Secret { }, }, - DefaultDuration: 1 * time.Hour, - DefaultGracePeriod: 10 * time.Minute, - Renew: b.secretCredsRenew, Revoke: b.secretCredsRevoke, } @@ -42,10 +38,10 @@ func (b *backend) secretCredsRenew( return nil, err } if lease == nil { - lease = &configLease{Lease: 1 * time.Hour} + lease = &configLease{} } - f := framework.LeaseExtend(lease.Lease, lease.LeaseMax, false) + f := framework.LeaseExtend(lease.Lease, lease.LeaseMax, b.System()) resp, err := f(req, d) if err != nil { return nil, err @@ -61,7 +57,7 @@ func (b *backend) secretCredsRevoke( if !ok { return nil, fmt.Errorf("secret is missing username internal data") } - username, ok := usernameRaw.(string) + username := usernameRaw.(string) // Get our connection client, err := b.Client(req.Storage)