Skip to content

Do not return error if CAPI SW key is found but CNG is not locatable #90

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

Open
djboris9 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Do not return error if CAPI SW key is found but CNG is not locatable #90

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fed4342
Do not return error if CAPI SW key is found but CNG is not locatable
djboris9 Sep 19, 2025
27407d7
Add option to ignore a not found CNG key
djboris9 Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions certtostore_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,9 @@ type WinCertStoreOptions struct {
// - certStoreCreateNewFlag: Create new store if it doesn't exist
// - certStoreOpenExistingFlag: Only open existing stores
StoreFlags uint32

// IgnoreNoCNG can be set in order to ignore a not found CNG key when a CAPI key exists.
IgnoreNoCNG bool
}

// WinCertStore is a CertStorage implementation for the Windows Certificate Store.
Expand All @@ -349,6 +352,7 @@ type WinCertStore struct {
stores map[string]*storeHandle
keyAccessFlags uintptr
storeFlags uint32
ignoreNoCNG bool

mu sync.Mutex
}
Expand All @@ -375,6 +379,7 @@ func DefaultWinCertStoreOptions(provider, container string, issuers, intermediat
LegacyKey: legacyKey,
CurrentUser: false,
StoreFlags: 0,
IgnoreNoCNG: false,
}
}

Expand Down Expand Up @@ -442,6 +447,7 @@ func OpenWinCertStoreWithOptions(opts WinCertStoreOptions) (*WinCertStore, error
container: opts.Container,
stores: make(map[string]*storeHandle),
storeFlags: opts.StoreFlags,
ignoreNoCNG: opts.IgnoreNoCNG,
}

// Deep copy the issuer slices to prevent external modification
Expand Down Expand Up @@ -1369,6 +1375,11 @@ func keyMetadata(kh uintptr, store *WinCertStore) (*Key, error) {
if err != nil {
return nil, err
}

if !store.ignoreNoCNG && uc == "" {
Copy link
Contributor

@apurv15 apurv15 Nov 20, 2025

Choose a reason for hiding this comment

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

Should this fix be made at

certtostore/certtostore_windows.go

Line 1744 in ee4ff6f

if cng == "" {
instead?

Copy link
Author

@djboris9 djboris9 Dec 12, 2025

Choose a reason for hiding this comment

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

softwareKeyContainers will return an empty string if the CNG key couldn't be located. An error will only be returned if keyMatch gets an error from os.Stat or ioutil.ReadDir call. Therefore it returns whatever is available without being smart.
Consequently I decided to leave the softwareKeyContainers function as-is and to delegate the judgement of returning an error on no-CNG-key up to the caller keyMetadata. I'm not sure if there are other callers of those functions hidden by copybara.

Hope this explains why I implemented it this way but I might be wrong.

Copy link
Contributor

@apurv15 apurv15 Dec 15, 2025

Choose a reason for hiding this comment

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

keyMatch is just a generic helper: it takes a known key file path and a directory, then tries to find a “matching” key by timestamp. Its contract is:
On success: returns the matching path.
If it can’t find a match: returns "" and nil error.
On real failure (stat/ReadDir issues): returns "" and a non‑nil error.
It has no notion of CNG vs CAPI, or what “missing is acceptable” means. It only knows “found” / “not found” / “hard failure”.
softwareKeyContainers is where the CAPI vs CNG checks exist.
Keep keyMatch as a low‑level primitive as is. Adjust softwareKeyContainers policy.
A missing CNG match (cng == "") is treated the same way as we treat missing CAPI when starting from CNG.

// key is not CNG backed, but store was opened with ignoreNoCNG=false
return nil, errors.New("CNG key was empty")
}
}

alg, err := getPropertyStr(kh, nCryptAlgorithmGroupProperty)
Expand Down Expand Up @@ -1745,9 +1756,6 @@ func softwareKeyContainers(uniqueID string, storeDomain uint32) (string, string,
if err != nil {
return "", "", fmt.Errorf("unable to locate CNG key: %v", err)
}
if cng == "" {
return "", "", errors.New("CNG key was empty")
Copy link
Contributor

@apurv15 apurv15 Sep 23, 2025

Choose a reason for hiding this comment

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

Can we add some log for this case?
Otherwise, handshake failure will be difficult to debug where tls applications expected CNG key.

Copy link
Author

@djboris9 djboris9 Sep 30, 2025

Choose a reason for hiding this comment

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

I'm not sure if emitting logs from a package used as a library is the right way to do it (unexpected stdout/stderr, formatting differences, log vs. slog etc.).
Can you elaborate more, in what use cases an explicit CNG key is required? Maybe the exported functions could be adapted in order to prevent a breaking change.

Copy link
Contributor

@apurv15 apurv15 Oct 1, 2025

Choose a reason for hiding this comment

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

Casual search says following cases will not work with CAPI keys.

  • Modern TLS stacks (e.g., Schannel with ECC or AES-GCM) may require CNG keys, especially if the server enforces modern cipher suites.
  • Smart cards or virtual smart cards that use CNG Key Storage Providers (KSPs) will not work with CAPI-only applications.
  • Custom applications using BCrypt* or NCrypt* APIs will not be able to use CAPI keys for mTLS.

I am not expert with these solutions and cannot say for certain.

Copy link

@HowDoCode HowDoCode Oct 2, 2025

Choose a reason for hiding this comment

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

One other thought is that this could be skipped via an option in the WinCertStoreOptions. That way if something is relying on this behavior it won't break.

Also FWIW, using non-exported keys has been a pain due to this so this change would massively improve that usability.

It would be fairly easy to:

  • add new option - IgnoreNoCNG - that is set on the WinCertStore
  • make no chg key a named error
  • in keyMetadata check is err is the new named error. If yes and skip is set continue, else return the err

Copy link
Author

@djboris9 djboris9 Oct 2, 2025

Choose a reason for hiding this comment

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

the idea using WinCertStoreOptions looks good to me as it doesn't break the existing behavior and allows to selectively ignore the no-CNG key issue. I will update this PR in around 1.5 weeks (holidays), if there are no votes against it

Copy link
Author

@djboris9 djboris9 Oct 29, 2025

Choose a reason for hiding this comment

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

I just added the IgnoreNoCNG option in 81cf01c and kept the original error string, both should provide backward compatibility without breaking stuff. Sorry for the late commit

}
default:
return "", "", fmt.Errorf("unexpected key type %q", keyType)
}
Expand Down