-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Testing] Use encrypted secrets #292
[Testing] Use encrypted secrets #292
Conversation
I just realized that we should support the more verbose secret specification like the docker cli does, see https://docs.docker.com/engine/reference/commandline/service_create/#create-a-service-with-secrets If we do this, it allows 3rd parties to provide more sophisticated secrets handling, such as mount secret $ docker service create --name redis \
--secret source=ssh-key,target=ssh The docker cli implements this advanced parsing here: https://github.com/docker/cli/blob/1401d5daf2f49a97791487dd5c5a8598907f0bf1/opts/secret.go#L19 I am currently in the process of trying to vendor the |
Is it still passed as a list of strings? |
guide/secure_secret_management.md
Outdated
```sh | ||
curl -H "Content-Type: application/json" \ | ||
-X POST \ | ||
-d '{"service":"nodeinfo","network":"func", "image": "functions/nodehelloenv:latest", "envVars": {"NODE_ENV": "production"}}' \ |
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.
NODE_ENV should be passed as environment rather than securely. Can we get a better example i.e. GITHUB_ACCESS_TOKEN or DOCKER_HUB_PASSWORD ?
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.
Also might be worth showing this via the CLI not curl.. it's too low-level.
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.
I can switch it to the cli, but this example is supposed to demonstrate using the environment variables with the later example showing the usage of secrets.
guide/secure_secret_management.md
Outdated
```sh | ||
curl -H "Content-Type: application/json" \ | ||
-X POST \ | ||
-d '{"service":"protectedapi","network":"func_functions", "image": "functions/api-key-protected:latest", "secrets": ["secret_api_key"]}' \ |
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.
Good to see re-use of the protected API example.
OK @LucasRoesler - marking as "WIP" |
I just pushed a change that vendors the I think this is ready now. |
@alexellis I'll have a look at this when I get out of work, looks good at first very cursory glance Something feels a bit iffy about pulling in the cli dependency for what is effectively a very light wrapper around the |
@johnmccabe I agree, but I didn't see a good alternative other than simple copy and paste of https://github.com/docker/cli/blob/master/opts/secret.go#L19 and I didn't want to take credit for their work. |
guide/secure_secret_management.md
Outdated
Hello from a production machine | ||
``` | ||
|
||
Where your `samples.yml` stack file looks like this |
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.
I'd consider moving the yaml before its invocation, as it flows more logically - ie here a secret in yaml, invoke the function, see the secret has been picked up
guide/secure_secret_management.md
Outdated
``` | ||
|
||
|
||
(_Optional_) If you are directly using the OpenFaaS Gateway API, then it will look like this |
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.
I had thought we were pushing people towards the CLI rather than providing curl examples - @alexellis wdyt?
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.
I can remove them completely, keep the guide simple. I have been directly using the api a lot, so it is where my first examples come from. The cli doesn't always expose all of the features of the gateway right away (or at least this was true in the past)
guide/secure_secret_management.md
Outdated
OpenFaaS deploys functions as Docker Swarm Services, as result there are several features that we can leverage to simplify the development and subsquent deployment of functions to hardened production environments. | ||
|
||
## Using Environment Variables | ||
First, and most simple, is the ability to set environment variables at deploy time. For example, you might want to set the `NODE_ENV` or `DEBUG` variable. Setting the `NODE_ENV` while using the `faas-cli` might look like this |
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.
Perhaps, "First, and least secure,"
guide/secure_secret_management.md
Outdated
|
||
|
||
## Using Swarm Secrets | ||
A very tempting thing to do is to now add database password or api secrets as environment variables. However, this is not secure. Instead, we can leverage the [Docker Swarm Secrets](https://docs.docker.com/engine/swarm/secrets/) feature to safely store and give our functions access to the needed values. Using secrets is a two step process. Take the [ApiKeyProtected](../sample-functions/ApiKeyProtected) example function, when we deploy this function we provide a secret key that it uses to authenticate requests to it. First we must add a secret to the swarm |
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.
If the envvar section begins pointing out its insecurity, I think you can drop the first two sentences here.
@@ -1,3 +1,11 @@ | |||
FROM golang:1.7.5 as builder |
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.
@@ -1,6 +1,6 @@ | |||
### Api-Key-Protected sample | |||
|
|||
To use this sample provide an env variable for the container/service in `secret_api_key`. | |||
To use this sample provide a secret for the container/service in `secret_api_key` using [Docker Swarm Secret](https://docs.docker.com/engine/swarm/secrets/#defining-and-using-secrets-in-compose-files). |
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.
Might read better with "in the secret_api_key
[Docker Swarm Secret].."
I don't quite follow the docker-compose link here, is this something to cover in the guide/needed?
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.
I will change it to link to the guide instead.
gateway/handlers/createhandler.go
Outdated
@@ -66,7 +75,8 @@ func MakeNewFunctionHandler(metricsOptions metrics.MetricOptions, c *client.Clie | |||
} | |||
} | |||
|
|||
func makeSpec(request *requests.CreateFunctionRequest, maxRestarts uint64, restartDelay time.Duration) swarm.ServiceSpec { | |||
func makeSpec(c *client.Client, request *requests.CreateFunctionRequest, maxRestarts uint64, restartDelay time.Duration) (swarm.ServiceSpec, error) { |
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.
Perhaps move the secrets creation (and client) out of the spec creation and just pass the created secrets in, the spec creation is just an assembly. Makes it easier to test.
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.
Can do, I waffled on this one, originally writing it as you suggest, so it will be easy to switch it.
gateway/handlers/createhandler.go
Outdated
@@ -66,7 +75,8 @@ func MakeNewFunctionHandler(metricsOptions metrics.MetricOptions, c *client.Clie | |||
} | |||
} | |||
|
|||
func makeSpec(request *requests.CreateFunctionRequest, maxRestarts uint64, restartDelay time.Duration) swarm.ServiceSpec { | |||
func makeSpec(c *client.Client, request *requests.CreateFunctionRequest, maxRestarts uint64, restartDelay time.Duration) (swarm.ServiceSpec, error) { | |||
linuxOnlyConstraints := []string{"node.platform.os == linux"} |
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.
this is already declared outside the function body
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.
must have gotten here while I was resolving a merge
secrets: | ||
type: array | ||
items: | ||
type: string |
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.
Can you add a description
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.
Done
guide/secure_secret_management.md
Outdated
Then, remove our old function and redeploy it with the new secret mounted in the same place as the old secret | ||
|
||
```sh | ||
$ echo "new$qzKzSJw51K9zP$pQ3R3N" | docker secret create secret_api_key_s - |
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.
this line should be removed?
-d '{}' \ | ||
http://localhost:8080/function/protectedapi | ||
|
||
Access denied! |
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.
Something a bit unexpected with the ApiKeyProtected example, its letting a sizeable substring match the key.
$ curl -H "Content-Type: application/json" -X POST -H "X-Api-Key: R^Y$qzKzSJw51K9zP$pQ3R3N" -d '{}' http://localhost:8080/function/protectedapi
Unlocked the function!
$ curl -H "Content-Type: application/json" -X POST -H "X-Api-Key: R^Y$q" -d '{}' http://localhost:8080/function/protectedapi
Unlocked the function!
$ curl -H "Content-Type: application/json" -X POST -H "X-Api-Key: R^Y$" -d '{}' http://localhost:8080/function/protectedapi
Access denied!
$ curl -H "Content-Type: application/json" -X POST -H "X-Api-Key: blahblahblah" -d '{}' http://localhost:8080/function/protectedapi
Access denied!
Are you seeing the same?
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.
I was able to reproduce using your curl command as well
➜ curl -H "Content-Type: application/json" -X POST -H "X-Api-Key: R^Y$q" -d '{}' http://localhost:8080/function/protectedapi
Unlocked the function!
That is really weird, the handler is fairly simple
func handle(body []byte) {
key := os.Getenv("Http_X_Api_Key")
secretBytes, err := ioutil.ReadFile("/run/secrets/secret_api_key")
if err != nil {
log.Fatal(err)
}
secret := strings.TrimSpace(string(secretBytes))
if key == secret {
fmt.Println("Unlocked the function!")
} else {
fmt.Println("Access denied!")
}
}
it seems like golang
is doing some sort of regex match instead
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.
Odd, if you test the string the go playground, it works as expected https://play.golang.org/p/EjWkzE1wXd
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.
This example includes the byte to string conversion and the trim: https://play.golang.org/p/yCHDYNPlkC but it still works as expected
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.
Just found the "bug", the $
is trying to do variable substitution in bash, so the secret actually looks like R^Y
inside the container and the key in the curl command R^Y$q
also looks like R^Y
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.
So how do we fix that?
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.
I don't think we can "fix" it because it is the terminal doing the variable substitution, by the time the value is accessible to any part of faas
or docker
, it has already been modified. I updated the walkthrough already to remove the $
that causes the issue. I think the only other thing we could do is warn people about using $
in a secret when creating the secret using echo
in the terminal. Basically, don't use $
if you plan on expressing the value in the terminal or ensure that you escape the $
correctly.
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.
OK 👍
guide/secure_secret_management.md
Outdated
(_Optional_) Directly using the API, the above looks like this. | ||
|
||
```sh | ||
$ curl -H "Content-Type: applicaiton/json" \ |
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.
applicaiton -> application
Looking pretty good, I've not dug into the secrets management on Swarm/K8s in detail yet so my comments are likely pretty superficial @alexellis for the ApiKeyProtected example do we expect access to such an API to be managed by something like Kong with it injecting the |
@johnmccabe I agree that you wouldn't normally use secrets like this to protect an API, Kong would be better for that, but I think most of the other examples where you would use this (DB password, or 3rd party API key) require a lot more setup to show the usage and might make the guide more difficult for people to use. Also, in this case, I was simply reusing an example that we already had. I could add a link to the Kong guide and recommendation to use it instead for running a secure API. |
I just pushed changes to address the comments on the previous diff |
I tested your PR, had no issues. Secrets are available in functions when deployed with |
I need to catch up on this and understand the equivalent for Kubernetes. Thank you for the work here. Would you be able to demo in the next contributor meeting? 22/10 |
I would be willing to do a demo, but I need to know the time of the meeting, I never received an actual invite. |
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. |
guide/secure_secret_management.md
Outdated
Note that unlike the `envVars` in the first example, we do not provide the secret value, just a list of names: `"secrets": ["secret_api_key"]`. The secret value has already been securely stored in the Docker swarm. One really great result of this type of configuration is that you can simplify your function code by always referencing the same secret name, no matter the environment, the only change is how the environments are configured. | ||
|
||
## Advanced Swarm Secrets | ||
For various reasons, you might add a secret to the Swarm under a different name than you want to us in your function, e.g. if you are rotating a secret key. The Docker Swarm secret specification allows us some advanced configuration of secrets [by supplying a comma-separated value specifying the secret](https://docs.docker.com/engine/reference/commandline/service_create/#create-a-service-with-secrets). The is best show in an example. Let's change the api key on our example function. |
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.
under a different name than you want to use in your function
Is this ready @LucasRoesler @ericstoekl ? |
I tested this and it is good to go. |
I am pushing a fresh rebase and a update for the typo in the guide |
67db65b
to
abdb6d9
Compare
**What** - During function creation, accept an array of strings defining swarm secrets that are required for the service - Update docs - Add new guide on using the secrets capability - Add new sample function to highlight using environment variables - Update `ApiKeyProtected` sample function to utilize the new secrets capabilities **Why** - This allows secrets to remain encrypted at rest instead of being unencrypted in environment variables and yaml files. Fixes openfaas#285 Signed-off-by: Lucas Roesler <lucas.roesler@gmail.com>
**What** - Add the ability to specify secrets as a csv - Vendor the docker/cli/opts - Update the guide for secrets to use the `faas-cli` **Why** - Allowing the csv specification of secrets gives users more control about how those secrets are mounted into the container. This is good for things like key rotation and for developers that are building on top of OpenFaaS. Signed-off-by: Lucas Roesler <lucas.roesler@gmail.com>
**What** - Add a description for the secret key to the api swagger spec. - Remove optional examples from the secret management guide. - Update the ApiKeyProtected README to point at the new guide. - Refactor the `makeSpec` function to accept the already assembled secrets array because this should be easier to unit test. Signed-off-by: Lucas Roesler <lucas.roesler@gmail.com>
Signed-off-by: Alex Ellis <alexellis2@gmail.com>
**What** - Pass secrets to the updateSpec method Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
**What** - Move the new secrets sample function to ApiKeyProtected-Secrets - Bring back the original ApiKeyProtected sample function Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
abdb6d9
to
8b16d67
Compare
Just pushed a fresh rebase the corrects the merge conflict |
``` | ||
|
||
Note that unlike the `envVars` in the first example, we do not provide the secret value, just a list of names: `"secrets": ["secret_api_key"]`. The secret value has already been securely stored in the Docker swarm. One really great result of this type of configuration is that you can simplify your function code by always referencing the same secret name, no matter the environment, the only change is how the environments are configured. | ||
|
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.
I haven't been able to get the secrets to work from the yml file... Here, you describe adding a "list of names", but there's no mention of it in the example above.
I've tried a yml style list:
functions:
secret_test:
lang: node
handler: ./secret_test
image: secret_test:test
secrets:
- API_SECRET_KEY
I've also tried matching your inline example format:
functions:
secret_test:
lang: node
handler: ./secret_test
image: secret_test:test
secrets: ["API_SECRET_KEY"]
with and without the quotes just in case
both with no luck.
Deploying using the cli did work though
faas-cli deploy -f secret_test.yml --secret API_SECRET_KEY
or, am I misreading this in that you don't define the secret in the yml file, but only through the cli?
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.
Your example yml files are definitely what is intended and I should update it. When you say deploying with the CLI worked, do you mean that faas-cli deploy -f secret_test.yml
did not work?
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.
Right.
So, setting the secrets only using the yml file and deploying with the cli faas-cli deploy -f secret_test.yml
I was not able to read the secret. This is the same with deploying via the UI (no secret available)
Deploying with the cli adding the additional secrets flag faas-cli deploy -f secret_test.yml --secret API_SECRET_KEY
did work and the secret is available
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.
Seems like it might be an error in the fass-cli
parsing the stack file, I will take another look
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.
@burtonr , are you updated to the latest version of the faas-cli? The secrets parsing was added fairly recently (last ~2 weeks)
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.
For the record, I just tried this out, deployed the apikeyprotected
function with the secret specified in the stack yaml, and the secret is showing up in the container (at /run/secrets/<secretname>
). @burtonr , are you seeing an issue with the secret not showing up in the container at all, or are you seeing the secret show up in the container but not be able to read the secret from its location in /run/secrets/
?
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.
@ericstoekl not sure what the difference is between being in the container, and being in the secrets location...
When testing this, I created a simple node function to read the secret from the /run/secrets/
location and it was throwing an error that the file didn't exist (the secret wasn't available in the function)
Also, the cli 0.5.1 hasn't been released yet. It's still marked as Pre-Release, which is why I don't have it yet. I'll grab the binary and try this out again.
The cli version compatibility should be a note in the guide 😉
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.
Alright, so, using the faas-cli version 0.5.1 with the secrets defined in the yml file:
functions:
secret_test:
lang: node
handler: ./secret_test
image: secret_test:test
secrets:
- API_SECRET_KEY
The secret is available and working as I expect it to.
Once the guide is updated to include these couple of little things I'd call this good to go!
Nice work @LucasRoesler!
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.
I have updated the guide with a note at the beginning of the section using Swarm secrets.
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.
Thanks for testing this out @burtonr
**What** - Add a note indicating that `faas-cli>=0.5.1` is required for the examples. Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
Thanks Lucas for working so hard on this and to others for collaborating. |
@@ -0,0 +1,9 @@ | |||
FROM golang:1.7.5 |
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.
Bit of a nit - I removed all the Dockerfile.build files etc in favour of multi-stage builds. Please could you update these two samples too?
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.
Just pushed an update that includes fixes for both sample functions
@@ -0,0 +1,13 @@ | |||
"use strict" |
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.
What is this sample for?
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.
I built this one to have a separate sample function to demonstrate accessing the ENV versus Secrets
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.
Just trying to work out if we still have the origin "ApiKeyProtected" example which used env-vars?
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.
I originally created this example for ENV and then update ApiKeyProtected
to use secrets. I was asked to put the original ApiKeyProtected
back and create a separate sample for secrets.
There should be both ApiKeyProtected
and ApiKeyProtected-Secrets
, the first uses ENV and the second secrets.
@@ -1,3 +1,4 @@ | |||
github.com/docker/cli/opts 08097edc7849e1543d723a3c382536909b58aa4f |
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.
What change needed the new vendored component?
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.
This was for processing secrets from strings, this allows for using the same style of arguments that the cli accepts. For example
source=mysql_password,target=wp_db_password,mode=0400
like they document at https://docs.docker.com/engine/swarm/secrets/#advanced-example-use-secrets-with-a-wordpress-service
Tested this briefly on my lunch break today. It appears to work well 🎉
|
Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
Merged. Thank you @LucasRoesler - we need to get the community to test the new release 0.16.15. |
Description
that are required for the service
ApiKeyProtected
sample function to utilize the new secretscapabilities
Motivation and Context
This allows secrets to remain encrypted at rest instead of being unencrypted
in environment variables and yaml files.
Fixes #285
How Has This Been Tested?
ApiKeyProtected
function to use the secrets functionalityTypes of changes
Checklist:
git commit -s