Skip to content

Add SNI support for TLS #1113

Closed
T3RR7 wants to merge 3 commits intoyhirose:masterfrom
Group-IB:master
Closed

Add SNI support for TLS #1113
T3RR7 wants to merge 3 commits intoyhirose:masterfrom
Group-IB:master

Conversation

@T3RR7
Copy link
Copy Markdown

@T3RR7 T3RR7 commented Nov 29, 2021

No description provided.

@yhirose
Copy link
Copy Markdown
Owner

yhirose commented Dec 1, 2021

@j3su5cr1st, thank you for the pull request, but I don't understand the necessity of this change. Could you explain why we need to set a different hostname to SSL_set_tlsext_host_name rather than the hostname set in the constructor? Thanks!

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 1, 2021

@yhirose Hi and thx for pretty library!
The best explanation given in nginx docs (Ctrl + F, Server Name Indication)

Short answer: "A more generic solution for running several HTTPS servers on a single IP address is the TLS Server Name Indication (SNI) extension (RFC 6066), which allows a browser (actually any client) to pass a requested server name during the SSL handshake".

@yhirose
Copy link
Copy Markdown
Owner

yhirose commented Dec 1, 2021

@j3su5cr1st, thanks for the info. I'll explain my question more clearly.

Suppose we host two domains (aaa.com, bbb.com) with a single IP 192.168.1.1 on a server. In order to send a request to aaa.com, I think we have to set "aaa.com" SSL_set_tlsext_host_name instead of something else, so that we can correctly get the certificate corresponding to the domain "aaa.com". So we don't need such set_sni method.

Only case we need set_sni is when a user create a SSLClient with IP address instead of hostname like SSLClient cli("192.168.1.1", 443, cert_path, key_path);. In this case, I understand we need to set either "aaa.com" or "bbb.com" via set_sni before the SSL handshake happens.

Please let me know if I misunderstand something. Thanks!

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 1, 2021

Yes, you understand the case clearly, but as I understand, this can spread not only on ip addresses.

@yhirose
Copy link
Copy Markdown
Owner

yhirose commented Dec 1, 2021

this can spread not only on ip addresses

What does it mean by that? Are there any other specific examples?

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 2, 2021

That means you can use not only IP addresses in host, but domain names too. SNI may be required inn case when actual server name differs from domain name, commonly in case when there are multiple servers on same host.

@PixlRainbow
Copy link
Copy Markdown
Contributor

PixlRainbow commented Dec 5, 2021

does it work if you use the existing set_hostname_addr_map and provide multiple domain names that link to the same ip? Though I admit this does not work if you don't know the IP of the host, and only know the WAN-facing hostname.

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 9, 2021

@PixlRainbow, Doesn't matter how many domain names you set via set_hostname_addr_map. The purpose is TLS handshake with internal server which is hidden under domain IP. Generally if you set SNI you pretty understand you would connet to, and what certificate you would get.

Check out this article: https://daniel.haxx.se/blog/2018/04/05/curl-another-host/.

@PixlRainbow
Copy link
Copy Markdown
Contributor

PixlRainbow commented Dec 9, 2021

httplib already calls SSL_set_tlsext_host_name to set SNI value. The hostname used for SNI will change depending on which host name is set in the constructor of the client, and the addr_map overrides the default resolution behaviour so that different hostname SNI may be used to connect to the same external server.
The main issue with the current implementation in httplib however is that it doesn't allow you to use DNS resolution and SNI override simultaneously, it only allows one or the other. Your PR solves that issue by separating it out into another method.

@yhirose
Copy link
Copy Markdown
Owner

yhirose commented Dec 10, 2021

@j3su5cr1st, @PixlRainbow, thank you for valuable inputs. After I read the article that @j3su5cr1st recommended and the curl documentation, I am now thinking of supporting curl-like methods resolve and connect_to as curl's --resolve and --connect-to options do respectively. I think these methods can cover all the scenarios that we could face.

void resolve(const std::string &host, int port, const std::string &ip);
void connect_to(const std::string &host1, int port1, const std::string &host2, int port2);

They look a bit redundant though, I think users can understand them easily because their behaviors are the same as the corresponding curl options. Also it's easier for me to document it on README.

Anyway, please let me know your thoughts before I start working on it. Thanks!

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 10, 2021

@yhirose IMO they looks like methods which do they things immediately, but not set the properties to future communication, i.e. connect_to seems like method should literally opens connection to server, maybe it will be a good idea to add set_ prefix to methods you offered.
Moreover, SNI is a very specific mechanism that yet again IMO is being used rarely and in very specific cases so, I think method to use it should be named explicitly and contains sni in it's name. As a second variant I see - make a pretty good documentation which unfortunately could be a bit better.

So, let me know about you opinion about this PR, maybe you see any fixes and changes should make this functionality more clear and comfortable in httplib.

@PixlRainbow
Copy link
Copy Markdown
Contributor

PixlRainbow commented Dec 10, 2021

what about a helper function (not class method) that resolves the IP address of an external hostname immediately? For example:

std::string httplib::hosted_at(const char* hostname);

Which you would then use with the existing set_hostname_addr_map like so:

// resolve external hostname "eggsamplers.com" to an IP address
// use the external IP address to connect to internal SNI host "example.com"
std::map<std::string, std::string> host_map = {
    { "example.com", httplib::hosted_at("eggsamplers.com") }
};
httplib::SSLClient cli("example.com");
cli.set_hostname_addr_map(host_map);
auto res = cli.Get("/");

Using the map would also allow you to handle the case of automatic redirects between multiple different SNI zones.
It does still have the issue with not making it explicitly clear to programmers that SNI is in use however.

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 10, 2021

@PixlRainbow Looks like overengineering :)
Why not to just add simple method that explicitly do the SNI functionality and do not requires many changes and additional lines of code, like it did in PR? IMO it could be better decision to use simple setters, they're a bit simpler to manage if errors occured. Something like that already offered by @yhirose, and implemented in curl with curl_easy_setopt.

Btw, in your example, if hosted_at wil fail, what will happen? Will it throw an exception or all just will get broken?

@PixlRainbow
Copy link
Copy Markdown
Contributor

PixlRainbow commented Dec 10, 2021

Thanks for your feedback, I think your simpler interface is better.
To answer your question, it will just return an empty (but not null) string. Httplib will attempt to open a socket with an empty address, and based on existing error handling, this will just abort the request and set the response error code to httplib::Error::Connection.
No modification is made to existing class methods or members. The helper function would just be a standalone static wrapper function around getnameinfo.

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 10, 2021

Okay, thanks for answer!
Waiting for @yhirose opinion/PR approve.

@yhirose
Copy link
Copy Markdown
Owner

yhirose commented Dec 10, 2021

@j3su5cr1st, @PixlRainbow, I am still trying to understand a whole picture of this issue, and thanks for your insightful comments.
This is also what I found helpful at https://everything.curl.dev/usingcurl/connections/name:

image

According to this information, they always set the host name in the given URL to the SNI, and the actual connection IP address can be changed with --resolve. This is exactly what the current httplib::SSLClient does. The host name in the constructor will always be used for SSL_set_tlsext_host_name, and the IP can be adjusted with set_hostname_addr_map. That's why @PixlRainbow's suggestion looks very similar to the curl solution with --resolve.

On the other hand, set_sni looks taking us to the opposite direction. We can now change the host name with the method and we are not supposed to change the IP address with set_hostname_addr_map any more.

So if we add set_sni, it seems to introduce a different concept from what set_hostname_addr_map currently gives, and it might get users confused. But as @PixlRainbow mentioned, this approach could be simpler and more flexible.

@j3su5cr1st , for better understanding, could you show me a few specific examples and how you can handle them with both the curl option (--resolve or --connect-to) and set_sni method on httplib::SSLClient? Then, I'll compare these to understand this issue more clearly and to finalize the best possible API. (I am even OK to make an adjustment to set_hostname_addr_map if necessary.)

Thanks for your help!

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 11, 2021

Simplest example is, as I wrote before, two servers with TLS on the same port and address. In this case just setting Host in the http headers is not enough, we should set SNI anyway to get right certificate. This is also required for server side too, lets see simple example:

We have:

  • foo.com as domain name
  • foo.com:443 as main service on this domain
  • foo-license.com:443 as auth service which allows to connect only trusted client with certificate signed by distributer (SSL pinning).

The first thing that client's application does at startup is checking license, it would connect to license server foo-license.com:443 with built-in certificate but using the same address and port as the public API has (foo.com:443). That approach can help to avoid some security issues . Server foo-license.com will get the trusted certificate and will allow communication, on the other side foo.com:443 shouldn't does with same cert.

This example is a bit synthetic but has the right to life and around close to real life case.

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 11, 2021

SNI.jpg

This should look like shown on this picture.

@yhirose
Copy link
Copy Markdown
Owner

yhirose commented Dec 13, 2021

@j3su5cr1st, (and @PixlRainbow) thanks for showing the above example. Since there is no specific code example, I just came up with 3 ways to handle your example as below.

curl

# Find a IP address for the host name
dig foo.com # or `nslookup foo.com`

# Use the  '[IP]' as IP address to connect resolved with dig or nslookup, and use 'https://foo-license.com' as SNI
curl --resolve foo-license.com:443:[IP] https://foo-license.com/bar

httplib::Client with set_hostname_addr_map and hosted_at

NOTE: hosted_at is not implemented in httplib.h, yet.

// The host name will be used for SNI
httplib::Client cli('https://foo-license.com");

// Use a value from `httplib::hostest_at("foo.com")` as IP address to connect
cli.set_hostname_addr_map({{"foo-license.com", httplib::hosted_at("foo.com")}});

cli.Get("/bar");

httplib::Client with set_sni

NOTE: This pull request currently supports set_sni only in SSLClient, but not in Client, yet.

// Use a resolved IP address from foo.com to connect
httplib::Client cli('https://foo.com");

// Set SNI to foo-license.com
cli.set_sni("foo-license.com");

cli.Get("/bar");

Do you think that all the above cases handle your example correctly, or do you find any mistakes in them?

Another thing is that the set_sni way doesn't seem to work with set_follow_location which may cause redirection. If a redirection happens, the new site most likely needs a different SNI name rather than the one set by set_sni.

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 15, 2021

This pull request currently supports set_sni only in SSLClient, but not in Client, yet.

Thi feature can't be implemented for HTTP client, it's only for TSL connection.

Do you think that all the above cases handle your example correctly, or do you find any mistakes in them?

Yes, whole cases above is right.

Another thing is that the set_sni way doesn't seem to work with set_follow_location which may cause redirection. If a redirection happens, the new site most likely needs a different SNI name rather than the one set by set_sni

Don't think that here's a problem because if you use SNI you're perfectly understand what you do. I know no one cases when you are setting up SNI that differs from original host name and using not own (or not documented) backed.

By the way, in you example with redirection the issue seems like currently exists too, isn't it?

@yhirose
Copy link
Copy Markdown
Owner

yhirose commented Dec 15, 2021

Thi feature can't be implemented for HTTP client, it's only for TSL connection.

Client supports both 'HTTP only interface' and 'Universal interface (http and https)'. If you construct a Client like httplib::Client cli('https://foo-license.com");, it will internally use SSLClient. So it actually have methods for SSL as well like ssl_context.

cpp-httplib/httplib.h

Lines 1157 to 1171 in 9fa426d

class Client {
public:
// Universal interface
explicit Client(const std::string &scheme_host_port);
explicit Client(const std::string &scheme_host_port,
const std::string &client_cert_path,
const std::string &client_key_path);
// HTTP only interface
explicit Client(const std::string &host, int port);
explicit Client(const std::string &host, int port,
const std::string &client_cert_path,
const std::string &client_key_path);

By the way, in you example with redirection the issue seems like currently exists too, isn't it?

I feel your example isn't a common case to me. (Please let me know if I am wrong.)
Here is the example that I think is common:

  • A hosting service (example_hosting_service.com) having a number of virtual domains
    • My virtual domain under the hosting service (example_my_servive.com) which is used for the SSL hand shaking as well
    • Other user's virtual domain under the hosting service (example_other_service.com) which is used for the SSL hand shaking as well

Then we suppose all the requests to 'example_my_domain.com' redirect to 'example_other_service.com'. I think it's a pretty common situation, and it works with the following:

httplib::Client cli("https://example_my_service.com"); // Connect to the IP of `example_hosting_service.com` and use `example_my_service.com` for virtual domain and SNI
cli.set_follow_location(true);
cli.Get("/hi");
// Internally this call does the following:
// httplib::Client cli("https://example_other_service.com");  // Connect to the IP of `example_other_service.com` and use `example_other_service.com` for virtual domain and SNI
// cli.set_follow_location(true);
// cli.Get("/hi");

As you can see, the current cpp-httplib implementation supports at least the above common situation even now.

@PixlRainbow, do you have any thoughts?

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 20, 2021

Client supports both 'HTTP only interface' and 'Universal interface (http and https)'. If you construct a Client like httplib::Client cli('https://foo-license.com");, it will internally use SSLClient. So it actually have methods for SSL as well like ssl_context.

Got you, okay.
But in this case HTTPClient can use set_sni internally, or even keep current implementation. I don't see the issue to add method I've requested to pull, or don't understand what issue consists in. Can you explain what do you think about it, thanks!

@PixlRainbow
Copy link
Copy Markdown
Contributor

PixlRainbow commented Dec 20, 2021

yhirose means that set_sni method can't be accessed through public interface if a user attempts to use TLS through normal Client's internal SSLClient, so the methods may need to be aliased in ClientImpl and Client where some other tls-related methods are already present

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 21, 2021

Okay, @PixlRainbow @yhirose, can someone provide the vision of set_sni implementation with ClientImpl in code? Thanks!

@yhirose
Copy link
Copy Markdown
Owner

yhirose commented Dec 21, 2021

@j3su5cr1st, thanks for your efforts. But not only the issue with Client, I very much worry the fact that the set_sni way cannot handle the redirection problem properly. So even if the the httplib::hosted_at looks uglier, I prefer the solution to the set_sni. Also it shares the same concept that curl uses and people are more familiar to. Thanks for your understanding.

@T3RR7
Copy link
Copy Markdown
Author

T3RR7 commented Dec 21, 2021

Okay, not problem. The main goal is functionality, but the interface defines the majority, thanks!

@yhirose
Copy link
Copy Markdown
Owner

yhirose commented Dec 21, 2021

@j3su5cr1st, but I really appreciate you raised this issue, and it made me understand SNI much better than before. I'll ponder over the actual interface more, then I'll get back here. Thanks a lot!

@yhirose yhirose closed this in 65a8f4c Dec 31, 2021
ExclusiveOrange pushed a commit to ExclusiveOrange/cpp-httplib-exor that referenced this pull request May 2, 2023
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.

3 participants