Introduce generic Hash abstraction as foundation for future algorithms by Trenly · Pull Request #6561 · microsoft/winget-cli

Kaleb Luedtke (Trenly) · 2026-09-23T17:39:57Z

📖 Description

Introduces a reusable CNG-backed hash abstraction in winget/Hash.h while preserving the existing AppInstaller::Utility::SHA256 API. This follow-up also clarifies the algorithm-bound alias name and explicitly preserves move support for hashers while disallowing copying.

🔗 References

Related to #5771; this PR does not implement SHA3-256 or close that issue.

🔍 Validation

src\x64\Debug\AppInstallerCLITests\AppInstallerCLITests.exe '[Cryptography]' — passed: 12 assertions in 5 test cases.
The author reports that the test project was built and the cryptography tests were run successfully.

✅ Checklist

Signed the Contributor License Agreement
Linked to an issue
Updated Release Notes (if applicable)
Updated documentation (if applicable)
Added or updated tests (or noted why not applicable)
Updated Copilot instructions (if build, architecture, or conventions changed)

🤖 AI Assistance

AI assistance was used and has been disclosed in this PR
No AI assistance was used

📋 Issue Type

Bug fix
Feature
Task

* Adds new Hash.cpp in SharedLib to provide base class access to BCrypt hashing functionality * Moves implementation from SHA256 into the the base class * Leaves the SHA256 namespace as a wrapper around the base class

Add a HashAlgorithmTraits/AlgorithmHash template in AppInstallerHash.h that binds the generic Hash engine to a specific algorithm at compile time, exposing the full SHA256-style API (Add, Get, ComputeHash*, ConvertTo*, AreEqual) without per-function forwarding code. Reduce SHA256 to an empty class deriving from AlgorithmHash<HashAlgorithm::Sha256>, and remove the now-unnecessary SHA256.cpp along with its project/filter entries. Adding a new algorithm such as SHA3-256 will only require a new HashAlgorithm enumerator, a HashAlgorithmTraits specialization, CNG metadata in Hash.cpp, and an empty named class, with no forwarding boilerplate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

azure-pipelines · 2026-09-23T17:40:06Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Flor Chacón (florelis) · 2026-09-30T21:07:55Z

+        THROW_IF_NTSTATUS_FAILED_MSG(
+            BCryptOpenAlgorithmProvider(
+                &algHandle,
+                algorithmInfo.CngAlgorithmId,
+                nullptr,
+                0),
+            "failed opening %hs algorithm provider",
+            algorithmInfo.Name);
+        m_context->AlgHandle.reset(algHandle);


Nit: Seems like we lost some comments that we had in SHA256.cpp

Restored concise comments describing opening the CNG algorithm provider, obtaining the hash length, creating the hash handle, stream badbit behavior, and the 1 MB buffer.

I know I'm being nitpicky and it's not a big deal if you keep this as-is, but I would prefer to keep the same level of comments we had before unless there is a reason to delete them, and also keep the same formatting. For example, making this section:

// Open an algorithm handle THROW_IF_NTSTATUS_FAILED_MSG(BCryptOpenAlgorithmProvider( &algHandle, // Alg Handle pointer algorithmInfo.CngAlgorithmId, // Cryptographic Algorithm name (null terminated unicode string) nullptr, // Provider name; if null, the default provider is loaded 0), // Flags "failed opening %hs algorithm provider", algorithmInfo.Name);

I'm hoping that would make git see this as a rename instead of a new file, and it would give us a nicer diff/blame.

I don't mind the nitpick, it makes sense. Totally valid feedback, will update.

Copilot

Copilot review overview

🟡 Changes recommended

The out-of-line destructor unintentionally removes the previous SHA256 type’s move semantics.

Review effort: Balanced
Findings: 1

Open (1)

Restore Hash move operations suppressed by destructor · New

What changed in this PR

Introduces a reusable CNG-backed hash abstraction for future algorithms while retaining the existing SHA256 API.

Changes:

Replaces the SHA256 implementation with generic Hash/HashT types.
Updates projects and consumers to include winget/Hash.h.
Adds SHA256 regression tests and cryptography tags.

File	Description
`src/Microsoft.Management.Configuration/ConfigurationProcessor.cpp`	Uses the new hash header.
`src/AppInstallerSharedLib/YamlWrapper.h`	Updates hash include.
`src/AppInstallerSharedLib/SHA256.cpp`	Removes the previous implementation.
`src/AppInstallerSharedLib/Public/winget/Yaml.h`	Uses the new public header.
`src/AppInstallerSharedLib/Public/winget/Hash.h`	Defines the generic hash API and SHA256 alias.
`src/AppInstallerSharedLib/Public/AppInstallerSHA256.h`	Removes the previous SHA256 declaration.
`src/AppInstallerSharedLib/Hash.cpp`	Implements generic CNG hashing.
`src/AppInstallerSharedLib/AppInstallerStrings.cpp`	Updates hash include.
`src/AppInstallerSharedLib/AppInstallerSharedLib.vcxproj.filters`	Replaces project filter entries.
`src/AppInstallerSharedLib/AppInstallerSharedLib.vcxproj`	Registers the new source and header.
`src/AppInstallerRepositoryCore/Rest/RestInformationCache.h`	Updates hash include.
`src/AppInstallerRepositoryCore/pch.h`	Updates precompiled-header dependency.
`src/AppInstallerRepositoryCore/Microsoft/Schema/1_3/Interface_1_3.cpp`	Updates hash include.
`src/AppInstallerRepositoryCore/IconExtraction.cpp`	Updates and reorders includes.
`src/AppInstallerCommonCore/Settings.cpp`	Updates hash include.
`src/AppInstallerCommonCore/Public/winget/PortableFileEntry.h`	Updates hash include.
`src/AppInstallerCommonCore/Public/winget/MSStoreDownload.h`	Updates hash include.
`src/AppInstallerCommonCore/Public/winget/ManifestYamlParser.h`	Updates hash include.
`src/AppInstallerCommonCore/Public/winget/Manifest.h`	Updates hash include.
`src/AppInstallerCommonCore/Public/winget/FileCache.h`	Updates hash include.
`src/AppInstallerCommonCore/Manifest/YamlWriter.cpp`	Updates hash include.
`src/AppInstallerCommonCore/Manifest/YamlParser.cpp`	Updates hash include.
`src/AppInstallerCommonCore/Manifest/ManifestYamlPopulator.cpp`	Updates hash include.
`src/AppInstallerCommonCore/Downloader.cpp`	Updates hash include.
`src/AppInstallerCommonCore/DODownloader.cpp`	Updates hash include.
`src/AppInstallerCommonCore/AppInstallerTelemetry.cpp`	Updates hash include.
`src/AppInstallerCLITests/YamlManifest.cpp`	Updates test include.
`src/AppInstallerCLITests/Strings.cpp`	Updates test include.
`src/AppInstallerCLITests/SQLiteIndexTestCommon.cpp`	Updates test include.
`src/AppInstallerCLITests/RestInterface_1_0.cpp`	Updates test include.
`src/AppInstallerCLITests/MSStoreDownloadFlow.cpp`	Updates test include.
`src/AppInstallerCLITests/HashCommand.cpp`	Adds SHA256 regression coverage.
`src/AppInstallerCLITests/Downloader.cpp`	Updates test include.
`src/AppInstallerCLICore/Workflows/WorkflowBase.cpp`	Updates hash include.
`src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp`	Updates hash include.
`src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp`	Updates hash include.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

The implementation can silently produce incorrect hashes on oversized input or read failure, and it removes the legacy public include path despite the compatibility goal.

Review effort: Balanced
Findings: 2

Open (2)

Reject oversized hash chunks before ULONG truncation · New
Propagate ReadFile errors instead of treating them as EOF · New

Resolved since last review (1)

Restore Hash move operations suppressed by destructor

Previously missed (1)

In code that hasn't changed since last review

Preserve deleted AppInstallerSHA256.h as a forwarding header

src/AppInstallerSharedLib/Public/winget/Hash.h:193

This alias does not fully preserve the existing source API because the public AppInstallerSHA256.h header that existing consumers include is deleted. All in-repo includes were migrated, but out-of-tree code will fail before it can see this compatibility alias. Keep AppInstallerSHA256.h as a forwarding header that includes winget/Hash.h (and retain it in the project) while deprecating the old include path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Flor Chacón (florelis) · 2026-10-01T00:32:52Z

/azp run

azure-pipelines · 2026-10-01T00:33:02Z

Azure Pipelines: Successfully started running 1 pipeline(s).

* Restore hash implementation comments * Carry over hash header comments

Kaleb Luedtke (Trenly) and others added 2 commits September 23, 2026 11:04

Make SHA256 a wrapper around Hashing base class

e726e11

* Adds new Hash.cpp in SharedLib to provide base class access to BCrypt hashing functionality * Moves implementation from SHA256 into the the base class * Leaves the SHA256 namespace as a wrapper around the base class

Kaleb Luedtke (Trenly) marked this pull request as ready for review September 23, 2026 17:46

Kaleb Luedtke (Trenly) requested a review from a team as a code owner September 23, 2026 17:46

Merge branch 'microsoft:master' into HashAbstraction

0efb1d3

JohnMcPMS reviewed Sep 29, 2026

View reviewed changes

Kaleb Luedtke (Trenly) and others added 5 commits September 29, 2026 17:58

Rename to winget\Hash

7f77e82

Move to Cryptography Namespace

6524a96

Refactor generic hash abstraction

e7ab2f8

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Change to alias and move alias to new header for consistency

f9fe6d0

Simplify hash context ownership

49990f8

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Kaleb Luedtke (Trenly) requested a review from JohnMcPMS September 29, 2026 23:14

Kaleb Luedtke (Trenly) and others added 2 commits September 29, 2026 18:20

Fix hash utility namespace reference

e3d9e14

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Tag SHA256 tests as cryptography tests

110fbab

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Flor Chacón (florelis) reviewed Sep 30, 2026

View reviewed changes

Flor Chacón (florelis) requested a balanced review from Copilot September 30, 2026 21:23

Copilot started reviewing on behalf of Flor Chacón (florelis) September 30, 2026 21:24 View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

Comment thread src/AppInstallerSharedLib/Public/winget/Hash.h

Restore hash move semantics and address review

68c9250

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Kaleb Luedtke (Trenly) requested a balanced review from Copilot September 30, 2026 23:06

Copilot started reviewing on behalf of Kaleb Luedtke (Trenly) September 30, 2026 23:07 View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

Comment thread src/AppInstallerSharedLib/Hash.cpp

Comment thread src/AppInstallerSharedLib/Hash.cpp Outdated

Handle oversized and failed hash reads

c2c3d6d

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Kaleb Luedtke (Trenly) requested a review from Flor Chacón (florelis) September 30, 2026 23:15

Flor Chacón (florelis) previously approved these changes Oct 1, 2026

View reviewed changes

Restore hash comments

1851b2b

* Restore hash implementation comments * Carry over hash header comments

Kaleb Luedtke (Trenly) dismissed Flor Chacón (florelis)’s stale review via 1851b2b October 1, 2026 04:11

Kaleb Luedtke (Trenly) requested a review from Flor Chacón (florelis) October 1, 2026 04:27

Conversation

Kaleb Luedtke (Trenly) commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📖 Description

🔗 References

🔍 Validation

✅ Checklist

🤖 AI Assistance

📋 Issue Type

Uh oh!

azure-pipelines Bot commented Sep 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Flor Chacón (florelis) Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Kaleb Luedtke (Trenly) Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Flor Chacón (florelis) Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

Kaleb Luedtke (Trenly) Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Flor Chacón (florelis) commented Oct 1, 2026

Uh oh!

azure-pipelines Bot commented Oct 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Kaleb Luedtke (Trenly) commented Sep 23, 2026 •

edited

Loading