Support buildkit build time secrets #7046

antiBaconMachine · 2019-11-22T09:20:33Z

Hi everyone. This is one possible solution to build time secret mounting. I've used the build section in the compose yaml, I think there's a good argument to be made for supporting --build-secret on the CLI as well / instead.

I'm new to this project and I don't typically write python so all feedback welcome, what do people think of using the build section like this?

    build:
      context: .
      secrets:
        - "id=secret,src=${FOO_SECRET:-secret_1}"

The use case I originally wanted this for is mounting .npmrc so I imagine I would check in something like id=npmrc,src=${FOO_SECRET:-~/.npmrc} and then anyone with their .npmrc in the "normal" place would never have to think about this again. That is is the motivation for doing it this way first before the CLI which puts more burden on user.

If this PR isn't to your liking that is fine but you should still consider the first commit which fixes an existing test which does not behave as I think it was intended.

Resolves ##6358

…uilder as intended The existing test was passing DOCKER_BUILDKIT=1 as an env var but the test was passing without using buildkit Adds buildkit only features to the Dockerfile so that it will fail unless buildkit is invoked. It also ensures that the test environment is passed to the forked native build process Signed-off-by: Ollie Edwards <oliver.s.edwards@gmail.com>

This is done using the build section in the yaml which may look like a strange decision given build secrets are almost by definition not in a well known location. It's intended that this feature be used with env substitution. Signed-off-by: Ollie Edwards <oliver.s.edwards@gmail.com>

ndeloof · 2019-11-22T10:18:59Z

Sounds a reasonable approach.
I'd prefer we abstract away from buildkit CLI syntax, and define build secrets as an id=>value map:

secrets:
        secret_id: "${FOO_SECRET:-secret_1}"

which possibly could be extended for later additional parameters buildKit could introduce:

secrets:
        secret_id: 
			src: "${FOO_SECRET:-secret_1}"
			x-to-be-debined: value

As a side-note, when you consider working on such an enhancement, please first report an issue so this can be discussed with maintainers, and you don't waste time working hard on code that might not be later approved.

note: should also be implemented in https://github.com/docker/buildx for consistency

janbuchar · 2019-11-22T11:15:02Z

Can this also be used to the pass SSH environment (e.g., docker build --ssh default) to the builder?

ndeloof · 2019-11-22T12:21:50Z

@Teyras would need to look into details, but assuming BuildKit did define distinct build args for this purpose, we probably would better introduce ssh build option for this specific use-case. See #7025

antiBaconMachine · 2019-11-22T12:31:23Z

Thanks @ndeloof for the feedback. I've had a go at an abstract buildkit syntax let me know what you think.

It looks a bit odd as it's a list of strings or dicts. I didn't want to force people to use the full src form if all they need is id.

Thanks for the hint about opening an issue. In this case I didn't really set out to write a PR it just kinda went that way once I started reading.

Can you please clarify your comment on buildx, this feature is already implemented there, I'm not sure what action you are recommending?

antiBaconMachine · 2019-11-22T12:33:37Z

@Teyras No this is specifically to get parity with buildkit --secret flag but if the maintainers accept this solution or one like it, no reason the same approach couldn't be used for ssh as well. I've tried to keep the buildkit string builder generic to support such an eventuality.

ndeloof · 2019-11-22T12:48:57Z

General approach looks good to me an could be used to add support for buildkit's --ssh option.
On schema update, please note the leading schema is hosted on docker/cli (see #7047). Should also avoid something generic as dict and define explicit build_secret type with single property src for the current buildkit implementation, which could evolve in future with more options. Also, for schema consistency, please mimic existing syntax in use, as services:volumes: https://github.com/docker/cli/blob/master/cli/compose/schema/data/config_schema_v3.9.json#L287

I'll discuss with maintainers on way we'd like to proceed with such schema updates.

Rather than putting buildkit strings directly in compose file, use a mapping abstraction. The format is slightly odd looking string or dict as the only truly required mount arg is id but we may also want to provide a whole dict of options including source and any other flags that buildkit supports for a mount spec. Signed-off-by: Ollie Edwards <oliver.s.edwards@gmail.com>

antiBaconMachine · 2019-11-22T13:26:34Z

Thanks @ndeloof, have patched the last commit to be in line with volumes, thanks for pointing that out, I was looking for a good example to crib.

Shame about the schema replication issue, that does complicate things somewhat, but I'm sure there's a reasonable solution.

maxwellsmart84 · 2019-12-05T18:39:59Z

Do you guys know if there is a known workaround for doing ssh forwarding with docker-compose till a solution is introduced? I have tried many suggested methods but can't seem to get around password protected RSA keys.

NinoFloris · 2020-01-28T14:27:45Z

Would this also include a way to pass a secret as a CLI argument?

CpuID · 2020-02-06T19:54:11Z

Can't wait to see this PR get over the line ;)

bilby91 · 2020-02-10T18:31:34Z

This change would allow using mount=type=ssh ? Looking forward on this one!

dsvictor94 · 2020-02-18T01:51:24Z

What is the behavior when building without DOCKER_BUILDKIT=1 and COMPOSE_DOCKER_CLI_BUILD=1 present?

psigen · 2020-03-18T02:16:06Z

It appears that it should just ignore the secrets if DOCKER_BUILDKIT is not set:
https://github.com/docker/compose/pull/7046/files#diff-db70ffd20201b81c947f72fea35551d2R1812

psigen

Would it make sense to just delete the commented old code in the tests?

psigen · 2020-03-18T02:16:55Z

tests/integration/service_test.py

@@ -973,12 +974,49 @@ def test_build_cli(self):
        self.addCleanup(shutil.rmtree, base_dir)

        with open(os.path.join(base_dir, 'Dockerfile'), 'w') as f:
-            f.write("FROM busybox\n")
+            # f.write("FROM busybox\n")


Remove commented code?

Suggested change

# f.write("FROM busybox\n")

psigen · 2020-03-18T02:17:00Z

tests/integration/service_test.py

+        secret_path_2 = os.path.join(base_dir, 'secret_2')
+
+        with open(os.path.join(base_dir, 'Dockerfile'), 'w') as f:
+            # f.write("FROM busybox\n")


Remove commented code?

Suggested change

# f.write("FROM busybox\n")

thaJeztah · 2020-04-01T14:18:08Z

compose/config/config_schema_v3.7.json

@@ -88,7 +88,8 @@
                "cache_from": {"$ref": "#/definitions/list_of_strings"},
                "network": {"type": "string"},
                "target": {"type": "string"},
-                "shm_size": {"type": ["integer", "string"]}
+                "shm_size": {"type": ["integer", "string"]},
+                "secrets": {"$ref": "#/definitions/build_secrets"}


Note that we can't update the 3.7 schema, as it's already been released, so to add this property to the schema, it probably has to be added to the upcoming 3.9 schema in https://github.com/docker/cli/blob/master/cli/compose/schema/data/config_schema_v3.9.json first

CpuID · 2020-05-05T20:34:14Z

@antiBaconMachine @ndeloof next steps here...? :)

gnagel · 2020-05-12T12:09:08Z

I'm very excited to be able to use ssh secrets in the docker compose builds.

greg-caffeine · 2020-05-20T04:36:34Z

Just curious if anyone thinks using the existing secrets syntax (v3.1+) would be cleaner:

services:
  my-application:
    build:
      secrets:
        - my-secret

secrets:
  my-secret:
    file: /path/to/local/file

The secret name ("my-secret") would be passed in as the secret "id" and the path to the file would be passed in as the "src".

alechirsch · 2020-07-01T16:56:32Z

Looking forward to seeing ssh flag for compose files as well. We just started to use ssh for git keys and this is a blocker right now. Any update on a timeline for this?

eliliam · 2020-07-08T21:04:24Z

@antiBaconMachine could we get this finally merged in?

alechirsch · 2020-07-09T13:56:25Z

It looks like this PR does not address the ssh flag, and only does it for the secrets flag

Billy- · 2020-07-10T09:33:18Z

could we get this finally merged in?

Agreed, what do we need to do to make it happen?!

@antiBaconMachine @ndeloof next steps here...? :)

This is the question! What else needs to be done, anyone? @ndeloof ? Or @thaJeztah ?

Is it mainly this?

thaJeztah (Sebastiaan van Stijn) on 1 Apr
Note that we can't update the 3.7 schema, as it's already been released, so to add this property to the schema, it probably has to be added to the upcoming 3.9 schema in docker/cli@master/cli/compose/schema/data/config_schema_v3.9.json first
#7046 (comment)

@antiBaconMachine any chance you're still around and willing if this PR needs updates? If not, anyone else willing to contribute? I am more than willing, although may need some hand-holding 😅

Although part of me wants to say nothing and advocate merging this ASAP, I can't help but think this is a great suggestion:

Just curious if anyone thinks using the existing secrets syntax (v3.1+) would be cleaner:
services:
  my-application:
    build:
      secrets:
        - my-secret

secrets:
  my-secret:
    file: /path/to/local/file
The secret name ("my-secret") would be passed in as the secret "id" and the path to the file would be passed in as the "src".

RE

It looks like this PR does not address the ssh flag, and only does it for the secrets flag

Imo that seems out of scope for this PR.. looks like it might need a separate issue/PR based on this comment:

@Teyras would need to look into details, but assuming BuildKit did define distinct build args for this purpose, we probably would better introduce ssh build option for this specific use-case. See #7025

TechTeam12 · 2020-08-10T16:45:17Z

Is there any update on this? Anything else that needs to be done aside from handling merge conflicts? Happy to help if needed.

pikeas · 2020-12-15T18:18:36Z

Would love to see support for build secrets in compose!

otherguy · 2021-03-12T11:50:44Z

Any update on this?

jhagege · 2021-05-06T15:59:46Z

Hi, any plans to support build time secrets ?

devrushit · 2021-06-04T12:47:01Z

UP, any plans ? It's a much appreciated feature

okainov · 2021-10-22T08:15:34Z

@psigen @thaJeztah @antiBaconMachine @ndeloof is there any update here? What is still needed from this PR to be ready for merging?

thaJeztah · 2021-10-22T08:33:21Z

@psigen @thaJeztah @antiBaconMachine @ndeloof is there any update here? What is still needed from this PR to be ready for merging?

This requires changes in the compose specification, so likely needs to wait for compose-spec/compose-spec#81 (or alternatives) to be accepted by the spec maintainers

ndeloof · 2021-10-22T10:18:04Z

best option would be for the buildx maintainers to make a proposal based on the work done with buildx bake cc @tonistiigi @crazy-max

crazy-max · 2021-10-22T10:34:08Z

This requires changes in the compose specification

best option would be for the buildx maintainers to make a proposal based on the work done with buildx bake

Yes exactly, that's why we added the x-bake extension field: https://github.com/docker/buildx/blob/master/docs/reference/buildx_bake.md#extension-field-with-compose

ndeloof · 2021-11-05T19:59:15Z

@crazy-max custom extension allow to experiment without the need to get a long debate on the spec on some abstract solution, but this doesn't mean we can't discuss on the spec while bake is used as a concrete implementation example.
Do you plan to contribute an actual proposal to the spec?

crazy-max · 2021-11-05T21:25:50Z

@ndeloof

Do you plan to contribute an actual proposal to the spec?

I think the proposal compose-spec/compose-spec#120 from @chris-crone is already aligned with what the x-bake already offers but I agree that it should be part of the spec in the future for consistency.

jheld · 2022-01-24T17:07:01Z

Can someone make a comment about the overall process remaining (e.g. what steps and governing bodies are involved and where those decisions are currently) since it seems like there is some positivity here but still feels like "nothing" has happened?

ndeloof · 2022-01-24T17:11:09Z

changes on the compose schema MUST be debated under Compose Specification => Support for BuildKit extensions (secrets) compose-spec/compose-spec#81 (staled, mostly for missing contributors on this topics)
implementation will probably only happen on Compose v2 branch (v1 is in "best-effort" maintenance mode, i.e will receive security fixes but not much)

Would be nice to get buildx contributors involved here, as they introduced custom extensions for this purpose in bake.

ni11c · 2022-03-25T16:54:12Z

Guys, the issue has been submitted more than two years ago, still no concrete progress on this ? Unless there is a decent tool chain that enables docker users to apply best security practices, secrets will continue to be provided in an insecure way for a long time, which is in the interest of nobody. I don't understand how such a crucial feature can take so long to make its way through the intricacies of the Compose Specification process. Thank you to all those involved to make this happen,

dfr-exnaton · 2022-04-19T17:15:19Z

Checkout v2 of compose: #9386

ndeloof · 2022-04-19T17:25:07Z

build.secrets support has been introduced in Compose v2, and we don't plan to have significant updates on the v1 branch (security fixes/maintenance), so better adopt the new compose generation 😇

Kludex · 2022-04-19T17:42:09Z

Just for reference, merged on compose-spec/compose-spec#238

Ollie Edwards added 2 commits November 22, 2019 08:07

