Annotate unsupported APIs in System.Security.* #50528
Comments
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue DetailsAs part of #47228 i am running an analyzer to detect APIs throwing PNSE but not annotated with Obsolete/SupportedOSPlatform/UnsupportedOSPlatform attributes, we need to annotate them so that developers get warnings when they use them unexpectedly For now, I have results only cross-platform and .Net 5.0 or higher builds, analysis of targeted builds or lower env are coming soon where more APIs could be detected
Note: We are suggesting to add [Obsolete] with the corresponding message for APIs supported in .Net framework but not supported in .NetCore, if it is only supported on a specific platform(s) consider adding [SupportedOSPlatform("platformName")] or if it is unsupported on a specific platform(s) [UnsupportedOSPlatform("platformName")] attribute instead Suggestions for changing the exception is optional cc @jeffhandley @terrajobst @GrabYourPitchforks
|
dotnet-issue-labeler
bot
added
the
untriaged
|
Notes need to consider:
|
marek-safar
removed
the
untriaged
|
Some of these APIs that "unconditionally throw" are virtual and are expected to be overridden in derived classes, so while the base class throws, they aren't necessarily obsolete because derived classes override them and some functionality depend on these classes. I think this would be the case for all of the methods on the |
That is right, removed HMAC related APIs, but others looks valid |
|
Adding few more:
CC @bartonjs |
The public entry, So I think that one is already taken care of. |
|
Removed, thanks |
|
@buyaa-n is this something you are working on or is this something I can contribute to? Some of these look reasonable. |
Not really, my intent here (and all other issues i filed) was to detect the PNSE and let the area owners know because some of the findings might be invalid or need more investigation. For now, I resolved this only for the areas I own, but if the security area owners are OK with the proposed change then i can go ahead and do updates
Sure, that would be great, thanks! |
I'm not sure adding an using System.Runtime.Versioning;
Foo f = new Bar();
f.Do(); // This is actually a `Bar` but since it's called through the base type, a compiler warning appears.
public class Foo
{
[UnsupportedOSPlatform("iOS")]
public virtual object Do() => throw new PlatformNotSupportedException();
}
public class Bar : Foo
{
public override object Do() => new();
}Similar concerns lie with Some of these do make sense to me to obsolete:
I don't see any immediately actionable things for 6.0. Presumably the obsoletions need to go through API review, and I don't know if we're taking any more obsoletions for 6.0 (granted all of them |
|
We could take these 3 obsoletions without going through API Review--just code review from @bartonjs and @GrabYourPitchforks. I'm comfortable taking them before the August 17th cutoff for .NET 6, especially since it's not a breaking change. |
buyaa-n
changed the title
|
Adding last findings, these are all internal APIs / overrides in the different platform source PALs, might be false positive but might worth checking
|
It does look like we can annotate |
Just to summarize findings that I think are actionable:
It would still be useful to have someone else validate this. |
For RC2-browser we seem to have marked the whole type, which was probably a bit of an overreach, because someone /could/ provide a working implementation of RC2. RC2.Create() on android I'm pretty sure succeeds, but returns an object that fails if you try to actually use it. Part of me thinks it'd be better to just put the PNSE there, but maybe there's a legit use case I can think of for the dummy RC2 instance. So... yeah, marking RC2.Create as Unsupported-android is probably right. With a comment that says a more formal version of "yeah, it returns, but the object is going to throw if you call CreateEncryptor or CreateDecryptor... so it's the same as not working" As for other things in @buyaa-n 's tables:
I think that only leaves ECDiffieHellmanPublicKey.ToByteArray and ECDiffieHellmanPublicKey..ctor(byte[]). I'd support Obsoleting these. I... thought... we had added SPKI export on this type, but I don't see it. So maybe we need to hold this one back one release. Using inbox types the ToByteArray only works on Windows, but since it's a virtual I don't like putting a SupportedOSAttribute on it. |
We did, unless I am misunderstanding what API you're thinking of. |
|
I'll tackle the rest of these over the next day or two. |
|
No, that's it. Just that I wasn't smart enough to remember to look in a partial file. |
Just so I am super clear, we are okay obsoleting a virtual on an abstract class that developers can inherit from for a RC? Or would it be better to push those for .NET vNext? |
I am. @ericstj or @danmoseley would you care to object? If there are more than three NuGet packages that have extended ECDiffieHellmanPublicKey I'll... I don't know... be flabbergasted. ("MC an API Review while only speaking French" (a language I don't know) sounds like a fun consequence, but would be incredibly unproductive). Even then, I doubt they'd've implemented To/FromByteArray in a way that interoperated with the inbox CNG one. Since https://github.com/novotnyllc/RSAKeyVaultProvider/tree/master/RSAKeyVaultProvider didn't do ECDH I'll even lower that expectation to two derived types. |
I need to figure out how to publish a few NuGet packages, then. Fair enough, just thought I'd ask. Will do those next then. |
Had there been an actual "eat my hat" type consequence I definitely would have thrown in a disclaimer like "(offer valid only for non-prerelease packages already present on nuget.org at the original time of this post)". I may not be smart enough to remember to look in partial files, but I'm not that naive 😄. |
|
@marklio is this an assumption we can easily verify from Nuget packages? |
|
I can look at this. I'll follow up with @bartonjs offline. |
|
OK, the landscape here is complicated, but in general it looks like @bartonjs's wager holds up, particularly through the lens of "ecosystem code exposed to such a change". I definitely found types that inherit from System.Security.Cryptography.ECDiffieHellmanPublicKey, but almost universally these are types in our own shipped assemblies (System.Core, System.Security.Cng/Algorithms/OpenSsl). These assemblies are in our own packages, as well as redistributed in other packages at a pretty high rate. None of those really represent problematic scenarios WRT deprecation. The few that were genuinely non-product types are in assemblies that are not published on NuGet.org, and instead come from other locations like internal test trees/tools. |
|
Yeah, I knew it. Obscure type, no clear implementation. No derivers. Obvs. Thanks, @marklio 😄. |
|
@bartonjs I was not able to get to the rest of these before the 6.0 branch was taken. Should this be moved to 7.0.0 or should I prioritize this so that it can get ported to the 6.0 branch (not sure that's even wise / a possibility)? |
|
Ah, yes. I'm guessing the answer will be move to 7 at this point. But if @danmoseley is happy to take another couple of obsoletions then I'd personally be happier to see them knocked out sooner. |
|
Based on "Obscure type, no clear implementation. No derivers." I would be comfortable porting these into release/6.0-rc1 if @danmoseley is. We would need tactics approval. Or we could take them in RC2 with just Dan's approval. 7.0 also wouldn't upset me. |
|
Moved to 7.0.0. |
I didn't quite take this as an affirmative "let's obsolete these". Do we want to obsolete |
Yes, please. The method is easy:
The constructor is... harder? The best message I've come up with is "The bytes-based constructor for ECDiffieHellmanPublicKey does not have an associated standard data format. This constructor only provides the bytes to the default implementation of the obsolete ToByteArray method, and is not necessary to call from a derived type." But that's clunky and awkward. So maybe that one is better as a docs-remark instead of an Obsolete. |
Yeah. But then it feels weird to have a constructor that takes the key blob and no not-obsolete way to do anything with it. Since it's a protected constructor on a class that no one is deriving from I would wager we could take a swing at obsoleting it with less than perfect wording. |
|
"ECDiffieHellmanPublicKey.ToByteArray() and the associated constructor do not..." might work? |
ghost
added
the
in-pr
|
@bartonjs What do you think about obsoleting |
|
I'm not really a fan of obsoleting types, as that makes every member declaration that uses it have to suppress the obsoletion. I think we just ignore it. |
|
That’s why we added the diagnostic ID. This way, you can add a global supression that’s still targeted. Existing code would do that, new code wouldn’t use it. Not sure that’s justified here though. |
I would imagine it would get obsolete under SYSLIB0042, if it were pursued. I don't know if there is telemetry on this, but I would be surprised if developers were using For dotnet/runtime members, it would only be half a dozen or so suppression I suspect, which seems manageable. From my own searching on GitHub, I didn't see a single use of |
ghost
removed
the
in-pr
ghost
locked as resolved and limited conversation to collaborators
As part of #47228 i am running an analyzer to detect APIs throwing PNSE but not annotated with Obsolete/SupportedOSPlatform/UnsupportedOSPlatform attributes, we need to annotate them so that developers get warnings when they use them unexpectedly
For now, I have results only for cross-platform builds, analysis of targeted builds are coming soon where more APIs could be detected
EDIT: Adding APIs found for
netstandardtargeted buildsNote: We are suggesting to add [Obsolete] for the APIs only supported in .Net framework but not supported in .NetCore, with the corresponding Message, DiagnosticId, and UrlFormat
runtime/src/libraries/System.Net.Requests/src/System/Net/AuthenticationManager.cs
Line 17 in cecc76a
If it is only supported on a specific platform(s) consider adding [SupportedOSPlatform("platformName")] or if it is unsupported on a specific platform(s) [UnsupportedOSPlatform("platformName")] attribute instead
cc @jeffhandley @terrajobst @GrabYourPitchforks
The text was updated successfully, but these errors were encountered: