HttpClient: Adding SSL Certification Verification #3176

salqadri · 2017-04-28T13:25:53Z

This PR adds a new API called "setRootCA" in which the user can pass in the root CA with which to validate an SSL connection.

WiFiClientSecure enables users to add multiple certificates for validation. But the underlying axTLS library actually only cares for the deepest certificate for verification. As such, I decided to have a much simpler API on HttpClient of setting a single certificate to prevent users from getting confused on how many certs they need to add. I call it Root to indicate that this must be the deepest certificate.

Unfortunately, the existing implementation of HttpClient was not conducive to what I wanted to do. I wanted the following API experience:
http.begin("https://www.google.com", 443); http.setRootCA(Equifax_cer, Equifax_cer_len); int result = http.GET();
But the problem was that both the connect call and the actual http request were both done in the path for http.GET. axTLS Certificate validation requires a TCP connection to first exist, and THEN you can load a certificate (I spent several hours figuring this and other issues like the fact that you HAVE to initialize sntp before validation can happen out by digging deep into the axTLS codebase).

So given this issue, I tried to solve this problem without causing API changes. The result was that I moved the connect phase into the begin command, and added a new state variable called _verified to determine if verification has already been done to avoid doing it again on subsequent connections.

I've also put in an example ino test file that anyone should be able to run to see how this works. One key thing to note is that you HAVE to initialize sntp before attempting validation because the certificate is validated with respect to time.

codecov-io · 2017-04-29T15:53:11Z

Codecov Report

❗ No coverage uploaded for pull request base (master@eb891cd). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #3176   +/-   ##
========================================
  Coverage          ?   27.6%           
========================================
  Files             ?      20           
  Lines             ?    3655           
  Branches          ?     678           
========================================
  Hits              ?    1009           
  Misses            ?    2468           
  Partials          ?     178

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb891cd...6c2a8a3. Read the comment docs.

igrr

Overall this is a valuable feature, however please revert the old behaviour of begin methods.

igrr · 2017-05-08T08:36:43Z

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

    clear();
+
+    // Remove protocol if any


This method takes host as an argument, i.e. just the DNS name or IP address. It should not expect to see a protocol prefix.

Makes the function more resilient for me. It is also more consistent with the other overload, making it more easy to swap out the API a developer may want to use.

igrr · 2017-05-08T08:41:33Z

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

+{
+    bool result = false;
+    if(_tcp && (_protocol == "https" || _port == 443)) {
+        auto client = static_cast<WiFiClientSecure*>(_tcp.get());


If you follow the existing TransportTraits pattern, you can create a new TransportTraits class which performs root certificate verification instead of fingerprint verification. In setRootCA method, you can set _transportTraits to an instance of, say, new TLSRootCertTraits(cert, size), which will then perform verification using the existing interface.

The approach you suggest would be more expensive as it requires storing the cert, which can be quite large.

igrr · 2017-05-08T08:45:09Z

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

+    }
+    _tcp = _transportTraits->create();
+
+    if(!_tcp->connect(_host.c_str(), _port)) {


This is a fairly big behavioural change, and it's not hard to come up with a bunch of cases when it will break existing sketches. If you need to defer setting the CA until the time connection is established, save the CA information in the appropriate Traits structure (probably a new one, dedicated to verification using a root CA cert). Then use this information when the verification has to take place. If HTTPClient needs to interact with Traits class at some other point in the process, add a new interface method and implement it as needed. It doesn't look like it is worth changing the behaviour of user-facing class, when the problem can be solved inside the implementation.

No saving the CA is a bad idea as it would consume a lot of RAM. I've been trying to explicitly avoid storing the CA.

OK, that is a fair observation.
Note that if the certificate is loaded from file, we need to have a copy of it in RAM at some point anyway to pass this copy into axTLS. Same for PROGMEM. So in these two cases, there is no optimization possible (which wouldn't involve changing axTLS to be capable of reading from streams).
In the case when the certificate is already in RAM (such as in your example) we can store the pointer to the certificate and pass it on to axTLS without making a copy. This however is likely the least useful option, because certificate places into .rodata will take up precious RAM during the entire application life. On the other hand, the certificate in FS or PROGMEM will be in RAM only when it is actually needed there — during the TLS connection phase.

Agreed. What I'm trying to do is make it a temporary cost; you only need it in RAM during the verification phase, and then you can unload it from RAM. Something like this:
SPIFFS.begin(); File ca = SPIFFS.open(F("/Equifax.cer"), "r"); if (ca) { if (!http->setRootCA(ca, ca.size())) { Serial.println(F("Root CA failed to load")); } ca.close(); } SPIFFS.end();

igrr · 2017-05-08T08:46:48Z

libraries/ESP8266HTTPClient/examples/SSLCertificateVerification/SSLCertificateVerification.ino

+ * Copyright (c) 2017 Syed Salman Qadri. All rights reserved.
+ * This file is part of the ESP8266HTTPClient for Arduino.
+ *
+ * This library is free software; you can redistribute it and/or


examples are normally licensed as public domain or CC0

igrr · 2017-05-08T08:47:10Z

libraries/ESP8266HTTPClient/examples/SSLCertificateVerification/SSLCertificateVerification.ino

+
+// This is a binary dump of the root CA.
+// You can also choose to load the cert via a file stream.
+// Here are the steps I did to dump the CA on my Mac:


Suggest writing this in a slightly more impersonal way :)

Also, what steps would users on other platforms take? Windows is probably the biggest question. Instructions for Linux will likely be the same, except for Mac-specific names ("Keychain Access", "Terminal").

The exact same commands works on Windows too.

Good to know. I can update that it works on Mac and Windows.

igrr · 2017-05-08T08:49:26Z

libraries/ESP8266HTTPClient/examples/SSLCertificateVerification/SSLCertificateVerification.ino

+//   2) Open Keychain Access. In the Search bar type "Equifax" and press Enter.
+//   3) Right-click the cert and click Export and save it as a .cer.
+//   4) In the terminal, type xxd -i <path_to_cer_file>
+unsigned char Equifax_cer[] = {


Can this be put in PROGMEM? We also need to support setRootCA_P in this case.

Great idea. Maybe we can do that in a subsequent PR.

igrr · 2017-05-08T08:49:55Z

libraries/ESP8266HTTPClient/examples/SSLCertificateVerification/SSLCertificateVerification.ino

+  0x70, 0xf1, 0x94, 0x21, 0x65, 0x44, 0x5f, 0xfa, 0xfc, 0xef, 0x29, 0x68,
+  0xa9, 0xa2, 0x87, 0x79, 0xef, 0x79, 0xef, 0x4f, 0xac, 0x07, 0x77, 0x38
+};
+unsigned int Equifax_cer_len = 804;


suggest sizeof(Equifax_cer) / sizeof(Equifax_cer[0]) here.

These two lines are generated by the previous command, so I'm trying to show that you can just copy-paste verbatim.

salqadri · 2017-05-16T23:38:30Z

@igrr I hadn't noticed that you guys provided feedback on my PR until now :). Thanks! I'll happily make the various fixes and redo it, but only if we don't make it so that we have to store the cert in the traits. That approach consumes too much memory, and I'd rather give the developer more control on how they wish the cert to be stored. Whether the developer is storing it in a file system, or in memory, either way the traits approach leads to duplication which causes crashes.

salqadri · 2017-06-10T16:54:00Z

@igrr My branch seems to be broken after I merged from master. SSL cert verification is no longer working for me. Sometime after c5c138e and between 8166263 something you did has caused my fork to now fail. It seems that there were a lot of changes to various SSL aspects, but I'm having a hard time understanding what you were trying to do. Could you possibly explain the changes to things like WiFiClientSecure etc. so that I can figure out what broken?

EDIT: Ok I isolated the issue to this commit: 1d41859

EDIT: I was able to reproduce the issue. It seems to happen for a particular cert. I created an issue with reproduction steps using your own sample here: #3340

Please fix this asap, since the bug is in axTLS and it is too complex for me to fix myself.

…roke the ability to do SSL validation with Google services

salqadri · 2017-07-29T06:56:07Z

I have now fixed this branch by getting everything from master except that axtls library update. So I'm using the version of axtls from prior to the May 9 2017 update. Would love it if @igrr you can fix axtls so that I don't have to do this workaround.

… which broke the ability to do SSL validation with Google services" This reverts commit 92219f6.

Defozo · 2017-08-08T14:14:26Z

When it'll be merged?

… 9 2017, which broke the ability to do SSL validation with Google services"" This reverts commit bb7ba50.

cimba007 · 2017-12-11T22:36:46Z

Any update so far?

cimba007 · 2018-01-13T13:11:44Z

@salqadri: Any chance to fix this?

earlephilhower · 2018-10-17T22:36:59Z

I believe this PR is superseded by #4980 which lets you stick in a custom configured WiFiClient* to HTTPClient.

salqadri added 8 commits April 23, 2017 22:22

Adding support for setting a Root CA in HTTPClient for https connections

e0c99ea

Fixed verification of CA

0a87071

Merge branch 'master' of https://github.com/esp8266/Arduino

094339c

Fixed CA verification for all overloads of begin

c2d8ed7

Adding an example of how to do CA validation with HTTPClient

f22234f

Merge branch 'master' of https://github.com/esp8266/Arduino

0ff4ab9

Added instructions on how to generate certificate dump

0dd5759

Merge branch 'master' of https://github.com/esp8266/Arduino

c114fa9

salqadri mentioned this pull request May 3, 2017

Is there a possibility to make HTTPClient accept all certificates (without checking fingerprint)? #2989

Closed

igrr requested changes May 8, 2017

View reviewed changes

salqadri added 2 commits May 30, 2017 16:06

Merge remote-tracking branch 'upstream/master'

903740e

Removed some personalized text from the example

5abe062

salqadri added 3 commits July 28, 2017 23:18

Merge remote-tracking branch 'upstream/master'

141e260

Adding a PROGMEM version of setRootCA

2c21178

Reverting libaxtls.a back to the version prior to May 9 2017, which b…

92219f6

…roke the ability to do SSL validation with Google services

Revert "Reverting libaxtls.a back to the version prior to May 9 2017,…

bb7ba50

… which broke the ability to do SSL validation with Google services" This reverts commit 92219f6.

Revert "Revert "Reverting libaxtls.a back to the version prior to May…

9b2eb78

… 9 2017, which broke the ability to do SSL validation with Google services"" This reverts commit bb7ba50.

proppy mentioned this pull request Sep 11, 2017

Needed to change Firebase Fingerprint FirebaseExtended/firebase-arduino#236

Closed

salqadri added 2 commits October 1, 2017 01:12

Enabling const arrays to be passed into setRootCA

826f8f4

Merged from upstream

6c2a8a3

earlephilhower closed this Oct 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpClient: Adding SSL Certification Verification #3176

HttpClient: Adding SSL Certification Verification #3176

salqadri commented Apr 28, 2017 •

edited

Loading

codecov-io commented Apr 29, 2017 •

edited

Loading

igrr left a comment

igrr May 8, 2017

salqadri May 16, 2017

igrr May 8, 2017

salqadri May 16, 2017

igrr May 8, 2017

salqadri May 16, 2017

igrr May 17, 2017

salqadri May 30, 2017 •

edited

Loading

igrr May 8, 2017

igrr May 8, 2017

igrr May 8, 2017

Defozo May 8, 2017 •

edited

Loading

salqadri May 16, 2017

igrr May 8, 2017

salqadri May 30, 2017

igrr May 8, 2017

salqadri May 16, 2017

salqadri commented May 16, 2017

salqadri commented Jun 10, 2017 •

edited

Loading

salqadri commented Jul 29, 2017

Defozo commented Aug 8, 2017

cimba007 commented Dec 11, 2017

cimba007 commented Jan 13, 2018

earlephilhower commented Oct 17, 2018

HttpClient: Adding SSL Certification Verification #3176

HttpClient: Adding SSL Certification Verification #3176

Conversation

salqadri commented Apr 28, 2017 • edited Loading

codecov-io commented Apr 29, 2017 • edited Loading

Codecov Report

igrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salqadri May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Defozo May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salqadri commented May 16, 2017

salqadri commented Jun 10, 2017 • edited Loading

salqadri commented Jul 29, 2017

Defozo commented Aug 8, 2017

cimba007 commented Dec 11, 2017

cimba007 commented Jan 13, 2018

earlephilhower commented Oct 17, 2018

salqadri commented Apr 28, 2017 •

edited

Loading

codecov-io commented Apr 29, 2017 •

edited

Loading

salqadri May 30, 2017 •

edited

Loading

Defozo May 8, 2017 •

edited

Loading

salqadri commented Jun 10, 2017 •

edited

Loading