Asynchronous SSLSTREAM

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 Connections;
public event Action AnnounceNewConnection;
public event Action AnnounceDisconnection;
public event Action AnnounceReceive;
private Socket _sock;

private X509Certificate certificate = X509Certificate.CreateFromCertFile("exportedcertificate.cer");

public Sock(int port)
{
try
{
Connections = new Dictionary();
_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;

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:

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:

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 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:

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

-
Instead of using Action:

public event Action AnnounceNewConnection;
public event Action AnnounceDisconnection;
public event Action AnnounceReceive;

use the more expressive and conventional EventHandler:

public event EventHandler Connected;
public event EventHandler Disconnected;
public event EventHandler ReceivedData;

-
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(this EventHandler 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 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);
}

StackExchange Code Review Q#15550, answer score: 9