Add a way to configure basic auth without storing passwords in plaintext in settings #2
Conversation
|
@jmagnusson is there anything I can do to help get the ball rolling here? |
|
Hi @immerrr, What's your use case for saving the passwords as base64 digests? Really, base64 is just an obfuscation method and shouldn't be seen as a way to increase security. I would recommend that you take a look at sops, which can be used to encrypt the data on its way to the server (i.e. CI/CD). |
|
please note, that in the PR before base64 is applied to the digest, it is actually hashed with SHA512 which should have the necessary protection from snooping the original value
Done that way, a user can generate a hash for their password, and pass that to the ops for further management without disclosing the actual secret at any time (not in the repo, not in the manifest, not in the kubernetes api server), thus applying the principle of least privilege. The implementation is conceptually similar to nginx, which takes its basic auth credentials from htpasswd files where the secrets are hashed with bcrypt. |
|
Ah, I'm sorry. I didn't notice that part. I'll make a code review now. |
|
@jacobsvante I have addressed your feedback (either in code or as a comment), please, let me know WDYT |
Codecov Report
@@ Coverage Diff @@
## main #2 +/- ##
===========================================
- Coverage 100.00% 98.52% -1.48%
===========================================
Files 9 10 +1
Lines 411 474 +63
===========================================
+ Hits 411 467 +56
- Misses 0 7 +7
Continue to review full report at Codecov.
|
| def test_gendigest_without_params(): | ||
| result = subprocess.run(["fastapi-security", "gendigest"], capture_output=True) | ||
| assert result.returncode == 2 | ||
| assert result.stdout.decode().splitlines() == [] | ||
| assert result.stderr.decode().splitlines() == [ | ||
| "usage: fastapi-security gendigest [-h] --salt SALT", | ||
| "fastapi-security gendigest: error: the following arguments are required: --salt", | ||
| ] |
There was a problem hiding this comment.
fastapi-security command is not found when I run poetry run pytest. Perhaps call cli.main directly like you've done in test_gendigest_smoke_test? E.g:
| def test_gendigest_without_params(): | |
| result = subprocess.run(["fastapi-security", "gendigest"], capture_output=True) | |
| assert result.returncode == 2 | |
| assert result.stdout.decode().splitlines() == [] | |
| assert result.stderr.decode().splitlines() == [ | |
| "usage: fastapi-security gendigest [-h] --salt SALT", | |
| "fastapi-security gendigest: error: the following arguments are required: --salt", | |
| ] | |
| def test_gendigest_without_params(capsys): | |
| with pytest.raises(SystemExit, match="2"): | |
| cli.main(["gendigest"]) | |
| captured = capsys.readouterr() | |
| assert captured.out == "" | |
| assert captured.err.splitlines() == [ | |
| "usage: fastapi-security gendigest [-h] --salt SALT", | |
| "fastapi-security gendigest: error: the following arguments are required: --salt", | |
| ] |
| main_parser = argparse.ArgumentParser( | ||
| description=_wrap_paragraphs(__doc__), | ||
| formatter_class=argparse.RawDescriptionHelpFormatter, | ||
| ) |
There was a problem hiding this comment.
prog needs to be added for unit test change suggestion I'm making below to work.
| main_parser = argparse.ArgumentParser( | |
| description=_wrap_paragraphs(__doc__), | |
| formatter_class=argparse.RawDescriptionHelpFormatter, | |
| ) | |
| main_parser = argparse.ArgumentParser( | |
| prog="fastapi-security", | |
| description=_wrap_paragraphs(__doc__), | |
| formatter_class=argparse.RawDescriptionHelpFormatter, | |
| ) |
| def test_usage_output_without_params(): | ||
| result = subprocess.run(["fastapi-security"], capture_output=True) | ||
| assert result.returncode == 2 | ||
| assert result.stdout.decode().splitlines() == [] | ||
| assert result.stderr.decode().splitlines() == [ | ||
| "usage: fastapi-security [-h] {gendigest} ...", | ||
| "fastapi-security: error: the following arguments are required: subcommand", | ||
| ] | ||
|
|
There was a problem hiding this comment.
To fix failing tests you could call cli.main directly as I've suggested below. Makes for faster tests as well.
There was a problem hiding this comment.
The failure was intentional, so to speak, but honestly I forgot about the pending change I intended to make. It is fixed now.
The problem is not in calling cli.main directly or not, but rather in that add_subparsers function only added required= kwarg in Python 3.7, so I wanted a test suite that would work with required=True in 3.7+ to then mimic the same behaviour (message and exit-code) for 3.6. See my most recent commit.
The reasons I went for subprocess approach in the first place are as follows:
- it does not require hard-coding the
prog=kwarg - it doubles as an integration test for the entire script (also testing that the entrypoint configuration is correct)
- it does not require catching a
SystemExitexception to ensure tests continue working
If it were up to me, I'd probably keep the existing tests as is, and use cli.main as soon as there are more unit tests to add. Although if you insist I can change go for cli.main in this PR as well.
There was a problem hiding this comment.
@immerrr Gotcha. Let's drop support for 3.6, it's reached end-of-life. You can include that change in your PR. Or if you'd rather I remove it and you rebase on top of my commit?
There was a problem hiding this comment.
Done (and reverted the change that enabled support for py3.6)
There was a problem hiding this comment.
Thanks @immerrr. Tests are still failing on Windows. And they also fail locally on my Mac OS. Fails to find command fastapi-security. I would suggest that you go the cli.main route.
| def test_usage_with_help_param(): | ||
| result = subprocess.run(["fastapi-security", "-h"], capture_output=True) | ||
| assert result.returncode == 0 | ||
| assert result.stdout.decode().splitlines() == [ | ||
| "usage: fastapi-security [-h] {gendigest} ...", | ||
| "", | ||
| "fastapi_security command-line interface", | ||
| "", | ||
| "positional arguments:", | ||
| " {gendigest} Specify a sub-command", | ||
| "", | ||
| "optional arguments:", | ||
| " -h, --help show this help message and exit", | ||
| ] | ||
| assert result.stderr.decode().splitlines() == [] |
This PR's goal is to enable storing password digests (instead of plaintext) to increase security.
It is admittedly a very early version aimed mostly at collecting feedback. I tried to introduce this with as little change as possible to the existing functionality to maintain backward compat, althought it is probably possible to add this to the
basic_authclass directly.I'm open to suggestions, and I have enabled edits by maintainers if you feel like applying some minor changes directly. Also, feel free to take this as a proof-of-concept, and implement it in a completely independent branch, that's absolutely fine by me.