Fix TLS chain building

What does this MR do?

Fixes the problem with TLS certificates chain generation for Git, that was introduced with our changes in 12.4.0.

Why was this MR needed?

Please check #4805 (closed) for context.

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 #4805 (closed)

changed milestone to %12.5

added [Deprecated] Category:Runner devopsverify grouprunner typebug + 1 deleted label

This implements the fix tested at #4805 (comment 234068922).

@steveazz Could you review it. Or maybe we should wait for your investigation effects for #4805 (comment 234673898) and fix it also with this MR? WDYT?

assigned to @steveazz

mentioned in issue #4805 (closed)

added 1 commit

• b2da7c33 - Do not remove received chain

Compare with previous version

added 2 commits

• 79ed5850 - Replicate certutil fetching with a fix for root certificates
• 2d8b7be4 - Do not remove received chain

Compare with previous version

@tmaczukin thank you so much for the fix, I left a few questions suggestions, nothing major though

assigned to @tmaczukin and unassigned @steveazz

added 5 commits

• f90ffe6a - Replicate certutil fetching with a fix for root certificates
• c4dc52ea - Do not remove received chain
• ae34d123 - Refactor logging
• 90d458d8 - Limit chain resoliving loop
• 6f5427ef - Refactor ca_chain package and improve test coverage

Compare with previous version

added 1 commit

• ecbb6e12 - Refactor ca_chain package and improve test coverage

Compare with previous version

added 1 commit

• 53bc0352 - Refactor ca_chain package and improve test coverage

Compare with previous version

@steveazz It's now refactored with a huge increase of code coverage :)

I've noticed, that the resolving by issuer URL and by certificate verification have exactly the same function header, so... I've divided the chainResolver to two other resolvers (urlResolver and verifyResolver) which also implement the resolver interface. Which allows us to have one mocked interface that we can use multiple times :)

I've also moved all helper-like functions to a separated file. And at this moment helpers.go file is the only that doesn't have full coverage. It's missing for one case that is almost impossible to happen (I was unable to prepare test case for now) and for http.Get() and cert.Verify() parts, since these relay on external resources.

We have now more files, but they are all short (excluding tests) and I think more clear on what they are suppose to do.

Let me know what do you think about this :)

assigned to @steveazz and unassigned @tmaczukin

mentioned in issue #4827 (closed)

resolved all threads

@tmaczukin I might be bikeshedding here, so this it not going to be a blocking comment:

Is there are a particular reason why we prefer to use the output and not test.NewLocal(logger) which creates a logrus hook specifically for testing? To me, it seems cleaner but both achieve the same thing so I'm good with having it like this, it just seems like we used hooks before?

diff --git a/helpers/tls/ca_chain/resolver_verify_test.go b/helpers/tls/ca_chain/resolver_verify_test.go
index 133c39d1c..5776d4551 100644
--- a/helpers/tls/ca_chain/resolver_verify_test.go
+++ b/helpers/tls/ca_chain/resolver_verify_test.go
@@ -1,13 +1,14 @@
 package ca_chain

 import (
-       "bytes"
        "crypto/x509"
        "errors"
        "testing"

        "github.com/sirupsen/logrus"
        "github.com/stretchr/testify/assert"
+
+       "github.com/sirupsen/logrus/hooks/test"
 )

 type verifierMockFactory func(t *testing.T) verifier
@@ -90,11 +91,9 @@ func TestVerifyResolver_Resolve(t *testing.T) {

        for tn, tc := range tests {
                t.Run(tn, func(t *testing.T) {
-                       out := new(bytes.Buffer)
-
                        logger := logrus.New()
                        logger.SetLevel(logrus.DebugLevel)
-                       logger.SetOutput(out)
+                       hook := test.NewLocal(logger)

                        r := newVerifyResolver(logger).(*verifyResolver)

@@ -112,14 +111,16 @@ func TestVerifyResolver_Resolve(t *testing.T) {

                        assert.Equal(t, tc.expectedCerts, newCerts)

-                       output := out.String()
-                       if len(tc.expectedOutput) > 0 {
-                               for _, expectedLine := range tc.expectedOutput {
-                                       assert.Contains(t, output, expectedLine)
+                       if tc.expectedOutput != nil {
+                               for idx, log := range hook.AllEntries() {
+                                       assert.Equal(t, tc.expectedOutput[idx], log.Message)
+                                       log.Printf("log.Message: %#+v", log.Message)
                                }
-                       } else {
-                               assert.Empty(t, output)
+
+                               return
                        }
+
+                       assert.Empty(t, hook.AllEntries())
                })
        }
 }

added customer needs investigation priority1 severity3 labels

removed needs investigation label

approved this merge request

@tmaczukin this looks good to me, and I feel a lot more confident now with our TLS solution thank you for that! Going through all the GitLab instances specified in #4805 (closed) we are now getting the expected certificate and also customer confirmed this. Let's merge this and prepare a v12.4.1 patch release

merged

mentioned in commit 3114487d

mentioned in issue #4756 (closed)

The MR was picked into 12-4-stable branch and will be released with v12.4.1

removed 1 deleted label

mentioned in commit 9d78e96e

changed title from Fix tls chain building to Fix TLS chain building

changed the description

mentioned in issue #4868 (closed)

mentioned in issue #4890