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

Systemd unit without chroot #150

Merged
merged 2 commits into from
Jan 23, 2020
Merged

Systemd unit without chroot #150

merged 2 commits into from
Jan 23, 2020

Conversation

Frzk
Copy link
Contributor

@Frzk Frzk commented Jan 23, 2020

Replaces #149.

@Frzk Frzk mentioned this pull request Jan 23, 2020
@wcawijngaards wcawijngaards self-assigned this Jan 23, 2020
Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

Nice explanation text! It is good to see the systemd protections in action.

@wcawijngaards wcawijngaards merged commit ff92edc into NLnetLabs:master Jan 23, 2020
@wcawijngaards
Copy link
Member

Thank you for the contribution, and the fixup with explanations. Was a good read and also the file can be useful to people that want that systemd configuration. Merged it and added a contrib/README and Changelog note.

@Frzk
Copy link
Contributor Author

Frzk commented Jan 23, 2020

Thanks a lot, it was a pleasure 😀

@Maryse47
Copy link
Contributor

Maryse47 commented Jan 23, 2020

@Frzk when I was fixing original systemd service I tried to make it work with and without chroot alongside being backward compatible for existing users. In fact, without fixes service worked more reliable without chroot enabled.

I just tested myself that it works without chroot for me but if it doesn't for you then it's a bug and we should try to investigate and fix it instead of proliferating services with minimal diffs from each other which will be confusing for users.

Unless I'm mistaken when UNBOUND_CHROOT_DIR isn't set it defaults to UNBOUND_RUN_DIR which should be still ok. Doesn't it work for you? What was your exact issue?

Your BindPaths/BindReadOnlyPaths doesn't make sense as they are bind to the very same dir. Also they are pointless here as they are needed only for chroot. You may misunderstood the purpose of those rules.

@wcawijngaards I recommend reverting this one and work collaboratively on fixing original service instead.

@Maryse47
Copy link
Contributor

Also all systemd hardenings are available in original systemd service as well but the description suggest they are exclusive for non-chroot version.

@Maryse47
Copy link
Contributor

@Frzk could you test if adding RuntimeDirectory=unbound and ConfigurationDirectory=unbound to original service make things work for you?

@wcawijngaards
Copy link
Member

Sure, if possible, we can have one file that can do it with and without chroot. So that both types of use can be supported.

@Maryse47
Copy link
Contributor

Maryse47 commented Jan 24, 2020

@wcawijngaards Could you revert it then before someone start using it? As I said original service works right now for me with chroot: "" and pidfile: /run/unbound/unbound.pid so there is no fundamental reason for having separate service for non-chroot.

@Frzk
Copy link
Contributor Author

Frzk commented Jan 24, 2020

Hi there,

First things first, I'd like to apologize if I hurt someone with this PR. As I stated in the original post (#149), these are changes I was suggesting (basically : let's discuss them). I originally made the changes against the existing file, but having another unit file seemed a good idea to me. Sounds like it wasn't.

Your BindPaths/BindReadOnlyPaths doesn't make sense as they are bind to the very same dir. Also they are pointless here as they are needed only for chroot. You may misunderstood the purpose of those rules.

That's a mistake of mine, they can be removed. (They are useful when running with RootDirectory=/RootImage= which is my case, I guess that's why they ended up here :-/ )

Also all systemd hardenings are available in original systemd service as well but the description suggest they are exclusive for non-chroot version.

Can't see where I suggest something like this (?).

That being said, I also made a bunch of tests all this morning, regardless of my use case (which is using unbound as a portable service).

I'm using ArchLinux, so chroot dir defaults to run-dir which in turn defaults to /etc/unbound which already sounds weird to me, but well, OK.
Also, in ArchLinux, unbound is compiled with --with-pidfile=/run/unbound.pid and --with-conf-file=/etc/unbound/unbound.conf - it's important.

All tests were done with the provided original unit file. For some tests I used a drop-in to add a few directives, keeping the unit file untouched.

Here is what I'm experiencing (tried to be synthetic):

1. with chroot (chroot: /etc/unbound)

1.a Base config file:

systemctl start systemctl stop Result
Creates /run/unbound.pid Leaves /run/unbound.pid Not OK
Creates /etc/unbound/dev Leaves /etc/unbound/dev ~OK
Creates /etc/unbound/run Leaves /etc/unbound/run ~OK

Results are the same when specifying pidfile: /run/unbound.pid in unbound configuration file.

1.b Setting pidfile: /run/unbound/unbound.pid:

systemctl start systemctl stop Result
Fails to create /run/unbound/unbound.pid Leaves /run/unbound Not OK
Creates /etc/unbound/dev Leaves /etc/unbound/dev ~OK
Creates /etc/unbound/run Leaves /etc/unbound/run ~OK

1.c Base config file and adding RuntimeDirectory=unbound in a drop-in:

systemctl start systemctl stop Result
Creates an empty /run/unbound directory Removes /run/unbound directory OK
Writes pid file in /run/unbound.pid Leaves /run/unbound.pid Not OK
Creates /etc/unbound/dev Leaves /etc/unbound/dev ~OK
Creates /etc/unbound/run Leaves /etc/unbound/run ~OK

1.d Setting pidfile: /run/unbound/unbound.pid and adding RuntimeDirectory=unbound in a drop-in:

systemctl start systemctl stop Result
Creates /run/unbound/unbound.pid Removes /run/unbound OK
Creates /etc/unbound/dev Leaves /etc/unbound/dev ~OK
Creates /etc/unbound/run Leaves /etc/unbound/run ~OK

We are finally OK.

2. without chroot (chroot: "")

2.a Setting chroot: "":

systemctl start systemctl stop Result
Creates /run/unbound.pid Leaves /run/unbound.pid Not OK
Creates /etc/unbound/dev Leaves /etc/unbound/dev ~OK
Creates /etc/unbound/run Leaves /etc/unbound/run ~OK

2.b Setting chroot: "" and pidfile: /run/unbound/unbound.pid:

systemctl start systemctl stop Result
Fails unless /run/unbound is created first - ~OK
Creates /run/unbound/unbound.pid Leaves /run/unbound/unbound.pid Not OK
Creates /etc/unbound/dev Leaves /etc/unbound/dev ~OK
Creates /etc/unbound/run Leaves /etc/unbound/run ~OK

