Re: [Openvpn-devel] OpenVPN Pf plugin/small status patch

2010-03-01 Thread David Sommerseth 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 01/03/10 12:03, Arne Schwabe wrote:
>  On 01.03.2010 11:16, David Sommerseth wrote:
>> -BEGIN PGP SIGNED MESSAGE-
>> Hash: SHA1
>>
>> On 28/02/10 15:56, Arne Schwabe wrote:
>>>   On 28.02.2010 14:22, David Sommerseth wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1

 On 26/06/09 17:00, Arne Schwabe wrote:
> Hi,
>
> I have written a simple plugin for packet filtering that looks up fw
> rules
> in the order
>
> Commonname.pf
> IP_Port.pf
> IP.pf
> default.pf
>
> If one of this files is found the file is used as PF configuration.
> Maybe
> this plugin is useful for someone else.
 Hi!

 Thank you for your patches.  I've been looking at both patches, and I
 have some questions in regards to this plug-in.

 How does this packet filtering further work?  I do not completely
 understand how you imagine this should work.  I see that it tries to
 open a number of files with different filename criteria , and if it
 finds a file it copies it somewhere.

>>> The packet filtering itself is already part of openvpn. It only works in
>>> tap mode iirc. See
>>> http://svn.openvpn.net/projects/openvpn/branches/BETA21/openvpn/pf.c. A
>>> description of the packet filter format is given in
>>> http://svn.openvpn.net/projects/openvpn/branches/BETA21/openvpn/management/management-notes.txt
>>>
>>>
>>> COMMAND -- client-pf  (OpenVPN 2.1 or higher)
>>>
>>> This plugin/patch only add the possibility to use the packet filter
>>> functionality without use of the management interface.
>> Thank you for your pointers.  I've been reading the docs and looking at
>> the code.  I'm a bit more informed now.  But there are still some magic
>> here which I don't understand.  How does copying a file enable the
>> packet filtering itself?  Granted the man page and management-notes.txt
>> might be a bit too vague here.
>>
>> man page:
>> -
>> 
>> --management-client-pf
>> Management interface clients must specify a packet
>> filter file for each connecting client.  See
>> management-notes.txt in OpenVPN distribution for
>> detailed notes.
>> -
>> 
>>
>>
>> management-notes.txt:
>> -
>> 
>> COMMAND -- client-pf  (OpenVPN 2.1 or higher)
>> - -
>>
>> Push a packet filter file to a specific client.
>>
>> The OpenVPN server should have been started with the
>> - --management-client-pf directive so that it will require that
>> VPN tunnel packets sent or received by client instances must
>> conform to that client's packet filter configuration.
>> -
>> 
>>
>> In the docs, "packet filter file" is mentioned, but the docs does a poor
>> job describing all the parts of the feature - which in fact might be
>> /my/ main problem.  It is not described the purpose of this file, except
>> what kind of contents you might find in it and how to understand that.
>> Further, I'm not sure if this should be run on the server or client
>> side, or if it can be used on both sides.  Is this something the server
>> can push to the clients?  It's many loose threads here, which confuses
>> me a little bit.
> As far as I recall correctly the packet filtering code runs *only* on
> the server if the server is in a) multi client mode and b) tap mode. You
> basically can restrict the addresses the clients can reach on a client
> basis. I needed some basic clients are allowd to access internal IP a
> but not b mechanism and the pf code of openvpn was good enough for me.
> But for the simple I did not want to keep another daemon around which
> waits for connecting client and then sends the pf rules so I wrote the
> plugin. That way I could have a default.pf
> 
>> Arne, you patch seems to play inside the defined playground you have
>> available, so I'm not criticising your plug-in here now.  But I need to
>> be able to understand the magic happening here to give your plug-in a
>> fair review.
>>
> Quite understandable.
>> Having that said, the whole packet filtering implementation in OpenVPN,
>> having very good intentions indeed, seems to be rather "hackerish".
>> Just to save the rules in a temporary file (which it looks like it does,
>> according to pf.c:497) seems odd and so un-logic. But that's not your
>> responsibility, Arne :)
>>
>> But if you can please try to enlighten me further, I would appreciate
>> that.  After all, you have a plug-in which solves an issue for you - and
>> I don't want to block your plug-in for inclusion as long as it is
>> considered useful.
> 
> Well the plugin is given a pointer to the temporary file name. If you

Re: [Openvpn-devel] OpenVPN Pf plugin/small status patch

2010-03-01 Thread Arne Schwabe 

 On 01.03.2010 11:16, David Sommerseth wrote:

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 28/02/10 15:56, Arne Schwabe wrote:

  On 28.02.2010 14:22, David Sommerseth wrote:

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 26/06/09 17:00, Arne Schwabe wrote:

Hi,

I have written a simple plugin for packet filtering that looks up fw
rules
in the order

Commonname.pf
IP_Port.pf
IP.pf
default.pf

If one of this files is found the file is used as PF configuration.
Maybe
this plugin is useful for someone else.

Hi!

Thank you for your patches.  I've been looking at both patches, and I
have some questions in regards to this plug-in.

How does this packet filtering further work?  I do not completely
understand how you imagine this should work.  I see that it tries to
open a number of files with different filename criteria , and if it
finds a file it copies it somewhere.


The packet filtering itself is already part of openvpn. It only works in
tap mode iirc. See
http://svn.openvpn.net/projects/openvpn/branches/BETA21/openvpn/pf.c. A
description of the packet filter format is given in
http://svn.openvpn.net/projects/openvpn/branches/BETA21/openvpn/management/management-notes.txt

COMMAND -- client-pf  (OpenVPN 2.1 or higher)

This plugin/patch only add the possibility to use the packet filter
functionality without use of the management interface.

Thank you for your pointers.  I've been reading the docs and looking at
the code.  I'm a bit more informed now.  But there are still some magic
here which I don't understand.  How does copying a file enable the
packet filtering itself?  Granted the man page and management-notes.txt
might be a bit too vague here.

man page:
- 
--management-client-pf
Management interface clients must specify a packet
filter file for each connecting client.  See
management-notes.txt in OpenVPN distribution for
detailed notes.
- 


management-notes.txt:
- 
COMMAND -- client-pf  (OpenVPN 2.1 or higher)
- -

Push a packet filter file to a specific client.

The OpenVPN server should have been started with the
- --management-client-pf directive so that it will require that
VPN tunnel packets sent or received by client instances must
conform to that client's packet filter configuration.
- 

In the docs, "packet filter file" is mentioned, but the docs does a poor
job describing all the parts of the feature - which in fact might be
/my/ main problem.  It is not described the purpose of this file, except
what kind of contents you might find in it and how to understand that.
Further, I'm not sure if this should be run on the server or client
side, or if it can be used on both sides.  Is this something the server
can push to the clients?  It's many loose threads here, which confuses
me a little bit.
As far as I recall correctly the packet filtering code runs *only* on 
the server if the server is in a) multi client mode and b) tap mode. You 
basically can restrict the addresses the clients can reach on a client 
basis. I needed some basic clients are allowd to access internal IP a 
but not b mechanism and the pf code of openvpn was good enough for me. 
But for the simple I did not want to keep another daemon around which 
waits for connecting client and then sends the pf rules so I wrote the 
plugin. That way I could have a default.pf



Arne, you patch seems to play inside the defined playground you have
available, so I'm not criticising your plug-in here now.  But I need to
be able to understand the magic happening here to give your plug-in a
fair review.


Quite understandable.

Having that said, the whole packet filtering implementation in OpenVPN,
having very good intentions indeed, seems to be rather "hackerish".
Just to save the rules in a temporary file (which it looks like it does,
according to pf.c:497) seems odd and so un-logic. But that's not your
responsibility, Arne :)

But if you can please try to enlighten me further, I would appreciate
that.  After all, you have a plug-in which solves an issue for you - and
I don't want to block your plug-in for inclusion as long as it is
considered useful.


