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

LibGit2 credentials callback overhaul #16308

Merged
merged 3 commits into from
May 21, 2016
Merged

Conversation

wildart
Copy link
Member

@wildart wildart commented May 11, 2016

Update of the LibGit2 credentials callback:

I added a credentials caching to Pkg.update which should be helpful when ssh support will arrive, #16041.

if isset(allowed_types, Cuint(Consts.CREDTYPE_SSH_KEY))
credid = "ssh://$host"
# first try ssh-agent if credentials support its usage
if (isdefined(creds, :use_ssh_agent) ? getfield(creds, :use_ssh_agent) : 0xFF) == 0x00
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this to be ternary

@wildart
Copy link
Member Author

wildart commented May 11, 2016

I added description of libgit2 authentication procedure to LibGit2.credentials_callback documentation and some Credentials types.

- ssh key pair (ssh-agent if specified in `payload`)
- plain text

**Note**: Due to the specifics of `libgit2` authentication procedure, when authentication is failed, this functions is called again without any indication whether authentication was successful or not. In order not to stuck in infinite loop by repeatedly using same faulty credentials, use a call counting strategy to counteract this behaviour.
Copy link
Contributor

Choose a reason for hiding this comment

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

wrap docstrings. "when authentication fails"

@tkelman
Copy link
Contributor

tkelman commented May 12, 2016

Something in the tests here is repeatably freezing on AppVeyor.

@wildart
Copy link
Member Author

wildart commented May 14, 2016

It is libgit2-online test that suddenly became interactive and stick in prompt.

@@ -21,12 +21,17 @@ mktempdir() do dir
finalize(repo)
end
#end
try
#@testset "with incorrect url" begin
repo_path = joinpath(dir, "Example2")
# credential are required because github try authenticate on uknown repo
Copy link
Contributor

Choose a reason for hiding this comment

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

unknown

Copy link
Member Author

Choose a reason for hiding this comment

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

my keybord is dying 💀

@wildart wildart changed the title LibGit2 credentials callback overhaul WIP: LibGit2 credentials callback overhaul May 14, 2016
@wildart
Copy link
Member Author

wildart commented May 14, 2016

I realized that public key isn't necessary in SSH authorization. OpenSSL (as well as MbedTLS) can extract public key from private one, see here. So, I'll designate PR as WIP while I work more on removing public key prompt and related code.

@wildart
Copy link
Member Author

wildart commented May 14, 2016

Above proposition went south. Apparently, system's libssh2 build against libgcrypt which does not support extraction of a public key from a private one. I guess it's another reason to move to mbedtls crypto backend for libssh2.

@tkelman
Copy link
Contributor

tkelman commented May 15, 2016

Any update from libssh2 on reviewing your PR? I tried an early version of it which didn't work, haven't tested your latest commits yet though.

@wildart
Copy link
Member Author

wildart commented May 15, 2016

Judging from accepted libssh2 PRs, this could take some time.

@tkelman
Copy link
Contributor

tkelman commented May 15, 2016

If it works we can use a locally patched version for a little while, but I don't want to be indefinitely shipping unreviewed crypto code.

- plain text

**Note**: Due to the specifics of `libgit2` authentication procedure, when authentication fails,
this functions is called again without any indication whether authentication was successful or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

this function

@tkelman
Copy link
Contributor

tkelman commented May 18, 2016

lgtm after squashing a bit so that none of the intermediate commits would have the hanging issue

@tkelman tkelman added the packages Package management and loading label May 18, 2016
@tkelman
Copy link
Contributor

tkelman commented May 18, 2016

Given #16373 should we hold back on merging this to allow a little bit more debugging?

@wildart
Copy link
Member Author

wildart commented May 19, 2016

Yep, I need to look at this.

wildart added 3 commits May 19, 2016 23:44
- handle https & ssh connections
- new credential types

added caching of credentials in `update`
added cross-platform `getpass` implementation [JuliaLang#8228]
added comments and documentation
show last saved value for pub/priv key path instead of default key path
move `getpass` to `Base`
introduced SSH credential type for cached credentials
changed PK variable to `SSH_KEY_PATH`, added PbK guessing from PK
@wildart
Copy link
Member Author

wildart commented May 20, 2016

It's ready to merge.

@tkelman tkelman merged commit e2f59bc into JuliaLang:master May 21, 2016
@wildart wildart deleted the credentials branch May 21, 2016 06:44
@stevengj
Copy link
Member

Shouldn't getpass be exported and documented?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants