fix(sqs,iam): stop two data-destruction paths (bug-hunt) (#2275)

vieiralucas · web-flow · commit 39936ea0ca28 · 2026-07-15T12:27:24.000-03:00
diff --git a/crates/fakecloud-e2e/tests/iam.rs b/crates/fakecloud-e2e/tests/iam.rs
@@ -565,6 +565,100 @@ async fn iam_group_lifecycle() {
     assert!(list.groups().is_empty());
 }
 
+/// Regression: DeleteGroup on a group that still has members must return a
+/// 409 DeleteConflict, NOT silently destroy the group and its membership.
+#[tokio::test]
+async fn iam_delete_nonempty_group_conflicts() {
+    let server = TestServer::start().await;
+    let client = server.iam_client().await;
+
+    client
+        .create_group()
+        .group_name("payroll")
+        .send()
+        .await
+        .unwrap();
+    client
+        .create_user()
+        .user_name("carol")
+        .send()
+        .await
+        .unwrap();
+    client
+        .add_user_to_group()
+        .group_name("payroll")
+        .user_name("carol")
+        .send()
+        .await
+        .unwrap();
+
+    // Member present -> DeleteConflict, group preserved.
+    let err = client
+        .delete_group()
+        .group_name("payroll")
+        .send()
+        .await
+        .expect_err("deleting a non-empty group must fail");
+    assert!(
+        format!("{err:?}").contains("DeleteConflict"),
+        "expected DeleteConflict, got: {err:?}"
+    );
+    assert_eq!(
+        client.list_groups().send().await.unwrap().groups().len(),
+        1,
+        "group must still exist after a rejected delete"
+    );
+
+    // Also blocked by an inline policy after the member is removed.
+    client
+        .remove_user_from_group()
+        .group_name("payroll")
+        .user_name("carol")
+        .send()
+        .await
+        .unwrap();
+    client
+        .put_group_policy()
+        .group_name("payroll")
+        .policy_name("inline")
+        .policy_document(r#"{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:GetObject","Resource":"*"}]}"#)
+        .send()
+        .await
+        .unwrap();
+    let err = client
+        .delete_group()
+        .group_name("payroll")
+        .send()
+        .await
+        .expect_err("deleting a group with an inline policy must fail");
+    assert!(
+        format!("{err:?}").contains("DeleteConflict"),
+        "expected DeleteConflict for inline policy, got: {err:?}"
+    );
+
+    // Clear the policy -> delete succeeds.
+    client
+        .delete_group_policy()
+        .group_name("payroll")
+        .policy_name("inline")
+        .send()
+        .await
+        .unwrap();
+    client
+        .delete_group()
+        .group_name("payroll")
+        .send()
+        .await
+        .unwrap();
+    assert!(client
+        .list_groups()
+        .send()
+        .await
+        .unwrap()
+        .groups()
+        .is_empty());
+}
+
 // ---- IAM Instance Profile Tests ----
 
 #[tokio::test]
diff --git a/crates/fakecloud-e2e/tests/sqs_kms.rs b/crates/fakecloud-e2e/tests/sqs_kms.rs
@@ -78,6 +78,70 @@ async fn sqs_send_receive_encrypted_round_trips_through_kms() {
     );
 }
 
+/// Regression: ReceiveMessage against an SSE-KMS queue whose key became
+/// unusable (disabled) must return an error WITHOUT destroying the batch.
+/// Previously the messages were popped out of the queue before the decrypt
+/// `?` unwound, permanently losing them. Real SQS leaves them visible.
+#[tokio::test]
+async fn sqs_receive_decrypt_failure_does_not_destroy_messages() {
+    let server = TestServer::start().await;
+    let sqs = server.sqs_client().await;
+    let kms = server.kms_client().await;
+
+    // Customer-managed key we can disable (managed alias/aws/sqs can't be).
+    let key = kms.create_key().send().await.unwrap();
+    let key_id = key.key_metadata().unwrap().key_id().to_string();
+
+    let queue = sqs
+        .create_queue()
+        .queue_name("decrypt-fail")
+        .attributes(
+            aws_sdk_sqs::types::QueueAttributeName::KmsMasterKeyId,
+            &key_id,
+        )
+        .send()
+        .await
+        .unwrap();
+    let queue_url = queue.queue_url().unwrap().to_string();
+
+    sqs.send_message()
+        .queue_url(&queue_url)
+        .message_body("do-not-lose-me")
+        .send()
+        .await
+        .unwrap();
+
+    // Disable the key: the next receive must fail to decrypt.
+    kms.disable_key().key_id(&key_id).send().await.unwrap();
+    let failed = sqs
+        .receive_message()
+        .queue_url(&queue_url)
+        .max_number_of_messages(1)
+        .send()
+        .await;
+    assert!(
+        failed.is_err(),
+        "ReceiveMessage must error when the KMS key is disabled, got: {failed:?}"
+    );
+
+    // Re-enable the key: the message must still be there (not destroyed).
+    kms.enable_key().key_id(&key_id).send().await.unwrap();
+    let recovered = sqs
+        .receive_message()
+        .queue_url(&queue_url)
+        .max_number_of_messages(1)
+        .send()
+        .await
+        .unwrap();
+    let msgs = recovered.messages();
+    assert_eq!(
+        msgs.len(),
+        1,
+        "message must survive a decrypt failure and be receivable after the key is re-enabled"
+    );
+    assert_eq!(msgs[0].body(), Some("do-not-lose-me"));
+}
+
 #[tokio::test]
 async fn sqs_unencrypted_queue_does_not_record_kms_usage() {
     use aws_sdk_sqs::types::QueueAttributeName;
diff --git a/crates/fakecloud-iam/src/iam_service/groups.rs b/crates/fakecloud-iam/src/iam_service/groups.rs
@@ -199,14 +199,46 @@ impl IamService {
         let mut accounts = self.state.write();
         let state = accounts.get_or_create(&req.account_id);
 
-        if state.groups.remove(&group_name).is_none() {
+        // AWS rejects DeleteGroup with a 409 DeleteConflict while the group
+        // still has members or attached/inline policies — it does NOT silently
+        // destroy the group and its membership. Guard before removing.
+        let (has_members, has_attached, has_inline) = {
+            let group = state.groups.get(&group_name).ok_or_else(|| {
+                AwsServiceError::aws_error(
+                    StatusCode::NOT_FOUND,
+                    "NoSuchEntity",
+                    format!("The group with name {group_name} cannot be found."),
+                )
+            })?;
+            (
+                !group.members.is_empty(),
+                !group.attached_policies.is_empty(),
+                !group.inline_policies.is_empty(),
+            )
+        };
+        if has_members {
             return Err(AwsServiceError::aws_error(
-                StatusCode::NOT_FOUND,
-                "NoSuchEntity",
-                format!("The group with name {group_name} cannot be found."),
+                StatusCode::CONFLICT,
+                "DeleteConflict",
+                "Cannot delete entity, must remove users from the group first.".to_string(),
+            ));
+        }
+        if has_attached {
+            return Err(AwsServiceError::aws_error(
+                StatusCode::CONFLICT,
+                "DeleteConflict",
+                "Cannot delete entity, must detach all policies first.".to_string(),
+            ));
+        }
+        if has_inline {
+            return Err(AwsServiceError::aws_error(
+                StatusCode::CONFLICT,
+                "DeleteConflict",
+                "Cannot delete entity, must delete policies first.".to_string(),
             ));
         }
 
+        state.groups.remove(&group_name);
         let xml = empty_response("DeleteGroup", &req.request_id);
         Ok(AwsResponse::xml(StatusCode::OK, xml))
     }
diff --git a/crates/fakecloud-kms/src/hook.rs b/crates/fakecloud-kms/src/hook.rs
@@ -76,13 +76,18 @@ pub enum KmsHookError {
     /// Ciphertext envelope is malformed or signed by a key that no
     /// longer exists.
     InvalidCiphertext(String),
+    /// The key resolves but is disabled / pending deletion, so it can't
+    /// be used for a cryptographic operation (mirrors real KMS, which
+    /// fails Decrypt with `DisabledException` / `KMSInvalidStateException`).
+    KeyDisabled(String),
 }
 
 impl std::fmt::Display for KmsHookError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             Self::KeyNotFound(k) => write!(f, "kms key not found: {k}"),
             Self::InvalidCiphertext(msg) => write!(f, "invalid ciphertext: {msg}"),
+            Self::KeyDisabled(k) => write!(f, "kms key is disabled: {k}"),
         }
     }
 }
@@ -181,11 +186,17 @@ impl KmsServiceHook {
             let state = mas
                 .get(account_id)
                 .ok_or_else(|| KmsHookError::KeyNotFound(key_short.clone()))?;
-            state
+            let key = state
                 .keys
                 .get(&key_short)
-                .map(|k| k.arn.clone())
-                .ok_or_else(|| KmsHookError::KeyNotFound(key_short.clone()))?
+                .ok_or_else(|| KmsHookError::KeyNotFound(key_short.clone()))?;
+            // A disabled / pending-deletion key can't decrypt — real KMS
+            // rejects the operation, and SSE-KMS consumers (SQS/SNS/...) must
+            // surface that rather than returning stale ciphertext as plaintext.
+            if !key.enabled {
+                return Err(KmsHookError::KeyDisabled(key_short.clone()));
+            }
+            key.arn.clone()
         };
 
         self.usage.write().push(KmsUsageRecord {
diff --git a/crates/fakecloud-sqs/src/service/messages.rs b/crates/fakecloud-sqs/src/service/messages.rs
@@ -737,9 +737,41 @@ impl SqsService {
         // already consumed even though the caller saw an error.
         let mut plaintext_bodies: Vec<String> = Vec::with_capacity(received.len());
         if kms_key_id.is_some() && self.kms_hook.is_some() {
+            let mut decrypt_err = None;
             for msg in received.iter() {
-                let plaintext = self.decrypt_message_body(account_id, &queue_arn, &msg.body)?;
-                plaintext_bodies.push(plaintext);
+                match self.decrypt_message_body(account_id, &queue_arn, &msg.body) {
+                    Ok(plaintext) => plaintext_bodies.push(plaintext),
+                    Err(e) => {
+                        decrypt_err = Some(e);
+                        break;
+                    }
+                }
+            }
+            // A decrypt failure (KMS key disabled / pending-deletion / deleted)
+            // must NOT destroy the batch: the messages were already popped out
+            // of `queue.messages` but not yet pushed onto `inflight`, so an
+            // early `?` here would drop them permanently. Real SQS leaves the
+            // messages visible and returns an error. Restore the popped
+            // messages to the front of the queue (preserving order), undo the
+            // receive-count bump and the receipt handle we minted, then error.
+            if let Some(e) = decrypt_err {
+                let restored: Vec<SqsMessage> = received
+                    .drain(..)
+                    .map(|mut m| {
+                        m.visible_at = None;
+                        m.receive_count = m.receive_count.saturating_sub(1);
+                        if let Some(handle) = m.receipt_handle.take() {
+                            if let Some(list) = queue.receipt_handle_map.get_mut(&m.message_id) {
+                                list.retain(|h| h != &handle);
+                            }
+                        }
+                        m
+                    })
+                    .collect();
+                for m in restored.into_iter().rev() {
+                    queue.messages.push_front(m);
+                }
+                return Err(e);
             }
         }
 
