Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
fe8bd8b
Change charm database user
marceloneppel Aug 16, 2022
dd395f5
Fix tests user
marceloneppel Aug 16, 2022
d325da6
Add password rotation action
marceloneppel Aug 17, 2022
4cf20fd
Merge branch 'main' into password-rotation
marceloneppel Aug 17, 2022
ca8f525
Merge branch 'main' into password-rotation
marceloneppel Aug 22, 2022
fc17a5e
Rework secrets management
marceloneppel Aug 23, 2022
05b707e
Add unit tests
marceloneppel Aug 23, 2022
36640b2
Add constants
marceloneppel Aug 23, 2022
e345432
Merge branch 'rework-secrets' into password-rotation
marceloneppel Aug 23, 2022
4735754
Update part of the code to correct actions
marceloneppel Aug 23, 2022
b8a3fd2
Merge branch 'main' into password-rotation
marceloneppel Aug 24, 2022
f7024e4
Implement password rotation
marceloneppel Aug 24, 2022
9c9b211
Improve messages
marceloneppel Aug 24, 2022
4b7a534
Improve get password test
marceloneppel Aug 24, 2022
c92fdbc
Fix comment and remove unneeded else block
marceloneppel Aug 24, 2022
0d7ea40
Add set password unit test
marceloneppel Aug 24, 2022
71f86ac
Improve unit test
marceloneppel Aug 24, 2022
a728a20
Change helper function
marceloneppel Aug 25, 2022
be497bc
Add copyright notice
marceloneppel Aug 25, 2022
ef2bd31
Improve username retrieval
marceloneppel Aug 25, 2022
8988aa3
Add initial code for password rotation test
marceloneppel Aug 25, 2022
7c75e45
Fix comment
marceloneppel Aug 25, 2022
34421e5
Add test checks
marceloneppel Aug 25, 2022
4bc38b8
Small fix
marceloneppel Aug 25, 2022
14ec0f0
Add retry to helper function
marceloneppel Aug 25, 2022
16dce63
Fix lint problems
marceloneppel Aug 25, 2022
b6f2d8a
Improve integration test
marceloneppel Aug 25, 2022
db8dbd8
Add pytest mark
marceloneppel Aug 26, 2022
d063f7d
Merge branch 'main' into password-rotation
marceloneppel Sep 1, 2022
4be4ed0
Add password parameter to the action
marceloneppel Sep 1, 2022
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
22 changes: 19 additions & 3 deletions actions.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,22 @@

get-primary:
description: Get the unit with is the primary/leader in the replication.
get-operator-password:
description: Get the operator user password used by charm.
It is internal charm user, SHOULD NOT be used by applications.
get-password:
description: Change the system user's password, which is used by charm.
It is for internal charm users and SHOULD NOT be used by applications.
params:
username:
type: string
description: The username, the default value 'operator'.
Possible values - operator, replication.
set-password:
description: Change the system user's password, which is used by charm.
It is for internal charm users and SHOULD NOT be used by applications.
params:
username:
type: string
description: The username, the default value 'operator'.
Possible values - operator, replication.
password:
type: string
description: The password will be auto-generated if this option is not specified.
30 changes: 29 additions & 1 deletion lib/charms/postgresql_k8s/v0/postgresql.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 1
LIBPATCH = 2


logger = logging.getLogger(__name__)
Expand All @@ -53,6 +53,10 @@ class PostgreSQLGetPostgreSQLVersionError(Exception):
"""Exception raised when retrieving PostgreSQL version fails."""


class PostgreSQLUpdateUserPasswordError(Exception):
"""Exception raised when updating a user password fails."""


class PostgreSQL:
"""Class to encapsulate all operations related to interacting with PostgreSQL instance."""

Expand Down Expand Up @@ -173,3 +177,27 @@ def get_postgresql_version(self) -> str:
except psycopg2.Error as e:
logger.error(f"Failed to get PostgreSQL version: {e}")
raise PostgreSQLGetPostgreSQLVersionError()

def update_user_password(self, username: str, password: str) -> None:
"""Update a user password.

Args:
username: the user to update the password.
password: the new password for the user.

Raises:
PostgreSQLUpdateUserPasswordError if the password couldn't be changed.
"""
try:
with self._connect_to_database() as connection, connection.cursor() as cursor:
cursor.execute(
sql.SQL("ALTER USER {} WITH ENCRYPTED PASSWORD '" + password + "';").format(
sql.Identifier(username)
)
)
except psycopg2.Error as e:
logger.error(f"Failed to update user password: {e}")
raise PostgreSQLUpdateUserPasswordError()
finally:
if connection is not None:
connection.close()
107 changes: 86 additions & 21 deletions src/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
import logging
from typing import Dict, List, Optional

from charms.postgresql_k8s.v0.postgresql import PostgreSQL
from charms.postgresql_k8s.v0.postgresql import (
PostgreSQL,
PostgreSQLUpdateUserPasswordError,
)
from lightkube import ApiError, Client, codecs
from lightkube.resources.core_v1 import Endpoints, Pod, Service
from ops.charm import (
Expand All @@ -30,7 +33,14 @@
from requests import ConnectionError
from tenacity import RetryError

from constants import PEER, REPLICATION_PASSWORD_KEY, USER, USER_PASSWORD_KEY
from constants import (
PEER,
REPLICATION_PASSWORD_KEY,
REPLICATION_USER,
SYSTEM_USERS,
USER,
USER_PASSWORD_KEY,
)
from patroni import NotReadyError, Patroni
from relations.db import DbProvides
from relations.postgresql_provider import PostgreSQLProvider
Expand Down Expand Up @@ -59,9 +69,8 @@ def __init__(self, *args):
self.framework.observe(self.on.postgresql_pebble_ready, self._on_postgresql_pebble_ready)
self.framework.observe(self.on.stop, self._on_stop)
self.framework.observe(self.on.upgrade_charm, self._on_upgrade_charm)
self.framework.observe(
self.on.get_operator_password_action, self._on_get_operator_password
)
self.framework.observe(self.on.get_password_action, self._on_get_password)
self.framework.observe(self.on.set_password_action, self._on_set_password)
self.framework.observe(self.on.get_primary_action, self._on_get_primary)
self.framework.observe(self.on.update_status, self._on_update_status)
self._storage_path = self.meta.storages["pgdata"].location
Expand Down Expand Up @@ -118,7 +127,7 @@ def postgresql(self) -> PostgreSQL:
return PostgreSQL(
host=self.primary_endpoint,
user=USER,
password=self._get_operator_password(),
password=self._get_secret("app", f"{USER}-password"),
database="postgres",
)

Expand Down Expand Up @@ -426,9 +435,74 @@ def _create_resources(self) -> None:
self.unit.status = BlockedStatus(f"failed to create services {e}")
return

def _on_get_operator_password(self, event: ActionEvent) -> None:
"""Returns the password for the operator user as an action response."""
event.set_results({USER_PASSWORD_KEY: self._get_operator_password()})
def _on_get_password(self, event: ActionEvent) -> None:
"""Returns the password for a user as an action response.

If no user is provided, the password of the operator user is returned.
"""
username = event.params.get("username", USER)
if username not in SYSTEM_USERS:
event.fail(
f"The action can be run only for users used by the charm or Patroni:"
f" {', '.join(SYSTEM_USERS)} not {username}"
)
return
event.set_results(
{f"{username}-password": self._get_secret("app", f"{username}-password")}
)

def _on_set_password(self, event: ActionEvent) -> None:
"""Set the password for the specified user."""
# Only leader can write the new password into peer relation.
if not self.unit.is_leader():
event.fail("The action can be run only on leader unit")
return

username = event.params.get("username", USER)
if username not in SYSTEM_USERS:
event.fail(
f"The action can be run only for users used by the charm:"
f" {', '.join(SYSTEM_USERS)} not {username}"
)
return

password = new_password()
if "password" in event.params:
password = event.params["password"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth obfuscating this somehow before sending it to postgres?

Copy link
Member Author

Choose a reason for hiding this comment

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

One thing that can improve the security here is TLS, that should encrypt the connection between the client (the charm) and PostgreSQL. Checking PostgreSQL documentation it seems that if we configure it correctly it is ok to send some sensitive data like passwords.

Copy link
Contributor

Choose a reason for hiding this comment

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

the set_password action has no password params defined

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Fixed on 4be4ed0.


if password == self._get_secret("app", f"{username}-password"):
event.log("The old and new passwords are equal.")
event.set_results({f"{username}-password": password})
return

# Ensure all members are ready before trying to reload Patroni
# configuration to avoid errors (like the API not responding in
# one instance because PostgreSQL and/or Patroni are not ready).
if not self._patroni.are_all_members_ready():
event.fail(
"Failed changing the password: Not all members healthy or finished initial sync."
)
return

# Update the password in the PostgreSQL instance.
try:
self.postgresql.update_user_password(username, password)
except PostgreSQLUpdateUserPasswordError as e:
logger.exception(e)
event.fail(
"Failed changing the password: Not all members healthy or finished initial sync."
)
return

# Update the password in the secret store.
self._set_secret("app", f"{username}-password", password)

# Update and reload Patroni configuration in this unit to use the new password.
# Other units Patroni configuration will be reloaded in the peer relation changed event.
self._patroni.render_patroni_yml_file()
self._patroni.reload_patroni_configuration()

event.set_results({f"{username}-password": password})

def _on_get_primary(self, event: ActionEvent) -> None:
"""Get primary instance."""
Expand Down Expand Up @@ -495,6 +569,8 @@ def _patroni(self):
self._namespace,
self.app.planned_units(),
self._storage_path,
self._get_secret("app", USER_PASSWORD_KEY),
self._get_secret("app", REPLICATION_PASSWORD_KEY),
)

@property
Expand Down Expand Up @@ -555,10 +631,8 @@ def _postgresql_layer(self) -> Layer:
"PATRONI_KUBERNETES_USE_ENDPOINTS": "true",
"PATRONI_NAME": pod_name,
"PATRONI_SCOPE": f"patroni-{self._name}",
"PATRONI_REPLICATION_USERNAME": "replication",
"PATRONI_REPLICATION_PASSWORD": self._replication_password,
"PATRONI_REPLICATION_USERNAME": REPLICATION_USER,
"PATRONI_SUPERUSER_USERNAME": USER,
"PATRONI_SUPERUSER_PASSWORD": self._get_operator_password(),
},
}
},
Expand All @@ -575,15 +649,6 @@ def _peers(self) -> Relation:
"""
return self.model.get_relation(PEER)

def _get_operator_password(self) -> str:
"""Get operator user password."""
return self._get_secret("app", USER_PASSWORD_KEY)

@property
def _replication_password(self) -> str:
"""Get replication user password."""
return self._get_secret("app", REPLICATION_PASSWORD_KEY)

def _unit_name_to_pod_name(self, unit_name: str) -> str:
"""Converts unit name to pod name.

Expand Down
3 changes: 3 additions & 0 deletions src/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@

DATABASE_PORT = "5432"
PEER = "database-peers"
REPLICATION_USER = "replication"
REPLICATION_PASSWORD_KEY = "replication-password"
USER = "operator"
USER_PASSWORD_KEY = "operator-password"
# List of system usernames needed for correct work of the charm/workload.
SYSTEM_USERS = [REPLICATION_USER, USER]
13 changes: 12 additions & 1 deletion src/patroni.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,22 @@ class Patroni:
"""This class handles the communication with Patroni API and configuration files."""

def __init__(
self, endpoint: str, endpoints: List[str], namespace: str, planned_units, storage_path: str
self,
endpoint: str,
endpoints: List[str],
namespace: str,
planned_units,
storage_path: str,
superuser_password: str,
replication_password: str,
):
self._endpoint = endpoint
self._endpoints = endpoints
self._namespace = namespace
self._storage_path = storage_path
self._planned_units = planned_units
self._superuser_password = superuser_password
self._replication_password = replication_password

def get_primary(self, unit_name_pattern=False) -> str:
"""Get primary instance.
Expand Down Expand Up @@ -137,6 +146,8 @@ def render_patroni_yml_file(self) -> None:
endpoints=self._endpoints,
namespace=self._namespace,
storage_path=self._storage_path,
superuser_password=self._superuser_password,
replication_password=self._replication_password,
)
self._render_file(f"{self._storage_path}/patroni.yml", rendered, 0o644)

Expand Down
5 changes: 5 additions & 0 deletions templates/patroni.yml.j2
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,9 @@ postgresql:
{%- for endpoint in endpoints %}
- host replication replication {{ endpoint }}.{{ namespace }}.svc.cluster.local md5
{%- endfor %}
authentication:
replication:
password: {{ replication_password }}
superuser:
password: {{ superuser_password }}
use_endpoints: true
Loading