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

Add rotate-root endpoint #70

Merged
merged 40 commits into from
Oct 28, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
ebce851
Remove bare returns
pcman312 Sep 27, 2021
d6bb52e
Readability cleanup
pcman312 Sep 27, 2021
6d07ce0
Remove errwrap
pcman312 Sep 27, 2021
b6addec
Make tests happy again
pcman312 Sep 27, 2021
ebb0325
Add rotate-root endpoint
pcman312 Oct 4, 2021
41bd7eb
Use correct response value
pcman312 Oct 5, 2021
56227f5
Fix merge failure
pcman312 Oct 5, 2021
59884a2
Add additional AAD warnings; Respond to code review
pcman312 Oct 7, 2021
5916c4b
Fix test
pcman312 Oct 7, 2021
714aff4
Don't pass config as a pointer so it gets a copy
pcman312 Oct 7, 2021
c3615a8
Fix expiration date logic; fix inverted warning logic
pcman312 Oct 8, 2021
5eb9e4c
Minor code review tweaks
pcman312 Oct 13, 2021
4085253
Move expiration to config
pcman312 Oct 13, 2021
938122f
Don't error if there isn't an error
pcman312 Oct 13, 2021
e5cc93a
Update the config & remove old passwords in the WAL
pcman312 Oct 13, 2021
b46951d
Return default_expiration on config get
pcman312 Oct 13, 2021
23fadb9
Return expiration from GET config
pcman312 Oct 13, 2021
8532464
Update path_rotate_root.go
jasonodonnell Oct 18, 2021
bf9c235
Update per review
jasonodonnell Oct 20, 2021
a693813
Rebase
jasonodonnell Oct 20, 2021
77ae610
Fix test
jasonodonnell Oct 20, 2021
9d5d3ae
Revert "Rebase"
jasonodonnell Oct 20, 2021
699e815
Remove named returns
jasonodonnell Oct 20, 2021
d5b7a4b
Update per review
jasonodonnell Oct 20, 2021
5ba6ffc
Update path_config.go
jasonodonnell Oct 20, 2021
33a8e60
Update per review
jasonodonnell Oct 20, 2021
484eb75
Use periodicFunc, change wal
jasonodonnell Oct 25, 2021
17e7da4
Fix config test
jasonodonnell Oct 25, 2021
f76c855
Add expiration date, update logger
jasonodonnell Oct 25, 2021
23e34b4
Fix timer bug
jasonodonnell Oct 26, 2021
a3aae79
Change root expiration to timestamp
jasonodonnell Oct 26, 2021
7c9842f
Fix named returns
jasonodonnell Oct 26, 2021
8571e52
Update backend.go
jasonodonnell Oct 27, 2021
7ffdfd4
Update per feedback, add more tests
jasonodonnell Oct 27, 2021
d5eb293
Fix conflicts
jasonodonnell Oct 27, 2021
de93161
Add wal min age
jasonodonnell Oct 27, 2021
5ac5c26
Update mock
jasonodonnell Oct 27, 2021
ac58246
Update go version
jasonodonnell Oct 27, 2021
495d8d8
Revert "Update go version"
jasonodonnell Oct 27, 2021
7236969
Remove unused wal code
jasonodonnell Oct 28, 2021
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
Prev Previous commit
Next Next commit
Update per feedback, add more tests
  • Loading branch information
jasonodonnell committed Oct 27, 2021
commit 7ffdfd4fa5bd4ad7aa0ad4e46868d6e0f7a5540d
24 changes: 10 additions & 14 deletions backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,9 @@ func backend() *azureSecretBackend {
secretServicePrincipal(&b),
secretStaticServicePrincipal(&b),
},
BackendType: logical.TypeLogical,
Invalidate: b.invalidate,

WALRollback: b.walRollback,

// Role assignment can take up to a few minutes, so ensure we don't try
// to roll back during creation.
WALRollbackMinAge: 10 * time.Minute,

BackendType: logical.TypeLogical,
Invalidate: b.invalidate,
WALRollback: b.walRollback,
PeriodicFunc: b.periodicFunc,
}
b.getProvider = newAzureProvider
Expand All @@ -89,7 +83,7 @@ func (b *azureSecretBackend) periodicFunc(ctx context.Context, sys *logical.Requ
}

// Password should be at least a minute old before we process it
if config.NewClientSecret == "" || (time.Since(config.NewClientSecretCreated) > time.Minute) {
if config.NewClientSecret == "" || (time.Since(config.NewClientSecretCreated) < time.Minute) {
return nil
}

Expand Down Expand Up @@ -120,10 +114,12 @@ func (b *azureSecretBackend) periodicFunc(ctx context.Context, sys *logical.Requ
}
}

b.Logger().Debug("periodic func", "rotate-root", "removing old passwords from Azure")
err = removeApplicationPasswords(ctx, client.provider, *app.ID, credsToDelete...)
if err != nil {
return err
if len(credsToDelete) != 0 {
b.Logger().Debug("periodic func", "rotate-root", "removing old passwords from Azure")
err = removeApplicationPasswords(ctx, client.provider, *app.ID, credsToDelete...)
if err != nil {
return err
}
}

b.Logger().Debug("periodic func", "rotate-root", "updating config with new password")
Expand Down
2 changes: 1 addition & 1 deletion path_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ type azureConfig struct {
PasswordPolicy string `json:"password_policy"`
UseMsGraphAPI bool `json:"use_microsoft_graph_api"`
RootPasswordTTL time.Duration `json:"root_password_ttl"`
RootPasswordExpirationDate time.Time `json:"root_password_expiration_date`
RootPasswordExpirationDate time.Time `json:"root_password_expiration_date"`
}

func pathConfig(b *azureSecretBackend) *framework.Path {
Expand Down
13 changes: 6 additions & 7 deletions path_rotate_root.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,7 @@ func (b *azureSecretBackend) pathRotateRoot(ctx context.Context, req *logical.Re
return nil, fmt.Errorf("failed to add new password: %w", err)
}

wal := walRotateRoot{
OldPassword: config.ClientSecret,
OldPasswordKeyID: config.ClientSecretKeyID,
OldPasswordExpirationDate: config.RootPasswordExpirationDate,
}

var wal walRotateRoot
walID, walErr := framework.PutWAL(ctx, req.Storage, walRotateRootCreds, wal)
if walErr != nil {
err = client.provider.RemoveApplicationPassword(ctx, *app.ID, *newPasswordResp.PasswordCredential.KeyID)
Expand All @@ -99,7 +94,11 @@ func (b *azureSecretBackend) pathRotateRoot(ctx context.Context, req *logical.Re
b.updatePassword = true
Copy link
Contributor

@austingebauer austingebauer Oct 27, 2021

Choose a reason for hiding this comment

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

I haven't totally wrapped by head around this bool, but something about it is giving me a bit of pause. I wonder if having this bool state will play well with concurrent modification (rotate-root handler, periodFunc+walRollbackFunc). I also wonder if a plugin/vault restart could reset this in a way that would produce unexpected results. Not blocking, but just wanted to share my thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

The primary use case of this bool is to give us a short circuit in the periodic function to avoid needing to read the config from storage when no password updates are needed. I do agree this is a little leaky, but only the periodic function will flip it to false and if set to true, the periodic function will double check the config from storage before proceeding.


err = framework.DeleteWAL(ctx, req.Storage, walID)
return addAADWarning(&logical.Response{}, config), err
if err != nil {
b.Logger().Error("rotate root", "delete wal", err)
}

return addAADWarning(&logical.Response{}, config), nil
}

type passwordRemover interface {
Expand Down
81 changes: 80 additions & 1 deletion path_rotate_root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ import (
"fmt"
"reflect"
"testing"
"time"

"github.com/hashicorp/vault/sdk/logical"
)

func TestRotateRoot(t *testing.T) {
func TestRotateRootSuccess(t *testing.T) {
b, s := getTestBackend(t, true)

resp, err := b.HandleRequest(context.Background(), &logical.Request{
Expand Down Expand Up @@ -52,6 +53,12 @@ func TestRotateRoot(t *testing.T) {
t.Fatal("update password is false, it shouldn't be")
}

config.NewClientSecretCreated = config.NewClientSecretCreated.Add(-(time.Minute * 1))
err = b.saveConfig(context.Background(), config, s)
if err != nil {
t.Fatal(err)
}

err = b.periodicFunc(context.Background(), &logical.Request{
Storage: s,
})
Expand All @@ -70,6 +77,78 @@ func TestRotateRoot(t *testing.T) {
}
}

func TestRotateRootPeroidicFunctionBeforeMinute(t *testing.T) {
b, s := getTestBackend(t, true)

resp, err := b.HandleRequest(context.Background(), &logical.Request{
Operation: logical.UpdateOperation,
Path: "rotate-root",
Data: map[string]interface{}{},
Storage: s,
})

if err != nil {
t.Fatal(err)
}

if resp != nil && resp.IsError() {
t.Fatal(resp.Error())
}

tests := []struct {
Name string
Created time.Duration
}{
{
Name: "1 second test:",
Created: time.Second * 1,
},
{
Name: "5 seconds test:",
Created: time.Second * 5,
},
{
Name: "30 seconds test:",
Created: time.Second * 30,
},
{
Name: "50 seconds test:",
Created: time.Second * 50,
},
}

for _, test := range tests {
t.Log(test.Name)
config, err := b.getConfig(context.Background(), s)
if err != nil {
t.Fatal(err)
}

config.NewClientSecretCreated = time.Now().Add(-(test.Created))
err = b.saveConfig(context.Background(), config, s)
if err != nil {
t.Fatal(test.Name, err)
}

err = b.periodicFunc(context.Background(), &logical.Request{
Storage: s,
})

if err != nil {
t.Fatal(test.Name, err)
}

newConfig, err := b.getConfig(context.Background(), s)
if err != nil {
t.Fatal(test.Name, err)
}

if newConfig.ClientSecret == config.NewClientSecret {
t.Fatal(test.Name, fmt.Errorf("old and new password are equal after periodic function, they shouldn't be"))
}
}
}

func TestIntersectStrings(t *testing.T) {
type testCase struct {
a []string
Expand Down
11 changes: 2 additions & 9 deletions wal.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,7 @@ func (b *azureSecretBackend) rollbackAppWAL(ctx context.Context, req *logical.Re
return nil
}

type walRotateRoot struct {
OldPassword string
OldPasswordKeyID string
OldPasswordExpirationDate time.Time
}
type walRotateRoot struct{}

func (b *azureSecretBackend) rollbackRootWAL(ctx context.Context, req *logical.Request, data interface{}) error {
// Decode the WAL data
Expand All @@ -93,15 +89,12 @@ func (b *azureSecretBackend) rollbackRootWAL(ctx context.Context, req *logical.R
return err
}

b.Logger().Debug("rolling back root password in storage")
b.Logger().Debug("rolling back config")
config, err := b.getConfig(ctx, req.Storage)
if err != nil {
return err
}

config.ClientID = entry.OldPasswordKeyID
config.ClientSecret = entry.OldPassword
config.RootPasswordExpirationDate = entry.OldPasswordExpirationDate
config.NewClientSecret = ""
config.NewClientSecretCreated = time.Time{}
config.NewClientSecretExpirationDate = time.Time{}
Expand Down