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

docker_api https fix for secured docker #673

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

alin-o
Copy link

@alin-o alin-o commented Jun 30, 2020

The certificate path is also needed when docker daemon mode is tlsverify

see https://docs.docker.com/engine/security/https/#connecting-to-the-secure-docker-port-using-curl

@buchdag
Copy link
Member

buchdag commented Jul 4, 2020

Hi.

Thanks for the contribution. Have you tested it ?

Do you think you could add documentation for this feature to your PR ?

@alin-o
Copy link
Author

alin-o commented Jul 4, 2020

Hi,

I tested this on my own servers, which are configured with daemon mode tlsverify, and it works. Before this change, no certificates were ever generated because the curl command returned just an error message,

I added a short documentation under docs/Container-configuration.md. I hope it is the right place.

@buchdag buchdag added the kind/feature-request Issue requesting a new feature label Jul 4, 2020
@buchdag
Copy link
Member

buchdag commented Jul 4, 2020

I added a short documentation under docs/Container-configuration.md. I hope it is the right place.

Yep, that's perfect, thanks.

I tested this on my own servers, which are configured with daemon mode tlsverify, and it works.

Okay this is going to be pretty hard to test on Travis.

@buchdag buchdag added type/feat PR for a new feature and removed kind/feature-request Issue requesting a new feature labels Jun 15, 2021
@buchdag buchdag added the status/pr-needs-tests This PR needs new or additional test(s) label Mar 2, 2022
Copy link
Member

@buchdag buchdag left a comment

Choose a reason for hiding this comment

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

Somehow I wrote change requests for this PR almost 4 years ago and never pressed the Submit review button. 🤦

@@ -201,6 +201,14 @@ function docker_api {
else
scheme="http://${DOCKER_HOST#*://}"
fi

if [[ -v DOCKER_TLS_VERIFY && -v DOCKER_CERT_PATH && ! -z "$DOCKER_TLS_VERIFY" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

For the sake of consistency with the other features, could we check if DOCKER_TLS_VERIFY is set to true/True/TRUE (using the lc() function) instead of just setting the variable to any value ?

If the docker host daemon socket is [protected](https://docs.docker.com/engine/security/https/):

* `DOCKER_TLS_VERIFY` - set it to value `1` if the docker host requires client TLS authentication
* `DOCKER_CERT_PATH` - path to TLS client certificates for the docker host. This folder should contain `cert.pem`, `key.pem` and `ca.pem` files. See [Create a CA, server and client keys with OpenSSL](https://docs.docker.com/engine/security/https/#create-a-ca-server-and-client-keys-with-openssl)
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 we should be a bit more clearer about the fact that this environment variable set a path that will be looked upon inside the container and that the expected file will have to be mounted to this path somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/pr-needs-tests This PR needs new or additional test(s) type/feat PR for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants