-
-
Notifications
You must be signed in to change notification settings - Fork 13
cleanup duplicate credential helpers #237
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
base: main
Are you sure you want to change the base?
Conversation
|
not sure whether actually nobody uses them. what if i configure the config.json as described in the readme { "credHelpers": { "mycr.azurecr.io": "acr-env" } }will it then go via the external binary instead of the statically linked source? Let me find who first statically linked them in. |
|
indeed, so you can still sideload your custom env-helpers, but they statically linked those into the binary, but then never removed them from the image? confusing to say the least. |
|
so as stated this basically means that the |
|
let me do some analysis whether we don't want to do a role reversal and instead rely on external binaries instead of statically linked code for everything cloud related. As it stands to me I think I would prefer external binaries so I can pick&choose for my environment, not only for size but mainly for security. Don't ship code you don't use. But that concern is less driven by authenticators, which are fairly slim, but the whole s3 context thingy which maybe 1% of users rely on. |
|
hmmm inlining the cred helpers allowed them to drop k8schain dependency. Yep, I would take that deal again |
|
I am not using them at all, I am building a custom container where I remove all of them. The image is a lot slimmer then. The only thing that is "pissing me off", is that there are credential helpers for anything, but not for the simple docker format, I need to add the authentication to the file with That is kind of messed up. |
|
does this help? documentation is still missing so it's a bit hard to find. security wise it actually makes no difference! Consider everything you put into kaniko container as potentially exposed. The security boundary is between the host system and the container, there is no boundary between kaniko and the payload. |
|
I am wondering how many people actually use the whole "build from s3" feature as compared to build in CI/CD with runner cloning the files. Majority of dependencies are for cloud connections and there is not a single integration test for them here (there are some for GCP with a bucket that google owns somewhere privately). I might consider creating an anorexic image (slimmer than slim) that drops the cloud auths and s3 contexts altogether. |
|
Actually things like #236 cost me a lot of time, I was writing the file to the wrong path and authentication still magically worked for the gitlab registry, but not for my other one. Finding that out took me hours. But yes, that normally helps.
I did not write secure, I meant safe. Safe with breaking, escaping chars, the need for
To be honest, I never heard about it before. Most people that use github or so won't use kaniko. Kaniko is more like building in kubernetes or with unprivileged gitlab runners and there you pretty much always get a token that you can use like with plain docker. But maybe I am totally wrong here. |
|
hmmm thats actually a good point. the problem is that login is implicit. that came to bite us here GoogleContainerTools/kaniko/issues/3328 and others already. I remedied a bit by giving the users the option to disable logins they don't expect to happen explicitly, but that is actually only halfway there. I like your idea of an explicit |
Description
The credential helpers were integrated into our keychain already and we don't need to ship them as separate binaries anymore, those binaries are not used.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Reviewer Notes
Release Notes
Describe any changes here so maintainer can include it in the release notes, or delete this block.