8
\$\begingroup\$

A library that I use doesn't support SecureString in its credentials for connecting to GitHub, so I'm submitting a pull request to add it. I know it's not ideal to pull the actual string out, but the idea here is to minimize the amount of time the string is in memory. Also, allowing client code to utilize the fact that WPF uses a SecureString for passwords without having to do this conversion themselves.

  1. Am I handling the SecureString "correctly" here? I know very little about unmanaged memory.
  2. Is it a poor decision to store the Username in one as well? I'm second guessing that decision now. NetworkCredential uses a plain string for this and the secure version only for the password.

Some notes:

  • I would use a constructor, but it's the style of the project to use an object initializer.
  • It's also in the style of the project to place the properties at the bottom like you'll see below.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using LibGit2Sharp.Core;
using System.Security;
using System.Runtime.InteropServices;

namespace LibGit2Sharp
{
    /// <summary>
    /// Class that uses <see cref="SecureString"/> to hold username and password credentials for remote repository access.
    /// </summary>
    public sealed class SecureUsernamePasswordCredentials : Credentials
    {
        /// <summary>
        /// Callback to acquire a credential object.
        /// </summary>
        /// <param name="cred">The newly created credential object.</param>
        /// <returns>0 for success, &lt; 0 to indicate an error, &gt; 0 to indicate no credential was acquired.</returns>
        protected internal override int GitCredentialHandler(out IntPtr cred)
        {
            if (Username == null || Password == null)
            {
                throw new InvalidOperationException("UsernamePasswordCredentials contains a null Username or Password.");
            }

            IntPtr passwordPtr = IntPtr.Zero;
            IntPtr userNamePtr = IntPtr.Zero;

            try
            {
                passwordPtr = Marshal.SecureStringToGlobalAllocUnicode(Password);
                userNamePtr = Marshal.SecureStringToGlobalAllocUnicode(Username);

                return NativeMethods.git_cred_userpass_plaintext_new(out cred, Marshal.PtrToStringUni(userNamePtr), Marshal.PtrToStringUni(passwordPtr));
            }
            finally
            {
                Marshal.ZeroFreeGlobalAllocUnicode(passwordPtr);
                Marshal.ZeroFreeGlobalAllocUnicode(userNamePtr);
            }

        }

        /// <summary>
        /// Username for username/password authentication (as in HTTP basic auth).
        /// </summary>
        public SecureString Username { get; set; }

        /// <summary>
        /// Password for username/password authentication (as in HTTP basic auth).
        /// </summary>
        public SecureString Password { get; set; }
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Oh how I wish that pull requesters would submit their work on CR first! \$\endgroup\$ – janos May 16 '15 at 10:59
4
\$\begingroup\$

I don't see a problem with how the SecureString objects are handled - seems pretty solid to me, but I've never used those so I'm possibly missing something that someone else could point out here.


Sealed Class

I haven't looked at the actual project's source code, but general design guidelines have this to say about sealing classes:

  • DO NOT seal classes without having a good reason to do so.

However it also defines a number of "good reasons" for sealing a class, including:

  • The class stores security-sensitive secrets in inherited protected members.

In other words, a sealed class seems warranted here.


InvalidOperationException

Out of all possible existing exception classes in the framework, and instead of creating your own derived exception class, you chose to throw an InvalidOperationException in the implementation of the GitCredentialHandler override.

MSDN describes this exception as follows:

The exception that is thrown when a method call is invalid for the object's current state.

Turns out this was, in my own humble opinion, the best decision to make: throwing any other exception type here would violate POLS - when client code is calling this method and the object's state shouldn't allow it, the programmer has all rights to expect an InvalidOperationException to be thrown.


XML Documentation

This is an API - having complete and accurate XML documentation is ideal.

    /// <summary>
    /// Callback to acquire a credential object.
    /// </summary>
    /// <param name="cred">The newly created credential object.</param>
    /// <returns>0 for success, &lt; 0 to indicate an error, &gt; 0 to indicate no credential was acquired.</returns>

I think this documentation could be better, but looking at the base class on GitHub I see that you've copied most of it from the base class, which is a good idea.

However the method can throw an InvalidOperationException - you could add something like this:

/// <exception cref="InvalidOperationException">
/// Thrown if this method is called when either <see cref="UserName"/> or <see cref="Password"/> is <c>null</c>.
/// </exception>

Style

You know me, I think this is too verbose:

IntPtr passwordPtr = IntPtr.Zero;
IntPtr userNamePtr = IntPtr.Zero;

Compared to:

var passwordPtr = IntPtr.Zero;
var userNamePtr = IntPtr.Zero;

Which is just as clear IMO. Not much gain here though. And given that the rest of the code seems to prefer the verbose way, sticking to the existing style is the best decision you could make.

And not only for usage of var:

  • I would use a constructor, but it's the style of the project to use an object initializer.
  • It's also in the style of the project to place the properties at the bottom like you'll see below.

UserName vs. Password

A user name can be sensitive information in certain circumstances - and although I don't think this is one, there's a saying that summarizes everything I'd have to say about whether or not having the user name as a SecureString is overkill:

Better safe than sorry.

-- unknown

However, the ironic part is this:

return NativeMethods.git_cred_userpass_plaintext_new(...);

Given that credentials end up being passed as plain text, I think I'd keep the user name as a plain string.

\$\endgroup\$
  • 1
    \$\begingroup\$ I would disagree about using var for the pointers I think. Changing the type of those variables could possibly break the code, right? So, yes, the type is obvious, but explicit declaration makes it harder to make a mistake in the future IMHO. Btw the project maintainer agrees with you about Username. \$\endgroup\$ – RubberDuck May 16 '15 at 10:49

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.