Deployment improvements #316

walac · 2017-09-20T16:56:40Z

A bunch of improvements for AMI building, towards deployment automation. Look at individual commits for details.

gregarndt · 2017-09-20T19:01:21Z

deploy/bin/import-docker-worker-secrets

@@ -0,0 +1,65 @@
+#!/bin/bash


I would like to get another set of eyes on this because of the security implications and also the use of password store (cc @djmitche)

What concerns me here is that we are dropping secrets into this directory. Wander said he would look into moving them elsewhere, but we also want to remove them afterwards so we do not have secrets floating around on people's machines.

Is there a way we could get these secrets from password store, and use them with the AMI generation, without having them dropped unencrypted on disk?

We can remove them at the end of the build script, I didn't do it because it was not done before and I thought there was a good reason for that. Btw, I changed the patch to put those into /tmp.

Update: notice that the import-docker-worker-secrets is just a utility script to automate what was previously done by hand.

So one way around this would be to use /dev/shm for the sensitive files -- and be careful to put them in an 0700 directory and remove them afterward.

gregarndt · 2017-09-20T19:03:24Z

deploy/packer/app.json

@@ -43,7 +43,7 @@
      "type":           "file",
      "direction":      "download",
      "source":          "/tmp/docker-worker.pub",
-      "destination":    "docker-worker-{{build_name}}-{{timestamp}}.pub"
+      "destination":    "docker-worker-{{build_name}}.pub"


the purpose of this was because we were always generating a new private key, so we had to extra the public key from the AMI. If we are reusing keys, we no longer need this at all.

I removed it in a later commit, if you look at the file changes tab, you will see this code section removed.

gregarndt · 2017-09-20T19:04:50Z

deploy/packer/app.json

@@ -79,5 +79,12 @@
        "Base_AMI":         "{{user `pvSourceAMI`}}"
      }
    }
+  ],
+  "post-processors": [


I didn't know about this. I'll have to read up. pretty cool!

also, packer build has a -machine-readable that might have helped too.

gregarndt · 2017-09-20T19:06:52Z

deploy/packer/app.json

+      "type":           "file",
+      "source":         "{{user `cotSigningKey`}}",
+      "destination":    "docker-worker-gpg-signing-key.key",
+      "only":           ["hvm-builder-trusted"]


djmitche

This works fine, just curious..

djmitche · 2017-09-21T15:26:49Z

deploy/bin/build

  # Package up the node app.
-  tar --exclude='./node_modules' --exclude='./*.pub' --exclude='./build/' --exclude='./deploy' --exclude='.test' --exclude='.git' --exclude='.vagrant' --exclude='./.DS_Store' --exclude='./vagrant/' --exclude='app.box' -zcvf docker-worker.tgz .
+  tar \
+    --exclude='./node_modules' \


why not just --exclude=./docker-worker.tgz?

That was my first try, but if the file doesn't exist beforehand, tar ignores the --exclude option.

Oh right.. recalls vague memories from work at Zmanda

djmitche · 2017-09-21T15:27:23Z

Oh, that was only looking at one commit, sorry

djmitche

nice work :)

djmitche · 2017-09-21T15:29:36Z

deploy/bin/import-docker-worker-secrets

@@ -0,0 +1,65 @@
+#!/bin/bash


So one way around this would be to use /dev/shm for the sensitive files -- and be careful to put them in an 0700 directory and remove them afterward.

djmitche · 2017-09-21T15:30:39Z

deploy/bin/import-docker-worker-secrets

+# See ssh://gitolite3@git-internal.mozilla.org/taskcluster/secrets.git
+# for details.
+
+export PASSWORD_STORE_GPG_OPTS="$*"


This probably deserves a header comment too? Or maybe just generally the user is expected to pass in any necessary PASSWORD_STORE_.. env variables to make pass work? (for example, I need to set PASSWORD_STORE_DIR)

This is intentional to use the command line to pass arguments to gpg using the command line. In special, the --passphrase option so the user doesn't need to type the passphrase each time pass is called.

Good catch on the chmod, I added it. The intention of this script is just to automate the generation of deploy.json file, which has been done manually so far. As we will soon move to automated deployment, I rather prefer not to invest much more time on this as it will be obsolete soon.

djmitche · 2017-09-21T15:31:06Z

deploy/bin/import-docker-worker-secrets

+
+base_dir=/tmp
+
+pass show aws/workers-key > $base_dir/docker-worker.key


Is set -e enabled here? If this (or the later pass invocations) fails, the script should fail..

djmitche · 2017-09-21T15:32:07Z

deploy/bin/import-docker-worker-secrets

+
+export PASSWORD_STORE_GPG_OPTS="$*"
+
+base_dir=/tmp


setting this to mktemp -d /dev/shm/docker-worker-XXXXX would make me feel a lot better :)

The problem is that this is not portable. macOS, for example, doesn't implement /dev/shm.

djmitche · 2017-09-21T15:37:07Z

deploy/bin/build

+      | flatten
+      | { (.[0]): .[1], (.[2]): .[3], (.[4]): .[5] }
+    }
+  ' packer-artifacts.json | jq -s add > docker-worker-deploy.json


that's some serious jq ninjery there.

djmitche · 2017-09-21T15:40:37Z

deploy/bin/gen-signing-key

+key=$(gpg --list-key $name | head -1 | awk '{print $2}' | awk -F/ '{print $2}')
+fingerprint=$(gpg --fingerprint $key | grep "fingerprint" | awk -F= '{print $2}' | tr -d " ")
+
+echo "Exporting public signing key"


s/public/private/

djmitche · 2017-09-21T15:40:53Z

deploy/bin/gen-signing-key

+  %echo Done generating key
+EOF
+
+echo "Generating public signing key"


s/public //

(it really generates both, right?)

djmitche · 2017-09-21T15:44:46Z

deploy/bin/build

  # Package up the node app.
-  tar --exclude='./node_modules' --exclude='./*.pub' --exclude='./build/' --exclude='./deploy' --exclude='.test' --exclude='.git' --exclude='.vagrant' --exclude='./.DS_Store' --exclude='./vagrant/' --exclude='app.box' -zcvf docker-worker.tgz .
+  tar \
+    --exclude='./node_modules' \


Oh right.. recalls vague memories from work at Zmanda

jonasfj

jonasfj · 2017-09-21T23:08:29Z

deploy/packer/app.json

+    },
+    {
+      "type":           "file",
+      "source":         "{{user `cotSigningKey`}}",


Are generating the private key on our host/master machine and then copying it to the AMI?

Note: I think it would be slightly safer to generate the key on the AMI and copy out the public key, that way it ever only exists on the worker AMI, the private key never gets into memory or disk of the host/master machine...

Granted this might be a follow up someday when we have time :)

We're using the same private/public keypair on all AMIs.

walac · 2017-09-22T12:39:31Z

I think I fixed all pointed issues. Thank you all for the valuable code review :)

deploy/bin/build invokes tar to compress the repository, but it creates a race condition because docker-worker.tgz, which is the name of the compressed file, is stored inside the same directory as the repo, so tar may try to include docker-worker.tgz in the compressed file, which is docker-worker.tgz, then tar will detect that docker-worker.tgz was modified while reading it. The way to fix this is generating docker-worker.tgz out of the tree and then moving it in, after tar is done.

Generating deploy.json file can a tedious and error prone task, opnening the possibility to deploy insecure docker-worker AMIs in EC2. We add the import-docker-worker-secrets script that reads credentials from the secrets [1] repo and generates the deploy.json file. [1] ssh://gitolite3@git-internal.mozilla.org/taskcluster/secrets.git

