diff --git a/app/services/levelcode/provider_oauth.rb b/app/services/levelcode/provider_oauth.rb index d1410769..5a31229a 100644 --- a/app/services/levelcode/provider_oauth.rb +++ b/app/services/levelcode/provider_oauth.rb @@ -23,6 +23,17 @@ module Levelcode module ProviderOAuth module_function + # How long a provider is given before a sign-in gives up on it. Net::HTTP allows sixty seconds for + # each of these by default, and a stalled provider then holds the browser on a blank spinner — + # the one outcome a user cannot recover from alone. A token exchange is a single small request, + # so seconds are enough: failing fast with an error the user can retry beats succeeding on the + # rare slow call. + # + # max_retries is part of the bound, not a detail: Net::HTTP silently retries an idempotent request + # once when it times out, so without this every GET here — the GitHub profile and email reads — + # is allowed twice as long as the numbers beside it say. + HTTP_OPTIONS = { open_timeout: 5, read_timeout: 10, write_timeout: 10, max_retries: 0 }.freeze + GITHUB_PROVIDER = "github" GITHUB_AUTHORIZE_URL = "https://github.com/login/oauth/authorize" @@ -267,12 +278,13 @@ def post_form(url, form, headers: {}) end def perform_http(uri, req) - http = Net::HTTP.new(uri.host, uri.port) - http.use_ssl = (uri.scheme == "https") - res = http.request(req) + res = Net::HTTP.start(uri.host, uri.port, use_ssl: uri.scheme == "https", **HTTP_OPTIONS) do |http| + http.request(req) + end res.is_a?(Net::HTTPSuccess) ? res.body : nil rescue StandardError => e - Rails.logger.warn("Levelcode OAuth HTTP error: #{e.message}") + # Host and path only — a query string is where a code or a token would be. + Rails.logger.warn("Levelcode OAuth HTTP error: #{e.class}: #{e.message} (#{req.method} #{uri.host}#{uri.path})") nil end diff --git a/config/application.rb b/config/application.rb index e495405e..5dfdadfe 100644 --- a/config/application.rb +++ b/config/application.rb @@ -20,9 +20,37 @@ module Backend class Application < Rails::Application + # What ActionDispatch::SSL is given in production, where config/environments/production.rb turns + # it on. Held here as a value so the specs can run the real middleware with the real options. + # + # redirect: false — the ALB already redirects http to https, and its health check reaches /up over + # PLAIN HTTP and accepts only a 200 (.ebextensions/03_healthcheck.config). A redirect there marks + # every instance unhealthy and takes the site down. + # + # hsts — deliberately short. A browser honours it for the full max-age and it cannot be withdrawn + # early. Raise `expires` to 1.year once a week has passed clean, and only then consider + # `subdomains: true`, which commits every present and future subdomain to HTTPS at once. + SSL_OPTIONS = { + redirect: false, + hsts: { expires: 1.week, subdomains: false, preload: false } + }.freeze + # Use cookies for session store config.middleware.use ActionDispatch::Cookies - config.middleware.use ActionDispatch::Session::CookieStore, key: "_your_app_session" + # `secure` is set HERE, on the store, because nothing else will set it: with its redirect off, + # ActionDispatch::SSL does not flag cookies Secure — Rails skips that for any request it would not + # have redirected. Production only: unconditionally, the cookie would never be sent over + # http://localhost, and neither development nor the specs could hold a session. + # + # same_site MUST STAY :lax. It is the Rails default, so it is stated explicitly to stop anyone + # "hardening" it to :strict — the Google OAuth callback is a cross-site top-level GET, :strict + # withholds the cookie on exactly that request, and the sign-in then dies with `session_expired` + # because the `state` stashed in the session never comes back. See Levelcode::WebController. + config.middleware.use ActionDispatch::Session::CookieStore, + key: "_your_app_session", + secure: Rails.env.production?, + same_site: :lax, + httponly: true config.api_only = true # Initialize configuration defaults for originally generated Rails version. diff --git a/config/environments/production.rb b/config/environments/production.rb index 347d11dd..cd0436ec 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -48,12 +48,11 @@ # Can be used together with config.force_ssl for Strict-Transport-Security and secure cookies. config.assume_ssl = true - # Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies. - # Disabled for CloudFront - CloudFront handles SSL termination - config.force_ssl = false - - # Skip http-to-https redirect for the default health check endpoint. - # config.ssl_options = { redirect: { exclude: ->(request) { request.path == "/up" } } } + # Strict-Transport-Security, and deliberately NOT the http->https redirect: the ALB health check + # reaches /up over plain HTTP and accepts only a 200. The options, and the reason for each, are + # SSL_OPTIONS in config/application.rb — kept there so the specs can run the real middleware. + config.force_ssl = true + config.ssl_options = Backend::Application::SSL_OPTIONS # Log to STDOUT by default config.logger = ActiveSupport::Logger.new(STDOUT) diff --git a/spec/config/ssl_config_spec.rb b/spec/config/ssl_config_spec.rb new file mode 100644 index 00000000..6730256a --- /dev/null +++ b/spec/config/ssl_config_spec.rb @@ -0,0 +1,93 @@ +require "rails_helper" + +# Production sits behind an ALB that terminates TLS and talks to the instance over plain HTTP. So +# ActionDispatch::SSL is on for one job — Strict-Transport-Security — and must never do its other +# one, the http->https redirect: the ALB health check accepts nothing but a 200 from /up, and a +# redirect there marks every instance unhealthy. +# +# These run the real middleware with the options production is given, so they describe what +# production does rather than what its config file looks like. +RSpec.describe "transport security in production" do + let(:app) { ->(_env) { [ 200, { "set-cookie" => "id=1; path=/" }, [ "ok" ] ] } } + let(:options) { Backend::Application::SSL_OPTIONS } + + def get(url, **overrides) + Rack::MockRequest.new(ActionDispatch::SSL.new(app, **options, **overrides)).get(url) + end + + describe "the load balancer's health check" do + it "gets its 200 from /up over plain HTTP, never a redirect" do + response = get("http://levelcode.ai/up") + + expect(response.status).to eq(200) + expect(response.location).to be_nil + end + + # The same request with the redirect left on. This is the outage — and the reason the example + # above cannot be passing by accident. + it "would be redirected if the redirect were ever left on" do + response = get("http://levelcode.ai/up", redirect: {}) + + expect(response.status).to eq(301) + expect(response.location).to eq("https://levelcode.ai/up") + end + + it "still accepts nothing but a 200 from /up" do + health_check = YAML.load_file(Rails.root.join(".ebextensions/03_healthcheck.config")) + .dig("option_settings", "aws:elasticbeanstalk:environment:process:default") + + expect(health_check).to include("HealthCheckPath" => "/up", "MatcherHTTPCode" => "200") + end + end + + describe "Strict-Transport-Security" do + # One assertion on the whole header: a longer max-age, includeSubDomains and preload would each + # change it, and each is a commitment a browser holds us to with no way to withdraw it early. + it "is sent over HTTPS for one week, for this host only" do + expect(get("https://levelcode.ai/").headers["strict-transport-security"]).to eq("max-age=604800") + end + end + + describe "the session cookie" do + let(:session_options) do + Rails.application.middleware.find { |m| m.klass == ActionDispatch::Session::CookieStore }.args.first + end + + # With its redirect off, ActionDispatch::SSL leaves cookies alone: Rails only flags them Secure + # on requests it would otherwise have redirected. So the store has to carry the flag itself. + it "is not made Secure by the middleware, which is why the store sets its own flag" do + expect(get("https://levelcode.ai/").headers["set-cookie"]).to eq("id=1; path=/") + end + + # :strict withholds the cookie on a cross-site top-level GET, which is exactly what a provider's + # redirect back to /ai/auth/callback is. The `state` stashed in the session would not come back, + # and every sign-in would fail with `session_expired`. + it "is HttpOnly, and SameSite=Lax rather than Strict" do + expect(session_options).to include(httponly: true, same_site: :lax) + end + + it "is not Secure outside production, or it would never be sent over http://localhost" do + expect(session_options).to include(secure: false) + end + end + + # Two things are decided only when Rails boots in production, so nothing above can observe them. + # They are read as whole lines of code: a comment that mentions a setting is not a match. + describe "what only a production boot evaluates" do + def lines_of(path) + Rails.root.join(path).readlines.map(&:strip) + end + + it "turns the middleware on, behind the proxy, with these options" do + expect(lines_of("config/environments/production.rb")).to include( + "config.assume_ssl = true", + "config.force_ssl = true", + "config.ssl_options = Backend::Application::SSL_OPTIONS" + ) + end + + it "marks the session cookie Secure" do + expect(lines_of("config/application.rb")).to include("secure: Rails.env.production?,") + end + end +end diff --git a/spec/requests/api/levelcode/v1/auth_spec.rb b/spec/requests/api/levelcode/v1/auth_spec.rb index 9faf3d49..a59080a4 100644 --- a/spec/requests/api/levelcode/v1/auth_spec.rb +++ b/spec/requests/api/levelcode/v1/auth_spec.rb @@ -304,8 +304,7 @@ def json emails_res = instance_double(Net::HTTPOK, body: [ { email: 'octo@example.com', primary: true, verified: true } ].to_json) allow(emails_res).to receive(:is_a?).with(Net::HTTPSuccess).and_return(true) http = instance_double(Net::HTTP) - allow(Net::HTTP).to receive(:new).and_return(http) - allow(http).to receive(:use_ssl=) + allow(Net::HTTP).to receive(:start) { |*, &session| session.call(http) } allow(http).to receive(:request) do |req| case req.path when '/login/oauth/access_token' then token_res diff --git a/spec/services/levelcode/provider_oauth_spec.rb b/spec/services/levelcode/provider_oauth_spec.rb index e45a4347..b276a492 100644 --- a/spec/services/levelcode/provider_oauth_spec.rb +++ b/spec/services/levelcode/provider_oauth_spec.rb @@ -116,4 +116,104 @@ def sign_in_with_google expect(Rails.logger).to have_received(:error).with(/google sign-up rejected by validation/i) end end + + # perform_http carries the GitHub token exchange and API reads, and the Google token exchange. + # (Google's signing keys are fetched by the googleauth gem itself, not through here.) These run it + # against a real local socket rather than a double, so they describe what a provider would see and + # hold however the Net::HTTP call happens to be written. + describe "talking to a provider" do + # A local stand-in for a provider. Given a response it answers each request with it; given none + # it accepts the connection and never answers — the failure the timeouts exist for. + def with_provider(response = nil) + server = TCPServer.new("127.0.0.1", 0) + connections = [] + listener = Thread.new do + loop do + socket = server.accept + connections << socket + next unless response + + socket.gets("\r\n\r\n") # the request head — the answered examples send no body + socket.write(response) + socket.close + end + end + yield URI("http://127.0.0.1:#{server.addr[1]}/token"), connections + ensure + listener&.kill + connections&.each { |socket| socket.close unless socket.closed? } + server&.close + end + + def http_response(status, body) + "HTTP/1.1 #{status}\r\nContent-Length: #{body.bytesize}\r\nConnection: close\r\n\r\n#{body}" + end + + def perform(uri, request) + described_class.send(:perform_http, uri, request) + end + + # A stall should cost the suite a fraction of a second, not the ten a real sign-in is allowed. + def with_read_timeout(seconds) + stub_const("#{described_class}::HTTP_OPTIONS", described_class::HTTP_OPTIONS.merge(read_timeout: seconds)) + end + + it "returns the body of a successful response" do + with_provider(http_response("200 OK", '{"access_token":"t"}')) do |uri| + expect(perform(uri, Net::HTTP::Get.new(uri))).to eq('{"access_token":"t"}') + end + end + + it "returns nil for any other status, so the caller can fail the sign-in" do + with_provider(http_response("503 Service Unavailable", "busy")) do |uri| + expect(perform(uri, Net::HTTP::Get.new(uri))).to be_nil + end + end + + # nil rather than an exception is what sends the callback to /ai/login?error=oauth_failed — an + # error the user can retry — instead of a 500. The clock is what proves the timeout was applied: + # without one, this still returns nil, after Net::HTTP's default sixty seconds. + it "gives up on a provider that accepts the connection and never answers" do + with_read_timeout(0.2) + with_provider do |uri| + request = Net::HTTP::Post.new(uri).tap { |post| post.set_form_data(code: "c") } + started = Process.clock_gettime(Process::CLOCK_MONOTONIC) + + expect(perform(uri, request)).to be_nil + expect(Process.clock_gettime(Process::CLOCK_MONOTONIC) - started).to be < 5 + end + end + + # Net::HTTP retries an idempotent request once after a timeout, on a fresh connection. Left on, + # the GitHub profile and email reads are each allowed double the timeout — which is how a bound + # advertised as fifteen seconds a call quietly becomes thirty. + it "does not retry a GET that timed out" do + with_read_timeout(0.2) + with_provider do |uri, connections| + expect(perform(uri, Net::HTTP::Get.new(uri))).to be_nil + expect(connections.size).to eq(1) + end + end + + # A timeout is now the likeliest way for this line to fire, and its message alone names neither + # the provider nor the call. + it "logs what failed and where: the exception, the method, the host and path — never the query" do + allow(Rails.logger).to receive(:warn) + with_read_timeout(0.2) + with_provider do |uri| + uri.query = "code=secret" + perform(uri, Net::HTTP::Get.new(uri)) + end + + expect(Rails.logger).to have_received(:warn).with( + a_string_matching(%r{\ALevelcode OAuth HTTP error: Net::ReadTimeout: .* \(GET 127\.0\.0\.1/token\)\z}) + ) + end + + # Changing one of these is a decision about how long a person waits on a spinner. This makes it + # a visible one. + it "allows a call five seconds to connect and ten to answer, once" do + expect(described_class::HTTP_OPTIONS).to eq(open_timeout: 5, read_timeout: 10, write_timeout: 10, max_retries: 0) + end + end end