Skip to content

Commit

Permalink
Better handling of passwords in init-db
Browse files Browse the repository at this point in the history
  • Loading branch information
bennybp committed May 21, 2024
1 parent b8018e6 commit e8dee45
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 31 deletions.
13 changes: 9 additions & 4 deletions qcfractal/qcfractal/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ class DatabaseConfig(ConfigBase):
description="The port the database is running on. If own = True, a database will be started, binding to this port",
)
database_name: str = Field("qcfractal_default", description="The database name to connect to.")
username: Optional[str] = Field(None, description="The database username to connect with")
password: Optional[str] = Field(None, description="The database password to connect with")
username: str = Field(..., description="The database username to connect with")
password: str = Field(..., description="The database password to connect with")
query: Dict[str, str] = Field({}, description="Extra connection query parameters at the end of the URL string")

own: bool = Field(
Expand Down Expand Up @@ -561,8 +561,13 @@ def write_initial_configuration(file_path: str, full_config: bool = True):
secret_key = secrets.token_urlsafe(32)
jwt_secret_key = secrets.token_urlsafe(32)

db_config = {
"username": "qcfractal",
"password": secrets.token_urlsafe(32),
}

default_config = FractalConfig(
base_folder=base_folder, api={"secret_key": secret_key, "jwt_secret_key": jwt_secret_key}
base_folder=base_folder, api={"secret_key": secret_key, "jwt_secret_key": jwt_secret_key}, database=db_config
)

default_config.database.port = find_open_port(5432)
Expand All @@ -580,7 +585,7 @@ def write_initial_configuration(file_path: str, full_config: bool = True):
"service_frequency": True,
"max_active_services": True,
"heartbeat_frequency": True,
"database": {"own", "host", "port", "database_name", "base_folder"},
"database": {"own", "host", "port", "database_name", "base_folder", "username", "password"},
"api": {"secret_key", "jwt_secret_key", "host", "port"},
}

Expand Down
42 changes: 19 additions & 23 deletions qcfractal/qcfractal/postgres_harness.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import os
import pathlib
import re
import secrets
import shutil
import subprocess
import time
Expand Down Expand Up @@ -123,7 +124,9 @@ def _get_tool(self, tool: str) -> str:
)
return tool_path

def _run_subprocess(self, command: List[str], env: Optional[Dict[str, Any]] = None) -> Tuple[int, str, str]:
def _run_subprocess(
self, command: List[str], env: Optional[Dict[str, Any]] = None, shell: bool = False
) -> Tuple[int, str, str]:
"""
Runs a command using subprocess, and output stdout into the logger
Expand All @@ -143,8 +146,14 @@ def _run_subprocess(self, command: List[str], env: Optional[Dict[str, Any]] = No
if env is not None:
full_env.update(env)

proc = subprocess.run(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=full_env)
self._logger.debug("Running subprocess: " + str(command))

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (password)
as clear text.
if shell:
proc = subprocess.run(
" ".join(command), stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=full_env, shell=True
)
else:
proc = subprocess.run(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=full_env)

stdout = proc.stdout.decode()
stderr = proc.stderr.decode()
if len(stdout) > 0:
Expand Down Expand Up @@ -454,27 +463,19 @@ def postgres_initialized(self):
psql_conf_file = os.path.join(self.config.data_directory, "postgresql.conf")
return os.path.exists(psql_conf_file)

def initialize_postgres(self, auth_method: str = "trust") -> None:
def initialize_postgres(self) -> None:
"""Initializes a postgresql instance and starts it
The data directory and port from the configuration is used for the postgres instance
This does not create the QCFractal database or its tables
Parameters
----------
auth_method
[ADVANCED] The authentication method to use for host and local connections.
The default is "trust" which is insecure but is easy for local installations.
"""

# Can only initialize if we are expected to manage it
assert self.config.own

# auth method must be 'trust' if user/password not given
if auth_method != "trust" and (self.config.username is None or self.config.password is None):
raise RuntimeError("Cannot use auth_method other than 'trust' if username or password are not given")
if not self.config.username or not self.config.password:
raise RuntimeError("Username or password are not given")

self._logger.info("Initializing the Postgresql database")

Expand All @@ -489,22 +490,15 @@ def initialize_postgres(self, auth_method: str = "trust") -> None:

initdb_path = self._get_tool("initdb")

cmd = [initdb_path, "-D", self.config.data_directory, "--auth", auth_method]
cmd = [initdb_path, "-D", self.config.data_directory, "--auth", "scram-sha-256"]
if self.config.username is not None:
cmd += ["--username", self.config.username]

# Initdb requires passwords come from a file
if self.config.password is not None:
pw_file_path = os.path.join(self.config.base_folder, ".initdb_pwfile")
with open(pw_file_path, "w") as pw_tmp:
pw_tmp.write(self.config.password)
cmd += ["--pwfile", pw_file_path]
cmd += [f'--pwfile=<(printf "%s\n" {self.config.password})']

try:
retcode, stdout, stderr = self._run_subprocess(cmd)
finally:
if self.config.password is not None:
os.remove(pw_file_path)
retcode, stdout, stderr = self._run_subprocess(cmd, shell=True)

if retcode != 0 or "Success." not in stdout:
err_msg = f"Error initializing a PostgreSQL instance:\noutput:\n{stdout}\nstderr:\n{stderr}"
Expand Down Expand Up @@ -617,6 +611,8 @@ def create_snowflake_postgres(data_dir: str) -> PostgresHarness:
"base_folder": data_dir,
"own": True,
"host": sock_dir,
"username": "qcfractal_snowflake",
"password": secrets.token_urlsafe(32),
}

config = DatabaseConfig(**db_config)
Expand Down
15 changes: 11 additions & 4 deletions qcfractal/qcfractal/test_db_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,15 @@
"testing_db_maint_1",
{"connect_timeout": 10},
),
("192.168.1.234", 6543, "test_user_1", None, "testing_db_2", "testing_db_maint_2", {"connect_timeout": 10}),
("db.example.com", 9988, None, None, "testing_db_3", "testing_db_maint_3", {"connect_timeout": 10}),
(
"192.168.1.234",
6543,
"test_user_1",
"test_pass_2",
"testing_db_2",
"testing_db_maint_2",
{"connect_timeout": 10},
),
(
"/var/run/postgresql",
9876,
Expand Down Expand Up @@ -142,7 +149,7 @@ def test_db_connection_hosts(tmp_path_factory):
pg_harness = PostgresHarness(db_config)

# Change trust method so we can actually check passwords
pg_harness.initialize_postgres(auth_method="scram-sha-256")
pg_harness.initialize_postgres()
pg_harness.create_database(create_tables=True)
assert pg_harness.can_connect()

Expand Down Expand Up @@ -178,7 +185,7 @@ def test_db_connection_full_uri(tmp_path_factory):
pg_harness = PostgresHarness(db_config)

# Change trust method so we can actually check passwords
pg_harness.initialize_postgres(auth_method="scram-sha-256")
pg_harness.initialize_postgres()
pg_harness.create_database(create_tables=True)
assert pg_harness.can_connect()

Expand Down

0 comments on commit e8dee45

Please sign in to comment.