Skip to content

Issues with authenticated redirects to the same destination host #64

Open

Description

@mkomitee

I've run into an issue w/ requests & requests_kerberos which I don't think we can fix without changes to requests.

We should not reuse an already established kerberos context on new requests. This means, if we're prompted for authentication, then receive a redirect, we shouldn't reuse the previously generated authorization header when following the redirect, even if the redirect is to the same hostname.

I can update our code to do the right thing (index HTTPKerberosAuth.context by response.url instead of response.host, and pop it off of self.context in authenticate_server.

What we can't do from here is change requests.Session.rebuild_auth to remove the Authorization header on a prepared request when following a redirect, even when the redirect is to the same hostname:

The code for rebuild_auth only has access to the prepared_request, the response, and the session. There's no way to check if the type of authentication object mandates the Authorization header be stripped or not:

 def rebuild_auth(self, prepared_request, response):
        """
        When being redirected we may want to strip authentication from the
        request to avoid leaking credentials. This method intelligently removes
        and reapplies authentication where possible to avoid credential loss.
        """
        headers = prepared_request.headers
        url = prepared_request.url

        if 'Authorization' in headers:
            # If we get redirected to a new host, we should strip out any
            # authentication headers.
            original_parsed = urlparse(response.request.url)
            redirect_parsed = urlparse(url)

            if (original_parsed.hostname != redirect_parsed.hostname):
                del headers['Authorization']

        # .netrc might have more auth for us on our new host.
        new_auth = get_netrc_auth(url) if self.trust_env else None
        if new_auth is not None:
            prepared_request.prepare_auth(new_auth)

        return

I looked through the hook documentation and it doesn't look like there's anywhere we can inject ourselves to modify the behavior. Do you have any ideas?

@sigmavirus24 @Lukasa I think I'll need your help on this one.

By the way, I think this is tangentially related to #54, and the reason @danc86 wasn't experiencing his problem with the "latest" requests at the time was because the latest requests happened to include the previous Authorization header on the redirected request. His suggestion to deregister the hook would have caused the latter responses from being mutually authenticated which would have solved his problem, but means he shouldn't trust that response (if he really wanted mutual authentication. If he didn't he could have just disabled it).

Maybe Session.rebuild_auth can delegate the responsibility for rebuilding the auth and manipulating headers (or not) to the authentication object if it's not from a built-in authentication mechanism ... but for that to work, we would need to be able to access the authentication object, which we can't if it's not on the session -- which it won't be for most of our users.

Activity

  1. Lukasa commented on Mar 11, 2016

    @Lukasa
    Member

    @mkomitee Yeah, so you're absolutely right here: this limitation most definitely does exist. Requests' redirect following logic simply doesn't have the smarts handle this at this time.

    I'm open to receiving a PR that changes this: possibly we want to add another method for auth handlers to have called that will allow them to be consulted by requests before requests goes to use .netrc (a kind of "are you still valid on this new host?" question).

    Thoughts @sigmavirus24?

  2. flv7a commented on May 27, 2016

    @flv7a

    Any idea when this might bubble up towards the top of the queue? I'm working on an integration with a SAML Identity Provider (SAML loves 302 redirects!) and unfortunately I'm hitting the same "Context is already fully established" error unless I disable mutual authentication entirely.

  3. Lukasa commented on May 28, 2016

    @Lukasa
    Member

    @flv7a Unfortunately we don't have any schedule on it, we're purely volunteer based so it'll get worked on when someone has time. I'm sure @mkomitee would be happy to guide you through providing a patch if that's something you're willing to do!

  4. mkomitee commented on May 28, 2016

    @mkomitee
    CollaboratorAuthor

    we can't create a patch without changes to requests proper. I wouldn't want
    to do the work in changing requests without some buy in on how you'd like
    the change designed.
    On Sat, May 28, 2016 at 10:45 AM Cory Benfield notifications@github.com
    wrote:

    @flv7a https://github.com/flv7a Unfortunately we don't have any
    schedule on it, we're purely volunteer based so it'll get worked on when
    someone has time. I'm sure @mkomitee https://github.com/mkomitee would
    be happy to guide you through providing a patch if that's something you're
    willing to do!

    —
    You are receiving this because you were mentioned.
    Reply to this email directly, view it on GitHub
    #64 (comment),
    or mute the thread
    https://github.com/notifications/unsubscribe/AAADOvpddZwQLoy8-JL17wOUHLmeRavtks5qGFULgaJpZM4HuC85
    .

  5. added a commit that references this issue on Oct 7, 2019
  6. added a commit that references this issue on Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions