[FFmpeg-devel] avformat/libssh: add AVOptions for authentication

This introduces two new AVOption options for the SFTP protocol,
one named sftp_user to supply the username to be used for auth,
one named sftp_password to supply the password to be used for auth.

These are useful for when an API user does not wish to deal with
URL manipulation and percent encoding.

Setting them while also having credentials in the URL will use the
credentials from the URL. The rationale for this is that credentials
embedded in the URL are probably more specific to what the user is
trying to do than anything set by some API user.

Signed-off-by: Nicolas Frattaroli <ffmpeg@fratti.ch>
---
 doc/protocols.texi   |  8 ++++++++
 libavformat/libssh.c | 17 +++++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

Thanks for the patch. I thnink it is a good idea. See preliminary
comments below.

I do not know if Lukasz, who wrote this file, still reads the list. If
he does not appear in the next few days, try Ccing him.


Nicolas Frattaroli (12020-07-14):
> This introduces two new AVOption options for the SFTP protocol,
> one named sftp_user to supply the username to be used for auth,
> one named sftp_password to supply the password to be used for auth.

Since the options are specific to the protocol, there is probably no
need to namespace them.

> These are useful for when an API user does not wish to deal with
> URL manipulation and percent encoding.
> 
> Setting them while also having credentials in the URL will use the
> credentials from the URL. The rationale for this is that credentials
> embedded in the URL are probably more specific to what the user is
> trying to do than anything set by some API user.

I am not really comfortable with that. I think returning an error for
conflicting options would be best.

> 
> Signed-off-by: Nicolas Frattaroli <ffmpeg@fratti.ch>
> ---
>  doc/protocols.texi   |  8 ++++++++
>  libavformat/libssh.c | 17 +++++++++++++----
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/protocols.texi b/doc/protocols.texi
> index 64ad3f05d6..40c8c572e5 100644
> --- a/doc/protocols.texi
> +++ b/doc/protocols.texi
> @@ -880,6 +880,14 @@ truncating. Default value is 1.
>  Specify the path of the file containing private key to use during authorization.
>  By default libssh searches for keys in the @file{~/.ssh/} directory.
>  
> +@item sftp_user
> +Set a user to be used for authenticating to the SSH daemon. This is overridden by the
> +user in the SFTP URL.
> +
> +@item sftp_password
> +Set a password to be used for authenticating to the SSH daemon. This is overridden by
> +the password in the SFTP URL.
> +
>  @end table
>  
>  Example: Play a file stored on remote server.
> diff --git a/libavformat/libssh.c b/libavformat/libssh.c
> index 21474f0f0a..a673e49a3a 100644
> --- a/libavformat/libssh.c
> +++ b/libavformat/libssh.c
> @@ -39,6 +39,8 @@ typedef struct {
>      int rw_timeout;
>      int trunc;
>      char *priv_key;
> +    const char *option_user;
> +    const char *option_password;
>  } LIBSSHContext;
>  
>  static av_cold int libssh_create_ssh_session(LIBSSHContext *libssh, const char* hostname, unsigned int port)
> @@ -192,13 +194,13 @@ static av_cold int libssh_close(URLContext *h)
>  static av_cold int libssh_connect(URLContext *h, const char *url, char *path, size_t path_size)
>  {
>      LIBSSHContext *libssh = h->priv_data;

> -    char proto[10], hostname[1024], credencials[1024];
> +    char proto[10], hostname[1024], credentials[1024];

Usually, we require this kind of change to be in a separate patch. But
for such a minimal change I think we could dispense.

>      int port = 22, ret;
>      const char *user = NULL, *pass = NULL;
>      char *end = NULL;
>  
>      av_url_split(proto, sizeof(proto),
> -                 credencials, sizeof(credencials),
> +                 credentials, sizeof(credentials),
>                   hostname, sizeof(hostname),
>                   &port,
>                   path, path_size,
> @@ -214,8 +216,13 @@ static av_cold int libssh_connect(URLContext *h, const char *url, char *path, si
>      if ((ret = libssh_create_ssh_session(libssh, hostname, port)) < 0)
>          return ret;
>  
> -    user = av_strtok(credencials, ":", &end);
> -    pass = av_strtok(end, ":", &end);

> +    if(!*credentials) {

Nit: spaces between if and (.

> +        user = libssh->option_user;
> +        pass = libssh->option_password;
> +    } else {
> +        user = av_strtok(credentials, ":", &end);
> +        pass = av_strtok(end, ":", &end);
> +    }
>  
>      if ((ret = libssh_authentication(libssh, user, pass)) < 0)
>          return ret;
> @@ -479,6 +486,8 @@ static const AVOption options[] = {
>      {"timeout", "set timeout of socket I/O operations", OFFSET(rw_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, -1, INT_MAX, D|E },
>      {"truncate", "Truncate existing files on write", OFFSET(trunc), AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, E },
>      {"private_key", "set path to private key", OFFSET(priv_key), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D|E },
> +    {"sftp_user", "user for SFTP login. Overridden by whatever is in the URL.", OFFSET(option_user), AV_OPT_TYPE_STRING, { 0 }, 0, 0, D|E },
> +    {"sftp_password", "password for SFTP login. Overridden by whatever is in the URL.", OFFSET(option_password), AV_OPT_TYPE_STRING, { 0 }, 0, 0, D|E },
>      {NULL}
>  };
>  

Regards,

On 7/14/2020 3:06 PM, Nicolas Frattaroli wrote:
> This introduces two new AVOption options for the SFTP protocol,
> one named sftp_user to supply the username to be used for auth,
> one named sftp_password to supply the password to be used for auth.
> 
> These are useful for when an API user does not wish to deal with
> URL manipulation and percent encoding.
> 
> Setting them while also having credentials in the URL will use the
> credentials from the URL. The rationale for this is that credentials
> embedded in the URL are probably more specific to what the user is
> trying to do than anything set by some API user.
> 
> Signed-off-by: Nicolas Frattaroli <ffmpeg@fratti.ch>
> ---
>  doc/protocols.texi   |  8 ++++++++
>  libavformat/libssh.c | 17 +++++++++++++----
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/protocols.texi b/doc/protocols.texi
> index 64ad3f05d6..40c8c572e5 100644
> --- a/doc/protocols.texi
> +++ b/doc/protocols.texi
> @@ -880,6 +880,14 @@ truncating. Default value is 1.
>  Specify the path of the file containing private key to use during authorization.
>  By default libssh searches for keys in the @file{~/.ssh/} directory.
>  
> +@item sftp_user
> +Set a user to be used for authenticating to the SSH daemon. This is overridden by the
> +user in the SFTP URL.
> +
> +@item sftp_password
> +Set a password to be used for authenticating to the SSH daemon. This is overridden by
> +the password in the SFTP URL.
> +
>  @end table
>  
>  Example: Play a file stored on remote server.
> diff --git a/libavformat/libssh.c b/libavformat/libssh.c
> index 21474f0f0a..a673e49a3a 100644
> --- a/libavformat/libssh.c
> +++ b/libavformat/libssh.c
> @@ -39,6 +39,8 @@ typedef struct {
>      int rw_timeout;
>      int trunc;
>      char *priv_key;
> +    const char *option_user;
> +    const char *option_password;
>  } LIBSSHContext;
>  
>  static av_cold int libssh_create_ssh_session(LIBSSHContext *libssh, const char* hostname, unsigned int port)
> @@ -192,13 +194,13 @@ static av_cold int libssh_close(URLContext *h)
>  static av_cold int libssh_connect(URLContext *h, const char *url, char *path, size_t path_size)
>  {
>      LIBSSHContext *libssh = h->priv_data;
> -    char proto[10], hostname[1024], credencials[1024];
> +    char proto[10], hostname[1024], credentials[1024];
>      int port = 22, ret;
>      const char *user = NULL, *pass = NULL;
>      char *end = NULL;
>  
>      av_url_split(proto, sizeof(proto),
> -                 credencials, sizeof(credencials),
> +                 credentials, sizeof(credentials),

Cosmetic changes should be in a separate commit.

>                   hostname, sizeof(hostname),
>                   &port,
>                   path, path_size,
> @@ -214,8 +216,13 @@ static av_cold int libssh_connect(URLContext *h, const char *url, char *path, si
>      if ((ret = libssh_create_ssh_session(libssh, hostname, port)) < 0)
>          return ret;
>  
> -    user = av_strtok(credencials, ":", &end);
> -    pass = av_strtok(end, ":", &end);
> +    if(!*credentials) {
> +        user = libssh->option_user;
> +        pass = libssh->option_password;
> +    } else {
> +        user = av_strtok(credentials, ":", &end);
> +        pass = av_strtok(end, ":", &end);
> +    }
>  
>      if ((ret = libssh_authentication(libssh, user, pass)) < 0)
>          return ret;
> @@ -479,6 +486,8 @@ static const AVOption options[] = {
>      {"timeout", "set timeout of socket I/O operations", OFFSET(rw_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, -1, INT_MAX, D|E },
>      {"truncate", "Truncate existing files on write", OFFSET(trunc), AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, E },
>      {"private_key", "set path to private key", OFFSET(priv_key), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D|E },
> +    {"sftp_user", "user for SFTP login. Overridden by whatever is in the URL.", OFFSET(option_user), AV_OPT_TYPE_STRING, { 0 }, 0, 0, D|E },
> +    {"sftp_password", "password for SFTP login. Overridden by whatever is in the URL.", OFFSET(option_password), AV_OPT_TYPE_STRING, { 0 }, 0, 0, D|E },

Perhaps this should instead be implemented as an AV_OPT_TYPE_DICT option
that can accept anything you would otherwise pass as part of the url, so
user, pass, port, etc.

>      {NULL}
>  };
>  
>

James Almer (12020-07-14):
> Perhaps this should instead be implemented as an AV_OPT_TYPE_DICT option
> that can accept anything you would otherwise pass as part of the url, so
> user, pass, port, etc.

It makes things more complicated for us (accessing a dictionary instead
of options directly), for API users (setting the dictionary plus the
option itself) and for command-line users (extra level of escaping).

Unless we expect the list of things obtained from the URL to grow in the
future, which is unlikely to say the least, I see no benefit compared to
just adding all this at once.

Am I missing something? What benefit would you expect?

On the other hand, since a lot of protocols do some kind of
av_url_split(), it makes sense to factor it.

Why not add all these fields to URLContext and have ffurl_connect()
split the URL, using options if they are available?

Regards,

Actually, forget this patch. It was modeled after the previous AVOption patch
I sent for the ftp protocol, but a better more generic approach would be to
have some sort of callback for API users so they can just implement .netrc.

Like this, with just AVOption for usernames and passwords, we can't really
implement it for HTTP due to a request leading to redirects which can then
change the domain to something else.