Initial Implementation #1

maxcoulombe · 2022-08-16T20:55:50Z

Overview

added initial elasticache redis implementation

Contributor Checklist

Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
- Doc will be added when the project is ready for release
Add output for any tests not ran in CI to the PR description (eg, acceptance tests)

/usr/local/go/bin/go tool test2json -t /tmp/GoLand/___redisElastiCacheClient_test_go.test -test.v -test.paniconexit0 -test.run ^\QTest_generateUserId\E$
=== RUN   Test_generateUserId
=== RUN   Test_generateUserId/compliant_username
=== RUN   Test_generateUserId/short_username
=== RUN   Test_generateUserId/username_too_long
=== RUN   Test_generateUserId/username_with_non-alphanumeric_characters
=== RUN   Test_generateUserId/username_starting_with_a_number
--- PASS: Test_generateUserId (0.00s)
    --- PASS: Test_generateUserId/compliant_username (0.00s)
    --- PASS: Test_generateUserId/short_username (0.00s)
    --- PASS: Test_generateUserId/username_too_long (0.00s)
    --- PASS: Test_generateUserId/username_with_non-alphanumeric_characters (0.00s)
    --- PASS: Test_generateUserId/username_starting_with_a_number (0.00s)
PASS

Backwards compatible

…eRedisBootstrap

+ added acceptance tests * converted to multiplex version

austingebauer

Looking good! Nice job on including the Terraform config for setting things up. I left a few suggestions/questions. It'd also be good to get the rest of the team added for review on this.

austingebauer · 2022-08-22T16:31:09Z

bootstrap/terraform/elasticache.tf

@@ -0,0 +1,78 @@
+provider "aws" {


internal/plugin/redisElastiCacheClient.go

internal/plugin/plugin.go

internal/plugin/redisElastiCacheClient.go

internal/plugin/redisElastiCacheClient_test.go

austingebauer · 2022-08-22T18:39:55Z

internal/plugin/redisElastiCacheClient.go

+// The ID 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 {


If we end up allowing our users to configure a username template, we should consider the potential for collision when using this function to derive the ID from the username. Perhaps there is a favorable alternative that we can come up with.

I was not completely sure about this strategy of generating a userid from the username tbh, I'm happy to discuss the pros and cons.

We could:

Add constraints so userId == username at all times

Generate random userId and use Elasticache's API to fetch and filter by username to get the userId over the network

Do what we do here and have a deterministic way to generate a userId from the information given by Vault

I'm good with option 1 or 3. Option 1 seems workable, but we'd need to validate that constraints are met (esp. when username templates are introduced). For Option 3, it seems unlikely that a collision would occur. I suspect it could if the username template didn't provide sufficient entropy.

I'd say let's go with 3 for now. We can re-evaluate when username templates come in.

internal/plugin/redisElastiCacheClient.go

README.md

fairclothjm · 2022-08-24T14:30:00Z

internal/plugin/redis_elasticache_client.go

+	return dbplugin.NewUserResponse{Username: *output.UserName}, nil
+}
+
+func (r *redisElastiCacheDB) UpdateUser(_ context.Context, req dbplugin.UpdateUserRequest) (dbplugin.UpdateUserResponse, error) {


I know this is just the initial implementation but have you considered any locking strategies?

Good question, I'm looking at it right now as part of the user group support.

AWS usually handle locking on their side. Whenever you add, modify or delete a resource on most of their APIs the resource changes status and you can only execute a subsequent request once the status goes back to "Active".

So most likely the locking strategy will be to rely on the AWS status but I still have to test and read more about this.

internal/plugin/plugin.go

* changed creation_statements format

maxcoulombe · 2022-08-25T03:07:16Z

redis_elasticache_client.go

+	accessString = strings.TrimSpace(accessString)
+
+	return accessString, nil
+}


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.

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

austingebauer · 2022-08-26T16:30:33Z

redis_elasticache_client.go

