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

improve git http authentication via repository configuration #5581

Merged
merged 3 commits into from
May 10, 2022

Conversation

abn
Copy link
Member

@abn abn commented May 9, 2022

replace Factory.create_config() w/ Config.create()

Prior to this change when Config was initialised for non-command use, user config.toml and auth.toml files were not loaded. This caused unintended side effects when configuration look up were performed from the Authenticator and other parts of the code.

ensure git repository authn uses exact urls

Since git repository authentication is a special case of repository configuration, the existing assumptions around path matching do not apply. In order to prevent unexpected behaviour due to similar path matching, git authentication will use exact url matching.

git: ignore http auth for ssh url

This change ensures that http-basic auth credentials are only passed to dulwich when the remote url uses http/https schemes.

In addition to the above, it is now ensured that username/password parameters are not passed through to dulwich unless both username and password are configured explicitly. This is to ensure that dulwich does not bail out if it detects a username in the url (eg: ssh://git@github.com).

Relates-to: #5567 (this feature relies on config being available)

@abn abn requested a review from a team May 9, 2022 19:45
@abn abn force-pushed the fix-git-auth branch 2 times, most recently from d0433e2 to c9debec Compare May 9, 2022 20:30
@abn abn added this to the 1.2 milestone May 9, 2022
@abn abn changed the title replace Factory.create_config() w/ Config.create() improve git http authentication via repository configuration May 9, 2022
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Only had time to glance at one thing for now.

.github/workflows/main.yml Outdated Show resolved Hide resolved
@abn abn mentioned this pull request May 10, 2022
@abn abn force-pushed the fix-git-auth branch 2 times, most recently from 110458a to 78c597d Compare May 10, 2022 17:34
abn added 3 commits May 10, 2022 19:38
Prior to this change when `Config` was initialised for non-command use,
user `config.toml` and `auth.toml` files were not loaded. This caused
unintended side effects when configuration look up were performed from
the `Authenticator` and other parts of the code.
Since git repository authentication is a special case of repository
configuration, the existing assumptions around path matching do not
apply. In order to prevent unexpected behaviour due to similar path
matching, git authentication will use exact url matching.
This change ensures that http-basic auth credentials are only passed to
dulwich when the remote url uses http/https schemes.

In addition to the above, it is now ensured that username/password
parameters are not passed through to dulwich unless both username and
password are configured explicitly. This is to ensure that dulwich does
not bail out if it detects a username in the url
(eg: `ssh://git@github.com`).
@abn abn requested a review from neersighted May 10, 2022 17:38
@neersighted neersighted merged commit f19d137 into python-poetry:master May 10, 2022
@abn abn deleted the fix-git-auth branch February 28, 2024 23:55
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2024
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.

2 participants