Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IdP-initiated SLO does not work with Python3 #81

Closed
artxki opened this issue Nov 17, 2017 · 2 comments · Fixed by #127
Closed

IdP-initiated SLO does not work with Python3 #81

artxki opened this issue Nov 17, 2017 · 2 comments · Fixed by #127

Comments

@artxki
Copy link

artxki commented Nov 17, 2017

SLO does not function properly. Probably only affects Python3.

Symptoms:

calling OneLogin_Saml2_Auth.process_slo() leads to:

  • in the log: invalid_logout_request
  • defusedxml writes into stdout: Unicode strings with encoding declaration are not supported. Please use bytes input or XML fragments without declaration.

Cause

logout_request.py:115:

self.__logout_request = compat.to_string(logout_request)

Afterwards python3-saml in OneLogin_Saml2_XML.to_etree() calls defusedxml.lxml.fromstring with text argument containing a str instance (unicode in Python3), which causes the error.

Notes:

defusedxml.lxml.fromstring is actually called twice during OneLogin_Saml2_Auth.process_slo().

  • First time while extracting ID of the request as far as I can remember. It passes correct arguments to defusedxml.lxml.fromstring.
  • Second time while validating the request. This time with invalid arguments which causes defusedxml to fail.

Possible solution:

Replacing in logout_request.py:115

self.__logout_request = compat.to_string(logout_request)

with

self.__logout_request = logout_request

solves the problem and makes SLO to function.

I am however not sure why compat.to_string() was used and which side effects removing it might cause.

Versions

python3-saml==1.3.0
defusedxml==0.5.0
lxml==4.1.1
xmlsec==1.3.3
@pitbulk
Copy link
Contributor

pitbulk commented Nov 17, 2017

That was already reported at #60 , can you verify that #77 solves the issue for you?

@artxki
Copy link
Author

artxki commented Nov 17, 2017

@pitbulk Sorry, maybe I should have searched better and updated #60 instead 😦

Unfortunately #77 does not solve the issue. Head of lxml_valuerror behaves exactly the same as the installed from pip version.

While checking #60 I found an important difference: I use an IdP-initiated SLO.

Unlike #60, I don't experience any problems with __logout_response. I don't even get an exception! 💢 Instead after the following:

response_url = auth.process_slo(delete_session_cb=logout_callback)
errors.extend(auth.get_errors())

now errors contain ['invalid_logout_request'] and response_url is None

When the ValueError is being raised (later caught and print()-ed in logout_request.py:343) the relevant part of the stack looks like the following:

File "./_/_/views.py", line 178, in get\n    response_url = auth.process_slo(delete_session_cb=logout_callback)
File "./python3-saml/src/onelogin/saml2/auth.py", line 160, in process_slo\n    logout_request = OneLogin_Saml2_Logout_Request(self.__settings, get_data[\\'SAMLRequest\\'])
File "./python3-saml/src/onelogin/saml2/logout_request.py", line 113, in __init__\n    self.id = self.get_id(logout_request)
File "./python3-saml/src/onelogin/saml2/logout_request.py", line 151, in get_id\n    elem = OneLogin_Saml2_XML.to_etree(request)
File "./python3-saml/src/onelogin/saml2/xml_utils.py", line 66, in to_etree\n    return OneLogin_Saml2_XML._parse_etree(xml)
File "./.venv/lib/python3.5/site-packages/defusedxml/lxml.py", line 143, in fromstring\n    rootelement = _etree.fromstring(text, parser, base_url=base_url)

Please let me know if I can provide any additional info that can help.

@artxki artxki changed the title SLO does not work with Python3 IdP-initiated SLO does not work with Python3 Nov 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants