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/5.0] accept empty realm for digest auth (#56369) #61203

Merged
merged 4 commits into from
Nov 8, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 4, 2021

Backport of #56455 to release/5.0

Fixes #56369

/cc @karelz @camillo-toselli

Customer Impact

This breaks web app managing telephone system of the University of Bologna (10K phone lines) - see details in #56369 (comment)
Upgrade to .NET 6.0 (where the issue is fixed) is blocked by missing Oracle EF Core 6 provider.

Note: It is similar to backport of similar issue about empty domain for digest auth - see #50598 which brought that one into 5.0.6 release.

Testing

New targeted test case was added.
Customer validated on Windows10/x64 (on private build locally built).

Risk

Small, because:

  • It is unlikely anyone depends on empty realm to report error.
  • We serviced 5.0 with similar problem with empty domain in [release/5.0] ignore empty domain for digest auth #50598 (we build on that PR).
  • Change is simple and straightforward, so risk of unexpected side effects is small.

@ghost
Copy link

ghost commented Nov 4, 2021

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

Issue Details

Backport of #56455 to release/5.0

/cc @karelz @camillo-toselli

Customer Impact

Testing

Risk

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

area-System.Net

Milestone: -

@karelz karelz added the Servicing-consider Issue for next servicing release review label Nov 4, 2021
@karelz karelz added this to the 5.0.x milestone Nov 4, 2021
@karelz
Copy link
Member

karelz commented Nov 4, 2021

@danmoseley can you please check the request? Any concerns?

@karelz
Copy link
Member

karelz commented Nov 4, 2021

Test failures are unrelated (re-running them):

    System.Diagnostics.Tests.EventLogRecordTests.ExceptionOnce [FAIL]
      System.Diagnostics.Eventing.Reader.EventLogProviderDisabledException : The publisher has been disabled and its resource is not available. This usually occurs when the publisher is in the process of being uninstalled or upgraded.
      Stack Trace:
        /_/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/Reader/EventLogException.cs(36,0): at System.Diagnostics.Eventing.Reader.EventLogException.Throw(Int32 errorCode)
        /_/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/Reader/NativeWrapper.cs(124,0): at System.Diagnostics.Eventing.Reader.NativeWrapper.EvtOpenProviderMetadata(EventLogHandle session, String ProviderId, String logFilePath, Int32 locale, Int32 flags)
        /_/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/Reader/ProviderMetadata.cs(69,0): at System.Diagnostics.Eventing.Reader.ProviderMetadata..ctor(String providerName, EventLogSession session, CultureInfo targetCultureInfo, String logFilePath)
        /_/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/Reader/ProviderMetadataCachedInformation.cs(139,0): at System.Diagnostics.Eventing.Reader.ProviderMetadataCachedInformation.GetProviderMetadata(ProviderMetadataId key)
        /_/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/Reader/ProviderMetadataCachedInformation.cs(218,0): at System.Diagnostics.Eventing.Reader.ProviderMetadataCachedInformation.GetLevelDisplayName(String ProviderName, EventLogHandle eventHandle)
        /_/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/Reader/EventLogRecord.cs(319,0): at System.Diagnostics.Eventing.Reader.EventLogRecord.get_LevelDisplayName()
        /_/src/libraries/System.Diagnostics.EventLog/tests/System/Diagnostics/Reader/EventLogRecordTests.cs(102,0): at System.Diagnostics.Tests.EventLogRecordTests.<>c__DisplayClass4_1.<ExceptionOnce>b__0()
        /_/src/libraries/System.Diagnostics.EventLog/tests/System/Diagnostics/Reader/EventLogRecordTests.cs(116,0): at System.Diagnostics.Tests.EventLogRecordTests.ThrowsMaxOnce[T](Action action)
        /_/src/libraries/System.Diagnostics.EventLog/tests/System/Diagnostics/Reader/EventLogRecordTests.cs(102,0): at System.Diagnostics.Tests.EventLogRecordTests.ExceptionOnce()
Console log: 'JIT.Regression.CLR-x86-JIT.V1-M12-M13' from job 8ac834ff-904e-4ab3-96b9-a96d6f30e073 workitem 8ff53c12-fb10-41d0-9568-e320d2ce8f35 (windows.10.arm64v8.open) executed on machine DDARM64-179

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 4, 2021
@leecow leecow modified the milestones: 5.0.x, 50.13, 5.0.13 Nov 4, 2021
@danmoseley
Copy link
Member

Approved once customer verifies fix.

Curious why this didn't show up when customer used #50598 fix?

@wfurt
Copy link
Member

wfurt commented Nov 4, 2021

Curious why this didn't show up when customer used #50598 fix?

It was different reporter so perhaps different server with different behavior @danmoseley. When the original fix was done, we really just fix the reported issue. AFAIK we did not do any extensive search through RFC for similar cases.

@karelz karelz added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 4, 2021
@karelz
Copy link
Member

karelz commented Nov 4, 2021

(marking it NO MERGE to block on customer validation)

@karelz
Copy link
Member

karelz commented Nov 5, 2021

Rerun test failures are again unrelated:

Console log: 'JIT.Methodical.a-dA-D' from job 815d9fa9-2c56-435c-8400-8b4dfb82a69f workitem e007438e-7141-4761-ba97-987e1412c1ad (windows.10.arm64v8.open) executed on machine DDARM64-179

It is the same machine DDARM64-179 as above in #61203 (comment) - @dotnet/dncenghot is that a known machine problem perhaps?

@karelz
Copy link
Member

karelz commented Nov 5, 2021

Removing NO MERGE as customer validated the bits, see #56369 (comment)

@karelz karelz removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 5, 2021
@lpatalas
Copy link

lpatalas commented Nov 5, 2021

It is the same machine DDARM64-179 as above in #61203 (comment) - @dotnet/dncenghot is that a known machine problem perhaps?

I'll take a look at that machine.

@lpatalas
Copy link

lpatalas commented Nov 5, 2021

I have put that machine offline and created an issue to track further work: https://github.com/dotnet/core-eng/issues/14861

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@karelz
Copy link
Member

karelz commented Nov 8, 2021

The remaining CI failure is unrelated -- Build Analysis warns that image of Ubuntu 16.04 is not available for "runtime-libraries enterprise-linux":

runtime-libraries enterprise-linux 20211104.1 / Job
❌ An image label with the label ubuntu-16.04 does not exist.

It is not related to this PR and would not help if it worked.
@wfurt is it something we should look into?

@danmoseley the PR is ready for merge now.

@Anipik Anipik merged commit 9c5ef08 into release/5.0 Nov 8, 2021
@MattGal
Copy link
Member

MattGal commented Nov 8, 2021

The remaining CI failure is unrelated -- Build Analysis warns that image of Ubuntu 16.04 is not available for "runtime-libraries enterprise-linux":

runtime-libraries enterprise-linux 20211104.1 / Job
❌ An image label with the label ubuntu-16.04 does not exist.

It is not related to this PR and would not help if it worked. @wfurt is it something we should look into?

@danmoseley the PR is ready for merge now.

This is definitely worth fixing, since that error means an entire pipeline didn't run.

Images are listed here: https://github.com/actions/virtual-environments and Ubuntu 16.04 was removed because it is past its end-of-life support window.

Simply changing ubuntu-16.04 to ubuntu-18.04, ubuntu-20.04, or ubuntu-latest will bring the pipeline back online.

@danmoseley
Copy link
Member

@Anipik please see Matt's note above. Seems like we need a yaml tweak in this branch

@danmoseley danmoseley deleted the backport/pr-56455-to-release/5.0 branch November 8, 2021 21:12
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants