7
\$\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\$
8
  • 2
    \$\begingroup\$ RNGCryptoServiceProvider implements IDisposable, so put it in a using block. \$\endgroup\$ Commented Dec 1, 2017 at 19:47
  • 1
    \$\begingroup\$ What are your concerns? Maybe give us some hints so we can try to be more precise in our suggestions :) \$\endgroup\$ Commented Dec 1, 2017 at 22:49
  • \$\begingroup\$ Encode the token in base64. see: Why base64? \$\endgroup\$ Commented Dec 2, 2017 at 0:18
  • \$\begingroup\$ If you use Base64 then you will need the URL-safe version of Base64. See RFC 4648 Setion 5. \$\endgroup\$ Commented Dec 3, 2017 at 21:10
  • 1
    \$\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\$ Commented Dec 4, 2017 at 9:48

2 Answers 2

Reset to default
5
\$\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\$
1
  • \$\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\$ Commented Mar 6, 2018 at 12:42
-4
\$\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\$
4
  • 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\$ Commented Mar 1, 2018 at 13:53
  • 3
    \$\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\$ Commented Mar 1, 2018 at 14:12
  • 4
    \$\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\$ Commented Mar 1, 2018 at 16:17
  • \$\begingroup\$ md5(random16bytes) can never be safer than random16bytes. If any, it is less random and therefore less safe. \$\endgroup\$ Commented May 26, 2020 at 23:42

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.