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

urllib2 sends Basic auth across redirects #48069

Closed
TFKyle mannequin opened this issue Sep 9, 2008 · 11 comments
Closed

urllib2 sends Basic auth across redirects #48069

TFKyle mannequin opened this issue Sep 9, 2008 · 11 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@TFKyle
Copy link
Mannequin

TFKyle mannequin commented Sep 9, 2008

BPO 3819
Nosy @orsenthil, @devdanzin
Files
  • test.py: test: server-side part 1 (auth+redirect)
  • untest.py: test: server-side part 2 (print env of redirected url)
  • bug3819.py: test: client
  • siteminder_3819.py: illustration of the problem using siteminder
  • urllib2-3819.diff: possible fix for removing auth headers
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/orsenthil'
    closed_at = <Date 2010-02-24.16:58:32.946>
    created_at = <Date 2008-09-09.15:12:43.966>
    labels = ['easy', 'type-bug', 'library']
    title = 'urllib2 sends Basic auth across redirects'
    updated_at = <Date 2010-05-19.23:15:10.281>
    user = 'https://bugs.python.org/TFKyle'

    bugs.python.org fields:

    activity = <Date 2010-05-19.23:15:10.281>
    actor = 'kiilerix'
    assignee = 'orsenthil'
    closed = True
    closed_date = <Date 2010-02-24.16:58:32.946>
    closer = 'orsenthil'
    components = ['Library (Lib)']
    creation = <Date 2008-09-09.15:12:43.966>
    creator = 'TFKyle'
    dependencies = []
    files = ['11441', '11442', '11443', '16198', '16204']
    hgrepos = []
    issue_num = 3819
    keywords = ['patch', 'easy']
    message_count = 11.0
    messages = ['72871', '72893', '73424', '76739', '81848', '99152', '99153', '99184', '100044', '100045', '106116']
    nosy_count = 6.0
    nosy_names = ['jjlee', 'orsenthil', 'ajaksu2', 'TFKyle', 'kiilerix', 'dfischer']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue3819'
    versions = ['Python 2.6']

    @TFKyle
    Copy link
    Mannequin Author

    TFKyle mannequin commented Sep 9, 2008

    when you request a url that requests Basic authentication info
    HTTPBasicAuthHandler adds the Authorization header to the request as a
    normal (not unredirected) header, then if the server returns a 301 or
    302 redirect HTTPRedirectHandler will send a request to the redirected
    address keeping the normal headers including the Authorization header
    HTTPBasicAuthHandler added, I'll attach the code I used to test this.

    GET from libwww-perl seems to do this but most browsers don't seem to
    by default and although I can't find much in the RFCs about how
    redirecting is supposed to work wrt. auth headers (feel free to point
    out sections if I'm blind) I think it breaks
    ftp://ftp.isi.edu/in-notes/rfc2617.txt somewhat (section 1.1,
    """
    The protection space determines the domain over which credentials can
    be automatically applied. If a prior request has been authorized, the
    same credentials MAY be reused for all other requests within that
    protection space for a period of time determined by the
    authentication scheme, parameters, and/or user preference. Unless
    otherwise defined by the authentication scheme, a single protection
    space cannot extend outside the scope of its server.
    """) since redirects can point to arbitrary urls off of the server.

    as in bug bpo-1480067 just adding the header as an unredirected header
    would stop the header being sent across redirects if that's indeed the
    proper behaviour.

    @TFKyle TFKyle mannequin added the stdlib Python modules in the Lib dir label Sep 9, 2008
    @orsenthil
    Copy link
    Member

    1. The section you refer to is 1.2 of RFC2617, which specifies the details on
      Access Authentication in General and not specific to url redirects. So, I don't
      think we should take it as a referece.

    2. Under the section - 3.3 Digest Operation, the Authentication cases under
      redirection is provided like this. (search for keyword 'redirect')

    """
    The client will retry the request, at which time the server might respond with a 301/302 redirection, pointing to the URI on the second server. The client will follow the redirection, and pass an Authorization header , including the <opaque> data...
    """

    This basically states that Authorization header should be passed on the
    redirects in Digest authentication case and (should we assume in Basic
    Authentication case also?) If yes, then urllib2 is actually doing the same
    thing. Do you have a practical scenario where this has resulted in failure/
    security loophole?

    @orsenthil
    Copy link
    Member

    This is working as designed and Requestor has not supplied any further
    information on why he thinks it a bug.

    @jjlee
    Copy link
    Mannequin

    jjlee mannequin commented Dec 2, 2008

    I agree this is a bug.

    Senthil -- re "1)", the paragraph you refer to (quoted by the OP) is
    relevant. The fact that it doesn't specifically mention redirection is
    not relevant.

    Re "2)": I don't know how digest auth works, but the paragraph you quote
    from appears to be there for explanation rather than for prescribing how
    digest auth or HTTP work (i.e. it appears to be "non-normative"). This
    bug doesn't say that redirected requests shouldn't contain the
    Authorization header. It says that the Authorization header for an old
    request shouldn't be sent with a new request (though it may turn out the
    new one is equal to the old one in some cases).

    @devdanzin
    Copy link
    Mannequin

    devdanzin mannequin commented Feb 13, 2009

    I think the test is close enough to acceptable, will adapt it if nobody
    does first :)

    @devdanzin devdanzin mannequin added the type-bug An unexpected behavior, bug, or error label Feb 13, 2009
    @devdanzin devdanzin mannequin added the easy label Apr 22, 2009
    @dfischer
    Copy link
    Mannequin

    dfischer mannequin commented Feb 10, 2010

    I believe this bug affects urllib2 when it talks to the corporate single-sign-on solution Siteminder. Siteminder usually is installed as a web server module. When a request is made to the server (origin server), Siteminder issues a 302 redirect to a central authentication server running SSL passing the original request URL of the origin server. The central server responds with a 401 basic authentication challenge. Urllib2 responds with the password from the HTTPPasswordMgr. The central server sets some cookies and responds with a 302 redirect to the origin server on the original URL. Urllib2 then sends the authentication and cookies to the origin server which is virtually always unprotected. Browsers do not send the authentication to the origin server -- only the cookies.

    @orsenthil
    Copy link
    Member

    Ok, in order to fix this bug, urllib2 should only send the cookies and not send the auth info across the the redirects. Yup, let me take this up.

    @orsenthil orsenthil self-assigned this Feb 10, 2010
    @dfischer
    Copy link
    Mannequin

    dfischer mannequin commented Feb 11, 2010

    I attached a diff of a fix for this bug. This may not be the ideal fix, but hopefully it will give the developer who actually does resolve it a good start.

    @orsenthil
    Copy link
    Member

    The Basic Auth Authorization headers are added to unredirected_headers
    and thus we will be able to prevent this problem. ( Digest Authentication was already so, it gives additional confidence that is the right way).

    Fixed and committed in revision 78422.

    @orsenthil
    Copy link
    Member

    merged into other branches r78423, r78426, r78428

    @kiilerix
    Copy link
    Mannequin

    kiilerix mannequin commented May 19, 2010

    FYI, this change caused a regression in Mercurial - see http://mercurial.selenic.com/bts/issue2179.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant