Do not return error if CAPI SW key is found but CNG is not locatable #90
Conversation
Signed-off-by: Boris Djurdjevic <boris@djurdjevic.ch>
| return "", "", fmt.Errorf("unable to locate CNG key: %v", err) | ||
| } | ||
| if cng == "" { | ||
| return "", "", errors.New("CNG key was empty") |
There was a problem hiding this comment.
Can we add some log for this case?
Otherwise, handshake failure will be difficult to debug where tls applications expected CNG key.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| return nil, err | ||
| } | ||
|
|
||
| if !store.ignoreNoCNG && uc == "" { |
There was a problem hiding this comment.
Should this fix be made at
certtostore/certtostore_windows.go
Line 1744 in ee4ff6f
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3 participants
Hi
This PR is open for discussion. We have servers that include a CAPI software container, but do not have a CNG software container.
When the key of a certificate is requested (
CertKey->keyMetadata->softwareKeyContainers), the process fails withCNG key was emptyif the CNG key is not present, even though the CAPI key is found correctly.According to Win32 / CNG / Key Storage and Retrieval, the application API prefers CNG and then falls back to CAPI. Therefore, a CNG key is not always expected to be available:
I propose to not return an error if the CNG key is not found but the CAPI key is present. In this case, the CNG path would simply not be populated, similar to how the CAPI path is omitted when its key is not found in
softwareKeyContainers.I am unsure if this change would have any implications, such as breaking changes, so feedback is appreciated.