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

captain-caveman2k
Copy link
Contributor

@captain-caveman2k captain-caveman2k 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
Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

captain-caveman2k 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
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.
…sage

Missed in fe20826 as it wasn't implemented in http.c in b4d6db8.
@captain-caveman2k
Copy link
Contributor Author

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 tidy-up branch May 19, 2019 20:27
@jay
Copy link
Member

jay 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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

2 participants