[Key Vault] Add warnings on RSA1_5 and RSA_OAEP encryption algorithms

[JonathanCrd]

Fixes #47994

Adding warnings from Azure/azure-rest-api-specs#32289 to RSA1_5 and RSA_OAEP encryption algorithms manually written docs.

[JonathanCrd]

Do you think it would be worth adding an Obsolete attribute to trigger a warning at build?

Suggested change

	public static EncryptionAlgorithm Rsa15 { get; } = new EncryptionAlgorithm(Rsa15Value);
	[Obsolete("Microsoft recommends using RSA_OAEP_256 or stronger algorithms for enhanced security. Microsoft does not recommend RSA_OAEP, which is included solely for backwards compatibility.", false)]
	public static EncryptionAlgorithm Rsa15 { get; } = new EncryptionAlgorithm(Rsa15Value);

[christothes]

It is used elsewhere in the repo, so I think it is OK. I would check with one of the architects to verify and document the decision in the guidelines (There doesn't seem to be any opinion in them now regarding the Obsolete attribute).

[jsquire]

Its a really good thought, and the architects have been moving towards using it recently. In this case, it falls under a security concern, so I think there's justification. I wonder if we should mark it as editor non browsable as well.

[tg-msft]

We historically avoided [Obsolete] because there are a few scenarios where it's problematic that it creates ancient compiler warnings instead of modern analyzer diagnostics (classic ASP.NET had a mode where it'd dynamically compile everything the first time it was requested - which could break if the latest version of a restored dependency added an [Obsolete] and your project was set to treat warnings as errors).

It is used elsewhere in the repo

It looks like mostly management plane and Track 1 per https://grep.app/search?q=%5BObsolete%28%22&filter[repo][0]=Azure/azure-sdk-for-net&filter[path][0]=sdk/. The Track 2 Attestation library was a unique case where we added [Obsolete] on the initial release so it could never source break customers on a restore. That said, I think Krzysztof's been relaxing his concerns around it because we have fewer and fewer customers using ancient web forms in that mode. I also heard the .NET team was considering a new version of [Obsolete] that would just create a diagnostic rather than a warning, but I'm not aware of any progress on that.

I think we should make this change, but also fire off a quick email to Krzysztof with a subject line like "I added [Obsolete] to KV crypto algorithms that we no longer recommend" and scream test for any feedback when he's back. Don't block the PR on it because I don't think you'll hear anything. I wouldn't make it [EditorBrowsable(Never)] yet because then it stops showing up in docs/intellisense. We can consider staging this with an [Obsolete] now and then augmenting with an [EB(Never)] in a future release when service telemetry shows the vast majority of customers have migrated.

I'd also recommend you switch to the C# names rather than the REST constants in your proposed message since that's the lens that people will be seeing this warning through:

- [Obsolete("Microsoft recommends using RSA_OAEP_256 or stronger algorithms for enhanced security. Microsoft does not recommend RSA_OAEP, which is included solely for backwards compatibility.", false)]
+ [Obsolete("Microsoft recommends using EncryptionAlgorithm.RsaOaep256 or stronger algorithms for enhanced security. Microsoft does not recommend EncryptionAlgorithm.Rsa15, which is included solely for backwards compatibility.", error: false)]
  public static EncryptionAlgorithm Rsa15 { get; } = new EncryptionAlgorithm(Rsa15Value);

 

 

 

 

In an ideal world if we had the resources, it would be great to create a custom analyzer for this. It'd both make it easier to globally suppress (i.e., you probably don't want to suppress all [Obsolete]s but you wouldn't mind suppressing all AZ00024s in a legacy KV project if you had to keep using that algorithm for compat) and we could offer a code fix to change to a supported algorithm that you could apply globally across your project in one click.

[jsquire]

We historically avoided [Obsolete] because there are a few scenarios where it's problematic that it creates ancient compiler warnings instead of modern analyzer diagnostics (classic ASP.NET had a mode where it'd dynamically compile everything the first time it was requested - which could break if the latest version of a restored dependency added an [Obsolete] and your project was set to treat warnings as errors).

Krzysztof has been recommending [Obsolete] more freely these days and no longer considers it something to avoid (unless I misunderstood him.) For a recent example, he asked to obsolete members in Event Hubs rather than just EBN them. (#47133) As part of that, we talked over the change in guidance regarding the use of obsolete.

[JonathanCrd]

Thanks everyone for all your comments ❤️

I'd also recommend you switch to the C# names rather than the REST constants in your proposed message since that's the lens that people will be seeing this warning through:

Thanks for pointing this out!

I think Krzysztof's been relaxing his concerns around it because we have fewer and fewer customers using ancient web forms in that mode.

I will talk with Krzysztof then. When adding the [obsolete()] attribute to the src code I noticed that several tests started failing because we treat warnings as errors, so I had to add #pragma warning disable or update them. Apologies for not catching this earlier. That made me worry about the impact this "simple" change could have, especially on Keys, which is widely used.

On the other hand, it helped me realize that I had some code snippets and samples that I needed to update.

[azure-sdk]

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Security.KeyVault.Keys