backport of commit 1bcbf48 (#15706)

Co-authored-by: Hamid Ghaf <83242695+hghaf099@users.noreply.github.com>
hashicorp · May 31, 2022 · bed62c5 · bed62c5
1 parent f3f1262
commit bed62c5
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 7 deletions.
diff --git a/changelog/15482.txt 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
@@ -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
@@ -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
@@ -384,12 +384,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) {
@@ -2559,7 +2575,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 == "" {
@@ -2601,6 +2617,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