7
\$\begingroup\$

Ordinarily I'm fairly confident in my own code, but seeing as how this pertains to security and I don't want to have overlooked anything that might cause security problems, I figured I should have this checked.

A little background here is that I needed some way to authenticate users of my API against an Active Directory. I wanted to use a system that was compatible with Basic Authentication as far as protocol, but allows token based authentication with a username of "token" and a password that is the token.

If they log in with a token, I won't generate a token that they can use, but if they log in with a username I will.

My base code is from http://piotrwalat.net/basic-http-authentication-in-asp-net-web-api-using-message-handlers/ but it's mostly just the idea and small bits of code that have made it through my heavy modifications.

I mostly just want to make sure this is safe for handling passwords and authentication when done over https.

Feel free to use portions of this in your programs if you like it.

BasicAuthMessageHandler.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Web;
using System.Net.Http;
using System.Net.Http.Headers;
using System.Security.Principal;
using System.Threading;
using System.DirectoryServices.AccountManagement;

namespace MyApp.Security
{
  public class BasicAuthMessageHandler : DelegatingHandler
  {

    protected override System.Threading.Tasks.Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
      AuthenticationHeaderValue authValue = request.Headers.Authorization;
      if (authValue != null && !String.IsNullOrWhiteSpace(authValue.Parameter))
      {
        string username = null;
        string password = null;
        string toSplit = Encoding.UTF8.GetString(Convert.FromBase64String(authValue.Parameter));
        int index;
        if ((index = toSplit.IndexOf(":")) != -1)//verifies that they included at least the : between the username and password
        {
          username = toSplit.Substring(0, index);
          password = toSplit.Substring(index + 1);
        }
        //makes sure that there IS some semblance of a username and password. Blanks aren't allowed.
        if ( !string.IsNullOrWhiteSpace(username) && !string.IsNullOrWhiteSpace(password))
        {
          IPrincipal currentPrincipal = CreatePrincipal(username, password);//Validates the credentials and creates a principal containing their domain groups
          Thread.CurrentPrincipal = currentPrincipal;//sets the principal for the thread
          HttpContext.Current.User = currentPrincipal;//sets the principal for the HttpContext
        }
      }
      return base.SendAsync(request, cancellationToken);
    }
    private IPrincipal CreatePrincipal(String username, String password)
    {
      PrincipalContext ctx = new PrincipalContext(ContextType.Domain);//Sets up a context for the domain the app is running in
      if (username.ToLower() == "token")//special case where the username is "token" means we're using token authentication instead of normal basic authentication
      {
        username = TokenCache.localTokenCache.validateToken(password);//checks for presence of the token in the cache and grabs the username associated with it if any.
        if (username == null)
        {
          return null;//No token? No login!
        }
        else
        {
          UserPrincipal user = UserPrincipal.FindByIdentity(ctx, username);//Grab the necessary information about the user from the domain.
          if (user.Enabled == true)//make sure their account isn't disabled.
          {
            //SIDs can be used to check NTFS file access permissions before doing an operation on behalf of the user.
            List<string> groups = new List<string>(user.GetAuthorizationGroups().Select(i => i.Sid.Value));//grab the SIDs from any groups they're in
            groups.Add(user.Sid.Value);//also add their own SID
            groups.Add("Auth:Token");//Lets me check later from APIs if they authenticated with a token or username and password.
            //Note that here I'm using the "Authentication Type" part of generic identity to store the token if I make one.
            //This is a convienient place to put a string that can be used by API methods later in the pipeline.
            return new GenericPrincipal(new GenericIdentity(user.SamAccountName, ""), groups.ToArray());//Creates and returns the Principal
          }
          else
          {
            return null;//disabled account? No login!
          }
        }
      }
      if (ctx.ValidateCredentials(username, password))//if they give an actual username and password, we authenticate against the domain!
      {
        UserPrincipal user = UserPrincipal.FindByIdentity(ctx, username);//Grab the necessary information about the user from the domain.
        string token = TokenCache.localTokenCache.generateNewToken(user.SamAccountName);//generate a new token for the user
        if (user.Enabled == true)//make sure their account isn't disabled.
        {
          //SIDs can be used to check NTFS file access permissions before doing an operation on behalf of the user.
          List<string> groups = new List<string>(user.GetAuthorizationGroups().Select(i => i.Sid.Value));//grab the SIDs from any groups they're in
          groups.Add(user.Sid.Value);//also add their own SID
          groups.Add("Auth:Basic");//Lets me check later from APIs if they authenticated with a token or username and password.
          //Note that here I'm using the "Authentication Type" part of generic identity to store the token if I make one.
          //This is a convienient place to put a string that can be used by API methods later in the pipeline.
          return new GenericPrincipal(new GenericIdentity(user.SamAccountName, token), groups.ToArray());//Creates and returns the Principal
        }
        else
        {
          return null;//disabled account? No login!
        }
      }
    return null;//invalid username and password? No login!
    }
  }
}

TokenCache.cs

using System;
using System.Security.Cryptography;
using System.Collections.Generic;

namespace MyApp.Security
{
  class TokenCache : IDisposable
  {
    public static readonly TokenCache localTokenCache = new TokenCache(new TimeSpan(20,0,0));//Singleton where tokens expire after 20 hours
    private Dictionary<string, string> userToToken = new Dictionary<string, string>();
    private Dictionary<string, string> tokenToUser = new Dictionary<string, string>();
    private Dictionary<string, DateTime> tokenToDate = new Dictionary<string, DateTime>();

    private RNGCryptoServiceProvider numgen;
    private TimeSpan _maxValidity;
    private TokenCache(TimeSpan maxValidity)
    {
      _maxValidity = maxValidity;
      numgen = new RNGCryptoServiceProvider();
    }
    public string validateToken(string token)
    {
      if (tokenToUser.ContainsKey(token) && DateTime.Now - tokenToDate[token] < _maxValidity)//do we have the token, and is it unexpired?
      {
        return tokenToUser[token];//Yes? Return username.
      }else
      {
        return null;//No? Then return null;
      }
    }
    public string generateNewToken(string username)
    {
      byte[] tokenarr = new byte[33];
      numgen.GetBytes(tokenarr);
      string token = SimpleMethods.Base642URL(Convert.ToBase64String(tokenarr));//Makes a URL friendly token
      DateTime createdon = DateTime.Now;
      removePreviousTokens(username, service);
      tokenToUser.Add(token, username);
      userToToken.Add(username, token);
      tokenToDate.Add(token, createdon);
      return token;
    }
    private void removePreviousTokens(string username)
    {
      if(userToToken.ContainsKey(username))
      {
        string token = userToToken[username];
        tokenToUser.Remove(token);
        userToToken.Remove(username);
        tokenToDate.Remove(token);
      }
    }

#region IDisposable Support
    private bool disposedValue = false; // To detect redundant calls

    protected virtual void Dispose(bool disposing)
    {
      if (!disposedValue)
      {
        if (disposing)
        {
          numgen.Dispose();
        }
        tokenToDate = null;
        tokenToUser = null;
        userToToken = null;
        disposedValue = true;
      }
    }

    public void Dispose()
    {
      Dispose(true);
    }
#endregion
  }
}

SimpleMethods.Base642URL

public static string Base642URL(string base64String)
{
  base64String = base64String.Replace("+", "-");
  base64String = base64String.Replace("/", "_");
  base64String = base64String.Replace("=", "");
  return base64String;
}
\$\endgroup\$
  • \$\begingroup\$ If you want to use this in your code, You'll also need to add something like: GlobalConfiguration.Configuration.MessageHandlers.Add(new BasicAuthMessageHandler()); to your Global.asax.cs Application_Start function \$\endgroup\$ – Andrew Hendrix Mar 6 '17 at 23:21
1
\$\begingroup\$

The first thing that stands out is this method:

public static string Base642URL(string base64String)
{
  base64String = base64String.Replace("+", "-");
  base64String = base64String.Replace("/", "_");
  base64String = base64String.Replace("=", "");
  return base64String;
}

I was wondering what a Base642 Url was, but then I realised it's Base64-2-Url, let's change that 2 to a To.

Let's also make that puppy an extension method:

public static class StringExtensions
{
    public static string Base642URL(this string base64String)
    {
        base64String = base64String.Replace("+", "-");
        base64String = base64String.Replace("/", "_");
        base64String = base64String.Replace("=", "");
        return base64String;
    }
}

That looks better.

Next, I really don't like your formatting, but that's because it hides bigger problems.

private IPrincipal CreatePrincipal(String username, String password)
{
  PrincipalContext ctx = new PrincipalContext(ContextType.Domain);//Sets up a context for the domain the app is running in
  if (username.ToLower() == "token")//special case where the username is "token" means we're using token authentication instead of normal basic authentication
  {
    username = TokenCache.localTokenCache.validateToken(password);//checks for presence of the token in the cache and grabs the username associated with it if any.
    if (username == null)
    {
      return null;//No token? No login!
    }
    else
    {
      UserPrincipal user = UserPrincipal.FindByIdentity(ctx, username);//Grab the necessary information about the user from the domain.
      if (user.Enabled == true)//make sure their account isn't disabled.
      {
        //SIDs can be used to check NTFS file access permissions before doing an operation on behalf of the user.
        List<string> groups = new List<string>(user.GetAuthorizationGroups().Select(i => i.Sid.Value));//grab the SIDs from any groups they're in
        groups.Add(user.Sid.Value);//also add their own SID
        groups.Add("Auth:Token");//Lets me check later from APIs if they authenticated with a token or username and password.
        //Note that here I'm using the "Authentication Type" part of generic identity to store the token if I make one.
        //This is a convienient place to put a string that can be used by API methods later in the pipeline.
        return new GenericPrincipal(new GenericIdentity(user.SamAccountName, ""), groups.ToArray());//Creates and returns the Principal
      }
      else
      {
        return null;//disabled account? No login!
      }
    }
  }
  if (ctx.ValidateCredentials(username, password))//if they give an actual username and password, we authenticate against the domain!
  {
    UserPrincipal user = UserPrincipal.FindByIdentity(ctx, username);//Grab the necessary information about the user from the domain.
    string token = TokenCache.localTokenCache.generateNewToken(user.SamAccountName);//generate a new token for the user
    if (user.Enabled == true)//make sure their account isn't disabled.
    {
      //SIDs can be used to check NTFS file access permissions before doing an operation on behalf of the user.
      List<string> groups = new List<string>(user.GetAuthorizationGroups().Select(i => i.Sid.Value));//grab the SIDs from any groups they're in
      groups.Add(user.Sid.Value);//also add their own SID
      groups.Add("Auth:Basic");//Lets me check later from APIs if they authenticated with a token or username and password.
      //Note that here I'm using the "Authentication Type" part of generic identity to store the token if I make one.
      //This is a convienient place to put a string that can be used by API methods later in the pipeline.
      return new GenericPrincipal(new GenericIdentity(user.SamAccountName, token), groups.ToArray());//Creates and returns the Principal
    }
    else
    {
      return null;//disabled account? No login!
    }
  }
return null;//invalid username and password? No login!
}

This method looks like a lot of 'arrow code' (that is, we indent level-after-level-after-level-after-level...), we can clean that up a lot with some basic guard clauses.

So let's look at it bit-by-bit:

if (username == null)
{
  return null;//No token? No login!
}
else
{

Why the else? It's completely unnecessary. There's no reason to have the code if username != null in an else block here, because you have an early-return.

if (username == null)
{
    return null;
}
// Just continue with the code

Then we have this section:

 if (user.Enabled == true)//make sure their account isn't disabled.
 {
   //SIDs can be used to check NTFS file access permissions before doing an operation on behalf of the user.
   List<string> groups = new List<string>(user.GetAuthorizationGroups().Select(i => i.Sid.Value));//grab the SIDs from any groups they're in
   groups.Add(user.Sid.Value);//also add their own SID
   groups.Add("Auth:Token");//Lets me check later from APIs if they authenticated with a token or username and password.
   //Note that here I'm using the "Authentication Type" part of generic identity to store the token if I make one.
   //This is a convienient place to put a string that can be used by API methods later in the pipeline.
   return new GenericPrincipal(new GenericIdentity(user.SamAccountName, ""), groups.ToArray());//Creates and returns the Principal
 }
 else
 {
   return null;//disabled account? No login!
 }

Again, we can eliminate the nesting:

if (user.Enabled != true) // Not the same as `== false`
{
    return null;
}
// Continue with the code

So the whole method is rewritten (following these principles) as follows:

private IPrincipal CreatePrincipal(String username, String password)
{
    PrincipalContext ctx = new PrincipalContext(ContextType.Domain);//Sets up a context for the domain the app is running in
    if (username.ToLower() == "token")//special case where the username is "token" means we're using token authentication instead of normal basic authentication
    {
        username = TokenCache.localTokenCache.validateToken(password);//checks for presence of the token in the cache and grabs the username associated with it if any.
        if (username == null)
        {
            return null;//No token? No login!
        }

        UserPrincipal user = UserPrincipal.FindByIdentity(ctx, username);//Grab the necessary information about the user from the domain.
        if (user.Enabled != true)//make sure their account isn't disabled.
        {
            return null;
        }

        //SIDs can be used to check NTFS file access permissions before doing an operation on behalf of the user.
        List<string> groups = new List<string>(user.GetAuthorizationGroups().Select(i => i.Sid.Value));//grab the SIDs from any groups they're in
        groups.Add(user.Sid.Value);//also add their own SID
        groups.Add("Auth:Token");//Lets me check later from APIs if they authenticated with a token or username and password.
        //Note that here I'm using the "Authentication Type" part of generic identity to store the token if I make one.
        //This is a convienient place to put a string that can be used by API methods later in the pipeline.
        return new GenericPrincipal(new GenericIdentity(user.SamAccountName, ""), groups.ToArray());//Creates and returns the Principal
    }
    if (ctx.ValidateCredentials(username, password))//if they give an actual username and password, we authenticate against the domain!
    {
        UserPrincipal user = UserPrincipal.FindByIdentity(ctx, username);//Grab the necessary information about the user from the domain.
        string token = TokenCache.localTokenCache.generateNewToken(user.SamAccountName);//generate a new token for the user
        if (user.Enabled != true)//make sure their account isn't disabled.
        {
            return null;//disabled account? No login!
        }

        //SIDs can be used to check NTFS file access permissions before doing an operation on behalf of the user.
        List<string> groups = new List<string>(user.GetAuthorizationGroups().Select(i => i.Sid.Value));//grab the SIDs from any groups they're in
        groups.Add(user.Sid.Value);//also add their own SID
        groups.Add("Auth:Basic");//Lets me check later from APIs if they authenticated with a token or username and password.
        //Note that here I'm using the "Authentication Type" part of generic identity to store the token if I make one.
        //This is a convienient place to put a string that can be used by API methods later in the pipeline.
        return new GenericPrincipal(new GenericIdentity(user.SamAccountName, token), groups.ToArray());//Creates and returns the Principal
    }
    return null;//invalid username and password? No login!
}

Now that we've done that, we can pretty easily read your code. It's still cluttered with comments, you don't really need the massive amount of comments there. Your code should describe what is happening, you don't need to comment on that. Your comments should say why something is happening, if it's for a non-standard reason. (You don't need to say 'check for the special case of username == token, I can tell that's happening, and if I have any basic knowledge of the application I know why you did that.) If we remove just the self-explanatory comments, we'll find we have the following:

