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.

Sign up for GitHub

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

Jump to bottom

Admin master key wrong message #200

Closed
wants to merge 7 commits into from
Closed

Admin master key wrong message #200

wants to merge 7 commits into from

Conversation

fluca1978
Copy link
Collaborator

While using pgagroal-admin I found some odd cases that could be improved:

  • warn the user when the inserted password is not valid, otherwise the user will see Master Key on the screen without any hint;
  • print out where the vault is stored
  • catch SIGINT during the insertion of the master key.

Since I'm not a day-by-day C developer, please review carefully.

@fluca1978
Warn the user when a master key cannot be accepted
5de2957
@fluca1978
Tell the user where the vault is saved
0455132
@fluca1978
Handle SIGINT when editing the master key of the vault.
0e89679 
If the user is in the process of typing the master key
and SIGINT the program, the file is created without
the right permissions, and thus a further invocation
of pgagroal-admin will result in an error
about the wrong permissions.
There is no a signal handler that is installed
before the master_key internal function is invoked
so to handle SIGINT.
@jesperpedersen
Copy link
Collaborator

I don't think we need the is_valid_key check anymore - but verify that (manually) for passwords under 8 chars...

@fluca1978
Remove is_valid_password function.
848100e 
See <agroal#200 (comment)>.
The is_valid_password() was checking only the password length and the fact
that was made by ASCII chars.
The check for the length can be done "inline" directly within a loop.
Added a constant with the minimal length of the password, so that it is possible
to insert a warning message for the user in the case she inputs a too short password.
@fluca1978
Add a do_check_master_key function.
11027c5 
The function checks that the password is of the right size, and contains
the right set of characters. If needed, it can print out
what is wrong with the password check.
@fluca1978
Remove CHARS[] as alphabet fr autogenerated master password.
a2f96ad 
Since there are now separated arrays for every piece of the master key
password, when automatically generating a password the function
concatenate the ALPHA_UC, ALPHA_LC, DIGITS, SPECIALS
arrays into a single one called 'alphabet' that is
used to generate the password.
@fluca1978
Copy link
Collaborator Author

I've done a few changes. In the beginning I deleted the is_valid_password, but after that I realized that having a function that do checks, not only about length, but about quality, would be useful. Therefore, I split CHARS into its part and created do_check_master_key to check the quality of the master key password.
Then, I refactored the generating function to use the concatenation of all the parts as the old CHARS.
The only part that remains, if we want, is to force the generation to validate the password, or better, to extract at least one char from every piece.

In the cae you want to integrate, squash or cherry pick.

@fluca1978
Bootstrap the generated password.
63c2443 
When automatically generating a password, select a different set of chars
for every single position in the password, so that the password
will have always an upper case, followed by a lower case, followed by
a digit, followed by a special, and so on.

This is not the most secure approach, but generates a quite
complex, and most notably, coherent and valid, master key password.
@fluca1978
Copy link
Collaborator Author

Ok, added also the capability to automatically generate a password picking different set of chars, so that it will always contain numbers, letters, specials, and so on.

@jesperpedersen
Copy link
Collaborator

An earlier commit remove all password strength checking for user based account - leaving it to the DBA to set the guidelines. So, I think we should do the same in this case. And, hence CHARS is only going to be used for generated keys.

@fluca1978
Copy link
Collaborator Author

An earlier commit remove all password strength checking for user based account - leaving it to the DBA to set the guidelines. So, I think we should do the same in this case. And, hence CHARS is only going to be used for generated keys.

In such case I believe that cherry-picking 848100e will do the job.

@jesperpedersen
Copy link
Collaborator

Yeah, the cherry-pick looks like it, but I would like to have a proper pull request for it, so rebase, fix indentation and squash, please :)

@fluca1978 fluca1978 mentioned this pull request Feb 21, 2022
Closed
@fluca1978
Copy link
Collaborator Author

Opened issue #206. I'm closing this one and opening a new pull request since most of the work done here is not accepted.

@fluca1978 fluca1978 closed this Feb 21, 2022
fluca1978 added a commit to fluca1978/pgagroal that referenced this pull request Feb 21, 2022
@fluca1978
[agroal#206] Remove is_valid_password function and provide better mes…
0a89423 
…sages.

Close agroal#206.

See <agroal#200 (comment)>.
The is_valid_password() was checking only the password length and the fact
that was made by ASCII chars.
The check for the length can be done "inline" directly within a loop.
Added a constant with the minimal length of the password, so that it is possible
to insert a warning message for the user in the case she inputs a too short password.

The system also prompts the user for a password with a message that explicitly tells
her the password will not appear on the terminal.

See also the initial work on agroal#200.
@fluca1978 fluca1978 mentioned this pull request Feb 21, 2022
Closed
jesperpedersen pushed a commit that referenced this pull request Feb 21, 2022
@fluca1978 @jesperpedersen
[#206] Remove is_valid_password function and provide better messages.
635fecf 
Close #206.

See <#200 (comment)>.
The is_valid_password() was checking only the password length and the fact
that was made by ASCII chars.
The check for the length can be done "inline" directly within a loop.
Added a constant with the minimal length of the password, so that it is possible
to insert a warning message for the user in the case she inputs a too short password.

The system also prompts the user for a password with a message that explicitly tells
her the password will not appear on the terminal.

See also the initial work on #200.
jesperpedersen pushed a commit that referenced this pull request Feb 21, 2022
@fluca1978 @jesperpedersen
[#206] Remove is_valid_password function and provide better messages.
279268f 
Close #206.

See <#200 (comment)>.
The is_valid_password() was checking only the password length and the fact
that was made by ASCII chars.
The check for the length can be done "inline" directly within a loop.
Added a constant with the minimal length of the password, so that it is possible
to insert a warning message for the user in the case she inputs a too short password.

The system also prompts the user for a password with a message that explicitly tells
her the password will not appear on the terminal.

See also the initial work on #200.
@fluca1978 fluca1978 mentioned this pull request Feb 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fluca1978 @jesperpedersen