'NoneType' object has no attribute 'require_signature'

[rrauenza]

Code Version

pysaml2==4.6.5

Expected Behavior

I'd expect a better error message regarding root cause

Current Behavior

My end points didn't match -- in short, my endpoints allowed http://foo.com:443, but the request was coming in on https://foo.com

The problem is that the above error is propagated up instead of an error about endpoints.

The root seems to be somewhere around the .verify() call which returns None when the endpoint validation fails rather than raising an exception.

Possible Solution

Raise exceptions and propagate up.

Steps to Reproduce

In short, I think if you just misconfigure a valid endpoint url you'll see the error.

[rvanlaar]

Response becomes None on this line:

pysaml2/src/saml2/entity.py

Line 1202 in f41f60c

response = response.verify(keys)

And I can confirm, it's due to the configured endpoint not being the same as where the requests comes is. In my case it was the difference between https and http.

[rrauenza]

pysaml2/src/saml2/response.py

Lines 408 to 413 in f41f60c

	if self.asynchop:
	if self.response.destination and \
	self.response.destination not in self.return_addrs:
	logger.error("%s not in %s", self.response.destination,
	self.return_addrs)
	return None

It seems to return None there to here:

pysaml2/src/saml2/response.py

Lines 422 to 427 in f41f60c

	def verify(self, keys=None):
	try:
	return self._verify()
	except AssertionError:
	logger.exception("verify")
	return None

[jessiebryan]

I am seeing this error while trying to set up SAML2 with my idP OneLogin.

[sheilatron]

Couldn't this also happen in IdP initiated scenario where the SAML response "Destination=" value does not match the ACS url? (versus having an incorrect ACS endpoint configuration)

[sheilatron]

Also what exception should be raised when response.destination is not in self.return_addrs? I would like to better understand the reason for this check.

[sheilatron]

It looks like the issue may have been introduced in this commit (between v2.1 and 2.3), where AuthnResponse.verify would now be able return None when it was expected to return self. It was described as fixing a security bug, but it is unclear/obscure what specific security issue is addressed. Sorry if I am being a pest, but am curious. Do you remember @rohe ?

Understanding would be helpful for known what exception should be raised.

[sheilatron]

I learned a little more about this and it looks like this functionality could be called signed destination validation. The security issue is summarized in this StackOverflow article, which explained this is required for SAML2 spec compliance and that "This is primarily to avoid if somebody retrieves the SAMLResponse sent by IdP in the middle and send that message to other SP party for which the SAMLResponse is not created and send for."

So maybe the exception to use would be to subclass response.VerificationError with a custom exception called DestinationVerificationError. That way it could be handled from where the verify method is called.

[sheilatron]

More notes about this: I was able to reproduce this issue by setting the Destination attribute of a signed SAML response to not match the ACS URL.

This StackOverflow article is the best explanation I could find for the security rationale behind validating response destination.

[peppelinux]

Hi @sheilatron, which kind of misconfiguration in a SP could produce this error?

Verify method use MetadataStore and configuration parameters, if parameters are good means that the problem should be investigated into metadata. Do you agree?

[claytondaley]

FWIW I got this problem on a development machine. Even though I had hosts set to redirect a full domain, return_addrs (here) list only ...127.0.0.1:8080....

What needs to be done to get this resolved?

1. Why does _verify handle errors two ways: raising an exception or returning None? Seems like it should pick one and stick to it (probably exceptions to provide actionable differences to callers).
2. Why does _verify return self? Even if a verify method changed the object (and it should not!), it has access to self. Why not return the semantically more sensible true/false (if you don't go exception/nothing).
3. Even if None is required somewhere (or must be retained for backwards compatibility), the caller (_parse_response) does an assignment in a finally block. This makes it impossible to escape the block once response is None -- unless we're able to recreate the response from scratch -- which seems unlikely.

I can submit a PR for just item 3, but really wish the interface was more intuitive. I'll cross-link if I end up finding/creating an issue for my underlying problem in case others run into the same.

[claytondaley]

OK. My underlying issue was outside the scope of this project. django-saml2-auth was building these URLs and the reverse proxy on my development machine wasn't populating the X-Forwarded headers (see here for the correct headers). Still willing to contribute a PR so future users get a useful error message.

Reverse proxies would be a natural way to run into this issue if your saml client is using request.get_host() to populate the endpoints metadata.

@sheilatron , I have already checked all my metadata files (from SP and the IDP), the SP configuration and the request redirect log / the IDP logs; but I couldn't find what I need to change to get it working, could you describe how did you exactly fixed it? @peppelinux , did you get what she was talking about?