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

Use pba authentication instead of authutils #1505

Merged
merged 3 commits into from
May 7, 2021
Merged

Use pba authentication instead of authutils #1505

merged 3 commits into from
May 7, 2021

Conversation

nichtsfrei
Copy link
Member

@nichtsfrei nichtsfrei commented Apr 28, 2021

To be able to verify new password hashes and update old ones gvmd is
using passwordbasedauthentication of gvm-libs.

This is depending on: greenbone/gvm-libs#484

What:

Why:

How did you test it:

Before you login with your previous account verify the hash is following the old pattern:

> docker-compose exec gvmd psql -d gvmd -c "SELECT * FROM users;"

login and verify if the hash is following the pattern $6$rounds=20000$.....

Then create a user and verify that the new user has the new hash format as well.

To verify the new parameter --pepper and --hashcount you can create a user with pepper and one without and then verify the password hash. The user with pepper should contain: 0000$ to indicate a 4 byte pepper within the db.

> docker-compose -p docker-companion exec gvmd gvmd --pepper DOCK --hashcount 42000 --create-user=testpepper --password=password
> docker-compose -p docker-companion exec gvmd gvmd --create-user=testwopepper --password=password
> docker-compose -p docker-companion exec gvmd psql -d gvmd -c "SELECT name, password FROM users;"
     name     |                                                        password
--------------+-------------------------------------------------------------------------------------------------------------------------

 testpepper   | $6$rounds=42000$UzzzRFyz76I20000$GYnQjEwC8XeTqcoyXhuN04yMRwrwm5ZdqCaYqvo/pEF5T80SC/gh8eumv.CtXwENzKXO0Z6RwdUPx/ex.GHiR0
 testwopepper | $6$rounds=20000$xZtz6zzztdoOmzzz$mDxidDdLgHJTQYoBq6K7lL.Vyu21B0rnuBKdd7BE/N6kSvFY754WIqznUKLnj.AQ5UktUIYI7rNapmySccrd71

Checklist:

@nichtsfrei nichtsfrei added backport-to-oldstable This pull request will be backported to the oldstable branch backport-to-21.04 labels Apr 28, 2021
@nichtsfrei nichtsfrei marked this pull request as ready for review May 3, 2021 15:51
@nichtsfrei nichtsfrei requested a review from a team as a code owner May 3, 2021 15:51
Copy link
Member

@timopollmeier timopollmeier left a comment

Choose a reason for hiding this comment

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

Is there a reason you handle the authentication config so it has to be passed to manage_option_setup and all the manage_... functions instead of handling it like other options?

If it's necessary to do it that way, it would be good if you could add line breaks to the extended function parameter lists so the lines are 80 characters or less.

@nichtsfrei
Copy link
Member Author

nichtsfrei commented May 5, 2021

Is there a reason you handle the authentication config so it has to be passed to manage_option_setup and all the manage_... functions instead of handling it like other options?

If it's necessary to do it that way, it would be good if you could add line breaks to the extended function parameter lists so the lines are 80 characters or less.

I'm not sure how the other options are handled, I need it within: managq_sql.c hence I copied the way how const db_conn_info_t *database is handled since they all call manage_option_setup I assumed that this is the way to go; if there's a more generic option to handle that I would like to change that but I am not aware of one.

In the meantime I will add line breaks.

@nichtsfrei nichtsfrei requested a review from timopollmeier May 5, 2021 10:22
@timopollmeier
Copy link
Member

I was thinking of using getter and setter functions like get_scanner_connection_retry and set_scanner_connection_retry in manage.c and making sure the options are processed before any of the manage_... calls that use them.

@nichtsfrei
Copy link
Member Author

I was thinking of using getter and setter functions like get_scanner_connection_retry and set_scanner_connection_retry in manage.c and making sure the options are processed before any of the manage_... calls that use them.

I don't think this is a good solution in this case because

  • it would either require to verify if that set method has been called or rely on a implicit contract
  • it is alien to manage_sql.c as far as I can tell all settings are either set in init_manage_internal or manage_option_setup

Since manage_option_setup is calling init_manage_helper which then is ending up in init_manage_internal it would have the same effect from an API point of view.

Although I agree that it is weird to introduce authentication settings to DB specific manage construct in my opinion it would not make it easier to maintain when we introduce a setter construct on top within manage_sql.c.

@nichtsfrei
Copy link
Member Author

nichtsfrei commented May 7, 2021

Another way to handle that could be create a separation of the actual authentication and information gathering for the authentication process; I think that could be achieved when separating authenticate_any_method I would need to look into that from a data point of view (to see how complex it is to fetch necessary information so that this could run independently) and may come back with a solution for that.

However I'm not sure if it would be a good idea to do such a massive change for 20.08 which is also targeted by this hash update pull request.

A middle ground could be to start a manage_authentication setup but just deal with password based authentication for now to not have to deal with authentication settings within manag_sql but just verify and create hash. That way the intrusion is limited and the footprint of the existing manage_* methods are reduced.

@timopollmeier
Copy link
Member

Moving the authentication to a separate source and header file to make it clearer that the options need to be initialized sound like a good idea. I think we should go ahead with that.

nichtsfrei added 2 commits May 7, 2021 15:01
 To be able to verify new password hashes and update old ones gvmd is
 using passwordbasedauthentication of gvm-libs.

 To configure:
 - pepper
 - counts
 - algorithm

 authentication_settings_t is introduced.

 manage_option_setup is using authentication_settings_t to translate it
 to
 PBASettings for password based authentication.

 To configure it from a user perspective use `--hashcount` and
 `--pepper`.

 Once a pepper is set you must reuse it; otherwise the login are not
 working due to incorrect salt.
src/manage_authentication.c Outdated Show resolved Hide resolved
src/manage_authentication.c Outdated Show resolved Hide resolved
@timopollmeier timopollmeier enabled auto-merge May 7, 2021 14:10
@timopollmeier timopollmeier disabled auto-merge May 7, 2021 14:26
@timopollmeier timopollmeier merged commit 3b972a1 into greenbone:master May 7, 2021
timopollmeier added a commit that referenced this pull request May 7, 2021
Use pba authentication instead of authutils (backport #1505)
timopollmeier added a commit that referenced this pull request May 7, 2021
Use pba authentication instead of authutils (backport #1505)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-oldstable This pull request will be backported to the oldstable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants