Use certutil to create certificate chain for Git

What does this MR do?

Uses certUtil library to pull the complete certificate chain - which doesn't happen by default in newer Go versions.

Why was this MR needed?

Because without all the certs we can't upgrade from Go 1.8.7

Are there points in the code the reviewer needs to double check?

Does this MR meet the acceptance criteria?

• Documentation created/updated
• Added tests for this feature/bug
• In case of conflicts with master - branch was rebased

What are the relevant issue numbers?

Closes #4019 (closed)

added [Deprecated] Category:Runner backstage [DEPRECATED] devopsverify grouprunner upgrade GO from 1.8 labels

added 1 commit

• e0e510f0 - Get cert chain with certutil

Compare with previous version

added 1 commit

• dd1dbfe1 - Move vendor file around

Compare with previous version

added 1 commit

• ef485f81 - Properly add deps

Compare with previous version

FYI @tmaczukin @ayufan

I started playing around with this on Friday after talking to @steveazz. This uses a library from github.com/zakjan/cert-chain-resolver that provides an implementation of unwinding the certificate chain back to the root certificate - much like happens by default in Go 1.8.7.

There's a test project here https://gitlab.com/erushton/go-lang-tls-connection-state/pipelines/82147216 which shows that the verified chain matches with this implementation on Go 1.9 as it did previously with Go 1.8.7 (You can see the output of the jobs BEFORE using this library on this pipeline https://gitlab.com/erushton/go-lang-tls-connection-state/pipelines/82094235)

Looks promising :)

This dependency should be added to Gopkg.toml :)

Yeah I don't know how I missed that part.

Is that the only feedback?

Yes, for now :). I think we should extend the test project to test also Go 1.13 (recently released, so current 'stable'). And the comparisons should also show comparison for all tested versions vs. 1.8.

But the change in Runner's code is pretty straightforward - there is not much to review here :).

Let's just test this good. For example we could install a Runner built with this change and on Go 1.9 on one (or maybe even both) of our private Runners and leave it for a while to see if it works properly. If yes, then I think we can proceed, and merge this into master. The chains created by Go 1.9+ with this update seem to be identical to the one from Go 1.8. So I expect we may get more problems from upgrading Go to 1.10+ than from this MR

Pipeline updated with Go 1.13 (and 1.11 for completion) https://gitlab.com/erushton/go-lang-tls-connection-state/pipelines/82769582

*Edited - I previously posted the wrong link

added 1 commit

• a10e6ca0 - Add certutil to gopkg.toml

Compare with previous version

@erushton I've double tested it, using the updates to the test project that I've prepared few months ago (adding them on top of your recent changes) - https://gitlab.com/tmaczukin-test-projects/go-lang-tls-connection-state/pipelines/82798163. github.com/zakjan/cert-chain-resolver/certUtil seems to do the trick: while with Go 1.9+ we receive a shorter certificates chain with the HTTP response struct, the final certificates chain, created by certUtil is identical to what we have currently with Go 1.8 by default.

I'd say that this resolves our problem :).

Let's prepare a binaries built with Go 1.9.x. Then we can ask the reporters from #3183 (closed), who discovered the problem with Runner on Go 1.9, to check if new binaries are working properly :)

For that I've created a temporary branch that will create a test binary basing on this MR and Go 1.9.7. The binaries will be available at: https://gitlab-runner-downloads.s3.amazonaws.com/test-certutil-with-go-1-9/index.html after the https://gitlab.com/gitlab-org/gitlab-runner/pipelines/82802362 Pipeline will succeed.

As for this MR we should do one thing: cleanup the commits history. @erushton Please rebase the commits to have two of them:

1. First one, that adds the dependency (including the Gopkg.toml, Gopkg.lock and vendor/ changes).
2. Second one, that adds the change in the Runner's code at network/client.go.

I'd also think about adding here a test based on the idea of our go-lang-tls-connection-state project, that would detect it the certificates chain would be changed from what is expected.

mentioned in issue #3183 (closed)

I've asked the users who reported the problem with Go 1.9 in #3183 (closed) to test the new binaries. Let's wait and see if they are able to help us validate the fixed version compiled with Go 1.9.7 :)

added 2 commits

• d597c276 - Add certUtil dependency
• 8ca39fd8 - Use certUtil to get certificate chain

Compare with previous version

unmarked as a Work In Progress

changed the description

mentioned in issue #4722

@tmaczukin I've rebased and put them into 2 commits as you suggested and opened #4722 to track adding a consistent test.

Assigning to you for review/merge

assigned to @tmaczukin

@erushton As the failing job tells use, you need to Go fmt the code:

go fmt network/client.go

And then amend the last commit and force-push the branch

assigned to @erushton and unassigned @tmaczukin

mentioned in issue #4714 (closed)

mentioned in issue gl-retrospectives/runner#1 (closed)

sigh

added 1 commit

• b7827702 - Use certUtil to get certificate chain

Compare with previous version

That's what I get for trying to do things the quick and dirty way

Back to you @tmaczukin

Use an IDE that supports Go, and you will have this covered mostly automatically (even if Vim is yours IDE)

assigned to @tmaczukin and unassigned @erushton

Let's wait with merging this to Monday. I'll then find the old MR that was upgrading the Go version to 1.9 (and that we've reverted), I'll update it and then we can merge both :)

Sounds good to me

changed milestone to %12.4

mentioned in issue #3202 (closed)

mentioned in merge request !1337 (closed)

Awesome! Nice work @erushton. :)

++

Woohoo, this is awesome!

mentioned in merge request !1594 (merged)

mentioned in merge request !1601 (merged)

changed the description

approved this merge request

OK, time to merge this. Thank you @erushton for the solution! :)

merged

mentioned in commit 2f2bd1d5

mentioned in issue #2947 (closed)

Waahoooo!! what a way to start a week!

after this MR , what does the roadmaps says for the next GO version? can we try 1.13?

@arangogutierrez we are going with 1.9 for %12.4 and then upgrading to 1.13 in a later release when we know no more regressions are found. The reason we are going with 1.9 is because we did run it in production before and we know it works (apart from this bug). The actual upgrading is happening in !1606 (merged)

changed title from Use certutil to Use certutil to create certificate chain for Git

mentioned in issue #4019 (closed)

changed the description

mentioned in merge request !1639 (merged)

mentioned in issue gl-retrospectives/runner#2 (closed)

mentioned in issue #4805 (closed)

mentioned in issue #4868 (closed)

mentioned in issue #4890

mentioned in commit c613d048

mentioned in merge request !3699 (merged)

mentioned in commit ed980ae9

mentioned in commit 5bb4e54a

mentioned in commit fc64dad2

mentioned in commit 7bb47a26

mentioned in commit f4862401

mentioned in commit fa2d557d

mentioned in commit b9e7341a

mentioned in commit 55703cc2

mentioned in commit shubhindia/gitlab-runner@281551ac