AspNetCore.LegacyAuthCookieCompat icon indicating copy to clipboard operation
AspNetCore.LegacyAuthCookieCompat copied to clipboard

HashProvider constructor breaks other HashProviders

Open edumbell opened this issue 2 years ago • 1 comments

I would like to be able to instantiate two LegacyFormsAuthenticationTicketEncryptor with different algorithms, to support legacy cookies and newer cookies going forward. Suggestions for a workaround would be very welcome! (If there is something in Core that does a similar job of asymmetric + symmetric, path keys simply passed in as byte[]/strings, that would be better)

from the source, here is the problem:

public abstract class HashProvider
    {
        private **static** int _HashSize;
...
        protected HashProvider(byte[] validationKey, int hashSize, int keySize)
        {
            **_HashSize = hashSize;**

reproduce in practise w/ this failing unit test:

            var ticket = new FormsAuthenticationTicket(2, "hi", DateTime.Now, DateTime.Now, true, "asdf", "123");
            var decryptionKey = HexUtils.HexToBinary("000000000000000000000000000000000000000000000000");
            var validationKey = HexUtils.HexToBinary("00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000");
            var encryptorSha256 = new LegacyFormsAuthenticationTicketEncryptor(decryptionKey, validationKey, ShaVersion.Sha256, CompatibilityMode.Framework20SP2);
            // static _HashSize is now 32
            var encryptorSha1 = new LegacyFormsAuthenticationTicketEncryptor(decryptionKey, validationKey, ShaVersion.Sha1, CompatibilityMode.Framework20SP2);
            // static _HashSize is now 20
            var encrypted = encryptorSha256.Encrypt(ticket);
            encryptorSha256.DecryptCookie(encrypted); // fails, expecting hash size of 20 (sha1) not 32 (sha256)

suggested fix: just make HashProvider._HashSize and HashProvider._KeySize instance variables

edumbell avatar Mar 14 '23 13:03 edumbell

Hi. I would accept a PR that changes these to instance variables as per the suggestion, along with the added test coverage shown above, for anyone kind enough to submit one.

dazinator avatar Mar 15 '23 07:03 dazinator