Skip to content

[DPE-7641] Authentication #8

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

[DPE-7641] Authentication #8

wants to merge 21 commits into from

Conversation

Zvirovyi
Copy link
Collaborator

@Zvirovyi Zvirovyi commented Jul 3, 2025

Overview

This PR:

  1. Splits config definition in corresponding manager ([DPE-7476] Basic charm structure #1 (comment)).
  2. Extends start logic to configure password for system cassandra user. Showcase diagram of changes is presented below.
  3. Migrates Cassandra client to new database manager ([DPE-7476] Basic charm structure #1 (comment)).

Diagram of startup logic changes

Diagram describes on_start event handling represented from the leader perspective as there's no changes for the subordinate units.

image

@Zvirovyi Zvirovyi marked this pull request as draft July 7, 2025 09:15
@Zvirovyi Zvirovyi changed the base branch from multinode to main July 16, 2025 10:49
@Zvirovyi Zvirovyi marked this pull request as ready for review July 16, 2025 11:28
@Zvirovyi Zvirovyi requested a review from deusebio July 16, 2025 11:32
Copy link

@welpaolo welpaolo left a comment

Choose a reason for hiding this comment

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

Good starting point. I thought that this PR was also including the handling of system user password with secrets like here. Will this be part of another PR ?

@Zvirovyi
Copy link
Collaborator Author

Good starting point. I thought that this PR was also including the handling of system user password with secrets like here. Will this be part of another PR ?

Did I understood it correctly, that this question is about secret changed event? I thought I'll put it in another PR, but considering that this PR is relatively small, I'll include it here. Thanks!

@Zvirovyi Zvirovyi marked this pull request as draft July 29, 2025 09:06
@Zvirovyi Zvirovyi marked this pull request as ready for review July 31, 2025 12:18
@Zvirovyi Zvirovyi requested review from welpaolo, deusebio and marcoppenheimer and removed request for welpaolo, deusebio and marcoppenheimer August 1, 2025 12:05
Copy link
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

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

Let's just discuss about the authenitcation flow, to be on the same page. I'll continue with the review after the discussion

seeds=["127.0.0.1:7000"],
authentication=False,
)
self.workload.start()
Copy link
Contributor

Choose a reason for hiding this comment

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

question If I understand the logic correctly, here we are starting the server with no authentication. This seems weird and it could pose a security threat. I understand that we are possibly restricting the listen_address into localhost, but why do we need to do this?

Another possibility could also be to start the server using a temporary username and password that only the units knows, such that it can still use the service in authenticated fashion, and then change the password to use the "official" one.

anyhow, let's discuss later at daily

keystore_password=self.state.unit.keystore_password,
truststore_password=self.state.unit.truststore_password,
authentication=bool(self.state.cluster.cassandra_password_secret),
Copy link
Contributor

Choose a reason for hiding this comment

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

quesiton why do we allow this? Maybe I would not expose this, and only allow authentication to be true, just to prevent mistakes. We must ensure that authentication is enabled at all times, if external access is possible.

yaml.dump(config, allow_unicode=True, default_flow_style=False)
yaml.dump(
self._merge_dicts(
[
Copy link
Contributor

Choose a reason for hiding this comment

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

praise I really really really like how you splitted the config into separate section. It is a lot more maintanable IMHO

self.database_manager.update_system_user_password(
"cassandra", self.state.cluster.cassandra_password_secret
)
self.cluster_manager.prepare_shutdown()
Copy link
Contributor

Choose a reason for hiding this comment

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

question why do we drain before reconfiguring the password? The drai operation may be massive?

):
self.database_manager.update_system_user_password("cassandra", password)
self.state.cluster.cassandra_password_secret = password
self.cluster_manager.prepare_shutdown()
Copy link
Contributor

Choose a reason for hiding this comment

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

issue it is strange that we need to drain for every config change/rotation password

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants