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

hsts: enable by default #6627

Closed
wants to merge 5 commits into from
Closed

hsts: enable by default #6627

wants to merge 5 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Feb 18, 2021

in:

  • default build system
  • Makefile.m32
  • MakefileBuild.vc

@vszakats
Copy link
Member Author

vszakats commented Feb 18, 2021

Reason for test failures:
https://travis-ci.org/github/curl/curl/jobs/759528699#L4529

test 0493...[HSTS and %{url_effective} after upgrade]

 493: protocol FAILED!
 There was no content at all in the file log/server.input.
 Server glitch? Total curl failure? Returned: 6
== Contents of files in the log/ dir after test 493
=== Start of file commands.log
 ../libtool --mode=execute /usr/bin/valgrind --tool=memcheck --quiet --leak-check=yes --suppressions=./valgrind.supp --num-callers=16 --log-file=log/valgrind493 ../src/curl --include --trace-ascii log/trace493 --trace-time -x http://127.0.0.1:46655 http://this.hsts.example/493 --hsts log/input493 -w '%{url_effective}\n' >log/stdout493 2>log/stderr493
=== End of file commands.log
=== Start of file ftpserver.cmd
 Testnum 493
=== End of file ftpserver.cmd
=== Start of file input493
 # Your HSTS cache. https://curl.se/docs/hsts.html
 # This file was generated by libcurl! Edit at your own risk.
 .hsts.example "20311001 04:47:41"
=== End of file input493
=== Start of file stderr493
   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                  Dload  Upload   Total   Spent    Left  Speed

   0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (6) Could not resolve host: this.hsts.example
=== End of file stderr493
=== Start of file stdout493
 https://this.hsts.example/493
=== End of file stdout493
=== Start of file trace493
 19:37:21.794854 == Info: Switched from HTTP to HTTPS due to HSTS => https://this.hsts.example/493
 19:37:22.006503 == Info: Could not resolve host: this.hsts.example
 19:37:22.015886 == Info: Closing connection 0
=== End of file trace493

And this one in ubuntu torture, also for 493:
https://dev.azure.com/daniel0244/curl/_build/results?buildId=4776&view=logs&jobId=0ac86f4c-a876-5e3d-a660-b619f41e4cf7&j=0ac86f4c-a876-5e3d-a660-b619f41e4cf7&t=6eb60c57-d761-5380-d590-c7206b742dc5

test 0493...[HSTS and %{url_effective} after upgrade]
 116 functions found, but only fail 40 (34.48%)
** MEMORY FAILURE
Leak detected: memory still allocated: 32 bytes
At 5586f2066788, there's 32 bytes.
 allocated by dynbuf.c:98
LIMIT urlapi.c:1181 strdup reached memlimit
 Failed on function number 69 in test.
 invoke with "-t69" to repeat this single case.
== Contents of files in the log/ dir after test 493
[...]

HSTS build error in msys2_mingw64_debug_schannel, msys2_mingw64_debug_openssl, msys1_mingw64_debug_schannel:
https://dev.azure.com/daniel0244/curl/_build/results?buildId=4776&view=logs&jobId=7ec05304-8f7a-5c91-423c-6e0e33c05883&j=7ec05304-8f7a-5c91-423c-6e0e33c05883&t=cbd1fe55-b0b0-5ea8-5b6e-d6189cdec260

  CC       libcurl_la-hsts.lo
hsts.c: In function 'debugtime':
hsts.c:63:25: error: conversion from 'time_t' {aka 'long long int'} to 'long unsigned int' may change value [-Werror=conversion]
   63 |     unsigned long val = strtol(timestr, NULL, 10) + deltatime;
      |                         ^~~~~~

Another build-related one in msys2_mingw32_debug_schannel, msys2_mingw32_debug_openssl, msys1_mingw32_debug_schannel:
https://dev.azure.com/daniel0244/curl/_build/results?buildId=4776&view=logs&jobId=4fb39701-b345-57f4-c08f-de22c161bda5&j=4fb39701-b345-57f4-c08f-de22c161bda5&t=2d1c25bb-8829-5a17-99cf-d2e7ea866f40

  CC       libcurl_la-hsts.lo
hsts.c: In function 'hsts_create':
hsts.c:117:18: error: conversion from 'curl_off_t' {aka 'long long int'} to 'time_t' {aka 'long int'} may change value [-Werror=conversion]
  117 |   sts->expires = expires;
      |                  ^~~~~~~
hsts.c: In function 'Curl_hsts_parse':
hsts.c:214:20: error: conversion from 'curl_off_t' {aka 'long long int'} to 'time_t' {aka 'long int'} may change value [-Werror=conversion]
  214 |     sts->expires = expires;
      |                    ^~~~~~~

(Interestingly, this wasn't an issue with production builds, where HSTS has been enabled for a few months now. The difference may be the use of LLVM/clang there (it isn't: can't reproduce with gcc 10.2 either), a different mingw-w64 env or stricter warnings. It's a little strange nevertheless.)

@vszakats
Copy link
Member Author

vszakats commented Feb 19, 2021

After addressing the Windows build issues, this came up (e.g. msys2_mingw32_debug_openssl, msys2_mingw64_debug_openssl):
https://dev.azure.com/daniel0244/curl/_build/results?buildId=4786&view=logs&jobId=ea1d4c80-679f-50fc-e5c4-6cbed7e43517&j=ea1d4c80-679f-50fc-e5c4-6cbed7e43517&t=c45ce094-cc60-54c8-cd04-c6f7e36e4f2a

test 1660...[HSTS]
sh: line 1:  6583 Segmentation fault      ./unit/unit1660 - > log/stdout1660 2> log/stderr1660

 1660: stdout FAILED:
--- log/check-expected	2021-02-18 17:41:37.602128000 -0800
+++ log/check-generated	2021-02-18 17:41:37.602128000 -0800
@@ -1,34 +0,0 @@
-readfrom.example [readfrom.example]: 1633063661 includeSubDomains[LF]
-'old.example' is not HSTS[LF]
-'readfrom.example' is not HSTS[LF]
-example.com [example.com]: 1579905261[LF]
[...]

bagder added a commit that referenced this pull request Feb 19, 2021
@vszakats vszakats changed the title hsts: enable by default [WIP] hsts: enable by default Feb 19, 2021
@vszakats vszakats force-pushed the hsts-enable branch 2 times, most recently from dadf6d0 to 766dd28 Compare February 19, 2021 16:19
@jay
Copy link
Member

jay commented Feb 19, 2021

CI output reference links aren't suited for commit messages because they seem to be mostly ephemeral.

@vszakats
Copy link
Member Author

vszakats commented Feb 19, 2021

I don't intend to add those to the final squashed patch.

[ Having said that, I didn't know that Azure log links are ephemeral. With Travis / AppVeyor CI those would exist till the curl account there is kept alive (or when the log is manually deleted, where there is such option). In Azure, the copy-URL-for-a-line feature is broken, and it's impossible (both could be local settings) to copy a log section larger than the viewable area. I'll only use these if absolutely necessary. ]

@jay
Copy link
Member

jay commented Feb 19, 2021

On a different project that was rarely updated we used appveyor artifacts as the download links and one day they expired

@vszakats
Copy link
Member Author

AppVeyor artifacts do indeed expire after 6 months. This was introduced a few years ago. But logs are kept online without an expiration. With travis-ci.org (and bintray.com, etc...) being retired soon, no link lasts forever, so what I used to do is copy-paste the single relevant error message (+ compiler command-line) being fixed, if it is non-trivial.

@vszakats
Copy link
Member Author

vszakats commented Feb 19, 2021

At this point the one remaining failure is some 32-bit mingw debug builds segfaulting in init1660:
msys1_mingw32_debug_schannel, msys1_mingw_debug_schannel, msys2_mingw32_debug_openssl, msys2_mingw32_debug_schannel

I'm also preparing a patch that fixes time_t size for 64-bit mingw builds, and one that lets manually enabling 64-bit time_t for 32-bit mingw builds. Newer mingw-w64 envs do support it. See: #6636.

@vszakats vszakats force-pushed the hsts-enable branch 2 times, most recently from 6d6d89b to a0f45dc Compare February 21, 2021 02:03
@vszakats
Copy link
Member Author

vszakats commented Feb 21, 2021

Can't put my finger on the underlying dynamic, but after swapping time_t for curl_off_t in the HSTS structure, the mingw failures/crashes are gone. Similar solution is already being used in cookies.c (but not in altsvc.c, where time_t is used internally.)

Anyhow, now test493 fails in one valgrind session, but that's probably easier to look after. All help is much welcome here. (This seems to be a Could not resolve host: issue.)

@vszakats vszakats changed the title [WIP] hsts: enable by default hsts: enable by default Feb 22, 2021
@vszakats
Copy link
Member Author

vszakats commented Mar 2, 2021

I'm sadly giving up on this ...on the note, that it'd be real cool to enable it, and some of these fixes may make sense in a future time this happened.

@vszakats vszakats closed this Mar 2, 2021
@vszakats
Copy link
Member Author

vszakats commented Mar 8, 2021

Rebooted here: #6700 — Thanks @bagder!

@vszakats vszakats deleted the hsts-enable branch March 8, 2021 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants