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
ADDED windows certificate store cert_skip_invalid options #4384
base: main
Are you sure you want to change the base?
Conversation
@dmpriso Can you add a unit test to positively check skip scenario? i.e. |
Done. |
Any idea on the failed build? It is very unlikely this is related to my change ... |
Looks ok now. |
@tbeets anything else required? |
@dmpriso Sorry for delay in PR response. One ask for option name change plus the test coverage ask is all that is required. |
Do we want this in main? Should this be considered for 2.9.22 @tbeets ? |
2.10.x a good target release. @derekcollison @dmpriso Are you able to make the requested updates? Once those are in we can re-base on Dev branch (2.10.x train) and merge the PR. |
@tbeets I fear I don't exactly understand what you mean - what is the name supposed to be changed to? Seems some information is missing (or I can't find it). |
@dmpriso Review comment on
|
|
Thanks @dmpriso . I can see that you made the option name changes. The unit test you wrote initially for the new skip feature doesn't actually start the embedded server and do a test of returning the non-expired cert of the two loaded in the cert store. So, just a re-factor of your test needed. Here's my review comment:
|
Ah I see. I can add that test but want to point out that I skipped that one for a reason:
Let me know how I should continue on that. Anyway, I found some possible simplification and clarification in my changes and pushed them. |
The server does process the config file at startup, but Windows store interaction happens subsequent to that in server startup flow and before listeners are enabled. I hear you on non-deterministic behavior from search of the Windows store. I think it is sufficient for now to have two sub-tests:
I think that is sufficient for now and importantly exercises the enhanced win32 API call. |
I see. Will probably manage to finish that next week. |
Resolves #
git pull --rebase origin main
)Resolves #4383
Changes proposed in this pull request: