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.

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

Initial Implementation #1

Merged
merged 22 commits into from
Sep 6, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ fmtcheck:

.PHONY: fmt
fmt:
gofumpt -l -w .
gofumpt -l -w . && cd bootstrap/terraform && terraform fmt

.PHONY: setup-env
setup-env:
Expand Down
10 changes: 4 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ Note that resources created via the Terraform project cost a small amount of mon

To set up the test cluster:

```hcl
```sh
$ make setup-env
...
Apply complete! Resources: 4 added, 0 changed, 0 destroyed.
Expand All @@ -80,7 +80,7 @@ Apply complete! Resources: 4 added, 0 changed, 0 destroyed.

The test cluster created via the setup-env command can be destroyed using the teardown-env command.

```hcl
```sh
$ make teardown-env
...
Destroy complete! Resources: 4 destroyed.
Expand Down Expand Up @@ -109,9 +109,7 @@ Once the server is started, register the plugin in the Vault server's [plugin ca

```sh
$ SHA256=$(openssl dgst -sha256 $GOPATH/vault-plugin-database-redis-elasticache | cut -d ' ' -f2)
$ vault write sys/plugins/catalog/database/vault-plugin-database-redis-elasticache \
command=vault-plugin-database-redis-elasticache \
sha256=$SHA256
$ vault plugin register -sha256=$SHA256 database vault-plugin-database-redis-elasticache
...
Success! Data written to: sys/plugins/catalog/database/vault-plugin-database-redis-elasticache
```
Expand Down Expand Up @@ -144,7 +142,7 @@ Configure a role:
```sh
$ vault write database/roles/redis-myrole \
db_name="redis-mydb" \
creation_statements="on ~* +@all" \
creation_statements='["~*", "+@read"]' \
default_ttl=5m \
max_ttl=15m
...
Expand Down
49 changes: 38 additions & 11 deletions bootstrap/terraform/elasticache.tf
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ provider "aws" {
// region = ""
}

resource "aws_elasticache_cluster" "vault_plugin_elasticache_test" {
cluster_id = "vault-plugin-elasticache-test"
engine = "redis"
engine_version = "6.2"
node_type = "cache.t4g.micro"
num_cache_nodes = 1
parameter_group_name = "default.redis6.x"
resource "aws_elasticache_replication_group" "vault_plugin_elasticache_test" {
replication_group_id = "vault-plugin-elasticache-test"
description = "vault elasticache plugin generated test cluster"
engine = "redis"
engine_version = "6.2"
node_type = "cache.t4g.micro"
num_cache_clusters = 1
parameter_group_name = "default.redis6.x"
transit_encryption_enabled = true

tags = {
"description" : "vault elasticache plugin generated test cluster"
Expand Down Expand Up @@ -47,7 +49,32 @@ data "aws_iam_policy_document" "vault_plugin_elasticache_test" {
"elasticache:ModifyUser",
"elasticache:DeleteUser",
]
resources = ["arn:aws:elasticache:*:*:user:*"]
resources = [
"arn:aws:elasticache:*:*:user:*",
]
}

statement {
actions = [
"elasticache:DescribeUserGroups",
"elasticache:CreateUserGroup",
"elasticache:ModifyUserGroup",
"elasticache:DeleteUserGroup",
"elasticache:ModifyReplicationGroup",
]
resources = [
"arn:aws:elasticache:*:*:usergroup:*",
]
}

statement {
actions = [
"elasticache:DescribeReplicationGroups",
"elasticache:ModifyReplicationGroup",
]
resources = [
"arn:aws:elasticache:*:*:replicationgroup:*",
]
}
}

Expand All @@ -60,15 +87,15 @@ output "username" {
// Use `terraform output password` to access the value
output "password" {
sensitive = true
value = aws_iam_access_key.vault_plugin_elasticache_test.secret
value = aws_iam_access_key.vault_plugin_elasticache_test.secret
}

// export TEST_ELASTICACHE_URL=${url}
output "url" {
value = format(
"%s:%s",
aws_elasticache_cluster.vault_plugin_elasticache_test.cache_nodes[0].address,
aws_elasticache_cluster.vault_plugin_elasticache_test.port)
aws_elasticache_replication_group.vault_plugin_elasticache_test.primary_endpoint_address,
aws_elasticache_replication_group.vault_plugin_elasticache_test.port)
}

// export TEST_ELASTICACHE_REGION=${region}
Expand Down
4 changes: 2 additions & 2 deletions cmd/vault-plugin-database-redis-elasticache/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"log"
"os"

"github.com/hashicorp/vault-plugin-database-redis-elasticache/internal/plugin"
"github.com/hashicorp/vault-plugin-database-redis-elasticache"
"github.com/hashicorp/vault/sdk/database/dbplugin/v5"
)

Expand All @@ -17,7 +17,7 @@ func main() {

// Run starts serving the plugin
func Run() error {
dbplugin.ServeMultiplex(plugin.New)
dbplugin.ServeMultiplex(rediselasticache.New)

return nil
}
2 changes: 1 addition & 1 deletion internal/plugin/plugin.go → plugin.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package plugin
package rediselasticache

import (
"os"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,28 @@
package plugin
package rediselasticache

import (
"context"
"encoding/json"
"errors"
"fmt"
"regexp"
"strings"
"unicode"

"github.com/hashicorp/go-hclog"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/credentials"
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/elasticache"
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/vault/sdk/database/dbplugin/v5"
"github.com/hashicorp/vault/sdk/database/helper/credsutil"
"github.com/mitchellh/mapstructure"
)

var nonAlphanumericRegex = regexp.MustCompile("[^a-zA-Z\\d]+")
var (
nonAlphanumericHyphenRegex = regexp.MustCompile("[^a-zA-Z\\d-]+")
doubleHyphenRegex = regexp.MustCompile("-{2,}")
)

// Verify interface is implemented
var _ dbplugin.Database = (*redisElastiCacheDB)(nil)
Expand Down Expand Up @@ -88,18 +92,21 @@ func (r *redisElastiCacheDB) NewUser(_ context.Context, req dbplugin.NewUserRequ
return dbplugin.NewUserResponse{}, fmt.Errorf("unable to generate username: %w", err)
}

accessString := strings.Join(req.Statements.Commands[:], " ")
accessString, err := parseCreationCommands(req.Statements.Commands)
if err != nil {
return dbplugin.NewUserResponse{}, fmt.Errorf("unable to parse access string: %w", err)
}

userId := generateUserId(username)
userId := normaliseId(username)

output, err := r.client.CreateUser(&elasticache.CreateUserInput{
AccessString: &accessString,
AccessString: aws.String(accessString),
Engine: aws.String("Redis"),
NoPasswordRequired: aws.Bool(false),
Passwords: []*string{&req.Password},
Tags: []*elasticache.Tag{},
UserId: &userId,
UserName: &username,
UserId: aws.String(userId),
UserName: aws.String(username),
})
if err != nil {
return dbplugin.NewUserResponse{}, fmt.Errorf("unable to create new user: %w", err)
Expand All @@ -111,7 +118,7 @@ func (r *redisElastiCacheDB) NewUser(_ context.Context, req dbplugin.NewUserRequ
func (r *redisElastiCacheDB) UpdateUser(_ context.Context, req dbplugin.UpdateUserRequest) (dbplugin.UpdateUserResponse, error) {
r.logger.Debug("updating AWS ElastiCache Redis user", "username", req.Username)

userId := generateUserId(req.Username)
userId := normaliseId(req.Username)

_, err := r.client.ModifyUser(&elasticache.ModifyUserInput{
UserId: &userId,
Expand All @@ -127,7 +134,7 @@ func (r *redisElastiCacheDB) UpdateUser(_ context.Context, req dbplugin.UpdateUs
func (r *redisElastiCacheDB) DeleteUser(_ context.Context, req dbplugin.DeleteUserRequest) (dbplugin.DeleteUserResponse, error) {
r.logger.Debug("deleting AWS ElastiCache Redis user", "username", req.Username)

userId := generateUserId(req.Username)
userId := normaliseId(req.Username)

_, err := r.client.DeleteUser(&elasticache.DeleteUserInput{
UserId: &userId,
Expand All @@ -139,19 +146,56 @@ func (r *redisElastiCacheDB) DeleteUser(_ context.Context, req dbplugin.DeleteUs
return dbplugin.DeleteUserResponse{}, nil
}

// The ID can have up to 40 characters, and must begin with a letter.
func parseCreationCommands(commands []string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: A small comment on what this function does would help future readers understand quickly. Details you shared in files#r954459565 would be perfect.

if len(commands) == 0 {
return "on ~* +@read", nil
}

accessString := ""
for _, command := range commands {
var rules []string
err := json.Unmarshal([]byte(command), &rules)
if err != nil {
return "", err
}

if len(rules) > 0 {
accessString += strings.Join(rules, " ")
accessString += " "
}
}

if strings.HasPrefix(accessString, "off ") || strings.Contains(accessString, " off ") || strings.HasSuffix(accessString, " off") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would strings.Contains(accessString, "off") be sufficient? Or are the spaces required here because off could occur in another context?

Copy link
Contributor Author

@maxcoulombe maxcoulombe Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could occur in another context. For example, if an eshop business caches special offers in Redis using the key pattern:
offer-summer-sales
And they try to create a user that has access to only the cache entries for these special offers they'd try to configure the role with:
on ~offer-* +@read
which would block them and return an erroneous error message even if the rules are legitimate.

It really needs to be the keyword "off" all by itself.

Would it be more readable with a word-boundary regex? \boff\b

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for verifying! That makes sense -- assuming key patterns cannot contain spaces.

Yeah my initial thought was to use a regex. But that is just a nitpick. I am good with either one.

return "", errors.New("creation of disabled or 'off' users is forbidden")
}

if !(strings.HasPrefix(accessString, "on ") || strings.Contains(accessString, " on ") || strings.HasSuffix(accessString, " on")) {
accessString = "on " + accessString
}

accessString = strings.TrimSpace(accessString)

return accessString, nil
}
Copy link
Contributor Author

@maxcoulombe maxcoulombe Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For people who already reviewed, this is a behaviour change that was introduced in between the updates.

We talked and decided that the format for the creation statements should be a list of strings instead of a string.
We also decided to handle the "ON" keyword for the users so they can give it or omit it at their preference. With this decision we also forbid the creation of disabled or "OFF" users as using vault to manage disabled users seems pointless.
Finally we decided that if no creation commands is given we'd create a general purpose read-only user as the default behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. The tests helped me understand the behavior here. Nice job on those!


// All Elasticache IDs can have up to 40 characters, and must begin with a letter.
// It should not end with a hyphen or contain two consecutive hyphens.
// Valid characters: A-Z, a-z, 0-9, and -(hyphen).
func generateUserId(username string) string {
userId := nonAlphanumericRegex.ReplaceAllString(username, "")
func normaliseId(raw string) string {
normalized := nonAlphanumericHyphenRegex.ReplaceAllString(raw, "")
normalized = doubleHyphenRegex.ReplaceAllString(normalized, "")

if len(normalized) > 40 {
normalized = normalized[len(normalized)-40:]
}

if len(userId) > 40 {
userId = userId[len(userId)-40:]
if unicode.IsNumber(rune(normalized[0])) {
normalized = string(rune('A'-17+normalized[0])) + normalized[1:]
}

if unicode.IsNumber(rune(userId[0])) {
userId = string(rune('A'-17+userId[0])) + userId[1:]
if strings.HasSuffix(normalized, "-") {
normalized = normalized[:len(normalized)-1] + "x"
}

return userId
return normalized
}
Loading