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

chore(database): Add ssl support #777

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
13 changes: 13 additions & 0 deletions .env
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,19 @@ IPBASE_API_KEY=
GOOGLE_GEOLOCATION_API_KEY=
GOOGLE_GEOCODING_API_KEY=

# Wether to use SSL/TLS connection to the database, defaults to `false`.
DATABASE_ENABLE_SSL=false

# Whether to use the host machine certificates or not to verify the connection
# with the database.
# DATABASE_USE_OS_CERTS=true

# The CA certificate file to use to verify the TLS connection to the database.
# DATABASE_SSL_CACERTFILE=./example_cacert.pem

# Whether verify the SSL connection with the database or not.
# DATABASE_SSL_VERIFY=true

# The top level domain of your edgehog instance.
# In case you want to make Edgehog visible in your LAN, consider setting the variable
# to <HOST_IP>.nip.io
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [0.10] - Unreleased
### Added
- Managed OTA operations expose the update target that created them in graphql ([#356](https://github.com/edgehog-device-manager/edgehog/issues/356).
- Ecto SSL configuration is exposed trough `DATABASE_*` environment variables (see [.env](./.env))

## [0.9.3] - Unreleased
### Fixed
Expand Down
23 changes: 13 additions & 10 deletions backend/config/runtime.exs
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,13 @@ end
# any compile-time configuration in here, as it won't be applied.
# The block below contains prod specific runtime configuration.
if config_env() == :prod do
database_username = System.fetch_env!("DATABASE_USERNAME")
database_password = System.fetch_env!("DATABASE_PASSWORD")
database_hostname = System.fetch_env!("DATABASE_HOSTNAME")
database_name = System.fetch_env!("DATABASE_NAME")
database = %{
username: System.fetch_env!("DATABASE_USERNAME"),
password: System.fetch_env!("DATABASE_PASSWORD"),
hostname: System.fetch_env!("DATABASE_HOSTNAME"),
name: System.fetch_env!("DATABASE_NAME"),
ssl: Config.database_ssl_config()
}

# The secret key base is used to sign/encrypt cookies and other secrets.
# A default value is used in config/dev.exs and config/test.exs but you
Expand Down Expand Up @@ -102,13 +105,13 @@ if config_env() == :prod do
forwarder_hostname = System.get_env("EDGEHOG_FORWARDER_HOSTNAME")

config :edgehog, Edgehog.Repo,
# ssl: true,
# socket_options: [:inet6],
username: database_username,
password: database_password,
hostname: database_hostname,
database: database_name,
pool_size: String.to_integer(System.get_env("POOL_SIZE") || "10")
username: database.username,
password: database.password,
hostname: database.hostname,
database: database.name,
pool_size: String.to_integer(System.get_env("POOL_SIZE") || "10"),
ssl: database.ssl

config :edgehog, EdgehogWeb.Endpoint,
http: [
Expand Down
89 changes: 89 additions & 0 deletions backend/lib/edgehog/config.ex
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,30 @@ defmodule Edgehog.Config do
os_env: "ADMIN_JWT_PUBLIC_KEY_PATH",
type: JWTPublicKeyPEMType

@envdoc "Whether edgehog should use a tls connection with the database or not."
app_env :database_enable_ssl, :edgehog, :database_enable_ssl,
os_env: "DATABASE_ENABLE_SSL",
type: :boolean,
default: false

@envdoc "The certificate file to use to verify the ssl connection with the database."
app_env :database_ssl_cacertfile, :edgehog, :database_ssl_cacertfile,
os_env: "DATABASE_SSL_CACERTFILE",
type: :binary,
default: ""

@envdoc "Whether to use the os certificates to communicate with the database over ssl."
app_env :database_use_os_certs, :edgehog, :database_use_os_certs,
os_env: "DATABASE_USE_OS_CERTS",
type: :boolean,
default: false

@envdoc "Whether to verify the ssl connection with the database or not."
app_env :database_ssl_verify, :edgehog, :database_ssl_verify,
os_env: "DATABASE_SSL_VERIFY",
type: :boolean,
default: false

@envdoc """
Disables tenant authentication. CHANGING IT TO TRUE IS GENERALLY A REALLY BAD IDEA IN A PRODUCTION ENVIRONMENT, IF YOU DON'T KNOW WHAT YOU ARE DOING.
"""
Expand Down Expand Up @@ -94,6 +118,71 @@ defmodule Edgehog.Config do
@spec admin_authentication_disabled?() :: boolean()
def admin_authentication_disabled?, do: disable_admin_authentication!()

@doc """
Returns true if edgehog should use an ssl connection with the database.
"""
@spec database_enable_ssl?() :: boolean()
def database_enable_ssl?, do: database_enable_ssl!()

@doc """
Reutrns whether to verify the ssl connection witht he database or not.
"""
@spec database_ssl_verify?() :: boolean()
def database_ssl_verify?, do: database_ssl_verify!()

@doc """
Returns true if edgehog should use the operative system certificates.
"""
@spec database_use_os_certs?() :: boolean()
def database_use_os_certs?, do: database_use_os_certs!()

@doc """
Returns the ssl configuration for the ssl connection with the database.
"""
@spec database_ssl_cert_config() :: {:cacerts, [term()]} | {:cacertfile, binary()}
def database_ssl_cert_config do
use_os_certs = database_use_os_certs?()

certfile = System.get_env("DATABASE_SSL_CACERTFILE")

case {certfile, use_os_certs} do
{nil, false} ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should raise regardless in this condition.
While it is a good idea to handle the situation, given that we can inform the user about the correct environment variables to set, the user may also just want to use :verify_none, in which case it does not need to set them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I should have made this a private function, but this gets called only from line 172, so only if the user explicitly set DATABASE_SSL_VERIFY=true, in that case the configuration requires either a valid certificate or to use the os certificates

raise """
invalid database SSL configuration:
either set DATABASE_USE_OS_CERTS true to use system's certificates
or provide a CA certificate file with DATABASE_SSL_CACERTFILE.
The latter will take precedence.
"""

{nil, true} ->
{:cacerts, :public_key.cacerts_get()}

{file, _} ->
# Assuming `file` is a file path
{:cacertfile, file}
end
end

@doc """
Returns the Ecto configuration for the ssl connection to the database.
"""
@spec database_ssl_config_opts() :: list(term())
def database_ssl_config_opts do
if database_ssl_verify?(),
do: [{:verify, :verify_peer}, database_ssl_cert_config()],
else: [verify: :verify_none]
end

@doc """
Returns the database configuration for the database connection.
"""
@spec database_ssl_config() :: false | list(term())
def database_ssl_config do
if database_enable_ssl?(),
do: database_ssl_config_opts(),
else: false
end

@doc """
Returns true if tenant authentication is disabled.
"""
Expand Down
Loading