Merge pull request #1 from KevinPike/rabbitmq

Address RabbitMQ feedback

builtin/logical/rabbitmq/README.md

@@ -0,0 +1,10 @@
+# RabbitMQ Backend
+
+## Testing
+
+There are unit and integration RabbitMQ backend tests. Unit tests can be run by `go test`. Integration tests require setting the following environment variables:
+```
+RABBITMQ_CONNECTION_URI=
+RABBITMQ_USERNAME=
+RABBITMQ_PASSWORD=
+```

builtin/logical/rabbitmq/backend.go

@@ -21,18 +21,12 @@ 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),
 			pathListRoles(&b),
-			pathRoles(&b),
 			pathRoleCreate(&b),
+			pathRoles(&b),
 		},
 
 		Secrets: []*framework.Secret{
@@ -95,7 +89,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
 	}

builtin/logical/rabbitmq/backend_test.go

@@ -44,15 +44,25 @@ func TestBackend_roleCrud(t *testing.T) {
 	})
 }
 
+const (
+	uriEnv      = "RABBITMQ_CONNECTION_URI"
+	usernameEnv = "RABBITMQ_USERNAME"
+	passwordEnv = "RABBITMQ_PASSWORD"
+)
+
+func mustSet(name string) string {
+	return fmt.Sprintf("%s must be set for acceptance tests", name)
+}
+
 func testAccPreCheck(t *testing.T) {
-	if uri := os.Getenv("RABBITMQ_MG_URI"); uri == "" {
-		t.Fatal("RABBITMQ_MG_URI must be set for acceptance tests")
+	if uri := os.Getenv(uriEnv); uri == "" {
+		t.Fatal(mustSet(uriEnv))
 	}
-	if username := os.Getenv("RABBITMQ_MG_USERNAME"); username == "" {
-		t.Fatal("RABBITMQ_MG_USERNAME must be set for acceptance tests")
+	if username := os.Getenv(usernameEnv); username == "" {
+		t.Fatal(mustSet(usernameEnv))
 	}
-	if password := os.Getenv("RABBITMQ_MG_PASSWORD"); password == "" {
-		t.Fatal("RABBITMQ_MG_PASSWORD must be set for acceptance tests")
+	if password := os.Getenv(passwordEnv); password == "" {
+		t.Fatal(mustSet(passwordEnv))
 	}
 }
 
@@ -61,9 +71,9 @@ func testAccStepConfig(t *testing.T) logicaltest.TestStep {
 		Operation: logical.UpdateOperation,
 		Path:      "config/connection",
 		Data: map[string]interface{}{
-			"uri":      os.Getenv("RABBITMQ_MG_URI"),
-			"username": os.Getenv("RABBITMQ_MG_USERNAME"),
-			"password": os.Getenv("RABBITMQ_MG_PASSWORD"),
+			"connection_uri": os.Getenv(uriEnv),
+			"username":       os.Getenv(usernameEnv),
+			"password":       os.Getenv(passwordEnv),
 		},
 	}
 }
@@ -100,7 +110,7 @@ func testAccStepReadCreds(t *testing.T, b logical.Backend, name string) logicalt
 			}
 			log.Printf("[WARN] Generated credentials: %v", d)
 
-			uri := os.Getenv("RABBITMQ_MG_URI")
+			uri := os.Getenv(uriEnv)
 
 			client, err := rabbithole.NewClient(uri, d.Username, d.Password)
 			if err != nil {
@@ -182,15 +192,15 @@ func testAccStepReadRole(t *testing.T, name, tags, rawVHosts string) logicaltest
 				}
 
 				if actualPermission.Configure != permission.Configure {
-					fmt.Errorf("expected permission %s to be %s, got %s", "configure", permission.Configure, actualPermission.Configure)
+					return fmt.Errorf("expected permission %s to be %s, got %s", "configure", permission.Configure, actualPermission.Configure)
 				}
 
 				if actualPermission.Write != permission.Write {
-					fmt.Errorf("expected permission %s to be %s, got %s", "write", permission.Write, actualPermission.Write)
+					return fmt.Errorf("expected permission %s to be %s, got %s", "write", permission.Write, actualPermission.Write)
 				}
 
 				if actualPermission.Read != permission.Read {
-					fmt.Errorf("expected permission %s to be %s, got %s", "read", permission.Read, actualPermission.Read)
+					return fmt.Errorf("expected permission %s to be %s, got %s", "read", permission.Read, actualPermission.Read)
 				}
 			}
 

