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

fix: DPE-6137 insecure password calls removal #553

Merged
merged 12 commits into from
Jan 24, 2025

Conversation

paulomach
Copy link
Contributor

@paulomach paulomach commented Jan 14, 2025

Port of VM PR#579, plus:

  • mysql.py shared lib update that:
    • disable general query log
    • changes log level for destructive actions (i.e. s/debug/info)

Copy link
Contributor

@carlcsaposs-canonical carlcsaposs-canonical left a comment

Choose a reason for hiding this comment

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

if password:
# password is needed after user
command.insert(3, "-p")
process = self.container.exec(command, timeout=timeout, stdin=password)
Copy link
Contributor

Choose a reason for hiding this comment

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

does password on stdin not work on vm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The (good) surprise is that it works with pebble actually.

Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason it doesn't work on vm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, mysqlcli will read password from console input, not stdin. Hence using pexpect, that will wrap the call in a pseudo tty and emulate console input.

Copy link
Contributor

Choose a reason for hiding this comment

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

but on k8s it reads it from stdin? I'm confused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pebble is doing something else. You can try this, with a running local pebble and mysql:

import subprocess
import pexpect
from ops.pebble import Client

cmd = [
    "mysql",
    "-u",
    "root",
    "-S",
    "/var/snap/charmed-mysql/common/var/run/mysqld/mysqld.sock",
    "-N",
    "-B",
    "-p",
    "-e",
    "Select user,host from mysql.user",
]

c = cmd.copy()
c[-1] = f"\"{c[-1]}\""
child = pexpect.spawnu(" ".join(c))
child.expect("Enter password:")
child.sendline("newpass")
stdout = child.readlines()

print("expect")
re = [line.strip().split() for line in stdout[1:]]


print("\nsubprocess")
stdout = subprocess.check_output(cmd, text=True, input="newpass")
rs = [line.split() for line in stdout.strip().split("\n")]


print("\npebble")
client = Client("/tmp/.pebble.socket")
p = client.exec(cmd, stdin="newpass")
stdout, _ = p.wait_output()
rp = [line.split("\t") for line in  stdout.strip().split("\n")]


print(rp == re)
print(re == rs)

Will output:

(venv) root@noble:~# python test.py
expect

subprocess
Enter password:  # <-- running with subprocess still requires a console input, blocking execution

pebble
True
True

@paulomach
Copy link
Contributor Author

upgrade test for arm consistently failing on main and other branches. Merging as is to unblock other PRs

@paulomach paulomach merged commit 7c6b120 into main Jan 24, 2025
98 of 99 checks passed
@paulomach paulomach deleted the fix/dpe-6137-database_calls branch January 24, 2025 00:25
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.

4 participants