private IPrincipal CreatePrincipal(String username, String password)
{
    PrincipalContext ctx = new PrincipalContext(ContextType.Domain);
    if (username.ToLower() == "token")
    {
        username = TokenCache.localTokenCache.validateToken(password);
        if (username == null)
        {
            return null;
        }

        UserPrincipal user = UserPrincipal.FindByIdentity(ctx, username);
        if (user.Enabled != true)
        {
            return null;
        }

        //SIDs can be used to check NTFS file access permissions before doing an operation on behalf of the user.
        List<string> groups = new List<string>(user.GetAuthorizationGroups().Select(i => i.Sid.Value));
        groups.Add(user.Sid.Value);
        groups.Add("Auth:Token");
        //Note that here I'm using the "Authentication Type" part of generic identity to store the token if I make one.
        //This is a convienient place to put a string that can be used by API methods later in the pipeline.
        return new GenericPrincipal(new GenericIdentity(user.SamAccountName, ""), groups.ToArray());
    }
    if (ctx.ValidateCredentials(username, password))
    {
        UserPrincipal user = UserPrincipal.FindByIdentity(ctx, username);
        string token = TokenCache.localTokenCache.generateNewToken(user.SamAccountName);
        if (user.Enabled != true)
        {
            return null;
        }

        //SIDs can be used to check NTFS file access permissions before doing an operation on behalf of the user.
        List<string> groups = new List<string>(user.GetAuthorizationGroups().Select(i => i.Sid.Value));
        groups.Add(user.Sid.Value);
        groups.Add("Auth:Basic");
        //Note that here I'm using the "Authentication Type" part of generic identity to store the token if I make one.
        //This is a convienient place to put a string that can be used by API methods later in the pipeline.
        return new GenericPrincipal(new GenericIdentity(user.SamAccountName, token), groups.ToArray());
    }
    return null;
}

Things like:

List<string> groups = new List<string>(user.GetAuthorizationGroups().Select(i => i.Sid.Value));

Do not need a comment telling us we're grabbing the Sid's from groups they're in, we can see that: user » GetAuthorizationGroups() » Select(i => i.Sid.Value), that's plainly obvious what's going on.

Now we finally get to the elephant in the room: why are you providing a magic term token as a username? This is a very bad practice, and I know because I've done it before. This type of practice has bit me in the arse so many times...well, don't do that.

Your identity should have a token-based mechanism for login, and should allow you to construct one in your code from the token itself, not needing to specify that token is the username.

Basically, you should be building a flow that allows you to build an identity FromUserPass(string, string) or FromToken(string). Security wise it's a very large potential threat (they would have to know a token itself to attack your application), but security-by-obscurity on it's own is never a good security scheme, you should also implement additional security mechanisms on-top-of the obscurity.

\$\endgroup\$
  • \$\begingroup\$ First of all, thank you very much. This has given me some good insight into my code. Especially my unnecessary else statements and over commenting. \$\endgroup\$ – Andrew Hendrix Aug 9 '17 at 19:39
  • \$\begingroup\$ For the token, I never intended it to be security through obscurity, the only reason I do it that way at all is because my client programs expect to be able to use Basic Authentication for all communication, but I don't like the idea of sending raw credentials with every message. The token functions as a temporary username and password for the given user. I suppose I could remove the token keyword and have them use their username instead, and just check for the temporary password under that username. Would that be a better model? \$\endgroup\$ – Andrew Hendrix Aug 9 '17 at 19:40
  • \$\begingroup\$ @AndrewHendrix A better model would be to design a secondary authentication mechanism - look at OAuth or OpenID: you authenticate, then get a 'token' which identifies you for the remainder of time it is valid. In this way you no longer need to worry about what the specific case of 'username' is or anything of the sort: you worry about what credentials they've specified. \$\endgroup\$ – 410_Gone Aug 9 '17 at 19:41
  • \$\begingroup\$ That's true, and I'm sure it would work, but at that point I'm no longer using the Basic Authentication protocol. Enough of our libraries expect that from a client perspective, it's easier to allow a user to use their real "username" and "password" to get a different temporary "username" and "password" with a limited duration. (Which if compromised wouldn't be as bad as compromising the real username and password) \$\endgroup\$ – Andrew Hendrix Aug 9 '17 at 19:52

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.