Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add additional git authentication rules #13477

Merged
merged 28 commits into from Feb 5, 2022

Conversation

Chumper
Copy link
Contributor

@Chumper Chumper commented Jan 10, 2022

Changes:

Add additional git authentication rules to the git environment.

Context:

We should support all common ways on how to fetch a repository.
This includes https and ssh.
When this is implemented it should solve some problems with docker sidecars not being able to fetch repositories because of missing private keys or missing tokens.
This makes it obsolete for bot users to somehow fiddle a ssh key into the side car containers.

Taken from:
https://coolaj86.com/articles/vanilla-devops-git-credentials-cheatsheet/

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@Chumper Chumper marked this pull request as draft January 10, 2022 23:27
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs also real test runs

lib/util/git/auth.ts Outdated Show resolved Hide resolved
lib/util/git/auth.ts Outdated Show resolved Hide resolved
lib/util/git/types.ts Outdated Show resolved Hide resolved
@Chumper
Copy link
Contributor Author

Chumper commented Jan 11, 2022

Needs also real test runs

Will do and report back

@Chumper Chumper marked this pull request as ready for review January 11, 2022 22:40
@Chumper Chumper requested a review from viceice January 11, 2022 22:40
@Chumper
Copy link
Contributor Author

Chumper commented Jan 14, 2022

Tested on a real repo with a private repo: renovate-support/feature-13477#2
This only tests the https case because go will not use git@...

However the next step will be to add it to python, where python can use private repos with git@...

lib/util/git/auth.ts Outdated Show resolved Hide resolved
lib/util/git/auth.ts Outdated Show resolved Hide resolved
lib/util/git/auth.ts Outdated Show resolved Hide resolved
lib/util/git/auth.ts Show resolved Hide resolved
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some doubts on the url changes

lib/manager/gomod/__snapshots__/artifacts.spec.ts.snap Outdated Show resolved Hide resolved
lib/manager/gomod/__snapshots__/artifacts.spec.ts.snap Outdated Show resolved Hide resolved
@Chumper
Copy link
Contributor Author

Chumper commented Jan 15, 2022

We may need to defer this or use another solution.
I did a deep dive into git config and what it looks like.

Some things I found out:

  • git config url."https://token@github.com/".insteadOf "https://github.com/"
    will be represented as the following in the config:
    [url "https://token@github.com/"]
      insteadOf = https://github.com/
    
    Which means that the insteadOf url is indeed the key that will be used for the lookup.
  • The git config supports multiple insteadOf like this:
    [url "https://token@github.com/"]
      insteadOf = https://github.com/
      insteadOf = git@github.com:
    
    This helps us to have a single replacement url for all protocols we want to support.
    This can be set with the following commands:
    git config url."https://token@github.com/".insteadOf "https://github.com/"
    git config --add url."https://token@github.com/".insteadOf "git@github.com:"
    
    Notice the additional --add in the second command.
    This is needed to add the second value.
  • The environment variables support no way of adding configs to a key.
    So the following will NOT work
    GIT_CONFIG_COUNT = 2
    GIT_CONFIG_VALUE_0 = url."https://token@github.com/".insteadOf
    GIT_CONFIG_KEY_0 = https://github.com/
    GIT_CONFIG_VALUE_1 = url."https://token@github.com/".insteadOf
    GIT_CONFIG_KEY_1 = git@github.com:
    
    This will result in the following:
    [url "https://token@github.com/"]
      insteadOf = git@github.dom:
    
    So the first option will be overriden.

In essence there is currently no way for me to add this feature with the same replacement url
This means that we can implement this feature but only partially for gitlab.

I would therefore suggest to keep the api, ssh and git usernames and treat the https replacement as priority.
That means that when the token is the same, only the https option will be activated because others are overriden.

@rarkins
Copy link
Collaborator

rarkins commented Jan 18, 2022

I like your idea of the workaround, and isn't it still superior to what we have now anyway?

@Chumper
Copy link
Contributor Author

Chumper commented Jan 19, 2022

Yes, it would still be an improvement, my plan is the following:

  • Add ssh, git as username in the url
  • leave the https replacement as is without username (that is completely backwards compatible)
  • Add the https replacement last, so that regardless if a username is given or not the https rule will always take priority

@Chumper
Copy link
Contributor Author

Chumper commented Jan 20, 2022

Refactored the code according to the plan mentioned above:

* Add ssh, git as username in the url
* Leave the https replacement as is without username (that is completely backwards compatible)
* Add the https replacement last, so that regardless if a username is given or not the https rule will always take priority

@CLAassistant
Copy link

CLAassistant commented Jan 27, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but we should validate this with real tests against major platforms.

@zharinov Any hints for best testing this?

@viceice
Copy link
Member

viceice commented Jan 28, 2022

needs deconflicting

@zharinov
Copy link
Collaborator

zharinov commented Jan 28, 2022

Any hints for best testing this?

I know almost nothing about this particular subject 🤔🤷‍♂️

@Chumper
Copy link
Contributor Author

Chumper commented Jan 28, 2022

@viceice We can test this once we implement this in python.
For golang it only supports https git repositories.
That's why the new additional rules will have no impact for the current implementation (which should only be golang)

Poetry supports git@ syntax as well, which this is the base for.
So when we introduce this for poetry I can run this agains a real repository.

If required, I can combine the two PRs into this one

@rarkins
Copy link
Collaborator

rarkins commented Feb 5, 2022

I think I'm OK to merge this as it appears backwards-compatible. Worst case it might makes some artifacts updating fail, but that's still a "soft" fail which should also be easily detectable.

@rarkins rarkins requested a review from viceice February 5, 2022 06:59
@viceice viceice enabled auto-merge (squash) February 5, 2022 07:10
@viceice viceice merged commit 2eadd19 into renovatebot:main Feb 5, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 31.67.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 8, 2022
@Chumper Chumper deleted the feat/additionalGitEnvs branch May 16, 2022 08:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants