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

[Nicolas George]

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 at 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.
>  
> + at 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.
> +
> + at 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,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200714/cf284d9a/attachment.sig>