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

cryptoapi: add flags for physical store and logical store #4

Merged
merged 4 commits into from
May 29, 2020

Conversation

aerth
Copy link
Contributor

@aerth aerth commented May 20, 2020

Closes #2

  • --capi.physical-store <name> names the physical store to inject certificate into

    • (default: system)
    • allows only system, current-user, enterprise, and group-policy
  • --capi.logical-store <store-name> names the logical store to inject certificate into.

    • (default: Root)
    • allows Root, Trust, CA, My, Disallowed or any other custom string

Currently, only system + Root combo passes the powershell test (testdata/ci-failtest.ps1)

@aerth aerth force-pushed the stores branch 10 times, most recently from 28528b5 to b7c9b4c Compare May 21, 2020 05:40
@JeremyRand
Copy link
Member

Since these new command-line flags are specific to CryptoAPI (e.g. they don't directly apply to NSS), maybe we should make a new flag group for this? I'm thinking maybe create a capi flag group as a subgroup of the certstore flag group. (You can do this by passing the parent flag group as the first arg to the NewGroup function.) So the command-line flags would become e.g. -certstore.capi.store <store-name> instead of -certstore.store <store-name>.

@aerth aerth changed the title [WIP] Stores cryptoapi: add flags to switch store May 25, 2020
)

// In 64-bit Windows, this key is shared between 64-bit and 32-bit applications.
// See https://msdn.microsoft.com/en-us/library/windows/desktop/aa384253.aspx
const cryptoApiCertStoreRegistryBase = registry.LOCAL_MACHINE
const cryptoApiCertStoreRegistryKey = `SOFTWARE\Microsoft\EnterpriseCertificates\Root\Certificates`
Copy link
Member

Choose a reason for hiding this comment

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

This PR removes the previously existing functionality of writing to the Enterprise physical store. Would be good to restore that functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, this is resolved by adding support for -scope enterprise in e3f4a1b

cryptoapi_windows.go Outdated Show resolved Hide resolved
cryptoapi_windows.go Outdated Show resolved Hide resolved
@aerth aerth force-pushed the stores branch 2 times, most recently from f44747e to e3f4a1b Compare May 26, 2020 17:40
@aerth aerth requested a review from JeremyRand May 26, 2020 18:03
cryptoapi_windows.go Outdated Show resolved Hide resolved
"current-user": Store{registry.CURRENT_USER, `SOFTWARE\Microsoft\SystemCertificates\`, `%s\Certificates`},
"system": Store{registry.LOCAL_MACHINE, `SOFTWARE\Microsoft\SystemCertificates\`, `%s\Certificates`},
"enterprise": Store{registry.LOCAL_MACHINE, `SOFTWARE\Microsoft\EnterpriseCertificates\`, `%s\Certificates`},
"group-policy": Store{registry.LOCAL_MACHINE, `SOFTWARE\Policies\SystemCertificates\`, `%s\Certificates`},
Copy link
Member

Choose a reason for hiding this comment

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

According to my notes, SOFTWARE\Policies\SystemCertificates\ should be SOFTWARE\Policies\Microsoft\SystemCertificates\. Not sure if this is a typo or if you found a different source that's not consistent with my notes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typo. good catch, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it may be Policy\Microsoft ?
( from https://docs.microsoft.com/en-us/windows/win32/seccrypto/system-store-locations#cert_system_current_user_group_policy )

HKEY_CURRENT_USER
   Software
      Policy
         Microsoft
            SystemCertificates

and

HKEY_LOCAL_MACHINE
   Software
      Policy
         Microsoft
            SystemCertificates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed on a windows machine that Policy does not exist, Policies is the one, fixed. 😄

cryptoapi_windows.go Outdated Show resolved Hide resolved
@aerth aerth force-pushed the stores branch 3 times, most recently from 97720e7 to 686f1e6 Compare May 27, 2020 04:21
since they are in a flag subgroup, use like so:

certinject --certstore.capi.physical-store system --certstore.capi.logical-store Root [certificate-file]
@aerth aerth force-pushed the stores branch 3 times, most recently from 2e07df8 to 9b13031 Compare May 27, 2020 04:44
@aerth aerth changed the title cryptoapi: add flags to switch store cryptoapi: add flags for physical store and logical store May 28, 2020
tests := []struct {
Name string // for logs
Physical string // from user flag
Logical string // from userflag
Copy link
Member

Choose a reason for hiding this comment

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

Missing a space in userflag?

@JeremyRand
Copy link
Member

utACK 886a2ae except for the missing space (noted above in inline comments).

@JeremyRand
Copy link
Member

utACK ee6a585; merging shortly.

@JeremyRand JeremyRand merged commit dbc1d33 into namecoin:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow selecting CryptoAPI logical store
2 participants