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.

Sign up for GitHub

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

Jump to bottom

Moving to approved cryptographic hash function (SHA256) #8843

Merged
merged 15 commits into from
Jun 23, 2023
Merged

Moving to approved cryptographic hash function (SHA256) #8843

merged 15 commits into from
Jun 23, 2023

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented Jun 19, 2023 •
edited
Loading

Summary of the changes

I noticed usages of SHA1 or MD5 hash functions and in this PR I am changing them to SHA256. They are mainly used for computing checksum so it is arguably not a big risk if they remained as-is.

I noticed that on roslyn repo there's also some usages of unapproved hash functions (e.g. here)

cc @333fred

As part of the review here, would be good to learn if the existing hash functions are expected (would there be a risk if we changed them to SHA256?)

Fixes: workitem 1804707

@maryamariyan maryamariyan requested review from a team as code owners June 19, 2023 21:25
@maryamariyan maryamariyan self-assigned this Jun 19, 2023
@maryamariyan
Copy link
Member Author

Investigating locally the test failure...

@davidwengier
Copy link
Contributor

We should probably move fully off SHA1. If there are test failures, it could just be that we need to update the baselines, as they would presumably have the checksum in them.

@jaredpar
Copy link
Member

jaredpar commented Jun 19, 2023 •
edited
Loading

I noticed that on roslyn repo there's also some usages of unapproved hash functions

The Roslyn compiler repo is more complicated because of places where we have to maintain old crypto for legacy reasons. That particular case is just a tool for build validation. It's not part of the prooduct though so it doesn't need to go through approval (to my understanding).

@maryamariyan
Copy link
Member Author

Completed my changes here. This PR is ready for another review

davidwengier
davidwengier reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Apologies for typos in my comments, GitHub does not enjoy the size of this PR :)

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/CodeGeneration/DefaultDocumentWriter.cs Outdated Show resolved Hide resolved
src/Compiler/Microsoft.AspNetCore.Razor.Language/src/CodeGeneration/DefaultDocumentWriter.cs Outdated Show resolved Hide resolved
src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/HashAlgorithmOperations.cs Outdated Show resolved Hide resolved
davidwengier
davidwengier approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Couple of nits, but looks good to me. Signing off on the tooling changes.

...t/Microsoft.AspNetCore.Razor.Test.Common/Language/IntegrationTests/IntermediateNodeWriter.cs Outdated Show resolved Hide resolved
src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/SourceTextExtensionsTest.cs Show resolved Hide resolved
@maryamariyan
code cleanup
02651ed
@maryamariyan
Merge remote-tracking branch 'origin/main' into fix-sha
5c1af9a 
 Conflicts:
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/Basic_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/IncompleteDirectives_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/InheritsViewModel_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/InheritsWithViewImports_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/InjectWithModel_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/InjectWithSemicolon_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/Inject_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/InvalidNamespaceAtEOF_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/MalformedPageDirective_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/ModelExpressionTagHelper_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/Model_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/PageWithNamespace_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/RazorPageWithNoLeadingPageDirective_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/RazorPage_WithCssScope.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/RazorView_Layout_WithCssScope.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/RazorView_WithCssScope.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/Sections_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/ViewComponentTagHelperOptionalParam_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/ViewComponentTagHelper_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/ViewWithNamespace_Runtime.codegen.cs
	src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions/test/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/_ViewImports_Runtime.codegen.cs
@maryamariyan
Copy link
Member Author

maryamariyan commented Jun 22, 2023 •
edited
Loading

pushed up a merge commit, needed to resolve conflicts with merged PR https://github.com/dotnet/razor/pull/8857/files on some of the *.codegen.cs integration test files.

@maryamariyan maryamariyan enabled auto-merge June 22, 2023 21:31
@maryamariyan maryamariyan merged commit de8d791 into main Jun 23, 2023
@maryamariyan maryamariyan deleted the fix-sha branch June 23, 2023 08:41
@allisonchou allisonchou mentioned this pull request Jun 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwengier davidwengier davidwengier approved these changes

@DustinCampbell DustinCampbell DustinCampbell left review comments

@chsienki chsienki chsienki approved these changes

@333fred 333fred 333fred approved these changes

@jaredpar jaredpar jaredpar approved these changes

Assignees

@maryamariyan maryamariyan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maryamariyan @davidwengier @jaredpar @DustinCampbell @333fred @chsienki