antiBaconMachine mentioned this pull request Nov 22, 2019

Support for docker build --secret for build-time docker secrets. #6358

Closed

ndeloof self-assigned this Nov 22, 2019

antiBaconMachine force-pushed the context-secrets branch from 89e6d53 to 2a195fd Compare November 22, 2019 13:21

antiBaconMachine changed the title ~~Support builkit build time secrets~~ Support buildkit build time secrets Nov 25, 2019

maxwellsmart84 mentioned this pull request Dec 5, 2019

Support buildkit? #6440

Closed

thaJeztah mentioned this pull request Dec 30, 2019

docker-compose requires build settings even with --no-build flag moby/moby#40327

Open

ulyssessouza added the format/v3 label Jan 9, 2020

psigen reviewed Mar 18, 2020

View reviewed changes

thaJeztah reviewed Apr 1, 2020

View reviewed changes

thaJeztah mentioned this pull request Apr 2, 2020

Add support to CLI build #6865

Merged

3 tasks

ppg mentioned this pull request May 11, 2020

Introduce a new env var "COMPOSE_DOCKER_CLI_BUILD_EXTRA_ARGS" #7296

Closed

jufemaiz mentioned this pull request Jul 14, 2020

Replace eval with xargs for parsing BUILDKITE_COMMAND buildkite-plugins/docker-compose-buildkite-plugin#165

Merged

Maks3w mentioned this pull request Nov 23, 2020

Scope limit secrets. Allow some secrets to be available on build time #7936

Closed

ppg mentioned this pull request Dec 30, 2020

Add build:cli_args to spec to remove strict coupling with CLI builds #8015

Closed

Niek mentioned this pull request Feb 18, 2021

Support for BuildKit extensions (secrets) compose-spec/compose-spec#81

Closed

Laura7089 mentioned this pull request May 7, 2021

Image may expose PANet credentials FragSoc/planetary-annihilation-docker#1

Open

ndeloof removed their assignment Oct 28, 2021

ndeloof added the Docker Compose V1 label Nov 24, 2021

firehydrant-ops mentioned this pull request Mar 28, 2022

Secrets not working on Windows (can´t bind .netrc) #9193

Closed

ndeloof closed this Apr 19, 2022

Support buildkit build time secrets #7046

Support buildkit build time secrets #7046

Conversation

antiBaconMachine commented Nov 22, 2019

ndeloof commented Nov 22, 2019

janbuchar commented Nov 22, 2019

ndeloof commented Nov 22, 2019

antiBaconMachine commented Nov 22, 2019 • edited Loading

antiBaconMachine commented Nov 22, 2019

ndeloof commented Nov 22, 2019

antiBaconMachine commented Nov 22, 2019

maxwellsmart84 commented Dec 5, 2019 • edited Loading

NinoFloris commented Jan 28, 2020

CpuID commented Feb 6, 2020

bilby91 commented Feb 10, 2020

dsvictor94 commented Feb 18, 2020

psigen commented Mar 18, 2020

psigen left a comment

Choose a reason for hiding this comment

psigen Mar 18, 2020

Choose a reason for hiding this comment

psigen Mar 18, 2020

Choose a reason for hiding this comment

thaJeztah Apr 1, 2020

Choose a reason for hiding this comment

CpuID commented May 5, 2020

gnagel commented May 12, 2020

greg-caffeine commented May 20, 2020

alechirsch commented Jul 1, 2020

eliliam commented Jul 8, 2020

alechirsch commented Jul 9, 2020

Billy- commented Jul 10, 2020

TechTeam12 commented Aug 10, 2020

pikeas commented Dec 15, 2020

otherguy commented Mar 12, 2021

jhagege commented May 6, 2021

devrushit commented Jun 4, 2021

okainov commented Oct 22, 2021

thaJeztah commented Oct 22, 2021

ndeloof commented Oct 22, 2021

crazy-max commented Oct 22, 2021

ndeloof commented Nov 5, 2021

crazy-max commented Nov 5, 2021

jheld commented Jan 24, 2022

ndeloof commented Jan 24, 2022

ni11c commented Mar 25, 2022

dfr-exnaton commented Apr 19, 2022

ndeloof commented Apr 19, 2022

Kludex commented Apr 19, 2022

antiBaconMachine commented Nov 22, 2019 •

edited

Loading

maxwellsmart84 commented Dec 5, 2019 •

edited

Loading