feat: det deploy local generates a password for you [DET-10197] #9518

jesse-amano-hpe · 2024-06-13T18:28:49Z

Ticket

Description

BREAKING CHANGE: if an initial user password is not specified, the det deploy local command generates one automatically; if that random password is recognized as the correct one for determined, it is printed so users can log in with it and change their password. Otherwise, it is assumed that this is a relaunch of a cluster with an existing database.

The proposal for this change was first described here: https://hpe-aiatscale.slack.com/archives/CLCE8D998/p1716571996243779?thread_ts=1716404697.160779&cid=CLCE8D998

Additional notes

det deploy aws and det deploy gcp can rely on Terraform state to determine whether a database (or database image) already contains passwords, and prompt interactively if not, and helm can run some simple checks during templating. However, det deploy local sets up a database in a docker container based on a volume that may or may not be empty, and it's hard to know migration states etc. until master is healthy. Rather than rely on master to decide in this case (which has caused problems in the past), this solution guarantees there will always be a password set on cluster creation, with the only major downside being that the admin user is still responsible for changing their password if the deployment logs are somehow exposed long-term.

Test Plan

I've manually tested and added integration tests, but it is probably worth triple-checking that the following is still true on next release:

det deploy local master-up --delete-db prompts for the user to enter an initial password, displays requirements if the first attempt is blank or weak, and falls back to displaying a generated password if the user can't set this correctly.
det deploy local master-up (with an existing database volume) allows login with whatever passwords were set before and does not print the generated password message
det deploy local master-up --delete-db --initial-user-password=$PASSWORD sets up the provided passwords and does not print the generated password message
time det deploy local master-up --det-version='203370a6cdb048aaa579c7406f584bb428e76df4' --master-port=8080 --delete-db --initial-user-password '' & (runs detached in background) skips prompting for a password due to the detached TTY, and displays the generated password (might need to fg the job to see it) in under 2 minutes (should not wait ~10 minutes for an overall task timeout)

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

codecov · 2024-06-13T18:28:55Z

Codecov Report

Attention: Patch coverage is 16.32653% with 41 lines in your changes missing coverage. Please review.

Project coverage is 49.82%. Comparing base (f9a5dd5) to head (633f529).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9518      +/-   ##
==========================================
- Coverage   49.83%   49.82%   -0.02%     
==========================================
  Files        1246     1246              
  Lines      162111   162159      +48     
  Branches     2887     2887              
==========================================
+ Hits        80791    80792       +1     
- Misses      81149    81196      +47     
  Partials      171      171

Flag	Coverage Δ
backend	`43.89% <ø> (-0.02%)`	⬇️
harness	`63.74% <16.32%> (-0.06%)`	⬇️
web	`46.20% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
harness/determined/deploy/local/cluster_utils.py	`18.87% <16.32%> (-0.82%)`	⬇️

... and 4 files with indirect coverage changes

netlify · 2024-06-13T18:29:07Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`633f529`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/667493cc3b6b4b00089c61f1

hamidzr

thanks for working on this. one comment:

hamidzr · 2024-06-14T17:09:34Z

harness/determined/deploy/local/cluster_utils.py

@@ -172,6 +176,10 @@ def master_up(
    if initial_user_password is not None:
        master_conf["security"]["initial_user_password"] = initial_user_password
        make_temp_conf = True
+    elif master_conf["security"].get("initial_user_password") is None:
+        generated_user_password = secrets.token_urlsafe(16)


what chars does this include? can we make sure it's can be picked up with a double click? eg - would break that flow. also maybe we printout what they'd use to create a user

``
det -u admin u create USERNAME --password $GENRATEDPWD`

to make it easier

actually, why not prompt the user for a pwd if it's an interactive shell session? they can confirm to keep/get the autogenerated version or provide a new one

Oh, good point -- I'll try extending this flow a little bit. I think it should still print the autogenerated password in case this is run with a closed stdin (i.e. non-interactively) or the user hits Ctrl+C or something, but prompting to change the password immediately would likely be helpful in the majority of cases. Thanks!
I'll also aim to make this something that can't generate hyphens.

this would be great to include as additional cases to your e2e test!

tara-det-ai · 2024-06-14T18:05:44Z

docs/release-notes/deploy-local-auto-password.rst

+-  CLI: If no user accounts have already been created, and neither
+   ``security.initial_user_password`` in master.yaml nor ``--initial-user-password`` is present when
+   running ``det deploy local`` with the ``master-up`` or ``cluster-up`` commands, an initial
+   password will be generated and displayed to the user.


not sure if this is what is intended:

:orphan:

Security Fixes

CLI: If no user accounts have already been created, and neither
security.initial_user_password in master.yaml nor the --initial-user-password is specified when
running det deploy local with the master-up or cluster-up commands, an initial
password will be generated and displayed to the user.

Would you mind clarifying what the proposed change is? Just the addition of the word "the" or was there something else I missed? I would probably say "the --initial-user-password CLI flag" in that case, to be extra clear.

tara-det-ai

suggested edits

jesse-amano-hpe · 2024-06-17T17:14:50Z

@hamidzr I added a prompt, with the generated password still being printed as a fallback if the user can't answer the prompt correctly. Ready for re-review, I think.

dannysauer · 2024-06-19T13:01:55Z

harness/determined/deploy/local/cluster_utils.py

+    except ValueError:
+        random_password_characters = string.ascii_uppercase + string.ascii_lowercase + string.digits
+        generated_user_password = "".join(
+            [secrets.choice(random_password_characters) for _ in range(16)]


This isn't guaranteed to select a password which meets the security requirements. It could generate sixteen lowercase letters, for example.

The way I've done this in the past is to select one from each set, fill the rest of the length from the combined character set, then shuffle the list by swapping each element with a random value from 0-len before joining.

That may not matter for local deployments, but I figured it was worth mentioning.

That's a good point -- I'll switch it to that. Thanks.

tara-det-ai · 2024-06-20T14:06:43Z

docs/release-notes/deploy-local-auto-password.rst

+-  CLI: If no user accounts have already been created, and neither
+   ``security.initial_user_password`` in master.yaml nor the ``--initial-user-password`` CLI flag is
+   present when running ``det deploy local`` with the ``master-up`` or ``cluster-up`` commands, an
+   initial password will be generated and displayed to the user.


@jesse-amano-hpe does the system display the option to change the auto generated password? Also, we should probably clarify this is "when deploying locally" (my fault)....

e.g.,

:orphan:

Security Fixes

CLI: When deploying locally using det deploy local with master-up or cluster-up commands and no user accounts have been created yet, an initial password will be automatically generated and shown to the user (with the option to change it) if neither security.initial_user_password in master.yaml nor the --initial-user-password CLI flag is
present.

carolinaecalderon

generally looks good, but I want to defer approval to @hamidzr since he initially reviewed the PR

carolinaecalderon · 2024-06-20T18:17:22Z

e2e_tests/tests/deploy/test_local.py

+        Resource.MASTER,
+        master_name,
+        {"initial-user-password": conf.USER_PASSWORD},
+        ["delete-db"],


what does "delete-db" command have to do with this feature? is this a stray change?

It's because an existing database could come with existing user/password settings (including those generated by other tests and not known to this test), which would break the tests. This gets us a clean slate.

carolinaecalderon · 2024-06-20T18:18:45Z

e2e_tests/tests/deploy/test_local.py

+def test_master_up_interactive_password() -> None:
+    cluster_name = "test_master_up_interactive_password"
+    master_name = f"{cluster_name}_determined-master_1"
+    password_input = "XDdOB9VUp8FLpTZ2"
+
+    with resource_manager(
+        Resource.MASTER,
+        master_name,
+        boolean_flags=["delete-db"],
+        cmd_input=bytes(f"{password_input}\n{password_input}\n", "utf8"),
+    ) as master_up_command:
+        client = docker.from_env()
+
+        containers = client.containers.list(filters={"name": master_name})
+        assert len(containers) > 0
+        assert (
+            b"Determined Master was launched without a strong initial_user_password set."
+            in master_up_command.stdout
+        )
+
+    containers = client.containers.list(filters={"name": master_name})
+    assert len(containers) == 0


can we run this with multiple cases? of passwords that pass & those that don't & and empty case?

carolinaecalderon · 2024-06-20T18:31:24Z

but I want to defer approval to @hamidzr since he initially reviewed the PR

Hamid is out for the next couple weeks, so, scratch that. I'll approve once my other comments are resolved

carolinaecalderon

LGTM

…cified

…use different passwords

… not specified" This reverts commit 203370a.

…cified

…ce hyphens etc can be disruptive

…launched from subprocess

…ty rules

(cherry picked from commit 1747b94)

cla-bot bot added the cla-signed label Jun 13, 2024

determined-ci requested a review from a team June 13, 2024 18:29

determined-ci added the documentation Improvements or additions to documentation label Jun 13, 2024

jesse-amano-hpe added the User-facing API Change label Jun 13, 2024

jesse-amano-hpe marked this pull request as ready for review June 13, 2024 22:56

jesse-amano-hpe requested a review from a team as a code owner June 13, 2024 22:56

jesse-amano-hpe requested a review from hamidzr June 13, 2024 22:56

hamidzr reviewed Jun 14, 2024

View reviewed changes

tara-det-ai reviewed Jun 14, 2024

View reviewed changes

tara-det-ai self-requested a review June 14, 2024 18:06

tara-det-ai approved these changes Jun 14, 2024

View reviewed changes

determined-ci requested a review from a team June 14, 2024 21:00

jesse-amano-hpe changed the title ~~feat: det deploy local generates a password for you~~ feat: det deploy local generates a password for you [DET-10197] Jun 14, 2024

jesse-amano-hpe requested a review from hamidzr June 17, 2024 17:13

jesse-amano-hpe force-pushed the jta/DET-10197/secure-det-deploy-local-4 branch from df866ff to dda0b7b Compare June 17, 2024 18:01

dannysauer reviewed Jun 19, 2024

View reviewed changes

tara-det-ai reviewed Jun 20, 2024

View reviewed changes

determined-ci requested a review from a team June 20, 2024 16:57

jesse-amano-hpe requested review from dannysauer, tara-det-ai and carolinaecalderon June 20, 2024 17:02

carolinaecalderon reviewed Jun 20, 2024

View reviewed changes

carolinaecalderon approved these changes Jun 20, 2024

View reviewed changes

jesse-amano-hpe added 3 commits June 20, 2024 12:31

tests: add test for master-up with auto generated password

f1ada9c

feat: det deploy local generates a password for you if one is not spe…

6dce4d7

…cified

make sure db is wiped between tests so login does not break if tests …

e96f109

…use different passwords

jesse-amano-hpe added 19 commits June 20, 2024 12:31

Revert "feat: det deploy local generates a password for you if one is…

9172c51

… not specified" This reverts commit 203370a.

feat: det deploy local generates a password for you if one is not spe…

5ebc728

…cified

docs: add release notes for det cli password generation

caf2001

refactor: satisfy linter

feb0087

tests: capture log output from det deploy local cmd

08d1856

test: use byte comparison

1dcee9d

refactor: generate copypastable password instead of using urlsafe sin…

ca7e934

…ce hyphens etc can be disruptive

feat: det deploy local prompts for password

9eee445

make the linter happy

50c3a02

docs: update release notes

6e1b015

test: remove attempt at using magicmock since the code under test is …

63de9b5

…launched from subprocess

fix: check for closed stdin to fail fast

3a099cb

fix: use py3.9 compatible warnings API

1cf0dce

fix: add more fail-fast scenarios

59dd60f

test the interactive password case instead

1bd977c

test: make the linter happier again

a657d0d

fix: guarantee generated password for local deploy satisfies complexi…

a29ab3b

…ty rules

docs: update docs to reflect new scope

702d51d

test: add more branch coverage for interactive password on local deploy

5fb7cc3

jesse-amano-hpe force-pushed the jta/DET-10197/secure-det-deploy-local-4 branch from ed8ebfa to 5fb7cc3 Compare June 20, 2024 19:31

fix: re-add fast fail if tty is closed

633f529

jesse-amano-hpe merged commit 1747b94 into main Jun 20, 2024
88 of 100 checks passed

jesse-amano-hpe deleted the jta/DET-10197/secure-det-deploy-local-4 branch June 20, 2024 22:03

jesse-amano-hpe added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Jun 20, 2024

github-actions bot pushed a commit that referenced this pull request Jun 20, 2024

feat: det deploy local generates a password for you [DET-10197] (#9518)

f15bda8

(cherry picked from commit 1747b94)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: det deploy local generates a password for you [DET-10197] #9518

feat: det deploy local generates a password for you [DET-10197] #9518

jesse-amano-hpe commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 13, 2024 •

edited

Loading

netlify bot commented Jun 13, 2024 •

edited

Loading

hamidzr left a comment

hamidzr Jun 14, 2024

jesse-amano-hpe Jun 14, 2024 •

edited

Loading

carolinaecalderon Jun 20, 2024

tara-det-ai Jun 14, 2024

jesse-amano-hpe Jun 14, 2024

tara-det-ai left a comment

jesse-amano-hpe commented Jun 17, 2024

dannysauer Jun 19, 2024 •

edited

Loading

jesse-amano-hpe Jun 20, 2024

tara-det-ai Jun 20, 2024

carolinaecalderon left a comment

carolinaecalderon Jun 20, 2024

jesse-amano-hpe Jun 20, 2024

carolinaecalderon Jun 20, 2024

carolinaecalderon commented Jun 20, 2024

carolinaecalderon left a comment

feat: det deploy local generates a password for you [DET-10197] #9518

feat: det deploy local generates a password for you [DET-10197] #9518

Conversation

jesse-amano-hpe commented Jun 13, 2024 • edited Loading

Ticket

Description

Additional notes

Test Plan

Checklist

codecov bot commented Jun 13, 2024 • edited Loading

Codecov Report

netlify bot commented Jun 13, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

hamidzr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesse-amano-hpe Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tara-det-ai left a comment

Choose a reason for hiding this comment

jesse-amano-hpe commented Jun 17, 2024

dannysauer Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolinaecalderon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolinaecalderon commented Jun 20, 2024

carolinaecalderon left a comment

Choose a reason for hiding this comment

jesse-amano-hpe commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 13, 2024 •

edited

Loading

netlify bot commented Jun 13, 2024 •

edited

Loading

jesse-amano-hpe Jun 14, 2024 •

edited

Loading

dannysauer Jun 19, 2024 •

edited

Loading