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

Fix race condition causing containers to fail to start. Fixes #6. #7

Merged
merged 2 commits into from Oct 28, 2021

Conversation

SharkWipf
Copy link
Contributor

This change fixes host firewall race conditions causing containers to sometimes fail to start.
In order to avoid concurrent modification, iptables tracks locks in the file /run/xtables.lock.
In order for iptables inside a VM to respect the locking, it will need access to this file as well (moby/moby#12547 (comment)).

The downside of this implementation, as explained in above linked comment, is that, in some configurations, the lockfile may not exist when the container is started, and Docker will create it as a directory.
Sadly there's no reliable/non-hacky way to avoid this within the scope of this project that I know of, as any method to create this file would require root access, and the container user on the host is not likely to be root.

@mitar
Copy link
Member

mitar commented Oct 22, 2020

Thanks. This looks awesome. But why not mount whole /run directory inside?

@@ -10,3 +10,6 @@ containers external IP, iptables will be configured to route container's traffic
A chain named `EXTERNAL_IP` is created in the `nat` table into which all the rules are added.
And one more empty chain is created after this one for any additional custom rules you might want
to add, named `AFTER_EXTERNAL_IP`.

Please make sure `/run/xtables.lock` exists on the host before starting the container.
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this mount is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some extra explanation in the latest commit

README.md Outdated Show resolved Hide resolved
@SharkWipf
Copy link
Contributor Author

Sorry for the delay, only just got back to this.

Thanks. This looks awesome. But why not mount whole /run directory inside?

This would expose a lot more than necessary, potentially even sensitive data.
This container already needs more access than most containers would need to do its job, and it could already potentially gain a lot of access to the host, but it's still good practice to not expose anything more than strictly necessary IMO.
On top of that, while this container doesn't currently run anything that uses /run directly that isn't already passed to the container, adding a broad mount like /run would increase the risk of conflicts in this directory, and potentially decrease portability.

I've updated and implemented the feedback in the latest commit in the PR, if there's anything else you'd like to see changed please let me know.

@mitar
Copy link
Member

mitar commented Oct 28, 2021

This would expose a lot more than necessary, potentially even sensitive data.

I mean, with access to docker.sock the container has literally root access on the host. So I am not sure if being precise about /run mount is really important and maybe it is just a security theater.

But having a file-only mount means that you have the whole issue with the file maybe not existing on the host.

preventing race conditions that can cause containers to fail to start.
If this file does not exist, Docker will incorrectly create it as a directory, which may cause issues both on the host and with the container.

=======
Copy link
Member

Choose a reason for hiding this comment

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

This is leftover from some bad merge?

@mitar mitar merged commit 51df5c5 into tozd:master Oct 28, 2021
@tozd tozd locked and limited conversation to collaborators Oct 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants