Tails

#2 Updated by intrigeri 2016-03-09 14:50:50

touch /etc/icingaweb2/.db_installed <- please put that in some more relevant place under /var.

#4 Updated by intrigeri 2016-03-12 11:42:49

> intrigeri wrote:
>> touch /etc/icingaweb2/.db_installed <- please put that in some more relevant place under /var.

> I’ve re-introduced this file dependency on the exec (rather than a mysql test, as discussed elsewhere), and moved the file in a directory managed by Icingaweb2.

/usr/share/icingaweb2/etc/.db_installed is even worse than /etc/icingaweb2/.db_installed for at least two reasons. It’s so obvious that such stuff should live in /var, that I won’t waste our time stating those reasons.

> All our Icingaweb2 related code now seems stable.

Great! I’m going to look at it shortly, and will check out the web interface as well.

#6 Updated by intrigeri 2016-03-12 12:57:27

Importing discussion I erroneously started on the parent ticket, and replying to it:

>> * I see execs like install_ido_database that assume “a bit” too much about what’s in the user-defined strings, before passing it to a shell; in particular, passwords can very well contain interesting special chars.

> This exec has disappeared since then, and the only remaining one quotes the different arguments.

I think you misunderstood me. It’s not about the scripts, it’s about the execs themselves. E.g. if web_db_pass contains a single quote, then Puppet will run different code than what you mean it to run. See what I mean?

Generally we don’t care about such things, because most of the parameters we would manually pass have about 0 chances to contain such special chars (modulo typo). But for passwords it’s different.

> Note that this class is not supposed to be included directly,

Then please encode/enforce this using the private() function from stdlib; it works better than hoping nobody will use it in the wrong way (even if it was documented I would rather see this enforced, and here it’s not even documented :)

> but though tails::monitoring::master, which validates the content of this arguments.

I see validate_string only. FYI this validates the type, not the content.

> Do you mean I should be more strict one the validate_ function I’m using on this arguments?

Yes, IMO for passwords passed to the shell we should ensure they only contain chars that are on a whitelist. Or whatever other solution you prefer, that fixes the problem I described above.

#7 Updated by intrigeri 2016-03-13 13:04:50

After login, can we have the session not expire at all, or live way longer?

#8 Updated by bertagaz 2016-03-14 15:00:12

intrigeri wrote:
> After login, can we have the session not expire at all, or live way longer?

Find that a bit annoying too. Did some research on the Web and in the code, but couldn’t find anything that enable us to configure longer sessions, sorry. :/

#9 Updated by intrigeri 2016-03-14 15:33:20

> Find that a bit annoying too. Did some research on the Web and in the code, but couldn’t find anything that enable us to configure longer sessions, sorry. :/

Maybe icingaweb2 is using the default PHP session handling, and then this can be configured in php.ini?

#10 Updated by bertagaz 2016-03-15 12:50:19

intrigeri wrote:
> > I’ve re-introduced this file dependency on the exec (rather than a mysql test, as discussed elsewhere), and moved the file in a directory managed by Icingaweb2.
>
> /usr/share/icingaweb2/etc/.db_installed is even worse than /etc/icingaweb2/.db_installed for at least two reasons. It’s so obvious that such stuff should live in /var, that I won’t waste our time stating those reasons.

Yeah, you don’t need to. I’ve put it in /usr/share/ because the Debian package doesn’t create nor use /var/lib/icingaweb2. But if you prefer it still in /var/lib/, it is now.

#11 Updated by bertagaz 2016-03-15 14:31:04

intrigeri wrote:
> I looked around in the web interface, and so far I like it a lot! Woohoo :) We’ll see how it goes when using it in practice.

Like it too. I’m happy to have given a try to Icingaweb2. Icingaweb wasn’t so appealing.

> I don’t understand the authentication/authorization setup. In the current state of things, it seems to me (correct me if I’m wrong) that:
>
> * non-sysadmins on tails@ can type a login/password to gain access to a login page, and then they’re blocked, which feels too much or too little;
> * sysadmins have to type a login/password to access the login page, and then another login/password to actually enter the Icinga2 web interface, which is a bit more painful than I would like.
>
> I assume you don’t mean to leave it in this state, so: what are your plans in this area?

You’re correct. My plan was to add a new web user with only read-only permissions, and add that to our shared credential, so that Tails devs can have a look if they wish. Maybe what you argue below imply it’s not even needed.

For our use case as sysadmins, sure two authentication steps is a bit too much. I don’t think we can remove the Icingaweb2 one, e.g by telling it it has to rely on the htaccess auth. I’ll dig.

> At first glance I see little value in giving access to tails@ members to that interface. What I would find useful is to give access, to the entire Tails contributors community, that is to the public in practice, to some basic status summary; I assume there are tools to do that, or some icinga API allows us to easily build it ourselves; if you don’t find this idea stupid I’ll file a ticket about that (not part of this deliverable!) so that we can deal with it later.

There’s a http://docs.icinga.org/icinga2/latest/doc/module/icinga2/chapter/icinga2-api#icinga2-api:"documented API“, and some ”$“:https://github.com/Icinga/icinga2-api-examples:”example code" around

> Then I looked at the corresponding Puppet code, and here are a few comments & questions — mostly style and implementation details:

Thx, this part was a bit more painful to do, as I didn’t have played a lot with Icingaweb2 before

> * I don’t understand why we have a “config” component in tails::monitoring::config::icingaweb2*. It adds no value I can see, it’s actually wrong (this class doesn’t only deals with configuration, it also sets up a DB), and it’s so generic that a lot of our classes could have well have “config” in their name, and this would be just as justified as for this one class, the way I see it. Maybe just drop it?

Yeah, I had the same questions when finishing this. Moved in commit puppet-tails:53f3ace.

> * May you please document what’s the difference between the “ido_db” and the “web_db” so one can easily get what the class params are about?

commit puppet-tails:63c1c61

> * Please use codenames instead of dist name (n=stretch instead of a=testing in APT pinning), so its meaning doesn’t suddenly change under our feet when a new Debian is released.

Done, commit puppet-tails:117e3d4

> * I’m not sure I understand what File['/usr/share/icingaweb2/public/index.php'] is for. This file is shipped by the icingaweb2 package. If we need to change its owner/permissions, then dpkg-statoverride is the right way to do it, as opposed to something that will break between each icingaweb2 upgrade and next time Puppet runs. If I guessed wrong what this is all about, I’m sorry (and then perhaps a comment would help).

Fixed in commit puppet-tails:803de01. I think this was a remaining of some experiments. It seems to work fine with the default ownership.

> * Where does /etc/apt/preferences.d/icingaweb2 come from? Class[icingaweb2] should depend on it, no?

Right, I must have forgotten it when encoding the installation tests in puppet… Fixed in commit puppet-tails:7cbaa2e.

> * I think I understand why Class[icingaweb2] would depend on pinning being set up by Apt::Preferences_snippet[$packages] first, but I don’t get why it depends on these packages actually being installed already.

Well, I often see such a deployment as the process of installing packages, then do configuration. We don’t rely fully on the icingaweb2 upstream puppet module for packages installation as it’s missing some bits on this side to support Debian Jessie correctly. We actually just use it to install the icingaweb2 package. For the rest, at the moment we mainly use it to configure Icingaweb2, once everything is installed. So this order makes sense to me, and it proved to work when testing it from scratch.

> * I’d rather see site_ssl.conf not being duplicated.

Right, commit puppet-tails:756e1e3 and puppet-lizard-manifest:2f9816d

> * Why does Package[php5-fpm] depend on the nginx vhost? The opposite relationship would make more sense to me. Maybe I’m confused though :)

I didn’t see much point to install the php5 package if there was no vhost to serve its content. But well, inverted the logic in puppet-tails:ed520cf.

> * Why do we have to do File_line['timezone_date']? All our systems are supposed to be UTC.

That’s required to be set (and not just empty as per default) for Icingaweb2 to work correctly according to the installation doc, so I followed it.

#12 Updated by bertagaz 2016-03-15 15:17:19

intrigeri wrote:
> I think you misunderstood me. It’s not about the scripts, it’s about the execs themselves. E.g. if web_db_pass contains a single quote, then Puppet will run different code than what you mean it to run. See what I mean?

Got it!

> > Note that this class is not supposed to be included directly,
>
> Then please encode/enforce this using the private() function from stdlib; it works better than hoping nobody will use it in the wrong way (even if it was documented I would rather see this enforced, and here it’s not even documented :)

Didn’t know this private() helper! Commit puppet-tails:9dc8b63

> > Do you mean I should be more strict one the validate_ function I’m using on this arguments?
>
> Yes, IMO for passwords passed to the shell we should ensure they only contain chars that are on a whitelist. Or whatever other solution you prefer, that fixes the problem I described above.

Commit puppet-tails:71a5d42

#15 Updated by intrigeri 2016-03-22 11:14:23

>> Yes, IMO for passwords passed to the shell we should ensure they only contain chars that are on a whitelist. Or whatever other solution you prefer, that fixes the problem I described above.

> Commit puppet-tails:71a5d42

… which is obviously buggy. I’ve noticed you fixed it later on, great! I will dare insisting though: please do test your validation code against input that is not supposed to validate.

#16 Updated by intrigeri 2016-03-22 11:14:43

>> Can we please have an URL to access that web interface, that does not depend on the exact host it is hosted on? E.g. we jenkins.tails.boum.org, not lizard.tails.boum.org/jenkins, and there are good reasons for that. I would suggest https://icingaweb2.tails.boum.org/.

> Done. Simpler that what I was afraid of.

Thanks!