Pass all configured CA certificates to builds

Previously, the runner attempted to infer a valid chain from its network operations with the configured CA certs, and passed a subset of the configured certificates to builds, shuffling the order as it went. This seems unnecessary, so remove it.

Closes #1283 (closed)

/cc @tmaczukin @ayufan @maxraab

mentioned in issue #1283 (closed)

None of this is particularly safe for concurrent use - either before or after the patch. Does this matter?

Woo, artifacts :D https://gitlab.com/nick.thomas/gitlab-ci-multi-runner/builds/3823034/artifacts/browse/out/binaries/

@nick.thomas ~20 builds without any error. I propose to upgrade one of our production runners and we test it a couple of days?

@maxraab thanks for testing! What version are your production runners at the moment? This change is very small and well-specified, but it's based on top of master at the moment, so pulls in a whole bunch of other stuff with it...

If this worries you, it shouldn't be too hard to prepare a branch against whatever you're running in production + this patch for you to test in production.

@nick.thomas Currently we are using the current stable 1.5.2. I think there is no need for a patchversion based on the latest stable :-)

I love it when things are up to date :). OK, further testing greatly appreciated, but at your own risk, etc, of course ;)

mentioned in issue #1103 (closed)

mentioned in issue #1300 (closed)

mentioned in issue #1297 (closed)

mentioned in issue #1396 (closed)

@nick.thomas

This brakes this case: Trusted certificate is part of the system CA bundle, then the certificate is not passed to build container and git clone fails.

@ayufan OK, so we're trying to work around user misconfigurations here (ie., incomplete bundle supplied)? Is there a compelling reason to do so? I'd value regularity over user-friendliness here, I think.

If a user supplies an incomplete bundle to the runner, the solution is easy - supply the correct bundle instead. It's their problem and the solution is entirely in their hands. It's easy to debug and easy to explain.

If a user supplies a valid bundle, or sets up their runner or build context correctly, and the below gymnastics break that setup, they have no recourse.

From my point of view (and this MR doesn't implement all this), the behaviour should be:

• If a bundle is provided, it (and only it) is used both by the runner and the build, exactly as provided
• If a bundle is not provided, the runner and build context use their respective system contexts

Instead, this is the current state:

• If a bundle is provided, it is used by the runner. Otherwise, it uses the system default.
• The runner's context is used to get a build
• Only those SSL certificates that were verified in that one TLS connection, are not self-signed, and are not presented by the server, are passed to the build, in a shuffled order
• Regardless of whether a bundle was specified or not, the build's context is overridden

This causes two problems, examples of which appear in the duplicate issues:

• Certificates are removed from the build context (both system and in the bundle), consistently breaking SSL for some setups
• Certificates are shuffled randomly in the build context, intermittently breaking SSL in some setups

@nick.thomas: intermediate result: 200 builds without failure

Thanks @maxraab !

Would you be willing to try a second build with just the random shuffling and certificate removal fixed, at some point? I"ve spoken a little about what I think is the "right" thing to do above, but I'm aware we have lots of existing customers, some of which will be broken if we do the right thing in 1.6, so I'm exploring intermediate solutions with a higher chance of being merged.

Yep, I can test another version :)

Great, thanks. I'm unlikely to get back onto it this week, but I'll ping you once I've got something.

I've dug a bit more into network/client.go; as createTransport() uses x509.NewCertPool(), it replaces the system CA configuration (rather than appending to it). Precis above amended to reflect that. go1.7 would permit appending, rather than replacing, at least on not-windows.

Of interest: https://github.com/golang/go/issues/16800

So the only circumstance in which this results in a user-visible regression is where a bundle isn't supplied, where this MR falls back to the build context's native SSL config rather than building one itself.

@ayufan if you don't like relying on the build context in this scenario (I think it's fine, personally), what do you think of, in the no-bundle-supplied scenario only, taking the VerifiedCertificates out of the tls.ConnectionState and writing them all?