-
Notifications
You must be signed in to change notification settings - Fork 417
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
Conversation
lib/livebook.ex
Outdated
@@ -58,6 +58,10 @@ defmodule Livebook do | |||
config :livebook, :data_path, data_path | |||
end | |||
|
|||
if Livebook.Config.force_ssl!("LIVEBOOK_FORCE_SSL") do | |||
Keyword.new() |> Plug.SSL.init() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to store this in config, something like config :livebook, :force_ssl, true | false
. And then, you need a plug in the endpoint.ex like this:
plug :force_ssl
@plug_ssl Plug.SSL.init([])
def force_ssl(conn, _opts) do
if Application.get_env(:livebook, :force_ssl, false) do
conn
else
Plug.SSL.call(conn, @plug_ssl)
end
end
There are probably other details, but this is the minimum required to get started!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think we should call the option LIVEBOOK_FORCE_SSL_HOST
and it should be set to a binary which is the host we redirect to!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think we should call the option
LIVEBOOK_FORCE_SSL_HOST
and it should be set to a binary which is the host we redirect to!
OK, I'll create new PR about this soon.
I truly appreciate your comments :)
Co-authored-by: José Valim <[email protected]>
lib/livebook_web/endpoint.ex
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got it!
README.md
Outdated
* LIVEBOOK_FORCE_SSL - force SSL connections and enable HSTS. | ||
Ensuring no data is ever sent via http, always redirect to https. | ||
|
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've solved it.
💚 💙 💜 💛 ❤️ |
#983
Is that right?