Add raw input in docker-compose for secrets and configs #712

WTFKr0 · 2017-11-23T22:11:51Z

Closes #320

- What I did
I add a new raw field for secret and config creation in docker-compose file

- How I did it
I changed the schema v3.5 to add this field
I modify the way config and secret are loaded from file to add option to load from a string

- How to verify it
With this compose file for example :

version: "3.5"
services:
  web:
    image: nginx
    configs:
      - source: rawconfig
        target: /rawconfig
    secrets:
      - onelineraw
      - multilineraw
      - filesecret
configs:
  rawconfig:
    raw: |
      Raw data
      of config
secrets:
  onelineraw:
    raw: "onelineraw data"
  multilineraw:
    raw: |
      a secret
      on

      multi lines
  filesecret:
    file: $PWD/mysecret

Once running, we got :

docker exec -it 6f6ce919151b grep "" /rawconfig /run/secrets/filesecret /run/secrets/multilineraw /run/secrets/onelineraw
/rawconfig:Raw data
/rawconfig:of config
/run/secrets/filesecret:sqdfsfsdfsdok
/run/secrets/multilineraw:a secret
/run/secrets/multilineraw:on
/run/secrets/multilineraw:
/run/secrets/multilineraw:multi lines
/run/secrets/onelineraw:onelineraw data

- Description for the changelog
Add raw input in docker-compose for secrets and configs

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: WTFKr0 <[email protected]>

codecov-io · 2017-11-23T22:37:41Z

Codecov Report

Merging #712 into master will increase coverage by 0.06%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master     #712      +/-   ##
==========================================
+ Coverage   51.81%   51.88%   +0.06%     
==========================================
  Files         218      218              
  Lines       17924    17934      +10     
==========================================
+ Hits         9288     9305      +17     
+ Misses       8160     8152       -8     
- Partials      476      477       +1

Signed-off-by: WTFKr0 <[email protected]>

dnephin

Thanks, this is looking good.

I think the only thing we're missing is an error when raw is used with file. That can be handled with a constraint in the schema.

WTFKr0 · 2017-11-24T21:01:28Z

Yeah, I'll try to add that
If raw and file are present, no error for the moment, and raw is taken in priority

WTFKr0 · 2017-11-24T21:16:35Z

Should I force user to set file or raw ?
It's no sense to create a config without one of those fields I think. I tried with that :

secrets:
  empty:
    name: plop

And docker stack deploy cli told me that my $PWD is a directory

dnephin · 2017-11-24T21:27:44Z

yes, one should be required

Signed-off-by: WTFKr0 <[email protected]>

WTFKr0 · 2017-11-24T22:37:27Z

After some tests, i think i got it with

      "oneOf": [
        {
          "required": ["file"],
          "not": {"required": ["raw"]}
        },
        {
          "required": ["raw"],
          "not": {"required": ["file"]}
        }
      ],

But errors messages are like that :

When neither file or raw :
configs.configname file is required
When both file and raw :
configs.configname Must not validate the schema (not)

WTFKr0 · 2017-11-24T23:00:28Z

Ahh need to allow no file and no raw for external config/secret...

Signed-off-by: WTFKr0 <[email protected]>

dnephin

Design LGTM

dnephin · 2017-11-28T14:11:16Z

cli/compose/schema/schema_test.go

+	}
+
+	err := Validate(config, "3.5")
+	assert.Error(t, err)


This should test for the error with EqualError(), or ErrorContains()

dnephin · 2017-11-28T14:11:22Z

cli/compose/schema/schema_test.go

+	}
+
+	err := Validate(config, "3.5")
+	assert.Error(t, err)


Signed-off-by: WTFKr0 <[email protected]>

dnephin

@thaJeztah do you see any issues with accepting secrets and configs from the Compose file?

dnephin · 2017-11-28T16:23:31Z

cli/compose/schema/schema_test.go

+	}
+
+	err := Validate(config, "3.5")
+	assert.EqualError(t, err, "configs.bar Must not validate the schema (not)")


