Generate secure URL safe token

Question

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?

Answer 1

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!

Answer 2

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);
  }