Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Adding fallback logic to NTLM#5770

Closed
shrutigarg wants to merge 1 commit into
dotnet:dev/negotiatestreamfrom
shrutigarg:devNegoFinal
Closed

Adding fallback logic to NTLM#5770
shrutigarg wants to merge 1 commit into
dotnet:dev/negotiatestreamfrom
shrutigarg:devNegoFinal

Conversation

@shrutigarg
Copy link
Copy Markdown
Contributor

This includes
implementing the NTLM logic for OSX/Linux.
fallback logic to ntlm if Kerberos fails.

tested locally as linux client against windows server.
ongoing testing with OSX client , will update here once testing is done.

Please refer to commit 5 for the changes.. other commits are maintained and reviewed separately in separate PRs.
cc @stephentoub @bartonjs @vijaykota

@vijaykota
Copy link
Copy Markdown
Contributor

cc: @joshfree for tracking #2483

This PR is dependent on other PRs - dev/negotiatestream equivalents of #5500, #5772 and #5773 Hence adding the "needs extra testing" label

@vijaykota
Copy link
Copy Markdown
Contributor

Also depends on the code of #5728. Without the pre-requisite PRs, the build is expected to fail

@stephentoub
Copy link
Copy Markdown
Member

Shouldn't those PRs be closed and new ones opened to this branch so they can be merged in?

@shrutigarg
Copy link
Copy Markdown
Contributor Author

yes. in progress of moving those here.

@stephentoub
Copy link
Copy Markdown
Member

Ok, good, thanks. I misunderstood the comments about the dependencies.

@vijaykota
Copy link
Copy Markdown
Contributor

Yes.. the real dependencies are the equivalent PRs. We will update those no.s once the move is complete

@shrutigarg
Copy link
Copy Markdown
Contributor Author

updated the PR numbers (of all dependent PRs) for this branch

@shrutigarg
Copy link
Copy Markdown
Contributor Author

Ping @stephentoub @bartonjs @vijaykota

@bartonjs
Copy link
Copy Markdown
Member

I looked only at commits 5 and higher. I'm not quite understanding how this is supposed to work, though. If this PR is merged then the other commits also get merged. Since they're not being reviewed here that seems bad.

Some of the Heimdal references didn't seem familiar, and I was having a hard time searching them out. So... I guess I'm really confused as to the macro-plan here.

@vijaykota
Copy link
Copy Markdown
Contributor

If this PR is merged then the other commits also get merged

We have labelled it with "needs-extra-testing" to prevent such a merge. Once this PR's dependencies are merged, we will do a rebase and remove the label.

@vijaykota vijaykota added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) and removed Needs extra testing before merge labels Feb 19, 2016
@stephentoub
Copy link
Copy Markdown
Member

synced and rebased

CI is reporting there are still merge conflicts.

@shrutigarg
Copy link
Copy Markdown
Contributor Author

@stephentoub That is supposed to fail as this PR depends on the shim changes of #5919 .. This will go after #5919 goes in.

@stephentoub
Copy link
Copy Markdown
Member

That is supposed to fail

I'm not saying the problem is that they're failing. I'm saying it's reporting merge conflicts:

GitHub pull request #5770 of commit 7fa5d82a155bde1d6b6d7d6dbbcfb9ff68af8ccf, has merge conflicts.

Why is that expected?

@shrutigarg
Copy link
Copy Markdown
Contributor Author

I don't know why it is coming only in *Ubuntu Debug *.. on other flavors it is fine.. not sure what needs to be done at my end,.

@vijaykota
Copy link
Copy Markdown
Contributor

Closing this since we don't want to pursue the libheimntlm option. The fallback logic here is closely tied to it.

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

Labels

area-System.Net * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants