-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unnecessary cryptographic derived types obsoletions #52303
Conversation
Refresh repository
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue DetailsClose #46934 I have questions that I need your help to resolve:
So, what should I do with obsolete classes here Line 45 in 94e4e3f
Line 50 in 94e4e3f
and here?
#pragma warning disable SYSLIB0021
#pragma warning restore SYSLIB0021 content in test files for classes now marked as
|
For PasswordDeriveBytes and CryptoConfig, just pragma suppress around their usages. For the System.Security.Cryptography.Algorithms.Tests and System.Security.Cryptography.Csp.Tests libraries, NoWarn in the csproj. |
@bartonjs thanks for the instructions, it helped. I also added documentation for the new obsoletion :) |
@@ -1,6 +1,7 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<PropertyGroup> | |||
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks> | |||
<NoWarn>$(NoWarn);SYSLIB0021</NoWarn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a reason why System.Runtime.Serialization.Xml.Tests would need the NoWarn.
If it's explicitly testing serialization for one of the obsoleted types, a pragma around that/those test(s) is more appropriate. Otherwise, it would probably be a place that's doing the old/wrong pattern that should be updated to better code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem was here.
According to the errors the problematic lines are
static MD5CryptoServiceProvider md5 = null; // <--- line 211
and
if (md5 == null)
md5 = new MD5CryptoServiceProvider(); // <--- line 270
The method using this type:
runtime/src/libraries/System.Runtime.Serialization.Xml/tests/SerializationTestTypes/DataContract.cs
Line 267 in 1b30c00
private static string GetNamespacesDigest(string namespaces) |
What then needs to be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this code is just an example of the bad pattern. So we should fix it 😄
- Change the field type from MD5CryptoServiceProvider to MD5
- Change the ctor call to MD5.Create()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for the instructions 🙂 Just fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @annchous! 💯
docs/project/list-of-diagnostics.md
Outdated
@@ -70,6 +70,7 @@ The PR that reveals the implementation of the `<IncludeInternalObsoleteAttribute | |||
| __`SYSLIB0018`__ | ReflectionOnly loading is not supported and throws PlatformNotSupportedException. | | |||
| __`SYSLIB0019`__ | RuntimeEnvironment members SystemConfigurationFile, GetRuntimeInterfaceAsIntPtr, and GetRuntimeInterfaceAsObject are no longer supported and throw PlatformNotSupportedException. | | |||
| __`SYSLIB0020`__ | JsonSerializerOptions.IgnoreNullValues is obsolete. To ignore null values when serializing, set DefaultIgnoreCondition to JsonIgnoreCondition.WhenWritingNull. | | |||
| __`SYSLIB0021`__ | Derived cryptographic types are obsolete. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When possible, the message should also indicate what should be done to resolve the warning. Is the following accurate, @bartonjs?
| __`SYSLIB0021`__ | Derived cryptographic types are obsolete. | | |
| __`SYSLIB0021`__ | Derived cryptographic types are obsolete. Use the Create method on the underlying type instead. | |
When this is changed, it will need to be updated in Obsoletions.cs
and the ref
sources too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's technically correct. I don't know if "underlying type" is the best wording, but I can't come up with an obviously better one.
- "algorithm's abstract class"
- "abstract class representing the algorithm"
- "base type"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gewarren do you have a suggestion for a message better than this?
Derived cryptographic types are obsolete. Use the Create method on the underlying type instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest "...Use the Create method on the base type instead."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest "...Use the Create method on the base type instead."
@jeffhandley apply this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please. Thanks!
Side note -- I'm sorry about the trouble with the project file; I hope you didn't mind that I pushed commits to fix it. Since that was an existing issue unrelated to your changes, I didn't want to burden you with fixing that up. I'm glad this PR uncovered it though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note -- I'm sorry about the trouble with the project file; I hope you didn't mind that I pushed commits to fix it. Since that was an existing issue unrelated to your changes, I didn't want to burden you with fixing that up. I'm glad this PR uncovered it though!
It's okay, of course I was not against helping with the resolution of problems in the project file! Finished with the documentation :)
....Security.Cryptography.Algorithms/tests/System.Security.Cryptography.Algorithms.Tests.csproj
Show resolved
Hide resolved
...braries/System.Security.Cryptography.Csp/tests/System.Security.Cryptography.Csp.Tests.csproj
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Csp/src/System.Security.Cryptography.Csp.csproj
Outdated
Show resolved
Hide resolved
My PR (#52366) caused merge conflicts. I'll resolve them and push to your branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, pending @gewarren's feedback on the message.
Commit f8067e9 is causing an error because of I'm going to push another commit that de-dupes the contents from those item groups to make this file more maintainable. |
…onfigs. Ensure Obsoletions.cs is included in all configs.
I need to mark this as no-merge for a little bit. We want to get out ahead of some upstream breaks this is going to cause in https://github.com/dotnet/aspnetcore. I'll merge this PR once we have aspnetcore fixed to use the new recommendations. |
dotnet/aspnetcore#32511 was merged, clearing the way for this PR to get merged. Thank you for this contribution, @annchous! I'm going to set the PR to auto-merge once the CI passes with the latest changes from main merged in. |
Hello @jeffhandley! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
It looks like my merge from main (done using the GitHub UI) brought back duplicate |
One of the "Libraries Test Run release mono Linux x64 Debug" test failures was #44689. Rerunning since others looked to be connectivity-related. |
Close #46934
I have questions that I need your help to resolve:
So, what should I do with obsolete classes here
runtime/src/libraries/System.Security.Cryptography.Csp/src/System/Security/Cryptography/PasswordDeriveBytes.cs
Line 45 in 94e4e3f
runtime/src/libraries/System.Security.Cryptography.Csp/src/System/Security/Cryptography/PasswordDeriveBytes.cs
Line 50 in 94e4e3f
and here?
content in test files for classes now marked as
obsolete
.But for
System.Security.Cryptography.Csp
there is a CreateTransformCompat class which also uses the classes now marked asobsolete
.The same problem exists in
System.Security.Cryptography.Algorithms
for individual tests and for CryptoConfigTests.