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

Add definitions that help with hostname checking #4492

Merged
merged 6 commits into from
Oct 10, 2018
Merged

Conversation

kaie
Copy link
Contributor

@kaie kaie commented Oct 9, 2018

This is related to pyca/pyopenssl#795

@@ -185,6 +185,8 @@
X509 *SSL_get_peer_certificate(const SSL *);
int SSL_get_ex_data_X509_STORE_CTX_idx(void);

X509_VERIFY_PARAM *SSL_get0_param(SSL *ssl);
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the ssl name, we don't include parameter names.

@alex alex added the bindings label Oct 9, 2018
@alex alex added this to the Twenty fourth release milestone Oct 9, 2018
@alex
Copy link
Member

alex commented Oct 9, 2018

It looks like some of these symbols do not exist in older OpenSSLs, so you'll need to make them conditional bindings.

You can see #4476 for an example of a PR adding optional bindings

@kaie
Copy link
Contributor Author

kaie commented Oct 9, 2018

Thanks for the review, will do that tomorrow.

@reaperhulk
Copy link
Member

This LGTM other than the need for conditionally binding it so the symbols aren't exposed on < 1.0.2.

@reaperhulk
Copy link
Member

As @tiran pointed out on the pyopenssl PR we'll also need to make sure the check covers LibreSSL < 2.7.1 as X509_VERIFY_PARAM_set1_host is not available until 2.7.1.

@kaie
Copy link
Contributor Author

kaie commented Oct 10, 2018

As @tiran pointed out on the pyopenssl PR we'll also need to make sure the check covers LibreSSL < 2.7.1 as X509_VERIFY_PARAM_set1_host is not available until 2.7.1.

x509_vfy.py already contains:

#if CRYPTOGRAPHY_OPENSSL_102_OR_GREATER
#else
#if !CRYPTOGRAPHY_LIBRESSL_27_OR_GREATER
int (*X509_VERIFY_PARAM_set1_host)(X509_VERIFY_PARAM *, const char *,
                                   size_t) = NULL;
#endif
#endif

I assume you want the equivalent for SSL_get0_param, as it seems to be a direct helper function for the set1_host API.

Besides X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS, I'm adding a couple of additional related X509_CHECK_FLAG_* flags.

I noticed you have a couple of #ifndef checks like the following:

/* These 3 defines are unavailable in LibreSSL 2.5.x, but may be added
   in the future... */
#ifndef X509_V_ERR_HOSTNAME_MISMATCH
static const long X509_V_ERR_HOSTNAME_MISMATCH = 0;
#endif

I see the X509_CHECK_FLAG_* aren't available in old libressel either, so you probably want to protect these flags in the same way.

@kaie
Copy link
Contributor Author

kaie commented Oct 10, 2018

I notice that _conditional.py needs to get updated, too. Is it acceptable to extend existing cryptography_has_102_verification_params?

@reaperhulk
Copy link
Member

Yes, cryptography_has_102_verification_params is appropriate for this binding so you can reuse that!

@alex
Copy link
Member

alex commented Oct 10, 2018

X509_CHECK_FLAG_NEVER_CHECK_SUBJECT is new in 1.1.0, so it'll need slightly seperate handling.

@kaie
Copy link
Contributor Author

kaie commented Oct 10, 2018

Thanks for the thorough review. The symbol also isn't supported by any libressl versions.

@alex alex merged commit ef18e61 into pyca:master Oct 10, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

3 participants