[libvirt] [PATCH] add support for libssh2 password from auth file
Hi, I was discussing with Daniel about the best way to pass the ssh password when using such kind of uri: 'xen+libssh2:// root@192.168.0.10?sshauth=password' As it seems passing the password in the uri is not a good option, maybe we can grab it from auth conf ? it seems it's not the case as now (tell me if i am wrong). So enclosed a patch to add this feature. As you can see in virnetclient.c there is no virAuthGetPassword call, so the authfile is never used. The patch enclosed is modifying the function prototype to add virConnectPtr parameter (so remote_driver.c virNetClientNewLibSSH2 call has to be updated too and the corresponding .h too). Once we have access to virConnectPtr, as you will see in the patch we can check if authMethods is set to 'password' and grab the password from auth file by calling virAuthGetPassword. regards, david
enclosed the patch from git repo On Thu, Jun 27, 2013 at 2:31 PM, David Maciejak <david.maciejak@gmail.com>wrote:
Hi,
I was discussing with Daniel about the best way to pass the ssh password when using such kind of uri: 'xen+libssh2:// root@192.168.0.10?sshauth=password'
As it seems passing the password in the uri is not a good option, maybe we can grab it from auth conf ? it seems it's not the case as now (tell me if i am wrong).
So enclosed a patch to add this feature.
As you can see in virnetclient.c there is no virAuthGetPassword call, so the authfile is never used.
The patch enclosed is modifying the function prototype to add virConnectPtr parameter (so remote_driver.c virNetClientNewLibSSH2 call has to be updated too and the corresponding .h too).
Once we have access to virConnectPtr, as you will see in the patch we can check if authMethods is set to 'password' and grab the password from auth file by calling virAuthGetPassword.
regards, david
On 06/27/13 14:31, David Maciejak wrote:
Hi,
I was discussing with Daniel about the best way to pass the ssh password when using such kind of uri: 'xen+libssh2://root@192.168.0.10?sshauth=password <http://root@192.168.0.10?sshauth=password>'
As it seems passing the password in the uri is not a good option, maybe we can grab it from auth conf ? it seems it's not the case as now (tell me if i am wrong).
I was planing on doing this stuff, but never managed to finish this.
So enclosed a patch to add this feature.
As you can see in virnetclient.c there is no virAuthGetPassword call, so the authfile is never used.
The patch enclosed is modifying the function prototype to add virConnectPtr parameter (so remote_driver.c virNetClientNewLibSSH2 call has to be updated too and the corresponding .h too).
Once we have access to virConnectPtr, as you will see in the patch we can check if authMethods is set to 'password' and grab the password from auth file by calling virAuthGetPassword.
please use git format-patch and send-email in the future, it makes reviewing easier. See the attached patch for the review. Peter
On 06/28/2013 12:28 PM, Peter Krempa wrote:
On 06/27/13 14:31, David Maciejak wrote:
Hi,
I was discussing with Daniel about the best way to pass the ssh password when using such kind of uri: 'xen+libssh2://root@192.168.0.10?sshauth=password <http://root@192.168.0.10?sshauth=password>'
As it seems passing the password in the uri is not a good option, maybe we can grab it from auth conf ? it seems it's not the case as now (tell me if i am wrong). I was planing on doing this stuff, but never managed to finish this.
So enclosed a patch to add this feature.
As you can see in virnetclient.c there is no virAuthGetPassword call, so the authfile is never used.
The patch enclosed is modifying the function prototype to add virConnectPtr parameter (so remote_driver.c virNetClientNewLibSSH2 call has to be updated too and the corresponding .h too).
Once we have access to virConnectPtr, as you will see in the patch we can check if authMethods is set to 'password' and grab the password from auth file by calling virAuthGetPassword.
please use git format-patch and send-email in the future, it makes reviewing easier.
See the attached patch for the review.
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi all, Peter, you could have reformatted it for better replies on separate code hunks ;-) Like I did now ;-) Comment inline ...
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 7a0c1f6..012d6d5 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -658,7 +658,9 @@ doRemoteOpen(virConnectPtr conn, sshauth, netcat, sockname, - auth); + auth, + conn);
This line will require tweaking ... see below ...
... the virConnectPtr should be the first argument in this case. Also the closing brace of the parameter list should be on the same line as the last argument.
+ ) { virNetSocketPtr sock = NULL; virNetClientPtr ret = NULL; @@ -402,6 +405,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, char *confdir = virGetUserConfigDirectory(); char *knownhosts = NULL; char *privkey = NULL; + char *password = NULL;
/* Use default paths for known hosts an public keys if not provided */ if (confdir) { @@ -471,11 +475,21 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, if (!(command = virBufferContentAndReset(&buf))) goto no_memory;
- if (virNetSocketNewConnectLibSSH2(host, port, username, NULL, privkey, + password = virAuthGetPassword(conn, authPtr, "libssh2", username, host); + + if ((strcmp(authMethods,"password") == 0) && password) {
authMethods is a comma separated list of possible methods to use. With a strcmp this will not work if the list will contain other methods too. Use strstr() instead of strcmp. Also this condition should make the call to virAuthGetPassword conditional otherwise we would always ask the user for the password if it isn't stored in the auth file even if it will never be used.
You suggest using strstr() ? Interesting libvirt doesn't have it's own implementation like it does on many other things. However using the substring matching approach is not good IMHO as it can also match the substring of real method, e.g. if you have e.g. authMethods = "password,auth-password" (bogus but works for the example to demonstrate) and you do strstr(authMethods, "password") is can also match if authMethods = "auth-password" but doesn't contain real "password" method. I suggest tokenizing it after commas, like I'm doing for my personal projects, e.g. something like: typedef struct tTokenizer { char **tokens; int numTokens; } tTokenizer; tTokenizer tokenize(char *string, char *by); void free_tokens(tTokenizer t); tTokenizer tokenize(char *string, char *by) { char *tmp; char *str; char *save; char *token; int i = 0; tTokenizer t; tmp = strdup(string); t.tokens = malloc( sizeof(char *) ); for (str = tmp; ; str = NULL) { token = strtok_r(str, by, &save); if (token == NULL) break; t.tokens = realloc( t.tokens, (i + 1) * sizeof(char *) ); t.tokens[i++] = strdup(token); } t.numTokens = i; return t; } void free_tokens(tTokenizer t) { int i; for (i = 0; i < t.numTokens; i++) t.tokens[i] = free( t.tokens[i]); } [Code borrowed from my CDVWS project (available at https://github.com/MigNov/CDVWS)] where you could easily call something like: tTokenizer authMethodTokens = tokenize(authMethods, ",") and then check it using approach like: hasPasswordMethod = false; for (i = 0; i < t.numTokens; i++) if (strcmp(t.tokens[i], "password") == 0) hasPasswordMethod = true; I think libvirt already have some similar functions to do it, doesn't it? Thanks, Michal -- Michal Novotny <minovotn@redhat.com>, RHCE, Red Hat Virtualization | libvirt-php bindings | php-virt-control.org
On Fri, Jun 28, 2013 at 1:03 PM, Michal Novotny <minovotn@redhat.com> wrote:
On 06/28/2013 12:28 PM, Peter Krempa wrote:
On 06/27/13 14:31, David Maciejak wrote:
Hi,
I was discussing with Daniel about the best way to pass the ssh password when using such kind of uri: 'xen+libssh2://root@192.168.0.10?sshauth=password <http://root@192.168.0.10?sshauth=password>'
As it seems passing the password in the uri is not a good option, maybe we can grab it from auth conf ? it seems it's not the case as now (tell me if i am wrong). I was planing on doing this stuff, but never managed to finish this.
So enclosed a patch to add this feature.
As you can see in virnetclient.c there is no virAuthGetPassword call, so the authfile is never used.
The patch enclosed is modifying the function prototype to add virConnectPtr parameter (so remote_driver.c virNetClientNewLibSSH2 call has to be updated too and the corresponding .h too).
Once we have access to virConnectPtr, as you will see in the patch we can check if authMethods is set to 'password' and grab the password from auth file by calling virAuthGetPassword.
please use git format-patch and send-email in the future, it makes reviewing easier.
See the attached patch for the review.
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi all, Peter, you could have reformatted it for better replies on separate code hunks ;-) Like I did now ;-)
Thanks for the review guys. ...
I suggest tokenizing it after commas, like I'm doing for my personal projects
I agree with Michal, using strstr instead of strcmp has also some drawbacks. regards, david
On 06/28/2013 01:41 PM, David Maciejak wrote:
On Fri, Jun 28, 2013 at 1:03 PM, Michal Novotny <minovotn@redhat.com <mailto:minovotn@redhat.com>> wrote:
On 06/28/2013 12:28 PM, Peter Krempa wrote: > On 06/27/13 14:31, David Maciejak wrote: >> Hi, >> >> I was discussing with Daniel about the best way to pass the ssh password >> when using such kind of uri: >> 'xen+libssh2://root@192.168.0.10?sshauth=password <http://root@192.168.0.10?sshauth=password> >> <http://root@192.168.0.10?sshauth=password>' >> >> As it seems passing the password in the uri is not a good option, maybe we >> can grab it from auth conf ? it seems it's not the case as now (tell me >> if i am wrong). > I was planing on doing this stuff, but never managed to finish this. > >> So enclosed a patch to add this feature. >> >> As you can see in virnetclient.c there is no virAuthGetPassword call, so >> the authfile is never used. >> >> The patch enclosed is modifying the function prototype to add >> virConnectPtr parameter (so remote_driver.c virNetClientNewLibSSH2 call >> has to be updated too and the corresponding .h too). >> >> Once we have access to virConnectPtr, as you will see in the patch we >> can check if authMethods is set to 'password' and grab the password from >> auth file by calling virAuthGetPassword. >> >> > please use git format-patch and send-email in the future, it makes > reviewing easier. > > See the attached patch for the review. > > Peter > > > > -- > libvir-list mailing list > libvir-list@redhat.com <mailto:libvir-list@redhat.com> > https://www.redhat.com/mailman/listinfo/libvir-list
Hi all, Peter, you could have reformatted it for better replies on separate code hunks ;-) Like I did now ;-)
Thanks for the review guys. ...
I suggest tokenizing it after commas, like I'm doing for my personal projects
I agree with Michal, using strstr instead of strcmp has also some drawbacks.
Hi David, if libvirt doesn't have the tokenizer support yet, it may be a good RFE as I believe it could be really useful ;-) Peter, do you know about anything libvirt supports to tokenize string? Thanks, Michal
regards, david
-- Michal Novotny <minovotn@redhat.com>, RHCE, Red Hat Virtualization | libvirt-php bindings | php-virt-control.org
On Fri, Jun 28, 2013 at 01:43:44PM +0200, Michal Novotny wrote:
if libvirt doesn't have the tokenizer support yet, it may be a good RFE as I believe it could be really useful ;-)
Peter, do you know about anything libvirt supports to tokenize string?
We have virStringSplit for tokenizing strings which have a fixed separator. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/28/2013 01:48 PM, Daniel P. Berrange wrote:
On Fri, Jun 28, 2013 at 01:43:44PM +0200, Michal Novotny wrote:
if libvirt doesn't have the tokenizer support yet, it may be a good RFE as I believe it could be really useful ;-)
Peter, do you know about anything libvirt supports to tokenize string? We have virStringSplit for tokenizing strings which have a fixed separator.
Daniel
That sounds good, however what about splitting the function to 2 separate functions, one accepting the second parameter as the separator, called e.g. virStringSplitBy() and second just calling the first one with the fixed separator? Wouldn't it be better? Michal -- Michal Novotny <minovotn@redhat.com>, RHCE, Red Hat Virtualization | libvirt-php bindings | php-virt-control.org
On Fri, Jun 28, 2013 at 01:50:08PM +0200, Michal Novotny wrote:
On 06/28/2013 01:48 PM, Daniel P. Berrange wrote:
On Fri, Jun 28, 2013 at 01:43:44PM +0200, Michal Novotny wrote:
if libvirt doesn't have the tokenizer support yet, it may be a good RFE as I believe it could be really useful ;-)
Peter, do you know about anything libvirt supports to tokenize string? We have virStringSplit for tokenizing strings which have a fixed separator.
Daniel
That sounds good, however what about splitting the function to 2 separate functions, one accepting the second parameter as the separator, called e.g. virStringSplitBy() and second just calling the first one with the fixed separator?
Wouldn't it be better?
That sounds like overkill to me unless we have a clear need for it somewhere in our code. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/28/13 13:50, Michal Novotny wrote:
On 06/28/2013 01:48 PM, Daniel P. Berrange wrote:
On Fri, Jun 28, 2013 at 01:43:44PM +0200, Michal Novotny wrote:
if libvirt doesn't have the tokenizer support yet, it may be a good RFE as I believe it could be really useful ;-)
Peter, do you know about anything libvirt supports to tokenize string? We have virStringSplit for tokenizing strings which have a fixed separator.
Daniel
That sounds good, however what about splitting the function to 2 separate functions, one accepting the second parameter as the separator, called e.g. virStringSplitBy() and second just calling the first one with the fixed separator?
Wouldn't it be better?
I think the current state is more than sufficient: /** * virStringSplit: * @string: a string to split * @delim: a string which specifies the places at which to split * the string. The delimiter is not included in any of the resulting * strings, unless @max_tokens is reached. * @max_tokens: the maximum number of pieces to split @string into. * If this is 0, the string is split completely. * * Splits a string into a maximum of @max_tokens pieces, using the given * @delim. If @max_tokens is reached, the remainder of @string is * appended to the last token. * * As a special case, the result of splitting the empty string "" is an empty * vector, not a vector containing a single string. The reason for this * special case is that being able to represent a empty vector is typically * more useful than consistent handling of empty elements. If you do need * to represent empty elements, you'll need to check for the empty string * before calling virStringSplit(). * * Return value: a newly-allocated NULL-terminated array of strings. Use * virStringFreeList() to free it. */ char **virStringSplit(const char *string, const char *delim, size_t max_tokens)
Michal
On 06/28/2013 01:54 PM, Peter Krempa wrote:
On 06/28/13 13:50, Michal Novotny wrote:
On 06/28/2013 01:48 PM, Daniel P. Berrange wrote:
On Fri, Jun 28, 2013 at 01:43:44PM +0200, Michal Novotny wrote:
if libvirt doesn't have the tokenizer support yet, it may be a good RFE as I believe it could be really useful ;-)
Peter, do you know about anything libvirt supports to tokenize string? We have virStringSplit for tokenizing strings which have a fixed separator.
Daniel That sounds good, however what about splitting the function to 2 separate functions, one accepting the second parameter as the separator, called e.g. virStringSplitBy() and second just calling the first one with the fixed separator?
Wouldn't it be better? I think the current state is more than sufficient:
/** * virStringSplit: * @string: a string to split * @delim: a string which specifies the places at which to split * the string. The delimiter is not included in any of the resulting * strings, unless @max_tokens is reached. * @max_tokens: the maximum number of pieces to split @string into. * If this is 0, the string is split completely. * * Splits a string into a maximum of @max_tokens pieces, using the given * @delim. If @max_tokens is reached, the remainder of @string is * appended to the last token. * * As a special case, the result of splitting the empty string "" is an empty * vector, not a vector containing a single string. The reason for this * special case is that being able to represent a empty vector is typically * more useful than consistent handling of empty elements. If you do need * to represent empty elements, you'll need to check for the empty string * before calling virStringSplit(). * * Return value: a newly-allocated NULL-terminated array of strings. Use * virStringFreeList() to free it. */ char **virStringSplit(const char *string, const char *delim, size_t max_tokens)
Ah, then I misunderstood the "fixed separator" thing ;-) I was thinking the delimiter it fixed. This one looks good ;-) David, you could use virStringSplit() instead ;-) Michal -- Michal Novotny <minovotn@redhat.com>, RHCE, Red Hat Virtualization | libvirt-php bindings | php-virt-control.org
On Fri, Jun 28, 2013 at 1:55 PM, Michal Novotny <minovotn@redhat.com> wrote:
On 06/28/2013 01:54 PM, Peter Krempa wrote:
On 06/28/13 13:50, Michal Novotny wrote:
On 06/28/2013 01:48 PM, Daniel P. Berrange wrote:
On Fri, Jun 28, 2013 at 01:43:44PM +0200, Michal Novotny wrote:
if libvirt doesn't have the tokenizer support yet, it may be a good RFE as I believe it could be really useful ;-)
Peter, do you know about anything libvirt supports to tokenize string? We have virStringSplit for tokenizing strings which have a fixed separator.
Daniel That sounds good, however what about splitting the function to 2 separate functions, one accepting the second parameter as the separator, called e.g. virStringSplitBy() and second just calling the first one with the fixed separator?
Wouldn't it be better? I think the current state is more than sufficient:
/** * virStringSplit: * @string: a string to split * @delim: a string which specifies the places at which to split * the string. The delimiter is not included in any of the resulting * strings, unless @max_tokens is reached. * @max_tokens: the maximum number of pieces to split @string into. * If this is 0, the string is split completely. * * Splits a string into a maximum of @max_tokens pieces, using the given * @delim. If @max_tokens is reached, the remainder of @string is * appended to the last token. * * As a special case, the result of splitting the empty string "" is an empty * vector, not a vector containing a single string. The reason for this * special case is that being able to represent a empty vector is typically * more useful than consistent handling of empty elements. If you do need * to represent empty elements, you'll need to check for the empty string * before calling virStringSplit(). * * Return value: a newly-allocated NULL-terminated array of strings. Use * virStringFreeList() to free it. */ char **virStringSplit(const char *string, const char *delim, size_t max_tokens)
Ah, then I misunderstood the "fixed separator" thing ;-) I was thinking the delimiter it fixed. This one looks good ;-)
David, you could use virStringSplit() instead ;-)
I will wait for Peter's patch ;) regards, david
participants (4)
-
Daniel P. Berrange -
David Maciejak -
Michal Novotny -
Peter Krempa