Asynchronous SSLSTREAM

Question

Recently I have written an SslStream class asynchronously authenticate clients and receive message from them. I still would like anyone to suggest improvements for my code.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Net;
using System.Net.Sockets;
using System.Net.Security;
using System.Security.Authentication;
using System.Security.Cryptography.X509Certificates;
using System.IO;
public class Wrapper
{
 public byte[] buffer;
 public SslStream sslStream;
 public object connector;
}
public class Sock
{
 private Dictionary<string, byte> Connections;
 public event Action<Wrapper> AnnounceNewConnection;
 public event Action<Wrapper> AnnounceDisconnection;
 public event Action<byte[], Wrapper> AnnounceReceive;
 private Socket _sock;
 private X509Certificate certificate = X509Certificate.CreateFromCertFile("exportedcertificate.cer");
 public Sock(int port)
 {
 try
 {
 Connections = new Dictionary<string, byte>();
 _sock = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
 _sock.Bind(new IPEndPoint(IPAddress.Any, port));
 _sock.Listen(500);
 _sock.BeginAccept(AcceptConnections, new Wrapper());
 }
 catch (Exception e)
 {
 Console.WriteLine(e);
 }
 }
 private void AcceptConnections(IAsyncResult result)
 {
 Wrapper wr = (Wrapper)result.AsyncState;
 try
 {
 wr.sslStream = new SslStream(new NetworkStream(_sock.EndAccept(result), true));
 wr.sslStream.BeginAuthenticateAsServer(certificate, EndAuthenticate, wr);
 _sock.BeginAccept(AcceptConnections, new Wrapper());
 }
 catch (Exception e) { Console.WriteLine(e); AnnounceDisconnection.Invoke(wr); wr.sslStream.Close(); wr.sslStream.Dispose(); }
 }
 private void EndAuthenticate(IAsyncResult result)
 {
 Wrapper wr = (Wrapper)result.AsyncState;
 try
 {
 try { wr.sslStream.EndAuthenticateAsServer(result); }
 catch { }
 if (wr.sslStream.IsAuthenticated)
 {
 AnnounceNewConnection.Invoke(wr);
 if (wr.sslStream.CanRead)
 {
 wr.buffer = new byte[5];
 wr.sslStream.BeginRead(wr.buffer, 0, wr.buffer.Length, ReceiveData, wr);
 }
 }
 else
 {
 AnnounceDisconnection.Invoke(wr); wr.sslStream.Close(); wr.sslStream.Dispose();
 }
 }
 catch (Exception e) { Console.WriteLine(e); AnnounceDisconnection.Invoke(wr); wr.sslStream.Close(); wr.sslStream.Dispose(); }
 }
 private void ReceiveData(IAsyncResult result)
 {
 Wrapper wr = (Wrapper)result.AsyncState;
 try
 {
 AnnounceReceive.Invoke(wr.buffer, wr);
 SocketError error = SocketError.Disconnecting;
 int size = wr.sslStream.EndRead(result);
 if (error == SocketError.Success && size != 0)
 {
 wr.buffer = new byte[size];
 wr.sslStream.BeginRead(wr.buffer, 0, wr.buffer.Length, ReceiveData, wr);
 }
 else
 {
 wr.sslStream.Close();
 wr.sslStream.Dispose();
 AnnounceDisconnection.Invoke(wr);
 }
 }
 catch (Exception e) { Console.WriteLine(e); AnnounceDisconnection.Invoke(wr); wr.sslStream.Close(); wr.sslStream.Dispose(); }
 }
}

Also, how can I verify server authentication?

Answer 1

I can't comment on how secure your code is, however there are a couple of other problems that I would like to point out.

Clutter

Your using directives can be cut down to:

using System;
using System.Net;
using System.Net.Sockets;
using System.Net.Security;
using System.Security.Cryptography.X509Certificates;

Naming

• Let me show you what I think of when I see a class called Sock:
  socks
  You need to give that class a better name, for instance SslSocketMonitor. Actually, I don't know enough about your class to find a good name; use your brain to find a better one.

• Let me show you what I think of when I see a class called Wrapper:
  enter image description here
  Because you are using it as a parameter to an event, this ought to be called something like SslSocketEventArgs, but certainly not Wrapper.

• Decide on one naming convention and stick to it. You are using three different conventions for naming private fields:

  private Dictionary<string, byte> Connections; // UpperCase
  private Socket _sock; // _lowerCasePrefixed
  private X509Certificate certificate; // lowerCase
  

• Your event names are nonstandard and not particularly clear.
  Instead of AnnounceNewConnection try Connected,
  instead of AnnounceDisconnection try Disconnected,
  and instead of AnnounceReceive use ReceivedData.

API problems

• .NET programmers are familiar with events. Therefore, it makes sense to implement the pattern properly:

  1. SslSocketEventArgs (formerly Wrapper) needs to be derived from System.EventArgs

  2. Instead of using Action:

     public event Action<Wrapper> AnnounceNewConnection;
     public event Action<Wrapper> AnnounceDisconnection;
     public event Action<byte[], Wrapper> AnnounceReceive;
     

     use the more expressive and conventional EventHandler<T>:

     public event EventHandler<SslSocketEventArgs> Connected;
     public event EventHandler<SslSocketEventArgs> Disconnected;
     public event EventHandler<SslSocketEventArgs> ReceivedData;
     

  3. It's not safe to directly invoke event handlers as they could easily be null. So define the following extension method to safely invoke the event handlers:

     public static class EventHandlerExtensions
     {
      public static void InvokeSafely<T>(this EventHandler<T> eventHandler, 
      object sender, T eventArgs) where T : EventArgs
      {
      if (eventHandler != null)
      {
      eventHandler(sender, eventArgs); // syntactic sugar for .Invoke
      }
      }
     }
     

  and replace all the usages:

  Disconnected.InvokeSafely(this, wr);
  Received.InvokeSafely(this, wr); // etc...
  

• I'm not a fan of beginning listening in the constructor. It would be better to invoke that explicitly in a separate method.

• There should be a way to cancel listening. Write a public method to enable this.

Programming errors

• Your field Dictionary<string, byte> Connections is initialized but never used. Delete it.

• You have lines like these spread throughout your code:

  wr.sslStream.Close();
  wr.sslStream.Dispose();
  

  That scares me. What if you forgot to close it somewhere? It would be best to enclose all uses of the class in using statements where possible.

• In ReceiveData, you make the following (constant) assignment:

  SocketError error = SocketError.Disconnecting;
  

  So the following condition will always be false:

  if (error == SocketError.Success && size != 0)
  

  What you are probably trying to do is instead of catch (Exception):

  catch (SocketException e)
  {
   // handle properly!
   Console.WriteLine(e.SocketErrorCode);
  }
  

• dreza: haha sock indeed

Answer 2

I like the layout

First thing, move your using statements out of the namespace declaration, and remove unused references (I use Resharper, but there is a command in VS that will do this for you, I just can't remember it):

using System;
using System.Collections.Generic;
using System.Net;
using System.Net.Sockets;
using System.Net.Security;
using System.Security.Cryptography.X509Certificates;

I don't see Wrapper used enough to comment on the class, but the names, if public should begin with capital letters.

In the Sock class:

Connections does not follow C# naming conventions. It should be named _connections;

_sock can be made readonly. This will prevent accidental overwrite, and I believe uses less resources when compiled. I would also rename it to _socket to avoid confusion in your class.

certificate could be made readonly, and I'd initialize it in the constructor like all your other class variables. It should also be renamed to _certificate. The alternative is to make it static, which might be a better idea because it looks like it needs to stay in memory. In which case, it should be renamed Certificate.

I would move the initialzation of _sock into it's own method. This will clean up your constructor a little, and portray intent much better.

_socket = InitializeSocket(port);
....
private static InitializeSocket(int port)
{
 var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
 socket.Bind(new IPEndPoint(IPAddress.Any, port));
 socket.Listen(500);
 socket.BeginAccept(AcceptConnections, new Wrapper());
 return socket;
}

The listen value should be put into a class constant instead of using a magic number. This will allow for easier changes later, and makes the initialization easier to read.

private const int ListenLength = 500;
...
socket.Listen(ListenLength);

In AcceptConnections, wr should be named to resultWrapper. I would also use the var keyword because of the boxing. This can be done in all your methods.

var resultWrapper= (Wrapper)result.AsyncState;

Add line breaks and whitespace to your catch statement. Makes it easier to debug.

I don't like that you have to inject a Wrapper into the call within a Wrapper:

resultWrapper.sslStream.BeginAuthenticateAsServer(certificate, EndAuthenticate, resultWrapper);

I would put a method in the Wrapper class call BeginAuthenticateAsServer:

public void BeginAuthenticateAsServer(X509Certificate certificate, AsyncCallback endAuthenticate)
{
 sslStream.BeginAuthenticateAsServer(certificate, endAuthenticate, this);
}

Then your call will be

resultWrapper.BeginAuthenticateAsServer(Certificate, EndAuthenticate);

Your call to AnnounceDisconnection will fail if AccounceDisconnection is null. Instead, add a method called Disconnect:

private void Disconnect(Wrapper wrapper)
{
 if (AnnounceDisconnection == null)
 {
 return;
 }
 AnnounceDisconnection.Invoke(wrapper);
}

Then in your code where its needed, call Disconnect

This line

try { wr.sslStream.EndAuthenticateAsServer(result); }
catch { }

Hides an exception. Not a very good idea. Either log it, retry, or let the application error handling deal with it. This will cause confusion in the future.

AnnounceReceive - See solution for AnnounceDisconnection;

Try this and see how it looks. I have a few more ideas, but this is a good start.

EDIT

I have taken CodeSparkle's suggestions and updated the code, with some of my ideas

The old Wrapper class:

public class SslSocketEventArgs : EventArgs
{
 private static readonly X509Certificate Certificate =
 X509Certificate.CreateFromCertFile("exportedcertificate.cer");
 private SslStream _sslStream;
 public byte[] Buffer { get; private set; }
 public void ReplaceSslStream(IAsyncResult result, Socket socket)
 {
 if (_sslStream != null)
 {
 CloseAndDispseSslStream();
 }
 _sslStream = new SslStream(new SslStream(new NetworkStream(socket.EndAccept(result), true)));
 }
 public void CloseAndDispseSslStream()
 {
 if (_sslStream == null)
 {
 return;
 }
 _sslStream.Close();
 _sslStream.Dispose();
 }
 public void BeginAuthenticateAsServer(AsyncCallback endAuthenticate)
 {
 _sslStream.BeginAuthenticateAsServer(Certificate, endAuthenticate, this);
 }
 public void EndAuthenticateAsServer(IAsyncResult result)
 {
 _sslStream.EndAuthenticateAsServer(result);
 }
 public void CreateBuffer(int size)
 {
 Buffer = new byte[size];
 }
 public void BeginRead(AsyncCallback receiveData)
 {
 if (Buffer == null)
 {
 throw new ApplicationException("Buffer has not been set.");
 }
 _sslStream.BeginRead(Buffer, 0, Buffer.Length, receiveData, this);
 }
 public int EndRead(IAsyncResult result)
 {
 return _sslStream.EndRead(result);
 }
 public bool IsAuthenticated()
 {
 return _sslStream.IsAuthenticated;
 }
 public bool CanRead()
 {
 return _sslStream.CanRead;
 }
}

The old Sock class:

public class SslSocketMonitor
{
 private const int ListenLength = 500;
 public event EventHandler<SslSocketEventArgs> Connected;
 public event EventHandler<SslSocketEventArgs> Disconnected;
 public event EventHandler<SslSocketEventArgs> ReceivedData;
 private readonly Socket _socket;
 public SslSocketMonitor(int port)
 {
 try
 {
 _socket = InitializeSocket(port);
 }
 catch (Exception e)
 {
 Console.WriteLine(e);
 }
 }
 private Socket InitializeSocket(int port)
 {
 var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
 socket.Bind(new IPEndPoint(IPAddress.Any, port));
 socket.Listen(ListenLength);
 socket.BeginAccept(AcceptConnections, new SslSocketEventArgs());
 return socket;
 }
 private void AcceptConnections(IAsyncResult result)
 {
 var resultWrapper = (SslSocketEventArgs) result.AsyncState;
 try
 {
 resultWrapper.ReplaceSslStream(result, _socket);
 resultWrapper.BeginAuthenticateAsServer(EndAuthenticate);
 _socket.BeginAccept(AcceptConnections, new SslSocketEventArgs());
 }
 catch (Exception e)
 {
 Console.WriteLine(e);
 Disconnected.InvokeSafely(this, resultWrapper);
 resultWrapper.CloseAndDispseSslStream();
 }
 }
 private void EndAuthenticate(IAsyncResult result)
 {
 var resultWrapper = (SslSocketEventArgs) result.AsyncState;
 try
 {
 try
 {
 resultWrapper.EndAuthenticateAsServer(result);
 }
 catch (Exception ex)
 {
 Console.WriteLine(ex);
 }
 if (resultWrapper.IsAuthenticated())
 {
 Connected.InvokeSafely(this, resultWrapper);
 if (resultWrapper.CanRead())
 {
 resultWrapper.CreateBuffer(5);
 resultWrapper.BeginRead(ReceiveData);
 }
 }
 else
 {
 Disconnected.InvokeSafely(this, resultWrapper);
 resultWrapper.CloseAndDispseSslStream();
 }
 }
 catch (Exception e)
 {
 Console.WriteLine(e);
 Disconnected.InvokeSafely(this, resultWrapper);
 resultWrapper.CloseAndDispseSslStream();
 }
 }
 private void ReceiveData(IAsyncResult result)
 {
 var resultWrapper = (SslSocketEventArgs) result.AsyncState;
 try
 {
 ReceivedData.InvokeSafely(this, resultWrapper);
 var size = resultWrapper.EndRead(result);
 if (size != 0)
 {
 resultWrapper.CreateBuffer(size);
 resultWrapper.BeginRead(ReceiveData);
 }
 else
 {
 resultWrapper.CloseAndDispseSslStream();
 Disconnected.InvokeSafely(this, resultWrapper);
 }
 }
 catch (Exception e)
 {
 Console.WriteLine(e);
 Disconnected.InvokeSafely(this, resultWrapper);
 resultWrapper.CloseAndDispseSslStream();
 }
 }
}

Extensions:

public static class EventHandlerExtensions
{
 public static void InvokeSafely<T>(this EventHandler<T> eventHandler,
 object sender, T eventArgs) where T : EventArgs
 {
 if (eventHandler != null)
 {
 eventHandler(sender, eventArgs);
 }
 }
}

Like CodeSparkle, I don't like the CloseAndDisposeSslStream method, but I'm not clear on a good solution for it right now.

Answer 3

Very good, but this only error is double invoke 'SslStream()'

_sslStream = new SslStream(NetworkStream(socket.EndAccept(result), true));

• 200_success: Please elaborate on what you think the problem is and how it should be fixed.