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 LIVEBOOK_FORCE_SSL env #1064

Merged
merged 8 commits into from
Mar 23, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,12 @@ The following environment variables configure Livebook:
Enabled by default unless `LIVEBOOK_PASSWORD` is set. Set it to "false" to
disable it.

* LIVEBOOK_FORCE_SSL - force SSL connections and enable HSTS.
Ensuring no data is ever sent via http, always redirect to https.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, maybe there was some confusion. We don't need LIVEBOOK_FORCE_SSL anymore, only LIVEBOOK_FORCE_SSL_HOST. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've solved it.

* LIVEBOOK_FORCE_SSL_HOST - set a new host to redirect to if the request's scheme is `http`.
josevalim marked this conversation as resolved.
Show resolved Hide resolved
Defaults to nil.
josevalim marked this conversation as resolved.
Show resolved Hide resolved

<!-- Environment variables -->

If running Livebook as a Docker image or an Elixir release, [the environment
Expand Down
8 changes: 8 additions & 0 deletions lib/livebook.ex
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ defmodule Livebook do
config :livebook, :data_path, data_path
end

if force_ssl = Livebook.Config.force_ssl!("LIVEBOOK_FORCE_SSL") do
config :livebook, :force_ssl, force_ssl
end

if force_ssl_host = Livebook.Config.force_ssl_host!("LIVEBOOK_FORCE_SSL_HOST") do
config :livebook, :force_ssl_host, force_ssl_host
end

config :livebook,
:cookie,
Livebook.Config.cookie!("LIVEBOOK_COOKIE") ||
Expand Down
14 changes: 14 additions & 0 deletions lib/livebook/config.ex
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,20 @@ defmodule Livebook.Config do
System.get_env(env, "1") in ~w(true 1)
end

@doc """
Parses force ssl setting from env.
"""
def force_ssl!(env) do
System.get_env(env, "0") in ~w(true 1)
end

josevalim marked this conversation as resolved.
Show resolved Hide resolved
@doc """
Parses force ssl host setting from env.
"""
def force_ssl_host!(env) do
System.get_env(env)
end

@doc """
Parses and validates default runtime from env.
"""
Expand Down
14 changes: 14 additions & 0 deletions lib/livebook_web/endpoint.ex
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,20 @@ defmodule LivebookWeb.Endpoint do

plug LivebookWeb.Router

plug :force_ssl

def force_ssl(conn, _opts) do
force_ssl = Application.get_env(:livebook, :force_ssl, false)
force_ssl_host = Application.get_env(:livebook, :force_ssl_host, nil)

if force_ssl || force_ssl_host do
plug_ssl = Plug.SSL.init(host: force_ssl_host)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
force_ssl = Application.get_env(:livebook, :force_ssl, false)
force_ssl_host = Application.get_env(:livebook, :force_ssl_host, nil)
if force_ssl || force_ssl_host do
plug_ssl = Plug.SSL.init(host: force_ssl_host)
force_ssl_host = Application.get_env(:livebook, :force_ssl_host, nil)
if force_ssl_host do
plug_ssl = Plug.SSL.init(host: force_ssl_host)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think you can do something like this:

@plug_ssl Plug.SSL.init(host: {Application, :get_env, [:livebook, :force_ssl_host, nil]})

And then skip the init call here. The module attribute approach is likely more efficient, even if it has to look up the host twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should I do with :force_ssl?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is no longer used as we renamed it to force_ssl_host. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it!

Plug.SSL.call(conn, plug_ssl)
else
conn
end
end

def access_struct_url() do
base =
case struct_url() do
Expand Down