builtin/logical/rabbitmq/path_config_connection.go

@@ -32,7 +32,7 @@ func pathConfigConnection(b *backend) *framework.Path {
 		},
 
 		Callbacks: map[logical.Operation]framework.OperationFunc{
-			logical.UpdateOperation: b.pathConnectionWrite,
+			logical.UpdateOperation: b.pathConnectionUpdate,
 		},
 
 		HelpSynopsis:    pathConfigConnectionHelpSyn,
@@ -40,7 +40,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("connection_uri").(string)
 	username := data.Get("username").(string)
 	password := data.Get("password").(string)

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.UpdateOperation: b.pathLeaseWrite,
+			logical.ReadOperation:   b.pathLeaseRead,
+			logical.UpdateOperation: b.pathLeaseUpdate,
 		},
 
 		HelpSynopsis:    pathConfigLeaseHelpSyn,
@@ -33,24 +44,15 @@ func pathConfigLease(b *backend) *framework.Path {
 	}
 }
 
-func (b *backend) pathLeaseWrite(
+func (b *backend) pathLeaseUpdate(
 	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)

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)
+		}
+	}
+}

builtin/logical/rabbitmq/path_role_create.go

@@ -30,7 +30,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)
@@ -50,15 +54,8 @@ func (b *backend) pathRoleCreateRead(
 		lease = &configLease{}
 	}
 
-	displayName := req.DisplayName
-	if len(displayName) > 26 {
-		displayName = displayName[:26]
-	}
-	userUUID := uuid.GenerateUUID()
-	username := fmt.Sprintf("%s-%s", displayName, userUUID)
-	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
@@ -67,6 +64,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,
@@ -85,7 +86,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
 		}
 	}
 

builtin/logical/rabbitmq/path_roles.go

@@ -2,12 +2,32 @@ 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 pathListRoles(b *backend) *framework.Path {
 	return &framework.Path{
 		Pattern: "roles/?$",
@@ -24,26 +44,11 @@ func pathListRoles(b *backend) *framework.Path {
 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.UpdateOperation: b.pathRoleCreate,
+			logical.UpdateOperation: b.pathRoleUpdate,
 			logical.DeleteOperation: b.pathRoleDelete,
 		},
 
@@ -71,7 +76,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
 	}
@@ -81,7 +92,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
 	}
@@ -107,9 +124,12 @@ func (b *backend) pathRoleList(
 	return logical.ListResponse(entries), nil
 }
 
-func (b *backend) pathRoleCreate(
+func (b *backend) pathRoleUpdate(
 	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)
 
@@ -147,6 +167,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.
 `

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)
+		}
+	}
+}

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"
@@ -39,7 +38,7 @@ 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, b.System())
@@ -58,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)

website/source/docs/secrets/rabbitmq/index.html.md

@@ -56,7 +56,7 @@ Optionally, we can configure the lease settings for credentials generated
 by Vault. This is done by writing to the `config/lease` key:
 
 ```
-$ vault write rabbitmq/config/lease lease=1h lease_max=24h
+$ vault write rabbitmq/config/lease ttl=3600 ttl_max=86400
 Success! Data written to: rabbitmq/config/lease
 ```
 
@@ -162,8 +162,7 @@ subpath for interactive help output.
 <dl class="api">
   <dt>Description</dt>
   <dd>
-    Configures the lease settings for generated credentials.
-    If not configured, leases default to 1 hour. This is a root
+    Configures the lease settings for generated credentials. This is a root
     protected endpoint.
   </dd>
 
@@ -177,16 +176,14 @@ subpath for interactive help output.
   <dd>
     <ul>
       <li>
-        <span class="param">lease</span>
+        <span class="param">ttl</span>
         <span class="param-flags">required</span>
-        The lease value provided as a string duration
-        with time suffix. Hour is the largest suffix.
+        The lease ttl provided in seconds.
       </li>
       <li>
-        <span class="param">lease_max</span>
+        <span class="param">ttl_max</span>
         <span class="param-flags">required</span>
-        The maximum lease value provided as a string duration
-        with time suffix. Hour is the largest suffix.
+        The maximum ttl provided in seconds.
       </li>
     </ul>
   </dd>