Uh oh!
There was an error while loading. Please reload this page.
Enable ExtendedProtectionPolicy in generic System.Net.Security build - #88871
Conversation
ghost
commented
Jul 13, 2023
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
| { | ||
| // .NET Core is supported only on Win7+ where ExtendedProtection is supported. | ||
| return true; | ||
| return OperatingSystem.IsWindows(); |
There was a problem hiding this comment.
This is controversial. I can change it to !OperatingSystem.IsBrowser() to be on the safe side and behave like older .NET versions did. However, the reality is that it's not really implemented on Linux/macOS either. Some bits and pieces may have effect (eg. SPN validation for NTLM Negotiation) but others are blatantly ignored (https://github.com/dotnet/runtime/pull/87930/files#r1263108882).
There was a problem hiding this comment.
It's better to say it's not supported when there's known gaps than to say it's supported and have the consumer believe ExtendedProtection is being used when it's actually not. It's off by default, so if you explicitly turn it on, it should actually do the expected thing.
There was a problem hiding this comment.
I think this is OK. I feel it is better to claim false if the functionality is not clear or complete.
wfurt
left a comment
There was a problem hiding this comment.
Generally looks good to me.
===========================================================================================================
/private/tmp/helix/working/ADA60906/w/A14A08D2/e /private/tmp/helix/working/ADA60906/w/A14A08D2/e
Discovering: System.Net.Security.Unit.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Net.Security.Unit.Tests (found 73 of 76 test cases)
Starting: System.Net.Security.Unit.Tests (parallel test collections = on, max threads = 6)
System.Net.Security.Tests.ExtendedProtectionPolicyTest.ExtendedProtectionPolicy_OSSupportsExtendedProtection [FAIL]
Assert.True() Failure
Expected: True
Actual: False
Stack Trace:
/_/src/libraries/System.Net.Security/tests/UnitTests/System/Security/Authentication/ExtendedProtection/ExtendedProtectionPolicyTest.cs(62,0): at System.Net.Security.Tests.ExtendedProtectionPolicyTest.ExtendedProtectionPolicy_OSSupportsExtendedProtection()
/_/src/mono/System.Private.CoreLib/src/System/Reflection/MethodInvoker.Mono.cs(30,0): at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, IntPtr* args)
/_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs(59,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
System.Net.Security.Tests.NegotiateAuthenticationTests.Package_Unsupported_NTLM [SKIP]
Condition(s) not met: "IsNtlmUnavailable"
Finished: System.Net.Security.Unit.Tests
=== TEST EXECUTION SUMMARY ===
seems like test failures are related - because of updated OSSupportsExtendedProtection
filipnavara
commented
Jul 17, 2023
I'll check. I missed it in all the test noise. |
filipnavara
commented
Jul 17, 2023
Fixed. |
wfurt
commented
Jul 17, 2023
test failures unrelated. |
Fixes#77405