-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Remove "Docker Content Trust" #5896
base: master
Are you sure you want to change the base?
Conversation
Heh, well, proof's in the pudding (I've lit CI up nice and red to prove that I've clearly missed some things). I'm also definitely confused by |
❤️ I think I had a branch similar like this at some point. The "trust" code is definitely interweaved in many places; while no decision is made yet on the fate of DCT (still waiting on direction), I did start to somewhat untangle the trust code from "non-trust", with the potential to add a compile-time build-tag to disable it (replacing DCT code with stubs possibly). Some are merged, but I have some WIP changes stashed locally;
As to Sometimes removing those lines from |
Can you elaborate on this? Why would we want to keep the code / add even more complexity here? (Whatever new things we implement, they're not very likely to match the usage patterns of DCT, nor do we want them to, because we'd want something that's actually enforced in the daemon, not just a CLI toggle like all this is, right?) |
4f2c9ac
to
d01ea4e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5896 +/- ##
==========================================
+ Coverage 58.94% 60.02% +1.07%
==========================================
Files 355 337 -18
Lines 29772 27863 -1909
==========================================
- Hits 17550 16725 -825
+ Misses 11251 10211 -1040
+ Partials 971 927 -44 🚀 New features to boost your workflow:
|
1368752
to
708c315
Compare
It's wild that it's no longer static -- I'm not sure what to make of that. This PR: > [build 2/2] RUN --mount=type=bind,target=.,ro --mount=type=cache,target=/root/.cache --mount=type=tmpfs,target=cli/winresources xx-go --wrap && TARGET=/out ./scripts/build/binary && xx-verify $([ "static" = "static" ] && echo "--static") /out/docker:
0.116 Building static docker-linux-amd64
0.119 + go build -o /out/docker-linux-amd64 -tags ' osusergo' -ldflags ' -X "github.com/docker/cli/cli/version.GitCommit=329f3ef" -X "github.com/docker/cli/cli/version.BuildTime=2025-03-07T08:07:09Z" -X "github.com/docker/cli/cli/version.Version=pr-5896" -extldflags -static' '-buildmode=pie' github.com/docker/cli/cmd/docker
30.03 file /out/docker is not statically linked: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib/ld-musl-x86_64.so.1, Go BuildID=0tBrrCP3FiHoexzL_l-B/PXXtsAeztugHiuX3XRMi/cXimdL1dgW63WVVLy9mO/_PAXMfK1hwrvKusQVO0k, with debug_info, not stripped vs master: #18 [build 2/2] RUN --mount=type=bind,target=.,ro --mount=type=cache,target=/root/.cache --mount=type=tmpfs,target=cli/winresources xx-go --wrap && TARGET=/out ./scripts/build/binary && xx-verify $([ "static" = "static" ] && echo "--static") /out/docker
#18 0.122 Building static docker-linux-amd64
#18 0.125 + go build -o /out/docker-linux-amd64 -tags ' osusergo pkcs11' -ldflags ' -X "github.com/docker/cli/cli/version.GitCommit=ceef542" -X "github.com/docker/cli/cli/version.BuildTime=2025-03-06T17:11:33Z" -X "github.com/docker/cli/cli/version.Version=master" -extldflags -static' '-buildmode=pie' github.com/docker/cli/cmd/docker
#18 DONE 33.3s The only difference is the |
I guess we don't actually need cgo with this change, thanks to the removal of pkcs11/yubikey "support", which would mean we get static binaries by default without so many (attempted) backflips. 🤔 |
womp womp (but, mostly green otherwise!) |
Quite possible yes! I recall at least that most of the CGO-related bits were for content trust; I think most of the other code would be just run of the mill Go. Not sure if we still need the |
Yeah, I managed to fix static+cgo in 81e57b2, but we can probably flip the "default to cgo" code to not do that anymore (and leave the rest so other builders can choose cgo if they have a reason). |
cli/command/container/create.go
Outdated
pullAndTagImage := func() error { | ||
if err := pullImage(ctx, dockerCli, config.Image, options); err != nil { | ||
return err |
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.
After this, we can probably inline this pullImage
now, and get rid of this closure.
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.
pullAndTagImage
is used twice below - would you rather inline pullImage
or ditch pullAndTagImage
and use pullImage
twice? (the latter sounds cleaner IMO)
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 just pushed the latter in https://github.com/docker/cli/compare/81e57b2d5fc1a95aef77545e182e466aa97c25ce..a5312466d6359558cee6bba555bcbc7e592fb149, but of course I'm happy to adjust further)
As an opener, I want to make it clear that this is 100% a self-motivated change; I am not making this change on behalf of Docker Inc, nor does my opinion here represent Docker Inc in any official capacity (ie, if you're writing a splashy news article about this, you're barking up the wrong tree by attributing it officially to Docker Inc; I'm "rogue" here / doing this on my own time as a personally interested party). My biggest motivation for making this proposal is frankly the state of the upstream Notary (v1) project. It has been completely unmaintained for at least a full year, and mostly unmaintained for quite a few years. No matter what value this feature might have once had, it currently is vastly overshadowed by mass bitrot, and it is my argument/opinion that we are actively doing the community a very large disservice (and even harm) by continuing to "support" the feature in the Docker CLI. Given the state of the upstream project, it is my belief that this should qualify for an exception to our regular "deprecation" process such that we remove the feature *immediately*, and IMO we could very reasonably even consider backporting this deprecation to any past supported branches. Arguably, we should have some official means of integrating *other* "trusted image" solutions into the Docker platform, but IMO those belong in the Engine (unlike DCT which is entirely implemented in the CLI), and I see that (more complex) discussion as orthogonal to removing this bitrot. There are still quite a few `TODO` items here (most notably that we probably need some period of time with no-op/warning/erroring `--disable-content-trust=xxx` flags and deprecation documentation). I'm also certain I missed a few things, as I was mostly doing a pretty serious hack job to see how difficult this would be, not focused on creating a 100% optimal change (and this touches so many parts of the codebase that it's frankly more than I'm comfortable determining by myself whether I've made the changes correctly anyways). Signed-off-by: Tianon Gravi <admwiggin@gmail.com>
Signed-off-by: Tianon Gravi <admwiggin@gmail.com>
As an opener, I want to make it clear that this is 100% a self-motivated change; I am not making this change on behalf of Docker Inc, nor does my opinion here represent Docker Inc in any official capacity (ie, if you're writing a splashy news article about this, you're barking up the wrong tree by attributing it officially to Docker Inc; I'm "rogue" here / doing this on my own time as a personally interested party).
My biggest motivation for making this proposal is frankly the state of the upstream Notary (v1) project. It has been completely unmaintained for at least a full year, and mostly unmaintained for quite a few years (ref https://github.com/notaryproject/notary/pulls?q=is%3Apr + notaryproject/.github#70). No matter what value this feature might have once had, it currently is vastly overshadowed by mass bitrot, and it is my argument/opinion that we are actively doing the community a very large disservice (and even harm) by continuing to "support" the feature in the Docker CLI.
Given the state of the upstream project, it is my belief that this should qualify for an exception to our regular "deprecation" process such that we remove the feature immediately, and IMO we could very reasonably even consider backporting this deprecation to any past supported branches.
Arguably, we should have some official means of integrating other "trusted image" solutions into the Docker platform, but IMO those belong in the Engine (unlike DCT which is entirely implemented in the CLI), and I see that (more complex) discussion as orthogonal to removing this bitrot.
There are still quite a few
TODO
items here (most notably that we probably need some period of time with no-op/warning/erroring--disable-content-trust=xxx
flags and deprecation documentation). I'm also certain I missed a few things, as I was mostly doing a pretty serious hack job to see how difficult this would be, not focused on creating a 100% optimal change (and this touches so many parts of the codebase that it's frankly more than I'm comfortable determining by myself whether I've made the changes correctly anyways).