Well the plugin is given a pointer to the temporary file name. If you 
copy a ruleset to that temporary file the openvpn pf filter code picks 
it up. I also think that this api is not the best around but at the 
Moment it is the one a plugin could use. When I wrote the patch, it was 
the least intrusive method to get the pf code working.


Arne

Re: [Openvpn-devel] OpenVPN Pf plugin/small status patch

2010-03-01 Thread David Sommerseth 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 01/03/10 04:52, Karl O. Pinc wrote:
>>> If one of this files is found the file is used as PF configuration.
>> > Maybe
>>> > > this plugin is useful for someone else.
>> > 
>> > Hi!
>> > 
>> > Thank you for your patches.  I've been looking at both patches, and I
>> > have some questions in regards to this plug-in.
>>
> I think that a vocabulary change could help avoid confusion, depending
> on exactly what the proposed vocabulary is here.  "pf" is the offical
> name for the OpenBSD packet filter.  IIRC it is also ported to FreeBSD
> and perhaps elsewhere.  Using the name "pf" for this plugin could
> be confusing.

Even though I do agree with you, Karl, that the vocabulary can be
confusing, I'm not sure it is up to us to change that.  OpenVPN has
implemented a packet filtering feature which placed in pf.[ch] and seems
to use the abbreviation PF several places in the code.

We may dislike it or not, but we probably can't change that now.  That
this plug-in uses pf in OpenVPN context is in my eyes correct, as it
corresponds to the OpenVPN implementation.  Having that said, it could
be better described in some comments that this plug-in is using
OpenVPN's packet filter implementation.

Anyhow, this topic do deserve a little discussion on the
#openvpn-discussion meeting on Thursdays@18:00 UTC with James.


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkuLisAACgkQDC186MBRfrouHwCfaLLg9QCtlEEN8CYOJdtQVm+8
qeUAoJUvSKh1XM61khqHFZ55V9kvFMFg
=jtqZ
-END PGP SIGNATURE-

Re: [Openvpn-devel] OpenVPN Pf plugin/small status patch

2010-03-01 Thread Karl O. Pinc 
On 02/28/2010 07:22:16 AM, David Sommerseth wrote:
> On 26/06/09 17:00, Arne Schwabe wrote:
> > Hi,
> >
> > I have written a simple plugin for packet filtering that looks up 
> fw
> rules
> > in the order
> >
> > Commonname.pf
> > IP_Port.pf
> > IP.pf
> > default.pf
> >
> > If one of this files is found the file is used as PF configuration.
> Maybe
> > this plugin is useful for someone else.
> 
> Hi!
> 
> Thank you for your patches.  I've been looking at both patches, and I
> have some questions in regards to this plug-in.

I think that a vocabulary change could help avoid confusion, depending
on exactly what the proposed vocabulary is here.  "pf" is the offical
name for the OpenBSD packet filter.  IIRC it is also ported to FreeBSD
and perhaps elsewhere.  Using the name "pf" for this plugin could
be confusing.


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein

Re: [Openvpn-devel] OpenVPN Pf plugin/small status patch

2010-03-01 Thread Arne Schwabe 

 On 28.02.2010 14:22, David Sommerseth wrote:

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 26/06/09 17:00, Arne Schwabe wrote:

Hi,

I have written a simple plugin for packet filtering that looks up fw rules
in the order

Commonname.pf
IP_Port.pf
IP.pf
default.pf

If one of this files is found the file is used as PF configuration. Maybe
this plugin is useful for someone else.

Hi!

Thank you for your patches.  I've been looking at both patches, and I
have some questions in regards to this plug-in.

How does this packet filtering further work?  I do not completely
understand how you imagine this should work.  I see that it tries to
open a number of files with different filename criteria , and if it
finds a file it copies it somewhere.

The packet filtering itself is already part of openvpn. It only works in 
tap mode iirc. See 
http://svn.openvpn.net/projects/openvpn/branches/BETA21/openvpn/pf.c. A 
description of the packet filter format is given in 
http://svn.openvpn.net/projects/openvpn/branches/BETA21/openvpn/management/management-notes.txt

COMMAND -- client-pf  (OpenVPN 2.1 or higher)

This plugin/patch only add the possibility to use the packet filter 
functionality without use of the management interface.

Re: [Openvpn-devel] OpenVPN Pf plugin/small status patch

2010-02-28 Thread David Sommerseth 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 26/06/09 17:00, Arne Schwabe wrote:
> Hi,
> 
> I have written a simple plugin for packet filtering that looks up fw rules
> in the order
> 
> Commonname.pf
> IP_Port.pf
> IP.pf
> default.pf
> 
> If one of this files is found the file is used as PF configuration. Maybe
> this plugin is useful for someone else.

Hi!

Thank you for your patches.  I've been looking at both patches, and I
have some questions in regards to this plug-in.

How does this packet filtering further work?  I do not completely
understand how you imagine this should work.  I see that it tries to
open a number of files with different filename criteria , and if it
finds a file it copies it somewhere.

But how does that itself really provide packet filtering?

The code itself is not bad, I'd like to have it cleaned up a little bit
before inclusion.  Things I'm picking on are consequent spacing,
removing unneeded blank lines, improved comments (explaining *what* is
happening).

In addition the copypffile() function can be written a bit more clever,
avoiding using stack space for an array of file names.  As you already
have a for() loop, it would be better, IMHO, to build up the filename
string in this loop, using a simple select() for the filename template.

You also have a potential buffer overflow bug with your snprintf().  If
common name (the most likely one to attack) is longer than 256 bytes,
the filename will consist of those 256 bytes without a NULL terminator,
as snprintf() expect the NULL terminator to be included in the
size/length provided.

I'd recommend something like:

   char str[258];  // 256 bytes + 2 bytes extra

   memset(, 0, 258);  // Make sure it's NULL
   snprintf(str, 256, "%.253s.pf%c", cn, '\0');

or:

   char *str = calloc(1, 258); // 256 + 2 extra
   snprintf(str, 256, "%.253s.pf%c", cn, '\0');

   free(str);

This way str will never exceed 256 bytes, including the extension, and
it will be NULL terminated for sure.  In addition you are safer against
buffer overflows.  If using calloc(), the memory can be expected to be
zeroed as well (one of the advantages of calloc() compared to malloc()).

Using the str pointer above in a for loop, you can just call snprintf()
several times to change the contents, and free it after the for() loop.

I would also recommend to be stricter to the allowed string length when
processing IP addresses, port numbers and device name.  F.ex. the
filename template for port number could be: "%.5s.pf", as a port number
cannot be bigger than 65535, which is 5 digits.

And the last important thing, you have no error checking if copypffile()
fails or not.  OpenVPN will believe that this function call will always
work.  Granted, you if no files exist to be copied that might not be a
failure at all.  But it could be a failure if it could not write to the
destination file when it does the copy.

You also have two functions which might not be needed as well,
openvpn_plugin_client_contstructor_v1() and
openvpn_plugin_client_destructor_v1().  They don't seem to do anything
useful and OpenVPN don't require them, AFAIK.

The printf() messages could also specify the plug-in name instead of
just generic function names ... especially important for users who use
several plug-ins in parallel.


> Add I would like to ask to include the patch for multi.c.
> 
> There are commands in the management interface which require the cid. The
> only way at the moment to get the cid of connected clients is to have
> always a management connection established. The patch adds the CID to the
> status output.

This patch is sent for review by more developers.  It will need an
official ACK before inclusion, so I hope that will come soon.

Thank you once again for your contributions!


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkuKbggACgkQDC186MBRfrrG6ACfWIuNyCPpufx1DwRmv2RGCqug
8TEAnjRy8EPYVfrlCXw6xdSNg0UPaJl2
=oaxR
-END PGP SIGNATURE-