Skip to content
Snippets Groups Projects

Set git SSL information only for gitlab host

Merged Alessio Caiazza requested to merge 2148-fix-ssl-cert into master
All threads resolved!

What does this MR do?

This MR overrides git SSL config only for the gitlab host

Why was this MR needed?

See #2148 (closed)

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

Does this MR meet the acceptance criteria?

  • Documentation created/updated
  • Tests
    • Added for this feature/bug
    • All builds are passing
  • Branch has no merge conflicts with master (if you do - rebase it please)

What are the relevant issue numbers?

Closes #2148 (closed)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Author Maintainer

    Test can be seen at https://gitlab.com/nolith/test-runner-2148/-/jobs/32762518

    Cloning into '/nolith/test-runner-2148'...
    * Couldn't find host gitlab.com in the .netrc file; using defaults
    *   Trying 52.167.219.168...
    * TCP_NODELAY set
    * Connected to gitlab.com (52.167.219.168) port 443 (#0)
    * ALPN, offering http/1.1
    * Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
    * successfully set certificate verify locations:
    *   CAfile: /nolith/test-runner-2148.tmp/CI_SERVER_TLS_CA_FILE
      CApath: none

    and submodule

    Submodule 'vendor/github.com/tevino/abool' (https://github.com/tevino/abool.git) registered for path 'vendor/github.com/tevino/abool'
    Cloning into '/nolith/test-runner-2148/vendor/github.com/tevino/abool'...
    * Couldn't find host github.com in the .netrc file; using defaults
    *   Trying 192.30.253.112...
    * TCP_NODELAY set
    * Connected to github.com (192.30.253.112) port 443 (#0)
    * ALPN, offering http/1.1
    * Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
    * successfully set certificate verify locations:
    *   CAfile: /etc/ssl/certs/ca-certificates.crt
      CApath: none
  • assigned to @ayufan

  • assigned to @nolith

  • Author Maintainer

    @ayufan the following patch is the cleanest approch I found without changing git global configuration.

    The problem is that git submodules ignores local config and only uses the global one. So if you are using submodules from your self-signed gitlab instance the build will not work.

    commit c028737b59877cb54b90b35c330c7a8e05d0adaa (HEAD -> refs/heads/2148-fix-ssl-cert)
    Author: Alessio Caiazza <acaiazza@gitlab.com>
    Date:   Mon Sep 18 17:11:02 2017 +0200
    
        Do not touch git global config
    
    diff --git a/shells/abstract.go b/shells/abstract.go
    index 2791d4a8..d6f1184c 100644
    --- a/shells/abstract.go
    +++ b/shells/abstract.go
    @@ -30,18 +30,20 @@ func (b *AbstractShell) writeExports(w ShellWriter, info common.ShellScriptInfo)
     	}
     }
     
    -func (b *AbstractShell) writeGitSSLConfig(w ShellWriter, info common.ShellScriptInfo) error {
    -	repoURL, err := url.Parse(info.Build.GitInfo.RepoURL)
    +func (b *AbstractShell) gitSSLConfig(w ShellWriter, build *common.Build) (conf map[string]string) {
    +	repoURL, err := url.Parse(build.GitInfo.RepoURL)
     	if err != nil {
    -		return err
    +		w.Warning("git SSL config: Can't parse repository URL. %s", err)
    +		return
     	}
     
     	repoURL.Path = ""
     	repoURL.RawPath = ""
     	repoURL.User = nil
     	gitlabHost := repoURL.String()
    +	conf = make(map[string]string)
     
    -	for _, variable := range info.Build.GetCITLSVariables() {
    +	for _, variable := range build.GetCITLSVariables() {
     		var configKey, configValue string
     		switch variable.Key {
     		case "CI_SERVER_TLS_CA_FILE":
    @@ -55,18 +57,19 @@ func (b *AbstractShell) writeGitSSLConfig(w ShellWriter, info common.ShellScript
     		}
     		configValue = w.TmpFile(variable.Key)
     		composedConfigKey := fmt.Sprintf("http.%s.%s", gitlabHost, configKey)
    -		w.Command("git", "config", "--global", composedConfigKey, configValue)
    +		conf[composedConfigKey] = configValue
     	}
     
    -	return nil
    +	return
     }
     
     func (b *AbstractShell) writeCloneCmd(w ShellWriter, build *common.Build, projectDir string) {
    -	templateDir := w.MkTmpDir("git-template")
    -	args := []string{"clone", "--no-checkout", build.GitInfo.RepoURL, projectDir, "--template", templateDir}
    +	args := []string{"clone", "--no-checkout", build.GitInfo.RepoURL, projectDir, "--config", "fetch.recurseSubmodules=false"}
    +	for key, value := range b.gitSSLConfig(w, build) {
    +		args = append(args, "--config", fmt.Sprintf("%s=%s", key, value))
    +	}
     
     	w.RmDir(projectDir)
    -	w.Command("git", "config", "-f", path.Join(templateDir, "config"), "fetch.recurseSubmodules", "false")
     
     	if depth := build.GetGitDepth(); depth != "" {
     		w.Notice("Cloning repository for %s with git depth set to %s...", build.GitInfo.Ref, depth)
    @@ -91,6 +94,10 @@ func (b *AbstractShell) writeFetchCmd(w ShellWriter, build *common.Build, projec
     	w.Cd(projectDir)
     	w.Command("git", "config", "fetch.recurseSubmodules", "false")
     
    +	for key, value := range b.gitSSLConfig(w, build) {
    +		w.Command("git", "config", key, value)
    +	}
    +
     	// Remove .git/{index,shallow}.lock files from .git, which can fail the fetch command
     	// The file can be left if previous build was terminated during git operation
     	w.RmFile(".git/index.lock")
    @@ -330,15 +337,12 @@ func (b *AbstractShell) writeSubmoduleUpdateCmds(w ShellWriter, info common.Shel
     
     func (b *AbstractShell) writeGetSourcesScript(w ShellWriter, info common.ShellScriptInfo) (err error) {
     	b.writeExports(w, info)
    -	if err := b.writeGitSSLConfig(w, info); err != nil {
    -		return err
    -	}
     
     	if info.PreCloneScript != "" && info.Build.GetGitStrategy() != common.GitNone {
     		b.writeCommands(w, info.PreCloneScript)
     	}
     
    -	if err = b.writeCloneFetchCmds(w, info); err != nil {
    +	if err := b.writeCloneFetchCmds(w, info); err != nil {
     		return err
     	}
     

    Next step

    I think we should inform the builder if it's running inside a container (disposable environment) or not. In the first case change the global config and go on, on the second case change only local settings and documents how to configure your enviroment properly.

  • Alessio Caiazza mentioned in merge request !690 (merged)

    mentioned in merge request !690 (merged)

  • I start to agree that this is the only solution :)

    Edited by Kamil Trzciński
  • added Deliverable ~1672339 typebug customer labels

  • Alessio Caiazza added 35 commits

    added 35 commits

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Author Maintainer

    @ayufan I've rebased after !690 (merged) work.

  • assigned to @ayufan

  • Kamil Trzciński
  • assigned to @nolith

  • added 1 commit

    Compare with previous version

  • Alessio Caiazza resolved all discussions

    resolved all discussions

  • Author Maintainer

    Waiting for the pipelines.

    @ayufan

  • assigned to @ayufan

  • @nolith Almost there, but it would help to have some tests for that code as it seems to me now that it is not working :)

  • assigned to @nolith

  • added 1 commit

    • ac5ab2fc - Remove gitConfigDestination alias type

    Compare with previous version

  • added 1 commit

    • 43cbee8f - Add AbstractShell.writeGitSSLConfig test

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 2baa9495 - Remove path from runner URL in git config

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • assigned to @ayufan

  • Kamil Trzciński
  • Kamil Trzciński
  • Kamil Trzciński
  • Kamil Trzciński
  • @nolith Awesome work! We only need to solve the certificate fetching, as we should not hardcode one.

  • assigned to @nolith

  • mentioned in issue #2815 (closed)

  • assigned to @ayufan

  • added 1 commit

    • e7361a6b - Remove useless var buf []byte

    Compare with previous version

  • Author Maintainer

    @ayufan I left a comment on !687 (comment 42529285) everything else should be covered.

  • Kamil Trzciński resolved all discussions

    resolved all discussions

  • Kamil Trzciński approved this merge request

    approved this merge request

  • mentioned in commit 0003707d

  • Jonathon Reinhart mentioned in issue #3497

    mentioned in issue #3497

  • mentioned in issue #2868 (closed)

  • mentioned in issue #1569 (closed)

  • Please register or sign in to reply
    Loading