+	return dbplugin.DeleteUserResponse{}, nil
+}
+
+func parseCreationCommands(commands []string) (string, error) {


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

fairclothjm · 2022-08-26T16:49:59Z

redis_elasticache_client.go

+		}
+	}
+
+	if strings.HasPrefix(accessString, "off ") || strings.Contains(accessString, " off ") || strings.HasSuffix(accessString, " off") {


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

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

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.

* supporting user groups * switched to static-role support only

austingebauer

LGTM!

README.md

redis_elasticache_client.go

…eRedisBootstrap

+ added initial elasticache redis implementation

9ebb9a6

maxcoulombe requested review from austingebauer and Zlaticanin August 16, 2022 21:01

maxcoulombe and others added 8 commits August 18, 2022 14:19

Merge branch 'main' into vault-7720-ElastiCacheRedisBootstrap

1ad98a1

+ added acceptance tests

7a03f33

Merge remote-tracking branch 'origin/main' into vault-7720-ElastiCach…

fb64bc5

…eRedisBootstrap

* go mod tidy

110ab66

* fix tests

0b39bfd

Vault 7721 ElastiCache Acceptance Tests (#3)

ecb43aa

+ added acceptance tests * converted to multiplex version

Merge branch 'main' into vault-7720-ElastiCacheRedisBootstrap

950f372

* update jira action

bced1df

austingebauer reviewed Aug 22, 2022

View reviewed changes

* review refactoring

28c410c

maxcoulombe requested review from robmonte, fairclothjm and vinay-gopalan August 23, 2022 15:50

fairclothjm reviewed Aug 24, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

fairclothjm reviewed Aug 24, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

fairclothjm reviewed Aug 24, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

fairclothjm reviewed Aug 24, 2022

View reviewed changes

internal/plugin/plugin.go Outdated Show resolved Hide resolved

maxcoulombe added 2 commits August 24, 2022 22:57

* refactored package

e84347d

* changed creation_statements format

* typo

bdc5375

maxcoulombe commented Aug 25, 2022

View reviewed changes

* forbid off users

78aef98

austingebauer self-requested a review August 25, 2022 16:44

maxcoulombe added 2 commits August 26, 2022 10:05

* supporting user groups

681e221

* refactor

65e1059

austingebauer reviewed Aug 26, 2022

View reviewed changes

fairclothjm reviewed Aug 26, 2022

View reviewed changes

maxcoulombe and others added 2 commits September 6, 2022 12:05

* switched to static-role support only

2f3a01e

Vault 7916 user group support (#10)

23a967c

* supporting user groups * switched to static-role support only

austingebauer self-requested a review September 6, 2022 16:10

* change readme for static creds only

0efd193

austingebauer approved these changes Sep 6, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

redis_elasticache_client.go Outdated Show resolved Hide resolved

maxcoulombe added 4 commits September 6, 2022 13:27

* clean-up

385357c

Merge branch 'vault-7916-userGroupSupport' into vault-7720-ElastiCach…

284933b

…eRedisBootstrap

* fmt

575ab78

* fix go.mod conflict

e22fb44

maxcoulombe merged commit 548b4d8 into main Sep 6, 2022

maxcoulombe deleted the vault-7720-ElastiCacheRedisBootstrap branch September 6, 2022 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Implementation #1

Initial Implementation #1

maxcoulombe commented Aug 16, 2022 •

edited

austingebauer left a comment

austingebauer Aug 22, 2022

austingebauer Aug 22, 2022

maxcoulombe Aug 23, 2022

austingebauer Aug 26, 2022

fairclothjm Aug 24, 2022

maxcoulombe Aug 24, 2022

maxcoulombe Aug 25, 2022 •

edited

austingebauer Aug 26, 2022

austingebauer Aug 26, 2022

fairclothjm Aug 26, 2022

maxcoulombe Aug 26, 2022 •

edited

fairclothjm Aug 26, 2022

austingebauer left a comment

Initial Implementation #1

Initial Implementation #1

Conversation

maxcoulombe commented Aug 16, 2022 • edited

Overview

Contributor Checklist

austingebauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxcoulombe Aug 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxcoulombe Aug 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austingebauer left a comment

Choose a reason for hiding this comment

maxcoulombe commented Aug 16, 2022 •

edited

maxcoulombe Aug 25, 2022 •

edited

maxcoulombe Aug 26, 2022 •

edited