Re: [PATCH] handle tls auth blobs correctly

[Beniamino Galvani]

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

signature.asc
Description: PGP signature

_______________________________________________
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list