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

http_ntlm_wb: Fix the error handling to align with that of native/SSPI NTLM #3894

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
2 participants
@captain-caveman2k
Copy link
Member

commented May 16, 2019

The current code doesn't clean up properly on failed authentication attempts nor does it handle a 401 error properly.

These fixes were implemented in the native/SSPI NTLM code in b4d6db8, 50b87c4 and fe6049f but not im the winbind code. As such this patch brings the code up to date.

@captain-caveman2k captain-caveman2k changed the title http_ntlm_wb: Align the error handling to that of the native NTLM http_ntlm_wb: Align the error handling to that of the native/SSPI based NTLM May 16, 2019

@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:tidy-up branch from 824d24e to ede110f May 16, 2019

@jay
Copy link
Member

left a comment

To be clear you are changing state here because you would like to cope with retries better? If there is just NTLM by itself isn't it better to be strict here and error?

@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:tidy-up branch from ede110f to 863776e May 17, 2019

@captain-caveman2k

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

No - I appreciate my description was a bit misleading. As such I have reworded it.

But in summary this is to align the error handling of the winbind NTLM to that of the native/SSPI behaviour that was missed in to the three commits that I mentioned above.

  • The first of which cleans up the handshake correctly on a failure (after a type 1 message) - I missed this in fe20826 but it hadn't been implemented in the winbind code as part of b4d6db8.
  • The second cleans up the handshake correctly on a failure (after a type 3 message).
  • The third makes sure that the authentication is only performed for a single request.

@captain-caveman2k captain-caveman2k changed the title http_ntlm_wb: Align the error handling to that of the native/SSPI based NTLM http_ntlm_wb: Fix the error handling to align with that of native/SSPI NTLM May 17, 2019

@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:tidy-up branch from 863776e to a7904ef May 17, 2019

captain-caveman2k added some commits May 18, 2019

http_ntlm_wb: Handle auth for only a single request
Currently when the server responds with 401 on NTLM authenticated
connection (re-used) we consider it to have failed.  However this is
legitimate and may happen when for example IIS is set configured to
'authPersistSingleRequest' or when the request goes thru a proxy (with
'via' header).

Implemented by imploying an additional state once a connection is
re-used to indicate that if we receive 401 we need to restart
authentication.

Missed in fe6049f.
http_ntlm_wb: Return the correct error on receiving an empty auth mes…
…sage

Missed in fe20826 as it wasn't implemented in http.c in b4d6db8.

@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:tidy-up branch from a7904ef to d3a95e1 May 18, 2019

@captain-caveman2k

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

Split into 3 commits to allow the later two to be referenced in the release notes.

You could argue that bd21fc9 should have been in fe20826 really and was my mistake for not digging further.

@captain-caveman2k captain-caveman2k deleted the captain-caveman2k:tidy-up branch May 19, 2019

@jay

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Split into 3 commits to allow the later two to be referenced in the release notes.

Any commit sent upstream ideally should have points of reference, Closes , Fixes , Bug: , Ref: etc. In the future if you're sending multiple commits upstream from a PR (ie not squashing as they're self-contained) it would be helpful to make sure they all have a reference to whatever issue and/or PR so we could find any discussion. Like for example someone bisects and it comes back as xxx it is helpful to know where the issue and PR is to review.

You could argue that bd21fc9 should have been in fe20826 really and was my mistake for not digging further.

We all have had to do follow-ups occasionally, it happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.