Skip to content

Commit

Permalink
[meta] update samael to 0.0.14 (#4878)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sunshowers authored Jan 24, 2024
1 parent cc64304 commit e261a96
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 17 deletions.
18 changes: 9 additions & 9 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
Expand Down
33 changes: 28 additions & 5 deletions nexus/tests/integration_tests/saml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:
//
// <saml:NameId>[email protected]<!--comment-->.evil.com</saml:NameId>
//
// What should the text node for this element be?
//
// * Some XML parsing libraries just return "[email protected]". That leads to a
// vulnerability, where an attacker can get a response signed with a
// different email address than intended.
// * Some XML libraries return "[email protected]". 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(),

Expand Down Expand Up @@ -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, "[email protected]");
}

// Test receiving a correct SAML response that has group attributes
Expand Down
2 changes: 0 additions & 2 deletions workspace-hack/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down Expand Up @@ -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" }
Expand Down

0 comments on commit e261a96

Please sign in to comment.