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

Allow user to provide registry certificate #1037

Conversation

antechrestos
Copy link
Contributor

@antechrestos antechrestos commented Feb 7, 2020

Description

Fixes #1100

Fixes #1101

Add the multi valued option registry-certificate to allow user the providing of certificate such as --registry-certificate my.registry.url=/path/to/the/certificate.cert. Doing so, user won't have to use the --insecure-registry option for the provided registry.

I found hard to had some tests and was forced to encapsulate te treatments of x509 package.

⚠️ Writting tests, I found out that makeTransport was using a static object http.DefaultTransport without cloning it which should lead to a bug as its configuration is modified. This change also brings a fix for this potential issue.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes [unit tests]
  • Adds integration tests if needed.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

Add the multi valued option 'registry-certificate' to allow user the providing of certificate such as '--registry-certificate my.registry.url=/path/to/the/certificate.cert'

@googlebot googlebot added the cla: yes CLA signed by all commit authors label Feb 7, 2020
@antechrestos antechrestos force-pushed the feature/add_option_to_import_registry_certificate branch from d778e4e to b83e1c2 Compare February 7, 2020 17:41
@tejal29
Copy link
Member

tejal29 commented Feb 13, 2020

@antechrestos Thank you for this PR?
Can you please mentions the manual steps to verify this PR? I can try it out and verify.

Thanks
Tejal

@tejal29 tejal29 self-assigned this Feb 13, 2020
@antechrestos
Copy link
Contributor Author

@tejal29 I will. I tested against a inner company registry. I answer you asap.

@antechrestos
Copy link
Contributor Author

antechrestos commented Feb 13, 2020

@tejal29

  1. Create a loopback
$> sudo echo "127.0.0.1	local.host" >> /etc/host
  1. Generate a couple of self-signed certificates for local.host
$> mkdir /tmp/registry_certificates && cd /tmp/registry_certificates
$> openssl req -x509 -newkey rsa:4096 -sha256 -days 3650 -nodes \                                            
    -keyout local.host.key -out local.host.crt -subj /CN=local.host \
    -addext subjectAltName=DNS:local.host
$> ls -1
local.host.crt
local.host.key
  1. Create a secured local registry with the self signed certificate
$ docker run -d \
  --restart=always \
  --name registry \
  -v /tmp/registry_certificates:/certs \
  -e REGISTRY_HTTP_ADDR=0.0.0.0:443 \
  -e REGISTRY_HTTP_TLS_CERTIFICATE=/certs/local.host.crt \
  -e REGISTRY_HTTP_TLS_KEY=/certs/local.host.key \
  -p 443:443 \
  registry:2
  1. Create a Dockerfile
$> echo "FROM busybox:latest" > Dockerfile
  1. Test with master without any security ignorance
$> KANIKO_PROJECT_DIR=<path to the kaniko project where you generated out/executor>
$> docker run --rm -v $(pwd):/sources -w /sources -v $KANIKO_PROJECT_DIR/out/executor:/kaniko/executor gcr.io/kaniko-project/executor:v0.17.1  --context /sources --dockerfile "/sources/Dockerfile" --destination local.host/test/certificates:master  --verbosity info 

You will gate the following error

error checking push permissions -- make sure you entered the correct tag name, and that you are authenticated correctly, and try again: checking push permission for "local.host/test/certificates:master": creating push check transport for local.host failed: Get https://local.host/v2/: x509: certificate signed by unknown authority

  1. Test it with my branch (get the code and compile it with make)

(note that I mount certificates directory as a volume -v /tmp/registry_certificates:/certs)

$> docker run --rm -v $(pwd):/sources -w /sources -v $KANIKO_PROJECT_DIR/out/executor:/kaniko/executor -v /tmp/registry_certificates:/certs gcr.io/kaniko-project/executor:v0.17.1  --context /sources --dockerfile "/sources/Dockerfile" --destination local.host/test/certificates:add-registry-certificate-option  --verbosity info --registry-certificate local.host=/certs/local.host.crt

You can check it by pulling it docker pull local.host/test/certificates:add-registry-certificate-option

@antechrestos antechrestos force-pushed the feature/add_option_to_import_registry_certificate branch from b83e1c2 to bf1fe4b Compare February 16, 2020 14:19
@antechrestos
Copy link
Contributor Author

@tejal29 did you have time to take a look at this?
Thanks 😊

@antechrestos antechrestos force-pushed the feature/add_option_to_import_registry_certificate branch from bf1fe4b to 8637dc2 Compare February 20, 2020 18:23
@antechrestos
Copy link
Contributor Author

antechrestos commented Feb 20, 2020

Rebase done

@tejal29
Copy link
Member

tejal29 commented Feb 21, 2020

@antechrestos Thanks. we are busy with v0.17.0 bug fixes and internal perf process. I will take a look next week. Thank you.

@antechrestos antechrestos force-pushed the feature/add_option_to_import_registry_certificate branch from 8637dc2 to 19c353a Compare February 28, 2020 10:48
@antechrestos
Copy link
Contributor Author

Any update needed.?

@cvgw
Copy link
Contributor

cvgw commented Mar 3, 2020

@antechrestos is there an GH issue related to this?

Also; did you investigate updating the certificate chain in the docker image as an alternative to handling this via golang?

@antechrestos
Copy link
Contributor Author

antechrestos commented Mar 3, 2020

@cvgw I did not open an issue should I?
In fact, I wanted to follow the job that was done in an other pr that was done few time ago: see here.

Importing the certificate in the docker image will do the job. However I wanted to be consistent with other options (skip tls verification for a given registry and the other one disabling all tls)

@antechrestos antechrestos force-pushed the feature/add_option_to_import_registry_certificate branch from 19c353a to 66e1f66 Compare March 3, 2020 20:22
@antechrestos
Copy link
Contributor Author

@cvgw I've just opened two issues that describes the feature and the fix brought by this PR

@tejal29
Copy link
Member

tejal29 commented Mar 4, 2020

@antechrestos Thank you so much for opening the issues.

I was testing your PR and i see the following error

/kaniko/executor -f dockerfiles/Dockerfile1 --context=dir://workspace --destination local.host/test/certificates:master --registry-certificate local.host=/certs/local.host.crt
error checking push permissions -- make sure you entered the correct tag name, and that you are authenticated correctly, and try again: checking push permission for "local.host/test/certificates:master": creating push check transport for local.host failed: Get "https://local.host/v2/": dial tcp: lookup local.host on 8.8.8.8:53: no such host

I added a host entry in /etc/hosts

tejaldesai@@registry_certificates$ cat /etc/hosts
127.0.0.1   local.host
127.0.0.1	localhost

Is there something missing?

@tejal29
Copy link
Member

tejal29 commented Mar 4, 2020

@medyagh helped me debug this. I had to get the ip address of the registry and append that to /etc/hosts instead of 127.0.0.1 within the docker container.

@antechrestos
Copy link
Contributor Author

@tejal29 sorry I was in a car trip. I had the same in /etc/hosts (I am on ubuntu 18.04) but I have local.host in the list of the environment variable no_proxy. Maybe linked to this.
but putting the registry address als does the trick. I will keep it in mind next time 😏
Thank you for this feedback.

@antechrestos
Copy link
Contributor Author

@tejal29 I've just learned --link in docker and it could have been easier without any tricks with /etc/hosts

Just launch registry without port mapping

$ docker run -d \
  --restart=always \
  --name registry \
  -v /tmp/registry_certificates:/certs \
  -e REGISTRY_HTTP_ADDR=0.0.0.0:443 \
  -e REGISTRY_HTTP_TLS_CERTIFICATE=/certs/local.host.crt \
  -e REGISTRY_HTTP_TLS_KEY=/certs/local.host.key \
  registry:2

And every other kaniko run with --link registry:local.host would have done it 😄

pkg/config/args.go Outdated Show resolved Hide resolved
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot removed the cla: yes CLA signed by all commit authors label Mar 7, 2020
@googlebot googlebot added the cla: no CLA not signed by all commit authors label Mar 7, 2020
@antechrestos antechrestos force-pushed the feature/add_option_to_import_registry_certificate branch from 28b6205 to abdbaa8 Compare March 7, 2020 17:58
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes CLA signed by all commit authors and removed cla: no CLA not signed by all commit authors labels Mar 7, 2020
@antechrestos antechrestos force-pushed the feature/add_option_to_import_registry_certificate branch 3 times, most recently from 5421a5b to cda06f8 Compare March 8, 2020 12:51
@antechrestos antechrestos force-pushed the feature/add_option_to_import_registry_certificate branch from cda06f8 to b73c2c1 Compare March 8, 2020 17:18
@tejal29 tejal29 merged commit 18de5d6 into GoogleContainerTools:master Mar 12, 2020
@antechrestos antechrestos deleted the feature/add_option_to_import_registry_certificate branch March 12, 2020 22:23
@antechrestos
Copy link
Contributor Author

@tejal29 thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed by all commit authors
Projects
None yet
5 participants