diff mbox

Replace ssh agent authentication with explicit key

Message ID 20161201060449.2742-1-andrew.donnellan@au1.ibm.com
State Changes Requested
Headers show

Commit Message

Andrew Donnellan Dec. 1, 2016, 6:04 a.m. UTC 
From: Russell Currey <ruscur@russell.cc>

libgit2 has a long-standing obscure bug where it gets in an infinite loop
calling ssh-agent.  ssh-agent has been a pain point in snowpatch for a
while now, so let's just not use it - instead, manually specify public and
private keys.  This adds the benefit of configuring the user ("git" was
hardcoded for GitHub/GitLab previously) as well as passphrase support.

Future possibility: support parsing "~/" in the settings file.

Signed-off-by: Russell Currey <ruscur@russell.cc>
---
 examples/openpower.toml |  5 +++++
 src/git.rs              | 15 ++++++++++++++-
 src/main.rs             |  4 ++--
 src/settings.rs         |  9 +++++++++
 4 files changed, 30 insertions(+), 3 deletions(-)

Comments

Andrew Donnellan Dec. 1, 2016, 6:08 a.m. UTC | #1 
On 01/12/16 17:04, Andrew Donnellan wrote:
> From: Russell Currey <ruscur@russell.cc>
>
> libgit2 has a long-standing obscure bug where it gets in an infinite loop
> calling ssh-agent.  ssh-agent has been a pain point in snowpatch for a
> while now, so let's just not use it - instead, manually specify public and
> private keys.  This adds the benefit of configuring the user ("git" was
> hardcoded for GitHub/GitLab previously) as well as passphrase support.
>
> Future possibility: support parsing "~/" in the settings file.
>
> Signed-off-by: Russell Currey <ruscur@russell.cc>

The only reservations I have about this patch:

* It would be nice if we could still fall back on ssh-agent for the 
circumstances where the libgit2 bug doesn't get exposed

* Different projects can use different git remotes which in turn could 
require different usernames or keys

Thoughts?
Russell Currey Dec. 2, 2016, midnight UTC | #2 
On Thu, 2016-12-01 at 17:08 +1100, Andrew Donnellan wrote:
> On 01/12/16 17:04, Andrew Donnellan wrote:
> > From: Russell Currey <ruscur@russell.cc>
> > 
> > libgit2 has a long-standing obscure bug where it gets in an infinite loop
> > calling ssh-agent.  ssh-agent has been a pain point in snowpatch for a
> > while now, so let's just not use it - instead, manually specify public and
> > private keys.  This adds the benefit of configuring the user ("git" was
> > hardcoded for GitHub/GitLab previously) as well as passphrase support.
> > 
> > Future possibility: support parsing "~/" in the settings file.
> > 
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> 
> The only reservations I have about this patch:

I wouldn't mind having a commented out bogus passphrase in the example file

> 
> * It would be nice if we could still fall back on ssh-agent for the 
> circumstances where the libgit2 bug doesn't get exposed

ssh-agent use is fundamentally broken in libgit2 and I don't want it to get used
at all.  If we get ~/ support and have ~/.ssh/id_rsa{.pub} as defaults it'll
catch the majority of cases.

> 
> * Different projects can use different git remotes which in turn could 
> require different usernames or keys

This wouldn't be too much of a PITA to deal with since we set up the auth
callbacks once we're isolated on a single project, so we could have base git
settings then overwrite them.  I think in *most* cases snowpatch will be pushing
to roughly the same place (i.e. GitHub or GitLab) so this won't be a big deal,
you can work around this by running multiple snowpatch instances that test
different projects.

So yeah we should do this but I don't think it should block this patch going in.

> 
> Thoughts?
> 

People talk about aliens looking at humanity and thinking we're super dumb
because of ironic memes and electing Trump and all that, but the likeliness that
other lifeforms are within the range of human intellect and not vastly superior
or inferior is crazy low.  They're either going to read about the LHC and think
we're morons, or look at a picture that calls a deer a "doggo with antlers" and
think we're geniuses.
Andrew Donnellan Dec. 2, 2016, 12:24 a.m. UTC | #3 
On 02/12/16 11:00, Russell Currey wrote:
> I wouldn't mind having a commented out bogus passphrase in the example file

ACK.