We need to find some way to provide a better error message here.

It looks like these errors are handled in cli/compose/schema/schema.go:getMostSpecificError(). We probably need a special case for not that provides more detail.

ok thanx, will check that soon

Signed-off-by: WTFKr0 <[email protected]>

dnephin

LGTM

Thanks!

WTFKr0 · 2017-11-29T15:28:31Z

D'you want me to resolve the conflict by merging last master onto this branch and regenerate bindata ?

dnephin · 2017-11-29T15:30:15Z

Ah yes, please do

thaJeztah · 2017-11-29T17:23:54Z

do you see any issues with accepting secrets and configs from the Compose file?

The bit I'm worried about is that putting secrets in this file effectively nullifies all the security measures that went into the docker secrets design (i.e., keep the secrets secure, don't store them on disk). Putting a secret in the compose file is a big risk; i.e. compose files are usually committed to version control, which (if they contain secrets) would be a big no-no.

What are the benefits of putting the secrets in this file, instead of a separate file? (If you want to store the secret in a file)

thaJeztah · 2017-11-29T17:26:04Z

For configs, we can be a bit more flexible, BUT, in many occasions those may contain secrets as well, so I'd like moby/moby#33702 to be implemented so that secrets in config-files would no longer be needed (i.e. I can have a config-template that refers to a secret to use)

WTFKr0 · 2017-11-29T19:39:53Z

My use case is a docker-compose file commited with a demo secret or demo config, it's better for users who want to test an app to have a one command deployment (docker stack deploy) that do all things !
Also, we often used compose file for a one step deployment, even in production. I think we can modify compose to set config to wanted value, deploy, and remove compose-file
We used this with secret environment variable as well (for passwords for example)

thaJeztah · 2017-11-29T19:42:26Z

But adding a separate demo-secret to the repository, and reference that from the compose file would bring the same, correct?

WTFKr0 · 2017-11-29T19:44:36Z

We often just provide a docker-compose file, not an entire git repo with all files
It's more like a catalog of compose-files, ready to deploy, and the user can override some values on the fly with what he wants
It's for a use case like a stacks self service
Today we don't use configs/secret and prefer env vars to store that

Signed-off-by: WTFKr0 <[email protected]>

dnephin · 2017-11-30T19:44:20Z

The bit I'm worried about is that putting secrets in this file effectively nullifies all the security measures that went into the docker secrets design (i.e., keep the secrets secure, don't store them on disk).

Supporting the raw field actually improves the situation. Right now the only way to provide secrets in a Compose file is by putting them into a file. With raw someone can use environment variables and the secret will be interpolated into the file, without ever having to store it on disk.

vdemeester

Design LGTM
cc @thaJeztah

thaJeztah · 2017-12-12T02:53:14Z

Sorry, I'm still -1 on this; as @WTFKr0's use case describes; this is gonna be used to replace environment variables (separate from the problem that environment variables are really not suitable for storing secrets), and store the literal secrets in a compose file.

I would like the security team to review this before we go ahead

endophage · 2017-12-12T05:22:18Z

@dnephin do you mean apart from the compose file in which the secret is written to disk and even worse, likely committed in to a VCS in some repo not specifically designed for protecting secrets?

If the compose/stack file is committed in to a repo, there is no reason not to simply commit your demo secrets in to files beside the compose file, or as we do in notary, in to a folder called something like fixtures next to the compose file, making it clear that they are not production secrets.

Beyond secrets, how much testing has been done for different file formats in raw? Does yaml work? How about toml, or ini?

From a security perspective, I strongly dislike this. From a testing/quality perspective, it's completely lacking in tests for various popular config file types that consider whitespace important.

dnephin · 2017-12-12T15:32:20Z

do you mean apart from the compose file in which the secret is written to disk and even worse, likely committed in to a VCS in some repo not specifically designed for protecting secrets?

I don't understand what you're asking.

Currently there is only one way to use secrets/config with the Compose file. You must have them in a file on the local disk. By adding a raw field, we would support:

secrets:
  foo:
    raw: "$FOO_SECRET"

Which removes the need to store something on disk.

likely committed in to a VCS

I don't think this is a fair assumption. Even if it is committed to disk, isn't that choice (and the associated risk) obvious to the user? For most dev and testing use cases that is unlikely to be a problem.

how much testing has been done for different file formats in raw? Does yaml work? How about toml, or ini?

This is not a concern. Yaml supports raw string value, the content is irrelevant, it is always interpreted as a string literal.

In [1]: src = """
   ...: field: |
   ...:   inline:
   ...:     yaml: [works, fine]
   ...:
   ...: """

In [2]: import yaml

In [3]: yaml.load(src)
Out[3]: {'field': 'inline:\n  yaml: [works, fine]\n'}

WTFKr0 · 2018-01-18T21:11:54Z

Hey all

Any information on status of this PR pls ?

Thanx for reading

thaJeztah · 2018-01-19T15:55:04Z

Any information on status of this PR pls ?

I'm still not convinced we should do this 😅

Currently there is only one way to use secrets/config with the Compose file. You must have them in a file on the local disk. By adding a raw field, we would support:
..
raw: "$FOO_SECRET"

Secrets were explicitly designed to prevent using env-vars for secrets; that example (albeit not on the container itself) re-introduces the usage of environment variables to store them.

I also disagree that "there is only one way to use secrets/config"; secrets/configs can be created up-front (which allows creating them from stdin or whatever means), and can be referenced using external:.

dnephin · 2018-01-19T17:20:26Z

Secrets were explicitly designed to prevent using env-vars for secrets

Container/image "environment variables" are a problem because they are stored in the config and viewable from inspect. When the engine is a multi-user host, anyone with accept to the engine can see them. That's clearly a problem.

I believe regular process environment variables are only viewable from /proc by the user who created the process (or root). Why are those a problem? I don't think we should assume the two are the same thing.

WTFKr0 · 2018-01-31T19:26:49Z

I need that for configs too
The process for reading configs and secret is the same for the moment, this is why i do it for both
Are you OK if we enable this feature only for configs, and not for secrets ?

Thanx

WTFKr0 · 2018-02-15T13:36:58Z

Hi all
Any thoughts on my last post ?

thaJeztah · 2018-02-15T13:53:32Z

Also have a look at moby/moby#33702, which may have some overlap

dnephin · 2018-02-15T16:57:25Z

I don't think it overlaps.

It would be great to get this feature in for configs at least, if there are still concerns about using it for secrets.

WTFKr0 · 2018-09-18T20:37:00Z

Hey, just reviewing my old staled PR on github 😄
What do we do for this one ?

Thanx

WTFKr0 · 2019-10-24T18:44:31Z

Hi

Any update ??

WTFKr0 requested review from dnephin and vdemeester as code owners November 23, 2017 22:11

GordonTheTurtle added dco/no status/0-triage labels Nov 23, 2017

WTFKr0 force-pushed the raw-config branch from 6a2b473 to 6bcee39 Compare November 23, 2017 22:15

GordonTheTurtle removed the dco/no label Nov 23, 2017

WTFKr0 added 6 commits November 23, 2017 23:36

Add raw data in compose file for configs and secrets

34b1362

Signed-off-by: WTFKr0 <[email protected]>

gofmt

4eafc61

Signed-off-by: WTFKr0 <[email protected]>

Add tests for raw secret / config

d0c1874

Signed-off-by: WTFKr0 <[email protected]>

Add comma

8b18667

Signed-off-by: WTFKr0 <[email protected]>

gofmt

b83058d

Signed-off-by: WTFKr0 <[email protected]>

Modify assert

12f5103

Signed-off-by: WTFKr0 <[email protected]>

WTFKr0 force-pushed the raw-config branch from b6e08c2 to 12f5103 Compare November 23, 2017 22:37

WTFKr0 added 2 commits November 24, 2017 00:19

Fix test

b50ffa5

Signed-off-by: WTFKr0 <[email protected]>

Fix tests

d63df6c

Signed-off-by: WTFKr0 <[email protected]>

dnephin added status/1-design-review and removed status/0-triage labels Nov 24, 2017

dnephin requested a review from thaJeztah November 24, 2017 16:55

dnephin reviewed Nov 24, 2017

View reviewed changes

Add constraint for schema

c6762c0

Signed-off-by: WTFKr0 <[email protected]>

WTFKr0 added 2 commits November 25, 2017 00:14

Fix tests and permit external

8f1b99f

Signed-off-by: WTFKr0 <[email protected]>

lint

48d8590

Signed-off-by: WTFKr0 <[email protected]>

dnephin reviewed Nov 28, 2017

View reviewed changes

Adapt test error messages

479583f

Signed-off-by: WTFKr0 <[email protected]>

dnephin reviewed Nov 28, 2017

View reviewed changes

WTFKr0 added 2 commits November 29, 2017 01:03

Add custom error message if raw and file are provided

8f6c979

Signed-off-by: WTFKr0 <[email protected]>

Adapt test with ne error message

b99fe12

Signed-off-by: WTFKr0 <[email protected]>

dnephin approved these changes Nov 29, 2017

View reviewed changes

Merge branch 'master' into raw-config

85d17c5

Signed-off-by: WTFKr0 <[email protected]>

docker deleted a comment from GordonTheTurtle Dec 11, 2017

vdemeester reviewed Dec 11, 2017

View reviewed changes

thaJeztah added area/swarm area/stack labels Apr 30, 2022

dazinator mentioned this pull request Jul 7, 2023

docker swarm - deploying non external secrets without writing them to disk #4406

Open

Add raw input in docker-compose for secrets and configs #712

Are you sure you want to change the base?

Add raw input in docker-compose for secrets and configs #712

Conversation

WTFKr0 commented Nov 23, 2017 • edited Loading

codecov-io commented Nov 23, 2017 • edited Loading

Codecov Report

dnephin left a comment

Choose a reason for hiding this comment

WTFKr0 commented Nov 24, 2017

WTFKr0 commented Nov 24, 2017

dnephin commented Nov 24, 2017

WTFKr0 commented Nov 24, 2017

WTFKr0 commented Nov 24, 2017

dnephin left a comment

Choose a reason for hiding this comment

dnephin Nov 28, 2017

Choose a reason for hiding this comment

dnephin Nov 28, 2017

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

dnephin Nov 28, 2017

Choose a reason for hiding this comment

dnephin Nov 28, 2017

Choose a reason for hiding this comment

WTFKr0 Nov 28, 2017

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

WTFKr0 commented Nov 29, 2017

dnephin commented Nov 29, 2017

thaJeztah commented Nov 29, 2017 • edited Loading

thaJeztah commented Nov 29, 2017

WTFKr0 commented Nov 29, 2017

thaJeztah commented Nov 29, 2017

WTFKr0 commented Nov 29, 2017 • edited Loading

dnephin commented Nov 30, 2017

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Dec 12, 2017

endophage commented Dec 12, 2017

dnephin commented Dec 12, 2017

WTFKr0 commented Jan 18, 2018

thaJeztah commented Jan 19, 2018

dnephin commented Jan 19, 2018 • edited Loading

WTFKr0 commented Jan 31, 2018

WTFKr0 commented Feb 15, 2018

thaJeztah commented Feb 15, 2018

dnephin commented Feb 15, 2018

WTFKr0 commented Sep 18, 2018

WTFKr0 commented Oct 24, 2019

WTFKr0 commented Nov 23, 2017 •

edited

Loading

codecov-io commented Nov 23, 2017 •

edited

Loading

thaJeztah commented Nov 29, 2017 •

edited

Loading

WTFKr0 commented Nov 29, 2017 •

edited

Loading

dnephin commented Jan 19, 2018 •

edited

Loading