Too strict type checking in SubjectConfirmationData

[MKodde]

While test driving the latest version of the SAML2 library in OpenConext Engineblock, I ran into a situation where consuming a SAML response from the IdP, Engineblock ran into an InvalidArgumentException thrown in the SubjectConfirmationData.

The issue in my situation was that the IdP actually returned an URL instead of an IP address in the Address attribute of the SubjectConfirmationData element. The SAML specification is not very explicit about what data should be in this attribute except for that it should be a string. Granted, setting an URL on this attribute does not make sense, but throwing an InvalidArgumentException is a bit harsh in my opinion.

Some suggestions:

1. Maybe log these kinds of data type violations instead of throwing an exception?
2. Provide a means to make parsing of the XML less strict.

[tvdijen]

For reference:

Address [Optional] The network address/location from which an attesting entity can present the assertion. For example, this attribute might be used to bind the assertion to particular client addresses to prevent an attackerfrom easily stealing and presenting the assertion from another location. IPv4 addresses SHOULD be represented in the usual dotted-decimal format (e.g., "1.2.3.4"). IPv6 addresses SHOULD be represented as defined by Section 2.2 of IETF RFC 3513 [RFC 3513] (e.g.,"FEDC:BA98:7654:3210:FEDC:BA98:7654:3210").

[thijskh]

I think there are two issues:

Specifically: whether we may enforce Address to be an IP address. Since the spec is quite vague on whether it would actually be obliged to be an IP address in the first place, and secondly only claims SHOULDs on the desired formats, this may be too strict.

Generally: I think adding these checks and throwing an exception makes the library less interoperable. We now throw an exception when Address is not in the expected format. This is already done in the assertion parsing. This means that e.g. one of our application now breaks on an IdP that happens to send something different as a value. But the thing is, our application does not even use this attribute. So an IdP and SP that have been working for years now break for no apparent benefit. Note that these messages are valid according to the XSD, which is likely the most common check implementers do.

From experience I know the SAML spec is so broad that in a large deployment, people will send all kinds of stuff in all kinds of attributes. If we are going to be so strict already in the parsing phase, we will run into many problems using it.

See also an earlier change in 6db983648ca2607bf0de8025f30f3007e962c374 where we precisely made the parsing more lenient because some of our SPs in eduGAIN would receive a string value for the attribute, and break, even if they didn't need the attribute at all.

I would prefer that we log a warning instead, and/or not set the value if it's invalid.

[MKodde]

Thanks for the reference @tvdijen! I must say that I agree with @thijskh on this.

[tvdijen]

I've released v3.3.7 to fix this!