image: fix signature import from secure registries

[mfojtik]

@miminar || @dmage || @legionus do you guys have secure registry running? I'm trying to verify if this fixes this problem: https://gist.github.com/zhouying7780/bfc1d61e68cdaf821b9538a9e6cca501

Note that we don't have to be super-secure here because the signature importer does not care if the registry is secure or not, just need for the ping to pass...

[mfojtik]

@miminar @mtrmac @bparees i see billion of errors in online where we trying to contact the internal registry (?) in NewImageSource and failing with error (which causing retries). I think we can safely ignore that error for now because we only care about RH signatures at this point. Also I don't understand why we need to do ping() for the registry when we want only get the signatures (which are downloaded from other source anyway).

example of error:

Oct 19 00:38:48 XXXX.ca-central-1.compute.internal atomic-openshift-master-controllers[4314]: E1019 00:38:48.538149    4314 signature_import_controller.go:109] error syncing image sha256:XXXXX, it will be retried: unauthorized: authentication required

@mtrmac can we make some option in system context that will skip all remote communication to registry?

[mfojtik]

@smarterclayton found why signature import was pretty high in heap :)

[mtrmac]

@mtrmac can we make some option in system context that will skip all remote communication to registry?

Not really; the client needs to connect to /v2 to see whether the server supports X-Registry-Supports-Signaturs, and based on that decide which signature access client to use.

In #15494 (comment) I was explicit that this is relying on an implementation detail, and that the controller should instead supply the appropriate credentials. That’s eventually going to have to happen anyway, AFAICT, so I’d really hate to extend this hack any further just to postpone doing the right thing.

[mfojtik]

(we need this for 3.7)

[miminar]

@mfojtik contrary to my initial approval, I'd say that it doesn't make sense to import signatures for something coming from insecure registry or coming over insecure protocol. I'd tell the reporter to secure the registry before playing with image signing.

[bparees]

, I'd say that it doesn't make sense to import signatures for something coming from insecure registry or coming over insecure protocol.

why not? the signature can't be faked, even if it can be intercepted/mutated.

[mfojtik]

we only care about RH signatures today, we can make this better in next release maybe where we can deal with auth against the registry...

the use-case there will be you are importing signatures for the image from third-party-but-openshift registry that has signature endpoint and that registry is secure and require out to get the signatures. That is broken today as we don't provide the auth the container/image needs. It is irrelevant now if the registry is secure or not (for RH registry at least).

[mfojtik]

/retest

[mtrmac]

Not thrilled about this, but it’s acceptable; OpenShift does not currently have a strong ambition to notice removed signatures, and this does not make it much worse.

[mtrmac]

GetSignatures when using the lookaside uses a client with default settings, which ignores this boolean. It could be argued that that is a bug.

Do note that using the X-Registry-Supports-Signatures is affected by this boolean; as soon as this code starts passing real credentials, it will have to also set DockerInsecureSkipTLSVerify appropriately instead of hard-conding to true.

[mtrmac]

, I'd say that it doesn't make sense to import signatures for something coming from insecure registry or coming over insecure protocol.

why not? the signature can't be faked, even if it can be intercepted/mutated.

We rely (only) on TLS to prevent an attacker from presenting an “old” signature. Consider the conventional :latest tag; a signature for the :latest tag should be served only for the truly latest image in the repo, and not for the older images. Once a signature for the :latest tag is created for a particular image, a MITM could continue to serve it indefinitely; TLS is needed for the client to be confident that the :latest signature is still marking the current state.

(Alternatively, the signatures could be created with an expiration date; but constantly re-signing and re-publishing images is much more complex than setting up an ordinary TLS and just removing a file.)

[bparees]

We rely (only) on TLS to prevent an attacker from presenting an “old” signature. Consider the conventional :latest tag; a signature for the :latest tag should be served only for the truly latest image in the repo, and not for the older images. Once a signature for the :latest tag is created for a particular image, a MITM could continue to serve it indefinitely; TLS is needed for the client to be confident that the :latest signature is still marking the current state.

the old image signature would not be valid for the "new" "latest" image, though, right? So this MITM attack would only work if the MITM served both the old signature and the old image. At which point yeah, you're broken, but in the same way you were already broken when you decided to pull images from an insecure registry.

In other words, you either trust the registry to give you the right image or you don't. Where/how you get the signature shouldn't matter.

I could maybe see the exploit of a MITM providing a since-revoked signature for an image which has a CVE (whereas the proper signature server would not offer a signature for that image), though for that i would say signatures probably do need to expire because MITM or not, old signatures will have a way of hanging around in various places.

[mtrmac]

I could maybe see the exploit of a MITM providing a since-revoked signature for an image which has a CVE (whereas the proper signature server would not offer a signature for that image),

Yes, that’s why freshness is needed.

So this MITM attack would only work if the MITM served both the old signature and the old image. At which point yeah, you're broken, but in the same way you were already broken when you decided to pull images from an insecure registry.

Yes, mostly. The one exception is an insecure registry + a TLS-protected signature lookaside (or, equivalently and perhaps more commonly, a registry serving data from a third-party semi-trusted CDN, and an internally hosted TLS-protected signature lookaside which is less expected to be compromised); in that case the signature guarantee authenticity against attacks by the registry, and TLS on the signature guarantees freshness.

(This does work right now, because the DockerInsecureSkipTLSVerify applies to the registry communication, but not to accessing a https:// signature lookaside. As I said elsewhere here, it could maybe be argued that this is a bug.)

OTOH, if the registry is using any authentication at all, the connections should have TLS enforcing either way. So, in most common deployments, these distinctions are not terribly important. I just want to make sure the value of using TLS for fetching signatures is generally understood.

[mfojtik]

So to get this back to the original issue:

1. We need this in order to avoid automatic image signature importer to attempt import signatures for local image (from internal docker registry) because we would need credentials to do so and we will need cert to do proper TLS communication. Also I think automatically importing signatures for images that we pushed manually (with signature) is stupid, because we already have that signatures so it will be a no-op for signature importer.
2. For now, the only signature store I know about is the RH signature store. Users can can configure their own private signature stores for automatic signature importing via the registry.yaml files, which in that case the registries they are importing from has to be public/http. With this they can be https but we are not going to verify the TLS/etc. Which makes them more insecure but on the other hand, a "root" has to place the YAML file on each master. The MITM attack is possible with faking DNS, but I believe there are more places in Origin code where this is possible, so we can defer this for now.

So I think this is safe to merge now with a Trello card that will make this process more secure (with TLS, with passing credentials, etc.). But for 3.7, I really want to avoid dealing with certs and secrets. @bparees agree?

[bparees]

@mfojtik is this going to allow the test mentioned in #16344 to pass? if so, can you re-enable it as part of this PR?

[mfojtik]

@bparees no, this is not fixing that test..

[bparees]

/lgtm

[openshift-merge-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, mfojtik

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• pkg/image/OWNERS [bparees,mfojtik]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[openshift-merge-robot]

Automatic merge from submit-queue (batch tested with PRs 16979, 16947).