ADDED windows certificate store cert_skip_invalid options

[dmpriso]

trafficstars

• [x] Link to issue, e.g. Resolves #
• [ ] Documentation added (if applicable)
• [x] Tests added
• [ ] Branch rebased on top of current main (git pull --rebase origin main)
• [x] Changes squashed to a single commit (described here)
• [x] Build is green in Travis CI
• [x] You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

Resolves #4383

Changes proposed in this pull request:

[tbeets]

@dmpriso Can you add a unit test to positively check skip scenario? i.e. certstore_windows_test.go Automated testing is a bit tricky. The existing tests use powershell etc. to do the needful with the test host's windows store. So need to load X certs with common lookup string and of these validate that the 1-of-X that is actually time valid reliably returned etc.

[dmpriso]

@dmpriso Can you add a unit test to positively check skip scenario? i.e. certstore_windows_test.go Automated testing is a bit tricky. The existing tests use powershell etc. to do the needful with the test host's windows store. So need to load X certs with common lookup string and of these validate that the 1-of-X that is actually time valid reliably returned etc.

Done.

[dmpriso]

Any idea on the failed build? It is very unlikely this is related to my change ...

[derekcollison]

Looks ok now.

[dmpriso]

@tbeets anything else required?

[tbeets]

@dmpriso Sorry for delay in PR response. One ask for option name change plus the test coverage ask is all that is required.

[derekcollison]

Do we want this in main? Should this be considered for 2.9.22 @tbeets ?

[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.

[dmpriso]

@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).

[tbeets]

@dmpriso Review comment on opts.go:

Change to CertMatchSkipInvalid and cert_match_skip_invalid to better scope to Cert Store feature and matching that happens there (vs. a general feature that works with a PEM bundle).

[dmpriso]

• The build failed again for a reason beyond the scope of my change, I think
• I'm sorry I don't see the review comments. I'm not working with github regularly (gitlab user), but I can't find your comment anywhere. I now renamed the field as per your quote, but not seeing your comments, I am also not seeing the comment about test coverage.

[tbeets]

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:

The tests needs to start a server, e.g. RunServerWithConfig() to exercise the cert search.

[dmpriso]

Ah I see. I can add that test but want to point out that I skipped that one for a reason:

• If I add two certs (one expired and one non-expired) to the store, the correct one could be just selected by incident. I cannot check if it has just be returned as first certificate by the OS which doesn't guarantee any order
• So in order to make sure the test really passes, I would have to add multiple invalid certificates (e.g. 10) and one valid - but, at best, in the middle of the 10 others, because OS could just return the last-added or first-added certificate first.
• Then I'd have to check whether the correct certificate is actually used by the server by connecting a client using TLS
• In the end, the server just calls ProcessConfigFile as I do in my test, and this is where I can check whether the cert skipping has worked or not. So, as far as I can see, the relevant code from certstore_windows.go already runs (I verified that with the debugger; being new to golang)

Let me know how I should continue on that.

Anyway, I found some possible simplification and clarification in my changes and pushed them.

[tbeets]

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:

1. Load only a single expired cert in the store, then start the server and check for appropriate (new) error when you have skip enabled.
2. Load a good cert and expired cert of same search string. Make sure we get the good cert by successfully making a client TLS connection.

I think that is sufficient for now and importantly exercises the enhanced win32 API call.

[dmpriso]

I see. Will probably manage to finish that next week.

[dungpa]

We are also hit with the invalid certificate issues on Window cert stores.

@tbeets @dmpriso Is there any chance that we can finalise and merge this PR?