From 7490f7410a5cf47c97bd6ab80cc05f652f5fe2be Mon Sep 17 00:00:00 2001 From: Hamid Ghaf <83242695+hghaf099@users.noreply.github.com> Date: Tue, 31 May 2022 18:22:04 +0000 Subject: [PATCH] backport of commit 1bcbf4826d5a5de914150af712686d503c176447 --- changelog/15482.txt | 3 +++ vault/external_tests/mfa/login_mfa_test.go | 6 +++++ vault/identity_store.go | 8 +++---- vault/login_mfa.go | 26 +++++++++++++++++++--- 4 files changed, 36 insertions(+), 7 deletions(-) create mode 100644 changelog/15482.txt diff --git a/changelog/15482.txt b/changelog/15482.txt new file mode 100644 index 000000000000..0dcfd6f34335 --- /dev/null +++ b/changelog/15482.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth: Prevent deleting a valid MFA method ID using the endpoint for a different MFA method type +``` diff --git a/vault/external_tests/mfa/login_mfa_test.go b/vault/external_tests/mfa/login_mfa_test.go index 427818640277..8a2bdb5b2145 100644 --- a/vault/external_tests/mfa/login_mfa_test.go +++ b/vault/external_tests/mfa/login_mfa_test.go @@ -187,6 +187,12 @@ func TestLoginMFA_Method_CRUD(t *testing.T) { t.Fatal("expected response id to match existing method id but it didn't") } + // delete with invalid path should fail + _, err = client.Logical().Delete(invalidPath) + if err == nil { + t.Fatal("expected deleting an MFA method ID with invalid path to fail") + } + // delete it _, err = client.Logical().Delete(myNewPath) if err != nil { diff --git a/vault/identity_store.go b/vault/identity_store.go index a1cb742d95cd..56ec0ef4a5d2 100644 --- a/vault/identity_store.go +++ b/vault/identity_store.go @@ -221,7 +221,7 @@ func mfaPaths(i *IdentityStore) []*framework.Path { Summary: "Update or create a configuration for the given MFA method", }, logical.DeleteOperation: &framework.PathOperation{ - Callback: i.handleMFAMethodDelete, + Callback: i.handleMFAMethodTOTPDelete, Summary: "Delete a configuration for the given MFA method", }, }, @@ -335,7 +335,7 @@ func mfaPaths(i *IdentityStore) []*framework.Path { Summary: "Update or create a configuration for the given MFA method", }, logical.DeleteOperation: &framework.PathOperation{ - Callback: i.handleMFAMethodDelete, + Callback: i.handleMFAMethodOKTADelete, Summary: "Delete a configuration for the given MFA method", }, }, @@ -391,7 +391,7 @@ func mfaPaths(i *IdentityStore) []*framework.Path { Summary: "Update or create a configuration for the given MFA method", }, logical.DeleteOperation: &framework.PathOperation{ - Callback: i.handleMFAMethodDelete, + Callback: i.handleMFAMethodDUODelete, Summary: "Delete a configuration for the given MFA method", }, }, @@ -431,7 +431,7 @@ func mfaPaths(i *IdentityStore) []*framework.Path { Summary: "Update or create a configuration for the given MFA method", }, logical.DeleteOperation: &framework.PathOperation{ - Callback: i.handleMFAMethodDelete, + Callback: i.handleMFAMethodPingIDDelete, Summary: "Delete a configuration for the given MFA method", }, }, diff --git a/vault/login_mfa.go b/vault/login_mfa.go index bf4447cc8de4..5e05b5736111 100644 --- a/vault/login_mfa.go +++ b/vault/login_mfa.go @@ -383,12 +383,28 @@ func (i *IdentityStore) handleMFAMethodPingIDUpdate(ctx context.Context, req *lo return i.handleMFAMethodUpdateCommon(ctx, req, d, mfaMethodTypePingID) } -func (i *IdentityStore) handleMFAMethodDelete(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { +func (i *IdentityStore) handleMFAMethodTOTPDelete(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { + return i.handleMFAMethodDeleteCommon(ctx, req, d, mfaMethodTypeTOTP) +} + +func (i *IdentityStore) handleMFAMethodOKTADelete(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { + return i.handleMFAMethodDeleteCommon(ctx, req, d, mfaMethodTypeOkta) +} + +func (i *IdentityStore) handleMFAMethodDUODelete(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { + return i.handleMFAMethodDeleteCommon(ctx, req, d, mfaMethodTypeDuo) +} + +func (i *IdentityStore) handleMFAMethodPingIDDelete(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { + return i.handleMFAMethodDeleteCommon(ctx, req, d, mfaMethodTypePingID) +} + +func (i *IdentityStore) handleMFAMethodDeleteCommon(ctx context.Context, req *logical.Request, d *framework.FieldData, methodType string) (*logical.Response, error) { methodID := d.Get("method_id").(string) if methodID == "" { return logical.ErrorResponse("missing method ID"), nil } - return nil, i.mfaBackend.deleteMFAConfigByMethodID(ctx, methodID, memDBLoginMFAConfigsTable, loginMFAConfigPrefix) + return nil, i.mfaBackend.deleteMFAConfigByMethodID(ctx, methodID, methodType, memDBLoginMFAConfigsTable, loginMFAConfigPrefix) } func (i *IdentityStore) handleLoginMFAGenerateUpdate(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { @@ -2552,7 +2568,7 @@ func (b *LoginMFABackend) MemDBDeleteMFALoginEnforcementConfigByNameAndNamespace return nil } -func (b *LoginMFABackend) deleteMFAConfigByMethodID(ctx context.Context, configID, tableName, prefix string) error { +func (b *LoginMFABackend) deleteMFAConfigByMethodID(ctx context.Context, configID, methodType, tableName, prefix string) error { var err error if configID == "" { @@ -2594,6 +2610,10 @@ func (b *LoginMFABackend) deleteMFAConfigByMethodID(ctx context.Context, configI return nil } + if mConfig.Type != methodType { + return fmt.Errorf("method type does not match the MFA config type") + } + mfaNs, err := b.Core.NamespaceByID(ctx, mConfig.NamespaceID) if err != nil { return err