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

Vendor pretty_bad_protocol #6836

Merged
merged 11 commits into from
Jun 21, 2023
Merged

Vendor pretty_bad_protocol #6836

merged 11 commits into from
Jun 21, 2023

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Jun 6, 2023

Status

Ready for review

Description of Changes

Vendor pretty_bad_protocol and apply minimal fixes to get it to pass CI. Then remove our monkey patching from EncryptionManager.

Each commit should have a description of what is happening and why.

Fixes #6807.

Testing

How should the reviewer test this PR?

  • CI passes
  • Basic smoke test in dev environment, submit a message/file, and then decrypt it as a journalist
  • Basic smoke test in a staging/prod environment (to verify apparmor, etc.), submit a message/file, and then decrypt it as a journalist

Deployment

Any special considerations for deployment? Just the standard-ish ones.

Checklist

  • Linting (make lint) and tests (make test) pass in the development container
  • I have updated AppArmor rules to include the change
  • I have written a test plan and validated it for this PR
  • These changes do not require documentation

@legoktm legoktm force-pushed the vendor-pbp branch 3 times, most recently from 1e780bc to fa00437 Compare June 12, 2023 19:12
legoktm added 10 commits June 12, 2023 15:33
pretty_bad_protocol is unmaintained upstream, not seeing any commits
since the 3.1.1 release in August 2018. As part of our shift to Sequoia,
we will just need a small part of this library during the migration, so
let's fork/vendor it and remove the parts we don't need. This will also
let us get rid of the monkey-patching that's accumulated over the years.

This is a direct copy of the 3.1.1 source tree:
 $ wget https://files.pythonhosted.org/packages/84/0d/814c6c96f64f9cfc235fe102024b00ee77d107977e32996c59aed8f27ec0/pretty-bad-protocol-3.1.1.tar.gz
 $ tar xvf pretty-bad-protocol-3.1.1.tar.gz
 $ cp -Rv pretty-bad-protocol-3.1.1/pretty_bad_protocol freedomofpress/securedrop/securedrop/

Follow-up commits will reformat it per our coding standards and other
necessary fixes.

Refs #6807.
First pass in cleaning up outdated things.
Remove code to support Python 2, Windows and old GPG versions.
focal ships with GPG 2.1+, so we don't need to bother checking for older
versions.
* ansistrm is only used if logging is enabled, but we never enable it
  (since SecureDrop has its own logger) so deleted entirely.
* Fix undefined variable `message` in error case.
* Delete insecure, broken and unused _make_passphrase().
* Raise ValueError instead of incorrect.
* Inline _version.py's version number.
Mostly not using "not in" properly and double hash ("##") for comments
We don't need this information available at runtime, so just stick it in
a documentation file.
I suspect there's more that we can delete out of pretty_bad_protocol in
the future, so for now just make mypy happy by suppressing lack of type
information.

In some places I inlined or deleted helpers that no longer make sense to
have in a Python 3-only world.
The use of a fixed /tmp directory is bad, but a pre-existing issue
so we can fix it later after investigating whether we actually use
this code path or not.
And just implement them in the code directly now. We still set
the `USERNAME` environment variable via encryption.py since there's
not really a logical place for it in pretty_bad_protocol.

Fixes #6807.
@legoktm legoktm marked this pull request as ready for review June 12, 2023 19:38
@legoktm legoktm requested a review from a team as a code owner June 12, 2023 19:38
@legoktm
Copy link
Member Author

legoktm commented Jun 12, 2023

I still need to verify this in staging, but everything else seems to check out.

@legoktm
Copy link
Member Author

legoktm commented Jun 12, 2023

okay it's broken in staging/prod >.> debugging...

@legoktm
Copy link
Member Author

legoktm commented Jun 13, 2023

okay it's broken in staging/prod >.> debugging...

I had forgotten to add the new folder to d/securedrop-app-code.install so it wasn't even in the package. Fixed now and looks good on staging.

@legoktm
Copy link
Member Author

legoktm commented Jun 13, 2023

Jun 13 19:30:19 app kernel: [55802.309949] audit: type=1400 audit(1686684619.955:22): apparmor="DENIED" operation="open" profile="/usr/sbin/apache2" name="/var/www/securedrop/pretty_bad_protocol/" pid=135658 comm="apache2" requested_mask="r" denied_mask="r" fsuid=33 ouid=0
Jun 13 19:30:19 app kernel: [55802.322290] audit: type=1400 audit(1686684619.967:23): apparmor="DENIED" operation="open" profile="/usr/sbin/apache2" name="/var/www/securedrop/pretty_bad_protocol/" pid=135659 comm="apache2" requested_mask="r" denied_mask="r" fsuid=33 ouid=0

stupid me, I didn't add the directory itself to apparmor, just the individual files. Really should get around to scripting this...

Include the newly added folder in the package and to the AppArmor list.
@eaon eaon self-assigned this Jun 20, 2023
Copy link
Contributor

@eaon eaon left a comment

Choose a reason for hiding this comment

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

I did not expect vendoring pbp to feel good, but this feels good. Great work!

@eaon eaon merged commit 2276721 into develop Jun 21, 2023
@eaon eaon deleted the vendor-pbp branch June 21, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Vendor pretty-bad-protocol and strip it down to what is needed
2 participants