fix: prep_for_nego_auth: avoid double signing redirect requests

[vladimir-mencl-eresearch]

Fixes IdentityPython/pysaml2#819 (again)

The prepare_for_negotiated_authenticate method has sign parameter defaulting to None.

The logic setting sign_redirect and sign_post does not properly handle the three-state aspects that sign has with None mixed with True and False.

Python evalutes None and <any value> as None, so as a result, None gets passed for both sign_redirect and sign_post.

However, None is interpreted by Entity._message as "sign if self.should_sign".

As a result, for Redirect binding, the authentication request gets signed both in XML and in HTTP parameter (recurrence of IdentityPython/pysaml2#819).

Fix this by passing an explicit False for exactly one of the branches (sign_post for REDIRECT binding and sign_redirect for all other bindings), passing through value of sign for the other branch.

Description

The feature or problem addressed by this PR

Fix double signing of of Authentication requests with redirect binding.

What your changes do and why you chose this solution

Fix logic to avoid passing None for both sign_post and sign_redirect (as they both get interpreted as "sign if should_sign)

Checklist

• [X] Checked that no other issues or pull requests exist for the same issue/change
• [ ] Added tests covering the new functionality
• [X] Updated documentation OR the change is too minor to be documented
• [X] Updated CHANGELOG.md OR changes are insignificant

[vladimir-mencl-eresearch]

Hi @c00kiemon5ter ,

I ran into the issue with double-signed authentication requests ( #819 ) again (even though it was fixed for SATOSA in IdentityPython/SATOSA#380) - and traced it to tri-state logic (None, True, False) passing None for both sign_post and sign_resolve, even though only one should be "sign if we should sign" and the other should be explicit False.

I see the current code was introduced in 44d967d26 - I believe the logic I put in is correct, but please let me know if you think it was reintroduce the issue that 44d967d26 (in #834) was solving. (Sorry, I could not deduce the real intent of the change there).

Please let me know if this is OK to merge or if you'd like me to make any changes.

Cheers, Vlad

[vladimir-mencl-eresearch]

@c00kiemon5ter , do the changes look right to you?

The double signing in SAML messages in Redirect profile was breaking our use of SATOSA, I believe this fixes it without breaking anything else.

Many thanks in advance for looking into it!

Cheers, Vlad

[c00kiemon5ter]

Thank you @vladimir-mencl-eresearch and sorry for such a big delay.

[jonathanperret]

Thank you @c00kiemon5ter for merging this! And apologies for the backlink spam above, it looks github isn’t smart enough to deduplicate rebased commits.