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

openssl: add support for CN / hostname verification #980

Closed

Conversation

lws-team
Copy link
Contributor

This series of three patches adds support to the openssl wrapper for OpenSSL APIs related to setting the expected hostname in the CN field of the peer cert, using mbedtls mbedtls_ssl_set_hostname(). That cannot be done at the moment because the necessary mbedtls ssl ctx is private to the wrapper. This makes it available to the user using only additional related OpenSSL apis.

This adds the standard OpenSSL api to get a pointer to the SSL struct's
X509_VERIFY_PARAM.  We need this for the OpenSSL api to set the peer
hostname introduced in the next patch.
This lets the user code set the mbedtls hostname using the standard OpenSSL
X509_VERIFY_PARAM_set1_host() API semantics.

The API takes an X509_VERIFY_PARAM pointer.  We use the fact that is
a composed member of the SSL struct to derive the SSL pointer.

The X509_VERIFY_PARAM_set1_host() is unusual in that it can accept a
NUL terminated C string as usual, or a nonterminated pointer + length.
This implementation converts the latter to the former if given, before
using it.

This is enough for user code to get the openssl wrapper to make
mbedtls confirm the CN on the peer cert belongs to the hostname used
to reach it, by doing, eg

	X509_VERIFY_PARAM_set1_host(SSL_get0_param(myssl), myhostname, 0);
This defines the OpenSSL X509_CHECK_FLAG_...s and the set/clear
accessors.  Since none of them are supported, the set / clear
accessor currently always does nothing and returns error.

This call is often part of the generic openssl user code to
set up certificate verification.  This patch allows it to
compile for ESP32 and decide at runtime what to do about
unsupported flags.
@projectgus
Copy link
Contributor

projectgus commented Oct 9, 2017

Thanks for sending this, @lws-team . Sorry noone has gotten back to you earlier.

Changes LGTM. I've asked the maintainer of the openssl compatibility layer to take a look as well.

@lws-team
Copy link
Contributor Author

lws-team commented Oct 9, 2017

I have a bunch more fixes on the wrapper too that are necessary for proper operation with nonblocking... basically we use the wrapper inside lws for mbedTLS compatibility.

What is "LGTM"?

@projectgus
Copy link
Contributor

I have a bunch more fixes on the wrapper too that are necessary for proper operation with nonblocking...

Great. FWIW, there are some similar changes (openssl non-blocking support) pending internally. Unsure of their ETA.

What is "LGTM"?

Looks Good To Me. :)

@lws-team
Copy link
Contributor Author

lws-team commented Oct 9, 2017

Well, then I'll leave the rest until we see what the author has updated himself.

@lws-team
Copy link
Contributor Author

... any eta on my patch getting looked at by "the maintainer of the openssl compatibility layer"?

@projectgus
Copy link
Contributor

Sorry for the (repeated) extended silence. I'm not sure what happened about this, but I'm going to chase it up again.

@lws-team
Copy link
Contributor Author

As I said there were a bunch of problems with the esp-idf wrapper (some of them serious data corruption). I appreciate the work done on it though, it helped us a lot and beers are owed to the original author. I can see it's kind of a hopeless / pointless open-ended task for him trying to cover every openssl api variation.

Since this is not an effective upstream for it (well... it isn't, is it), and so there's no way to cooperate to improve it, I forked a copy of it into our tree a while back and gave up trying to produce nice patches on it; where possible I add stuff using native mbedtls apis directly. That is a shame.

@lws-team lws-team closed this Nov 17, 2017
igrr pushed a commit that referenced this pull request Nov 21, 2017
This adds the standard OpenSSL api to get a pointer to the SSL struct's
X509_VERIFY_PARAM.  We need this for the OpenSSL api to set the peer
hostname introduced in the next patch.

Part of #980
igrr pushed a commit that referenced this pull request Nov 21, 2017
This lets the user code set the mbedtls hostname using the standard OpenSSL
X509_VERIFY_PARAM_set1_host() API semantics.

The API takes an X509_VERIFY_PARAM pointer.  We use the fact that is
a composed member of the SSL struct to derive the SSL pointer.

The X509_VERIFY_PARAM_set1_host() is unusual in that it can accept a
NUL terminated C string as usual, or a nonterminated pointer + length.
This implementation converts the latter to the former if given, before
using it.

This is enough for user code to get the openssl wrapper to make
mbedtls confirm the CN on the peer cert belongs to the hostname used
to reach it, by doing, eg

	X509_VERIFY_PARAM_set1_host(SSL_get0_param(myssl), myhostname, 0);

Merges #980
igrr pushed a commit that referenced this pull request Nov 21, 2017
This defines the OpenSSL X509_CHECK_FLAG_...s and the set/clear
accessors.  Since none of them are supported, the set / clear
accessor currently always does nothing and returns error.

This call is often part of the generic openssl user code to
set up certificate verification.  This patch allows it to
compile for ESP32 and decide at runtime what to do about
unsupported flags.

Merges #980
@projectgus
Copy link
Contributor

Hi @lws-team ,

After the long delay we ended up merging these with some minor whitespace/formatting cleanup.

I appreciate that this experience probably doesn't make you want to send any more PRs against the openssl wrapper code. For what it's worth if you change your mind then I'll push hard to get any future PRs looked at quickly without this kind of long delay.

Angus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants