diff --git a/internal/credential/vault/private_library.go b/internal/credential/vault/private_library.go index a161f4ce1f..8b99947d52 100644 --- a/internal/credential/vault/private_library.go +++ b/internal/credential/vault/private_library.go @@ -20,6 +20,7 @@ import ( "github.com/hashicorp/boundary/internal/credential" "github.com/hashicorp/boundary/internal/credential/vault/internal/sshprivatekey" "github.com/hashicorp/boundary/internal/credential/vault/internal/usernamepassword" + "github.com/hashicorp/boundary/internal/db/sentinel" "github.com/hashicorp/boundary/internal/db/timestamp" "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/kms" @@ -46,6 +47,7 @@ func (bc *baseCred) Library() credential.Library { return bc.lib } func (bc *baseCred) Purpose() credential.Purpose { return bc.lib.GetPurpose() } func (bc *baseCred) getExpiration() time.Duration { return bc.expiration } func (bc *baseCred) getCredential() *Credential { return bc.Credential } +func (bc *baseCred) isRevokable() bool { return bc.ExternalId != sentinel.ExternalIdNone } // convert converts bc to a specific credential type if bc is not // UnspecifiedType. @@ -289,6 +291,7 @@ type dynamicCred interface { credential.Dynamic getExpiration() time.Duration getCredential() *Credential + isRevokable() bool } // retrieveCredential retrieves a dynamic credential from Vault for the diff --git a/internal/credential/vault/repository_credentials.go b/internal/credential/vault/repository_credentials.go index ab715847e6..f84a824d6f 100644 --- a/internal/credential/vault/repository_credentials.go +++ b/internal/credential/vault/repository_credentials.go @@ -75,6 +75,12 @@ func (r *Repository) Issue(ctx context.Context, sessionId string, requests []cre return nil, err } + creds = append(creds, cred) + if !cred.isRevokable() { + // No need to persist since the credential cannot be revoked nor renewed + continue + } + if cred.getExpiration() < runJobsInterval { event.WriteError(ctx, op, fmt.Errorf("WARNING: credential will expire before job scheduler can run"), @@ -123,8 +129,6 @@ func (r *Repository) Issue(ctx context.Context, sessionId string, requests []cre ); err != nil { return nil, errors.Wrap(ctx, err, op) } - - creds = append(creds, cred) } // Best effort update next run time of credential renewal job, but an error should not diff --git a/internal/credential/vault/repository_credentials_test.go b/internal/credential/vault/repository_credentials_test.go index 0d66bf1b9c..5fe236f4df 100644 --- a/internal/credential/vault/repository_credentials_test.go +++ b/internal/credential/vault/repository_credentials_test.go @@ -23,7 +23,7 @@ import ( ) // since we're not using gorm tags to retrieve this, it's faster and easier to just make a new struct than import usrPassCred from the vault package -type userPassCred struct { +type revokableCred struct { PublicId string LibraryId string SessionId string @@ -35,7 +35,7 @@ type userPassCred struct { ExpirationTime *timestamp.Timestamp } -func lookupDbCred(t *testing.T, ctx context.Context, rw *db.Db, upc credential.UsernamePassword) userPassCred { +func lookupDbCred(t *testing.T, ctx context.Context, rw *db.Db, dc credential.Dynamic) *revokableCred { rows, err := rw.Query(ctx, ` select public_id, @@ -49,11 +49,11 @@ func lookupDbCred(t *testing.T, ctx context.Context, rw *db.Db, upc credential.U expiration_time from credential_vault_credential where public_id = ?; - `, []any{upc.GetPublicId()}) + `, []any{dc.GetPublicId()}) require.NoError(t, err) rowCount := 0 - got := userPassCred{} + got := revokableCred{} for rows.Next() { rowCount++ @@ -69,9 +69,46 @@ func lookupDbCred(t *testing.T, ctx context.Context, rw *db.Db, upc credential.U &got.ExpirationTime, )) } - assert.Equal(t, 1, rowCount) + // Should never get more than one that matches, but can get 0 + assert.LessOrEqual(t, rowCount, 1) - return got + if rowCount == 0 { + return nil + } + + return &got +} + +type libT int + +const ( + libDB libT = iota + libUsrPassDB + libErrUsrPassDB + libPKI + libErrPKI + libKV + libErrKV + libUsrPassKV + libSshPkKV + libExpiredToken +) + +type testLib struct { + PublicId string + HasLease bool + IsRenewable bool +} + +type testLibMap map[libT]testLib + +func (m testLibMap) GetByPublicId(id string) (testLib, bool) { + for _, v := range m { + if v.PublicId == id { + return v, true + } + } + return testLib{}, false } func TestRepository_IssueCredentials(t *testing.T) { @@ -133,21 +170,7 @@ func TestRepository_IssueCredentials(t *testing.T) { assert.Equal(t, 1, num) v.RevokeToken(t, expToken) - type libT int - const ( - libDB libT = iota - libUsrPassDB - libErrUsrPassDB - libPKI - libErrPKI - libKV - libErrKV - libUsrPassKV - libSshPkKV - libExpiredToken - ) - - libs := make(map[libT]string) + libs := make(testLibMap) { libPath := path.Join("database", "creds", "opened") libIn, err := vault.NewCredentialLibrary(origStore.GetPublicId(), libPath) @@ -156,7 +179,7 @@ func TestRepository_IssueCredentials(t *testing.T) { lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) assert.NoError(t, err) require.NotNil(t, lib) - libs[libDB] = lib.GetPublicId() + libs[libDB] = testLib{PublicId: lib.GetPublicId(), HasLease: true, IsRenewable: true} } { libPath := path.Join("pki", "issue", "boundary") @@ -166,7 +189,7 @@ func TestRepository_IssueCredentials(t *testing.T) { lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) assert.NoError(t, err) require.NotNil(t, lib) - libs[libPKI] = lib.GetPublicId() + libs[libPKI] = testLib{PublicId: lib.GetPublicId(), HasLease: false, IsRenewable: false} } { @@ -177,7 +200,7 @@ func TestRepository_IssueCredentials(t *testing.T) { lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) assert.NoError(t, err) require.NotNil(t, lib) - libs[libErrPKI] = lib.GetPublicId() + libs[libErrPKI] = testLib{PublicId: lib.GetPublicId(), HasLease: true, IsRenewable: true} } { libPath := path.Join("secret", "data", "my-up-secret") @@ -187,7 +210,7 @@ func TestRepository_IssueCredentials(t *testing.T) { lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) assert.NoError(t, err) require.NotNil(t, lib) - libs[libKV] = lib.GetPublicId() + libs[libKV] = testLib{PublicId: lib.GetPublicId(), HasLease: false, IsRenewable: true} } { libPath := path.Join("secret", "data", "fake-secret") @@ -197,7 +220,7 @@ func TestRepository_IssueCredentials(t *testing.T) { lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) assert.NoError(t, err) require.NotNil(t, lib) - libs[libErrKV] = lib.GetPublicId() + libs[libErrKV] = testLib{PublicId: lib.GetPublicId(), HasLease: false, IsRenewable: true} } { libPath := path.Join("database", "creds", "opened") @@ -210,7 +233,7 @@ func TestRepository_IssueCredentials(t *testing.T) { lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) assert.NoError(t, err) require.NotNil(t, lib) - libs[libUsrPassDB] = lib.GetPublicId() + libs[libUsrPassDB] = testLib{PublicId: lib.GetPublicId(), HasLease: true, IsRenewable: true} } { libPath := path.Join("database", "creds", "opened") @@ -227,7 +250,7 @@ func TestRepository_IssueCredentials(t *testing.T) { lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) assert.NoError(t, err) require.NotNil(t, lib) - libs[libErrUsrPassDB] = lib.GetPublicId() + libs[libErrUsrPassDB] = testLib{PublicId: lib.GetPublicId(), HasLease: true, IsRenewable: true} } { libPath := path.Join("secret", "data", "my-up-secret") @@ -240,7 +263,7 @@ func TestRepository_IssueCredentials(t *testing.T) { lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) assert.NoError(t, err) require.NotNil(t, lib) - libs[libUsrPassKV] = lib.GetPublicId() + libs[libUsrPassKV] = testLib{PublicId: lib.GetPublicId(), HasLease: false} } { libPath := path.Join("secret", "data", "my-sshpk-secret") @@ -253,7 +276,7 @@ func TestRepository_IssueCredentials(t *testing.T) { lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) assert.NoError(t, err) require.NotNil(t, lib) - libs[libSshPkKV] = lib.GetPublicId() + libs[libSshPkKV] = testLib{PublicId: lib.GetPublicId(), HasLease: false} } { libPath := path.Join("secret", "data", "my-up-secret") @@ -266,7 +289,7 @@ func TestRepository_IssueCredentials(t *testing.T) { lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) assert.NoError(t, err) require.NotNil(t, lib) - libs[libExpiredToken] = lib.GetPublicId() + libs[libExpiredToken] = testLib{PublicId: lib.GetPublicId(), HasLease: false} } at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) @@ -303,7 +326,7 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2dc, requests: []credential.Request{ { - SourceId: libs[libDB], + SourceId: libs[libDB].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -313,11 +336,11 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2dc, requests: []credential.Request{ { - SourceId: libs[libDB], + SourceId: libs[libDB].PublicId, Purpose: credential.BrokeredPurpose, }, { - SourceId: libs[libPKI], + SourceId: libs[libPKI].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -327,7 +350,7 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2dc, requests: []credential.Request{ { - SourceId: libs[libErrPKI], + SourceId: libs[libErrPKI].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -338,11 +361,11 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2nil, requests: []credential.Request{ { - SourceId: libs[libDB], + SourceId: libs[libDB].PublicId, Purpose: credential.BrokeredPurpose, }, { - SourceId: libs[libPKI], + SourceId: libs[libPKI].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -353,7 +376,7 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2dc, requests: []credential.Request{ { - SourceId: libs[libKV], + SourceId: libs[libKV].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -363,7 +386,7 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2dc, requests: []credential.Request{ { - SourceId: libs[libUsrPassDB], + SourceId: libs[libUsrPassDB].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -373,7 +396,7 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2dc, requests: []credential.Request{ { - SourceId: libs[libErrKV], + SourceId: libs[libErrKV].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -384,7 +407,7 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2dc, requests: []credential.Request{ { - SourceId: libs[libErrUsrPassDB], + SourceId: libs[libErrUsrPassDB].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -395,7 +418,7 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2dc, requests: []credential.Request{ { - SourceId: libs[libUsrPassKV], + SourceId: libs[libUsrPassKV].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -405,7 +428,7 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2dc, requests: []credential.Request{ { - SourceId: libs[libSshPkKV], + SourceId: libs[libSshPkKV].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -415,7 +438,7 @@ func TestRepository_IssueCredentials(t *testing.T) { convertFn: rc2dc, requests: []credential.Request{ { - SourceId: libs[libExpiredToken], + SourceId: libs[libExpiredToken].PublicId, Purpose: credential.BrokeredPurpose, }, }, @@ -451,11 +474,6 @@ func TestRepository_IssueCredentials(t *testing.T) { if upc, ok := dc.(credential.UsernamePassword); ok { assert.NotEmpty(upc.Username()) assert.NotEmpty(upc.Password()) - // we also want to retrieve the cred from the db and make sure it's the same as the one returned by Issue - retrieved := lookupDbCred(t, ctx, rw, upc) - require.Equal(retrieved.SessionId, sess.GetPublicId()) - require.Equal(retrieved.PublicId, upc.GetPublicId()) - // these asserts are super basic because the main check we want is that lookupDbCred succeeded break } assert.Fail("want UsernamePassword credential from library with credential type UsernamePassword") @@ -464,6 +482,19 @@ func TestRepository_IssueCredentials(t *testing.T) { assert.Fail("do not want UsernamePassword credential from library with credential type Unspecified") } } + if lib, ok := libs.GetByPublicId(dc.Library().GetPublicId()); ok { + retrieved := lookupDbCred(t, ctx, rw, dc) + if lib.HasLease { + // we also want to retrieve the cred from the db and make sure it's the same as the one returned by Issue + require.NotNil(retrieved) + require.Equal(retrieved.SessionId, sess.GetPublicId()) + require.Equal(retrieved.PublicId, dc.GetPublicId()) + assert.NotEmpty(retrieved.ExternalId) + assert.Equal(retrieved.IsRenewable, lib.IsRenewable) + } else { + assert.Nil(retrieved) + } + } } }) }