Add handling of invalid SCM passwords in database

Review Request #10393 — Created Jan. 26, 2019 and updated

easyb
Review Board
release-3.0.x
aba8835...
reviewboard
E.g. after a database import the SCM repository passwords can no longer
be decrypted. This would lead to an exception when trying to decode the
password to UTF-8. Instead, `Repository._get_password()` will now return
`None` resulting in requiring the user to reset the password.
  • Added SVN repository with authentication.
  • Dumped database using: mysqldump --all-databases
  • Imported the dump into a fresh MySQL instance.
  • Successfully navigated to: /admin/db/scmtools/repository/1/
  • Observed error message in log.
easyb
Review request changed

Testing Done:

   
  • Added SVN repository with authentication.
~  
  • Dumped database using mysqldump --all-databases
  ~
  • Dumped database using: mysqldump --all-databases
   
  • Imported the dump into a fresh MySQL instance.
   
  • Successfully navigated to: /admin/db/scmtools/repository/1/
   
  • Observed error message in log.
chipx86
  1. This is a great issue to fix! I think we can go a bit further to make this even more helpful when problems occur. There's a couple main issues that have been known to happen:

    1. The password decrypts incorrectly due to settings.SECRET_KEY being wrong (changed).
    2. A bad database import corrupted the password data.

    Both of these can manifest as:

    1. A ValueError: "Valid" Base64 data but invalid IV (Initialization Vector) length.
    2. A TypeError: Invalid Base64 data, resulting in an incorrect padding length.
    3. A UnicodeDecodeError: "Valid" Base64 and IV, but resulting decoded data is not in an expected encoding (possibly a changed SECRET_KEY that happens to work but doesn't provide the right result)
    4. A decodable result that seems to work but is the wrong password (again, possibly changed data).

    This change deals with failure type 3, but types 1, 2, and 4 should still be addressed. If a user encounters any of these, it'd be nice if they had some form of help.

    This also doesn't address failure type 3 when working with repositories backed by hosting service.

    So here's my proposal:

    1. Update decrypt_password to catch ValueError and TypeError, and raise a DecryptPasswordError (you'd need to introduce this in errors.py) with an error message helpful to admins (something like "Unable to decode the password for repository %d. This may be caused by a change in settings.SECRET_KEY or bad encoded data from a database import."). This addresses failure types 1 and 2.
    2. Update decrypt_password to also validate the content as Unicode (still return a bytes result, as we need that for many reasons, but at least attempt a decode and check the result), raising the above error as well.
    3. Update both Repository._get_password and Repository.get_credentials (wrapping the call to service.get_password()) to catch this new exception, log the result, return None. This means we no longer need to hard-code the error message here.

    This gives us more comprehensive checking and coverage, and means that other callers of decrypt_password that aren't explicitly catching the exception will at least allow it to bubble up and present something useful in the log files.

    It's also important that we have unit tests covering all three cases that would result in the DecryptPasswordError, and unit tests checking the log and result of the Repository methods when encountering encoded password failures (we don't need to test all three types here, just any ol' failure).

    I know that sounds like a lot more work, but it's just a new exception class, a try/except for decrypt_password and get_credentials, the matching doc updates for these methods, and a few small unit tests. This would go a long way toward improving the lives of those encountering the problem.

    1. Thanks for the answer. I'll see what I can do.

  2. 
      
Loading...