[libvirt] [PATCH v2] esx: do not store escaped password in esxVI_Context.

[Michal Privoznik]

On 26.05.2016 17:30, Dawid Zamirski wrote:
> This patch fixes an issue where screenshot API call was failing when
> the esx/vcenter password contains special characters such as
> apostrophee. The reason for failures was that passwords were escaped
> for XML and stored in esxVI_Context which was then passed to raw CURL
> API calls where the password must be passed in original form to
> authenticate successfully. So this patch addresses this by storing
> original passwords in the esxVI_Context struct and escape only for
> esxVI_Login call.
> ---
> 
> Changes made since v1:
>  * Also patch esxVI_EnsureSession
>  * Added cleanup to esxVI_Context_Connect to make sure escapedPassword
>    is freed where appropiate. Also move the code block that escapes the
>    password to happen earlier in the function body so that it can bail
>    earlier in the event of failure and skip the rest of the processing.
>  * Updated virReportError calls to pass syntax-check
> 
>  src/esx/esx_driver.c | 22 ++++---------------
>  src/esx/esx_vi.c     | 62 ++++++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 50 insertions(+), 34 deletions(-)
> 
> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
> index 00d0e0a..031c666 100644
> --- a/src/esx/esx_driver.c
> +++ b/src/esx/esx_driver.c
> @@ -617,7 +617,6 @@ esxConnectToHost(esxPrivate *priv,
>      int result = -1;
>      char ipAddress[NI_MAXHOST] = "";
>      char *username = NULL;
> -    char *unescapedPassword = NULL;
>      char *password = NULL;
>      char *url = NULL;
>      esxVI_String *propertyNameList = NULL;
> @@ -647,18 +646,13 @@ esxConnectToHost(esxPrivate *priv,
>          }
>      }
>  
> -    unescapedPassword = virAuthGetPassword(conn, auth, "esx", username, conn->uri->server);
> +    password = virAuthGetPassword(conn, auth, "esx", username, conn->uri->server);
>  
> -    if (!unescapedPassword) {
> +    if (!password) {
>          virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed"));
>          goto cleanup;
>      }
>  
> -    password = esxUtil_EscapeForXml(unescapedPassword);
> -
> -    if (!password)
> -        goto cleanup;
> -
>      if (virAsprintf(&url, "%s://%s:%d/sdk", priv->parsedUri->transport,
>                      conn->uri->server, conn->uri->port) < 0)
>          goto cleanup;
> @@ -705,7 +699,6 @@ esxConnectToHost(esxPrivate *priv,
>  
>   cleanup:
>      VIR_FREE(username);
> -    VIR_FREE(unescapedPassword);
>      VIR_FREE(password);
>      VIR_FREE(url);
>      esxVI_String_Free(&propertyNameList);
> @@ -726,7 +719,6 @@ esxConnectToVCenter(esxPrivate *priv,
>      int result = -1;
>      char ipAddress[NI_MAXHOST] = "";
>      char *username = NULL;
> -    char *unescapedPassword = NULL;
>      char *password = NULL;
>      char *url = NULL;
>  
> @@ -752,18 +744,13 @@ esxConnectToVCenter(esxPrivate *priv,
>          }
>      }
>  
> -    unescapedPassword = virAuthGetPassword(conn, auth, "esx", username, hostname);
> +    password = virAuthGetPassword(conn, auth, "esx", username, hostname);
>  
> -    if (!unescapedPassword) {
> +    if (!password) {
>          virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed"));
>          goto cleanup;
>      }
>  
> -    password = esxUtil_EscapeForXml(unescapedPassword);
> -
> -    if (!password)
> -        goto cleanup;
> -
>      if (virAsprintf(&url, "%s://%s:%d/sdk", priv->parsedUri->transport,
>                      hostname, conn->uri->port) < 0)
>          goto cleanup;
> @@ -799,7 +786,6 @@ esxConnectToVCenter(esxPrivate *priv,
>  
>   cleanup:
>      VIR_FREE(username);
> -    VIR_FREE(unescapedPassword);
>      VIR_FREE(password);
>      VIR_FREE(url);
>  
> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> index 6520196..5fb2279 100644
> --- a/src/esx/esx_vi.c
> +++ b/src/esx/esx_vi.c
> @@ -996,39 +996,52 @@ esxVI_Context_Connect(esxVI_Context *ctx, const char *url,
>                        const char *ipAddress, const char *username,
>                        const char *password, esxUtil_ParsedUri *parsedUri)
>  {
> +    int result = -1;
> +    char *escapedPassword = NULL;
> +
>      if (!ctx || !url || !ipAddress || !username ||
>          !password || ctx->url || ctx->service || ctx->curl) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
>          return -1;
>      }
>  
> +    escapedPassword = esxUtil_EscapeForXml(password);
> +
> +    if (!escapedPassword) {
> +        VIR_FREE(escapedPassword);

Well, if escapedPassword is NULL, there's no need to free it.

> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Failed to escape password for XML"));
> +        goto cleanup;
> +    }
> +

Otherwise looking good. ACKed and pushed.

Michal