Changes related to Debian packaging work #86

kushaldas · 2018-10-30T14:32:58Z

Now we have securedrop-client script, which will execute the steps required to run the application following all standard default paths (from the Debian package).

We are trying to apply any alembic revision at the start of the tool, that way, when ever the user restarts the application, it will have the right database structure.

One can improve the ./run.sh in future. For now I kept it as simple as possible.

How to test?

./run.sh to use the tool like previously and connect to the server using web API
Inside of qubes and a proxy vm, use ./run.sh -p to connect using the Proxy VM.

heartsucker · 2018-10-30T14:35:52Z

run.sh

@@ -16,14 +16,20 @@ while [ -n "$1" ]; do
 done

 SDC_HOME=${SDC_HOME:-$(mktemp -d)}
+DB_HOME=$SDC_HOME/data


This logic should be kept inside the app. There should only be a single injection of SDC_HOME, and the app should figure everything else out from there.

This helps to make sure that I am creating the directory structure properly, as I need that directory structure to create the DB file.

All of this logic is inside the app. Doing it in two places runs the risk of us relying on something we shouldn't.

heartsucker · 2018-10-30T14:37:09Z

run.sh

-from securedrop_client.models import Base, make_engine
-Base.metadata.create_all(make_engine("$SDC_HOME"))
-EOF
+./createdb.py $DB_HOME


I think it's fine to leave this as a here-doc since run.sh is a dev-only tool to help people with launching the app while iterating. Additionally, this doesn't solve the "bootstrap at first run" ticket.

Bootstrapping in production is handled by the securedrop-client script.

heartsucker · 2018-10-30T14:39:08Z

alembic/env.py

@@ -16,6 +16,8 @@
 # add your model's MetaData object here
 # for 'autogenerate' support
 sys.path.insert(0, path.realpath(path.join(path.dirname(__file__), '..')))
+# This path is purely for alembic to work on the packaged application
+sys.path.insert(1, "/opt/venvs/securedrop-client/lib/python3.5/site-packages")


Why is this necessary? Shouldn't we keep the alembic directory in a fix position relative to the app? The first path insertion should catch every case, no?

Because this is how it can help in packaging. We need static config files to able to package them easily and without any dynamic magic at runtime.

I still don't understand. What is the exact case that you're describing?

The first path is for the development environment, and the second path is for the case where we are running using Debian package. The alembic directory is installed in the standard /usr/share/securedrop-workstation/ location.

Why isn't the alembic directory placed somewhere relative to the source code? Wouldn't that be simpler?

heartsucker · 2018-10-30T14:40:31Z

files/alembic.ini

+
+[alembic]
+# path to migration scripts
+script_location = /usr/share/securedrop-client/alembic


If we're going to be running alembic, we shouldn't be using a hard coded value you like this. We should use a template and a %%ALEMBIC_PATH%% or something similar to replace the values in a pre-alembic step.

This is to help the Debian packaging. All packaged applications (as far as I know) in Fedora land also uses the same idea.

Right, but the alembic.ini is useless without this line

sqlalchemy.url = sqlite:////path/to/database.sqlite

The app at boot could pull an alembic.ini.tmpl from /var/lib/ or /usr/share and then inject that value before running alembic upgrade head.

It is here for the default path in the Debian package https://github.com/freedomofpress/securedrop-client/pull/86/files#diff-3f5cd7c244dcadbe672e3644a1f283bdR38

heartsucker · 2018-10-30T14:40:46Z

files/client.ini

@@ -0,0 +1,3 @@
+[client]
+homedir=~/.securedrop_client
+use_securedrop_proxy=True


Nit: missing newline at EOF

Yup, will add this.

I will wait for all the other issues to be discussed and resolved, I will fix at the very end :)

heartsucker · 2018-10-30T14:43:02Z

securedrop_client/app.py

@@ -104,6 +105,9 @@ def arg_parser() -> ArgumentParser:
        type=expand_to_absolute,
        help=('SecureDrop Client home directory for storing files and state. '
              '(Default {})'.format(DEFAULT_SDC_HOME)))
+    parser.add_argument(
+        '-p', '--proxy', action='store_true',


I'm somewhat not a fan of using single letters for boolean values because of the chances for confusion.

I can remove that.

heartsucker · 2018-10-30T14:43:17Z

securedrop_client/app.py

@@ -120,6 +124,7 @@ def start_app(args, qt_args) -> None:
    - configure the client (logic) object.
    - ensure the application is setup in the default safe starting state.
    """
+    print(args.proxy)


Looks like you left a debug print here.

Yup, will be removed.

heartsucker · 2018-10-30T14:44:05Z

securedrop_client/app.py

@@ -150,7 +156,14 @@ def start_app(args, qt_args) -> None:


 def run() -> None:
+    config_file = "/etc/securdrop-client/client.ini"


We should not be configuring from /etc. Configs should be at the root of sdc_home.

We should configure with /etc/ as that will be useful to update through the Debian package. The package will be installed at the template, and we should have the default values already in the config for the same.

I'm a little confused on the goals then. Is the securedrop-client so hyper targeted toward Qubes that we aren't going to work in having it operate anywhere else ever? Or should we use some patterns that allow us to be able to run it in other linux-y environments? Because what I don't like about SecureDrop core is that the Debian packages are useless without running ansible first. IMO configuration should be entirely outside the Debian side and use defaults in the Python code to handle the no-config cases.

heartsucker · 2018-10-30T14:44:52Z

securedrop_client/app.py

+            os.path.exists(config_file):  # pragma: no cover
+        config = configparser.ConfigParser()
+        config.read(config_file)
+        args.sdc_home = config["client"]["homedir"]


Why are you overriding the sdc_home after reading a config from sdc_home?

I am reading the config from /etc/securdrop-client/client.ini file.

Ok I still think this make no sense. The client is passed a sdc_home, then we read another value, then we sed that other value to sdc_home. I feel like this isn't a good pattern and is kind confusing.

heartsucker · 2018-10-30T14:46:39Z

securedrop_client/app.py

+        config = configparser.ConfigParser()
+        config.read(config_file)
+        args.sdc_home = config["client"]["homedir"]
+        args.proxy = config["client"]["use_securedrop_proxy"]
    # reinsert the program's name
    qt_args.insert(0, 'securedrop-client')


More generally, I think that the CLI parsing should all happen outside the app, and then we pass those as args to start_app (or run_app, whatever the name was). In side there, we handle the logic of how to include values from the configs. I think reusing the Namespace from argparse as the "master config" seems wrong. We might want to have a Config class that does all this logic.

I am going to leave this for a future PR.

kushaldas · 2018-10-30T16:59:50Z

Notes from the video call with @heartsucker .

@heartsucker's point of view

Having any configuration file under /etc/ means dpkg will try to merge it in case of updates, and there is a chance of failure. That is why he suggested to keep configuration files outside of /etc/.
He is also okay to have a sane default application where it will think that it is running under Qubes with all default values and will not require any configuration file.

@kushaldas's point of view

I thought (as a packager) that any configuration should be under /etc/, that way the system package management tool will take care of it. I am also totally okay with a sane default application which will just work thinking that it is running inside of Qubes.

Another option is to keep alembic.ini or any other such configuration file under /usr/share/securdrop-client/ directory and the tools to keep using those paths as default values. The + point in this is that the application will always use the version of config file which came with the Debianpackage.

redshiftzero · 2018-10-31T02:00:24Z

run.sh

+mkdir -p $LOGS_HOME
+chmod 0700 $SDC_HOME
+chmod 0700 $DB_HOME
+chmod 0700 $LOGS_HOME


again isn't this also done in the app itself?

Because we want to have the db file as ~/.securedrop_client/data, if we have it under ~/.securedrop_client/ then I can remove these lines (keeping the lines only for $SDC_HOME). As the database must come up (using Alembic) before the application starts.

true, we gotta create the database before starting the application - but see my comment here, we can at least delegate these folder creation to the application (the less logic we have duplicated the better)

redshiftzero · 2018-10-31T02:04:25Z

securedrop_client/logic.py

@@ -85,7 +85,8 @@ class Client(QObject):

    finish_api_call = pyqtSignal()  # Acknowledges reciept of an API call.

-    def __init__(self, hostname, gui, session, home: str) -> None:
+    def __init__(self, hostname, gui, session,
+                 home: str, proxy: bool = False) -> None:


proxy should be True if the default application is Qubes first

redshiftzero · 2018-10-31T02:13:40Z

files/alembic.ini

+# are written from script.py.mako
+# output_encoding = utf-8
+
+sqlalchemy.url = sqlite:////home/user/.securedrop_client/data/svs.sqlite


note right now the db path is:

/home/user/.securedrop_client/svs.sqlite

(which I think is actually good as the data directory contains only downloaded messages / files)

Ah, I misread this line

~/.securedrop_client/data -> config, database, files (submissions, messages)

#2 (comment)

So, I will modify to change the path.

sorry that's my bad :-(

kushaldas · 2018-10-31T06:24:00Z

Updates config locations and making proxy default for Client

Now we have all the configurations under
/usr/share/securedrop-client/ including the alembic.ini file.

The database file is now located at ~/securedrop_client/

Based on the PR review comments from @heartsucker @redshiftzero

redshiftzero

I was able to successfully build a deb package on this, thanks for this @kushaldas and for making the changes!

I will flag that in the coming weeks we do need to invest some time into build automation, as building deb packages manually like I just did is a bit tedious and error-prone

anyway, for now I think we should continue onwards - @heartsucker - even if you have no blocking issues here can you file followups for anything else that you think should be addressed/discussed packaging wise for beta? (reason is that beta is when this will get installed in news orgs so we really need to have anything that impacts the long term maintenance plan nailed down - if all goes well we'll be supporting those beta installations for maybe years)

heartsucker · 2018-11-01T16:15:18Z

Just got off a call with Jen were were talked about this PR. Here's my issues.

the alembic migrations should be in a fixed relative path to the source code
no configs, even defaults should be shipped with the packages
- however, templates could/should be shipped
configs (config.json or alembic.ini) need to be in the sdc_home directory

The only config that would need to be shipped with the package so far is the alembic.ini, and this can be rendered to the sdc_home. We already do somthing like that here, and we could probably just copy the code completely.

To auto-handle configs and load a local config if present, we should probably have a Config class that does the default handling / Qubes detection. This should be injected into the app / logic / controller and passed around that way. Def do not want a global reference (because testing).

If we have a "master" config in /var/lib or /usr/share or where ever*, then we have configs and data in many places, and IMO this should all be in sdc_home. It also raises the problem that we are foreced to always use the known home /root/.securedrop-client because alembic will have to have a hardcoded value in it's alembic.ini. If we want to be able to debug / test on a workstation or just keep with the the current pattern of configuration and isolation, then we need to have a per-sdc_home config.

* please not /etc/ because these "configs" are actually hardcoded and shouldn't be edited by a user, and they need to be updated by dpkg without intervention

Last, I don't want to have to rely on ansible or salt to manage configs. I know we've kinda discussed "config-less", and I really think that that also means not needing ops tools to get it to work. I don't want to have us end up how we have in SecureDrop core where a whole bunch of would-be-nice changes are blocked by not wanting to make an admin run the ops tools to push out the changes we need.

Addressing Jen's concern, there should be a Makefile target called debian that Just Works, and this should be part of CI.

heartsucker · 2018-11-01T18:16:53Z

One thing. Can you remove the fixes #31 line, because this doesn't call alembic upgrade head anywhere, so we're not actually bootstrapping the DB. That ticket needs to stay open.

edit: Just kidding, ignore this.

heartsucker · 2018-11-01T18:18:30Z

files/client.ini

@@ -0,0 +1,3 @@
+[client]


Can we use .json as the config type because it allows for deeper nesting, lists, and also ops tools have nice functions for mangling/merging JSON?

can we stick with ini for simplicity for now?

heartsucker · 2018-11-01T18:20:25Z

securedrop_client/app.py

+            os.path.exists(config_file):  # pragma: no cover
+        config = configparser.ConfigParser()
+        config.read(config_file)
+        args.sdc_home = config["client"]["homedir"]


Ok I still think this make no sense. The client is passed a sdc_home, then we read another value, then we sed that other value to sdc_home. I feel like this isn't a good pattern and is kind confusing.

kushaldas · 2018-11-01T18:33:02Z

securedrop_client/app.py

+            os.path.exists(config_file):  # pragma: no cover
+        config = configparser.ConfigParser()
+        config.read(config_file)
+        args.sdc_home = config["client"]["homedir"]


This is done to make sure that we use the whatever values are there in the config file. This part is to be used only after installing via debian package, and we can improve this in future.

We have a hardcoded home directory in Qubes, so we're using that hard coded value to read a hardcoded config to then switch the home to something else hardcoded. This seems like a lot of indirection.

Dismissing review to unblock. I'm generally fine with this, for the alpha, but there's things I still think should be changed.

heartsucker · 2018-11-01T18:52:41Z

createdb.py

+import sys
+
+from securedrop_client.models import Base, make_engine
+Base.metadata.create_all(make_engine(sys.argv[1]))


I think the comment got lost, but I still think this is better as a here-doc since it's just for the dev env and it's simple enough to just shove into the bash script.

We need these config files for the Debian packaging. The securedrop-client script will make sure that we have the right database structure, or will apply the migration as required. We will need a future PR to fix ./run.sh to work with the changes related to Debian packaging.

We are now using the similar directory structure in run.sh and also in securdrop-client. We are also using the createdb.py to create the db in the local environment.

Use ./run.sh --proxy to connect to the proxy vm.

Now we have all the configurations under /usr/share/securedrop-client/ including the alembic.ini file. The database file is now located at ~/securedrop_client/ Based on the PR review comments.

redshiftzero · 2018-11-01T21:51:16Z

hey @heartsucker, I've filed #97 and added it to the beta milestone so we can continue this discussion there, and please feel free to open other issues as followups to this. For now, merging so that we resolve the must-do issues for alpha

Add link to Code of Conduct to README

kushaldas requested review from joshuathayer, heartsucker and redshiftzero October 30, 2018 14:32

heartsucker previously requested changes Oct 30, 2018

View reviewed changes

kushaldas force-pushed the packaging_db_config branch from a9c9e5c to 1dd9570 Compare October 30, 2018 15:09

kushaldas mentioned this pull request Oct 30, 2018

Create Debian package #22

Closed

redshiftzero reviewed Oct 31, 2018

View reviewed changes

kushaldas force-pushed the packaging_db_config branch from e2d3aca to 187c5be Compare October 31, 2018 06:23

redshiftzero approved these changes Nov 1, 2018

View reviewed changes

heartsucker reviewed Nov 1, 2018

View reviewed changes

kushaldas commented Nov 1, 2018

View reviewed changes

heartsucker reviewed Nov 1, 2018

View reviewed changes

kushaldas added 8 commits November 2, 2018 00:37

Keeps setup.py outside of codestyle checks

dbdc501

Adds setup.py and MANIFEST.in files

fe4ea9b

Updates the required versions based on upstream releases

68dc14f

Adds path for the debian package in sys.path

8777d8a

Adds createdb.py and updates run.sh to work for local instance

ca1c21b

We are now using the similar directory structure in run.sh and also in securdrop-client. We are also using the createdb.py to create the db in the local environment.

The Debian package tool now uses proxy by default

f52192b

Use ./run.sh --proxy to connect to the proxy vm.

Cleans up the default client config

a3d0d72

Updates config locations and making proxy default for Client

c331034

Now we have all the configurations under /usr/share/securedrop-client/ including the alembic.ini file. The database file is now located at ~/securedrop_client/ Based on the PR review comments.

kushaldas force-pushed the packaging_db_config branch from 187c5be to c331034 Compare November 1, 2018 19:09

redshiftzero mentioned this pull request Nov 1, 2018

iterate on config and packaging logic #97

Closed

redshiftzero merged commit 2b2f6fc into master Nov 1, 2018

redshiftzero deleted the packaging_db_config branch November 1, 2018 21:52

This was referenced Nov 1, 2018

Move alembic directory #30

Closed

Consider how to bootstrap the database at first run #31

Closed

legoktm pushed a commit that referenced this pull request Dec 11, 2023

Merge pull request #86 from freedomofpress/add-code-of-conduct

4a907ce

Add link to Code of Conduct to README

		@@ -150,7 +156,14 @@ def start_app(args, qt_args) -> None:


		def run() -> None:
		config_file = "/etc/securdrop-client/client.ini"

Changes related to Debian packaging work #86

Changes related to Debian packaging work #86

Conversation

kushaldas commented Oct 30, 2018

How to test?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heartsucker Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kushaldas commented Oct 30, 2018

@heartsucker's point of view

@kushaldas's point of view

Choose a reason for hiding this comment

kushaldas Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kushaldas commented Oct 31, 2018

redshiftzero left a comment

Choose a reason for hiding this comment

heartsucker commented Nov 1, 2018

heartsucker commented Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redshiftzero commented Nov 1, 2018

heartsucker Oct 30, 2018 •

edited

Loading

kushaldas Oct 31, 2018 •

edited

Loading

heartsucker commented Nov 1, 2018 •

edited

Loading