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

Reorganize Debian packaging, have debhelper do most of the work #6544

Merged
merged 19 commits into from
Nov 1, 2022
Merged

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Sep 14, 2022

Status

Ready for review

Diffs

These are slightly out of date now, but gives a general idea of the expected changes.

  • securedrop-config, only metadata changes, version number now follows SecureDrop ("2.5.0")
  • securedrop-keyring, only metadata changes
  • securedrop-ossec-agent, mypy fixes plus metadata changes
  • securedrop-ossec-server, mypy fixes plus metadata changes
  • securedrop-app-code:
    • debdiff, shows removal of non-reproducible RECORD files, plus the addition of setup.py.
    • full diffoscope this is absurdly long because SOURCES.txt no longer contains every file in the package (it was entirely useless to begin with).
    • diffoscope without SOURCES.txt, shows removal of RECORD files, removal of debuginfo, and subtle changes to the binary .so files, because those are not reproducible (yet!).

Description of Changes

It's recommended to review this by stepping through the commits one-by-one. The first 7 commits are all cleanup.

Then the important commits:

And then the remaining stuff is just fixing lint issues and other minor things as a result of the changes.

Testing

Package building

  • Run make build-debs without this PR, save the debs somewhere ("originals")
  • Run make build-debs with this PR, with no errors
  • Run debdiff and/or diffoscope against the "originals" and the new packages, they should roughly match the diffs I posted above. Visual review that nothing that's needed is missing

Package testing

  • Install the packages on both an app and mon server (staging or physical, doesn't matter). No errors during installation
  • Basic testing of SecureDrop functionality that nothing is broken
  • Run verification tests from an admin workstation, they should pass

Deployment

Any special considerations for deployment?

Not really, diffoscope should help us feel confident about how minimal the changes really are.

Checklist

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

@legoktm legoktm force-pushed the hmmmmmm branch 2 times, most recently from 477c237 to fc27d84 Compare September 15, 2022 19:30
@legoktm legoktm force-pushed the hmmmmmm branch 3 times, most recently from c88032d to 3861573 Compare September 19, 2022 19:54
@legoktm legoktm marked this pull request as ready for review September 19, 2022 21:07
@legoktm legoktm requested a review from a team as a code owner September 19, 2022 21:07
@legoktm legoktm added this to the 2.6.0 milestone Sep 21, 2022
@legoktm
Copy link
Member Author

legoktm commented Sep 29, 2022

Notes for myself when I do the next rebase:

  • Make sure that i18n.json is included properly
  • Drop MANIFEST.in
  • Update update_version.sh (and grep for some more things that might be accessing those obsolete paths directly)

@cfm
Copy link
Member

cfm commented Oct 6, 2022

I've read through the commits (thanks for laying it out so narratively, @legoktm!) and think this looks great. The plan we've just discussed is that I'll aim to:

  1. step through the test plan, including diffing, next week, with the goal of approving without merging; and
  2. merge the week of October 17 after the SecureDrop 2.5.0 release, to avoid the risk of messy backports from develop while release/2.5.0 is still open.

Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

Diff-and-test plan looks great, with one necessary change requested inline. @legoktm, I'm pleased to be able to affirm, and I'd like to highlight to any other reviewers, your observation that

diffoscope should help us feel confident about how minimal the changes really are[.] (#6544 (comment))

This proposal builds (very nearly) the same packages we currently use, in a much more intuitive, Debian-idiomatic, and maintainable way.

Nit: This branch, with its arbitrary number of ms, is not very ergonomic to test. ;-)

Package building

  • Run make build-debs without this PR, save the debs somewhere ("originals")
  • Run make build-debs with this PR, with no errors
  • Run debdiff and/or diffoscope against the "originals" and the new packages, they should roughly match the diffs I posted above. Visual review that nothing that's needed is missing
    • securedrop-config, only metadata changes, version number now follows SecureDrop ("2.5.0")
    • securedrop-keyring, only metadata changes
    • securedrop-ossec-agent, mypy fixes plus metadata change
    • securedrop-ossec-server, mypy fixes plus metadata changes
    • securedrop-app-code:
      • debdiff, shows removal of non-reproducible RECORD files, plus the addition of setup.py.
      • full diffoscope this is absurdly long because SOURCES.txt no longer contains every file in the package (it was entirely useless to begin with).
      • diffoscope without SOURCES.txt, shows removal of RECORD files, removal of debuginfo, and subtle changes to the binary .so files, because those are not reproducible (yet!).
    • Also checked securedrop-grsec: unchanged according to debdiff; metadata-only changes according to Diffoscope

Package testing

  • Install the packages on both an app and mon server (staging or physical, doesn't matter). No errors during installation
root@sd-staging:~/securedrop# make staging

Required patch:

--- a/install_files/ansible-base/group_vars/securedrop_application_server.yml
+++ b/install_files/ansible-base/group_vars/securedrop_application_server.yml
@@ -6,9 +6,9 @@ ip_info:
 
 ### Used by the install_local_deb_pkgs role ###
 local_deb_packages:
-  - "securedrop-keyring-0.1.6+{{ securedrop_version }}+{{ securedrop_target_distribution }}-amd64.deb"
-  - "securedrop-config-0.1.4+{{ securedrop_version }}+{{ securedrop_target_distribution }}-amd64.deb"
-  - "securedrop-ossec-agent-3.6.0+{{ securedrop_version }}+{{ securedrop_target_distribution }}-amd64.deb"
+  - "securedrop-keyring_0.1.6+{{ securedrop_version }}+{{ securedrop_target_distribution }}_all.deb"
+  - "securedrop-config_{{ securedrop_version }}+{{ securedrop_target_distribution }}_all.deb"
+  - "securedrop-ossec-agent_3.6.0+{{ securedrop_version }}+{{ securedrop_target_distribution }}_all.deb"
   -  securedrop-grsec-{{ grsec_version }}+{{ securedrop_target_distribution }}-amd64.deb
   - "{{ securedrop_app_code_deb }}.deb"
   - "ossec-agent-3.6.0+{{ securedrop_target_distribution }}-amd64.deb"
diff --git a/install_files/ansible-base/group_vars/securedrop_monitor_server.yml b/install_files/ansible-base/group_vars/securedrop_monitor_server.yml
index 722fdc5ff..a5979b050 100644
--- a/install_files/ansible-base/group_vars/securedrop_monitor_server.yml
+++ b/install_files/ansible-base/group_vars/securedrop_monitor_server.yml
@@ -6,9 +6,9 @@ ip_info:
 
 ### Used by the install_local_deb_pkgs role ###
 local_deb_packages:
-  - "securedrop-keyring-0.1.6+{{ securedrop_version }}+{{ securedrop_target_distribution }}-amd64.deb"
-  - "securedrop-config-0.1.4+{{ securedrop_version }}+{{ securedrop_target_distribution }}-amd64.deb"
-  - "securedrop-ossec-server-3.6.0+{{ securedrop_version }}+{{ securedrop_target_distribution }}-amd64.deb"
+  - "securedrop-keyring_0.1.6+{{ securedrop_version }}+{{ securedrop_target_distribution }}_all.deb"
+  - "securedrop-config_{{ securedrop_version }}+{{ securedrop_target_distribution }}_all.deb"
+  - "securedrop-ossec-server_3.6.0+{{ securedrop_version }}+{{ securedrop_target_distribution }}_all.deb"
   - securedrop-grsec-{{ grsec_version }}+{{ securedrop_target_distribution }}-amd64.deb
   - ossec-server-3.6.0+{{ securedrop_target_distribution }}-amd64.deb
  • Basic testing of SecureDrop functionality that nothing is broken
  • Run verification tests from an admin workstation, they should pass
root@sd-staging:~/securedrop# make testinfra
[...]
    =========================== short test summary info ============================
    FAILED ../testinfra/common/test_automatic_updates.py::test_unattended_upgrades_functional[ansible:/mon-staging]
    FAILED ../testinfra/common/test_grsecurity.py::test_grsecurity_paxtest[ansible:/app-staging]
    = 2 failed, 469 passed, 13 skipped, 3 xfailed, 1 xpassed, 2 warnings in 38.24s =

Not worried about test_unattended_upgrades_functional. test_grsecurity_paxtest is failing with:

    E               AssertionError: Unexpected exit code 1 for CommandResult(command=b'sudo /bin/sh -c \'paxtest blackhat /tmp/paxtest.log | grep -P \'"\'"\'^(Executable|Return)\'"\'"\'\'', exit_status=1, stdout=None, stderr=b"Warning: Permanently added '192.168.121.150' (ECDSA) to the list of known hosts.\r\n/bin/sh: 1: paxtest: not found\n")
    E               assert 1 == 0
    E                 +1
    E                 -0
    
    ../testinfra/common/test_grsecurity.py:127: AssertionError

I'll see if I can reproduce on develop.

@cfm
Copy link
Member

cfm commented Oct 14, 2022

#6544 (review):

test_grsecurity_paxtest is failing with: [...] I'll see if I can reproduce on develop.

Nay; this passes on both fresh develop and fresh hmmmmmm, so this seems to be a flake after multiple make {build-debs,staging,testinfra} invocations across branches. An investigation for another day.

@legoktm
Copy link
Member Author

legoktm commented Oct 14, 2022

Pushed your patch (can rebase it into the stack post-review). The PR needs to be rebased (yay for strict yamllint validation), which I will do next week!

@cfm
Copy link
Member

cfm commented Oct 26, 2022

Thanks, @legoktm! Can you resolve the current conflicts for a final review and merge (barring feedback to the contrary from anyone else) on Thursday?

@legoktm
Copy link
Member Author

legoktm commented Oct 26, 2022

Done.

We don't actually run lintian right now. While I'd like to in the future,
these overrides are pretty outdated (e.g. it's fine to link GPL code with
OpenSSL now!) that there's no value in keeping them around.
It's AGPL, not GPL. Copy the copyright holders out of LICENSE and use the
correct dates.
These are blank and entirely unused.
Per the comment, this is apparently only needed for SecureDrop installs
that existed prior to 0.3.10. All running SecureDrops should have done a
clean reinstall during the focal migration, so this doesn't apply
anymore.
All of the actual changes are contained in `changelog.md` anyways.
Developers no longer use grsec kernels
These are all entirely no-ops and can be safely removed.
Copy link
Member

@cfm cfm 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 rebasing (and signing), @legoktm. I've pushed your latest head at c51c86f to stg-6544 to verify in staging-test-with-rebase the failure I see locally:

https://app.circleci.com/pipelines/github/freedomofpress/securedrop/5050/workflows/3d3b9a23-e26f-4c5b-a9e1-920bf2dcca13/jobs/65647?invite=true#step-104-107

$ make build-debs
[...]
    TASK [build-securedrop-app-code-deb-pkg : Build securedrop-app-code Debian package] ***
fatal: [focal-sd-app]: FAILED! => {"changed": true, "cmd": ["dpkg-buildpackage", "-us", "-uc"], "delta": "0:00:00.154732", "end": "2022-10-27 19:33:01.044973", "msg": "non-zero return code", "rc": 25, "start": "2022-10-27 19:33:00.890241", "stderr": " dpkg-source --before-build .\ndpkg-source: error: source package has two conflicting values - securedrop and securedrop-app-code\ndpkg-buildpackage: error: dpkg-source --before-build . subprocess returned exit status 25", "stderr_lines": [" dpkg-source --before-build .", "dpkg-source: error: source package has two conflicting values - securedrop and securedrop-app-code", "dpkg-buildpackage: error: dpkg-source --before-build . subprocess returned exit status 25"], "stdout": "dpkg-buildpackage: info: source package securedrop-app-code\ndpkg-buildpackage: info: source version 2.6.0~rc1+focal\ndpkg-buildpackage: info: source distribution focal\ndpkg-buildpackage: info: source changed by SecureDrop Team <securedrop@freedom.press>\ndpkg-buildpackage: info: host architecture amd64", "stdout_lines": ["dpkg-buildpackage: info: source package securedrop-app-code", "dpkg-buildpackage: info: source version 2.6.0~rc1+focal", "dpkg-buildpackage: info: source distribution focal", "dpkg-buildpackage: info: source changed by SecureDrop Team <securedrop@freedom.press>", "dpkg-buildpackage: info: host architecture amd64"]}

I can resolve this with the following patch, but I suspect you'll want to take a look at why the changelog rebased since the v2.5.0 release is failing in this way.

--- a/securedrop/debian/changelog
+++ b/securedrop/debian/changelog
@@ -1,4 +1,4 @@
-securedrop-app-code (2.6.0~rc1+focal) focal; urgency=medium
+securedrop (2.6.0~rc1+focal) focal; urgency=medium

   *

I'll block off time for another review on Monday. :-)

legoktm and others added 12 commits October 31, 2022 14:47
Instead of building each package separately and using ansible to
organize files and preprocess templates, use standard Debian
packaging techniques and have debhelper do the heavy lifting.

The packaging is now organized under `securedrop/debian/` and
is all built from one source package. In the future it should be
possible to use standard Debian build tools to build these
packages instead of needing ansible. I also hope that this makes
the actual build process easier to understand, for example, it's
clearer now that we're actually building all of the Python dependencies
from source *twice* (something to fix in a follow-up).

All jinja2 templates have been filled in (all the variables were
fixed/not changing anyways). Control files have been merged into
a single file, the version is now properly read out of the changelog.

Files are installed using `<package>.install`. For convenience, each
package has a folder that matches the hierarchy of how things should
be installed.

The remaining logic still in ansible is compiling of translations, which
is migrated in the next commit for ease of review.

I did not adjust the packaging for the securedrop-grsec metapackage
since it's already being moved into the kernel-builder repository.
Same with the ossec-agent and ossec-server packages, since those are
upstream software they should probably end up in their own repository.

All of the packages except securedrop-app-code are now bit-for-bit
reproducible thanks to all the magic that debhelper does!
The RECORD files are not reproducible. Instead of trying to fix them
up with some sed invocation, we can just delete them, like we do in
all the SecureDrop Workstation packages.

Then there are some known reproducibility issues[1] with the debug
info in the built C extensions. However, the suggested fix of
fixing the file path didn't work because virtualenv uses its own
random file paths that we can't map. In any case, we can just
strip the debug info to remove that source of variance.

This leaves us with only the build ID varying, which is caused by
the same random build path issue. Fixing that will require us to
have reproducible wheels, which is complicated enough that it will
be attempted in a separate PR.

[1] https://reproducible-builds.org/docs/build-path/
Port the ansible translations tasks to a bash script that
debian/rules will invoke prior to the dh_install step.
The file is identical in both securedrop-ossec-agent and
securedrop-ossec-server, so just keep one copy of the script and
install it in both packages.
These packages all contain no architecture-specific code, so they
should be "all". In any case, the difference is mostly meaningless
for our usecase since we only support amd64 anyways.
This was previously used by the packaging process to partially control
what files would be included in the sdist build before further pruning
via the now-removed rsync-filter list.

Given that we don't distribute sdist builds of SecureDrop, only
Debian packages and the Git repo itself, there's no use for this
anymore.
@legoktm
Copy link
Member Author

legoktm commented Oct 31, 2022

Bah, messed up in the rebase. The source package is now named "securedrop", so the changelog entry is supposed to start with that.

@legoktm
Copy link
Member Author

legoktm commented Nov 1, 2022

I also pushed it to the https://github.com/freedomofpress/securedrop/tree/stg-6544 branch, GitHub is smart enough to show the passing staging-test-with-rebase job on this PR too :)

Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

The rebase-level diff looks good, so I'm approving based on this final review (below). Thanks so much for your direction and work on this, @legoktm, and for your patience with this back-and-forth review!

Package building

#6544 (review)

Package testing

  • Install the packages on both an app and mon server (staging or physical, doesn't matter). No errors during installation
root@sd-staging:~/securedrop# make build-debs
root@sd-staging:~/securedrop# make staging
  • Basic testing of SecureDrop functionality that nothing is broken
  • Run verification tests from an admin workstation, they should pass
root@sd-staging:~/securedrop# make testinfra
[...]
    ====== 471 passed, 13 skipped, 3 xfailed, 1 xpassed, 2 warnings in 48.06s ======
Verifier completed successfully.

@cfm cfm merged commit 6170370 into develop Nov 1, 2022
@eaon eaon deleted the hmmmmmm branch November 1, 2022 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