Currently, the AMI ids for a newly generated image is only printed in stdout. This means that, from a CI POV, the only way to retrieve it is from output logs, and if a tool needs to save it, it needs to parse the output and filter out the ids. Worse, the AMI info logs are multilined, so we need a context free parser to safely extract AMI ids. This blocks we from using the wonderful line oriented Unix text command line tools. We solve this probem but storing the AMI ids in a json file that then can be uploaded to a remote storage, like Taskcluster artifacts or S3 buckets. packer has the option to generate a manifest json file with artifacts information, and from this file we extract the AMI ids and store in a new json file that's easier to read.

So far, each time we build a new AMI for docker-worker trusted worker-types, we generate the cot signing key pair during AMI creation, extract the public key and add it to the cot repo. The original mindset is that the private key should never be in anyone's hands. A careful threat modeling shows that the private key is exposed to anyone with AWS credentials. Conclusion: the private is as safe as the AWS credentials are. Therefore, generating a new key pair each time a AMI is built is useless, and adds complexity to AMI rollout (push pub key to cot repo) for no reward. That said, move to use a permanent signing key instead of generating a new one on each AMI build. We now have a third AMI set, called "trusted", which ships the cot private key. The private key is stored in passwordstore. We also add a couple of utility scripts: * gen-signing-key: generate a new signing key pair. Useful when we need to rotate signing keys. * cleanup-images: given a packer-artifacts.json file, it deregister the referenced AMIs.

walac requested a review from gregarndt September 20, 2017 16:56

gregarndt reviewed Sep 20, 2017

View reviewed changes

walac requested review from djmitche, jonasfj and gregarndt September 20, 2017 19:17

walac force-pushed the automated-deployment branch from 140b39c to 5ced190 Compare September 20, 2017 19:29

djmitche approved these changes Sep 21, 2017

View reviewed changes

djmitche self-requested a review September 21, 2017 15:27

djmitche approved these changes Sep 21, 2017

View reviewed changes

jonasfj approved these changes Sep 21, 2017

View reviewed changes

walac force-pushed the automated-deployment branch from 5ced190 to 98d604e Compare September 22, 2017 12:38

Wander Lairson Costa added 2 commits September 22, 2017 13:29

walac force-pushed the automated-deployment branch 2 times, most recently from 9bbec42 to 960e708 Compare September 22, 2017 16:54

Wander Lairson Costa added 2 commits September 22, 2017 13:55

walac force-pushed the automated-deployment branch from 960e708 to 7f32bb1 Compare September 22, 2017 16:55

walac merged commit 2724185 into taskcluster:master Sep 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deployment improvements #316

Deployment improvements #316

walac commented Sep 20, 2017

gregarndt Sep 20, 2017

walac Sep 20, 2017 •

edited

djmitche Sep 21, 2017

gregarndt Sep 20, 2017

walac Sep 20, 2017

gregarndt Sep 20, 2017

gregarndt Sep 20, 2017

djmitche left a comment

djmitche Sep 21, 2017

walac Sep 21, 2017

djmitche Sep 21, 2017

djmitche commented Sep 21, 2017

djmitche left a comment

djmitche Sep 21, 2017

djmitche Sep 21, 2017

walac Sep 22, 2017

walac Sep 22, 2017

djmitche Sep 21, 2017

walac Sep 22, 2017

djmitche Sep 21, 2017

walac Sep 22, 2017

djmitche Sep 21, 2017

djmitche Sep 21, 2017

djmitche Sep 21, 2017

djmitche Sep 21, 2017

jonasfj left a comment

jonasfj Sep 21, 2017

djmitche Sep 22, 2017

walac commented Sep 22, 2017


		base_dir=/tmp

		pass show aws/workers-key > $base_dir/docker-worker.key

Deployment improvements #316

Deployment improvements #316

Conversation

walac commented Sep 20, 2017

Choose a reason for hiding this comment

walac Sep 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djmitche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djmitche commented Sep 21, 2017

djmitche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasfj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walac commented Sep 22, 2017

walac Sep 20, 2017 •

edited