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

Add support for loading user CA certs from an arbitrary Windows cert store. #7503

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kareem-wolfssl
Copy link
Contributor

Description

Add support for loading user CA certs from an arbitrary Windows cert store.

Testing

Tested on Windows machine.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@kareem-wolfssl kareem-wolfssl self-assigned this May 3, 2024
@kareem-wolfssl
Copy link
Contributor Author

Please do not merge yet, just looking for a review + pipeline tests passing for now. Will merge after customer confirms patch works for them.

return NULL;
}

int wolfSSL_CTX_load_windows_user_CA_certs(WOLFSSL_CTX* ctx, const char* userStore,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to integrate this with wolfSSL_CTX_load_system_CA_certs? Is wolfSSL_CTX_load_windows_user_CA_certs a compatibility API? If not I would avoid making it Windows specific in case we wanted to expand its coverage beyond Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The customer has requested the ability to configure which cert store is loaded at runtime in wolfSSH, which requires arguments to be passed in to wolfSSL. This is not an OpenSSL compatibility API, but it didn't make sense to me to add Windows specific arguments to wolfSSL_CTX_load_system_CA_certs. I am not sure how to avoid adding a Windows specific API while still allowing wolfSSH to configure this at runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we could come up with a better name or API that would be easier to extend in the future. @ejohnstown I'd like your input as well on this since it is related to an SSH PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could there be a generic wolfSSL_CTX_load_CA_cert_store()? There can be a ctx pointer to a struct with OS specific data. For Windows, that would have a flag for user or system. Maybe at some point someone will want certs stored in a LDAP database. Perhaps the existing load function can be rewritten as a specific use case for this function.

@dgarske dgarske requested review from ejohnstown and removed request for wolfSSL-Bot May 16, 2024 16:52
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.

None yet

3 participants