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

fix: use pre-downloaded binaries for cfssl #568

Merged
merged 5 commits into from Jun 1, 2023
Merged

Conversation

virgilp
Copy link
Contributor

@virgilp virgilp commented May 25, 2023

This PR makes sure that the init pod will use pre-downloaded binaries, so that the operator can be deployed in offline scenarios. Also it no longer makes the assumption that binaries like sh and chmod exist in the docker image (a lot of hardened/ minimal images don't have shell on them).

Other changes:

  • modified server.pem and server-key.pem to ca.pem and ca-key.pem - AFAICT, these are the file names generated by cfssljson ; yes I know it's confusing but these are different than the input ones.
  • passed additional argument "-r" to the init pod - we don't want it failing on subsequent deployments/ if a webhook already exists.

Fixes #567.

@buehler
Copy link
Owner

buehler commented Jun 1, 2023

Thank you!

Do you have any other suggestions on how to harden the Dockerfile?

@buehler buehler merged commit f2478de into buehler:master Jun 1, 2023
7 checks passed
@ewassef
Copy link
Contributor

ewassef commented Jun 14, 2023

This seems to have introduced a bug:

webhook-installer Generating server certificate for "cloud-operator" in namespace "vcp".
webhook-installer Unhandled exception. System.ComponentModel.Win32Exception (2): An error occurred trying to start process 'cfssl' with working directory '/certs'. No such file or directory

@virgilp
Copy link
Contributor Author

virgilp commented Jun 22, 2023

This seems to have introduced a bug:

webhook-installer Generating server certificate for "cloud-operator" in namespace "vcp".
webhook-installer Unhandled exception. System.ComponentModel.Win32Exception (2): An error occurred trying to start process 'cfssl' with working directory '/certs'. No such file or directory

It's not a bug, you need to pre-download the binary on your docker image (instead of expecting the image itself to download it).

@virgilp
Copy link
Contributor Author

virgilp commented Jun 22, 2023

Thank you!

Do you have any other suggestions on how to harden the Dockerfile?

Sorry @buehler I just now saw this.

I use an internal company image for runtime, but I would say, use a distroless image. E.g. instead of

FROM mcr.microsoft.com/dotnet/aspnet:{DotnetImageTag} as final

the operator could propose this for the Dockerfile:

FROM mcr.microsoft.com/dotnet/runtime-deps:7.0-cbl-mariner2.0-distroless AS final

Warning: I haven't tested this. it may be that "addgroup" does not actually exist in the cbl-mariner image, and you have to rely on pre-existing user/group (it should be running as non-root already by default).

@ewassef
Copy link
Contributor

ewassef commented Jul 16, 2023

It's not a bug, you need to pre-download the binary on your docker image (instead of expecting the image itself to download it).

It is very much a bug, as its a breaking change. It should be v8.x... also, all users who arent using this docker file (such as ourselves using Cloud Native Buildpacks) or others that are combining this with other code and utilizing their own dockerfile will not be regenerating a Dockerfile which every build. (That's not standard practice at all). Also, a better error message when the tool is not found would be helpful since most users are likely not using it in the same way you are at your company.

I think a hook that can be extended with code ( to add the old functionality back for those of us who would need it ) would be a good compromise. I will submit a fix for this if that's ok, @buehler

#578 for your consideration

@virgilp
Copy link
Contributor Author

virgilp commented Jul 27, 2023

Agree it was a breaking change, sorry for that/ that I hadn't considered existing users. Thanks for the PR!

ahmelsayed added a commit to ahmelsayed/dotnet-operator-sdk that referenced this pull request Aug 22, 2023
With buehler#568 these executables are no longer downloaded on demand, but rather predownloaded to the system. 

When running `dotnet build` locally, it attempts to run `./cfssl` and `./cfssljson` from `/operator` or `$CFSSL_EXECUTABLES_PATH`. However, it deletes them after it's done. 

Next time you try build again after cleaning the local dir it'll fail.
buehler added a commit that referenced this pull request Sep 13, 2023
With #568 these executables are no longer downloaded on demand, but
rather predownloaded to the system.

When running dotnet build locally, it attempts to run `./cfssl` and
`./cfssljson` from `/operator` or `$CFSSL_EXECUTABLES_PATH`. However, it
deletes them after it's done.

Next time you try build again after cleaning the local dir it'll fail.

Co-authored-by: Christoph Bühler <buehler@users.noreply.github.com>
@ewassef
Copy link
Contributor

ewassef commented Oct 27, 2023

I am still having a problem with this and we are stuck at an older version. Does the generator still create the certs and docker file? Its expecting these executables in the /operator folder but nothing puts them there https://github.com/buehler/dotnet-operator-sdk/blob/v7.6.1/src/KubeOps/Operator/Commands/CommandHelpers/CertificateGenerator.cs#L59

v7.6.1 is broken? @buehler @virgilp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: The operator sdk should work with secure images & offline Kubernetes clusters
3 participants