Latest lxml cannot retrieve AttributeValues

[acrividenco]

Latest lxml (4.9.2) seems to have broken iterchildren, and the iteration linked below receives an empty attributes list.
I downgraded to lxml==4.6.5 and it works. I suggest updating the setup install_requires

python3-saml/src/onelogin/saml2/response.py

Line 597 in 538622d

for attr in attribute_node.iterchildren('{%s}AttributeValue' % OneLogin_Saml2_Constants.NSMAP['saml']):

[bitti]

I get

onelogin.saml2.errors.OneLogin_Saml2_Error: Invalid dict settings: sp_cert_not_found_and_required

when calling OneLogin_Saml2_Settings after updating from 1.14.0 to 1.15.0. Is it related?

[acrividenco]

I did not have any issues with certs or the actual OneLogin code, just that function call (iterchildren) that was broken by the lxml library and could not retrieve the attributes anymore. Same python3-saml library, older lxml library and everything works. There are other open issues on this project related to that broken library. Seems like 'lxml>=4.6.5, !=4.7.0' is not quite right.

[pitbulk]

@bitti you get that error when you are not providing a cert on the SP settings, but you enabled any flag related to sign AuthNrequest, LogoutRequest, LogoutResponse, Metadatra at the advanced settings.

@acrividenco what version of python3-saml are you using?
are you using encrypted SAML Assertions?

You can see in the last build that past the tests that lxml 4.9.2 was installed

Can you check if the SAMLResponse was properly validated? Are we missing an specific case?

[bitti]

@pitbulk thanks for the hint. But why should this surface through an update to 1.15.0 then? I can't see changes in the corresponding code since years. I also don't see that we set any "advanced settings". Did some defaults change?

[pitbulk]

In the latest version of python3-saml I removed some restrictions on the version of the lxml dependence.
The previous version fixed lxml to an specific version, but it seems lxml was vulnerable for such version.

Read:

• The most recent version depends on the vulnerable lxlm version #336
• lxml has a vulnerability but we can not update because of python3-saml relies on <4.7.1 #319

[bitti]

I know, that's why I was wondering if it was related to the lxml update. Apparently not. The only other change which may be related seems to be #338, but that only seems to be related to IdP settings, not sp settings, so I'm at a loss here.

[pitbulk]

@bitti are you sure you get "Invalid dict settings: sp_cert_not_found_and_required" only on 1.15 and not in 1.14?

That error is raised here

Maybe you can compare the condition in one and the other version, to check if there are discrepancies.

[bitti]

@pitbulk: Ok, I just set a debug breakpoint at line 486, once for version 1.14.0 and once for 1.15.0. The decisive difference seems to be that the authn_sign flag is True in the newer version (all other flags are still False). When looking at the security dictionary, I can see that key 'authnRequestsSigned' is mapped to the string 'false':

(Pdb) security
{'wantAttributeStatement': True, 'authnRequestsSigned': 'false'}

It looks like this should be False instead, since any nonempty string evaluates to True. The same Dictionary doesn't have an entry for 'authnRequestsSigned' at all when using the old version:

(Pdb) security
{'wantAttributeStatement': True}

Should I open a separate ticket for this or do you already have an idea what the problem could be?

[bitti]

The culprit is indeed #338. The problem goes away when I revert line https://github.com/SAML-Toolkits/python3-saml/pull/338/files#diff-27e3b11cb40e52fdab3ec806451468c5d1d347b6d43d75ed55f08b15b6831ed2R151. entity_descriptor_node.get('WantAuthnRequestsSigned', None) evaluates to None and idp_descriptor_node.get('WantAuthnRequestsSigned') to 'false' in my case. Maybe the letter construct should be replaced by idp_descriptor_node.get('WantAuthnRequestsSigned') == 'true'?

[pitbulk]

@bitti , can you check #349?

[AbdealiLoKo]

I'm seeing cases where having a encrypted response is giving me empty attributes too - is that related ?

I notice the original issue was for attributes in saml response but the latest PR is in the IDP-metadata area and hence may not be related ?

[AbdealiLoKo]

I have created a reproducible test case to show this issue: #370

I can reproduce this even with latest master branch and latest lxml:

$ venv/bin/pip install lxml
...
Successfully installed lxml-4.9.3
$ venv/bin/pytest -k testGetEncryptedAttributes
...
FAILED tests/src/OneLogin/saml2_tests/response_test.py::OneLogin_Saml2_Response_Test::testGetEncryptedAttributes - AssertionError: {'uid': ['smartin'], 'mail': ['[email protected]'], [87 chars]in']} != {'uid': []...

Trying with older lxml:

$ venv/bin/pip uninstall lxml -y
$ venv/bin/pip install 'lxml<4.7'
$ venv/bin/pytest -k testGetEncryptedAttributes
...
tests/src/OneLogin/saml2_tests/response_test.py::OneLogin_Saml2_Response_Test::testGetEncryptedAttributes PASSED [100%]
...

[acrividenco]

Yes, this is exactly the issue. The response I had was encrypted. I didn't pay too much attention lately because I used the workaround mentioned in the beginning.

[AbdealiLoKo]

I see. @pitbulk would be great to get this fixed and merged !

As of now, I am doing the following workaround if it helps anyone else:

from onelogin.saml2.response import OneLogin_Saml2_Response

class MyResponse(OneLogin_Saml2_Response):
    def _decrypt_assertion(self, xml):
        xml = super()._decrypt_assertion(xml)
        xml = copy.deepcopy(xml)
        return xml

auth = OneLogin_Saml2_Auth(request_data, SAML_SETTINGS)
auth.response_class = MyResponse

This will ensure that python3-saml can use any version of lxml