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

sectransp_connect_step1: bail out if SSLSetPeerDomainName fails #8798

Closed
wants to merge 1 commit into from

Conversation

piru
Copy link

@piru piru commented May 5, 2022

Before the code would only warn about SSLSetPeerDomainName() errors.

Caller requested host verification and if we can't do it we must fail to connect.

Before the code would just warn about SSLSetPeerDomainName() errors.
@jay jay added the TLS label May 5, 2022
@jay
Copy link
Member

@jay jay commented May 5, 2022

Was the intention to continue even if SSLSetPeerDomainName fails? There is a "WARNING" there which makes me think so. @nickzman

@nickzman
Copy link
Member

@nickzman nickzman commented May 5, 2022

I'm not sure if this is even necessary. If you look at the function's internal implementation, you'll see that it fails only under two circumstances: the context is not set, and the session is already active. Neither of which will ever be hit unless the code in this function gets scrambled, so I will be very surprised if this function fails for anyone anywhere.

@piru , what are you trying to accomplish?

@piru
Copy link
Author

@piru piru commented May 5, 2022

@nickzman Defensive programming. The API call has an error code. If the error code is set, the operation can be assumed to have failed. Continuing when this operation has failed will (likely) result in security check bypass (hostname will not be validated).

Now, if the function can practically never fail, what's the harm in checking the result code and bailing out if error occurred?

Also:

What is this code trying to accomplish by continuing even if error occurred?

bagder
bagder approved these changes May 6, 2022
@nickzman
Copy link
Member

@nickzman nickzman commented May 6, 2022

Okay. Although it's not likely to fail, I'm okay with this change. Thanks!

@bagder
Copy link
Member

@bagder bagder commented May 6, 2022

Thanks!

@piru piru deleted the bail_on_SSLSetPeerDomainName_error branch May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants