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.

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

Jump to bottom

Correct client certificate validation policy on Windows and macOS. #1966

Merged
merged 15 commits into from
Sep 21, 2021
Merged

Correct client certificate validation policy on Windows and macOS. #1966

merged 15 commits into from
Sep 21, 2021

Conversation

anrossi
Copy link
Contributor

@anrossi anrossi commented Sep 7, 2021 •
edited by nibanks
Loading

The wrong certificate validation policy was being used for client certificates with OpenSSL on Windows and macOS. Closes #1803.

@anrossi anrossi added OS: Windows (User) OS: macOS TLS: OpenSSL Bug: Platform A code bug in platform/TLS specific code. labels Sep 7, 2021
@anrossi anrossi requested a review from a team as a code owner September 7, 2021 23:45
nibanks
nibanks reviewed Sep 7, 2021
View reviewed changes
src/platform/cert_capi.c Outdated Show resolved Hide resolved
nibanks
nibanks reviewed Sep 7, 2021
View reviewed changes
src/platform/cert_capi.c Outdated Show resolved Hide resolved
anrossi and others added 2 commits September 7, 2021 16:56
@anrossi
reverse sense of if-statements
e5b0dd0
@anrossi @nibanks
Update src/platform/darwin_openssl.c
7254d8f 
Co-authored-by: Nick Banks <nibanks@microsoft.com>
nibanks
nibanks reviewed Sep 7, 2021
View reviewed changes
src/platform/cert_capi.c Outdated Show resolved Hide resolved
@ThadHouse
Copy link
Contributor

This change passing this easily has me worried. Its making me wonder if somehow the right tests are not being ran in openssl. Something we should definitely double check.

@nibanks
Copy link
Member

nibanks commented Sep 8, 2021

This change passing this easily has me worried. Its making me wonder if somehow the right tests are not being ran in openssl. Something we should definitely double check.

I also agree with this sentiment. We need to take a close look at the tests (with this PR) to (1) make sure the existing tests are working and (2) add any tests that we are missing to actually validate this. The fact that tests passed without this change is bad.

@nibanks
Copy link
Member

nibanks commented Sep 8, 2021

Also, CLOG failed, so you need to regen the sidecar/headers.

anrossi
anrossi commented Sep 14, 2021
View reviewed changes
src/platform/cert_capi_openssl.c
@@ -87,15 +87,12 @@ CxPlatTlsVerifyCertificate(
CertFlags |= CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT;
}

uint32_t IgnoreFlags =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like I did this on purpose when I wrote it, but I don't remember now, so I'm removing it.

@nibanks nibanks merged commit 4429da0 into main Sep 21, 2021
@nibanks nibanks deleted the anrossi/capi-client-cert-validation branch September 21, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nibanks nibanks nibanks approved these changes

@thhous-msft thhous-msft thhous-msft approved these changes

Assignees
No one assigned
Labels
Bug: Platform A code bug in platform/TLS specific code. OS: macOS OS: Windows (User) TLS: OpenSSL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Client Certificate Support for OpenSSL
4 participants
@anrossi @ThadHouse @nibanks @thhous-msft