From e261a960cb365ad92f103a35b262713118ea6441 Mon Sep 17 00:00:00 2001 From: Rain Date: Wed, 24 Jan 2024 15:38:25 -0800 Subject: [PATCH] [meta] update samael to 0.0.14 (#4878) Required to unblock a bunch of other updates. The behavior of a test changed, but in a way that to my understanding based on [the Duo article](https://duo.com/blog/duo-finds-saml-vulnerabilities-affecting-multiple-implementations) is still safe. See the comment included in the PR for more. --- Cargo.lock | 18 +++++++-------- Cargo.toml | 2 +- nexus/tests/integration_tests/saml.rs | 33 +++++++++++++++++++++++---- workspace-hack/Cargo.toml | 2 -- 4 files changed, 38 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0c3eb15179..6ee028bbc5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -479,11 +479,11 @@ dependencies = [ [[package]] name = "bindgen" -version = "0.65.1" +version = "0.69.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cfdf7b466f9a4903edc73f95d6d2bcd5baf8ae620638762244d3f60143643cc5" +checksum = "a4c69fae65a523209d34240b60abe0c42d33d1045d445c0839d8a4894a736e2d" dependencies = [ - "bitflags 1.3.2", + "bitflags 2.4.0", "cexpr", "clang-sys", "lazy_static", @@ -5267,7 +5267,6 @@ dependencies = [ "sha2", "similar", "slog", - "snafu", "socket2 0.5.5", "spin 0.9.8", "string_cache", @@ -6496,9 +6495,9 @@ checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" [[package]] name = "quick-xml" -version = "0.23.1" +version = "0.30.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "11bafc859c6815fbaffbbbf4229ecb767ac913fecb27f9ad4343662e9ef099ea" +checksum = "eff6510e86862b57b210fd8cbe8ed3f0d7d600b9c2863cd4549a2e033c66e956" dependencies = [ "memchr", "serde", @@ -7372,8 +7371,9 @@ dependencies = [ [[package]] name = "samael" -version = "0.0.10" -source = "git+https://github.com/njaremko/samael?branch=master#52028e45d11ceb7114bf0c730a9971207e965602" +version = "0.0.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b75583aad4a51c50fc0af69c230d18078c9d5a69a98d0f6013d01053acf744f4" dependencies = [ "base64", "bindgen", @@ -7391,7 +7391,7 @@ dependencies = [ "quick-xml", "rand 0.8.5", "serde", - "snafu", + "thiserror", "url", "uuid", ] diff --git a/Cargo.toml b/Cargo.toml index 093e972b42..ed54ae8c6a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -325,7 +325,7 @@ rustfmt-wrapper = "0.2" rustls = "0.22.2" rustls-pemfile = "2.0.0" rustyline = "12.0.0" -samael = { git = "https://github.com/njaremko/samael", features = ["xmlsec"], branch = "master" } +samael = { version = "0.0.14", features = ["xmlsec"] } schemars = "0.8.16" secrecy = "0.8.0" semver = { version = "1.0.21", features = ["std", "serde"] } diff --git a/nexus/tests/integration_tests/saml.rs b/nexus/tests/integration_tests/saml.rs index fc04bbf908..b1b0429c2e 100644 --- a/nexus/tests/integration_tests/saml.rs +++ b/nexus/tests/integration_tests/saml.rs @@ -964,12 +964,33 @@ fn test_reject_unsigned_saml_response() { assert!(result.is_err()); } -// Test rejecting a correct SAML response that contains a XML comment in -// saml:NameID. +// Test accepting a correct SAML response that contains a XML comment in +// saml:NameID, and ensuring that the full text node is extracted (and not a +// substring). // -// See: https://duo.com/blog/duo-finds-saml-vulnerabilities-affecting-multiple-implementations +// This used to be a test that _rejected_ such responses, but a change to an +// upstream dependency (quick-xml) caused the behavior around text nodes with +// embedded comments to change. Specifically, consider: +// +// user@example.com.evil.com +// +// What should the text node for this element be? +// +// * Some XML parsing libraries just return "user@example.com". That leads to a +// vulnerability, where an attacker can get a response signed with a +// different email address than intended. +// * Some XML libraries return "user@example.com.evil.com". This is safe, +// because the text after the comment hasn't been dropped. This is the behavior +// with quick-xml 0.30, and the one that we're testing here. +// * Some XML libraries are unable to deserialize the document. This is also +// safe (and not particularly problematic because typically SAML responses +// aren't going to contain comments), and was the behavior with quick-xml +// 0.23. +// +// See: +// https://duo.com/blog/duo-finds-saml-vulnerabilities-affecting-multiple-implementations #[test] -fn test_reject_saml_response_with_xml_comment() { +fn test_handle_saml_response_with_xml_comment() { let silo_saml_identity_provider = SamlIdentityProvider { idp_metadata_document_string: SAML_RESPONSE_IDP_DESCRIPTOR.to_string(), @@ -1004,7 +1025,9 @@ fn test_reject_saml_response_with_xml_comment() { ), ); - assert!(result.is_err()); + let (authenticated_subject, _) = + result.expect("expected validation to succeed"); + assert_eq!(authenticated_subject.external_id, "some@customer.com"); } // Test receiving a correct SAML response that has group attributes diff --git a/workspace-hack/Cargo.toml b/workspace-hack/Cargo.toml index cda4426c9b..25a72838a0 100644 --- a/workspace-hack/Cargo.toml +++ b/workspace-hack/Cargo.toml @@ -96,7 +96,6 @@ serde_json = { version = "1.0.111", features = ["raw_value", "unbounded_depth"] sha2 = { version = "0.10.8", features = ["oid"] } similar = { version = "2.3.0", features = ["inline", "unicode"] } slog = { version = "2.7.0", features = ["dynamic-keys", "max_level_trace", "release_max_level_debug", "release_max_level_trace"] } -snafu = { version = "0.7.5", features = ["futures"] } socket2 = { version = "0.5.5", default-features = false, features = ["all"] } spin = { version = "0.9.8" } string_cache = { version = "0.8.7" } @@ -204,7 +203,6 @@ serde_json = { version = "1.0.111", features = ["raw_value", "unbounded_depth"] sha2 = { version = "0.10.8", features = ["oid"] } similar = { version = "2.3.0", features = ["inline", "unicode"] } slog = { version = "2.7.0", features = ["dynamic-keys", "max_level_trace", "release_max_level_debug", "release_max_level_trace"] } -snafu = { version = "0.7.5", features = ["futures"] } socket2 = { version = "0.5.5", default-features = false, features = ["all"] } spin = { version = "0.9.8" } string_cache = { version = "0.8.7" }