15
\$\begingroup\$

This is code that's part of a library (closed source) that I have that generates a unique nonce value for Twitter OAuth. Essentially, it generates a random number, combines it in string form with an extra string separated by a pipe, computes a Sha-1 hash, and then repeats the process if the Sha-1 hash (when converted to Base64) contained any non-alpha-numeric characters, which are subsequently stripped.

/// <summary>
/// Generates a (presumably) unique NONCE string for use in Twitter OAuth requests.
/// </summary>
/// <param name="extra">Extra data to add during generation.</param>
/// <returns>
/// A (presumably) unique NONCE string.
/// </returns>
/// <remarks>
/// While we can assume the generated NONCE strings will be somewhat unique, there is a small possibility that they will not be, and in such cases a new NONCE should be generated.
/// 
/// The generated NONCE values are 32 characters in length, and are generated by a Sha-1 hash and a Random generator, which generates four random numbers to be hashed until the generated string is 32 characters in length.
/// </remarks>
public string GenerateNonce(string extra = "")
{
    string result = "";
    SHA1 sha1 = SHA1.Create();

    Random rand = new Random();

    while (result.Length < 32)
    {
        string[] generatedRandoms = new string[4];

        for (int i = 0; i < 4; i++)
        {
            generatedRandoms[i] = rand.Next().ToString();
        }

        result += Convert.ToBase64String(sha1.ComputeHash(Encoding.ASCII.GetBytes(string.Join("", generatedRandoms) + "|" + extra))).Replace("=", "").Replace("/", "").Replace("+", "");
    }

    return result.Substring(0, 32);
}

The idea is to generate somewhat unique nonce strings for use with Twitter OAuth (et al.).

I use the term "presumably," because, while it's not considered a bug that the nonce strings will not be guaranteed to be unique, collisions are expected to be fairly rare. I do welcome any and all suggestions that would lead to more uniqueness.

\$\endgroup\$
21
\$\begingroup\$

If I understand the requirements correctly, Guid.NewGuid().ToString("N") would work for generating a nonce.

A GUID is a 128-bit integer (16 bytes) that can be used across all computers and networks wherever a unique identifier is required. Such an identifier has a very low probability of being duplicated.

The "N" format specifier will return the GUID as 32 (lowercase) hexadecimal digits.

\$\endgroup\$
  • \$\begingroup\$ I feel like an idiot for missing that solution. \$\endgroup\$ – 410_Gone Oct 7 '15 at 22:41
8
\$\begingroup\$

Although @mjolka has provided the way to go, I would like to review the code nevertheless.

public string GenerateNonce(string extra = "")
{
    string result = "";
    SHA1 sha1 = SHA1.Create();

    Random rand = new Random();

    while (result.Length < 32)
    {
        string[] generatedRandoms = new string[4];

        for (int i = 0; i < 4; i++)
        {
            generatedRandoms[i] = rand.Next().ToString();
        }

        result += Convert.ToBase64String(sha1.ComputeHash(Encoding.ASCII.GetBytes(string.Join("", generatedRandoms) + "|" + extra))).Replace("=", "").Replace("/", "").Replace("+", "");
    }

    return result.Substring(0, 32);
}  

There are a few things that bothers me.

  1. the creation of the string[] generatedRandoms in a loop and later on using string.join() with an empty string as separator

    This should be replaced using a StringBuilder like so

    public static string GenerateNonce(string extra = "")
    {
        string result = "";
        SHA1 sha1 = SHA1.Create();
    
        Random rand = new Random();
        StringBuilder sb = new StringBuilder(1024);
        while (result.Length < 32)
        {
            sb.Length = 0;
            string[] generatedRandoms = new string[4];
    
            for (int i = 0; i < 4; i++)
            {
                sb.Append(rand.Next());
            }
    
            sb.Append("|")
                .Append(extra);
    
            result += Convert.ToBase64String(sha1.ComputeHash(Encoding.ASCII.GetBytes(sb.ToString()))).Replace("=", "").Replace("/", "").Replace("+", "");
        }
    
        return result.Substring(0, 32);
    }
    
  2. this very long line of code where you add to the result

    This could be made pretier by using multiple lines like so

    result += Convert.ToBase64String(
        sha1.ComputeHash(Encoding.ASCII.GetBytes(sb.ToString()))
        ).Replace("=", "")
         .Replace("/", "")
         .Replace("+", "");  
    

    in this way one wouldn't need to scroll to the right that much

  3. the usage of Replace to replace a = from the string

    For a base64 string the only place a = can occur will be at the end of the string. So instead of using Replace you could do a simple TrimEnd which is a lot faster.

    public static string GenerateNonce(string extra = "")
    {
        string result = "";
        SHA1 sha1 = SHA1.Create();
    
        Random rand = new Random();
        StringBuilder sb = new StringBuilder(1024);
        while (result.Length < 32)
        {
            sb.Length = 0;
            string[] generatedRandoms = new string[4];
    
            for (int i = 0; i < 4; i++)
            {
                sb.Append(rand.Next());
            }
    
            sb.Append("|")
                .Append(extra);
    
            result += Convert.ToBase64String(
                sha1.ComputeHash(Encoding.ASCII.GetBytes(sb.ToString()))
                ).TrimEnd('=')
                 .Replace("/", "")
                 .Replace("+", "");
        }
    
        return result.Substring(0, 32);
    }
    
\$\endgroup\$
  • \$\begingroup\$ This is exactly what I wanted it to do - the only change I made was replacing string result with a StringBuilder as well. \$\endgroup\$ – 410_Gone Oct 8 '15 at 18:51

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.