winhttp: support optional client cert#5384
Conversation
| while (request_failed && attempts++ < 3) { | ||
| request_failed = 0; | ||
| cert_valid = 1; | ||
| client_cert_requested = 0; |
There was a problem hiding this comment.
Both cert_valid and client_cert_requested can be declared in the while loop
| case ERROR_WINHTTP_SECURE_FAILURE: { | ||
| cert_valid = 0; | ||
| break; | ||
| } |
There was a problem hiding this comment.
We usually avoid braces in case bodies in case they're not required
| git_error_set(GIT_ERROR_OS, "failed to set client cert context"); | ||
| return -1; | ||
| } | ||
| } |
There was a problem hiding this comment.
So we don't start supporting client certs, but simply start telling servers we ain't got one. I guess this is a good fix until we might land client certificates some time in the future and matches what Microsoft documents for this API.
There was a problem hiding this comment.
Yep, to support client certs we would need to provide a callback for users to specify which client cert they would like to use. We can do this by querying WINHTTP_OPTION_CLIENT_CERT_ISSUER_LIST to get a list of valid client certs for the provided CA, providing this list in the callback, and letting the user select one. We could also provide a default callback that just picks the 1st valid cert. I'm able to successfully authenticate using this method, I would just need some free time to implement the callback and polish/test it so I'd like to handle that in a follow up.
|
Forgot to say: thanks for your effort! |
Thanks for getting to this so quickly! |
|
On Thu, Feb 06, 2020 at 08:25:02AM -0800, Ian Hattendorf wrote:
> Forgot to say: thanks for your effort!
Thanks for getting to this so quickly!
Note that we're currently in code freeze until getting v1.0 out, so only
bugfixes get in. Will merge as soon as that's out.
|
Includes unmerged PRs: - libgit2/libgit2#5384 - libgit2/libgit2#5347 - libgit2/libgit2#4205
Includes unmerged PRs: - libgit2/libgit2#5384 - libgit2/libgit2#5347 - libgit2/libgit2#4205
|
@pks-t just wanted to ping you to make sure this doesn't get forgotten since it looks like v1.0 is out now. |
pks-t
left a comment
pks-t
left a comment
There was a problem hiding this comment.
One remark, but otherwise this looks good to me. Sorry for the delays and thanks for your work!
| request_failed = 1; | ||
| cert_valid = 0; | ||
| } | ||
| if (!request_failed || send_request_error == ERROR_WINHTTP_SECURE_FAILURE) { |
There was a problem hiding this comment.
I think this could be if (!request_failed || !cert_valid), which would make it code clearer to me.
|
Thanks @ianhattendorf - sorry for the delay here, I'm trying to make my way through the backlog. LGTM. |
3 participants
Some reverse proxies/applications can be configured to require client certificates (mutual TLS authentication). It's also possible that the server requests a client certificate, but doesn't require it (allowing anonymous access). For example, an HAProxy frontend can be configured as so:
bind *:8443 ssl crt /etc/ssl/certs/server.pem ca-file /etc/ssl/certs/ca.crt verify required.verify requiredindicates that the server requires a valid client certificate to continue. Changingverify requiredtoverify optionalcauses the server to request a client certificate, but allow anonymous access as well. In either case,WinHttpSendRequestwill returnERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED. If this is returned, the request can be retried after setting the optionWINHTTP_OPTION_CLIENT_CERT_CONTEXTto eitherWINHTTP_NO_CLIENT_CERT_CONTEXT(in which case we are requesting anonymous access) or by attaching a valid client certificate context.Originally reported in: #4204
Related PR: #4787