Skip to content
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

DataProtection: Move away from soon to be obsolete APIs #32511

Merged
merged 1 commit into from
May 10, 2021
Merged

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented May 7, 2021

Preparation for runtime obsoleting these APIs in an incoming runtime PR

@ghost ghost added the area-dataprotection Includes: DataProtection label May 7, 2021
@HaoK HaoK changed the title DataProtection: Move away from obsolete APIs DataProtection: Move away from soon to be obsolete APIs May 7, 2021
@HaoK HaoK requested review from GrabYourPitchforks, jeffhandley and a team May 10, 2021 16:35
@HaoK HaoK marked this pull request as ready for review May 10, 2021 16:35
@HaoK HaoK requested a review from dougbu May 10, 2021 16:35
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}

return factory ?? Aes.Create;
return Aes.Create;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the FIPS compliance statement no longer valid?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AesCryptoServiceProvider is being obsoleted, not sure if the base API is FIPS compliant?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, but that's in netcore, no? Aes in net461 is probably not FIPs compliant. Do we need to cross-compile this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GrabYourPitchforks is this safe to just always use Aes.Create now?

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Keep in mind that since you're cross-compiling for downlevel, if you need to change how hash algorithm factories work, you'll want to ifdef core vs. full framework so that you call the correct overload to maintain FIPS compliance.

For encryption algorithms like AES / 3DES, your PR doesn't affect FIPS compliance, so things look good.

@HaoK
Copy link
Member Author

HaoK commented May 10, 2021

Keep in mind that since you're cross-compiling for downlevel, if you need to change how hash algorithm factories work, you'll want to ifdef core vs. full framework so that you call the correct overload to maintain FIPS compliance.

I'm not sure I understand the issue here, but I'm assuming you are talking about possible future changes only?

@GrabYourPitchforks
Copy link
Member

I'm assuming you are talking about possible future changes only?

Correct. I mean that on Full Framework and netstandard, you should continue to prefer things like new SHA256Cng over SHA256.Create. The latter may fail.

For AES and 3DES (the algorithms touched as part of this PR), this isn't a concern. I just wanted to alert you to something that you might have to consider in a future PR.

@HaoK HaoK merged commit 9445d00 into main May 10, 2021
@HaoK HaoK deleted the haok/datap branch May 10, 2021 21:50
@ghost ghost added this to the 6.0-preview5 milestone May 10, 2021
@jeffhandley
Copy link
Member

@dougbu With this merged, is ASP.NET now clear for dotnet/runtime#52303 to be merged?

@ghost
Copy link

ghost commented May 10, 2021

Hi @jeffhandley. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@dougbu
Copy link
Member

dougbu commented May 10, 2021

Worst that will happen is the dependency PR needs a bit more work @jeffhandley 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dataprotection Includes: DataProtection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants