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 HSTS to F# ApiServer and BwdServer #3665

Merged
merged 14 commits into from
Apr 21, 2022

Conversation

StachuDotNet
Copy link
Member

@StachuDotNet StachuDotNet commented Apr 8, 2022

This matches the HSTS config found here and here.

  • add HSTS into both ApiServer and BwdServer; ensure the expected header is present
    Strict-Transport-Security: max-age=31536000; includeSubDomains; preload
  • ensure existing tests pass (haven't checked yet)
  • should this be turned on only in prod, or is local OK too? localhost prevents HSTS locally, so maybe OK to leave it on locally. Edit: works fine locally, so I'm seeing no reason to disable.
  • add test(s) against ApiServer
  • add test(s) against BwdServer

@pbiggar
Copy link
Member

pbiggar commented Apr 8, 2022

  • do we need this against both ApiServer and BwdServer?

yes

The implementation here is a bit of a hack; for
some reason, the constructs used in ApiServer.fs are
not working here. A follow-up commit is likely to clean
this up.

Note: 205/211 needed adjustment; the other 6
did not, as the requests do not hit our HTTP stack

Several (5) of the tests required careful adjustment
through vim because of their inconsistent line endings.
@StachuDotNet
Copy link
Member Author

StachuDotNet commented Apr 18, 2022

Most of the tests (200/211), I could just edit normally in VS Code to expect the "Strict-Transport-Security: max-age=31536000; includeSubDomains; preload" F#-server-only header expectation.

The requests of these tests fail to reach our HTTP stack, and did not need update:

  • request-header-content-length-too-long
  • request-header-content-length-too-short
  • request-header-unicode-invalid-name
  • request-path-invalid-unicode
  • request-path-start-0-to-3
  • vars-unicode-valid

These tests required a more careful edit because of the awkward newline situation:

  • favicon-fallback
  • request-header-transfer-encoding-incomplete
  • request-header-transfer-encoding-json
  • request-header-transfer-encoding-slow
  • request-header-transfer-encoding-valid

Today, I successfully used (and escaped) vim. I tried using some hex editors for this, but I was somehow more lost there.

  • fix the readme format
  • figure out why the HSTS setup used in ApiServer isn't working well in BwdServer
    (this PR currently includes a hack around that fact)

@StachuDotNet
Copy link
Member Author

StachuDotNet commented Apr 18, 2022

Close. Remaining TODOs:

  • fix request-json-value-obj-trailing-comma.test which I apparently broke
  • decide if we need to expand upon ApiServer tests, and if so - how
  • figure out why BwdServer wouldn't work with the same method of adding HSTS that ApiServer.fs used

@StachuDotNet
Copy link
Member Author

Out of the URLs that ApiServer exposes,

These are now tested for the Strict-Transport-Security header:

  • GET /a/{canvasName}
  • POST /api/{canvasName}/initial_load
  • POST /api/{canvasName}/packages
  • POST /api/{canvasName}/get_worker_stats
  • POST /api/{canvasName}/all_traces
  • POST /api/{canvasName}/get_404s
  • POST /api/{canvasName}/get_db_stats
  • POST /api/{canvasName}/get_trace_data
  • POST /api/{canvasName}/get_unlocked_dbs
  • GET /login
  • GET /logout
  • GET /check-apiserver

along with 2 bogus endpoints (GET and POST to completely-fake-url)

Only remaining step on my list is to figure out why BwdServer seemed to require the hacky set-up for HSTS.

@StachuDotNet StachuDotNet marked this pull request as ready for review April 19, 2022 18:02
@StachuDotNet
Copy link
Member Author

@pbiggar ready for a review. I've tried my best to figure out why BwdServer won't work with AddHsts and UseHsts like ApiServer does, but couldn't figure it out. Regardless, it feels like the current implementation may be OK?

@pbiggar
Copy link
Member

pbiggar commented Apr 19, 2022

Could you explain what happens when you try to use HSTS for Bwdserver?

@StachuDotNet
Copy link
Member Author

StachuDotNet commented Apr 19, 2022

Could you explain what happens when you try to use HSTS for Bwdserver?

It seems to have no effect; the header is then not present in all http responses.

I had a theory that configureApp.handler was clearing the header earlier set. I tested this by printfn-ing the results of getHeaders at the line where I'm currently setting the header manually, seeing if the Strict-Transport-Security was there, and it wasn't.

I also tried seeing if testing in a local environment was a relevant factor - after some logging there, I determined it wasn't, but I'm curious if it'd work in prod.

@StachuDotNet
Copy link
Member Author

StachuDotNet commented Apr 20, 2022

If the above description doesn't make sense, it may be worthwhile to briefly Zoom over it? I can alternatively push a commit that will use the UseHsts+AddHsts code instead, resulting in all the related tests failing.

Or, we can just add a CLEANUP/Issue to follow up on later.

@pbiggar
Copy link
Member

pbiggar commented Apr 20, 2022

Ready to merge once conflicts are fixed.

@pbiggar pbiggar merged commit 2eb9a86 into darklang:main Apr 21, 2022
@StachuDotNet StachuDotNet deleted the add-hsts-to-fsharp-backends branch May 13, 2022 21:29
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