Server certificate pinning for Store source #2347

JohnMcPMS · 2022-07-19T04:03:32Z

Change

This change adds a generic certificate chain verification infrastructure for pinning certificate chains. It is specifically used to pin the Microsoft Store source by default. More sources may be pinned later, but currently the packaged index is less in need of it because it is already signed.

The pinning configuration consists of 1 or more chains, only one of which needs to successfully validate the incoming certificate. This allows for rolling to a new certificate when needed. Each chain consists of a fixed set of certificates, which can each be configured to validate any or all of the following properties:

Public Key
Subject
Issuer

If the certificate is configured to validate none of the values, it will allow any certificate through.

An admin setting is added to disable pinning, both as an emergency measure in the event that there is a bug or rolled certificate that was not communicated, but also because there are test scenarios where the user actively wants to disable it (HTTPS redirection via something like Fiddler).

The configuration can be loaded from JSON for future dynamic configuration, but it is currently only as a test hook to enable configuration via Group Policy.

In order to better secure the source by default, reconfiguring (remove then add) the Store source manually will convert it back to the built-in values. This includes the pinning configuration.

It was necessary to modify the cpprestsdk subtree to add a new callback. This enables the request handle to be passed back to our code when the server certificate is first available. We can then check the server certificate against the configured pinning chain, making a decision to terminate the request before it is sent.

Validation

Unit tests are added for the various utilities and the primary pinning configuration/validation.
An E2E test is added for a negative case with the Store, but more could be done to test REST in general in our CI pipeline.

Microsoft Reviewers: Open in CodeFlow

…ttached to source

…nual reconfiguration of builtin sources become fully builtin. In process of allowing test config via GP source definitions for E2E tests.

…s the just moved to a new chain.

…y default

yao-msft · 2022-07-21T21:03:03Z

src/CertificateResources/resource.h

@@ -0,0 +1,14 @@
+//{{NO_DEPENDENCIES}}
+// Microsoft Visual C++ generated include file.
+// Used by WinGetSchemas.rc


WinGetSchemas.rc

nit: CertificateResources.rc

yao-msft · 2022-07-21T21:27:53Z

src/cpprestsdk/cpprestsdk/Release/src/http/client/http_client_winhttp.cpp

@@ -2039,6 +2039,18 @@ class winhttp_client final : public _http_client_communicator
            case WINHTTP_CALLBACK_STATUS_SENDING_REQUEST:
            {
                p_request_context->on_send_request_validate_cn();
+
+                try


try

Are we directly modifying subtree modules?

The upstream codebase is in maintenance mode. We can try to get this change in there but it seems unlikely, and very unlikely to be able to do so in the time frame we need.

…g from E2E tests.

yao-msft · 2022-07-21T23:25:58Z

src/AppInstallerCommonCore/AdminSettings.cpp

+            PolicyState localManifestFilesPolicy = GroupPolicies().GetState(TogglePolicy::Policy::LocalManifestFiles);
+            if (localManifestFilesPolicy != PolicyState::NotConfigured)
+            {
+                return (localManifestFilesPolicy == PolicyState::Enabled ? true : false);


return (localManifestFilesPolicy == PolicyState::Enabled ? true : false);

nit: return localManifestFilesPolicy == PolicyState::Enabled

yao-msft · 2022-07-22T00:04:29Z

src/AppInstallerCommonCore/GroupPolicy.cpp

@@ -196,6 +196,19 @@ namespace AppInstaller::Settings
                return std::nullopt;
            }

+#ifndef AICLI_DISABLE_TEST_HOOKS
+            // Enable certificate pinning configuration through GP sources for testing
+            std::string pinningConfigurationName = "CertificatePinning";


std::string

nit const

yao-msft · 2022-07-22T00:04:49Z

src/AppInstallerCommonCore/GroupPolicy.cpp

@@ -196,6 +196,19 @@ namespace AppInstaller::Settings
                return std::nullopt;
            }

+#ifndef AICLI_DISABLE_TEST_HOOKS


AICLI_DISABLE_TEST_HOOKS

Are we planning to expose cert pinning config in the future?

Possibly, but I want that to be an explicit decision, not part of this change.

yao-msft · 2022-07-22T00:13:20Z

src/AppInstallerCommonCore/Certificates.cpp

+        return *this;
+    }
+
+    bool PinningDetails::LoadFrom(const Json::Value& configuration)


PinningDetails::LoadFrom

A comment with sample configuration json would better help with reading the parsing logic

yao-msft · 2022-07-22T00:14:16Z

src/AppInstallerCommonCore/Certificates.cpp

+
+    bool PinningDetails::LoadFrom(const Json::Value& configuration)
+    {
+        std::string validationName = "Validation";


std::string

nit: const or declare above for all the json node names across this function?

yao-msft · 2022-07-22T00:15:56Z

src/AppInstallerCommonCore/Certificates.cpp

+                &m_certificateContext.get()->pCertInfo->SubjectPublicKeyInfo,
+                &certContext->pCertInfo->SubjectPublicKeyInfo))
+            {
+                AICLI_LOG(Core, Verbose, << "Public key mismatch: Expected certificate [" << GetSimpleDisplayName(m_certificateContext.get()) << "], Actual certificate [" << GetSimpleDisplayName(certContext) << "]");


Verbose

Should these logging levels at least warning?

I chose not to log them by default for the case of multiple chains. It is expected that only one of the chains would be valid for any given connection, so logging "Hey there is seemingly a security issue over here!" is not warranted when it turns out that it is actually just the one not being used.

PinningConfiguration::Validate, which is the expected entry point for any non-test use case, does log at Error on a failure to find any good chain. If that is happening, then the user can manually gather a verbose log.

yao-msft · 2022-07-22T00:20:42Z

src/AppInstallerCommonCore/Certificates.cpp

+        if (m_chain.empty())
+        {
+            // An empty chain rejects all inputs.
+            AICLI_LOG(Core, Verbose, << "Empty pinning chain blindly rejecting chain context");


Verbose

I think logging level of this function should be revisited?

I changed this line specifically to Warning as an empty chain is not an expected use case.

yao-msft · 2022-07-22T00:35:47Z

src/AppInstallerCLITests/GroupPolicy.cpp

+
+        std::wostringstream pinningConfig;
+        pinningConfig << LR"({
+"Chains": [{


nit: seems bad indentation

It is a raw string, but I can bring the beginning down to make it look less awkward.

yao-msft

JohnMcPMS · 2022-07-27T23:56:26Z

/azp run

azure-pipelines · 2022-07-27T23:56:40Z

Azure Pipelines successfully started running 1 pipeline(s).

JohnMcPMS added 9 commits July 13, 2022 17:37

Create pipe for pinning REST server certificates

4a95406

Begin the configuration portion

e843e2f

Implement pinning configuration interface, needs actual configuring a…

f13a4d2

…ttached to source

Added additional tests for pinning verification

e996d01

Attach to builtin store source with admin setting to disable. Make ma…

a8f846b

…nual reconfiguration of builtin sources become fully builtin. In process of allowing test config via GP source definitions for E2E tests.

Finish GP config code and unit tests, only E2E tests remain.

cfa9b37

Add E2E infra and test, fix spelling

8b0ce58

Another spelling

a5bca1e

Merge from master

7c391f4

JohnMcPMS requested a review from a team as a code owner July 19, 2022 04:03

JohnMcPMS added 5 commits July 20, 2022 15:57

Don't delete server cert from directory in tests. Update Store cert a…

3ede239

…s the just moved to a new chain.

Merge from master

66bc44f

Change AddChain log to Verbose. Fix encoding for leaf certificate.

297cf30

Change test source name, log GP list values, make E2E tests verbose b…

1ad0d67

…y default

Merge from mater

6df8868

yao-msft reviewed Jul 21, 2022

View reviewed changes

JohnMcPMS added 2 commits July 21, 2022 15:20

Improve verbose logging for GP sources, remove default verbose loggin…

d03adb2

…g from E2E tests.

Merge from master

fa84ab7

yao-msft reviewed Jul 21, 2022

View reviewed changes

yao-msft reviewed Jul 22, 2022

View reviewed changes

yao-msft approved these changes Jul 22, 2022

View reviewed changes

JohnMcPMS added 2 commits July 21, 2022 17:56

Add Data field as it is required

8308182

Merge from master

6b1626a

JohnMcPMS added 2 commits July 22, 2022 15:05

PR feedback

458aceb

Merge from master

0e8589f

JohnMcPMS merged commit 888b4ed into microsoft:master Jul 28, 2022

JohnMcPMS deleted the certpin branch July 28, 2022 01:03

jadodd-CCG mentioned this pull request Jan 30, 2023

"0x8a15005e : The server certificate did not match any of the expected values." when trying to search on msstore with winget 1.4 #2879

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server certificate pinning for Store source #2347

Server certificate pinning for Store source #2347

JohnMcPMS commented Jul 19, 2022 •

edited by ghost

Loading

yao-msft Jul 21, 2022

yao-msft Jul 21, 2022

JohnMcPMS Jul 21, 2022

yao-msft Jul 21, 2022

yao-msft Jul 22, 2022

yao-msft Jul 22, 2022 •

edited

Loading

JohnMcPMS Jul 22, 2022

yao-msft Jul 22, 2022

yao-msft Jul 22, 2022

yao-msft Jul 22, 2022

JohnMcPMS Jul 22, 2022

yao-msft Jul 22, 2022

JohnMcPMS Jul 22, 2022

yao-msft Jul 22, 2022

JohnMcPMS Jul 22, 2022

yao-msft left a comment

JohnMcPMS commented Jul 27, 2022

azure-pipelines bot commented Jul 27, 2022

Server certificate pinning for Store source #2347

Server certificate pinning for Store source #2347

Conversation

JohnMcPMS commented Jul 19, 2022 • edited by ghost Loading

Change

Validation

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yao-msft Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yao-msft left a comment

Choose a reason for hiding this comment

JohnMcPMS commented Jul 27, 2022

azure-pipelines bot commented Jul 27, 2022

JohnMcPMS commented Jul 19, 2022 •

edited by ghost

Loading

yao-msft Jul 22, 2022 •

edited

Loading