-
-
Notifications
You must be signed in to change notification settings - Fork 747
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
Adds docker image. #4372
Adds docker image. #4372
Conversation
@@ -0,0 +1,20 @@ | |||
FROM debian:stretch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker-library/official-images#1516
ends with:
So for most of us there is no way to run verified trusted
docker images in production. Very unfortunate.
I am not familiar with docker, but sounds pretty bad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not wrong, but in order for an attack like this to happen a malicious user would have to:
- Steal the Docker corporation's login credentials
- Point the Docker library repo from the official debian repo to somewhere else (or compromise the Debian repo)
- Put it up for review and hope nobody notices the alarm bells that will be going off everywhere
- Wait for the automated build to push the official image up (yet again that would throw a fit)
- Clean any cached images on a user's disks
- Re-pull and re-run images
All content (official and not) served from Docker is done so via HTTPS/TLS so stealing credentials should be the only vector of attack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, my idea of security is not to rely on some infrastructure being secure and nothing going wrong, but to just check a gpg signature. it doesn't matter how or from where you get the stuff as long as you can verify the sig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could pin to a sha256 hash of the base image instead of a tag, but then you'd have the maintenance burden of verifying every new version manually and updating this hash.
Gpg is no magic security fairy dust either, in Debian you also rely on the infrastructure of whoever created and distributes the signature. Unfortunately Docker images are not created in a deterministic way themselves (and this Dockerfile also adds additional nondeterminism on top by using whatever package version is current at build time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarkusTeufelberger if you have the correct public key of the release maintainer, you can verify all releases with their signature, no matter how you get the release or the release signature. you can not do that just with sha256.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a new feature Docker content trust, I'm not familiar with it yet, and this is an "enterprise feature" (at least in docker)... But other container runtime use it also (like cri-o) and other docker registry too (like quay.io). I propose that we investigate that in a future iteration.
one general thing: the whole docker image building should run inside the stretch64 vagrant vm. |
I vote for versioned building. We're asking for trouble always building the latest. |
Maybe it could be also moved to |
@pierreozoux are you still working on this? |
9091277
to
2ae2bcf
Compare
Codecov Report
@@ Coverage Diff @@
## master #4372 +/- ##
==========================================
+ Coverage 83.71% 84.38% +0.67%
==========================================
Files 37 37
Lines 9567 9826 +259
Branches 1596 1611 +15
==========================================
+ Hits 8009 8292 +283
+ Misses 1090 1074 -16
+ Partials 468 460 -8
Continue to review full report at Codecov.
|
Sorry for the delay, holiday.. ;) I put some brain juice on it. And about your questions, I think this PR is the best possible way to tackle that.
docker is a third party repo in debian. Copying from VM to local machine would have been a hassle, so I though it is just pain easier to build it from your dev machine. (as anyway you'll need the client for pushing to the hub).
I think I like that the Dockerfile is at the root. As a docker user, in a quick look, I can see docker support of a repo, but no strong opinion there.
I could have choosen another method, but I'm not really familiar with python. And so I though that the fat binary was equivalent to any other method. But I have no strong opinion there. If you have a strong opinion, we can change that.
It is just an attempt, if it creates too much debate, I'd prefer to not ship it with this PR, and open a separate PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some stuff i found.
scripts/docker/README.md
Outdated
--entrypoint=/usr/bin/borgbackup \ | ||
--name borg-backup \ | ||
borgbackup/borg | ||
docker rm borg-backup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably makes sense to just run with docker run --rm
so you don't have to run docker rm
afterwards. Nothing is saved inside the container (which is good), so it's best to be a good citizen and remove it afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is using docker volumes, so it would delete the associated volume.
scripts/docker/README.md
Outdated
|
||
``` | ||
docker pull borgbackup/borg | ||
docker run \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as you use json syntax (which we are) for the entrypoint, you don't need to do anything weird to pass arguments to the container.
You could use docker run --rm borgbackup/borg -v $YOUR_MOUNTS $NORMAL_BORG_OPTIONS
as if it were a binary.
Let me add a quick thank you for working on this guys! :) |
2ae2bcf
to
4396a8d
Compare
Ok, I separated the documentation from the binary and the convenience scripts (borgbackup and borgserver), I clearly tagged them as beta. |
@@ -0,0 +1,177 @@ | |||
## CLient side |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Client
@ThomasWaldmann if I remove all the client and server scripts, would you feel better about it? I can spend a bit of time to rework this to have a basic docker image at least. I'll be able to maintain it as well, as I'm still using this. |
Yeah, would feel better with a very basic thing. But as I don't use docker, I can't comment much about how that should look like. But there were some other reviewers here, maybe you can discuss about how a basic docker image for borg should look like, so that it is generally useful. |
Any updates on this? Would be nice to have an official containerd borg ! |
I am looking for an official image too |
@@ -0,0 +1,9 @@ | |||
FROM debian:stretch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sure this program needs Debian
You should use Alpine
Docker content trust works fine, you can read how to automate or use it here: https://github.com/sudo-bot/action-docker-sign
-e 's/^#PasswordAuthentication yes$/PasswordAuthentication no/g' \ | ||
-e 's/^PermitRootLogin without-password$/PermitRootLogin no/g' \ | ||
/etc/ssh/sshd_config | ||
if [[ $(ls /etc/ssh-keys/ssh_host_*) ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK one should not use ls in scripts/parse it. Okay, maybe there is not much parsing happening, but I you may anyway get crazy edge-cases/it is just not clean when you e.g. have spaces or so in there…
What I wanna say: Maybe there is a better solution? 🙂
Also, were these scripts tested with shellcheck? (Just pasted this script and it did not complain to my surprise, so yeah,hmm…)
from the borg command line to continue this Quick Start. | ||
|
||
Note that this example is using the `latest` version of borg, and we recommend you | ||
to pin the version by appending version e.g. `borgbackup/borg:1.0.7` to use `1.0.7`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm why would you recommend it?
I mean, maybe to not break setups the major version could be pinned aka borgbackup/borg:1
, but semantic versioning says everything else should not break the API…
|
||
To start a borg server: | ||
- generate an ssh-key | ||
- `ssh-keygen -f ./id_rsa -N '' -t rsa` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's 2021, not sure whether recommending RSA keys is still what we need. ED25519 could be used just as that, is shorter and at least as secure…
chmod 400 /etc/ssh-keys/ssh_host_* | ||
sed -i 's|/ssh/|/ssh-keys/|' /etc/ssh/sshd_config | ||
sed -i '/^#HostKey/s/^#//' /etc/ssh/sshd_config | ||
sed -e "s#SSH_KEY#${SSH_KEY}#g" /home/borg/authorized_keys.sample > /home/borg/.ssh/authorized_keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, where does this sample file come from? Debian I guess?
'
Okay, then it is empty.
Anyway, you should really suggest to save this file/dir in some (persistent) volume or prefill it (and not only SSHFS_IDENTITY_FILE
), because otherwise you can get client connection errors…
Or oh well, this is the server part, nevermind I comment above…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But should not this be configurable via a command-line, too? Otherwise, clients cannot connect if they are not authorized.
Also you'd like to restrict the commands to borg serve
for security reasons.
See https://borgbackup.readthedocs.io/en/stable/deployment/central-backup-server.html#restrictions and https://borgbackup.readthedocs.io/en/stable/deployment/hosting-repositories.html
The script here could do that too/configure it properly…
if [ -n "${SSHFS:-}" ]; then | ||
if [ -n "${SSHFS_IDENTITY_FILE:-}" ]; then | ||
if [ ! -f "$SSHFS_IDENTITY_FILE" ] && [ -n "${SSHFS_GEN_IDENTITY_FILE:-}" ]; then | ||
ssh-keygen -t rsa -b 4096 -N '' -f "$SSHFS_IDENTITY_FILE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again in 2021, we maybe should use ed25519 instead of RSA…
SSHFS_PASSWORD_OPT='' | ||
fi | ||
mkdir -p /mnt/sshfs | ||
eval "${SSHFS_PASSWORD} sshfs -o StrictHostKeyChecking=no ${SSHFS} /mnt/sshfs ${SSHFS_IDENTITY_FILE} ${SSHFS_PASSWORD_OPT}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StrictHostKeyChecking=no
? Please, by all means, do not do this. This image is likely intended to be used as a client with remote servers, so not checking the server key could allow MITM attacks…
So better pass in the host key either as a variable or just use SSH's default TOFU security scheme, at least – with allowing the user to confirm the step/check the key manually. After all, they will run this manually one time at least, so that should work.
Sorry, I'll close this PR, feel free to make your own PR based on this code or not. It was a really nice software, I really enjoyed it! We are now using s3 and minio, using versionning and mirroring of s3, basically our requirements changed from let's have our backups encrypted to let's have a mirror site that can start fast in case of emergency. |
Relates to #4364
This is the first step, and then once you detail what is the release process, we can add that later.
It always build latest, but if you want we can add an ARG with the version.