Feature #5894

APT repository: notify incoming

Added by Tails about 4 years ago. Updated 26 days ago.

Status:In ProgressStart date:
Priority:NormalDue date:
Assignee:bertagaz% Done:

80%

Category:Infrastructure
Target version:Tails_3.2
QA Check:Dev Needed Blueprint:
Feature Branch:puppet-tails:feature/5894-reprepro-notify-incoming-changes Easy:Yes
Type of work:Sysadmin Affected tool:

Description

Tell reprepro to email what processincoming does, probably using reprepro hooks: file:///usr/share/doc/reprepro/manual.html#hooks, or piggy-backing on the existing inotifyincoming system (https://gitlab.com/shared-puppet-modules-group/reprepro/blob/master/files/inoticoming.init, https://gitlab.com/shared-puppet-modules-group/reprepro/blob/master/templates/inoticoming.default.erb).

Ideally, the granularity of change notification should be uploading a .changes file (which dput/dupload do after they've finished uploading the files referenced in the .changes file).

The initial research can be done without any special setup: one needs reprepro with basic configuration and running reprepro processincoming.

The script/hook that emails changes should allow customizing the destination email address without modifying the code (command line argument, preferably).

Regarding implementation language: Python, Ruby or Modern Perl; but very good shell might be OK. The code should be defensive enough: assume the input is untrusted.

Then, it should be integrated in:

Sources of inspiration:

Now, regarding security, this notification system can be bypassed by developers with SSH access to the reprepro account: they can run arbitrary reprepro commands to modify the contents of our repository. To bring this to the next level, we need to limit their access to scp'ing files to the incoming directory (likely with sftp only accounts or similar, with adequate ownership/permissions or chroot'ing to ensure they cannot modify the other repository files directly). Once the notification system is in place, a new ticket must be created to track these next steps.

changenote.tar (10 KB) groente, 04/11/2017 12:14 PM

reprepro-notify-incoming-changes.tar (10 KB) groente, 05/01/2017 09:52 AM

reprepro-notify-incoming-changes (904 Bytes) groente, 06/12/2017 01:34 PM


Related issues

Blocks Tails - Feature #13233: Core work 2017Q3: Sysadmin (Maintain our already existing services) Confirmed 06/29/2017

History

#1 Updated by intrigeri almost 4 years ago

  • Category set to Infrastructure
  • Easy set to No

#2 Updated by intrigeri over 3 years ago

  • Easy changed from No to Yes

#3 Updated by intrigeri over 3 years ago

  • Description updated (diff)

#4 Updated by intrigeri about 3 years ago

  • Description updated (diff)

#5 Updated by intrigeri 4 months ago

  • Description updated (diff)

#6 Updated by intrigeri 4 months ago

  • Description updated (diff)

#7 Updated by intrigeri 4 months ago

  • Description updated (diff)

#8 Updated by intrigeri 4 months ago

  • Description updated (diff)

#9 Updated by groente 4 months ago

Here's a unitfile, defaults file and a little script that should get the job done, provided inoticoming and an mta are installed on the system.

#10 Updated by intrigeri 4 months ago

  • Status changed from Confirmed to In Progress
  • Assignee set to intrigeri
  • % Done changed from 0 to 10
  • QA Check set to Ready for QA

#11 Updated by intrigeri 3 months ago

  • Assignee changed from intrigeri to groente
  • % Done changed from 10 to 20
  • QA Check changed from Ready for QA to Dev Needed

Here's a unitfile, defaults file and a little script that should get the job done, provided inoticoming and an mta are installed on the system.

Thanks! Here's an initial review. I don't expect you to implement all the changes I suggest, if you can state good reasons why things are done in the way they currently are: this is a conversation, not me giving orders :)

  • This looks good. In particular, it's good that you used parameters for things that may change depending on the deployment environment.
  • Please use /bin/sh unless there's a good reason to use bashisms in the script (and thus /bin/bash in the shebang).
  • Please use set -eu that help detect many small programming or configuration issues, e.g. $MAILTO being unset.
  • It would be nice if the script had some basic sanity checks for all variables it expects, e.g. [ -n "$MAILTO" ].
  • Can't $MAIL be quoted in echo -e $MAIL? (I'm unsure about the interaction with echo -e, it might be that double-quotes wouldn't work as I expect, but if they do, then please add them.)
  • I'd rather not introduce a new manually managed log file (whose content is not in the systemd Journal, and that would need a new logrotate config snippet). Printing to stdout should be enough (thanks to --foreground) to have logs included in the systemd Journal.
  • I suggest renaming changenote.sh to reprepro-notify-incoming-changes, and similarly changenote.service to reprepro-notify-incoming-changes.service: more explicit names make things easier to track and find.
  • What's the purpose of Alias=inoticoming.service?
  • Have you verified that two instances of inoticoming that monitor the same directory and suffix both trigger actions when a file lands in the monitored directory? I would hope so, but it would be nice to be 100% sure it works :)
  • We added support to the reprepro Puppet module for managing multiple APT repositories, so ideally the systemd unit would be a template one, instantiated for each managed APT repo: see "Optionally, units may be instantiated from a template file at runtime" in systemd.unit(5). But we don't need that yet, so take this as a mere suggestion, and work on it only if you are excited at the idea of learning more about systemd :) I'd rather see you first polish the current version so that we can deploy it in production, and then work on this later if/whenever you feel like it.

#12 Updated by groente 3 months ago

Here's an updated version, renamed to reprepro-notify-incoming-changes:
- added some sanity checks
- replaced /bin/bash by /bin/sh and replaced the echo's with printf to avoid troubles with (non-)posix-compliant shells
- replaced writing to a logfile with writing to stdout
- removed the Alias from the unit file.

Two instances can indeed monitor the same directory without interfering with eachother.

#13 Updated by groente 3 months ago

  • QA Check changed from Dev Needed to Ready for QA

#14 Updated by intrigeri 3 months ago

  • Target version set to Tails_3.0~rc1

#15 Updated by intrigeri 2 months ago

  • Target version changed from Tails_3.0~rc1 to Tails_3.0

Sorry!

#16 Updated by intrigeri about 2 months ago

  • Assignee changed from intrigeri to bertagaz

I've asked bertagaz over email to take over the review of one of #5894 and #11523; I think this one will probably be the easiest for him to grasp so I'll focus on #11523 myself. You'll want to read the previous review round comments above. Once happy, someone will have to Puppet-ize and deploy this, so we can evaluate how it fares in the real world; I can do it if you don't want to :)

#17 Updated by bertagaz about 2 months ago

intrigeri wrote:

I've asked bertagaz over email to take over the review of one of #5894 and #11523; I think this one will probably be the easiest for him to grasp so I'll focus on #11523 myself.

Thanks! Sorry groente for the delay in the reviewing. I'm quite busy with the changes in the build system, but I hope to be able to review this ticket at the end of the week, or next week if I don't manage to do it before.

#18 Updated by bertagaz about 1 month ago

  • Assignee changed from bertagaz to groente
  • Target version changed from Tails_3.0 to Tails_3.1
  • QA Check changed from Ready for QA to Dev Needed

bertagaz wrote:

Thanks! Sorry groente for the delay in the reviewing. I'm quite busy with the changes in the build system, but I hope to be able to review this ticket at the end of the week, or next week if I don't manage to do it before.

Had time to look at this and test it. I have only a few remarks:

  • Please don't always use the same exit code in the variables sanity checks. We usually use one for each (> 1) and increment. It helps a bit to debug what went wrong.
  • Why are you adding ';' at the end of each lines inside this if ... fi?
  • Please quote the variables you're testing there also.

Other than that looks good! I've tested it locally and it works fine, including when several services are watching the same directory indeed. I'll puppetize this once you'll have fixed what's been raised above, and will point you to this puppet code.

#19 Updated by groente about 1 month ago

Thanks! Attached is an updated version of the script with incremental exit codes, less semicolons (that was just force of habit), and some extra quotes.

#20 Updated by groente about 1 month ago

  • Assignee changed from groente to bertagaz
  • % Done changed from 30 to 40

#21 Updated by intrigeri about 1 month ago

  • QA Check changed from Dev Needed to Ready for QA

(Adjusting metadata accordingly.)

#22 Updated by bertagaz about 1 month ago

  • QA Check changed from Ready for QA to Dev Needed

groente wrote:

Thanks! Attached is an updated version of the script with incremental exit codes, less semicolons (that was just force of habit), and some extra quotes.

Ok, that's better! Note that you've set the exit codes starting from "1", where I asked them to be "> 1", but that's fine, I'll adapt them when I'll import your code. Just wanted you to know for the next time. :)

I'll puppetize this later, and will point you to the puppet code.

#23 Updated by intrigeri about 1 month ago

where I asked them to be "> 1", but that's fine,

Just curious: why?

#24 Updated by intrigeri about 1 month ago

  • Priority changed from Normal to Elevated

The earlier this is done, the better wrt. the process we're in with groente.

#25 Updated by bertagaz about 1 month ago

intrigeri wrote:

The earlier this is done, the better wrt. the process we're in with groente.

True! As stated on the list, I'll take care of that in the coming days.

#26 Updated by bertagaz 29 days ago

  • Assignee changed from bertagaz to intrigeri
  • % Done changed from 40 to 60
  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch set to puppet-tails:feature/5894-reprepro-notify-incoming-changes

intrigeri wrote:

where I asked them to be "> 1", but that's fine,

Just curious: why?

Because other commands in the script (like the printf one in this case) may return "1" if they fail, so it does not mix the exit code we set up with the one that may arise from other commands (even if I know that this commands may also return something else).

Anyway, I've just pushed commit 65811f3 and commit 989ec075 in a dedicated branch. Did not test or deploy it yet, but I believe it's ready to be reviewed now.

#27 Updated by intrigeri 28 days ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Dev Needed

Looks good, except:

  • mail_to => 'tails-sysadmins@boum.org' ← uh, why would sysadmins be responsible for monitoring what's essentially code changes? I would says RMs to start with, and possibly tails-dev@ later once we know it works fine.
  • mail_from => 'reprepro@apt.lizard': won't the internal FQDN make the email be rejected by the destination MTA?

Feel free to fix those and then deploy directly at a time when you're ready to 1. immediately test that it indeed works as expected; 2. deal with the fallout (if any :)

#28 Updated by bertagaz 28 days ago

  • Assignee changed from bertagaz to intrigeri
  • % Done changed from 60 to 70
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

Looks good, except:

  • mail_to => 'tails-sysadmins@boum.org' ← uh, why would sysadmins be responsible for monitoring what's essentially code changes? I would says RMs to start with, and possibly tails-dev@ later once we know it works fine.
  • mail_from => 'reprepro@apt.lizard': won't the internal FQDN make the email be rejected by the destination MTA?

Makes sense. Fixed that.

Feel free to fix those and then deploy directly at a time when you're ready to 1. immediately test that it indeed works as expected; 2. deal with the fallout (if any :)

Done. We've just received the first notification while I uploaded a new package for #12696. Yay!

Shall I change the recipient now that we know it works?

#29 Updated by intrigeri 28 days ago

  • Assignee changed from intrigeri to bertagaz
  • % Done changed from 70 to 80
  • QA Check changed from Ready for QA to Dev Needed

Great! :)))

Three comments:

  • "this little daemon just saw the following file appear" is too verbose for an automated message we'll receive pretty often
  • "on apt" is not very useful info as is, IMO it's either too much or too little: better use the FQDN
  • I would appreciate being given a little bit more info, at least the target dist (e.g. we might want to detect issues like uploading straight to a release branch outside of the release process, or uploading to a tag). IMO just cat'ing the .changes file is the simplest option to start with, that should be good enough.

Shall I change the recipient now that we know it works?

40 minutes of runtime + 1 single test email is not enough to make me comfortable targetting a public mailing list, given our history wrt. reacting in a timely manner to problems caused by such automated messages.

I say fix the 3 issues raised above, postpone to 3.2, wait until the 3.1 release process (when you'll be uploading a number of packages), and redirect to tails-dev@ once 3.1 is out (+ of course explain tails-dev@ what's happening). Fair enough?

#30 Updated by intrigeri 27 days ago

  • Blocks Feature #13232: Core work 2017Q2: Sysadmin (Maintain our already existing services) added

#31 Updated by bertagaz 26 days ago

  • Priority changed from Elevated to Normal
  • Target version changed from Tails_3.1 to Tails_3.2

intrigeri wrote:

Great! :)))

Three comments:

  • "this little daemon just saw the following file appear" is too verbose for an automated message we'll receive pretty often
  • "on apt" is not very useful info as is, IMO it's either too much or too little: better use the FQDN
  • I would appreciate being given a little bit more info, at least the target dist (e.g. we might want to detect issues like uploading straight to a release branch outside of the release process, or uploading to a tag). IMO just cat'ing the .changes file is the simplest option to start with, that should be good enough.

Fixed, pushed in the master branch of the puppet-tails Git repo (see diff) and deployed.

I say fix the 3 issues raised above, postpone to 3.2, wait until the 3.1 release process (when you'll be uploading a number of packages), and redirect to tails-dev@ once 3.1 is out (+ of course explain tails-dev@ what's happening). Fair enough?

Ack.

#32 Updated by intrigeri 22 days ago

  • Blocks Feature #13233: Core work 2017Q3: Sysadmin (Maintain our already existing services) added

#33 Updated by intrigeri 22 days ago

  • Blocks deleted (Feature #13232: Core work 2017Q2: Sysadmin (Maintain our already existing services))

Also available in: Atom PDF