Support insecure registries

[dlion]

This PR Implements the support for insecure registries.

Sample command

creator --run-image=develoment-registry.com/run-java:v16 --insecure-registry=develoment-registry.com --insecure-registry=testing-registry.com testing-registry.com/java-sample:latest

Others

Tracking issue: buildpacks/rfcs#246

[natalieparellano]

Apologies for being slow to review @dlion. I left a few comments... let's discuss how we might be able to test this out at the acceptance level.

[dlion]

Uhm, locally the TestCreator is passing, same for the amd64 on the CI so I guess it's flaky.
@natalieparellano Can you have a look on my latest changes to see if the code makes sense and I haven't forgot anything?
After your approval I will go starting investigating on the default values for the WithRegistrySettings function.

UPDATE:
Nevermind I just saw that I still need to complete the rebaser and the restorer

[natalieparellano]

Looking good @dlion - and you added an acceptance test for every phase 🥇

Thanks for being so thorough and for making the code incrementally better :)

[dlion]

This check makes difficult to test the insecure-registry feature properly so for now I'm skipping it.
We are doing this check in the analyzer only (and used also in the creator of course making the acceptance test difficult to create)

[dlion]

Some update on the refactoring

I wanted to use the RegistryHandler in the image.NewHandler using the RegistryHandler.GetInsecureRegistryOptions to get the RegistrySetting option that we can use in the remote.NewImage call.

I tried to start creating a test for it but then I realised that the WithRegistrySetting function implementation returns an anonymous function that receive a parameter of type options which is a private struct making it impossible to use/mock it from lifecycle.

Refactoring this part would require lots of time and effort, so I decided to leave it there, trying to minimize the duplication as much as possible and then open a new issue to address this problem as a future improvement.

[natalieparellano]

Amazing work @dlion! I left a few comments. Overall this is looking really great.

[jabrown85]

I am a bit confused. There are 2 distinct insecure paths that seemingly are tied to a single setting.

The first path is accessing http only registries, which seems to work in imgutil with

        destinationImageName := "myregistry.domain.com:6443/test/test:latest"
	remoteImage, err := remote.NewImage(destinationImageName,
		authn.DefaultKeychain,
		remote.FromBaseImage(baseImageName),		
		remote.WithRegistrySetting("myregistry.domain.com:6443", true),
	)

However, if myregistry.domain.com:6443 only accepts HTTPS requests - this does not work. Am I missing a way to only set TLSSkipVerify? I still need to speak over HTTPS, but am OK skipping the TLS verification step for a self-signed situation.

[dlion]

I still need to speak over HTTPS, but am OK skipping the TLS verification step for a self-signed situation.
Uhm, we haven't considered that scenario, we thought that if you are using insecure-registry it means you are doing that for testing purposes or an internal network where having a self-sign certificated doesn't really matter.

Here the PR I opened to simplify those parameters into a single one trying to emulate what Google did: buildpacks/imgutil#218

Do you think we should change this implementation giving that scenario?

I also /cc @natalieparellano since we had this conversation a few weeks ago (and from that the PR)

[jabrown85]

Hmm, maybe we need to use the remote.DefaultTransport instead of http.Transport in imgutil. The transport looks to have some magic in it that may help with the http/https negotiation.

[jabrown85]

Ok - I think I found part of the problem. The doSave here needs the same if insecure { InsecureSkipVerify stuff } logic. When you create a remote image with a base image, it hits the InsecureSkipVerify path on reading from that registry, but doesn't do anything with insecure during the save process.

I do wonder if there are other operations that need the InsecureSkipVerify that we don't have yet in imgutil remote. Found/Delete/etc.

I do believe we still need to be able to have multiple remote.WithRegistrySetting to in remote.NewImage. The current pattern only lets a single registry be marked as insecure/tlsnoverify AFAICT.

[jabrown85]

This looks pretty good to me! I'll let @natalieparellano take a peek as well.

[jjbustamante]

@natalieparellano is this PR fixing #524 ?

[natalieparellano]

@jjbustamante I believe so... it's worth noting that while this has been merged, it only applies for Platform APIs later than 0.12, for which we don't have anything released yet in the spec.