Add handling of corrupted SCM password data

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

Information

Review Board
release-3.0.x
46ef38e...

Reviewers

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.
Description From Last Updated

F841 local variable 'e' is assigned to but never used

reviewbotreviewbot

F821 undefined name 'invalid'

reviewbotreviewbot
easyb
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. 
      
easyb
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

easyb
  1. 
      
  2. reviewboard/scmtools/errors.py (Diff revision 2)
     
     

    If we would want to have here (...) for repository %d, we would need to pass the Repository model's ID to the constructor, right? We do not have that information where the exception is raised...

  3. Ooops.

  4. 
      
easyb
Review request changed

Summary:

-Add handling of invalid SCM passwords in database
+Add handling of corrupted SCM password data

Commit:

-e13fcc045dfcbfd2830f9fc02149cc5672884586
+46ef38e626a01ecd136b32c73acc813312389c54

Diff:

Revision 3 (+120 -3)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...