Re: [PATCH] handle tls auth blobs correctly

[Beniamino Galvani <bgalvani redhat com>]

On Fri, Jan 22, 2016 at 12:50:49PM +0100, Matthias Berndt wrote:
> Hi Beniamino et al,
>
> networkmanager-openvpn doesn't currently handle <tls-auth> blobs correctly. I've modified 
> networkmanager-openvpn to support this functionality and fixed two minor bugs in the process. I'd be 
> thrilled if you could comment on the patch.

Hi Matthias,

overall looks good, only some suggestions.

> @@ -578,6 +600,10 @@ do_import (const char *path, const char *contents, GError **error)
>                       continue;
>               }
>  
> +             if (!strncmp(*line, KEY_DIRECTION_TAG, strlen (KEY_DIRECTION_TAG))) {
> +                     last_seen_key_direction = *line + strlen(KEY_DIRECTION_TAG);
> +             }

Braces are not required for single line blocks; add a space after last strlen;

> @@ -859,13 +885,16 @@ do_import (const char *path, const char *contents, GError **error)
>               if (handle_path_item (*line, KEY_TAG, NM_OPENVPN_KEY_KEY, s_vpn, default_path, NULL))
>                       continue;
>  
> -             if (handle_blob_item ((const char ***)&line, NM_OPENVPN_KEY_CA, s_vpn, basename, NULL))
> +             if (handle_blob_item ((const char ***)&line, NM_OPENVPN_KEY_CA, s_vpn, basename, NULL, 
> last_seen_key_direction))
> +                     continue;
> +
> +             if (handle_blob_item ((const char ***)&line, NM_OPENVPN_KEY_CERT, s_vpn, basename, NULL, 
> last_seen_key_direction))
>                       continue;
>  
> -             if (handle_blob_item ((const char ***)&line, NM_OPENVPN_KEY_CERT, s_vpn, basename, NULL))
> +             if (handle_blob_item ((const char ***)&line, NM_OPENVPN_KEY_KEY, s_vpn, basename, NULL, 
> last_seen_key_direction))
>                       continue;
>  
> -             if (handle_blob_item ((const char ***)&line, NM_OPENVPN_KEY_KEY, s_vpn, basename, NULL))
> +             if (handle_blob_item ((const char ***)&line, NM_OPENVPN_KEY_TA, s_vpn, basename, NULL, 
> last_seen_key_direction))
>                       continue;

Instead of handling the key direction inside handle_blob_item() maybe
it would be cleaner to do:

  if (handle_blob_item ((const char ***)&line, NM_OPENVPN_KEY_TA, s_vpn, basename, NULL) {
      handle_direction("tls-auth",
                       NM_OPENVPN_KEY_TA_DIR,
                       last_seen_key_direction,
                       s_vpn);
      continue;
  }

Also, as Thomas suggested, the two fixes should be in a separate patch.

Thanks!
Beniamino

Attachment: signature.asc
Description: PGP signature