2
\$\begingroup\$

I'm a Windows sysadmin who is new to C# and "professional" coding. My background is in VBScript ("get it done") and I've apparently built up a number of bad habits. I'm hoping a review of the following code can help me do things the right way when tasks are still easy so that more difficult projects go smoother.

I needed a program to connect to a given site and return the number of days until the certificate expires. It also needed to be able to query against different ports and different protocols (HTTPS, LDAPS, FTPS). I also added a /Verbose switch to return more info when used stand-alone.

I'm interested in where I should be using functions (and what code should be removed from Main) and better ways to parse the command line args. Also, any obvious security issues since I am allowing CMD line input. I really appreciate any comments!

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

namespace CertificateVerification
{
    class CertificateVerification
   {
    // Method for displaying program usage
    public static void Usage()
    {
        Console.WriteLine("\r\nDisplays certificate inforamtion about URI\r\n");
        Console.Write("{0} URI[:PORT] [/V]\r\n", System.Diagnostics.Process.GetCurrentProcess().ProcessName);
    }

    // Method for ignoring certificate errors
    public static bool TrustAllCertificates(
        object sender,
        System.Security.Cryptography.X509Certificates.X509Certificate certificate,
        System.Security.Cryptography.X509Certificates.X509Chain chain,
        System.Net.Security.SslPolicyErrors errors)
    {
        return true;
    }

    static void Main(string[] args)
    {
        // Set basic variables
        bool verbose = false;
        string uri = string.Empty;
        int port = 443;
        List<string> display = new List<string>();

        // 1 or 2 arguments required
        if (args.Length < 1 || args.Length > 2)
        {
            Usage();
            Environment.Exit(1);
        }

        // If the first argument contains a switch, exit
        if (args[0].Substring(0, 1) == "/" || args[0].Substring(0, 1) == "-")
        {
            Usage();
            Environment.Exit(1);
        }

        // Set uri as the first argument
        uri = args[0];

        // Parse arguments
        foreach (string arg in args)
        {
            // Choose to display VERBOSE or USAGE
            if (arg.Substring(0, 1) == "/" || arg.Substring(0, 1) == "-")
            {
                if (arg.Substring(1).ToUpper() == "V")
                {
                    verbose = true;
                }
                else if (arg.Substring(1) == "?")
                {
                    Usage();
                    Environment.Exit(0);
                }
                else
                {
                    Console.WriteLine(String.Format("Unknown Argument Value: \"{0}\"", arg.Substring(1).ToUpper()));
                    Usage();
                    Environment.Exit(1);
                }
            }

            // Set URI and PORT
            if (arg.IndexOf(":") > 0)
            {
                uri = arg.Split(':')[0];
                port = Int32.Parse(arg.Split(':')[1]);
            }
        }

        // Use data from Arguments to connect
        // Ignore certificate errors
        X509Certificate certificate = new X509Certificate();
        try
        {
            TcpClient tcpClient = new TcpClient(uri, port);
            NetworkStream tcpStream = tcpClient.GetStream();
            SslStream sslStream = new SslStream(tcpStream, false, new RemoteCertificateValidationCallback(TrustAllCertificates));
            sslStream.AuthenticateAsClient(uri);
            certificate = sslStream.RemoteCertificate;
        }
        catch (Exception e)
        {
            Console.Write(e.Message);
            Environment.Exit(1);
        }

        // Format display based on results from above
        display.Add("URI: " + uri);
        display.Add("   Subject:" + new string(' ', 14 - "   Subject".Length) + certificate.Subject);
        display.Add("   Issuer:" + new string(' ', 14 - "   Issuer".Length) + certificate.Issuer);
        display.Add("   ValidFrom:" + new string(' ', 14 - "   ValidFrom".Length) + certificate.GetEffectiveDateString());
        display.Add("   ValidTo:" + new string(' ', 14 - "   ValidTo".Length) + certificate.GetExpirationDateString());
        display.Add("   Days Left:" + new string(' ', 14 - "   Days Left".Length) + (Convert.ToDateTime(certificate.GetExpirationDateString()) - DateTime.Now).Days);

        // Display the results
        if (verbose)
        {
            foreach (var item in display)
            {
                Console.WriteLine(item.ToString());
            }
        }
        else
        {
            int dtm = (Convert.ToDateTime(certificate.GetExpirationDateString()) - DateTime.Now).Days;
            Console.Write(dtm.ToString());
        }
    }
}

}

I should mention that the program above is just one building block in a larger task that does the actual alerting, which is of course written in VBScript and called from Task Scheduler :). I like the idea of a simple program that can stand on its own, and more specific tasks (alert when a certificate is about to expire) are handled elsewhere, which is why I didn't try to write a single executable to handle everything.

\$\endgroup\$
  • \$\begingroup\$ This "bigger program" sounds just like a monitoring system, such as nagios, naemon, op5. They already have plugins for certificate validations too... \$\endgroup\$ – Mattias Åslund Apr 28 '16 at 22:00
  • \$\begingroup\$ I don't mean to argue with you, because you do have a point. However I don't believe your comment is helpful from a 'code review' stand point. Yes, I'm reinventing a small part of a larger wheel, but in doing so I'm learning how to write code. I posted here not to get help a specific problem, but to learn about the ways to 'do things the right way' when it comes to writing a program - and using the above program as a building block in that direction. \$\endgroup\$ – mattw01 Apr 29 '16 at 22:22
  • \$\begingroup\$ If you purportely reinvent you can always add the tag reinventing-the-wheel. My comment was a friendly nudge towards existing alternatives. I had put constructive critizism in an answer. :) \$\endgroup\$ – Mattias Åslund Apr 30 '16 at 11:26
0
\$\begingroup\$
// Method for displaying program usage
public static void Usage()
{
    Console.WriteLine("\r\nDisplays certificate inforamtion about URI\r\n");
    Console.Write("{0} URI[:PORT] [/V]\r\n", System.Diagnostics.Process.GetCurrentProcess().ProcessName);
}  

If you need a comment to explain what a specific method is doing, you have named the method bad. That being said you should rename the method to DisplayProgramUsage() which makes the comment superflous. In addition, this method doesn't need to be public.

Comments should be used to explain why something is done in the way it is done. Let the code talk for itself about what is done by using meaningful and descriptive names for classes, methods, properties and fields.

// Method for ignoring certificate errors
public static bool TrustAllCertificates(
    object sender,
    System.Security.Cryptography.X509Certificates.X509Certificate certificate,
    System.Security.Cryptography.X509Certificates.X509Chain chain,
    System.Net.Security.SslPolicyErrors errors)
{
    return true;
}  

Here we have a comment which is lying. The method isn't used to ignore errors but to trust all certificates. In addition, why is this method public ?

// Set basic variables
bool verbose = false;
string uri = string.Empty;
int port = 443;
List<string> display = new List<string>();  

You should declare variables as near to their usage as possible. In this case you declare the variables before any argument validation takes place. For invalid arguments which are validated later this would be superflous.

Always validate arguments before any declaration and processing.

You should extract the validation to a separate method to keep responsibilities separated.

Your validation logic doesn't proper validate edge cases. Assume a user passes a second argument with neither a / nor a - in front. Then it would be seen as a valid argument which it isn't.

The following is dangerous as well

if (arg.IndexOf(":") > 0)
{
    uri = arg.Split(':')[0];
    port = Int32.Parse(arg.Split(':')[1]);
}

for a given first argument of e.g www.google.de:test it will throw a FormatException which isn't handled by your program.

As a rule of thumb: Never ever trust data entered by the user !

Instead of Parse you should use TryParse() which returns a bool flagging if the string could be parsed to an integer.

Implementing the mentioned points would lead to

private static void ValidateArguments(string[] args, Action displayProgramUsage)
{
    if (args.Length < 1 || args.Length > 2)
    {
        displayProgramUsage();
        Environment.Exit(1);
    }

    if (StartsWithSwitch(args[0]))
    {
        displayProgramUsage();
        Environment.Exit(1);
    }

    if (args.Length == 1)
    {
        return;
    }

    string argument = args[1];
    if (StartsWithSwitch(argument))
    {
        argument = args[1].Substring(1).ToUpper();
        if (argument == "V" || argument == "?")
        {
            return;
        }
    }

    Console.WriteLine(String.Format("Unknown Argument Value: \"{0}\"", argument));
    displayProgramUsage();
    Environment.Exit(1);

}  
private static bool StartsWithSwitch(string argument)
{
    return argument.StartsWith("/") || argument.StartsWith("-");
}

uri, port and verbose

I would add a private class ProcessingParameter like so

private class ProcessingParameter
{
    public string Uri { get; private set; }
    public int Port { get; private set; }
    public bool Verbose { get; private set; }

    private ProcessingParameter()
    {
        Port = 443;
        Verbose = false;
    }

    public static ProcessingParameter FromArguments(string[] arguments, Action displayProgramUsage)
    {
        ProcessingParameter processingParameter = new ProcessingParameter();

        string[] parameters = arguments[0].Split(':');

        processingParameter.Uri = parameters[0];

        if (parameters.Length == 2)
        {
            int port = 0;
            if (!int.TryParse(parameters[1], out port))
            {
                throw new ArgumentException("The value representing the port has an invalid format");
            }
            processingParameter.Port = port;
        }

        processingParameter.Verbose = arguments.Length == 2;

        return processingParameter;
    }
}

which creates an instance by the FromArguments() method.


X509Certificate certificate = new X509Certificate();
try
{
    TcpClient tcpClient = new TcpClient(uri, port);
    NetworkStream tcpStream = tcpClient.GetStream();
    SslStream sslStream = new SslStream(tcpStream, false, new RemoteCertificateValidationCallback(TrustAllCertificates));
    sslStream.AuthenticateAsClient(uri);
    certificate = sslStream.RemoteCertificate;
}
catch (Exception e)
{
    Console.Write(e.Message);
    Environment.Exit(1);
}  

This seems ok but should be improved. If an object implements IDisposable like streams it should be enclosed inside a using statements which takes care of disposing the object.

Fixing this is simple

X509Certificate certificate = new X509Certificate();
try
{
    TcpClient tcpClient = new TcpClient(uri, port);
    using(NetworkStream tcpStream = tcpClient.GetStream())
    using(SslStream sslStream = new SslStream(tcpStream, false, new RemoteCertificateValidationCallback(TrustAllCertificates)))
    {
        sslStream.AuthenticateAsClient(uri);
        certificate = sslStream.RemoteCertificate;
    }
}
catch (Exception e)
{
    Console.Write(e.Message);
    Environment.Exit(1);
} 
finally
{
    if (tcpClient != null)
    {
        tcpClient.Close();
    }
}  

but it should be extracted to a separate method as well.


The code below // Format display based on results from above has a few issues:

  • it doesn't belong in main but in a separate method to make the responsibility clear.
  • the List<string> is filled regardless if verbose is true or false
  • there is some strange formatting going on e.g + new string(' ', 14 - " Subject".Length). The string class has a PadRight() which fills a given string to the passed totalWidth by padding with spaces.

Putting all together the code will look like so

    static void Main(string[] args)
    {
        ValidateArguments(args, DisplayProgramUsage);

        if (ShouldProgramUsageBeDisplayed(args))
        {
            DisplayProgramUsage();
        }

        ProcessingParameter processingParameter = null;
        try
        {
            processingParameter = ProcessingParameter.FromArguments(args, DisplayProgramUsage);
        }
        catch (ArgumentException ex)
        {
            Console.WriteLine(ex.Message);
            DisplayProgramUsage();
        }

        X509Certificate certificate = FetchCertificate(processingParameter);

        int daysLeftUntilExpiration = (Convert.ToDateTime(certificate.GetExpirationDateString()) - DateTime.Now).Days;

        if (processingParameter.Verbose)
        {
            Console.WriteLine(FormatVerboseOutput(processingParameter.Uri, certificate, daysLeftUntilExpiration));

        }
        else
        {
            Console.Write("Days left: " + daysLeftUntilExpiration);
        }
    }

    private static void ValidateArguments(string[] args, Action displayProgramUsage)
    {
        if (args.Length < 1 || args.Length > 2)
        {
            displayProgramUsage();
            Environment.Exit(1);
        }

        if (StartsWithSwitch(args[0]))
        {
            displayProgramUsage();
            Environment.Exit(1);
        }

        if (args.Length == 1)
        {
            return;
        }

        string argument = args[1];
        if (StartsWithSwitch(args[1]))
        {
            argument = args[1].Substring(1).ToUpper();
            if (argument == "V" || argument == "?")
            {
                return;
            }
        }

        Console.WriteLine(String.Format("Unknown Argument Value: \"{0}\"", argument));
        displayProgramUsage();
        Environment.Exit(1);

    }

    private static bool StartsWithSwitch(string argument)
    {
        return argument.StartsWith("/") || argument.StartsWith("-");
    }

    private static bool ShouldProgramUsageBeDisplayed(string[] arguments)
    {
        if (arguments.Length == 1) { return false; }

        string argument = arguments[1];
        string delemiter = argument.Substring(0, 1);
        if (delemiter != "/" || delemiter != "-")
        {
            return false;
        }
        return argument.Substring(1) == "?";
    }

    private static X509Certificate FetchCertificate(ProcessingParameter processingParameter)
    {
        X509Certificate certificate = new X509Certificate();
        TcpClient tcpClient = null;
        try
        {
            tcpClient = new TcpClient(processingParameter.Uri, processingParameter.Port);
            using (var tcpStream = tcpClient.GetStream())
            using (var sslStream = new SslStream(tcpStream, false, new RemoteCertificateValidationCallback(TrustAllCertificates)))
            {
                sslStream.AuthenticateAsClient(processingParameter.Uri);
                return sslStream.RemoteCertificate;
            }
        }
        catch (Exception e)
        {
            Console.Write(e.Message);
            Environment.Exit(1);
        }
        finally
        {
            if (tcpClient != null)
            {
                tcpClient.Close();
            }
        }

        // this exists only because the IDE complains about returning a value.
        throw new NotImplementedException();

    }
    private static bool TrustAllCertificates(
        object sender,
        System.Security.Cryptography.X509Certificates.X509Certificate certificate,
        System.Security.Cryptography.X509Certificates.X509Chain chain,
         System.Net.Security.SslPolicyErrors errors)
    {
        return true;
    }

    private static void DisplayProgramUsage()
    {
        Console.WriteLine("\r\nDisplays certificate inforamtion about URI\r\n");
        Console.Write("{0} URI[:PORT] [/V]\r\n", System.Diagnostics.Process.GetCurrentProcess().ProcessName);
    }

    private class ProcessingParameter
    {
        public string Uri { get; private set; }
        public int Port { get; private set; }
        public bool Verbose { get; private set; }

        private ProcessingParameter()
        {
            Port = 443;
            Verbose = false;
        }

        public static ProcessingParameter FromArguments(string[] arguments, Action displayProgramUsage)
        {
            ProcessingParameter processingParameter = new ProcessingParameter();

            string[] parameters = arguments[0].Split(':');

            processingParameter.Uri = parameters[0];

            if (parameters.Length == 2)
            {
                int port = 0;
                if (!int.TryParse(parameters[1], out port))
                {
                    throw new ArgumentException("The value representing the port has an invalid format");
                }
                processingParameter.Port = port;
            }

            processingParameter.Verbose = arguments.Length == 2;

            return processingParameter;


        }
    }

    private static string FormatVerboseOutput(string uri, X509Certificate certificate, int daysLeftUntilExpiration)
    {
        var sb = new StringBuilder();
        sb.AppendLine("URI: " + uri)
            .AppendLine("   Subject:".PadRight(15) + certificate.Subject)
            .AppendLine("   Issuer:".PadRight(15) + certificate.Issuer)
            .AppendLine("   ValidFrom:".PadRight(15) + certificate.GetEffectiveDateString())
            .AppendLine("   ValidTo:".PadRight(15) + certificate.GetExpirationDateString())
            .AppendLine("   Days Left:".PadRight(15) + daysLeftUntilExpiration);

        return sb.ToString();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer! It's going to take quite a bit more time than I have right now to digest it all, but I plan to make good use of it... \$\endgroup\$ – mattw01 Dec 22 '16 at 20:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.