search

simplesamlphp / saml2

SimpleSAMLphp low-level SAML2 PHP library
https://www.simplesamlphp.org
GNU Lesser General Public License v2.1
286 stars 135 forks source link

Mutually exclude ACS index / url / ProtocolBinding #316

Closed tvdijen closed 1 year ago

tvdijen commented 1 year ago

As per spec paragraph 3.4.1;

AssertionConsumerServiceURL [Optional] [..] This attribute is mutually exclusive with the AssertionConsumerServiceIndex attribute and is typically accompanied by the ProtocolBinding attribute.

ProtocolBinding [Optional] [..] This attribute is mutually exclusive with the AssertionConsumerServiceIndex attribute and is typically accompanied by the AssertionConsumerServiceURL attribute.

tvdijen commented 1 year ago

Tim:

Personally, I think we can be a little bit strict on what we accept here.. I mean, the specs leave no room for debate on this one

Jaime:

I’d like to know first if that could break any major sp software. It would also be nice to know the reason to make them exclusive as in… basically confirm that the reason is to avoid ambiguous behavior in the IdP because if that’s the reason, I think it’s a bit too strict to fail on this… but if there are security implications…

thijskh commented 1 year ago

I indeed think it's predictability; otherwise it's undefined which one will be chosen. I would be in favour of making it more predictable if at all possible. But indeed this is useless if it breaks major other implementations. Maybe we can check some logs to see what we can find out about this?

thijskh commented 1 year ago

I've checked 15 million logins since 2023-01-01 and 0 of them would violate these assertions.