4
\$\begingroup\$
using Microsoft.AspNetCore.WebUtilities;
using System.Security.Cryptography;

namespace UserManager.Cryptography
{
    public class UrlToken
    {
        private const int BYTE_LENGTH = 32; 

        /// <summary>
        /// Generate a fixed length token that can be used in url without endcoding it
        /// </summary>
        /// <returns></returns>
        public static string GenerateToken()
        {
            // get secure array bytes
            byte[] secureArray = GenerateRandomBytes();

            // convert in an url safe string
            string urlToken = WebEncoders.Base64UrlEncode(secureArray);

            return urlToken;
        }

        /// <summary>
        /// Generate a cryptographically secure array of bytes with a fixed length
        /// </summary>
        /// <returns></returns>
        private static byte[] GenerateRandomBytes()
        {
            using (RNGCryptoServiceProvider provider = new RNGCryptoServiceProvider()) { 
                byte[] byteArray = new byte[BYTE_LENGTH];
                provider.GetBytes(byteArray);

                return byteArray;
            }
        }
    }
}

I've created the above class (C#, .Net Core 2.0) to generate a cryptographically secure string token that is URL safe, so it can be used in an URL without the necessity to be encoded.

I will use that token as a GET parameter (e.g. www.site.com/verify/?token=v3XYPmQ3wD_RtOjH1lMekXloBGcWqlLfomgzIS1mCGA) in a user manager application where I use the token to verify the user email or to recover a user password.

The above link will be sent as email to the user that has requested the service.

I store the token into a DB table with an associated expiration datetime.

I've seen other implementations on this and other sites but all seem to be unnecessarily complicated. Am I missing something?

\$\endgroup\$
  • 1
    \$\begingroup\$ Looks ok to me. I tend to use hex encoding rather than base64, and typically generate this in SQL server using HASHBYTES. I usually have parameters which identify the user, and an expiry date for the token (included in the hash), that way I don't have to store anything extra in the database, but perhaps that doesn't suit your security requirements. \$\endgroup\$ – George Barwood Jan 28 at 12:37
  • \$\begingroup\$ "cryptographically secure string token" - well, it's cryptographically RANDOM, but it's only as secure as your systems protect and validate it. \$\endgroup\$ – Jesse C. Slicer Jan 28 at 17:13
  • \$\begingroup\$ It should be obvious, but I didn't mention in my comment above that with that approach, it's essential to include some secret key material in the hash ( or there is no security at all ). \$\endgroup\$ – George Barwood Jan 29 at 7:48
  • \$\begingroup\$ Considering the fact that the token is used to verify email or to recover lost password and has an expiration datetime, is not safe to just store it in the DB with a SHA256 hash without any salt or secure key? \$\endgroup\$ – S.Orioli Jan 29 at 9:09
4
\$\begingroup\$

Minor suggestions:

public class UrlToken

The class has no instance data, so it could be made static:

public static class UrlToken

Microsoft's Naming Guidelines and their Framework Design Guidelines suggest not using underscores and also using PascalCasing for constants, so

    private const int BYTE_LENGTH = 32; 

could be:

    private const int ByteLength = 32; 

However, even that name doesn't tell us much of what it is for. Let's try again:

    private const int NumberOfRandomBytes = 32; 

Typo/misspelling in the XML doc comment: "encoding" is written as "endcoding".

There is mixed curly brace formatting. Microsoft guidelines (see links above) suggest the opening and closing curly braces should be on their own line.

        using (RNGCryptoServiceProvider provider = new RNGCryptoServiceProvider()) { 

to:

        using (RNGCryptoServiceProvider provider = new RNGCryptoServiceProvider())
        { 

By the way, kudos to you on your proper use of the using construct! Looks fantastic!

\$\endgroup\$
  • \$\begingroup\$ Hi, thanks for the advices; making the class static has any performance benefit or usability improvement or is just a stylish thing? \$\endgroup\$ – S.Orioli Jan 29 at 9:14
  • 1
    \$\begingroup\$ @S.Orioli yes, it prevents wasteful constructs like var thing = new UrlToken(); from being able to be written. \$\endgroup\$ – Jesse C. Slicer Jan 29 at 13:42
3
\$\begingroup\$

There are not much to work with in a review besides what mr. Slicer has already written.


To make it a little more flexible you could though provide the number of bytes as an argument instead of a constant member value:

      public static string GenerateToken(int numberOfBytes = 32)

Refactoring the class to handle that could be:

public class ReviewdUrlToken
{
  /// <summary>
  /// Generate a fixed length token that can be used in url without endcoding it
  /// </summary>
  /// <returns></returns>
  public static string GenerateToken(int numberOfBytes = 32)
  {
    return WebEncoders.Base64UrlEncode(GenerateRandomBytes(numberOfBytes));
  }

  /// <summary>
  /// Generate a cryptographically secure array of bytes with a fixed length
  /// </summary>
  /// <returns></returns>
  private static byte[] GenerateRandomBytes(int numberOfBytes)
  {
    using (RNGCryptoServiceProvider provider = new RNGCryptoServiceProvider())
    {
      byte[] byteArray = new byte[numberOfBytes];
      provider.GetBytes(byteArray);
      return byteArray;
    }
  }
}

The class is OK as is, if you use it now and then in separate occasions, but if you want to generate more codes at the same time, you could consider to implement a factory like static method:

  public static IEnumerable<string> GenerateTokens(int numberOfBytes = 32)

The benefit of that is that you can avoid the (possible expensive) instantiation of a new number generator for each token.

A revision of the class to accommodate to that could be:

public class NewUrlToken : IDisposable
{
  RNGCryptoServiceProvider _provider = new RNGCryptoServiceProvider();
  int _numberOfBytes;

  public NewUrlToken(int numberOfBytes)
  {
    _numberOfBytes = numberOfBytes;
  }

  /// <summary>
  /// Generate a cryptographically secure array of bytes with a fixed length
  /// </summary>
  /// <returns></returns>
  private byte[] GenerateRandomBytes()
  {
    byte[] byteArray = new byte[_numberOfBytes];
    _provider.GetBytes(byteArray);
    return byteArray;
  }

  public void Dispose()
  {
    // TODO Implement the proper Disposable pattern.
    if (_provider != null)
    {
      _provider.Dispose();
      _provider = null;
    }
  }

  private string GenerateToken()
  {
    return WebEncoders.Base64UrlEncode(GenerateRandomBytes());
  }

  /// <summary>
  /// Generate a fixed length token that can be used in url without endcoding it
  /// </summary>
  /// <returns></returns>
  public static string GenerateToken(int numberOfBytes = 32)
  {
    return GenerateTokens(numberOfBytes).First();
  }

  public static IEnumerable<string> GenerateTokens(int numberOfBytes = 32)
  {
    using (NewUrlToken factory = new NewUrlToken(numberOfBytes))
    {
      while (true)
      {
        yield return factory.GenerateToken();
      }
    }
  }
}

Usage:

  foreach (string token in NewUrlToken.GenerateTokens().Take(10))
  {
    Console.WriteLine(token);
  }
\$\endgroup\$

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.