Conjurctl create account with password #2042

jvanderhoof · 2021-02-17T14:27:12Z

What does this PR do?

Enable operators to specify an admin user's password:
- conjurctl account create --name demo --password-from-stdin (STDIN)
- conjurctl server --account demo --password-from-stdin (STDIN)
How to manually test?
- cd dev
- dev: docker-compose up -d --no-deps pg conjur client
- dev: docker-compose exec conjur bundle
- dev: docker-compose exec conjur conjurctl db migrate
- exec into the conjur container and run echo -n "testP@SS1alice" | conjurctl server --account demo --password-file -
- Attempt to authenticate the admin user with the password set above
- Also test echo -n "testP@SS1alice" | conjurctl account create demo1 -
- Attempt to authenticate the admin user with the password set above
- Repeat the previous steps but write the password to a file and provide the path instead of utilizing STDIN

What ticket does this PR close?

Resolves #2043

Checklists

Change log

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

bin/conjur-cli.rb

lib/tasks/account.rake

lib/conjur/password_complexity.rb

bin/conjur-cli.rb

codeclimate · 2021-02-19T17:09:01Z

lib/conjur/password.rb

+    VALID_PASSWORD_REGEX = %r{^(?=.*?[A-Z].*[A-Z]) # 2 uppercase letters
+      (?=.*?[a-z].*[a-z])                                 # 2 lowercase letters
+      (?=.*?[0-9])                                        # 1 digit
+      (?=.*[ !"#$%&'()*+,-.\/:;<=>?@\[\\\]^_`{|}~]).      # 1 special character


Redundant escape inside regexp literal

micahlee · 2021-02-23T18:40:54Z

bin/conjur-cli.rb

 Example:

-$ conjurctl account create myorg
+$ conjurctl account create myorg [password_file]


Given it's optional, would it make since for this to be a flag (--password-file) instead? That also has the benefit of being consistent with the server command.

this is small, but there is a difference between providing a reference to a file and to providing the file contents. should we be overloading the same flag with both use cases? maybe it would be better to have a password flag for the password value, and a password-file flag for a reference to a file containing the value?

I am leaning towards just including a password flag (which would only accept input via STDIN). If there is value in adding the flag password-file I will be happy to add it in the future.

micahlee · 2021-02-23T18:54:26Z

bin/conjur-cli.rb

+def file_input(path)
+  if path == '-'
+    $stdin.read.force_encoding('ASCII-8BIT')
+  else
+    File.read(path, mode: 'rb')
+  end
+end


It looks like there are a number of potential failure cases here. Do these manifest in the messages you would want the user to see?

For example:

The file doesn't exist

The file is empty

The - argument is given, but nothing is provided to STDIN

Along these lines, is there any way some of this could be unit tested? Or is there no infrastructure/fixture for that now with this CLI?

If the file doesn't exist the user will see the following error:
conjurctl server --account demo --password-file /no_such_file error: No such file or directory @ rb_sysopen - /no_such_file.
I can wrap this error if you think it would provide a better UX.

If the - argument is given, but nothing is provided to STDIN ex.
conjurctl server --account demo --password-file, it will hang.
However, a user could still provide input by typing in a password and then Ctrl+D.
I could add a timeout if you think this would improve the UX, but there are pros and cons to that approach as well.

We currently do not have any unit tests for conjurctl. I will look into adding a cucumber test for conjurctl account create

Regarding:

If the - argument is given, but nothing is provided to STDIN ex.
conjurctl server --account demo --password-file, it will hang.
However, a user could still provide input by typing in a password and then Ctrl+D.
I could add a timeout if you think this would improve the UX, but there are pros and cons to that approach as well.

You can actually detect this without a timeout using $stdin.tty?

command $stdin.tty?

echo 'password' | conjurctl ... --password-file - false

conjurctl ... --password-file - < password-file.txt false

conjurctl ... --password-file - true

That would give you either an opportunity to interactively prompt for it, or just raise an error if the standard input isn't piped in.

CHANGELOG.md

izgeri · 2021-02-24T15:06:57Z

will this flow have test cases added for it? just wanted to flag that here, since the PR doesn't say WIP / it's not a draft - if we can add tests, we should do that before merging.

also, is there a docs card?

h-artzi · 2021-02-24T15:10:39Z

will this flow have test cases added for it? just wanted to flag that here, since the PR doesn't say WIP / it's not a draft - if we can add tests, we should do that before merging.

also, is there a docs card?

@izgeri I do not believe we currently have tests for the conjurctl commands. That doesn't mean they can't be added for this PR.

I was unable to find the docs for the conjurctl commands, do you mind pointing me in the right direction?

izgeri · 2021-02-26T18:05:38Z

@h-artzi I hoped we had something in the docs, but it looks like the best reference is actually the quick start :(

Maybe we can create a new small section after "Next Steps" here that explains "Configuring Conjur with a pre-defined admin password"?

I am uncomfortable merging this without tests, or if the scope of adding tests is too big, without and issue filed with a plan to address it that will add the appropriate tests. There are a few cukes that test conjurctl functionality - can we add some cukes or a new test file here: https://github.com/cyberark/conjur/tree/master/lib/test - something that gives us assurance we won't inadvertently break this in a future release?

On a totally separate note - is there a default value for account if you don't pass one to conjurctl? If not, I'd like to consider a follow-up that sets it to default, so that our clients can start assuming a sane default for account value.

CHANGELOG.md

spec/conjurctl/server_spec.rb

h-artzi · 2021-03-24T23:09:57Z

Relative follow up tickets:
add test when no STDIN is provided but --password-from-stdin is present
default account when none is provided

micahlee

This looks good to me!

It might be helpful to get a quick check from @izgeri and @jonahx to make sure their suggestions have been addressed though.

This lets us to check password validity outside of the Credentials class

The previous command would just kill the parent process id.

codeclimate · 2021-03-30T19:23:15Z

Code Climate has analyzed commit 2aecf28 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Style	1

The test coverage on the diff in this pull request is 48.1% (50% is the threshold).

This pull request will bring the total coverage in the repository to 89.2% (-0.2% change).

View more on Code Climate.

jvanderhoof requested a review from a team as a code owner February 17, 2021 14:27

codeclimate bot reviewed Feb 17, 2021

View reviewed changes

jonahx reviewed Feb 17, 2021

View reviewed changes

lib/conjur/password_complexity.rb Outdated Show resolved Hide resolved

jonahx reviewed Feb 17, 2021

View reviewed changes

bin/conjur-cli.rb Outdated Show resolved Hide resolved

jonahx reviewed Feb 17, 2021

View reviewed changes

bin/conjur-cli.rb Outdated Show resolved Hide resolved

h-artzi linked an issue Feb 17, 2021 that may be closed by this pull request

Set admin password via conjurctl #2043

Closed

h-artzi force-pushed the conjurctl_create_account_with_password branch 5 times, most recently from 8ffc5ae to 753a3c2 Compare February 19, 2021 16:44

codeclimate bot reviewed Feb 19, 2021

View reviewed changes

h-artzi changed the title ~~[WIP] Conjurctl create account with password~~ Conjurctl create account with password Feb 19, 2021

h-artzi requested a review from jonahx February 19, 2021 18:15

h-artzi assigned jvanderhoof Feb 19, 2021

micahlee reviewed Feb 23, 2021

View reviewed changes

izgeri reviewed Feb 24, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

h-artzi force-pushed the conjurctl_create_account_with_password branch from fd5a194 to 9d17bad Compare March 19, 2021 20:56

h-artzi requested a review from izgeri March 19, 2021 20:56

h-artzi force-pushed the conjurctl_create_account_with_password branch from 9d17bad to bf310cb Compare March 19, 2021 21:37

izgeri reviewed Mar 22, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

izgeri reviewed Mar 22, 2021

View reviewed changes

spec/conjurctl/server_spec.rb Show resolved Hide resolved

h-artzi force-pushed the conjurctl_create_account_with_password branch 5 times, most recently from 364057f to 542089d Compare March 23, 2021 20:55

h-artzi force-pushed the conjurctl_create_account_with_password branch from 01664eb to 635a10f Compare March 24, 2021 16:19

h-artzi force-pushed the conjurctl_create_account_with_password branch from 635a10f to d7df439 Compare March 24, 2021 23:10

h-artzi requested a review from a team March 25, 2021 15:50

h-artzi force-pushed the conjurctl_create_account_with_password branch from d7df439 to 4961f34 Compare March 26, 2021 15:01

h-artzi assigned h-artzi and unassigned jvanderhoof Mar 26, 2021

h-artzi force-pushed the conjurctl_create_account_with_password branch from 4961f34 to 5a86e65 Compare March 29, 2021 13:10

micahlee previously approved these changes Mar 30, 2021

View reviewed changes

h-artzi dismissed micahlee’s stale review via 434b3c6 March 30, 2021 15:18

h-artzi force-pushed the conjurctl_create_account_with_password branch 3 times, most recently from 1672553 to fe8f44c Compare March 30, 2021 16:55

micahlee previously approved these changes Mar 30, 2021

View reviewed changes

h-artzi dismissed micahlee’s stale review via 3b30d11 March 30, 2021 18:00

h-artzi force-pushed the conjurctl_create_account_with_password branch from fe8f44c to 3b30d11 Compare March 30, 2021 18:00

micahlee previously approved these changes Mar 30, 2021

View reviewed changes

h-artzi added 7 commits March 30, 2021 15:00

Refactor password validation

5d7433c

This lets us to check password validity outside of the Credentials class

Add password validity tests

a231b28

Add a password parameter to account creation

78129b2

Kill conjurctl server and its child processes

1ee8da9

The previous command would just kill the parent process id.

Test conjurctl server with password-from-stdin

7698fb1

Test conjurctl account with password-from-stdin

2796da7

Update CHANGELOG messages (#2043)

2aecf28

h-artzi dismissed micahlee’s stale review via 2aecf28 March 30, 2021 19:00

h-artzi force-pushed the conjurctl_create_account_with_password branch from 3b30d11 to 2aecf28 Compare March 30, 2021 19:00

micahlee approved these changes Mar 30, 2021

View reviewed changes

h-artzi merged commit a168d44 into master Mar 30, 2021

h-artzi deleted the conjurctl_create_account_with_password branch March 30, 2021 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conjurctl create account with password #2042

Conjurctl create account with password #2042

jvanderhoof commented Feb 17, 2021 •

edited by h-artzi

Loading

codeclimate bot Feb 19, 2021

micahlee Feb 23, 2021

izgeri Feb 26, 2021

h-artzi Mar 2, 2021

micahlee Feb 23, 2021

h-artzi Feb 24, 2021

micahlee Feb 24, 2021 •

edited

Loading

h-artzi Feb 24, 2021

izgeri commented Feb 24, 2021

h-artzi commented Feb 24, 2021 •

edited

Loading

izgeri commented Feb 26, 2021

h-artzi commented Mar 24, 2021

micahlee left a comment

codeclimate bot commented Mar 30, 2021

command	`$stdin.tty?`
`echo 'password' \| conjurctl ... --password-file -`	false
`conjurctl ... --password-file - < password-file.txt`	false
`conjurctl ... --password-file -`	true

Conjurctl create account with password #2042

Conjurctl create account with password #2042

Conversation

jvanderhoof commented Feb 17, 2021 • edited by h-artzi Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

codeclimate bot Feb 19, 2021

Choose a reason for hiding this comment

micahlee Feb 23, 2021

Choose a reason for hiding this comment

izgeri Feb 26, 2021

Choose a reason for hiding this comment

h-artzi Mar 2, 2021

Choose a reason for hiding this comment

micahlee Feb 23, 2021

Choose a reason for hiding this comment

h-artzi Feb 24, 2021

Choose a reason for hiding this comment

micahlee Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

h-artzi Feb 24, 2021

Choose a reason for hiding this comment

izgeri commented Feb 24, 2021

h-artzi commented Feb 24, 2021 • edited Loading

izgeri commented Feb 26, 2021

h-artzi commented Mar 24, 2021

micahlee left a comment

Choose a reason for hiding this comment

codeclimate bot commented Mar 30, 2021

jvanderhoof commented Feb 17, 2021 •

edited by h-artzi

Loading

micahlee Feb 24, 2021 •

edited

Loading

h-artzi commented Feb 24, 2021 •

edited

Loading