[20.05] Assign random password in user manager if none specified

[nuwang]

I ran into this error when trying to integrate an OpenID connect provider.

112.134.192.179 - - [01/Apr/2020:21:09:52 +1000] "GET /authnz/custos/callback?code=itp%3Aac%3A476282cd-194b-4d96-baf2-0cc465ee0760&state=UfFhjyMS8oPcJ4Z9rRliKRdgHJevDz HTTP/1.1" 500 - "https://galaxy-aust-staging.genome.edu.au/root/login?is_logout_redirect=true" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.149 Safari/537.36"
Traceback (most recent call last):
  File "lib/galaxy/authnz/managers.py", line 260, in callback
    return True, message, backend.callback(state_token, authz_code, trans, login_redirect_url)
  File "lib/galaxy/authnz/custos_authnz.py", line 110, in callback
    user = trans.app.user_manager.create(email=email, username=username)
  File "lib/galaxy/managers/users.py", line 96, in create
    user.set_password_cleartext(password)
  File "lib/galaxy/model/__init__.py", line 393, in set_password_cleartext
    self.password = new_secure_hash(text_type=cleartext)
  File "lib/galaxy/util/hash_util.py", line 72, in new_secure_hash
galaxy.model.database_heartbeat DEBUG 2020-04-01 21:09:53,146 [p:10875,w:1,m:0] [database_heartbeart_main.web.1.thread] main.web.1 is not config watcher
    assert text_type is not None
AssertionError

It turns out that the user_manager does not handle empty passwords correctly. The issue appears to be straightforward, but was wondering how it was not breaking authnz completely, and whether this was caused by a recent code change perhaps?

[VJalili]

@nuwang My apologies for the late response. I guess you're using a provider via the CustosAuthnz; have you tried authentication flow against a PSA-based provider as well? (e.g., Google or Elixir)

[nuwang]

@nuwang My apologies for the late response. I guess you're using a provider via the CustosAuthnz; have you tried authentication flow against a PSA-based provider as well? (e.g., Google or Elixir)

I only tried with Custos because only Custos exposed configuration through a well-known endpoint. It worked quit well.

[nuwang]

@VJalili Just a ping. Is this looking ok to go?

[dannon]

@nuwang This looks fine, but I am curious about "but was wondering how it was not breaking authnz completely, and whether this was caused by a recent code change perhaps?" as well. Did you manage to look into this more?

[VJalili]

@nuwang can you please test this against a PSA-based backend and make sure it does not break them? Testing against Google should be sufficient.

Unfortunately I guess with Custos we diverged a bit, so we should make sure the PSA-based backend is equally developed/maintained.

[nuwang]

@dannon I believe this refactoring commit caused it: 2a18857#diff-74566421da44253f075336826614cf9c
trans.app.user_manager.create(email=email, username=username) did not support empty passwords, while the earlier code created a User object directly, hence the empty password was not an issue.

@VJalili I agree that it would be nice to have PSA tested better, but that needs a separate integration test I think. I don't think this commit is related, and right now, I can't commit the time to test it unfortunately.

[dannon]

@nuwang Thanks for the context; it looks like there's a bit more to do here if that's the code that broke it. Let me test it out.

[dannon]

@nuwang I just figured out how we never saw this in any of the interim work -- do you have use_pbkdf2 set to false? We migrated that to the recommended default ~7 years ago, and that's the only way this bug would get hit that I can see.

[nuwang]

@dannon Yes, that seems to be the case. I hit this on the GA staging server, and presumably, that was carrying old defaults forward. Might be an issue with older cloudman versions even. How does one set about changing this? I imagine just flipping the setting won't do?

However, I don't quite follow why only setting pbkdf2 triggers this. Won't hitting this line do so? https://github.com/galaxyproject/galaxy/pull/9587/files#diff-74566421da44253f075336826614cf9cL108
Or is it that by the time it gets there, the user is guaranteed to have been created unless pbkdf2?

[dannon]

@nuwang It's all going to go through https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/model/__init__.py#L392, which only uses new_secure_hash for password generation when use_pbkdf2 is disabled. Otherwise we use galaxy.security.passwords.hash_password, which already generates a random password without blowing up when you pass it None.

>  galaxy.security.passwords.hash_password_PBKDF2(None)
'PBKDF2$sha256$100000$1W6gGBjfjwP50id5$b6B40Ckxb1f4m/r3fceRy9rCrfaBdY08'

[nuwang]

Right, that makes sense. So this fix should perhaps be made in new_secure_hash instead to handle empty clear text?

Although that has a very specific check to assert that it should be non null:

galaxy/lib/galaxy/util/hash_util.py

Line 72 in f0ebdd4

assert text_type is not None

[nsoranzo]

Right, that makes sense. So this fix should perhaps be made in new_secure_hash instead to handle empty clear text?

I was thinking that instead we should also raise an Exception in galaxy.security.passwords.hash_password() if password is None.
Presently None is a valid password:

>>> from galaxy.security.passwords import check_password, hash_password
>>> hashed = hash_password(None)                                                                                                                                                                              
>>> check_password(None, hashed)                                                                                                                                                                              
True

[dannon]

@nsoranzo Good catch; I shouldn't have said it generates a random password -- it generates a valid hash of the null password! :) That should probably throw an exception, agreed.

Galaxy still needs refactoring to make user creation all go through the manager, but that probably shouldn't happen here. To handle all the cases where someone's trying to set a null password, should we just update set_password_cleartext to set a random password when invoked with cleartext==None?

[nsoranzo]

Seems a good plan! To summarise:

1. add galaxy.security.passwords.hash_password():

    if password is None:
        raise Exception("password cannot be None")

2. Move Nuwan's changes from UserManager.create() to User.set_password_cleartext() and I'd also add a log.info() when the password is None and we create a random one.

[nuwang]

@dannon @nsoranzo I did a bit of refactoring on the password handling. See what you think.
Instead of validating only an empty password in set_password_cleartext, I thought it might be better to validate all properties of a password, including length, so that the password policy is centralized. The decision of whether or not to set a random password will remain with the caller, instead of set_password_cleartext. I think this would be more in line with caller expectations.

While doing this, also noticed a bit of repetition between the admin controller, toolshed and galaxy in terms of validation, so tried to factor that out too. One minor difference is that the toolshed public name regex is a bit different from the Galaxy one, and I opted to use the Galaxy public name regex. If this is a problem, I can restore the original regex for the toolshed.

[mvdbeek]

This looks good to me, what do you think @nsoranzo @dannon ?

[mvdbeek]

I've changed the base to 20.05, since this does seem to have the potential to affect user-facing behavior. If something goes wrong it seems easier to isolate this if we roll it out with 20.05. Feel free to backport it further if this is absolutely required in 20.01.