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

[release/6.0] Fix compatibility with NTLM authentication to McAfee Web Gateway #66315

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Mar 7, 2022

Backport of #66305 to release/6.0
/cc @wfurt @filipnavara

Fixes #62136

Customer Impact

This is regression introduced in 6.0 that basically prevents use of HTTP(s) in certain network topologies.
It blocks migration to 6.0 and leaves users broken in cases when the transition was forced.
There are several reports from users who are behind McAfee Web Gateway proxy but it is not 100% if this is only impacted scenario. The users are basically forced to always go through the proxy for security reasons. But since the NTLM implementation has some small differences we fail to authenticate and send out HTTP request. There is no known workaround besides downgrading to 5.0.

#54101 added some extra flags to fix NTLM authentication for macOS but the new setting is not compatible with McAfee WG and had unexpected behavior impact.

Testing

I build custom binary and had two customers to verify it. Our test coverage is weak around enterprise authentication and we don't have McAfee Web Gateway available for testing.

Risk

Risk is moderate. This is finicky area with very low test coverage.
This change basically restores previous flags when connections to HTTP proxy.

@ghost
Copy link

ghost commented Mar 7, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #66305 to release/6.0

/cc @wfurt @filipnavara

Customer Impact

Testing

Risk

IMPORTANT: If this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@wfurt wfurt added the Servicing-consider Issue for next servicing release review label Mar 8, 2022
@wfurt wfurt requested a review from rzikm March 8, 2022 01:17
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM

@filipnavara
Copy link
Member

Thanks for the backport and filling up the servicing template!

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 8, 2022
@leecow leecow added this to the 6.0.4 milestone Mar 8, 2022
@mmitche
Copy link
Member

mmitche commented Mar 10, 2022

Please check this in or it will miss the 6.0 servicing window.

@wfurt
Copy link
Member

wfurt commented Mar 10, 2022

We did not do direct checkins for servicing in the past. Did the process changed @mmitche?

@mmitche
Copy link
Member

mmitche commented Mar 10, 2022

We did not do direct checkins for servicing in the past. Did the process changed @mmitche?

No. I don't mean to direct check-in, just please merge asap.

@wfurt
Copy link
Member

wfurt commented Mar 10, 2022

merge is commit, right? For example #64252 I did not merge in the past.

@ericstj
Copy link
Member

ericstj commented Mar 10, 2022

AOT failure is addressed in #66366.
Triggering a rerun of https://dev.azure.com/dnceng/public/_build/results?buildId=1650024&view=logs&j=35d3790f-8c7d-56fd-eac0-8a56c1219de3&t=995e2cd5-5714-5543-343f-237ccb8745a1&s=b15d9194-8f26-5328-b47f-5968c76b37e7, I see @filipnavara filed #65621. The failure looks like it was a DNS issue on the machine when building.

@akoeplinger
Copy link
Member

akoeplinger commented Mar 10, 2022

Looks like runtime-libraries enterprise-linux failed with the same name resolution issue. I actually hit that exact issue a while ago and fixed it by moving from the AzDO Hosted Pool to the 1ES pool: #61001 (comment)

It's possible that they got the same restrictions now and that's why it fails. edit: seems it's not consistent, I see some PRs failing in main and some succeeding so might be part of a rollout or something.

@ericstj
Copy link
Member

ericstj commented Mar 10, 2022

Is it possible there was a yml change that fixed it (eg, to change pools?). Reruns wouldn't pick that up. @wfurt does this pipeline provide coverage of the change here? If so we might want to force a rerun.

edit: I see we have coverage of this pipeline in main. I'm going to go ahead and merge this for now.

@ericstj ericstj merged commit 14ee76b into release/6.0 Mar 10, 2022
@akoeplinger akoeplinger deleted the backport/pr-66305-to-release/6.0 branch March 10, 2022 23:03
@akoeplinger
Copy link
Member

akoeplinger commented Mar 10, 2022

The pool change was backported to 6.0 a long time ago with #63014 and I checked it is using the 1ES pool in this PR, so no idea.

@wfurt
Copy link
Member

wfurt commented Mar 11, 2022

thanks @ericstj. We did not have in-house repro for the issue. The pipeline is somewhat relevant as it provides some guard agains regressions. The main was fixed with #65981. There is still pending issue in core-eng but the changes I made at least make the runtime-libraries enterprise-linux runnable again.

@danmoseley
Copy link
Member

merge is commit, right?

Terminology is likely leaking from time before we used Git...

@ericstj
Copy link
Member

ericstj commented Mar 11, 2022

I queued a couple builds to help us understand what's going on with that leg:
release/6.0-tip: https://dev.azure.com/dnceng/public/_build/results?buildId=1659265&view=results
release/6.0 prior to this change: https://dev.azure.com/dnceng/public/_build/results?buildId=1659273&view=results

@ericstj
Copy link
Member

ericstj commented Mar 11, 2022

@akoeplinger looks like main does have a different pool name:

name: NetCore1ESPool-Public

vs

name: NetCore1ESPool-Svc-Public

@wfurt you made a change here 9174037
Do we nee this in release/6.0 too?

@akoeplinger
Copy link
Member

@ericstj that is expected, servicing branches use the -Svc versions of the pools but afaik that is just an accounting thing, the actual images are the same.

@ghost ghost locked as resolved and limited conversation to collaborators May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants