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

HttpListenerContext.AcceptWebSocketAsync unsupported for IWindowsPrincipal in .NET Core 3.1 #63738

Closed
iconics-janb opened this issue Jan 13, 2022 · 7 comments · Fixed by #65746
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@iconics-janb
Copy link

iconics-janb commented Jan 13, 2022

Description

HttpListenerContext.AcceptWebSocketAsync throws PlatformNotSupportedException when called with User set to WindowsPrincipal. This happens in .NET Core 3.1 when running on Windows. Works fine when running in .NET Framework.

Reproduction Steps

using System;
using System.Diagnostics;
using System.Net;
using System.Net.WebSockets;
using System.Threading;
using System.Threading.Tasks;

namespace WebSocketCloseTest
{
    public class Program
    {
        public static async Task Main(string[] args)
        {
            var serverTask = Server(http://localhost:45000/test/ws/);
            await Client("ws://localhost:45000/test/ws/");
            await serverTask;
        }

        private static async Task Server(String serverAddress)
        {
            var listener = new HttpListener();
            listener.Prefixes.Add(serverAddress);
            listener.AuthenticationSchemes = AuthenticationSchemes.IntegratedWindowsAuthentication;
            listener.Start();

            HttpListenerContext ctx = await listener.GetContextAsync();
            try
            {
                Debug.Assert(ctx.User != null); // this is supposed to be a WindowsPrincipal
                HttpListenerWebSocketContext wsCtx = await ctx.AcceptWebSocketAsync("subProtocol"); // this throws on .NET Core
                await wsCtx.WebSocket.CloseAsync(WebSocketCloseStatus.NormalClosure, null, CancellationToken.None);
            }
            finally
            {
                ctx.Response.Close();
            }
        }

        private static async Task Client(String serverAddress)
        {
            try
            {
                using (ClientWebSocket webSocket = new ClientWebSocket())
                {
                    webSocket.Options.AddSubProtocol("subProtocol");
                    webSocket.Options.Credentials = CredentialCache.DefaultCredentials;
                    await webSocket.ConnectAsync(new Uri(serverAddress), CancellationToken.None);
                    await webSocket.CloseAsync(WebSocketCloseStatus.NormalClosure, null, CancellationToken.None);
                }
            }
            catch (Exception e)
            {
                Console.WriteLine($"Client WS exception: {e.Message}");
            }
        }
    }
}

Expected behavior

It should not throw the exception and work the same way as it does in .NET Framework

Actual behavior

Throws PlatformNotSupportedException.

Regression?

Yes, works in .NET Framework

Known Workarounds

Use reflection to set the _user field to null, make the call and set it back to the original value, like this:

HttpListenerContext ctx = await listener.GetContextAsync();

IPrincipal originalPrincipal = ctx.User;
FieldInfo field = ctx.GetType().GetField("_user", BindingFlags.NonPublic | BindingFlags.Instance);
field.SetValue(_ctx, null);

// make the call with User = null to prevent the exception
wsCtx = await ctx.AcceptWebSocketAsync(WsConstants.SubProtocol, TimeSpan.FromSeconds(WsConstants.KeepAliveIntervalSeconds));

// revert to the original value
field.SetValue(_ctx, originalPrincipal);

Configuration

.NET Core 3.1, Windows 10 x64

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Net untriaged New issue has not been triaged by the area owner labels Jan 13, 2022
@ghost
Copy link

ghost commented Jan 13, 2022

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

Issue Details

Description

HttpListenerContext.AcceptWebSocketAsync throws PlatformNotSupportedException when called with User set to IWindowsPrincipal. This happens in .NET Core 3.1 when running on Windows. Works fine when running in .NET Framework.

Reproduction Steps

HttpListenerContext ctx = await listener.GetContextAsync();
wsCtx = await ctx.AcceptWebSocketAsync(WsConstants.SubProtocol, TimeSpan.FromSeconds(WsConstants.KeepAliveIntervalSeconds));

Expected behavior

It should not throw the exception and work the same way as it does in .NET Framework

Actual behavior

Throws PlatformNotSupportedException.

Regression?

Yes, works in .NET Framework

Known Workarounds

Use reflection to set the _user field to null, make the call and set it back to the original value, like this:

HttpListenerContext ctx = await listener.GetContextAsync();

IPrincipal originalPrincipal = ctx.User;
FieldInfo field = ctx.GetType().GetField("_user", BindingFlags.NonPublic | BindingFlags.Instance);
field.SetValue(_ctx, null);

// make the call with User = null to prevent the exception
wsCtx = await ctx.AcceptWebSocketAsync(WsConstants.SubProtocol, TimeSpan.FromSeconds(WsConstants.KeepAliveIntervalSeconds));

// revert to the original value
field.SetValue(_ctx, originalPrincipal);

Configuration

.NET Core 3.1, Windows 10 x64

Other information

No response

Author: iconics-janb
Assignees: -
Labels:

area-System.Net, untriaged

Milestone: -

@karelz
Copy link
Member

karelz commented Jan 13, 2022

Can you give us simple self-contained repro we can run? (I assume the above code snippet is not sufficient)
Can you please test it also on .NET 6?

@iconics-janb
Copy link
Author

iconics-janb commented Jan 14, 2022 via email

@iconics-janb
Copy link
Author

iconics-janb commented Jan 14, 2022 via email

@CarnaViire
Copy link
Member

Indeed, as @iconics-janb pointed out, our code will throw PNSE if user is WindowsPrincipal:


but .NET Framework code contained an implementation for the same condition
https://referencesource.microsoft.com/#System/net/System/Net/WebSockets/HttpListenerWebSocketContext.cs,150

@wfurt @stephentoub Do you know anything that would prevent us from reusing the original code with WindowsPrincipal existing in .NET Framework? I wonder why it was not used in the first place, and why PNSE was used (The PR that introduced it (dotnet/corefx#20398 (comment)) doesn't help). I would expect that user couldn't be WindowsPrincipal on non-Windows platform in the first place - am I missing something?

@wfurt
Copy link
Member

wfurt commented Jan 14, 2022

Yes, I think we can make it work for Windows (assuming all the dependencies are in place) and PNSE for other platforms.
The original exception was "not implemented" so int feels just something that got dropped from the initial implementation. (and perhaps lost)

@karelz
Copy link
Member

karelz commented Jan 18, 2022

Triage: Needs to be ported from .NET Framework. Fairly straightforward work - see link to referencesource above.
Given that this is 1st report in .NET Core timeframe, moving it to Future. We would take a contribution though.

@karelz karelz added this to the Future milestone Jan 18, 2022
@karelz karelz added enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Jan 18, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 22, 2022
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Mar 15, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 15, 2022
@karelz karelz modified the milestones: Future, 7.0.0 Apr 8, 2022
@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 enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants