possible support for golang tls.Config's VerifyConnection

[gabemontero]

/kind question

So in OCP we have a customer case around openshift builds captured by https://bugzilla.redhat.com/show_bug.cgi?id=1916531 where a customer was taking advantage of the old x509: certificate relies on legacy Common Name field support for verification of the cert

evidently that support has been removed from the major browsers and is on its way out with golang

golang/go#39568 (comment) has a good encapsulation of the issue, including a short term workaround that is still suppose to work with golang 1.15, but is going away with 1.16

In its stead, golan is proposing adding verification functions via the tls.Config ... https://tip.golang.org/pkg/crypto/tls/#example_Config_verifyConnection is the reference supplied in that golang issue comment above.

My question is two fold:
a) do we even care about supporting injecting such verification functions into the tls.Config leveraged by repo
b) is my guess that SystemContext would have to be updated to specify such verify connection functions, and then openshift-> buildah would then have to propagate such defs down to containers/image

thanks

@nalind @adambkaplan @bparees FYI

[mtrmac]

In short:

• Personally I’m not at all a fan of making legacy CN use easier; just fix the certificates.
• OTOH there is some prior art; propagating that setting would be cleanest (but limits us to Go 1.15).
• If we do that in Go, it should just be a boolean (referring to some shared implementation) and not a callback.

(I don’t know what is a good solution; every option I can think of seems somewhat terrible. So, read the following as somewhat tentative.)

---

The browser ecosystem has been consistently saying that all certificate owners must be ready to replace their certificates any time, at short notice, because a key, or a CA, can be compromised any time, and to protect users the certificate owners must be able to react (in particular, it is not acceptable to have any month-long approval processes or change freezes). AFAICS that’s unambiguously true that certificate owners should set up their processes like that.

So, it should be possible for certificate owners to just re-generate certificates without too much hassle. And they will have to do that anyway as soon as Go 1.16 starts being used by any client; might just as well do that now.

---

OTOH there is openshift/machine-config-operator#2141 , which I guess allows CN use for CRI-O already. The related discussions originally were explicit that this is for 4.6 only, and would be reverted for 4.7 again, which, it seems, hasn’t happened. (And anyway if Go really removes the GODEBUG flag in 1.16, this will no stop working by then.)

Ideally, once this MCO value gets reverted, it should automatically apply to everything; if c/image adds its own Go-level override, reverting the MCO configuration above will not apply to that Go-level override.

So, I think it would be most elegant (OTOH, maybe impossible given current Pod objects, and contrary to OpenShift design principles?) for builds to automatically inherit the GODEBUG value from the host environment.

OTOH MCO is already not the only place (the MCO issue already links to openshift/os ), sure.

---

Using InsecureSkipVerify+VerifyConnection is, IMHO, absolutely unsustainable. Already compare the https://tip.golang.org/pkg/crypto/tls/#example_Config_verifyConnection example with the current https://github.com/golang/go/blob/dbab07983596c705d2ef12806e0f9d630063e571/src/crypto/tls/handshake_client.go#L837-L851 . Especially note the update of verifiedChains, which apparently affects other code paths… somehow. Keeping this in sync forever by every single caller of c/image, where there is no automated way to notice that something should be updated, just isn’t reasonable.

This is security-critical code and both the TLS protocol and known attacks are evolving. This is not something that can be done once and forgotten about, nor something that can be validated with a test suite (because a test suite will not find currently unknown vulnerabilities).

(In addition to implementation bugs and difficulty keeping up, there’s also a very long history of APIs providing a “replace certificate verification” hook being misused with a “temporary implementation just to be able to continue implementing the rest of the codebase” that just returns “everything is secure, go on”. So I’m rather tempted to say that such options should not be provided at all; the InsecureSkipTLSVerify is already enough rope.)

So, if we had to do this in Go, I think the only secure way to do that would be:

• to have a single OpenShift-level (or even more general) implementation as a shared Go module, with a dedicated process/people tasked to keep it up to date
• For c/image to consume such a module, and to just expose an “accept CommonName for DNS names” boolean.

That well-targeted GODEBUG which changes exactly that one single thing with hopefully no side-effects seems, WRT maintainability/clarity, much preferable to InsecureSkipVerify+VerifyConnection. OTOH it’s not Go-level, and it’s probably going to go away…

[gabemontero]

Thanks for the quick response @mtrmac

I did not say it initially, but my opening up the discussion at all was in part due diligence, and getting feedback beyond my specific team.

Lot's of good feedback as well. And I think I'm in line with your takes as well.

I'm going to gather a couple of more opinions from other groups, but I quite possibly will either be quoting you, referencing this discussion, or copying/pasting your points here where safe/reasonable when I get back to the customer with an official take.

Thanks again.

Closing out - my "question" has been answered.