5
\$\begingroup\$
public static class SimpleToken
{
    const string TOKENALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-.";
    static string NewToken(int length = 16)
    {
        var rnd = new RNGCryptoServiceProvider();
        var tokenBytes = new byte[length];
        rnd.GetBytes(tokenBytes);
        var token =
            Enumerable
                .Range(0, length)
                .Select(i => TOKENALPHABET[tokenBytes[i] % TOKENALPHABET.Length])
                .ToArray();

        return new String(token);
    }
}

I needed a quick and dirty(?) way to generate long urls for onetime use. It's a simple login-scheme where a user enters his e-mail and gets a one-time URL for logging in. The URL is discarded after one use.

I.e. http://example.com/tokenlogin/3cuzLkh8GcANjqnWcijEeJIHphHx6ZDwfj-2XTR4bfkkqmzmmFYAY2tWsZWST1.5

\$\endgroup\$
  • 2
    \$\begingroup\$ RNGCryptoServiceProvider implements IDisposable, so put it in a using block. \$\endgroup\$ – Brad M Dec 1 '17 at 19:47
  • \$\begingroup\$ What are your concerns? Maybe give us some hints so we can try to be more precise in our suggestions :) \$\endgroup\$ – kuskmen Dec 1 '17 at 22:49
  • \$\begingroup\$ Encode the token in base64. see: Why base64? \$\endgroup\$ – Feras Dec 2 '17 at 0:18
  • \$\begingroup\$ If you use Base64 then you will need the URL-safe version of Base64. See RFC 4648 Setion 5. \$\endgroup\$ – rossum Dec 3 '17 at 21:10
  • \$\begingroup\$ But why would I want to use Base64? I don't have a source value to encode. I only want to generate a random string for use as a one-time token. And I want it to be a certain length because longer looks more secure (client thing). My real concerns was just that it looked so simple that I guessed I might have overlooked something :) \$\endgroup\$ – Christian Wattengård Dec 4 '17 at 9:48
3
\$\begingroup\$
    const string TOKENALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-.";

I learnt the hard way that the problem with using . in a URL token sent by e-mail is that certain mail clients (Outlook in particular) will attempt to auto-detect URLs in a plain text email, but will exclude a trailing . from the inferred URL, so when your user clicks on the auto-generated link they send an invalid token. I suggest that you change . to _.


    static string NewToken(int length = 16)

Length in what units? Generally with cryptographic stuff it's clearer to explicitly use bits as the unit of length.


        var rnd = new RNGCryptoServiceProvider();

As already noted in comments, this is IDisposable and the standard using pattern is preferred.


        var token =
            Enumerable
                .Range(0, length)
                .Select(i => TOKENALPHABET[tokenBytes[i] % TOKENALPHABET.Length])
                .ToArray();

Firstly, this could be simplified to

        var token =
            tokenBytes
                .Select(b => TOKENALPHABET[b % TOKENALPHABET.Length])
                .ToArray();

But secondly, by only using 6 bits per byte you're throwing away 25% of the entropy which the system just produced for you. On busy servers, cryptographic-grade entropy is a valuable resource and you should only request as much as you need.

\$\endgroup\$
  • \$\begingroup\$ Thank you. That looks nice. I think I'll keep the length parameter as it is, this function isn't primarily of a cryptographic nature, it's more just to generate a pseudo-random string of length n, hence the length specified in bytes. \$\endgroup\$ – Christian Wattengård Mar 6 '18 at 12:42
-3
\$\begingroup\$

If you just want to have one-time use random URL, i change your code and now

  • It,s cleaer
  • Much faster than your code
  • Really random string
  • URL safe

    public static string NewToken()
    {
        using (RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider())
        {
            byte[] randomBuffer = new byte[16];
            rng.GetBytes(randomBuffer);
    
            using (MD5 md5 = MD5.Create())
            {
                byte[] hashBytes = md5.ComputeHash(randomBuffer);
    
                StringBuilder sBuilder = new StringBuilder();
                foreach (byte byt in hashBytes)
                {
                    sBuilder.Append(byt.ToString("x2"));
                }
    
                return sBuilder.ToString();
            }
        }
    }
    

You can change the method to combine random generated buffer with user's email address and then calculate hash of theme, it's more reliable.

\$\endgroup\$
  • 3
    \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. \$\endgroup\$ – Dannnno Mar 1 '18 at 13:53
  • 2
    \$\begingroup\$ It's not that I don't understand what you did. It's that you haven't actually reviewed his code, you've just put up your own version without explaining in your answer why it is an improvement, hence my downvote. I'd happily retract once that is added to the question \$\endgroup\$ – Dannnno Mar 1 '18 at 14:12
  • 2
    \$\begingroup\$ "hash functions guarantee that the result is unique if the input is unique" is not only wrong but obviously wrong. Every standard hash function has collisions because its output space is smaller than its input space. It may be hard to find a collision, but that's not the same thing. \$\endgroup\$ – Peter Taylor Mar 1 '18 at 16:17

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.