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

Enhance passphrase handling (Fixes #8496) #8605

Merged
merged 9 commits into from
Jan 8, 2025

Conversation

alighazi288
Copy link
Contributor

@alighazi288 alighazi288 commented Dec 26, 2024

Description

This pull request improves the handling of passphrases and adds comprehensive debug information. The main changes include:

  • Displaying hex bytes for passphrases in all cases, including ASCII and non-ASCII characters.
  • Adding an option to display the hex bytes when the password is used and found incorrect.
  • Showing which environment variables were used during passphrase operations.
  • Preventing sensitive passphrase information from being logged unintentionally by directing it to sys.stderr.
  • Adding tests to verify:
    • Handling of incorrect passphrases.
    • Passphrase verification logic.
    • Debug information display for passphrases.

Copy link

codecov bot commented Dec 27, 2024

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 81.79%. Comparing base (1559a1e) to head (d09d2d8).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
src/borg/crypto/key.py 0.00% 2 Missing ⚠️
src/borg/helpers/passphrase.py 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8605      +/-   ##
==========================================
- Coverage   81.83%   81.79%   -0.05%     
==========================================
  Files          74       74              
  Lines       13319    13333      +14     
  Branches     1963     1966       +3     
==========================================
+ Hits        10900    10906       +6     
- Misses       1755     1761       +6     
- Partials      664      666       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Some feedback.

src/borg/crypto/key.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Show resolved Hide resolved
@alighazi288
Copy link
Contributor Author

Thank you so much for the feedback! Will try to fix all this by tomorrow!

Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

some feedback.

beside addressing the feedback and writing a unit test, i also recommend that you practically try this code for some scenarios.

E.g.:

Use borg repo-create -e repokey ... - this will ask you to define a passphrase. Answer y or answer n when it asks to display the passphrase.

Use borg repo-list ... to access the repo. It will ask for a passphrase and under normal circumstances, it MUST NOT display what you entered. But there should be a way for optional "passphrase debugging" and it displaying the passphrase and its hex encoding.

Do the same, but use an env var with a correct and with a incorrect passphrase.

src/borg/crypto/key.py Outdated Show resolved Hide resolved
src/borg/crypto/key.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
@alighazi288
Copy link
Contributor Author

alighazi288 commented Dec 30, 2024

This is exactly how I've been testing. I was just uncertain whether the requirement was to always show debug information when a wrong passphrase is entered, or if it should be shown "optionally" based on the user's input. I chose to conditionally display it based on the new environment variable BORG_DEBUG_PASSPHRASE.

Now, debugging information (including passphrase and environment variable details) is only shown when the BORG_DEBUG_PASSPHRASE environment variable is set to "YES".

I hope this matches the intended behavior. Looking forward to your thoughts!

src/borg/helpers/passphrase.py Fixed Show fixed Hide fixed
src/borg/helpers/passphrase.py Fixed Show fixed Hide fixed
Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

good to require that env var, so the stuff isn't printed by default.

src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Dismissed Show dismissed Hide dismissed
Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

consistency / simplification suggestions.

src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Show resolved Hide resolved
src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Outdated Show resolved Hide resolved
src/borg/helpers/passphrase.py Dismissed Show dismissed Hide dismissed
@ThomasWaldmann
Copy link
Member

The docs CI failure seems unrelated to your PR changes.

Maybe some stuff in sphinx changed, so it is triggering new warnings.

Guess I need to change so that code unit tests do not depend on docs test.

@alighazi288
Copy link
Contributor Author

@ThomasWaldmann if this is ready to close, do you have any suggestions for another issue I could work on next?

@ThomasWaldmann
Copy link
Member

I've fixed the docs issue in master.

Please update your local master branch, then rebase the PR branch onto master, then force push to github.

In the issue tracker, some stuff is labelled as easy, good first issue or help wanted - choose from these if you feel it is a good fit for you.

- Ensure hex bytes for passphrases are displayed in all cases, covering both ASCII and non-ASCII characters.

- Introduce an option to show the hex bytes when an incorrect password is detected.

- Indicate which environment variables were utilized during passphrase operations.

- Direct sensitive passphrase information to sys.stderr to prevent unintended logging.
Added tests to verify:
- Handling of incorrect passphrases.
- Passphrase verification logic.
- Debug information display for passphrases.
- Ensured the passphrase argument is correctly passed to the PassphraseWrong exception.
- Displaying passphrase debugging information only when the `BORG_DEBUG_PASSPHRASE` environment variable is set to "YES".
- If the env var is not set or set to a value other than "YES", debugging info will not be displayed.
@alighazi288 alighazi288 force-pushed the fix-issue-8496-clean branch from bbdb424 to 6463ad6 Compare January 5, 2025 19:27
Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

found quite some more todo in the tests.

src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
@alighazi288
Copy link
Contributor Author

I realized I completely forgot to update the tests after changing my implementation—apologies for that oversight! I've reviewed and adjusted everything now, so the tests should align correctly with the updated implementation. Let me know if there's anything else you'd like me to refine.

@alighazi288 alighazi288 force-pushed the fix-issue-8496-clean branch from d09d2d8 to b907173 Compare January 7, 2025 18:12
Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

LGTM!

@ThomasWaldmann ThomasWaldmann merged commit 40df2f3 into borgbackup:master Jan 8, 2025
27 of 29 checks passed
@alighazi288 alighazi288 deleted the fix-issue-8496-clean branch January 8, 2025 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants