Obsolete HMACSHA1 constructor with useManagedSha1

[vcsjones]

Background and Motivation

HMACSHA1 has a constructor that the other HMACSHA2/MD5 classes don't, which is to accept a boolean indicating if SHA1Managed should be used as the underlying hash. As of .NET Core, the HMAC implementations are not managed themselves anymore, but instead defer to the platform. As such the useManagedSha1 parameter is ignored.

Proposed API

namespace System.Security.Cryptography {
    public class HMACSHA1 : HMAC {
+       [Obsolete("HMACSHA1 always uses the algorithm implementation provided by the platform. Use the constructor without the useManagedSha1 parameter.")]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public HMACSHA1(byte[] key, bool useManagedSha1);
    }

    public class HMACSHA384 : HMAC {
+       [Obsolete("Producing legacy HMAC values is no longer supported.")]
        public bool ProduceLegacyHmacValues { get; set; }
    }

    public class HMACSHA512 : HMAC {
+       [Obsolete("Producing legacy HMAC values is no longer supported.")]
        public bool ProduceLegacyHmacValues { get; set; }
    }
}

An analyzer could be made to update call sites to change simple invocations of the constructor like new HMACSHA1(key_expression, false) and new HMACSHA1(key_expression, true) to new HMACSHA1(key_expression).

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and Motivation

HMACSHA1 has a constructor that the other HMACSHA2/MD5 classes don't, which is to accept a boolean indicating if SHA1Managed should be used as the underlying hash. As of .NET Core, the HMAC implementations are not managed themselves anymore, but instead defer to the platform. As such the useManagedSha1 parameter is ignored.

Proposed API

namespace System.Security.Cryptography {
    public class HMACSHA1 : HMAC {
+       [Obsolete("HMACSHA1 always uses the algorithm implementation provided by the platform. Use the constructor without the useManagedSha1 parameter.")]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public HMACSHA1(byte[] key, bool useManagedSha1);
    }
}

An analyzer could be made to update call sites to change simple invocations of the constructor like new HMACSHA1(key_expression, false) and new HMACSHA1(key_expression, true) to new HMACSHA1(key_expression).

Author:	vcsjones
Assignees:	-
Labels:	api-suggestion, area-System.Security, untriaged
Milestone:	-

[GrabYourPitchforks]

Can we also use this opportunity to obsolete HMACSHA512.ProduceLegacyHmacValues? The property setter throws in .NET Core.

[vcsjones]

Can we also use this opportunity to obsolete HMACSHA512.ProduceLegacyHmacValues? The property setter throws in .NET Core.

Makes sense. I updated the proposal. Maybe it makes more sense for the obsolete to just be on the setter, but I opted to do the whole property since any application that is getting the value and perhaps making a decision from the getter might need to be updated.

[bartonjs]

Video

Looks good as proposed.

• Use a new ID for HMACSHA1..ctor
• Use a different new ID for all the ProduceLegacyHmacValues properties.
  • Double check that HMACSHA256 doesn't have one, too.

namespace System.Security.Cryptography {
    public class HMACSHA1 : HMAC {
+       [Obsolete("HMACSHA1 always uses the algorithm implementation provided by the platform. Use the constructor without the useManagedSha1 parameter.")]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public HMACSHA1(byte[] key, bool useManagedSha1);
    }

    public class HMACSHA384 : HMAC {
+       [Obsolete("Producing legacy HMAC values is no longer supported.")]
        public bool ProduceLegacyHmacValues { get; set; }
    }

    public class HMACSHA512 : HMAC {
+       [Obsolete("Producing legacy HMAC values is no longer supported.")]
        public bool ProduceLegacyHmacValues { get; set; }
    }
}

[vcsjones]

Double check that HMACSHA256 doesn't have one, too.

It does not; only HMACs with a 1024-bit block size had the particular bug in .NET Framework that resulted in these compatibility properties.

[GrabYourPitchforks]

@vcsjones Did you want this assigned to you?

[vcsjones]

@GrabYourPitchforks I can at least do that much now :-). Will start this one after the X.509 obsoletions.