feat(cli): Add support for password credential in redis helper #6213

laero · 2025-10-29T21:43:31Z

Description

Parse password credential into a UsernamePassword for usage in brokering
Reworked redis connect: we now prompt user for a password when no password is brokered
Updated and cleaned up redis tests

Sample CLI Usage

(most responses omitted for brevity)

boundary scopes create -scope-id global
boundary scopes create -scope-id o_fJtwTPHZdM
boundary targets create tcp -name "test-target" -scope-id p_SdrMtBv6Zi -default-port 22 -address 127.0.0.1
boundary credential-stores create static -scope-id p_SdrMtBv6Zi
export TESTPASS="test-password"
boundary credentials create password -credential-store-id csst_POG2btczXC -password env://TESTPASS -name test-pcred

Credential information:
  Created Time:          Thu, 30 Oct 2025 11:27:44 CDT
  Credential Store ID:   csst_POG2btczXC
  ID:                    credp_DlHnSSRBpu
  Name:                  test-pcred
  Type:                  password
  Updated Time:          Thu, 30 Oct 2025 11:27:44 CDT
  Version:               1

  Scope:
    ID:                  p_SdrMtBv6Zi
    Name:
    Parent Scope ID:     o_fJtwTPHZdM
    Type:                project

  Authorized Actions:
    update
    delete
    no-op
    read

  Attributes:
    Password HMAC:       eoqMVkU3PezYWtRJjMKGNOdi0xu0B1pCvT6aumdlU9o

boundary targets add-credential-sources -id ttcp_tucp5A15x1 -brokered-credential-source credp_DlHnSSRBpu
boundary connect -target-id ttcp_tucp5A15x1

Proxy listening information:
  Address:             127.0.0.1
  Connection Limit:    -1
  Expiration:          Thu, 30 Oct 2025 19:29:52 CDT
  Port:                63121
  Protocol:            tcp
  Session ID:          s_5CBGrxXLJ7

  Credentials:
    Credential Source ID:   credp_DlHnSSRBpu
    Credential Source Name: test-pcred
    Credential Store ID:    csst_POG2btczXC
    Credential Store Type:  static
    Credential Type:        password
    Secret:
          password:   test-password

louisruch

Thanks for adding e2e tests for this - great to see it coming together.
I left a comment around the what is happening parseCredentials

Also when working on add CLI comments I really love to see some sample CLI usage in the PR description as it clearly shows what the UX will be. Could you add CLI request and response for:

Create a password credential
Add it as a brokered credential source, and then should the response from boundary connect (not the redis connect as I don't want it consumed)

api/proxy/credentials.go

testing/internal/e2e/tests/base/target_tcp_connect_redis_test.go

bgajjala8 · 2025-11-03T18:32:00Z

testing/internal/e2e/tests/base/target_tcp_connect_redis_test.go

+			t.Cleanup(func() {
+				flushRedis(t, ctx, resources.targetId)
+			})


I see why you did this here. However, can we just flush it at the end of the test (where we are running the commands) after the GET command? This would save us from connecting to the target once again?

i chose to separate the cleanup from the validation logic here for readability and maintainability. i think the trade-off of requiring a reconnect is worth it

if for some reason someone needs to update this test they don't need to pay attention to cleanup logic (they can't accidentally insert after flushing for example)

bgajjala8 · 2025-11-03T18:32:27Z

testing/internal/e2e/tests/base/target_tcp_connect_redis_test.go

+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			// Setup
+			resources := setupBoundaryCredentialStore(t, ctx, redisInfo.Hostname, redisInfo.Port)


(nit): can we reuse one credential store for all tests in the list instead of setting one up per test? I feel like both approaches are fine so I will leave it up to you on this one

i think it's better to have complete isolation here. it's unlikely but possible for a test to reconfigure resources, and that could cause issues if they are shared. we just avoid a lot of issues by ensuring there's a clean setup for each step

i'd consider recreating redis too, but i think that is probably overkill 🤔

One idea I'm toying with is actually just separating these into two different tests in different files

target_tcp_connect_redis_username_password_test.go target_tcp_connect_redis_password_test.go

That would help with readability of the test and how this file is currently organized. That would make it so you're recreating redis, but docker containers aren't too bad to spin up and down.

redis starts up pretty quickly. would you choose separate files over creating two separate test functions in this file? i originally had two functions like so:

func TestCliTcpTargetConnectRedisWithUsernamePassword(t *testing.T) {...} func TestCliTcpTargetConnectRedisWithPassword(t *testing.T) {...}

we moved to using subtests to re-use redis and reduce repeated code. i am partial to to having separate functions within the same file because it's a bit easier to browse in an IDE, and simpler to share helper functions if needed

I may be off-base in my opinion right now 😅 but here's what i'm thinking

I'm less concerned about repeated code in e2e tests as they are limited in scope. They also help to serve as sort of like documentation for how to run certain tests.