2.c Setting chroot: "", pidfile: /run/unbound/unbound.pid and adding RuntimeDirectory=unbound in a drop-in:

systemctl start systemctl stop Result
Creates /run/unbound/unbound.pid  Removes /run/unbound OK
Creates /etc/unbound/dev Leaves /etc/unbound/dev ~OK
Creates /etc/unbound/run Leaves /etc/unbound/run ~OK

We are OK here.


As you can see, it seems to me that we are pretty far from working out of the box.

I guess the results could be improved if unbound was compiled with --with-pidfile=/run/unbound/unbound.pid because it would mean we'd just have to set RuntimeDirectory=unbound to get something working in these cases.

But again, I'm not speaking about my use case. I'll try to provide further inputs about it later, I've already spent 5 or 6 hours on this, way more than I can afford.

@Maryse47
Copy link
Contributor

Maryse47 commented Jan 24, 2020

First things first, I'd like to apologize if I hurt someone with this PR. As I stated in the original post (#149), these are changes I was suggesting (basically : let's discuss them). I originally made the changes against the existing file, but having another unit file seemed a good idea to me. Sounds like it wasn't.

No problem, you raised a valid issue. I only wish that proposed fix wasn't merged so fast before any discussion could happen 😄 . Anyway I hope we can agree about finding better solution.

They are useful when running with RootDirectory=/RootImage= which is my case, I guess that's why they ended up here :-/ )

Hm, I don't see how binding them to the same dir would help in any case.

Can't see where I suggest something like this (?).

You wrote that:

; Running without the chroot doesn't mean it's less secure. Simply put, we will
; instead rely on a few systemd directives to harden the service.
; To quote systemd : it's like a chroot on steroids !

That suggests chroot and systemd hardening are alternatives despite they can be used together just fine. Also same systemd hardening rules are elaborately described only in new unit while those are exactly same rules like those in other service. I think we can just put those descriptions to main service.

I'm using ArchLinux

Same as me so finding common ground should be easy 😄

As you can see, it seems to me that we are pretty far from working out of the box.

Your testing shows that situation is exactly the same with and without chroot (1a == 2a) so adding separate non-chroot service wasn't appropriate solution from the beginning as I expected.

Some minor issue happens when pidfile: /run/unbound.pid is set or both chroot: "" and pidfile: /run/unbound/unbound.pid are set- it doesn't remove the pidfile after service stopped (1a + 1c +2a +2b). This however doesn't have practical consequences as pidfile will be always overwritten when service starts. Your new config doesn't fix this issue and makes it even more severe instead as lack of write access to /run make impossible to create pidfile when pidfile: /run/unbound.pid is set. I consider this as core unbound problem which systemd service can't help for although severity of it is quite low and may be ignored.

The only real issue happens when pidfile: /run/unbound/unbound.pid is set and it fails to create /run/unbound/ subdirectory (1b + 2b). As your testing shows adding RuntimeDirectory=unbound fixes this problem (1d + 2c) which is a simple and proper solution which can be easily added to original service.

I hope above analysis make situation clearer.

But again, I'm not speaking about my use case. I'll try to provide further inputs about it later, I've already spent 5 or 6 hours on this, way more than I can afford.

I spent many hours within several weeks in order to make this service usable for official Arch package and I'm open to help with improving it further. I'll prepare changes with adding RuntimeDirectory=unbound to original service which should make it working for you.

@Maryse47 Maryse47 mentioned this pull request Jan 24, 2020
@Maryse47
Copy link
Contributor

Maryse47 commented Jan 24, 2020

@Frzk please confirm if #151 fixes your problem. I believe we can agree that adding separate service is way over the top when adding single line would suffice.

@Frzk
Copy link
Contributor Author

Frzk commented Jan 24, 2020

@Maryse47 Constantly deleting/reposting your comments makes it very difficult to answer with context and precisions 😕

I'll try to answer everything here later this evening with further explanations about my use cases.

@Maryse47
Copy link
Contributor

Maryse47 commented Jan 24, 2020

Constantly deleting/reposting your comments makes it very difficult to answer with context and precisions

Sorry, I'm continuously improving my answers in quest for perfection (not deleting but editing). I want be be as specific and to the point as I can to minimize misunderstandings. I didn't know you read them already.

@Frzk
Copy link
Contributor Author

Frzk commented Jan 24, 2020

They are useful when running with RootDirectory=/RootImage= which is my case, I guess that's why they ended up here :-/ )

Hm, I don't see how binding them to the same dir would help in any case.

This is basically why they exists : making a part of the filesystem tree available in a mount namespace... Why would I want them somewhere else ?

You wrote that:

; Running without the chroot doesn't mean it's less secure. Simply put, we will
; instead rely on a few systemd directives to harden the service.
; To quote systemd : it's like a chroot on steroids !

That suggests chroot and systemd hardening are alternatives despite they can be used together just fine.

I think you are over-interpreting here :sorry:

Also, (real, serious question), what's the benefit of the chroot in this case ?

Also same systemd hardening rules are elaborately described only in new unit while those are exactly same rules like those in other service. I think we can just put those descriptions to main service.

Please do.

Your testing shows that situation is exactly the same with and without chroot (1a == 2a) so adding separate non-chroot service wasn't appropriate solution from the beginning as I expected.

I don't know if it was appropriate or not, but at least it was working (pid removed), filesystem kept clean, working with portable services.

Some minor issue happens when pidfile: /run/unbound.pid is set or both chroot: "" and pidfile: /run/unbound/unbound.pid are set- it doesn't remove the pidfile after service stopped (1a + 1c +2a +2b). This however doesn't have practical consequences as pidfile will be always overwritten when service starts.

Having a leftover .pid is far from being a minor issue IMHO : I'm basically relying on them to monitor my services... And it also leaves a dev and run directories in my config directory...

Your new config doesn't fix this issue and makes it even more severe instead as lack of write access to /run make impossible to create pidfile when pidfile: /run/unbound.pid is set.

