SslStream Class Review

Question

I have created an SslStream Abstract Class for my Client and i want you to review and enhance it for me if needed

SecureStream.cs

using System;
using System.Net;
using System.Net.Security;
using System.Net.Sockets;
using System.Security.Cryptography.X509Certificates;
using System.Threading.Tasks;
using Program.Connections.Packets;
using Program.Core.Misc;

namespace Program.Connections.Network
{
    public abstract class SecureStream
    {
        private static readonly byte[] Buffer = new byte[0x1f80];
        private const string IpAddress = "192.168.1.65";
        private readonly int _port;
        private readonly Socket _socket;
        private SslStream _sslStream;

        public abstract void Receive(byte[] buffer);
        public abstract void Error(Exception e);

        protected SecureStream(int port)
        {
            _socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
            _socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, true);
            _socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.DontLinger, true);
            _port = port;
        }

        public void Connect()
        {
            _socket.BeginConnect(new IPEndPoint(IPAddress.Parse(IpAddress), _port), Authenticate, null);            
        }

        private async void Authenticate(IAsyncResult ar)
        {
            try
            {
                var onCertificateValidationCallBack =
                    new RemoteCertificateValidationCallback(
                        OnCertificateValidation);
                _sslStream = new SslStream(new NetworkStream(_socket, true), false, onCertificateValidationCallBack);

                await _sslStream.AuthenticateAsClientAsync(IpAddress);
                if (!_sslStream.IsAuthenticated) throw new Exception("not^authenticated");

                do
                {
                    var size = await _sslStream.ReadAsync(Buffer, 0, Buffer.Length);
                    if (size == 0) throw new Exception("invalid^size");
                    var buffer = new byte[size];
                    Native.MemoryCopy(Buffer, buffer, (uint)buffer.Length);
                    Receive(buffer);
                    Array.Clear(Buffer, 0, Buffer.Length);

                } while (_sslStream.CanRead);
            }
            catch (Exception e)
            {
                Error(e);
            }
        }

        internal async Task Send(PacketWriter writer)
        {
            var buffer = writer.GetWorkspace();
            await _sslStream.WriteAsync(buffer, 0, buffer.Length);
        }

        private static bool OnCertificateValidation(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslpolicyerrors)
        {
            return true;
        }
    }
}

CoreServer.cs

using System;
using System.Windows.Forms;

namespace Program.Connections.Network
{
    public sealed class CoreServer : SecureStream
    {
        public CoreServer(int p) : base(p) { }

        public override void Receive(byte[] buffer)
        {
            //TODO: Handle The Buffer
        }

        public override void Error(Exception e)
        {
            //TODO: Add Error Writer.
            MessageBox.Show(
                string.Format("Disconnected from the server."),
                string.Format("Connection Error"), MessageBoxButtons.OK,
                MessageBoxIcon.Error);
            Environment.Exit(1);
        }
    }
}

And i Simply Execute it using

var coreServer = new CoreServer(/*port*/);
coreServer.Connect();

Answer 1

new byte[0x1f80]

I think using decimal would be more readable here. And I don't understand why is your buffer 8096 bytes, usual sizes are powers of two, like 8192.

IpAddress = "192.168.1.65"

This looks like a perfect example of something that should be configurable, not set in stone. You should accept the IP address in the constructor, like you already do with port number.

public abstract void Receive(byte[] buffer);
public abstract void Error(Exception e);

I'm not sure I like the use of inheritance here. I think using events, or something like that would make more sense here.

_socket.BeginConnect(…)

I don't like that you're using the old Asynchronous Programming Model when you could (and do in other parts of your code) use async-await. Unfortunately, Socket doesn't expose awaitable methods, but there are ways around that.

private async void Authenticate(IAsyncResult ar)

I think Authenticate is not a very good name for this method, it does much more than just authentication.

throw new Exception("not^authenticated")

You shouldn't throw Exception, you should instead create a custom class that derives from Exception and use that. And the exception message should be human-readable, not some error code like it seems in your case. If you need to differentiate different errors, either have different exception types for them, or add a property for that to your custom exception.

if (size == 0) throw new Exception("invalid^size");

When ReadAsync() returns 0, it's not invalid size, it means the end of the stream was reached, so you should act accordingly.

Native.MemoryCopy(Buffer, buffer, (uint)buffer.Length);

Why are using some custom memory copying method here? Do you have some reason for not using Buffer.BlockCopy()? Also, why are you even doing the copying here? It seems you class is intended only for limited use (since you have a constant IP address), so couldn't you make sure the buffer isn't used after Receive() returns, so you didn't have to copy at all?

return true;

I'm no expert on security, but I think you should perform at least some checks of the certificate here. Otherwise, your class isn't really a “secure stream”.

MessageBox.Show(…);

You shouldn't mix code like this with GUI code, they should be separated.