From 21ee835b72737d8ea88ff390acbeb43edafadb12 Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Sat, 4 Jul 2026 01:35:44 -0300 Subject: [PATCH] fix(transfer): 8 behavioral fixes from post-merge bug-hunt - ResourceExistsException now returns HTTP 409 (per model httpError), not 400 - CreateServer defaults Protocols to ["SFTP"] so DescribeServer never omits it - ImportHostKey derives Type (ssh-ed25519/ecdsa-sha2-nistp*/ssh-rsa) from the key body - DeleteServer now removes the tag entries of every cascaded child resource - UpdateServer ignores the create-only Domain member (update-specific field list) - ListTagsForResource honours MaxResults/NextToken (model has NextToken) - CreateServer folds insert + tag storage into a single write lock - ImportCertificate parses the PEM to populate Serial/NotBeforeDate/NotAfterDate Adds a regression test per fix; conformance stays 71/71 ops, 2756/2756 variants. --- Cargo.lock | 1 + crates/fakecloud-transfer/Cargo.toml | 1 + crates/fakecloud-transfer/src/service.rs | 131 ++++++++-- .../fakecloud-transfer/tests/service_tests.rs | 244 ++++++++++++++++++ 4 files changed, 361 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a88f9e420..f245c1c2a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5207,6 +5207,7 @@ dependencies = [ "tokio", "tracing", "uuid", + "x509-cert", ] [[package]] diff --git a/crates/fakecloud-transfer/Cargo.toml b/crates/fakecloud-transfer/Cargo.toml index b6721c62f..6723b7bc2 100644 --- a/crates/fakecloud-transfer/Cargo.toml +++ b/crates/fakecloud-transfer/Cargo.toml @@ -21,6 +21,7 @@ thiserror = { workspace = true } tokio = { workspace = true } tracing = { workspace = true } uuid = { workspace = true } +x509-cert = { version = "0.2", features = ["pem"] } [dev-dependencies] tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } diff --git a/crates/fakecloud-transfer/src/service.rs b/crates/fakecloud-transfer/src/service.rs index 69b3df91d..f8f1d35cd 100644 --- a/crates/fakecloud-transfer/src/service.rs +++ b/crates/fakecloud-transfer/src/service.rs @@ -282,7 +282,8 @@ fn not_found(msg: &str) -> AwsServiceError { } fn resource_exists(msg: &str) -> AwsServiceError { - AwsServiceError::aws_error(StatusCode::BAD_REQUEST, "ResourceExistsException", msg) + // The Smithy model marks `ResourceExistsException` with `httpError: 409`. + AwsServiceError::aws_error(StatusCode::CONFLICT, "ResourceExistsException", msg) } /// Read a required, non-empty string field. @@ -410,6 +411,28 @@ const SERVER_FIELDS: &[&str] = &[ "Domain", ]; +/// Fields an `UpdateServer` request may mutate. Mirrors `SERVER_FIELDS` but +/// excludes create-only members (`Domain`) that are absent from the model's +/// `UpdateServerRequest` shape, so an `UpdateServer` carrying `Domain` cannot +/// silently rewrite an immutable attribute. +const UPDATE_SERVER_FIELDS: &[&str] = &[ + "Certificate", + "ProtocolDetails", + "EndpointDetails", + "EndpointType", + "IdentityProviderDetails", + "IdentityProviderType", + "LoggingRole", + "PostAuthenticationLoginBanner", + "PreAuthenticationLoginBanner", + "Protocols", + "SecurityPolicyName", + "WorkflowDetails", + "StructuredLogDestinations", + "S3StorageOptions", + "IpAddressType", +]; + impl TransferService { fn require_server<'a>( &self, @@ -441,6 +464,12 @@ impl TransferService { .cloned() .unwrap_or(json!("SERVICE_MANAGED")), ); + // AWS defaults `Protocols` to `["SFTP"]` and `DescribedServer` always + // echoes it, so default it here to avoid terraform drift. + srv.insert( + "Protocols".into(), + b.get("Protocols").cloned().unwrap_or(json!(["SFTP"])), + ); // Settle to ONLINE immediately so `server-online` waiters complete // against the synchronous control plane. srv.insert("State".into(), json!("ONLINE")); @@ -451,16 +480,10 @@ impl TransferService { ); copy_present(b, &mut srv, SERVER_FIELDS); let srv = Value::Object(srv); - self.state - .write() - .get_or_create(&ctx.account) - .servers - .insert(server_id.clone(), srv); - { - let mut guard = self.state.write(); - let data = guard.get_or_create(&ctx.account); - store_tags(data, &server_arn, b); - } + let mut guard = self.state.write(); + let data = guard.get_or_create(&ctx.account); + data.servers.insert(server_id.clone(), srv); + store_tags(data, &server_arn, b); ok(json!({ "ServerId": server_id })) } @@ -499,7 +522,7 @@ impl TransferService { .get_mut(&server_id) .ok_or_else(|| not_found(&format!("Server {server_id} does not exist.")))?; let obj = srv.as_object_mut().unwrap(); - copy_present(b, obj, SERVER_FIELDS); + copy_present(b, obj, UPDATE_SERVER_FIELDS); ok(json!({ "ServerId": server_id })) } @@ -515,6 +538,26 @@ impl TransferService { data.tags.remove(arn); } let prefix = format!("{server_id}/"); + // Collect the ARNs of every cascaded child so their tag entries are + // removed too (otherwise deleting the server leaks child tags). + let mut child_arns: Vec = Vec::new(); + for map in [ + &data.users, + &data.accesses, + &data.host_keys, + &data.agreements, + ] { + for (k, v) in map.iter() { + if k.starts_with(&prefix) { + if let Some(arn) = v.get("Arn").and_then(Value::as_str) { + child_arns.push(arn.to_string()); + } + } + } + } + for arn in &child_arns { + data.tags.remove(arn); + } data.users.retain(|k, _| !k.starts_with(&prefix)); data.accesses.retain(|k, _| !k.starts_with(&prefix)); data.host_keys.retain(|k, _| !k.starts_with(&prefix)); @@ -802,10 +845,30 @@ fn listed_user(user: &Value) -> Value { // ===================== host keys ===================== +/// Derive the `HostKeyType` AWS reports from the supplied key material. AWS +/// inspects the algorithm embedded in the (private or public) key body; we +/// detect the algorithm token pragmatically and fall back to `ssh-rsa`. +fn host_key_type(body: &str) -> &'static str { + // The algorithm token appears verbatim in an OpenSSH public key line and in + // the header/blob of a private key. Probe the more specific tokens first; + // RSA (`ssh-rsa`, PEM `BEGIN RSA`, generic OpenSSH) is the default. + for token in [ + "ssh-ed25519", + "ecdsa-sha2-nistp256", + "ecdsa-sha2-nistp384", + "ecdsa-sha2-nistp521", + ] { + if body.contains(token) { + return token; + } + } + "ssh-rsa" +} + impl TransferService { fn import_host_key(&self, ctx: &Ctx, b: &Value) -> Result { let server_id = req_str(b, "ServerId")?.to_string(); - req_str(b, "HostKeyBody")?; + let host_key_body = req_str(b, "HostKeyBody")?.to_string(); let mut guard = self.state.write(); let data = guard.get_or_create(&ctx.account); self.require_server(data, &server_id)?; @@ -819,7 +882,7 @@ impl TransferService { "HostKeyFingerprint".into(), json!(format!("SHA256:{}", hex17())), ); - hk.insert("Type".into(), json!("ssh-rsa")); + hk.insert("Type".into(), json!(host_key_type(&host_key_body))); hk.insert("DateImported".into(), json!(now_ts())); copy_present(b, &mut hk, &["Description"]); data.host_keys.insert(key, Value::Object(hk)); @@ -1622,10 +1685,31 @@ impl TransferService { // ===================== certificates ===================== +/// Parse a PEM `Certificate` body and extract the computed +/// `Serial`/`NotBeforeDate`/`NotAfterDate` fields AWS returns on a +/// `DescribedCertificate`. Returns `None` on any parse failure (never panics). +fn parse_certificate_fields(pem: &str) -> Option<(String, f64, f64)> { + use x509_cert::der::DecodePem; + use x509_cert::Certificate; + + let cert = Certificate::from_pem(pem.as_bytes()).ok()?; + let tbs = &cert.tbs_certificate; + // Serial number as a lowercase hex string, matching AWS's rendering. + let serial = tbs + .serial_number + .as_bytes() + .iter() + .map(|byte| format!("{byte:02x}")) + .collect::(); + let not_before = tbs.validity.not_before.to_unix_duration().as_secs() as f64; + let not_after = tbs.validity.not_after.to_unix_duration().as_secs() as f64; + Some((serial, not_before, not_after)) +} + impl TransferService { fn import_certificate(&self, ctx: &Ctx, b: &Value) -> Result { req_str(b, "Usage")?; - req_str(b, "Certificate")?; + let certificate_pem = req_str(b, "Certificate")?.to_string(); let certificate_id = format!("cert-{}", hex17()); let c_arn = arn(ctx, &format!("certificate/{certificate_id}")); let mut c = Map::new(); @@ -1633,6 +1717,14 @@ impl TransferService { c.insert("CertificateId".into(), json!(certificate_id)); c.insert("Status".into(), json!("ACTIVE")); c.insert("Type".into(), json!("CERTIFICATE")); + // Populate the computed Serial/NotBeforeDate/NotAfterDate from the PEM. + // A body that does not parse (e.g. a placeholder) simply leaves them + // unset, exactly as an unparsable cert would omit them. + if let Some((serial, not_before, not_after)) = parse_certificate_fields(&certificate_pem) { + c.insert("Serial".into(), json!(serial)); + c.insert("NotBeforeDate".into(), json!(not_before)); + c.insert("NotAfterDate".into(), json!(not_after)); + } copy_present( b, &mut c, @@ -2051,6 +2143,13 @@ impl TransferService { .collect() }) .unwrap_or_default(); - ok(json!({ "Arn": resource_arn, "Tags": tags })) + // `ListTagsForResourceResponse` carries `MaxResults`/`NextToken`, so + // honour the pagination window the validator already accepts. + ok(list_response( + "Tags", + tags, + b, + &[("Arn", json!(resource_arn))], + )) } } diff --git a/crates/fakecloud-transfer/tests/service_tests.rs b/crates/fakecloud-transfer/tests/service_tests.rs index 119fe4e11..7d09fc9b6 100644 --- a/crates/fakecloud-transfer/tests/service_tests.rs +++ b/crates/fakecloud-transfer/tests/service_tests.rs @@ -62,6 +62,15 @@ async fn err_code(s: &TransferService, action: &str, body: Value) -> String { e.code().to_string() } +async fn err_status(s: &TransferService, action: &str, body: Value) -> u16 { + let e = s + .handle(request(action, body)) + .await + .err() + .expect("expected error"); + e.status().as_u16() +} + async fn new_server(s: &TransferService) -> String { call( s, @@ -76,6 +85,20 @@ async fn new_server(s: &TransferService) -> String { const ROLE: &str = "arn:aws:iam::000000000000:role/transfer-role"; +/// A real self-signed ECDSA P-256 certificate (serial 0x3039), used to exercise +/// the PEM-parsing path in `ImportCertificate`. +const TEST_CERT_PEM: &str = "-----BEGIN CERTIFICATE-----\n\ +MIIBfzCCASWgAwIBAgICMDkwCgYIKoZIzj0EAwIwHjEcMBoGA1UEAwwTZmFrZWNs\n\ +b3VkLXRlc3QtY2VydDAeFw0yNjA3MDQwNDMyNDBaFw0zNjA3MDEwNDMyNDBaMB4x\n\ +HDAaBgNVBAMME2Zha2VjbG91ZC10ZXN0LWNlcnQwWTATBgcqhkjOPQIBBggqhkjO\n\ +PQMBBwNCAAR8LePc+d6fQ07Gd8HC18k6FdRwW2uBUzceP0iwL2O9Hh7bjacYNJPf\n\ +FelbZTDBUUaAjnj7s7Uo4fLUGpa03pADo1MwUTAdBgNVHQ4EFgQUQUA6HL+QbCOx\n\ +mRXO4LoqwQMxvSswHwYDVR0jBBgwFoAUQUA6HL+QbCOxmRXO4LoqwQMxvSswDwYD\n\ +VR0TAQH/BAUwAwEB/zAKBggqhkjOPQQDAgNIADBFAiEAlpmlaZLCBcJYDs+j3B47\n\ +6J3PptcVgoSmAetCad/w1uQCIHFdmHdYR2QBGis74scDSWqLR/DAqhpQtPPFpD9o\n\ +Lljq\n\ +-----END CERTIFICATE-----\n"; + #[tokio::test] async fn server_crud_and_state_transitions() { let s = svc(); @@ -577,3 +600,224 @@ async fn duplicate_server_children_conflict() { "ResourceExistsException" ); } + +// ===================== regression tests (post-merge bug-hunt) ===================== + +/// Fix 1: a duplicate create returns HTTP 409 (ResourceExistsException maps to +/// `httpError: 409` in the model), not 400. +#[tokio::test] +async fn resource_exists_returns_409() { + let s = svc(); + let server_id = new_server(&s).await; + call( + &s, + "CreateUser", + json!({ "ServerId": server_id, "UserName": "dup", "Role": ROLE }), + ) + .await; + let status = err_status( + &s, + "CreateUser", + json!({ "ServerId": server_id, "UserName": "dup", "Role": ROLE }), + ) + .await; + assert_eq!(status, 409); +} + +/// Fix 2: a server created without `Protocols` still reports the AWS default +/// `["SFTP"]` on describe, avoiding terraform drift. +#[tokio::test] +async fn server_protocols_default_to_sftp() { + let s = svc(); + let server_id = new_server(&s).await; + let desc = call(&s, "DescribeServer", json!({ "ServerId": server_id })).await; + assert_eq!(desc["Server"]["Protocols"], json!(["SFTP"])); + + // An explicit value is preserved. + let sid2 = call( + &s, + "CreateServer", + json!({ "IdentityProviderType": "SERVICE_MANAGED", "Protocols": ["FTP", "FTPS"] }), + ) + .await["ServerId"] + .as_str() + .unwrap() + .to_string(); + let desc2 = call(&s, "DescribeServer", json!({ "ServerId": sid2 })).await; + assert_eq!(desc2["Server"]["Protocols"], json!(["FTP", "FTPS"])); +} + +/// Fix 3: the host-key `Type` is derived from the key material, not hardcoded. +#[tokio::test] +async fn host_key_type_derived_from_body() { + let s = svc(); + let server_id = new_server(&s).await; + + let cases = [ + ( + "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAI... user@host", + "ssh-ed25519", + ), + ( + "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTY... user@host", + "ecdsa-sha2-nistp256", + ), + ("ssh-rsa AAAAB3NzaC1yc2EAAAADAQAB... user@host", "ssh-rsa"), + ]; + for (body, expected) in cases { + let hk = call( + &s, + "ImportHostKey", + json!({ "ServerId": server_id, "HostKeyBody": body }), + ) + .await; + let host_key_id = hk["HostKeyId"].as_str().unwrap().to_string(); + let desc = call( + &s, + "DescribeHostKey", + json!({ "ServerId": server_id, "HostKeyId": host_key_id }), + ) + .await; + assert_eq!(desc["HostKey"]["Type"], json!(expected), "body: {body}"); + } +} + +/// Fix 4: deleting a server also removes the tag entries of its cascaded +/// children so no orphaned tags linger. +#[tokio::test] +async fn delete_server_removes_child_tags() { + let s = svc(); + let server_id = new_server(&s).await; + call( + &s, + "CreateUser", + json!({ "ServerId": server_id, "UserName": "tagged", "Role": ROLE, + "Tags": [{ "Key": "team", "Value": "ops" }] }), + ) + .await; + let user_arn = call( + &s, + "DescribeUser", + json!({ "ServerId": server_id, "UserName": "tagged" }), + ) + .await["User"]["Arn"] + .as_str() + .unwrap() + .to_string(); + // Tags are visible before deletion. + let tags = call(&s, "ListTagsForResource", json!({ "Arn": user_arn })).await; + assert_eq!(tags["Tags"].as_array().unwrap().len(), 1); + + call(&s, "DeleteServer", json!({ "ServerId": server_id })).await; + + // After cascade the child's tags are gone. + let tags = call(&s, "ListTagsForResource", json!({ "Arn": user_arn })).await; + assert!(tags["Tags"].as_array().unwrap().is_empty()); +} + +/// Fix 5: UpdateServer must ignore the create-only `Domain` member. +#[tokio::test] +async fn update_server_ignores_domain() { + let s = svc(); + let server_id = new_server(&s).await; + let before = call(&s, "DescribeServer", json!({ "ServerId": server_id })).await; + assert_eq!(before["Server"]["Domain"], json!("S3")); + + call( + &s, + "UpdateServer", + json!({ "ServerId": server_id, "Domain": "EFS", "LoggingRole": ROLE }), + ) + .await; + let after = call(&s, "DescribeServer", json!({ "ServerId": server_id })).await; + // Domain is unchanged; the legitimate LoggingRole update still applied. + assert_eq!(after["Server"]["Domain"], json!("S3")); + assert_eq!(after["Server"]["LoggingRole"], json!(ROLE)); +} + +/// Fix 6: ListTagsForResource honours MaxResults/NextToken. +#[tokio::test] +async fn list_tags_paginates() { + let s = svc(); + let server_id = new_server(&s).await; + let arn = call(&s, "DescribeServer", json!({ "ServerId": server_id })).await["Server"]["Arn"] + .as_str() + .unwrap() + .to_string(); + call( + &s, + "TagResource", + json!({ "Arn": arn, "Tags": [ + { "Key": "a", "Value": "1" }, + { "Key": "b", "Value": "2" }, + { "Key": "c", "Value": "3" } + ] }), + ) + .await; + + let page = call( + &s, + "ListTagsForResource", + json!({ "Arn": arn, "MaxResults": 2 }), + ) + .await; + assert_eq!(page["Tags"].as_array().unwrap().len(), 2); + let next = page["NextToken"] + .as_str() + .expect("NextToken present") + .to_string(); + + let page2 = call( + &s, + "ListTagsForResource", + json!({ "Arn": arn, "MaxResults": 2, "NextToken": next }), + ) + .await; + assert_eq!(page2["Tags"].as_array().unwrap().len(), 1); + assert!(page2.get("NextToken").is_none()); +} + +/// Fix 8: importing a real PEM certificate populates the computed +/// Serial/NotBeforeDate/NotAfterDate fields; a non-PEM body leaves them unset. +#[tokio::test] +async fn certificate_dates_populated_from_pem() { + let s = svc(); + let pem = TEST_CERT_PEM; + let cert = call( + &s, + "ImportCertificate", + json!({ "Usage": "SIGNING", "Certificate": pem }), + ) + .await; + let certificate_id = cert["CertificateId"].as_str().unwrap().to_string(); + let desc = call( + &s, + "DescribeCertificate", + json!({ "CertificateId": certificate_id }), + ) + .await["Certificate"] + .clone(); + assert!( + desc.get("Serial").is_some(), + "Serial should be populated: {desc}" + ); + assert!(desc.get("NotBeforeDate").is_some(), "NotBeforeDate missing"); + assert!(desc.get("NotAfterDate").is_some(), "NotAfterDate missing"); + // Dates are epoch-seconds numbers; NotAfter is after NotBefore. + let nb = desc["NotBeforeDate"].as_f64().unwrap(); + let na = desc["NotAfterDate"].as_f64().unwrap(); + assert!(na > nb); + + // A non-PEM placeholder body leaves the computed fields unset (no panic). + let cert2 = call( + &s, + "ImportCertificate", + json!({ "Usage": "SIGNING", "Certificate": "-----BEGIN CERTIFICATE-----" }), + ) + .await; + let cid2 = cert2["CertificateId"].as_str().unwrap().to_string(); + let desc2 = call(&s, "DescribeCertificate", json!({ "CertificateId": cid2 })).await + ["Certificate"] + .clone(); + assert!(desc2.get("Serial").is_none()); +}