Skip to content
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

RFC: Support Dockerfiles #173

Merged
merged 45 commits into from
Aug 10, 2022
Merged

RFC: Support Dockerfiles #173

merged 45 commits into from
Aug 10, 2022

Conversation

sclevine
Copy link
Member

Readable

This proposal is part of a larger initiative to reduce complexity originally outlined in #167

Signed-off-by: Stephen Levine <[email protected]>
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
[unresolved-questions]: #unresolved-questions

- Should `genpkgs` be part of this proposal? Opinion: Yes, otherwise it's difficult to maintain a valid SBoM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this weaken the security stance of buildpacks? (My opinion: yes) You can literally do anything in a Dockerfile.

I feel like stack packs, while maybe not the answer, did put more controls around what you could do. The stack packs were buildpacks, so presumably (IDK exactly as it wasn't implemented), an Operations team could control which stack packs are available to their users, thereby limiting what their users could install or modify to some degree.

There's also a stronger guarantee that a stack pack would produce a legitimate BOM, since Operations teams can control what stack packs are available (and audit them). With this proposal, someone could make a Dockerfile that installs something malicious and set genpkgs to be a copy of the true binary or some other no-op binary. The proposal essentially trusts the creator of the Dockerfile to be honest and report what they've installed, which I don't think is merited givn that this proposal would allow any app dev to customize what's install with a Dockerfile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operations teams can control what stack packs are available (and audit them)

I don't have any answers here, but this seems like a fairly important point to linger on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this concern is pretty well addressed by including the hooks on the builder. If the app-specified Dockerfile hook (above example) is not present, then the developer cannot provide a Dockerfile. And I suppose that trusting hook authors to provide a valid BOM is the same as trusting buildpack authors to provide a valid BOM.

In the case of the app-specified Dockerfile, the hook author could apply some label to the image so that consumers would know to approach the BOM with some suspicion. I wonder if there's value in having that spec'd ('io.buildpacks.extended'?). It's just a thought.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of a label to easily identify images that have had any hooks executed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also /cnb/image/genpkgs (because it is executed after Dockerfiles have been applied) is helping to trust the BOM.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also /cnb/image/genpkgs (because it is executed after Dockerfiles have been applied) is helping to trust the BOM.

@natalieparellano I don't really understand how this helps. By the time it is invoked, the hook author could have replaced /cnb/image/genpgks with a shell script that writes anything to the BOM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samj1912 and I have recently been discussing having the platform inject genpgks. That may help here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samj1912 and I have recently been discussing having the platform inject genpgks. That may help here...

How would that work? I don't think a mount would work, since the hook could discover and overwrite it. Since the hook could do anything, I'm not even sure running a program like genpgks is safe even if the binary was untouched. I would think the genpgks would almost have to be executed in a new container against the extended image to truly be accurate.

That said - these hooks are likely authored and for sure approved/distributed by the stack author. Just because Dockerfiles are used, doesn't mean the stack distributor has to extend that control to buildpack authors or the app author. They can create a much more simple constraint (like defining an Aptfile to read package names).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think the genpgks would almost have to be executed in a new container against the extended image to truly be accurate.

That's basically what we're proposing - please see https://github.com/buildpacks/rfcs/pull/173/files#r794608075 for the download I got from @samj1912 and @sclevine .

text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
@cmoulliard
Copy link

Will it be possible to run the build using a root user as proposed here: https://github.com/buildpacks/rfcs/blob/root-buildpacks/text/0000-root-buildpacks.md ?

@sclevine
Copy link
Member Author

sclevine commented Jul 1, 2021

Will it be possible to run the build using a root user as proposed here: https://github.com/buildpacks/rfcs/blob/root-buildpacks/text/0000-root-buildpacks.md ?

Only the Dockerfiles can run as root. This proposal does not allow buildpacks to run as root.

text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
@cmoulliard
Copy link

cmoulliard commented Jul 7, 2021

Only the Dockerfiles can run as root. This proposal does not allow buildpacks to run as root.

So you confirm that the limitation (see hereafter) will not be fixed by this RFC ? @sclevine

"The execution of a privileged command such as "yum install", as declared within a "build bash script", will fail due to the following error This command has to be run with superuser privileges (under the root user on most systems).
The error is due to the fact that the container created by "pack build" is using the UID/GUID of the build image and never the root user"

If the answer is Yes, how will it be then possible to support such a use case (see example hereafter) ?

Example proposed within the RFC69:

[buildpack]
id = "example/cacerts"
privileged = true

[[stacks]]
id = "io.buildpacks.stacks.bionic"

...

bin/build

#!/usr/bin/env bash

# filter to the cert
for file in $(cat $3 | yj -t | jq -r ".entries | .[] | select(.name==\"cacert\") | .metadata | .file"); do
 cert="$(cat $3 | yj -t | jq -r ".entries | .[] | select(.name==\"cacert\") | .metadata | select(.file==\"$file\") | .content")"
 echo $cert > /usr/share/ca-certificates/$file
 chmod 644 /usr/share/ca-certificates/$file
done

update-ca-certificates

Using the RFC69 ? Is it approved and implemented ?

@sclevine @jkutner

@sclevine
Copy link
Member Author

sclevine commented Jul 7, 2021

RFC0069 creates a special type of buildpack that can run as root. This proposal would revert RFC0069 and instead require Dockerfiles to run commands as root.

@natalieparellano
Copy link
Member

The RFC text in its current state reflects the implementation proposed in buildpacks/spec#307 and buildpacks/spec#308. Could everyone please re-read and add your review comments and/or approval, so that this could (hopefully) be merged? cc @buildpacks/toc @buildpacks/platform-maintainers @buildpacks/implementation-maintainers @buildpacks/buildpack-authors-tooling-maintainers @buildpacks/distribution-maintainers

text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
Comment on lines +124 to +129
##### `/cnb/ext/com.example.appext/bin/generate`
```
#!/bin/sh
[ -f build.Dockerfile ] && cp build.Dockerfile "$1/"
[ -f run.Dockerfile ] && cp run.Dockerfile "$1/"
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we are allowing static Dockerfiles in the case where there is no bin/generate script, is this example misleading? Should this instead be an example w/o a generate executable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this example is copying the Dockerfiles from the app directory to the output directory

text/0000-dockerfiles.md Outdated Show resolved Hide resolved
Signed-off-by: Natalie Arellano <[email protected]>
- Change launch.toml -> run.toml since there are no overlapping fields between buildpacks and extensions
- Remove Dockerfile without a prefix since the args must be specified in either build.toml or run.toml
- Change must -> should for defaulting build_id
- Nest pre-populated output files under detect or generate (future proofing)
- Mention how we'll preserve top layer information for rebase

Signed-off-by: Natalie Arellano <[email protected]>
Copy link
Member

@ekcasey ekcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My sincere apologies for getting pedantic about the paths. My approval stand regardless :)

text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
text/0000-dockerfiles.md Outdated Show resolved Hide resolved
Co-authored-by: Emily Casey <[email protected]>

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Copy link
Member

I believe this RFC now has all the required approvals for FCP. Given @jromero is out, I'm going to put the closing date as Wednesday, 7/27.

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Copy link
Member

This is well past FCP close date, could it be merged? It's worth noting, due to the complexity involved and the phased implementation, there are still some ambiguities outstanding (in particular around run image extension and how rebase is going to work). But I think we have clearly decided to proceed here. If necessary I am happy to "amend" the RFC with information learned from the implementation.

@natalieparellano
Copy link
Member

^^ cc @buildpacks/team-leads @hone (as the assignee)

@natalieparellano
Copy link
Member

For anyone interested in following the implementation of this RFC, the status can be found here: #224

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period roadmap/cloud-ecosystem Issues/PRs that provide a better out-of-the box Kubernetes and Docker integration story. spec/buildpack spec/platform team/implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.