I'm currently leaning towards having separate files. Part of me likes having one e2e test in a file. It keeps the file length smaller and helps to see the e2e coverage when looking at the files in a folder.

Please feel free to challenge those opinions.

i saw your comment after i updated my comment, so i will just reiterate to make sure it's not lost:

that's a really great point about repeated code in e2e, thanks for the insight!

regarding the test organization:

having the tests in one file improves readability/organization when there are shared helper functions (although based on other comments these helpers are probably getting removed or refactored into a more appropriate place anyways)

just noting: some of the other e2e test files have multiple test functions already so using relying on filenames isn't giving us a full picture as it is. so you'd have to dive into those files anyways. it can still help if we stick to one test per file moving forward though

i think in most scenarios, people would be more interested in the coverage over a particular feature rather than overall coverage. it's easier to scroll down the outline / function list to see coverage of a feature, there is less to parse through because i'm not looking at a list containing unrelated filenames

i do agree that having smaller test files is a lot more readable than the large ones

i don't have a strong opinion on this, just exploring the options here. let me know if that has changed your mind or if you'd prefer to stick to one test per file 😄

i went ahead and put them into separate files for now based on your original suggestion:

target_tcp_connect_redis_username_password_test.go target_tcp_connect_redis_password_test.go

testing/internal/e2e/tests/base/helpers_test.go

moduli · 2025-11-03T20:50:13Z

testing/internal/e2e/boundary/credential.go

 // provided static credential store.
 // Returns the id of the new credential
-func CreateStaticCredentialPasswordCli(t testing.TB, ctx context.Context, credentialStoreId string, user string, password string) (string, error) {
+func CreateStaticCredentialUsernamePasswordCli(t testing.TB, ctx context.Context, credentialStoreId string, user string, password string) (string, error) {


This change makes sense. Appreciate the cleanup/refactor here

testing/internal/e2e/tests/base/helpers_test.go

moduli · 2025-11-03T21:17:19Z

testing/internal/e2e/tests/base/target_tcp_connect_redis_test.go

+			require.NoError(t, err)
+			require.Equal(t, c.expectedUsername, output)
+
+			output, err = sendRedisCommand(stdin, stdout, "SET e2etestkey e2etestvalue\r\n")


Maybe we could randomize or name the key that's used here to prevent any potential conflicts? A potential idea could be

t.Name() + timestamp

That way, we could potentially remove the need for that FLUSH.

i like this idea, i will add it 😄

i'd prefer to keep the flush to ensure a clean slate for the next test (and to stick to the principle of isolating these tests from each other). i don't think there is much overhead to doing that here, and in the scenario someone updates this test, it will help ensure the tests don't affect each other.

although if we split the test apart like you suggested in the other comment and spin up redis per test, then we wouldn't need to flush

actually ended up keeping the existing code since we're recreating the redis containers now

moduli · 2025-11-03T21:27:26Z

testing/internal/e2e/tests/base/target_tcp_connect_redis_test.go

+}
+
+// setupRedisContainer starts a Redis container and returns its connection info
+func setupRedisContainer(t *testing.T, ctx context.Context) *redisContainerInfo {


Feels like some of these redis methods might be better suited in another package? One idea is in e2e/infra/redis_docker.go?

i agree, especially if we want to consider splitting the test into separate files. your suggestion makes sense, i'll go ahead and try that

moduli · 2025-11-03T21:30:04Z

testing/internal/e2e/tests/base/target_tcp_connect_redis_test.go

+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			// Setup
+			resources := setupBoundaryCredentialStore(t, ctx, redisInfo.Hostname, redisInfo.Port)


One idea I'm toying with is actually just separating these into two different tests in different files

target_tcp_connect_redis_username_password_test.go target_tcp_connect_redis_password_test.go

That would help with readability of the test and how this file is currently organized. That would make it so you're recreating redis, but docker containers aren't too bad to spin up and down.

louisruch

Great work on this! Please wait for @moduli to approve as well before merging

bgajjala8

LG2M! 😄

moduli

one minor comment, but looks good

moduli · 2025-11-05T19:01:32Z

testing/internal/e2e/tests/base/target_tcp_connect_redis_username_password_test.go


-// TestCliTcpTargetConnectRedis uses the boundary cli to connect to a target using `connect redis`
-func TestCliTcpTargetConnectRedis(t *testing.T) {
+func TestCliTcpTargetConnectRedisUsernamePassword(t *testing.T) {


nit: perhaps a comment describing the intent of the test?

// TestCliTcpTargetConnectRedisUsernamePassword uses the boundary cli to connect to a target using `connect redis`. The target is configured to use a username/password credential... etc. etc.

same with the other test

will add descriptions, thanks for catching this

Moving password changes as a new migration

laero requested review from bgajjala8, hugoghx and louisruch October 29, 2025 21:43

laero requested a review from a team as a code owner October 29, 2025 21:43

github-actions bot added api core labels Oct 29, 2025

laero force-pushed the ICU-17729-password-credential-redis-helper branch from 317c4cf to 997c3ce Compare October 29, 2025 21:54

louisruch reviewed Oct 30, 2025

View reviewed changes

api/proxy/credentials.go Outdated Show resolved Hide resolved

api/proxy/credentials.go Show resolved Hide resolved

laero force-pushed the ICU-17729-password-credential-redis-helper branch 2 times, most recently from 8f10b67 to 24e2d38 Compare October 30, 2025 22:00

laero requested a review from louisruch October 31, 2025 14:57

bgajjala8 reviewed Oct 31, 2025

View reviewed changes

testing/internal/e2e/tests/base/target_tcp_connect_redis_test.go Outdated Show resolved Hide resolved

laero force-pushed the ICU-17729-password-credential-redis-helper branch 3 times, most recently from 3b9ffeb to d7d5258 Compare November 1, 2025 18:51

laero requested a review from bgajjala8 November 3, 2025 14:56

bgajjala8 reviewed Nov 3, 2025

View reviewed changes

testing/internal/e2e/tests/base/helpers_test.go Outdated Show resolved Hide resolved

bgajjala8 requested a review from moduli November 3, 2025 18:37

laero commented Nov 3, 2025

View reviewed changes

testing/internal/e2e/tests/base/helpers_test.go Outdated Show resolved Hide resolved

laero commented Nov 3, 2025

View reviewed changes

testing/internal/e2e/tests/base/helpers_test.go Outdated Show resolved Hide resolved

moduli reviewed Nov 3, 2025

View reviewed changes

laero force-pushed the ICU-17729-password-credential-redis-helper branch 2 times, most recently from ad72114 to 45054ca Compare November 4, 2025 20:09

louisruch approved these changes Nov 5, 2025

View reviewed changes

laero requested review from bgajjala8 and moduli November 5, 2025 17:07

bgajjala8 approved these changes Nov 5, 2025

View reviewed changes

laero force-pushed the ICU-17729-password-credential-redis-helper branch from 45054ca to 779e378 Compare November 5, 2025 18:21

moduli approved these changes Nov 5, 2025

View reviewed changes

feat(cli): Add support for password credential in redis helper

2b4a102

laero force-pushed the ICU-17729-password-credential-redis-helper branch from 779e378 to 2b4a102 Compare November 5, 2025 19:26

laero merged commit 071b262 into llb-password-credential-type Nov 5, 2025
56 of 57 checks passed

laero deleted the ICU-17729-password-credential-redis-helper branch November 5, 2025 20:46

bgajjala8 pushed a commit that referenced this pull request Nov 5, 2025

feat(cli): Add support for password credential in redis helper (#6213)

d4735f9

bgajjala8 pushed a commit that referenced this pull request Nov 5, 2025

feat(cli): Add support for password credential in redis helper (#6213)

9882007

bgajjala8 pushed a commit that referenced this pull request Nov 5, 2025

feat(cli): Add support for password credential in redis helper (#6213)

0443a52

bgajjala8 pushed a commit that referenced this pull request Nov 5, 2025

feat(cli): Add support for password credential in redis helper (#6213)

38df8f5

bgajjala8 pushed a commit that referenced this pull request Nov 11, 2025

feat(cli): Add support for password credential in redis helper (#6213)

b024f5d

Moving password changes as a new migration

bgajjala8 pushed a commit that referenced this pull request Nov 11, 2025

feat(cli): Add support for password credential in redis helper (#6213)

5c06365

Moving password changes as a new migration

bgajjala8 pushed a commit that referenced this pull request Nov 12, 2025

feat(cli): Add support for password credential in redis helper (#6213)

d20d09f

bgajjala8 pushed a commit that referenced this pull request Nov 13, 2025

feat(cli): Add support for password credential in redis helper (#6213)

1ac444d

feat(cli): Add support for password credential in redis helper #6213

feat(cli): Add support for password credential in redis helper #6213

Uh oh!

Conversation

laero commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Sample CLI Usage

Uh oh!

louisruch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bgajjala8 Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laero Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laero Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laero Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laero Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laero Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laero Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisruch left a comment

Choose a reason for hiding this comment

Uh oh!

bgajjala8 left a comment

Choose a reason for hiding this comment

Uh oh!

moduli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laero commented Oct 29, 2025 •

edited

Loading

louisruch left a comment •

edited

Loading

bgajjala8 Nov 3, 2025 •

edited

Loading

laero Nov 3, 2025 •

edited

Loading

laero Nov 3, 2025 •

edited

Loading

laero Nov 3, 2025 •

edited

Loading

laero Nov 3, 2025 •

edited

Loading

laero Nov 4, 2025 •

edited

Loading

laero Nov 3, 2025 •

edited

Loading