Summary

Add error handling for when we can't decrypt stored passwords.

Review Request #14276 — Created Dec. 16, 2024 and submitted Dec. 17, 2024, 7:44 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

When a repository has a stored password that can not be decrypted

(usually due to the SECRET_KEY having been changed), we run into some

especially ugly behavior. Any review requests which attempt to access

that repository will just crash entirely with a UnicodeDecodeError,

and attempting to open the repository form to reset the username and

password will show an empty form because the data cannot be loaded.
This change adds some error handling so that if we cannot decrypt the

password, we'll log the error and then return None. This will result

in authentication errors attempting to access the repository, and allows

opening the repository form in the admin to fix it as well.
This also fixes the pysvn implementation of normalize_error to pass in

the error message as the msg parameter to the exception, which makes

it so we don't try to convert that error message to a list for display

(the base SSH authentication error class that SVN auth errors inherit

from has the SSH authentication methods list as its first argument).

Testing Done

Created an authenticated repository with one SECRET_KEY and then
changed it.

Saw that I was able to open the repository settings form and have it
populated with everything except the password. Previously the form
would not only be blank, but the current repository type
("Subversion") wasn't appearing in the tool drop-down at all.
Loaded a review request and diff and saw that instead of getting an
unhandled UnicodeDecodeError exception, I now was able to view the
review request and saw authentication error notices in the diff
content.
Saw that said authentication errors were correctly formatted.

Commits

Summary	ID
Add error handling for when we can't decrypt stored passwords. When a repository has a stored password that can not be decrypted (usually due to the `SECRET_KEY` having been changed), we run into some especially ugly behavior. Any review requests which attempt to access that repository will just crash entirely with a `UnicodeDecodeError`, and attempting to open the repository form to reset the username and password will show an empty form because the data cannot be loaded. This change adds some error handling so that if we cannot decrypt the password, we'll log the error and then return `None`. This will result in authentication errors attempting to access the repository, and allows opening the repository form in the admin to fix it as well. This also fixes the pysvn implementation of normalize_error to pass in the error message as the `msg` parameter to the exception, which makes it so we don't try to convert that error message to a list for display (the base SSH authentication error class that SVN auth errors inherit from has the SSH authentication methods list as its first argument). Testing Done: Created an authenticated repository with one `SECRET_KEY` and then changed it. - Saw that I was able to open the repository settings form and have it populated with everything except the password. Previously the form would not only be blank, but the current repository type ("Subversion") wasn't appearing in the tool drop-down at all. - Loaded a review request and diff and saw that instead of getting an unhandled UnicodeDecodeError exception, I now was able to view the review request and saw authentication error notices in the diff content. - Saw that said authentication errors were correctly formatted.	c35f3acc92a9193544c71b6c3f61efae9c1d011c

Summary

Add error handling for when we can't decrypt stored passwords.

When a repository has a stored password that can not be decrypted (usually due to the `SECRET_KEY` having been changed), we run into some especially ugly behavior. Any review requests which attempt to access that repository will just crash entirely with a `UnicodeDecodeError`, and attempting to open the repository form to reset the username and password will show an empty form because the data cannot be loaded. This change adds some error handling so that if we cannot decrypt the password, we'll log the error and then return `None`. This will result in authentication errors attempting to access the repository, and allows opening the repository form in the admin to fix it as well. This also fixes the pysvn implementation of normalize_error to pass in the error message as the `msg` parameter to the exception, which makes it so we don't try to convert that error message to a list for display (the base SSH authentication error class that SVN auth errors inherit from has the SSH authentication methods list as its first argument). Testing Done: Created an authenticated repository with one `SECRET_KEY` and then changed it. - Saw that I was able to open the repository settings form and have it populated with everything except the password. Previously the form would not only be blank, but the current repository type ("Subversion") wasn't appearing in the tool drop-down at all. - Loaded a review request and diff and saw that instead of getting an unhandled UnicodeDecodeError exception, I now was able to view the review request and saw authentication error notices in the diff content. - Saw that said authentication errors were correctly formatted.

c35f3acc92a9193544c71b6c3f61efae9c1d011c

Issues

Description	From	Last Updated
Can you add unit tests that also check the exception message for some common cases? Say, changed key, corrupt data?	chipx86	Dec. 16, 2024, 9:35 p.m.
I think we should log the repository ID so it's easier to follow up on this, but we'll want to …	chipx86	Dec. 16, 2024, 9:35 p.m.
Let's do an assertLogs just to cover that.	chipx86	Dec. 16, 2024, 9:45 p.m.
'reviewboard.scmtools.models.logger as models_logger' imported but unused Column: 1 Error code: F401	reviewbot	Dec. 16, 2024, 9:46 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can you add unit tests that also check the exception message for some common cases? Say, changed key, corrupt data?

david Dec. 16, 2024, 9:23 p.m.

I don't think we can know why the password failed to decrypt. All we know is that we have garbage that can't be decoded as utf-8.

chipx86 Dec. 16, 2024, 9:27 p.m.

I think it's still best we have test coverage for this, since we're fixing a crash.

reviewboard/scmtools/models.py (Diff revision 1)

The issue has been resolved. Show all issues

I think we should log the repository ID so it's easier to follow up on this, but we'll want to be very careful not to log the value we're decrypting if it comes from the exception (does it? I can't tell).

david Dec. 16, 2024, 9:23 p.m.

The exception just has "UnicodeDecodeError: 'utf-8' codec can't decode byte 0xd6 in position 3: invalid continuation byte" (so... there's one byte from the data).

I can change it to not use logger.exception, and just log that we failed to decrypt the password.

chipx86 Dec. 16, 2024, 9:27 p.m.

Yeah that's probably fine. I just don't want to risk sensitive information in logs.

Commits:

	Summary	ID
	Add error handling for when we can't decrypt stored passwords. When a repository has a stored password that can not be decrypted (usually due to the `SECRET_KEY` having been changed), we run into some especially ugly behavior. Any review requests which attempt to access that repository will just crash entirely with a `UnicodeDecodeError`, and attempting to open the repository form to reset the username and password will show an empty form because the data cannot be loaded. This change adds some error handling so that if we cannot decrypt the password, we'll log the error and then return `None`. This will result in authentication errors attempting to access the repository, and allows opening the repository form in the admin to fix it as well. This also fixes the pysvn implementation of normalize_error to pass in the error message as the `msg` parameter to the exception, which makes it so we don't try to convert that error message to a list for display (the base SSH authentication error class that SVN auth errors inherit from has the SSH authentication methods list as its first argument). Testing Done: Created an authenticated repository with one `SECRET_KEY` and then changed it. - Saw that I was able to open the repository settings form and have it populated with everything except the password. Previously the form would not only be blank, but the current repository type ("Subversion") wasn't appearing in the tool drop-down at all. - Loaded a review request and diff and saw that instead of getting an unhandled UnicodeDecodeError exception, I now was able to view the review request and saw authentication error notices in the diff content. - Saw that said authentication errors were correctly formatted.	3a417829d63cea2bff3851aa701e3c3632b7b32b
	Add error handling for when we can't decrypt stored passwords. When a repository has a stored password that can not be decrypted (usually due to the `SECRET_KEY` having been changed), we run into some especially ugly behavior. Any review requests which attempt to access that repository will just crash entirely with a `UnicodeDecodeError`, and attempting to open the repository form to reset the username and password will show an empty form because the data cannot be loaded. This change adds some error handling so that if we cannot decrypt the password, we'll log the error and then return `None`. This will result in authentication errors attempting to access the repository, and allows opening the repository form in the admin to fix it as well. This also fixes the pysvn implementation of normalize_error to pass in the error message as the `msg` parameter to the exception, which makes it so we don't try to convert that error message to a list for display (the base SSH authentication error class that SVN auth errors inherit from has the SSH authentication methods list as its first argument). Testing Done: Created an authenticated repository with one `SECRET_KEY` and then changed it. - Saw that I was able to open the repository settings form and have it populated with everything except the password. Previously the form would not only be blank, but the current repository type ("Subversion") wasn't appearing in the tool drop-down at all. - Loaded a review request and diff and saw that instead of getting an unhandled UnicodeDecodeError exception, I now was able to view the review request and saw authentication error notices in the diff content. - Saw that said authentication errors were correctly formatted.	db28ebdc50f1de17519fe48e0378719b63c60ddc

Diff:

Revision 2 (+54 -4)

Show changes

	reviewboard/scmtools/models.py
	reviewboard/scmtools/svn/pysvn.py
	reviewboard/scmtools/tests/test_repository.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/scmtools/tests/test_repository.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Let's do an assertLogs just to cover that.
```

Commits:

	Summary	ID
	Add error handling for when we can't decrypt stored passwords. When a repository has a stored password that can not be decrypted (usually due to the `SECRET_KEY` having been changed), we run into some especially ugly behavior. Any review requests which attempt to access that repository will just crash entirely with a `UnicodeDecodeError`, and attempting to open the repository form to reset the username and password will show an empty form because the data cannot be loaded. This change adds some error handling so that if we cannot decrypt the password, we'll log the error and then return `None`. This will result in authentication errors attempting to access the repository, and allows opening the repository form in the admin to fix it as well. This also fixes the pysvn implementation of normalize_error to pass in the error message as the `msg` parameter to the exception, which makes it so we don't try to convert that error message to a list for display (the base SSH authentication error class that SVN auth errors inherit from has the SSH authentication methods list as its first argument). Testing Done: Created an authenticated repository with one `SECRET_KEY` and then changed it. - Saw that I was able to open the repository settings form and have it populated with everything except the password. Previously the form would not only be blank, but the current repository type ("Subversion") wasn't appearing in the tool drop-down at all. - Loaded a review request and diff and saw that instead of getting an unhandled UnicodeDecodeError exception, I now was able to view the review request and saw authentication error notices in the diff content. - Saw that said authentication errors were correctly formatted.	db28ebdc50f1de17519fe48e0378719b63c60ddc
	Add error handling for when we can't decrypt stored passwords. When a repository has a stored password that can not be decrypted (usually due to the `SECRET_KEY` having been changed), we run into some especially ugly behavior. Any review requests which attempt to access that repository will just crash entirely with a `UnicodeDecodeError`, and attempting to open the repository form to reset the username and password will show an empty form because the data cannot be loaded. This change adds some error handling so that if we cannot decrypt the password, we'll log the error and then return `None`. This will result in authentication errors attempting to access the repository, and allows opening the repository form in the admin to fix it as well. This also fixes the pysvn implementation of normalize_error to pass in the error message as the `msg` parameter to the exception, which makes it so we don't try to convert that error message to a list for display (the base SSH authentication error class that SVN auth errors inherit from has the SSH authentication methods list as its first argument). Testing Done: Created an authenticated repository with one `SECRET_KEY` and then changed it. - Saw that I was able to open the repository settings form and have it populated with everything except the password. Previously the form would not only be blank, but the current repository type ("Subversion") wasn't appearing in the tool drop-down at all. - Loaded a review request and diff and saw that instead of getting an unhandled UnicodeDecodeError exception, I now was able to view the review request and saw authentication error notices in the diff content. - Saw that said authentication errors were correctly formatted.	8dd82b63b30d88b00f006afa08ff62b534c96136

Diff:

Revision 3 (+68 -6)

Show changes

	reviewboard/scmtools/models.py
	reviewboard/scmtools/svn/pysvn.py
	reviewboard/scmtools/tests/test_repository.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/scmtools/tests/test_repository.py (Diff revision 3)

The issue has been dropped. Show all issues

'reviewboard.scmtools.models.logger as models_logger' imported but unused

Column: 1
Error code: F401

Commits:

	Summary	ID
	Add error handling for when we can't decrypt stored passwords. When a repository has a stored password that can not be decrypted (usually due to the `SECRET_KEY` having been changed), we run into some especially ugly behavior. Any review requests which attempt to access that repository will just crash entirely with a `UnicodeDecodeError`, and attempting to open the repository form to reset the username and password will show an empty form because the data cannot be loaded. This change adds some error handling so that if we cannot decrypt the password, we'll log the error and then return `None`. This will result in authentication errors attempting to access the repository, and allows opening the repository form in the admin to fix it as well. This also fixes the pysvn implementation of normalize_error to pass in the error message as the `msg` parameter to the exception, which makes it so we don't try to convert that error message to a list for display (the base SSH authentication error class that SVN auth errors inherit from has the SSH authentication methods list as its first argument). Testing Done: Created an authenticated repository with one `SECRET_KEY` and then changed it. - Saw that I was able to open the repository settings form and have it populated with everything except the password. Previously the form would not only be blank, but the current repository type ("Subversion") wasn't appearing in the tool drop-down at all. - Loaded a review request and diff and saw that instead of getting an unhandled UnicodeDecodeError exception, I now was able to view the review request and saw authentication error notices in the diff content. - Saw that said authentication errors were correctly formatted.	8dd82b63b30d88b00f006afa08ff62b534c96136
	Add error handling for when we can't decrypt stored passwords. When a repository has a stored password that can not be decrypted (usually due to the `SECRET_KEY` having been changed), we run into some especially ugly behavior. Any review requests which attempt to access that repository will just crash entirely with a `UnicodeDecodeError`, and attempting to open the repository form to reset the username and password will show an empty form because the data cannot be loaded. This change adds some error handling so that if we cannot decrypt the password, we'll log the error and then return `None`. This will result in authentication errors attempting to access the repository, and allows opening the repository form in the admin to fix it as well. This also fixes the pysvn implementation of normalize_error to pass in the error message as the `msg` parameter to the exception, which makes it so we don't try to convert that error message to a list for display (the base SSH authentication error class that SVN auth errors inherit from has the SSH authentication methods list as its first argument). Testing Done: Created an authenticated repository with one `SECRET_KEY` and then changed it. - Saw that I was able to open the repository settings form and have it populated with everything except the password. Previously the form would not only be blank, but the current repository type ("Subversion") wasn't appearing in the tool drop-down at all. - Loaded a review request and diff and saw that instead of getting an unhandled UnicodeDecodeError exception, I now was able to view the review request and saw authentication error notices in the diff content. - Saw that said authentication errors were correctly formatted.	c35f3acc92a9193544c71b6c3f61efae9c1d011c

Diff:

Revision 4 (+64 -4)

Show changes

	reviewboard/scmtools/models.py
	reviewboard/scmtools/svn/pysvn.py
	reviewboard/scmtools/tests/test_repository.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (642bc77)