IdentityPython / pysaml2

Python implementation of SAML2
Apache License 2.0
550 stars 422 forks source link

'NoneType' object has no attribute 'require_signature' #598

Open rrauenza opened 5 years ago

rrauenza commented 5 years ago

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 commented 5 years ago

Response becomes None on this line: https://github.com/IdentityPython/pysaml2/blob/f41f60ccd4700ea72104e9bab362be071fcb0e1b/src/saml2/entity.py#L1202

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 commented 5 years ago

https://github.com/IdentityPython/pysaml2/blob/f41f60ccd4700ea72104e9bab362be071fcb0e1b/src/saml2/response.py#L408-L413

It seems to return None there to here:

https://github.com/IdentityPython/pysaml2/blob/f41f60ccd4700ea72104e9bab362be071fcb0e1b/src/saml2/response.py#L422-L427

jessiebryan commented 5 years ago

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

sheilatron commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

ghost commented 4 years ago

@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?

sheilatron commented 4 years ago

@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?

Did you check whether the destination attribute of a signed SAML response matches a configured ACS url? When I encountered this error, that was the fix. Unfortunately the error handling does not make that clear, and there might be other situation that could cause the same exception.

peppelinux commented 4 years ago

Hi guys,

try to put a logging level to debug. The answer is near.

https://github.com/IdentityPython/pysaml2/blob/f41f60ccd4700ea72104e9bab362be071fcb0e1b/src/saml2/response.py#L392

Can you see some of the previous error messages? Otherwise follow it with python debugger

ghost commented 4 years ago

The response the IDP send to the SP just has a SAMLResponse key in the form data. I'm using Simple SAML PHP as my IDP and the Django SAML2 Auth as my SP; in the IDP configuration the saml20.sign.response is already set to true, but I don't know if it is indeed sending a signed response (obs: everything is running without SSL)... besides this, I have not found any 'destination' attribute, my metadatas can be foudn here.

Debug by editing the module code can be a little complicated, there are many containers. Don't know what to do anymore, I have already read all the issues that mention something related to this error, here and in the Django SAML2 Auth module.

Update: I have set the HTTPS and nothing have changed

waza-ari commented 4 years ago

Let me join the party - I have the same problem while using the example code here:

https://github.com/jpf/okta-pysaml2-example

Any idea where to start debugging?

claytondaley commented 4 years ago

@waza-ari pretty much any error inside _verify can land you here. The problem actually arises here if the the verify command here returns None as it does here if any assertion error in _verify causes an error. (I may have the exact lines wrong for a given situation, but the same idea holds).

If possible, the simplest thing to do is to add an as e to the verify exception clause in your trace and log/print the actual error.

bbearce commented 3 years ago

I've been hacking away at this for 3 days now. I'm so lost. I was successful in making this work for the polls application, but I had no login really and I just redirected to the polls app.

Note, in a separate project with Flask I have also got SAML to work, so I believe I roughly understand the SAML workflow.

After attempting to add this to a larger django project (1.11.+), I can't get passed the Okta login. Once I "sign in" the redirect goes to "http://\<domain name>/saml2_auth/acs/" and I get "AttributeError: 'NoneType' object has no attribute 'require_signature'" in my logs. I'm really trying to follow what has been said above, but all I gather so far is that "In short, I think if you just misconfigure a valid endpoint url you'll see the error." I also see something about "SAML response "Destination=" value does not match the ACS url".

How can I check this? More specifically what is the Destination URL? I have set 'DEFAULT_NEXT_URL' to '/admin'? After following the tutorial I believe I've set everything correctly.

SAML settings below:

    SAML2_AUTH = {
        # Metadata is required, choose either remote url or local file path
        'METADATA_AUTO_CONF_URL': 'https://dev-942176.okta.com/app/exkm6byo094UPAYCq4x6/sso/saml/metadata',
        # 'METADATA_LOCAL_FILE_PATH': '[The metadata configuration file path]',

        # # Optional settings below
        'DEFAULT_NEXT_URL': '/admin',  # Custom target redirect URL after the user get logged in. Default to /admin if not set. This setting will be overwritten if you have parameter ?next= specificed in the login URL.
        # 'CREATE_USER': 'TRUE', # Create a new Django user when a new user logs in. Defaults to True.
        # 'NEW_USER_PROFILE': {
        #     'USER_GROUPS': [],  # The default group name when a new user logs in
        #     'ACTIVE_STATUS': True,  # The default active status for new users
        #     'STAFF_STATUS': True,  # The staff status for new users
        #     'SUPERUSER_STATUS': False,  # The superuser status for new users
        # },
        # 'ATTRIBUTES_MAP': {  # Change Email/UserName/FirstName/LastName to corresponding SAML2 userprofile attributes.
        #     'email': 'Email',
        #     'username': 'UserName',
        #     'first_name': 'FirstName',
        #     'last_name': 'LastName',
        # },
        # 'TRIGGER': {
        #     'CREATE_USER': 'path.to.your.new.user.hook.method',
        #     'BEFORE_LOGIN': 'path.to.your.login.hook.method',
        # },
        # 'ASSERTION_URL': 'https://mysite.com', # Custom URL to validate incoming SAML requests against
        'ENTITY_ID': 'http://mgh-azure-covid-collab.eastus.cloudapp.azure.com/saml2_auth/acs/', # Populates the Issuer element in authn request
        # 'NAME_ID_FORMAT': FormatString, # Sets the Format property of authn NameIDPolicy element
        # 'USE_JWT': False, # Set this to True if you are running a Single Page Application (SPA) with Django Rest Framework (DRF), and are using JWT authentication to authorize client users
        # 'FRONTEND_URL': 'https://myfrontendclient.com', # Redirect URL for the client if you are using JWT auth with DRF. See explanation below
    }

Is this still being worked on? Please pardon my ignorance, but I can't tell if this is considered resolved or not. I'd love to help but am a bit of a newb when it comes to github contributions, but am willing to learn. The last comments I see seem to suggest we essentially add print statements (logs) inside verify() to see what exactly is messed up, but how does that help us? It seems like many people experience this issue...

This may be taboo here, but has anyone tried this package (https://pypi.org/project/djangosaml2/#configuration). It looks way more complicated to setup, but I'm just wondering which is considered the best solution.

As always thanks so much for creating this package and fixing it!

peppelinux commented 3 years ago

Probably @sheilatron does already answered to this problem and many other hints have come (great!). We should consider some configuration example to made this reproducible and then decide how to handle it, after after recognizing it was a configuration error we just have to handle correctly the exception.