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.

Sign up for GitHub

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

Jump to bottom

Use GITHUB_ACCESS_TOKEN as auth for cloning github.com private repos in HTTPS #834

Closed
wants to merge 1 commit into from
Closed

Use GITHUB_ACCESS_TOKEN as auth for cloning github.com private repos in HTTPS #834

wants to merge 1 commit into from

Conversation

flovilmart
Copy link
Contributor

When specifying GIT_AUTH environment variable in the form of either "domain.com=token" or "domain.com=username:pass" the value will be injected into the git url to provide headless authentication for build servers.

The default "token" or "username:password" will be applied to all github.com requests.

@flovilmart flovilmart mentioned this pull request Oct 7, 2015
Closed
@flovilmart
Copy link
Contributor Author

In reference of #815

The problem was actually at the time Carthage tries to resolve the dependencies with https:// urls that was failing. With private repos it is possible to skip the login phase normally required with inserting the GITHUB_ACCESS_TOKEN for all github.com URLs.

mdiep
mdiep reviewed Oct 8, 2015
View reviewed changes
Source/CarthageKit/Git.swift
@@ -157,7 +157,7 @@ public func cloneRepository(cloneURL: GitURL, _ destinationURL: NSURL, bare: Boo
arguments.append("--bare")
}

return launchGitTask(arguments + [ "--quiet", cloneURL.URLString, destinationURL.path! ])
return launchGitTask(arguments + [ "--quiet", cloneURL.URLStringWithGithubAccessToken, destinationURL.path! ])
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to bake this in earlier if possible, so that Git.swift can know as little as possible about GitHub.swift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So rename add another environment variable that could serve as the identity for the the git URL?

@mdiep
Copy link
Member

mdiep commented Oct 8, 2015

Is this still an El Capitan-specific issue? Access tokens were working correctly before, right? What changed?

@flovilmart
Copy link
Contributor Author

I can't say if it was working before but the token wasn't used to actually fetch/clone the repo. It was supposedly using the keychain to pick up the credentials. Now that's not working for me, on El Capitan.

I realized while trying to fix the issue that it works while setting the params in the run scheme options, but not from the terminal.

How should one specify creds for https github urls?

@jdhealy
Copy link
Member

jdhealy commented Oct 8, 2015

It was supposedly using the keychain to pick up the credentials. Now that's not working for me, on El Capitan.

Maybe the credential helper was affected by System Integrity Protection? Step 3 of this GitHub help article instructs to put the helper executable in the same directory as the git executable. Try which git-credential-osxkeychain.

Maybe also check if the credential.helper config is set?

git config --get --global credential.helper

@flovilmart
Copy link
Contributor Author 

$ which git-credential-osxkeychain
/usr/local/bin/git-credential-osxkeychain

$ git config --get --global credential.helper
osxkeychain

@flovilmart
Copy link
Contributor Author

@jdhealy after investigation it seems that the error was related to improper configuration on my dev machine. I never use HTTPS but SSH based clones.

However, in the context of CI, one may want to build the carthage framework with it's dependencies so this fix could prove helpful (once added with all domains) in those scenarios.

@flovilmart flovilmart changed the title Adds github access token to clone URLs for private repositories Adds support for GIT_AUTH environment variable Oct 9, 2015
@mdiep
Copy link
Member

mdiep commented Oct 10, 2015

Would you mind clarifying the current state of this and the bug that it's fixing? I'm a little confused based on some of the recent comments.

@flovilmart
Copy link
Contributor Author

The bug I experienced with HTTPS git update was not a bug but a 'wrong' config on my machine. I never use HTTPS, rather git+ssh

The intended feature that it adds:
on a build server, letting Carthage clone/fetch private repositories would require adding the credentials to the keychain.
This proposed fix allows passing a GIT_AUTH environment variable that can take the form of either:

GIT_AUTH=token
GIT_AUTH=domain.com=token,otherdomain.com=othertoken

In order to provide proper authentication to the private repository instead of adding keychain username and passwords

mdiep
mdiep reviewed Oct 17, 2015
View reviewed changes
Source/CarthageKit/Git.swift
let host = url.host,
let auth = gitAuth[host] {
return _URLString.stringByReplacingOccurrencesOfString("\(host)", withString: "\(auth)@\(host)")
}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I wasn't clearer before, but I think the appropriate place to add this logic is inside GitHubRepository.HTTPSURL. Then all the logic about GitHub authentication tokens can stay inside GitHub.swift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move it around then :)

@mdiep
Copy link
Member

mdiep commented Oct 17, 2015

Thanks for the explanation! That helped a lot. 😄

@flovilmart flovilmart force-pushed the swift2-fix-github-access branch 3 times, most recently from 49b00ac to 6363bcc Compare October 17, 2015 13:44
@flovilmart flovilmart changed the title Adds support for GIT_AUTH environment variable Use GITHUB_ACCESS_TOKEN as auth for cloning github.com private repos in HTTPS Oct 17, 2015
@flovilmart
Copy link
Contributor Author

@mdiep I rebased in 1 commit and updated the commit to reflect the simple change to add auth in GitHub.HTTPSUrl

mdiep
mdiep reviewed Oct 17, 2015
View reviewed changes
Source/CarthageKit/GitHub.swift

var serverAuth:String = ""
if let auth = gitAuth[server.hostname] {
serverAuth = "\(auth)@"
Copy link
Member

Choose a reason for hiding this comment

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

The indentation looks off on this line. Otherwise this looks great! ✨

@flovilmart
Copy link
Contributor Author

rebased and pushed should the PR be on the master instead?

@jdhealy
Copy link
Member

jdhealy commented Oct 17, 2015

Minor concern: could the modified URL (specifically, including the token) show up in any files written to disk? Mainly, .git/config including the private token, with --use-submodules.

@flovilmart
Copy link
Contributor Author

@jdhealy I don't believe we should store the token anywhere, and that it should be per environment basis. I just checked and when using --use-submodules the token doesn't get stored locally in .git/config

is that the behaviour you'd expect?

@mdiep
Copy link
Member

mdiep commented Oct 17, 2015

rebased and pushed should the PR be on the master instead?

Yeah, that would probably be a good idea. 👍

@jdhealy
Copy link
Member

jdhealy commented Oct 17, 2015

@flovilmart Cool, seems like git rightfully treats that like private information. People do unintuitive things with git repos and their .git directories (e.g., checking them into other git repositories), so thankfully this PR doesn't make it any easier for them to accidentally publish a secret token.

LGTM 👍

@flovilmart
Copy link
Contributor Author

@jdhealy I unfortunately started this branch out of the swift-2 branch. This branch also seems fully functional now. Any plan / schedule to merge swift-2 to master?

@mdiep
Copy link
Member

mdiep commented Oct 18, 2015

Any plan / schedule to merge swift-2 to master?

We're waiting on our upstream dependencies to create Swift 2 releases. So I think it will be another week or two yet.

@flovilmart
Copy link
Contributor Author

I had no problems with Xcode 7 to build :)

@carlossless
Copy link

+1

@mdiep
Copy link
Member

mdiep commented Nov 4, 2015

Swift 2 is on master now if you want to retarget this PR.

@flovilmart
Copy link
Contributor Author

@mdiep I need to close and reopen?

@flovilmart flovilmart mentioned this pull request Nov 4, 2015
Merged
@flovilmart
Copy link
Contributor Author

See #891

@flovilmart flovilmart closed this Nov 4, 2015
@nlutsenko nlutsenko mentioned this pull request Mar 10, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@flovilmart @mdiep @jdhealy @carlossless