My unit file fixes this at the cost of specifying chroot: "" and pidfile: /run/unbound/unbound.pid. I made this clear since the beginning. That's why we ended up with 2 unit files. This is also why it's literally written down in the 12 first lines of the unit file. (BTW, isn't storing a pid file directly in /run considered a bad practice ?)

I consider this as core unbound problem which systemd service can't help for although severity of it is quite low and may be ignored.

So is it severe or not ?

The only real issue happens when pidfile: /run/unbound/unbound.pid is set and it fails to create /run/unbound/ subdirectory (1b + 2b). As your testing shows adding RuntimeDirectory=unbound fixes this problem (1d + 2c) which is a simple and proper solution which can be easily added to original service.

Nope, that's not the "only real issue". Please see below.

I spent many hours within several weeks in order to make this service usable for official Arch package and I'm open to help with improving it further. I'll prepare changes with adding RuntimeDirectory=unbound to original service which should make it working for you.

I'm really sorry, but it doesn't.

Let's get back to testing, this time trying to make unbound a portable service.

As root, without modifying the provided unit file:

cd ~
mkdir ./unbound_test
pacstrap -i ./unbound_test/ base unbound
mksquashfs ./unbound_test/ /var/lib/portables/unbound_test.raw
portablectl attach /var/lib/portables/unbound_test.raw
systemctl start unbound

Fails with:

unbound.service: Failed to set up mount namespacing: /run/systemd/unit-root/etc/unbound/dev: No such file or directory
unbound.service: Failed at step NAMESPACE spawning /usr/bin/unbound: No such file or directory

Which is expected, /etc/unbound/{dev, run} don't exist in the image.
Let's add them:

portablectl detach /var/lib/portables/unbound_test.raw
rm /var/lib/portables/unbound_test.raw
mkdir -p ./unbound_test/etc/unbound/{dev,run}
mksquashfs ./unbound_test/ /var/lib/portables/unbound_test.raw
portablectl attach /var/lib/portables/unbound_test.raw
systemctl start unbound

It now fails with the following:

unbound.service: Failed to set up mount namespacing: /run/systemd/unit-root/run/dbus/system_bus_socket: No such file or directory
unbound.service: Failed at step NAMESPACE spawning /usr/bin/unbound: No such file or directory

(Which is quite logic too since /run/dbus/system_bus_socket doesn't exist either in the image).

How am I supposed to proceed thereafter ?

Now if I edit the provided unit file, according to the comment in it (still with the chroot), it works !
Oh well nope, because it loads the configuration from the image /etc/unbound/unbound.conf file, not from the host's one. Also the pid file is nowhere to be found. All this is in fact expected.

Results are the same with chroot: "".

In fact to make it work I'd have to create a drop-in with a ConfigurationDirectory=unbound directive. (And you can also comment the ReadWritePaths= on line 33 in this case, finally getting the exact same config I proposed...)


I suspect using it with systemd-nspawn would give the same results although I didn't tested it.

I understand that keeping backward compatibility is important, but here, IMHO, we are keeping it for no reason (what is the benefit ?) at the cost of a complex unit file which prevents legitimate usage.

Again, my proposal was about providing a unit file that is simple, yet providing enough security and allowing for other use cases like the one presented here without all the hassle of understanding what the unit file does (and yes, maybe breaking compatibility with people running with chroot set.)


Now please @wcawijngaards could you please merge #151 to end all this non sense ?
I have my unit file, I'll just copy it in my image and I'll be done 😀

@Maryse47
Copy link
Contributor

This is basically why they exists : making a part of the filesystem tree available in a mount namespace... Why would I want them somewhere else ?

Ok, for RootDirectory=/RootImage= this makes sense, sorry for confusion.

Also, (real, serious question), what's the benefit of the chroot in this case ?

Most systemd hardenings make various path read-only while chroot make them invisible. That's why we have to hack around some things to make them visible again like /run/systemd/notify.

I don't know if it was appropriate or not, but at least it was working (pid removed), filesystem kept clean, working with portable services.

Yes, but you fixed not the problem that you described. Description says "This unit file is provided to run unbound without chroot." This is misleading because chroot works just fine with original unit. It may not work with portable services so if you described it as "This unit file is provided to run unbound as portable service." instead it would make more sense.

My unit file fixes this at the cost of specifying chroot: "" and pidfile: /run/unbound/unbound.pid. I made this clear since the beginning. That's why we ended up with 2 unit files. This is also why it's literally written down in the 12 first lines of the unit file.

The point is if I specify chroot: "" and pidfile: /run/unbound/unbound.pid then I don't need your unit file at all because original one + RuntimeDirectory=unbound will work just fine. What's the purpose of the second unit then if everything depends on user config in the end? Original unit can work with and without chroot, with pidfile: /run/unbound/unbound.pidand with pidfile: /run/unbound.pid. Second unit is totally redundant here.

(BTW, isn't storing a pid file directly in /run considered a bad practice ?)

It is a bad practice but it is what many distros and users do and I wanted this unit be usable for as much people as possible.

So is it severe or not ?

IMHO it's not. By core I meant core unbound code in opposition to contrib stuff like systemd units.

Having a leftover .pid is far from being a minor issue IMHO : I'm basically relying on them to monitor my services... And it also leaves a dev and run directories in my config directory...

Unbound leaves those pids all the time in most configurations and I didn't saw complaints here or in Arch so I think vast majority of people don't care. Is leaving dev and run in config directory another serious issue for you? They are needed for chroot to work but I don't see how they're causing problems.

Nope, that's not the "only real issue". Please see below.

The only real issue your first tests showed. I thought they were complete.

I'm really sorry, but it doesn't.

Let's get back to testing, this time trying to make unbound a portable service.

Wait, what still doesn't work with first tests you presented here? The pidfiles are left? Please open separate issue about that. Portable services are different case and I didn't said it will work with them. I think we shouldn't jump to the next case before we can settle on the previous one. Saying "it doesn't work" isn't helpful.

How am I supposed to proceed thereafter ?

It's possible that chroot workarounds are incompatible with portable services. It's quite possible if you put portable services instead of (non)chroot everywhere in description the whole discussion wouldn't happen. The reality is that portable services never occurs in your otherwise extensive unit description (so how someone would now that it mainly relates to that?). As I said above you fixed not the problem that you described.

In fact to make it work I'd have to create a drop-in with a ConfigurationDirectory=unbound directive.

Do you mean ConfigurationDirectory=unbound makes original unit compatible with portable services or removing chroot hack are still needed? Adding ConfigurationDirectory=unbound to original unit should be harmless, I can do that.

I understand that keeping backward compatibility is important, but here, IMHO, we are keeping it for no reason (what is the benefit ?) at the cost of a complex unit file which prevents legitimate usage.

Do you know how many users reported broken chroot in unbound when upstream unit was used for the first time in official package? Enabled chroot is default unbound behavior so breaking it in upstream template would mean breaking almost all its users. Also Arch builds unbound with with-pidfile=/run/unbound.pid (I wish they don't, you may open issue on their bugtracker) so removing write access to /run will break Arch. All of this is totally legitimate usage of unbound. You can't just say "works for me" and leave everyone else in the dust.

Again, my proposal was about providing a unit file that is simple, yet providing enough security and allowing for other use cases like the one presented here without all the hassle of understanding what the unit file does (and yes, maybe breaking compatibility with people running with chroot set.)

Again, your proposal didn't provide any meaningful new security and was totally orthogonal to chroot. As I understand now it was all about portable services from the beginning but that fact was lost while your prepared final PR. Your unit doesn't bring any meaningful value for users who don't use portable services and it's not something that distros want to package. If unbound maintainers think that additional portable services version is valuable then it's ok but please name and describe things accordingly.

@Maryse47
Copy link
Contributor

Now please @wcawijngaards could you please merge #151 to end all this non sense ?
I have my unit file, I'll just copy it in my image and I'll be done 😀

@Frzk Please take a look at modifications I did for #151 . I kept your unit but renamed it and changed its description to be more appropriate. I hope this is solution we can finally agree about.

@Frzk
Copy link
Contributor Author

Frzk commented Jan 26, 2020

Also, (real, serious question), what's the benefit of the chroot in this case ?

Most systemd hardenings make various path read-only while chroot make them invisible.

So it confirms what I suspected : no benefit.

I don't know if it was appropriate or not, but at least it was working (pid removed), filesystem kept clean, working with portable services.

Yes, but you fixed not the problem that you described.

Quoting the very first lines in #149:

I'm trying to use (and deploy) unbound as a portable service. With the provided .service file, it seems very difficult, mostly because it puts everything under a chroot directory. The chroot thing seems a bit cumbersome though since we are using systemd to harden the environment unbound will run in.

How doesn't it fix the problem I described ?

Description says "This unit file is provided to run unbound without chroot." This is misleading because chroot works just fine with original unit. It may not work with portable services so if you described it as "This unit file is provided to run unbound as portable service." instead it would make more sense.

That's not misleading at all, that's what it does:

  • it allows one to run unbound without chroot, hence the name unbound_nochroot.service.in. Also the comments in the file provides more details on the conditions required to run this unit file ;
  • it was never meant to replace your unit file, it was just an alternative one ;
  • never said chroot wasn't "working just fine with the original unit" before my yesterday's comment and testings. In fact I never cared about chroot before doing the tests yesterday. Again, I think you are over-interpreting here.

My unit file fixes this at the cost of specifying chroot: "" and pidfile: /run/unbound/unbound.pid. I made this clear since the beginning. That's why we ended up with 2 unit files. This is also why it's literally written down in the 12 first lines of the unit file.

The point is if I specify chroot: "" and pidfile: /run/unbound/unbound.pid then I don't need your unit file at all because original one + RuntimeDirectory=unbound will work just fine.

As you just said, you would need it for RuntimeDirectory=unbound that your unit isn't shipping. You'd also need it to keep a config directory clean. Or also if you'd want to run with a read-only /etc/unbound dir.

What's the purpose of the second unit then if everything depends on user config in the end?

The purpose is to bring an alternative. Then by modifying the compile options (store pidfile in /run/unbound/unbound.pid instead of /run/unbound.pid + remove the chroot) we would have better defaults IMHO (easy to understand unit file, filesystem tree kept clean, runs out of the box, doesn't break systemd functionalities).

(BTW, isn't storing a pid file directly in /run considered a bad practice ?)

It is a bad practice but it is what many distros and users do and I wanted this unit be usable for as much people as possible.

Again I understand this. That's why the unit was proposed as an alternative. Not replacing yours

Having a leftover .pid is far from being a minor issue IMHO : I'm basically relying on them to monitor my services... And it also leaves a dev and run directories in my config directory...

Unbound leaves those pids all the time in most configurations and I didn't saw complaints here or in Arch so I think vast majority of people don't care. Is leaving dev and run in config directory another serious issue for you? They are needed for chroot to work but I don't see how they're causing problems.

That may sound harsh but I don't really care if other users don't mind running bad practice.
Also, yes, creating the dev and run dirs in my configuration directory is an issue because at some point I'd like to make this directory and its content read-only (ConfigurationDirectoryMode= directive).

Wait, what still doesn't work with first tests you presented here? The pidfiles are left? Please open separate issue about that. Portable services are different case and I didn't said it will work with them. I think we shouldn't jump to the next case before we can settle on the previous one. Saying "it doesn't work" isn't helpful.

I just demonstrated that it doesn't work with a complete set of reproducible instructions and explanations. So telling me that "saying «it doesn't work» isn't helpful" just sounds like trolling to me.

How am I supposed to proceed thereafter ?

It's possible that chroot workarounds are incompatible with portable services.

Demonstrated before.

It's quite possible if you put portable services instead of (non)chroot everywhere in description the whole discussion wouldn't happen.

Yes, instead we would have a discussion about the leftover pid file, then a discussion about the leftovers run and dev directories (even when you don't use chroot), and then a discussion about portable services (and maybe one about systemd-nspawn). And we'd finally end up with 2 unit files: one that cares with backward compatibility and another one that follows nowadays best practices. Which is where we are now. That'd be really productive.

The reality is that portable services never occurs in your otherwise extensive unit description (so how someone would now that it mainly relates to that?). As I said above you fixed not the problem that you described.

Reality is the provided unit file only works if:

  • you don't care about having an always-existing pid file, even when the service doesn't run ;
  • you don't care about a unit file writing in your /etc/unbound configuration directory ;
  • you don't care about having an always-existing /etc/unbound/dev directory ;
  • you don't care about having an always-existing /etc/unbound/run directory ;
  • you don't care about breaking systemd functionalities ;
  • you do care about chrooting which gives you zero benefit.

Whereas mine (+ some config or modified build options) was given as an alternative to circumvent all this. So yes, I don't see the point of putting "portable service" everywhere in it.
Following your reasoning, mine doesn't only work in a portable service scenario but in all nochroot scenarii so why should we make the users think it's limited to the portable service use case ?

Do you mean ConfigurationDirectory=unbound makes original unit compatible with portable services or removing chroot hack are still needed? Adding ConfigurationDirectory=unbound to original unit should be harmless, I can do that.

I'll let you do the tests 😀

I understand that keeping backward compatibility is important, but here, IMHO, we are keeping it for no reason (what is the benefit ?) at the cost of a complex unit file which prevents legitimate usage.

Do you know how many users reported broken chroot in unbound when upstream unit was used for the first time in official package? Enabled chroot is default unbound behavior so breaking it in upstream template would mean breaking almost all its users. Also Arch builds unbound with with-pidfile=/run/unbound.pid (I wish they don't, you may open issue on their bugtracker) so removing write access to /run will break Arch. All of this is totally legitimate usage of unbound. You can't just say "works for me" and leave everyone else in the dust.

Actually I'm not. It's provided as an alternative. And yes, I'll get in touch with Gaetan to discuss the build options for ArchLinux.

Again, my proposal was about providing a unit file that is simple, yet providing enough security and allowing for other use cases like the one presented here without all the hassle of understanding what the unit file does (and yes, maybe breaking compatibility with people running with chroot set.)

Again, your proposal didn't provide any meaningful new security

Never was the goal. Never stated it does.

and was totally orthogonal to chroot.

Sure. That's why it's suffixed nochroot. How should have we made it clearer ?!

As I understand now it was all about portable services from the beginning but that fact was lost while your prepared final PR.

This is just how I discovered things. I would probably have noticed later when setting up monitoring (because of the leftover pid file). So, nope, it wasn't all about portable services.

Your unit doesn't bring any meaningful value for users who don't use portable services

At least it fixes the missing RuntimeDirectory= directive that you included in your PR...
I think my comments bring everything needed to decide if it brings meaningful value or not. I'm not going to list the advantages (and inconveniences) here again. And I'm certainly not going to comment on all this again.

and it's not something that distros want to package.

I don't know, I'm (thankfully !) not in packagers minds.

If unbound maintainers think that additional portable services version is valuable then it's ok but please name and describe things accordingly.

IMO that's what we've done here:

  • if you want to run without chroot, you might be interested in using the alternative nochroot service file.
  • if you are a packager and desire to enforce nowadays best practices as defaults, you might be interested in getting rid of the chroot behavior and thus you might take the risk to ship the ready-to-use alternative nochroot service file.

@Maryse47
Copy link
Contributor

Maryse47 commented Jan 26, 2020

So it confirms what I suspected : no benefit.

Making various paths not accessible instead of read-only has no benefit?

How doesn't it fix the problem I described ?

Did you seriously think user will dig into a comment for old PR which was rejected in order to know context about shipped code? Merged PR has absolutely no such context.

That may sound harsh but I don't really care if other users don't mind running bad practice.
Also, yes, creating the dev and run dirs in my configuration directory is an issue because at some point I'd like to make this directory and its content read-only (ConfigurationDirectoryMode= directive).

The bad practice comes from unbound itself perhaps for some historical reasons. I don't think the purpose of systemd unit is to mandate how unbound works. Arch (and possibly nobody else) wouldn't adopt upstream systemd unit if it broke existing users and everyone would be left with basic unit with no hardening shipped by ditros.

I just demonstrated that it doesn't work with a complete set of reproducible instructions and explanations. So telling me that "saying «it doesn't work» isn't helpful" just sounds like trolling to me.

I explained that your issue from your first set of instructions could fixed by RuntimeDirectory= and you answered it doesn't work without details. This was unhelpful. Next, you provided another set of instructions (for portable services) which indeed don't work with original service but it doesn't prove that first set of instructions is broken.

Demonstrated before.

You demonstrated it in the same post.

Yes, instead we would have a discussion about the leftover pid file

I already adviced to open issue about leftover pids. Unbound never removes those even while using your unit, it's just systemd which removes whole directory. You may easily check this with RuntimeDirectoryPreserve=yes, the file is still there after exit.

then a discussion about the leftovers run and dev directories (even when you don't use chroot)

Original service is chroot agnostic - it works the same regardless if chroot is enabled or not. If you know how to make chroot working without creating those files then just show it, otherwise there is nothing to discuss.

then a discussion about portable services (and maybe one about systemd-nspawn)

Isn't there agreement about those? I don't see anything to discuss and that was my point - if you didn't stubbornly repeated claims about not working chroot (which is false) and instead focus on portable services then agreement could be reached immediately.

And we'd finally end up with 2 unit files: one that cares with backward compatibility and another one that follows nowadays best practices. Which is where we are now. That'd be really productive.

I'll much prefer that we end up with one unit working 99% of time that may be packaged by distros and one for special case like portable services which may be used for experimentation. Producing separate units with subtle differences with misleading descriptions is counterproductive as nobody will use more than one.

Reality is the provided unit file only works if:

you don't care about having an always-existing pid file, even when the service doesn't run ;

As said above this is unbound issue., not sytemd unit. It can be solved with RuntimeDirectory=unbound + chroot: "" + pidfile: /run/unbound/unbound.pid while using original service. Your service needs exactly same things so it's completely pointless from this perspective.

you don't care about a unit file writing in your /etc/unbound configuration directory ;

Question for @wcawijngaards do you think @UNBOUND_RUN_DIR@ and @UNBOUND_CHROOT_DIR@ need to be writable for anything? Maybe we can remove write access there?

you don't care about having an always-existing /etc/unbound/dev directory ;
you don't care about having an always-existing /etc/unbound/run directory ;

As I said those are needed for chroot to work. Unbound uses chroot by default. Breaking default configuration in systemd unit sounds awkward to me.

you don't care about breaking systemd functionalities ;

Do you mean portable services? They are incompatible with chroot, but see above.

you do care about chrooting which gives you zero benefit.

Perhaps unbound developers would want to hear more about it.

Whereas mine (+ some config or modified build options) was given as an alternative to circumvent all this. So yes, I don't see the point of putting "portable service" everywhere in it.
Following your reasoning, mine doesn't only work in a portable service scenario but in all nochroot scenarii so why should we make the users think it's limited to the portable service use case ?

With exactly same some config or modified build options the original service works as well as yours, including all nochroot cases except for portable services. This is fundamental issue with your unit and what make it pointless for all users who don't use portable services. This is also the message I'm trying to pass to you without much success.

I'll let you do the tests 😀

I'm sorry but I spent so many hours on this issue and won't sacrifice more only because you refrain from providing simple answer about things you already know.

Actually I'm not. It's provided as an alternative. And yes, I'll get in touch with Gaetan to discuss the build options for ArchLinux.

Good luck. Improving Arch package will be certainly beneficial.

Never was the goal. Never stated it does.

Sorry, I was just annoyed by your mentions of security so many times. Perhaps I should ignore it.

Sure. That's why it's suffixed nochroot. How should have we made it clearer ?!

The nochroot suffix suggest that original unit is incompatible with no-chroot which is utterly false. That makes its purpose not clear. The portable services are incompatible with chroot which is the reason you have to disable chroot for them. That's why portable suffix will be more appropriate and more to the point.

This is just how I discovered things. I would probably have noticed later when setting up monitoring (because of the leftover pid file). So, nope, it wasn't all about portable services.

Without portable services not single point of yours stands.

At least it fixes the missing RuntimeDirectory= directive that you included in your PR...
I think my comments bring everything needed to decide if it brings meaningful value or not. I'm not going to list the advantages (and inconveniences) here again. And I'm certainly not going to comment on all this again.

Could you at least say if my corrections are acceptable for you? You already agreed for complete removal so I hope keeping your unit intact with just description change will be even more welcomed.

if you want to run without chroot, you might be interested in using the alternative nochroot service file.

It makes little sense as original service work without chroot just fine. It's much easier for users to modify something with overrides than grabbing whole new unit (obviously distro can package only one).

if you are a packager and desire to enforce nowadays best practices as defaults, you might be interested in getting rid of the chroot behavior and thus you might take the risk to ship the ready-to-use alternative nochroot service file.

From packager perspective, breaking backward compatibility would be quite troublesome. Original unit allows for best practices but not enforce them which is more sensible for general audience and involves no risk and almost no downsides.

@wcawijngaards
Copy link
Member

Hi Maryse47 and Frzk,

Sure a rename of _nochroot to _portable sounds fine to me. So #151 looks reasonable.

About chroot, chroot predates systemd. And it is there to protect the system. This is why I try to enable this to encourage safer configurations. If systemd can provide protection just as good, that is fine with me, just like SELinux might. And some users find that too hard, because chroot makes things difficult, and there is the chroot: "" option, (that I also use in testing). Chroot is valuable, because it protects the system against a compromised server.

About pidfile removal. Unbound actually attempts to remove the pidfile. It needs permissions, on the file and directory (to remove a file from that directory). For that the file and directory need to be inside the chroot path, otherwise the pidfile has disappeared from unbound after the chroot, eg. unbound locked itself out of accessing the pidfile with the chroot security call. Unbound also attempts to truncate the file (to 0 bytes) and then delete it, and also attempts to chown the pidfile (to config username). Perhaps you could put the pidfile location inside the chroot, or mount the pidfile directory inside the chroot eg. bindmount /etc/unbound/run/unbound, (for /etc/unbound as chroot and /run/unbound as pidfile dir).

Thanks for the work in figuring out the best possible systemd options! There seems to be a lot of possibilities. And use cases for the unbound server. I think right now merging #151 allows both the original case of #149 (portable use), and we have the normal file for regular, chroot or nonchroot use. I believe a lot of packaging systems ship their own systemd service file, anyway, as people want to store stuff like pidfiles in different places.

@Maryse47, you asked if unbound needs write permissions. Unbound needs write permissions for two reasons, the pidfile removal and trust anchor updates. It depends on where you keep those files, and then unbound needs write permission on that. If chroot is used, those locations have to be inside the chroot. Unbound exits if the trustanchor cannot be written to, if that trustanchor is configured with auto updates, making sure that RFC5011 root trust anchor rollover works.

Best regards, Wouter

@Frzk
Copy link
Contributor Author

Frzk commented Jan 27, 2020

FYI: http://0pointer.de/blog/projects/changing-roots

and trust anchor updates. It depends on where you keep those files, and then unbound needs write permission on that.

Sound like a good candidate for the StateDirectory= directive.

@wcawijngaards
Copy link
Member

I just realised that RFC7706 root zone copies also can have a file, with the zonefile contents, that has to be written to. This is for auth-zone directives with a zonefile configured. (It can also be without zonefile; it then transfers every time, eg. zonefile: "").

@Frzk Frzk deleted the systemd_unit_without_chroot branch January 27, 2020 10:38
@Maryse47
Copy link
Contributor

@wcawijngaards

Unbound actually attempts to remove the pidfile. It needs permissions, on the file and directory (to remove a file from that directory)...also attempts to chown the pidfile (to config username).

For some reason chowning the pidfile doesn't seem to work:

sudo ls -al /run/unbound
total 4
drwxrwx---  2 root root  60 Jan 27 12:31 .
drwxr-xr-x 31 root root 760 Jan 27 12:31 ..
-rw-r--r--  1 root root   5 Jan 27 12:31 unbound.pid

and this may be the problem - unbound lacks of permissions to remove the pidfile. It's possible to use User=unbound (or even DynamicUser) directive to start unbound unprivileged from the beginning which should make pidfile owned by unbound user but it needs several assumptions (unbound user needs to exist, it have to have permissions for port binding, write anchors, etc. It's possible to configure system to make it work but certainly not every system is configured that way. @Frzk You may consider DynamicUser=yes for portable service.

Perhaps you could put the pidfile location inside the chroot, or mount the pidfile directory inside the chroot eg. bindmount /etc/unbound/run/unbound, (for /etc/unbound as chroot and /run/unbound as pidfile dir).

We did this before but it was causing issues for some people and was dropped. Removing pidfile didn't work anyway for above reasons.

Unbound needs write permissions for two reasons, the pidfile removal and trust anchor updates. It depends on where you keep those files, and then unbound needs write permission on that. If chroot is used, those locations have to be inside the chroot. Unbound exits if the trustanchor cannot be written to, if that trustanchor is configured with auto updates, making sure that RFC5011 root trust anchor rollover works.

Ok, so this is what i expected. We have to keep those dirs writable in generic config then

@Frzk

FYI: http://0pointer.de/blog/projects/changing-roots

and trust anchor updates. It depends on where you keep those files, and then unbound needs write permission on that.

Sound like a good candidate for the StateDirectory= directive.

Using state dir under /var/lib is good practice in general but in reality neither of Arch or Debian even ship /var/lib/unbound in their packages. Nevertheless StateDirectory= is good candidate to portable services if you want to keep ConfigureDirectory not writable

@Frzk
Copy link
Contributor Author

Frzk commented Jan 27, 2020

@Frzk You may consider DynamicUser=yes for portable service.

Thanks, I'm already doing this along with PrivateUsers=true.

@Frzk

Sound like a good candidate for the StateDirectory= directive.

Using state dir under /var/lib is good practice in general but in reality neither of Arch or Debian even ship /var/lib/unbound in their packages.

Doesn't sound something really difficult to add a mkdir in a build package config file.

Nevertheless StateDirectory= is good candidate to portable services if you want to keep ConfigureDirectory not writable

Thanks, I'm already doing it in my unit file. It works well.

@Maryse47
Copy link
Contributor

@Frzk do you think we can add DynamicUser=yes to portable service in this repo?

Doesn't sound something really difficult to add a mkdir in a build package config file.

Adding StateDirectory=unbound will automatically create this dir but the point was that nobody uses it to write mentioned files so we can't rely on it and make @UNBOUND_RUN_DIR@ & @UNBOUND_CHROOT_DIR@ not writable.

Thanks, I'm already doing it in my unit file. It works well.

I think we can add it to both generic & portable unit.

@wcawijngaards
Copy link
Member

About the pidfile, the chown error is shown if you increase verbosity to 3 or higher. Right at the start. If you use log to syslog, it should be written to syslog. Otherwise, use unbound -dd to have it printed to stderr at startup (and unbound stays attached to the console).

Perhaps the systemcallfilter stops chown. Or a bindpaths to make the pidfile appear in the chroot, but for that chowning the pidfile has to work too.

Looking at the source, I see unbound only chowns, if the username is set in unbound config, (i.e. not disabled with username: ""), and if the pidfile is inside the chroot dir, eg. the pidfile pathname starts with the absolute path of chroot. pidfile: "/etc/unbound/run/unbound/unbound.pid" for chroot: "/etc/unbound" and I gues this could work with a BindPaths that makes /run/unbound appear there. This is probably why it is not happening now, because the pidfile is 'obviously' not accessible later, what with outside of the chroot.

You could cheat on it with ExecStartPost=chown ...thefile.. in the systemd service file, but that may be too ugly?

But maybe this does not work, in which case I do not want to waste your time on it.

@Maryse47
Copy link
Contributor

Maryse47 commented Jan 27, 2020

So I can see debug: cannot chown xxx.xxx /run/unbound.pid: Operation not permitted message. Unfortunately it's always reproducible, even after disabling all systemd hardenings, changing pidfile to /run/unbound/unbound.pid, etc. All of this with chroot disabled.

@wcawijngaards
Copy link
Member

You could try starting unbound from the commandline with unbound -dd -c /etc/unbound/unbound.conf, if that fails to chown too; then something is wrong with directory permissions, somehow. Or it does not fail, and then some setting in the systemd looks like it is it.

@Maryse47
Copy link
Contributor

Starting unbound -dd -c /etc/unbound/unbound.conf also doesn't work with same message. The /run dir has normal 755 permissions, /run/unbound.pid also has 755. Manually chowning pidfile works.

@wcawijngaards
Copy link
Member

Could it be that the /run dir group permission of 5 (readonly) is affecting this? I.e. not user root, but only group? If /run/unbound.pid is owned by the same user that started it, then I would expect it to work just like manually.

@wcawijngaards
Copy link
Member

if you manually chown with the numbers from the error message chown owner:group file that works, or is the group somehow illegal or something along those lines?

@wcawijngaards
Copy link
Member

Does unbound really make its file somewhere else, eg. it exists in some other directory, and the file in /run that you look at is an old file? (This happens to FreeBSD people a lot, because unbound is shipped in packages to /usr/local/etc/unbound and there is also a /etc/unbound in some cases). Also because of Debian's alternatives systems for systems like Ubuntu too, the files are somewhere else.

@Maryse47
Copy link
Contributor

Maryse47 commented Jan 27, 2020

Could it be that the /run dir group permission of 5 (readonly) is affecting this? I.e. not user root, but only group? If /run/unbound.pid is owned by the same user that started it, then I would expect it to work just like manually.

If unbound runs as root user then group access shouldn't affect it. This is really weird.

if you manually chown with the numbers from the error message chown owner:group file that works, or is the group somehow illegal or something along those lines?

When I chown this file with sudo it works without issues.

Does unbound really make its file somewhere else, eg. it exists in some other directory, and the file in /run that you look at is an old file?

I removed /run/unbound.pid if it existed. Starting unbound creates /run/unbound.pid but chmod fails.

@Maryse47
Copy link
Contributor

@wcawijngaards ah, sorry. I had apparmor profile for unbound with deny capability chown,. deny rules are silent in apparmor so this wasn't logged anywhere. Now unbound -dd -c /etc/unbound/unbound.conf does chown pidfile.

@wcawijngaards
Copy link
Member

Agah, I was already typing a reply, but apparmor could be it!

@wcawijngaards
Copy link
Member

wcawijngaards commented Jan 27, 2020

If it can chown, I suspect it can also unlink later on (with ReadWrite Paths and BindPaths and so on configuration adjusted for it). Because it chowns after checking the chroot pathname and capabilities are sortof similar. (Unless there is a separate unlink(2) apparmor rule (that is the file delete call it uses to delete the pidfile when unbound stops)).

@Maryse47
Copy link
Contributor

It still doesn't chown itwhen started from systemd so I will investigate it further.

@Frzk
Copy link
Contributor Author

Frzk commented Jan 27, 2020

How are you running it with systemd ?

@Maryse47
Copy link
Contributor

@wcawijngaards @Frzk so obviously CAP_CHOWN was missing from CapabilityBoundingSet. Adding it, chowning with systemd works I'll add it to my PR

Now, after stop pidfile is getting truncated but not removed not sure why but the same happens with unbound -dd -c /etc/unbound/unbound.conf and disabling apparmor doesn't help.

Above still doesn't work when chroot is being used but making pidfile available in chroot without breaking someones config seems rather impossible.

@edmonds
Copy link
Contributor

edmonds commented Jan 27, 2020

systemd doesn't need a pidfile. Just invoke unbound with the -p argument ("do not create a pidfile"). If you need to obtain the PID of the unbound daemon, run systemctl show --property MainPID --value unbound instead of cat /run/unbound.pid.

If you need a pidfile for some reason, use the systemd.service PIDFile= directive so that systemd enforces a pidfile cleanup when the service is shut down. Doing it this way means chown privileges are not needed by the service and write permission by an unprivileged user are not needed against the directory containing the pidfile.

https://www.freedesktop.org/software/systemd/man/systemd.service.html

PIDFile=

Takes a path referring to the PID file of the service. Usage of this option is recommended for services where Type= is set to forking. The path specified typically points to a file below /run/. If a relative path is specified it is hence prefixed with /run/. The service manager will read the PID of the main process of the service from this file after start-up of the service. The service manager will not write to the file configured here, although it will remove the file after the service has shut down if it still exists. The PID file does not need to be owned by a privileged user, but if it is owned by an unprivileged user additional safety restrictions are enforced: the file may not be a symlink to a file owned by a different user (neither directly nor indirectly), and the PID file must refer to a process already belonging to the service.

The ExecStop=+/bin/kill -TERM $MAINPID directive appears to be redundant. If I understand https://www.freedesktop.org/software/systemd/man/systemd.kill.html correctly, sending SIGTERM and then SIGKILL is already the default behavior for stopping a service. We don't actually set ExecStop in the Debian unbound.service file.

The set of capabilities granted in CapabilityBoundingSet seems fairly permissive. E.g., why are CAP_IPC_LOCK and CAP_NET_RAW needed? Some capabilities (e.g. CAP_SETGID, CAP_SETUID) can be removed if unbound is started unprivileged by systemd (https://www.freedesktop.org/software/systemd/man/systemd.exec.html#User=), and if capabilities support is added to unbound, the daemon could even drop a capability once it's no longer needed.

In another DNS server, its usage of capabilities was re-designed recently so that it could be started unprivileged and with minimal capabilities, and then all capabilities dropped after binding to port 53. See https://gitlab.labs.nic.cz/knot/knot-dns/issues/546#note_57098 and https://gitlab.labs.nic.cz/knot/knot-dns/merge_requests/864 for details.

@Maryse47
Copy link
Contributor

Maryse47 commented Jan 27, 2020

@edmonds

systemd doesn't need a pidfile. Just invoke unbound with the -p argument ("do not create a pidfile"). If you need to obtain the PID of the unbound daemon, run systemctl show --property MainPID --value unbound instead of cat /run/unbound.pid

That sounds like good idea. We can add -p to ExecStart then drop ReadWritePaths=/run. @wcawijngaards do you think there will be any downsides of that?

If you need a pidfile for some reason, use the systemd.service PIDFile= directive so that systemd enforces a pidfile cleanup when the service is shut down. Doing it this way means chown privileges are not needed by the service and write permission by an unprivileged user are not needed against the directory containing the pidfile.

The problem with that is pidfile path is configurable by user and may vary across distros. Dropping pidfile completely sounds better then.

The ExecStop=+/bin/kill -TERM $MAINPID directive appears to be redundant.

Yes, I already dropped it in my PR.

The set of capabilities granted in CapabilityBoundingSet seems fairly permissive. E.g., why are CAP_IPC_LOCK and CAP_NET_RAW needed? Some capabilities (e.g. CAP_SETGID, CAP_SETUID) can be removed if unbound is started unprivileged by systemd (https://www.freedesktop.org/software/systemd/man/systemd.exec.html#User=), and if capabilities support is added to unbound, the daemon could even drop a capability once it's no longer needed.

CAP_NET_RAW: needed for IP_TRANSPARENT socket option.

CAP_IPC_LOCK: I don't know. @wcawijngaards do you think unbound uses it for something or should we drop it?

CAP_SETGID, CAP_SETUID: as I pointed out in #150 (comment) running unbound completely rootless from the start need existing systems configured for that and I doubt they are.

@Frzk
Copy link
Contributor Author

Frzk commented Jan 27, 2020

Just invoke unbound with the -p argument ("do not create a pidfile")

If you need a pidfile for some reason, use the systemd.service PIDFile= directive

Not sure to follow here, but systemd will not create the pidfile (it's made clear in the quoted doc). It will only make sure it's removed:

The service manager will not write to the file configured here, although it will remove the file after the service has shut down if it still exists.

@wcawijngaards
Copy link
Member

CAP_IPC_LOCK: needed for mmap, shmctl and unbound can use that for statistics reporting. With unbound-control stats_shm.

No pidfile: yes we have that feature for people that want to have no pidfile. It does not do anything else.

@Maryse47
Copy link
Contributor

@wcawijngaards should I add -p to systemd service then? This will allow dropping ReadWritePaths=/run and CapabilityBoundingSet=CAP_CHOWN.

@wcawijngaards
Copy link
Member

@Maryse47 That or add the pidfile location to the PIDFile= directive like Robert (@edmonds) suggests, and then it would have been deleted by systemd at the exit of the service, it looks like. It is @UNBOUND_PIDFILE@

@edmonds
Copy link
Contributor

edmonds commented Jan 28, 2020

CAP_IPC_LOCK: needed for mmap, shmctl and unbound can use that for statistics reporting. With unbound-control stats_shm.

If I understand correctly, CAP_IPC_LOCK controls whether a process can lock pages into physical memory (for instance to prevent passwords or private keys from being swapped to disk), e.g. mmap() with the MAP_LOCKED flag or shmctl() with the SHM_LOCK command, neither of which seem to be used by unbound. So this capability appears unnecessary.

@wcawijngaards
Copy link
Member

Yes, I think you are correct, and unbound does not need it.

@Maryse47
Copy link
Contributor

@wcawijngaards I added -p and dropped CAP_CHOWN and ReadWritePaths=/run. I also dropped CAP_IPC_LOCK.

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.

4 participants