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 support for HTTP Strict Transport Security using libhsts #2682

Open
wants to merge 5 commits into
base: master
from

Conversation

@zagor
Copy link
Member

commented Jun 23, 2018

This patch adds two new configure parameters:
--with-libhsts=PATH to point to libhsts
--with-hsts-file=FILE to specify location of the dafsa file that
contains the domain database

lib/url.c Outdated
@@ -293,6 +296,10 @@ void Curl_freeset(struct Curl_easy *data)
data->change.url = NULL;

Curl_mime_cleanpart(&data->set.mimepost);

#if defined(USE_HSTS)

This comment has been minimized.

Copy link
@bagder

bagder Jun 23, 2018

Member

I think you should stick with plain #ifdef when there's just one define to check, as that's what we usually do in the code.

@bagder

This comment has been minimized.

Copy link
Member

commented Jun 23, 2018

@rockdaboot

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2018

Thanks for letting me know about this patch ! Great work !

I didn't even make a release of libhsts yet. Nor did I make a real implementation by myself (but it's on my list for wget and wget2 - just striked out curl ;-). I was so busy the last weeks with polishing the GnuTLS build architecture... But I take this as a butt-kick to get busy with libhsts again :-)

@bagder

This comment has been minimized.

Copy link
Member

commented Jun 24, 2018

The test 1140 failure is irrelevant and due to commit b6a16af, subsequently fixed in commit 810ce31

@zagor zagor force-pushed the zagor:hsts branch from e71a5fb to 13f66f1 Jun 25, 2018

@@ -4274,5 +4371,6 @@ AC_MSG_NOTICE([Configured to build curl/libcurl:
metalink support: ${curl_mtlnk_msg}
PSL support: ${curl_psl_msg}
HTTP2 support: ${curl_h2_msg}
hsts support: ${curl_hsts_msg} (using ${HSTS_FILE})

This comment has been minimized.

Copy link
@vszakats

vszakats Jun 27, 2018

Member

Just a minor nit, lowercase hsts would look more readable (when referring to the standard) as uppercase HSTS in above line, here, here and here.

This comment has been minimized.

Copy link
@bagder

bagder Jun 27, 2018

Member

I agree with @vszakats!

@@ -861,6 +861,7 @@ CURL_VERSION_CURLDEBUG 7.19.6
CURL_VERSION_DEBUG 7.10.6
CURL_VERSION_GSSAPI 7.38.0
CURL_VERSION_GSSNEGOTIATE 7.10.6 7.38.0
CURL_VERSION_HSTS 7.61.0

This comment has been minimized.

Copy link
@bagder

bagder Jun 27, 2018

Member

same version number comment as below...

@@ -149,6 +149,9 @@ libcurl was built with support for TLS-SRP. (Added in 7.21.4)
.IP CURL_VERSION_NTLM_WB
libcurl was built with support for NTLM delegation to a winbind helper.
(Added in 7.22.0)
.IP CURL_VERSION_HSTS
libcurl was built with support for HSTS
(Added in 7.61.0)

This comment has been minimized.

Copy link
@bagder

bagder Jun 27, 2018

Member

Make it 7.62.0, which seems the likely candidate since we won't merge new features like this before 7.61.0 and the next release with new features will be 7.62.0...

@@ -4274,5 +4371,6 @@ AC_MSG_NOTICE([Configured to build curl/libcurl:
metalink support: ${curl_mtlnk_msg}
PSL support: ${curl_psl_msg}
HTTP2 support: ${curl_h2_msg}
hsts support: ${curl_hsts_msg} (using ${HSTS_FILE})

This comment has been minimized.

Copy link
@bagder

bagder Jun 27, 2018

Member

I agree with @vszakats!

lib/url.c Outdated
@@ -119,6 +119,9 @@ bool curl_win32_idn_to_ascii(const char *in, char **out);
#include "dotdot.h"
#include "strdup.h"
#include "setopt.h"
#ifdef USE_HSTS
#include "libhsts.h"

This comment has been minimized.

Copy link
@bagder

bagder Jun 27, 2018

Member

should this perhaps include it <libhsts.h> since it isn't a local header?

@@ -313,6 +313,10 @@ typedef enum {
#include <iconv.h>
#endif

#ifdef USE_HSTS
#include "libhsts.h"

This comment has been minimized.

Copy link
@bagder

bagder Jun 27, 2018

Member

same as above

@bagder

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@zagor let me know if you need any help getting changes to the test scripts done so we can have a test or two of this added.

@zagor

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

@bagder @vszakats Thanks for the reviews, fixes are coming.

@bagder Sure, if you can point to examples of tests that use similar features that would be a good help

@bagder

This comment has been minimized.

Copy link
Member

commented Jun 28, 2018

I think the PSL test, test 1136, is a good example. Note how it sets "PSL" as a required feature for the test to run.

The runtests.pl script first checks if the build has the feature enabled and if so, sets the associated variable. The logic that then makes sure that the test's required feature matches what the local build features is here.

You could use a similar approach for HSTS.

I would also suggest that you make the test depend on "debug", which makes it only work on debug builds but then you can add custom logic #ifdefed on DEBUGBUILD that isn't present in release builds. It could for example include something like:

#ifdef DEBUGBUILD
char *special = curl_getenv("CURL_HSTSFILE");
if(special) {
  /* use this file name instead of the real one */
  free(special);
}
#endif

You can then make the test case set that environment variable. Test 1136 sets the TZ environment variable, which shows how to do it.

@bagder

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

@zagor you up to getting this polished and ready for merge?

I'll volunteer to write up a travis adjustment for it post-merge.

@zagor

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2018

Yes, sorry for the delay. I had a long vacation and sort of forgot about this. I'll trim the final bits soon!

zagor added some commits Apr 15, 2018

Add support for HTTP Strict Transport Security using libhsts
This patch adds two new configure parameters:
 --with-libhsts=PATH to point to libhsts
 --with-hsts-file=FILE to specify location of the dafsa file that
contains the domain database
Added CURL_VERSION_HSTS
Added library version output
Added infof() explaining https upgrade
Fixed ifdefs

@zagor zagor force-pushed the zagor:hsts branch from c49c083 to 37d654e Sep 14, 2018

@zagor

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2018

Fixed the review comments. Test case coming soon.

@zagor zagor force-pushed the zagor:hsts branch from 37d654e to 8c2a47c Sep 19, 2018

@bagder

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

I think the tests look fine!

Some questions/wishes on the patch in general:

  1. The 'hsts' handle should probably be moved to the UrlState instead of the UserDefined in lib/urldata.h for consistency. The UserDefined tend to hold the exact values set with curl_easy_setopt() while the UrlState is for holding things that are updated run-time in the handle.
  2. I would actually presume that (some) users will want to specify a dedicated HSTS file when doing transfers, rather than just having the default path set at build-time. To support a custom path, the loading of the HSTS file can't be done in Curl_open() since that happens before any options is set in the handle.
  3. I would like users to be able to switch off the HSTS use in case they want to test stuff without it (a little in the --insecure spirit). If we do (2) that way could then be to just point to a missing/other file.
@zagor

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

Doesn't #1 and #2 combined mean that I'd have to initialize libhsts once for every easy handle?

Initialization is not very lightweight, as the default database is ~700KB

@bagder

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

Sorry, forgot to weigh in on this but thanks for underscoring the issue for me. Here's what I think...

Loading 700KB only to do a single transfer, kill the handle and then do another one that loads the same 700KB again is certainly almost like a nightmare scenario to many users. At the same time, doing it unconditionally with a fixed path in curl_global_init() because it was enabled at built-time is also inconveniently inflexible and will also cause sad faces (possibly in another group of users).

However, doing HSTS globally still has so many downsides that I can't see it being a very good way forward. For example users will be stuck with that single HSTS file, no matter how many easy handles or threads it uses.

I think we pretty much have to do the load per-handle in spite of all. We should probably then only do it for users that ask for it, and then we can allow a custom path to the file.

As a follow-up, we can then add HSTS sharing to the share API so that a user can select to share that single HSTS between exactly those handles they think is suitable. An application could then even create/kill easy handles and re-use the same cached share object if they want to use HSTS without reloading it many times.

@vszakats

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

Initialising the HSTS object on first-use and sharing it afterwards looks like a nice solution — hsts_search() doesn't seem to want to modify the HSTS object, which helps.

@stale

This comment has been minimized.

Copy link

commented Apr 3, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 3, 2019

@zagor

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

Thanks for the reminder, bot. I haven't abandoned this, just being really bad at finishing what I started. 😄

@stale stale bot removed the stale label Apr 4, 2019

@bagder

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

I have @zagor's work rebased and squashed in a separate branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.