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

Already on GitHub? Sign in to your account

Attempt user/pass authentication for git repos not on github #1162

Merged
merged 4 commits into from Mar 10, 2013

Conversation

Projects
None yet
9 participants
Contributor

machee commented Sep 30, 2012

If git clone fails because of authentication, the repo isn't on github, and the url has http or https scheme composer should attempt http basic authentication.

Also, ask if the user would like to reuse the user/pass for repos at the same scheme://domain

Owner

Seldaek commented Oct 1, 2012

This looks good but could you also fix https://github.com/machee/composer/blob/9d03dc5a899a1e6f4c1215034a509c681f4aa023/src/Composer/Downloader/GitDownloader.php#L54 so that it detects the user/pass at the next run and doesn't prompt every time it needs an update?

Owner

Seldaek commented Oct 1, 2012

Also I'd say the authorization storage calls should only use $match[2] or the hostname instead of including the scheme.

Contributor

machee commented Oct 1, 2012

Added the GIT_ASKPASS env var because git was picking up on the need for user/pass during updates.

I added "origin" to the regex searching for user/pass in git remote because at one point when I was testing, the user and pass weren't present for the "composer" remote. Later, user/pass was present for both and I couldn't reproduce the original output.

I ran into the need for urlencode() because I happened to have a special character in my password. It should probably be added for the previous github section?

That also led me to an issue where escapeshellarg removes the % on Windows. Not sure of a solution to that or how much of a concern it is.

Contributor

cs278 commented Oct 1, 2012

@Seldaek That means a credential saved for a secure channel could be unintentionally transmitted over an insecure channel.

Contributor

machee commented Oct 1, 2012

@cs278 I suppose that's basically what I had in mind when originally writing it that way. Given the hubbub in the MITM PR #1092 I'm definitely for writing it with security in mind.

Should I warn the user when a repo is insecure and confirm they'd like to continue? Seems to maintain usability while encouraging security.

Also, I was surprised to see credentials stored in the git remotes. I'm not sure of a way to prevent that while running git non-interactively, but it seems like there should be an option if possible.

Owner

Seldaek commented Oct 2, 2012

@machee @cs278 true it might be safer to just save it including the scheme, though I'd guess if one same host has both http & https available, security conscious people would only use the safer option. Anyway I'm fine with reverting that change to how it originally was.

As for storing the credentials in the remote, yeah it's "ugly", but it's that or prompting at every update/install which sucks too. I'm really not sure what's best.. We could encrypt it in a global cache so you would just have to type your password once to unlock it, but that still sucks a bit if you update frequently.

@machee Regarding escapeshellarg removing %, no idea how to fix that.

Contributor

till commented Oct 19, 2012

Couldn't you store that stuff in $COMPOSER_HOME?

I think if it's not readable by anyone but the current $USER then this is fine in terms of convenience and security. People should be aware of what they do but e.g. I bet most people using git, cache the passphrase anyway and don't type it in each time.

When you start encrypting stuff you likely create a circle of events where you need another password to decrypt the passwords that have been encrypted.

At least it sounds like there is no gain — it just looks more obscure.

Owner

Seldaek commented Oct 19, 2012

@till agreed. I would say either store in the config like the github oauth token, or in another file.. either way this stuff should be chmod'ed to 600.

jebbench commented Jan 8, 2013

Any idea when this will be merged into a release?

emodric commented Feb 21, 2013

I would also like to see this merged as we use bitbucket for our private repos.

Contributor

benji07 commented Mar 1, 2013

👍

This would be VERY handy

👍

@Seldaek Seldaek merged commit 5ed5f13 into composer:master Mar 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment