Skip to content

Windows: default credentials / fallback credential handling#4743

Merged
ethomson merged 3 commits intolibgit2:masterfrom
Agent00Log:dev/winbugfixes
Aug 2, 2018
Merged

Windows: default credentials / fallback credential handling#4743
ethomson merged 3 commits intolibgit2:masterfrom
Agent00Log:dev/winbugfixes

Conversation

@Agent00Log
Copy link
Copy Markdown
Contributor

While trying to use pygit2 with my TFS Git I stumbled accross two problems.

Whenever the server reports that it supports the NTLM and Negotiate authentication schemes both of the enum values are combined with the bitwise or assignment and the subsequent call to WinHttpSetCredentials fails.

The second problem was due to the fact that I had the python code running in PythonWin and it seems that PythonWin is setting the apartment state of the thread to single threaded. In this case the call to CoInitializeEx in the fallback credential function fails with the RPC_E_CHANGED_MODE and the fallback default credentials are never set.

@ethomson
Copy link
Copy Markdown
Member

Whenever the server reports that it supports the NTLM and Negotiate authentication schemes both of the enum values are combined with the bitwise or assignment and the subsequent call to WinHttpSetCredentials fails.

Indeed, the documentation seems clear that this is a single scheme to select. From the MSDN documentation for WinHttpSetCredentials:

An unsigned integer that specifies a flag that contains the authentication scheme.

I'm surprised that I didn't catch this in testing. 😕 Thanks for the report.

The second problem was due to the fact that I had the python code running in PythonWin and it seems that PythonWin is setting the apartment state of the thread to single threaded. In this case the call to CoInitializeEx in the fallback credential function fails with the RPC_E_CHANGED_MODE and the fallback default credentials are never set.

This change makes me more nervous. Ideally, we shouldn't be changing the COM mode out from under our containing process. PythonWin may have selected that carefully and is relying on it for their COM interactions.

I'd propose that instead of calling CoInitializeSecurity that we try the first COM call, and if it errors that COM was not initialized then call CoInitializeSecurity. That way we don't require end-users to set up COM ahead of time, but we also don't stomp all over their choices.

@Agent00Log
Copy link
Copy Markdown
Contributor Author

I don't quite get the problem with the COM initialization. There is no call to CoInitializeSecurity.
Once the concurrency model is set it cannot be changed unless fully uninitialized.

As far as I can see there are four different states:

  • Not initialized
    • The call to CoInitializeEx returns S_OK and the concurrency model is set to MTA
    • After we're finished we call CoUnitialize and everything is back to normal.
  • Already initialized (MTA)
    • The call to CoInitializeEx returns S_FALSE and the init count went one up.
    • After we're finished we call CoUnitialize and the init count is back to the previous value but the COM library is not closed.
  • Already initialized (STA)
    • The call to CoInitializeEx returns RPC_E_CHANGED_MODE which indicates that the thread was previously initialized with a different concurrency model. As far as I can tell we don't require a specific model for our calls to the IInternetSecurityManager.
    • After we're finished we don't call CoUninitialize since the call to CoInitializeEx was not successful.
  • Any other error:
    • Nothing happens, error code 1 is returned.

The change is to accept that some other code already set the concurrency model to something else and just go with it.

@ethomson
Copy link
Copy Markdown
Member

I don't quite get the problem with the COM initialization. There is no call to CoInitializeSecurity.

Sorry; I misspoke and meant CoInitializeEx.

Looking at the MSDN docs, it says that RPC_E_CHANGED_MODE can be returned:

This could also indicate that a change from neutral-threaded apartment to single-threaded apartment has occurred.

So my concern is that there's an opportunity here somewhere to change the model out from underneath somebody. However, it's not clear what the "neutral-threaded apartment" is, and reading this article only made me more confused.

@Agent00Log
Copy link
Copy Markdown
Contributor Author

😊 I know this one gave me a lot of trouble as well.
I think the description for the return value in the MSDN docs is a little bit off.

In the remarks section of the same entry it says:

When an object that is configured to run in the neutral threaded apartment (NTA) is called by a thread that is in either an STA or the MTA, that thread transfers to the NTA. If this thread subsequently calls CoInitializeEx, the call fails and returns RPC_E_CHANGED_MODE.

This sounds a lot more plausible. Otherwise it would be possible to change the model when calling CoInitializeEx inside a neutral object but at the same time get an error that the model could not be changed as the return value.

@ethomson
Copy link
Copy Markdown
Member

ethomson commented Aug 2, 2018

Cool, thanks @Agent00Log!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants