Resolve concurrency issue

[mkomitee]

Here's another go at it. The biggest differences between this and #114 are:

• The changes to generate_request_header should be less invasive.
• We avoid the memory leak by dropping the established context (still indexed by prepared request) once we're done using it.
• We add back the winrm functions and have them raise more helpful error messages than the attribute errors we would have raised before.
• We document the winrm support being dropped in the history file
• We bump the version (minor version, since we're on 0.x).

[mkomitee]

To be clear, I don't have access to a full kerberos infrastructure at the moment to test this, so it's possible that it's entirely broken. I intend to test it as soon as I'm able, but I'm interested in feedback.

[nitzmahone]

a) removing the winrm stuff without a deprecation period has the same effect regardless of how nice the error messages are: people's stuff is broken

b) thinking about it a bit more, I'm not even sure this will work properly- doesn't requests pool the underlying connections? If so, when there's more than one open to a given host, how will you ensure the right pooled connection with the state matching a given context is used to send the next request?

[mkomitee]

a) removing the winrm stuff without a deprecation period has the same effect regardless of how nice the error messages are: people's stuff is broken

How do you recommend we proceed / how long should we leave things broken so that people can update their code?

b) thinking about it a bit more, I'm not even sure this will work properly- doesn't requests pool the underlying connections? If so, when there's more than one open to a given host, how will you ensure the right pooled connection with the state matching a given context is used to send the next request?

Maybe I'm missing something, but why does that matter? Our object w/ the self.context dict will get called with the response either way, we can extract the prepared request from the response, and index into self.context to extract the ctx to finish up. Then we add the ctx to the response and pop it off of self.context.

[nitzmahone]

How do you recommend we proceed / how long should we leave things broken so that people can update their code?

I'm still not seeing why you need to kill the winrm encryption support immediately to make this work. At a glance, I don't think what you're proposing will work with any Microsoft origin server for more than one (logical) request, especially if there's more than one connection to that server in the pool.

Maybe I'm missing something, but why does that matter? Our object w/ the self.context dict will get called with the response either way, we can extract the prepared request from the response, and index into self.context to extract the ctx to finish up. Then we add the ctx to the response and pop it off of self.context.

The context is only half the problem- the underlying connection itself has to have the same state as the context object you're using to talk to it (as associated by the origin server). Each connection has its own session key- remember, you don't even have to pass the Authorization header on subsequent requests- it's all RFC-noncompliant behavior... IIRC, if you use the wrong context to generate a subsequent Authorization header on a connection that doesn't match, the server will just abruptly close the connection.

The problem is sort of self-solving when there's only one connection to each origin in the pool, but I don't recall how that gets handled when it's not (ie, what the pooling discriminators are or in what scope the pools are stored). In any case, all I'm saying is that I don't think there's a solution to the threaded case that wouldn't require the caller to do something different, so we should be able to accommodate the existing behavior and some TBD new behavior, at least temporarily.

[mkomitee]

When it comes to normal HTTP, from the servers perspective, there is no need for multi-round-trip state.

The server receives an unauthenticated request and responds with a 401. At that point in time there's no state server-side.

It then receives an authenticated request, verifies the client auth data from the header, processes the request, then generates a response token and sends it back in the response headers --- then drops the kerberos context.

It's only the client that maintains state after it generates it's token and sends it to the server. It can then drop it after it's authenticated the server.

In the case of winrm, it's not a generic http server, and it must maintain state in some way. I'm guessing a session cookie.

To make winrm work, we need to keep the established context alive forever in the authentication object indexed by hostname for all requests, which means a leak and if we have threads, we inherently have a race condition.

edit: A second request to the same server will blow away the previously created context, even if the first one is still waiting on a response from the server -- and the new context won't be able to authenticate the server's response to the first context.

Also, since there's no configuring the auth object to indicate that it's going to be used for winrm, we have to do this whether winrm is going to be used or not.

[nitzmahone]

This definitely fixes the issue with overlapping threaded requests to the same host with a shared session, and exposing the context on the response makes it theoretically possible for external code to use it for future body computation.

Kerberos seems to squeak by the pool > 1 issue; I've never been able to trigger more than one round-trip for the auth exchange. The "specs" for this auth allow for it, and others like NTLM/CredSSP that use a similar pattern definitely do require >1 roundtrip with connection-tied state, so those may never be reliable in a case where the pool has >1 connection. I think that also implies that an NTLM mode/fork like you were talking about in #89 (comment) wouldn't work with pool >1.

There's still a couple things broken for pool size > 1 - I'm guessing you won't be able to do anything about them though, since requests/urllib3 doesn't really handle stateful connections... Since the pooling appears to be auth-agnostic but shared at the session level, any request that overrides the session-defined auth (eg session.get('https://mykerbhost.com/', auth=HTTPKerberosAuth(...))) is not guaranteed to use the settings passed- it may use whatever's laying around in the pool (thus inheriting someone else's credentials). Use of an explicit per-request auth handler instance with force_preemptive=True could work around this, but looking at it in terms of threaded usage from a shared Session, force_preemptive=True is also broken there, since the state is stored in the auth handler itself (and not even per host, so double lame on me for that- yet another bug).

In light of all this, I'm going to recommend that pywinrm just embed a customized copy of requests-kerberos; perhaps what we should have done all along. It'll make keeping things working long-term much easier, and free you to do whatever you want here. Would appreciate it if you'd hold off releasing this until we can get a pywinrm release out with the embedded copy, or at least one with a pinned requests-kerberos version that works.

[jborean93]

@mkomitee I'm trying to modernise the library by removing the WinRM cruft (now that pywinrm has their own vendored copy) and updating the auth library that is used away from pykerberos and winkerberos. The PR #163 will do this and tries to hide away the context from external use so this just focuses on the authentication part. If you are still interested in trying to get this fix in you can update the PR and I'll give it a review otherwise I'll close this in a weeks time.

Just as an FYI my modernising is really just to have it be buildable on more modern systems (now that pykerberos is failing) rather than having it add new features. I'm trying to keep this purely as a Kerberos authentication provider for requests and nothing more.

[mkomitee]

awesome. i'll get to it asap. 

On October 30, 2021, Frost y ***@***.***> wrote: @mkomitee <https://github.com/mkomitee> I'm trying to modernise the library by removing the WinRM cruft (now that pywinrm has their own vendored copy) and updating the auth library that is used away from pykerberos and winkerberos. The PR #163 <#163> will do this and tries to hide away the context from external use so this just focuses on the authentication part. If you are still interested in trying to get this fix in you can update the PR and I'll give it a review otherwise I'll close this in a weeks time.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#133 (comment)- 955602528>, or unsubscribe <https://github.com/notifications/unsubscribe- auth/AAAAGOS6L64VXJBYOTA5JOLUJRW6FANCNFSM4H4WTXEQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification- email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification- email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[jborean93]

@mkomitee just closing this PR to clear out the queue, if you do eventually get back to this feel free to re-open this or open a new PR.