> ssh-agent use is fundamentally broken in libgit2 and I don't want it to get used
> at all.  If we get ~/ support and have ~/.ssh/id_rsa{.pub} as defaults it'll
> catch the majority of cases.

Ooh, having ~/.ssh/id_rsa as a default could be useful... I'll look into it.

> This wouldn't be too much of a PITA to deal with since we set up the auth
> callbacks once we're isolated on a single project, so we could have base git
> settings then overwrite them.  I think in *most* cases snowpatch will be pushing
> to roughly the same place (i.e. GitHub or GitLab) so this won't be a big deal,
> you can work around this by running multiple snowpatch instances that test
> different projects.
>
> So yeah we should do this but I don't think it should block this patch going in.

OK.

> People talk about aliens looking at humanity and thinking we're super dumb
> because of ironic memes and electing Trump and all that, but the likeliness that
> other lifeforms are within the range of human intellect and not vastly superior
> or inferior is crazy low.  They're either going to read about the LHC and think
> we're morons, or look at a picture that calls a deer a "doggo with antlers" and
> think we're geniuses.

...ACK? I propose the ruscur equation as an extension of the Drake 
equation to encompass the probability distribution of "doggo with 
antlers" to "LHC"...
Andrew Donnellan Dec. 2, 2016, 12:28 a.m. UTC | #4 
On 01/12/16 17:04, Andrew Donnellan wrote:
> diff --git a/src/settings.rs b/src/settings.rs
> index a4a5614..b440885 100644
> --- a/src/settings.rs
> +++ b/src/settings.rs
> @@ -28,6 +28,14 @@ use std::collections::BTreeMap;
>  // TODO: Give more informative error messages when we fail to parse.
>
>  #[derive(RustcDecodable, Clone)]
> +pub struct Git {
> +    pub user: String,
> +    pub public_key: Option<String>,
> +    pub private_key: String,

This should probably be Option as well.

> +    pub passphrase: Option<String>

We should probably enforce having either keys or passphrase...

> +}
> +
> +#[derive(RustcDecodable, Clone)]
>  pub struct Patchwork {
>      pub url: String,
>      pub port: Option<u16>,
> @@ -65,6 +73,7 @@ impl Project {
>
>  #[derive(RustcDecodable, Clone)]
>  pub struct Config {
> +    pub git: Git,
>      pub patchwork: Patchwork,
>      pub jenkins: Jenkins,
>      pub projects: BTreeMap<String, Project>
>
Russell Currey Dec. 2, 2016, 12:50 a.m. UTC | #5 
On Fri, 2016-12-02 at 11:28 +1100, Andrew Donnellan wrote:
> On 01/12/16 17:04, Andrew Donnellan wrote:
> > diff --git a/src/settings.rs b/src/settings.rs
> > index a4a5614..b440885 100644
> > --- a/src/settings.rs
> > +++ b/src/settings.rs
> > @@ -28,6 +28,14 @@ use std::collections::BTreeMap;
> >  // TODO: Give more informative error messages when we fail to parse.
> > 
> >  #[derive(RustcDecodable, Clone)]
> > +pub struct Git {
> > +    pub user: String,
> > +    pub public_key: Option<String>,
> > +    pub private_key: String,
> 
> This should probably be Option as well.

It's mandatory, see http://alexcrichton.com/git2-rs/git2/struct.Cred.html

> 
> > +    pub passphrase: Option<String>
> 
> 
 * We should probably enforce having either keys or passphrase...
> 
> > +}
> > +
> > +#[derive(RustcDecodable, Clone)]
> >  pub struct Patchwork {
> >      pub url: String,
> >      pub port: Option<u16>,
> > @@ -65,6 +73,7 @@ impl Project {
> > 
> >  #[derive(RustcDecodable, Clone)]
> >  pub struct Config {
> > +    pub git: Git,
> >      pub patchwork: Patchwork,
> >      pub jenkins: Jenkins,
> >      pub projects: BTreeMap<String, Project>
> > 
> 
>
Andrew Donnellan Dec. 2, 2016, 12:52 a.m. UTC | #6 
On 02/12/16 11:28, Andrew Donnellan wrote:
> On 01/12/16 17:04, Andrew Donnellan wrote:
>> diff --git a/src/settings.rs b/src/settings.rs
>> index a4a5614..b440885 100644
>> --- a/src/settings.rs
>> +++ b/src/settings.rs
>> @@ -28,6 +28,14 @@ use std::collections::BTreeMap;
>>  // TODO: Give more informative error messages when we fail to parse.
>>
>>  #[derive(RustcDecodable, Clone)]
>> +pub struct Git {
>> +    pub user: String,
>> +    pub public_key: Option<String>,
>> +    pub private_key: String,
>
> This should probably be Option as well.
>
>> +    pub passphrase: Option<String>
>
> We should probably enforce having either keys or passphrase...

Ah, disregard these comments...
diff mbox

Patch

diff --git a/examples/openpower.toml b/examples/openpower.toml
index 5c0ba40..d582576 100644
--- a/examples/openpower.toml
+++ b/examples/openpower.toml
@@ -14,6 +14,11 @@ 
 # openpower.toml - example configuration file
 #
 
+[git]
+user = "git"
+public_key = "/home/ruscur/.ssh/id_rsa.pub"
+private_key = "/home/ruscur/.ssh/id_rsa"
+
 [patchwork]
 url = "https://russell.cc/patchwork"
 port = 443 #optional
diff --git a/src/git.rs b/src/git.rs
index 679d387..ac4c95e 100644
--- a/src/git.rs
+++ b/src/git.rs
@@ -14,13 +14,15 @@ 
 // git.rs - snowpatch git functionality
 //
 
-use git2::{Repository, Commit, Remote, Error, PushOptions};
+use git2::{Repository, Commit, Remote, Error, PushOptions, Cred};
 use git2::build::CheckoutBuilder;
 
 use std::result::Result;
 use std::path::Path;
 use std::process::{Command, Output};
 
+use settings::Git;
+
 pub static GIT_REF_BASE: &'static str = "refs/heads";
 
 pub fn get_latest_commit(repo: &Repository) -> Commit {
@@ -85,6 +87,17 @@  pub fn apply_patch(repo: &Repository, path: &Path)
     }
 }
 
+pub fn cred_from_settings(settings: &Git) -> Result<Cred, Error> {
+    // We have to convert from Option<String> to Option<&str>
+    let public_key = settings.public_key.as_ref().map(String::as_ref);
+    let passphrase = settings.passphrase.as_ref().map(String::as_ref);
+
+    Cred::ssh_key(&settings.user,
+                  public_key,
+                  Path::new(&settings.private_key),
+                  passphrase)
+}
+
 #[cfg(test)]
 mod tests {
     #[test]
diff --git a/src/main.rs b/src/main.rs
index 426bfdf..07fd32e 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -30,7 +30,7 @@  extern crate url;
 extern crate log;
 extern crate env_logger;
 
-use git2::{Cred, BranchType, RemoteCallbacks, PushOptions};
+use git2::{BranchType, RemoteCallbacks, PushOptions};
 
 use hyper::Client;
 
@@ -150,7 +150,7 @@  fn test_patch(settings: &Config, client: &Arc<Client>, project: &Project, path:
 
     let mut push_callbacks = RemoteCallbacks::new();
     push_callbacks.credentials(|_, _, _| {
-        return Cred::ssh_key_from_agent("git");
+        git::cred_from_settings(&settings.git)
     });
 
     let mut push_opts = PushOptions::new();
diff --git a/src/settings.rs b/src/settings.rs
index a4a5614..b440885 100644
--- a/src/settings.rs
+++ b/src/settings.rs
@@ -28,6 +28,14 @@  use std::collections::BTreeMap;
 // TODO: Give more informative error messages when we fail to parse.
 
 #[derive(RustcDecodable, Clone)]
+pub struct Git {
+    pub user: String,
+    pub public_key: Option<String>,
+    pub private_key: String,
+    pub passphrase: Option<String>
+}
+
+#[derive(RustcDecodable, Clone)]
 pub struct Patchwork {
     pub url: String,
     pub port: Option<u16>,
@@ -65,6 +73,7 @@  impl Project {
 
 #[derive(RustcDecodable, Clone)]
 pub struct Config {
+    pub git: Git,
     pub patchwork: Patchwork,
     pub jenkins: Jenkins,
     pub projects: BTreeMap<String, Project>