date:20170914

[Openvpn-devel] [PATCH] Check whether in pull_mode before warning about previous connection blocks

2017-09-14 Thread selva . nair

From: Selva Nair 

Eliminate the confusing message that says "explicit-exit-notify is ignored by
previous  blocks" when the option is pushed.
Reported by: Eike Lohmann e.lohm...@ic3s.de
https://www.mail-archive.com/openvpn-users@lists.sourceforge.net/msg04052.html

Signed-off-by: Selva Nair 
---
 src/openvpn/options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 3a5bccf..d302bd4 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6110,7 +6110,7 @@ add_option(struct options *options,
 #ifdef ENABLE_OCC
 else if (streq(p[0], "explicit-exit-notify") && !p[2])
 {
-
VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_CONNECTION|OPT_P_EXPLICIT_NOTIFY);
+VERIFY_PERMISSION(OPT_P_GENERAL|(pull_mode? 
0:OPT_P_CONNECTION)|OPT_P_EXPLICIT_NOTIFY);
 if (p[1])
 {
 options->ce.explicit_exit_notification = positive_atoi(p[1]);
-- 
2.6.2


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 2/2] pf: clean up temporary files if plugin init fails

2017-09-14 Thread Antonio Quartulli

Hi,

On 15/09/17 05:34, Steffan Karger wrote:
> close_instance() tries to remove the file in c2.pf.filename, but that only
> works if we actually set that if we fail.  So, set that filename as soon
> as we know we've created the file.
> 
> Signed-off-by: Steffan Karger 
> ---
>  src/openvpn/pf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
> index 5fe1734..3faaebd 100644
> --- a/src/openvpn/pf.c
> +++ b/src/openvpn/pf.c
> @@ -625,12 +625,12 @@ pf_init_context(struct context *c)
>  const char *pf_file = create_temp_file(c->options.tmp_dir, "pf", 
> );

what prevents us from calling create_temp_file() directly with
c->c2.pf.filename and >c2.gc as arguments?

Cheers,

>  if (pf_file)
>  {
> +c->c2.pf.filename = string_alloc(pf_file, >c2.gc);
>  setenv_str(c->c2.es, "pf_file", pf_file);
>  
>  if (plugin_call(c->plugins, OPENVPN_PLUGIN_ENABLE_PF, NULL, 
> NULL, c->c2.es) == OPENVPN_PLUGIN_FUNC_SUCCESS)
>  {
>  event_timeout_init(>c2.pf.reload, 1, now);
> -c->c2.pf.filename = string_alloc(pf_file, >c2.gc);
>  c->c2.pf.enabled = true;
>  #ifdef ENABLE_DEBUG
>  if (check_debug_level(D_PF_DEBUG))
> 

-- 
Antonio Quartulli



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 1/2] Don't throw fatal errors from create_temp_file()

2017-09-14 Thread Antonio Quartulli

I just did a quick skim for now and I only have a codestyle comment below:

On 15/09/17 05:34, Steffan Karger wrote:
> This function is called in response to connecting clients, and can fail
> when I/O fails for some (possibly temporary) reason.  In such cases we
> should not exit the process, but just reject the connecting client.
> 
> This commit changes the function to actually return NULL on errors, and
> (where needed) changes the callers to check for and handle errors.
> 
> Note that this changes the behavior for pf plugins: instead of just not
> initializing the firewall rules and happily continuing, this now rejects
> the client in the case of an (unlikely) failure to initialize the pf.
> 
> Since the tls-crypt-v2 metadata code also calls create_temp_file() when
> clients connect, I consider this a prerequisite for tls-crypt-v2.
> 
> Signed-off-by: Steffan Karger 
> ---
>  src/openvpn/misc.c   |  6 +++---
>  src/openvpn/pf.c |  8 
>  src/openvpn/ssl_verify.c | 32 +---
>  3 files changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 8c7f611..25f3800 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -740,7 +740,7 @@ create_temp_file(const char *directory, const char 
> *prefix, struct gc_arena *gc)
>  retfname = gen_path(directory, BSTR(), gc);
>  if (!retfname)
>  {
> -msg(M_FATAL, "Failed to create temporary filename and path");
> +msg(M_WARN, "Failed to create temporary filename and path");
>  return NULL;
>  }
>  
> @@ -755,14 +755,14 @@ create_temp_file(const char *directory, const char 
> *prefix, struct gc_arena *gc)
>  else if (fd == -1 && errno != EEXIST)
>  {
>  /* Something else went wrong, no need to retry.  */
> -msg(M_FATAL | M_ERRNO, "Could not create temporary file '%s'",
> +msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
>  retfname);
>  return NULL;
>  }
>  }
>  while (attempts < 6);
>  
> -msg(M_FATAL, "Failed to create temporary file after %i attempts", 
> attempts);
> +msg(M_WARN, "Failed to create temporary file after %i attempts", 
> attempts);
>  return NULL;
>  }
>  
> diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
> index 5cb002b..5fe1734 100644
> --- a/src/openvpn/pf.c
> +++ b/src/openvpn/pf.c
> @@ -639,10 +639,10 @@ pf_init_context(struct context *c)
>  }
>  #endif
>  }
> -else
> -{
> -msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
> -}
> +}
> +if (!c->c2.pf.enabled)
> +{
> +register_signal(c, SIGUSR1, "plugin-pf-init-failed");
>  }
>  }
>  #endif /* ifdef PLUGIN_PF */
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index 9cd36d7..df2736c 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -547,14 +547,14 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, 
> const char *tmp_dir, stru
>  FILE *peercert_file;
>  const char *peercert_filename = "";
>  
> -if (!tmp_dir)
> +/* create tmp file to store peer cert */
> +if (!tmp_dir ||
> +!(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))


The '||' should go at the beginning of the second line.



>  {
> +msg (M_WARN, "Failed to create peer cert file");
>  return NULL;
>  }
>  
> -/* create tmp file to store peer cert */
> -peercert_filename = create_temp_file(tmp_dir, "pcf", gc);
> -
>  /* write peer-cert in tmp-file */
>  peercert_file = fopen(peercert_filename, "w+");
>  if (!peercert_file)
> @@ -589,10 +589,13 @@ verify_cert_call_command(const char *verify_command, 
> struct env_set *es,
>  
>  if (verify_export_cert)
>  {
> -if ((tmp_file = verify_cert_export_cert(cert, verify_export_cert, 
> )))
> +tmp_file = verify_cert_export_cert(cert, verify_export_cert, );
> +if (!tmp_file)
>  {
> -setenv_str(es, "peer_cert", tmp_file);
> +ret = false;
> +goto cleanup;
>  }
> +setenv_str(es, "peer_cert", tmp_file);
>  }
>  
>  argv_parse_cmd(, verify_command);
> @@ -609,6 +612,7 @@ verify_cert_call_command(const char *verify_command, 
> struct env_set *es,
>  }
>  }
>  
> +cleanup:
>  gc_free();
>  argv_reset();
>  
> @@ -879,21 +883,21 @@ key_state_rm_auth_control_file(struct key_state *ks)
>  }
>  }
>  
> -static void
> +static bool
>  key_state_gen_auth_control_file(struct key_state *ks, const struct 
> tls_options *opt)
>  {
>  struct gc_arena gc = gc_new();
> -const char *acf;
>  
>  key_state_rm_auth_control_file(ks);
> -acf = create_temp_file(opt->tmp_dir, "acf", );
> +const char *acf

[Openvpn-devel] [PATCH 2/2] pf: clean up temporary files if plugin init fails

2017-09-14 Thread Steffan Karger

close_instance() tries to remove the file in c2.pf.filename, but that only
works if we actually set that if we fail.  So, set that filename as soon
as we know we've created the file.

Signed-off-by: Steffan Karger 
---
 src/openvpn/pf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
index 5fe1734..3faaebd 100644
--- a/src/openvpn/pf.c
+++ b/src/openvpn/pf.c
@@ -625,12 +625,12 @@ pf_init_context(struct context *c)
 const char *pf_file = create_temp_file(c->options.tmp_dir, "pf", );
 if (pf_file)
 {
+c->c2.pf.filename = string_alloc(pf_file, >c2.gc);
 setenv_str(c->c2.es, "pf_file", pf_file);
 
 if (plugin_call(c->plugins, OPENVPN_PLUGIN_ENABLE_PF, NULL, NULL, 
c->c2.es) == OPENVPN_PLUGIN_FUNC_SUCCESS)
 {
 event_timeout_init(>c2.pf.reload, 1, now);
-c->c2.pf.filename = string_alloc(pf_file, >c2.gc);
 c->c2.pf.enabled = true;
 #ifdef ENABLE_DEBUG
 if (check_debug_level(D_PF_DEBUG))
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 1/2] Don't throw fatal errors from create_temp_file()

2017-09-14 Thread Steffan Karger

This function is called in response to connecting clients, and can fail
when I/O fails for some (possibly temporary) reason.  In such cases we
should not exit the process, but just reject the connecting client.

This commit changes the function to actually return NULL on errors, and
(where needed) changes the callers to check for and handle errors.

Note that this changes the behavior for pf plugins: instead of just not
initializing the firewall rules and happily continuing, this now rejects
the client in the case of an (unlikely) failure to initialize the pf.

Since the tls-crypt-v2 metadata code also calls create_temp_file() when
clients connect, I consider this a prerequisite for tls-crypt-v2.

Signed-off-by: Steffan Karger 
---
 src/openvpn/misc.c   |  6 +++---
 src/openvpn/pf.c |  8 
 src/openvpn/ssl_verify.c | 32 +---
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 8c7f611..25f3800 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -740,7 +740,7 @@ create_temp_file(const char *directory, const char *prefix, 
struct gc_arena *gc)
 retfname = gen_path(directory, BSTR(), gc);
 if (!retfname)
 {
-msg(M_FATAL, "Failed to create temporary filename and path");
+msg(M_WARN, "Failed to create temporary filename and path");
 return NULL;
 }
 
@@ -755,14 +755,14 @@ create_temp_file(const char *directory, const char 
*prefix, struct gc_arena *gc)
 else if (fd == -1 && errno != EEXIST)
 {
 /* Something else went wrong, no need to retry.  */
-msg(M_FATAL | M_ERRNO, "Could not create temporary file '%s'",
+msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
 retfname);
 return NULL;
 }
 }
 while (attempts < 6);
 
-msg(M_FATAL, "Failed to create temporary file after %i attempts", 
attempts);
+msg(M_WARN, "Failed to create temporary file after %i attempts", attempts);
 return NULL;
 }
 
diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
index 5cb002b..5fe1734 100644
--- a/src/openvpn/pf.c
+++ b/src/openvpn/pf.c
@@ -639,10 +639,10 @@ pf_init_context(struct context *c)
 }
 #endif
 }
-else
-{
-msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
-}
+}
+if (!c->c2.pf.enabled)
+{
+register_signal(c, SIGUSR1, "plugin-pf-init-failed");
 }
 }
 #endif /* ifdef PLUGIN_PF */
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 9cd36d7..df2736c 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -547,14 +547,14 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, 
const char *tmp_dir, stru
 FILE *peercert_file;
 const char *peercert_filename = "";
 
-if (!tmp_dir)
+/* create tmp file to store peer cert */
+if (!tmp_dir ||
+!(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
 {
+msg (M_WARN, "Failed to create peer cert file");
 return NULL;
 }
 
-/* create tmp file to store peer cert */
-peercert_filename = create_temp_file(tmp_dir, "pcf", gc);
-
 /* write peer-cert in tmp-file */
 peercert_file = fopen(peercert_filename, "w+");
 if (!peercert_file)
@@ -589,10 +589,13 @@ verify_cert_call_command(const char *verify_command, 
struct env_set *es,
 
 if (verify_export_cert)
 {
-if ((tmp_file = verify_cert_export_cert(cert, verify_export_cert, 
)))
+tmp_file = verify_cert_export_cert(cert, verify_export_cert, );
+if (!tmp_file)
 {
-setenv_str(es, "peer_cert", tmp_file);
+ret = false;
+goto cleanup;
 }
+setenv_str(es, "peer_cert", tmp_file);
 }
 
 argv_parse_cmd(, verify_command);
@@ -609,6 +612,7 @@ verify_cert_call_command(const char *verify_command, struct 
env_set *es,
 }
 }
 
+cleanup:
 gc_free();
 argv_reset();
 
@@ -879,21 +883,21 @@ key_state_rm_auth_control_file(struct key_state *ks)
 }
 }
 
-static void
+static bool
 key_state_gen_auth_control_file(struct key_state *ks, const struct tls_options 
*opt)
 {
 struct gc_arena gc = gc_new();
-const char *acf;
 
 key_state_rm_auth_control_file(ks);
-acf = create_temp_file(opt->tmp_dir, "acf", );
+const char *acf = create_temp_file(opt->tmp_dir, "acf", );
 if (acf)
 {
 ks->auth_control_file = string_alloc(acf, NULL);
 setenv_str(opt->es, "auth_control_file", ks->auth_control_file);
-} /* FIXME: Should have better error handling? */
+}
 
 gc_free();
+return acf;
 }
 
 static unsigned int
@@ -1184,7 +1188,12 @@ verify_user_pass_plugin(struct tls_session *session, 
const struct user_pass *up,
 
 #ifdef PLUGIN_DEF_AUTH

[Openvpn-devel] Better error recognition and error/status reporting to the mgmt i/f

2017-09-14 Thread Selva

Hi,

Quoting from the meeting logs:


> Discussed having more fine-grained signalling from OpenVPN to OpenVPN
> GUI. The lack of clear signals from OpenVPN to OpenVPN GUI has been a
> rather common problem:
>


> 
> 
> 
>


> This problem is probably not limited to OpenVPN GUI (Windows), but also
> affects other GUI's like NetworkManager. It was agreed that the best
> approach is that Selva, mattock and others involved on the Windows side
> come up with a PoC or "RFC" of how this issue could be tackled ...
> OpenVPN core will then be modified if necessary.


No time to write an "RFC" or such for something of this sort, but here are
some comments/suggestions:

0. Do not send a status message that reads "CONNECTED, SUCCESS" to the
management when there has been critical errors such as: failed to add
routes or to set ipv6 address using the service or to talk to the service,
etc.. Send something like "CONNECTED,ERROR".

A status signalling with more fine-grained info to the management would be
helpful, but as a short-term fix, just changing SUCCESS to ERROR in some
cases may be a good start.

1. Mark all messages to the management as I (for info), W (for warning), N
(for non fatal error) etc. -- on some log lines this info is currently
missing. I think, part of the problem is not all messages are printed with
an M_xx flag.

2. Mark non-fatal opevpn_execve errors as M_NONFATAL instead of M_WARN

3. Treat failure to talk to the service (when msg_channnel is defined) as
M_NONFATAL not M_WARN

4. Mark route add/del errors via service as M_NONFATAL -- currently M_WARN.
 Mark route addition failure by other means as M_NONFATAL -- currently
M_WARN on all platforms, and all methods, I think.

5. Mark "ifconfig" (set address) errors as consistently M_FATAL or
M_NONFATAL (see comment on "fatality" below).
Currently these are M_WARN if done by service, M_FATAL otherwise.

Given that M_FATAL should be used only very rarely, if at all --- e.g., if
proceeding further makes no sense ---  most errors should be M_NONFATAL.
The above comments reflect that sentiment.

That said, whether address and route addition errors should be FATAL
deserves some discussion -- In case of address addition, an error probably
has to be FATAL, but "route add" failures are borderline cases. E.g., if
 "--redirect-gateway" fails, the tunnel may be  considered meaningless in
many use cases and thus a fatal error. So, some but not all route-add
errors may have to be treated as FATAL.

If there is consensus, and an appetite for patch review, I can send in some
patches for 2 to 5 and possibly 1. For 0, I'm not sure how to keep track of
past errors to construct a useful status message.

Thanks,

Selva
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [openvpn-devel] Forum upgrade problems

2017-09-14 Thread Eric Crist

I will look into these. I'm surprised [oconf] is broken, since I wrote that 
myself and it is not a normal part of phpBB. 

Eric Crist

> On Sep 14, 2017, at 6:36 AM, fragmentux  wrote:
> 
> The forum upgrade has broken the following BBCodes.
> 
> 1. [quote="Name"]
> "Name" is no longer shown.
> 
> 2. [code]
> Appears to strip out all newline / CRLF.
> 
> 3. [oconf=x]
> Appears to strip off last line.
> 
> 4. [list]
> Always inserts a bullet point even if no [*] is used.
> 
> 
> 
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [openvpn-devel] Forum upgrade problems

2017-09-14 Thread fragmentux


The forum upgrade has broken the following BBCodes.

1. [quote="Name"]
"Name" is no longer shown.

2. [code]
Appears to strip out all newline / CRLF.

3. [oconf=x]
Appears to strip off last line.

4. [list]
Always inserts a bullet point even if no [*] is used.



--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Check whether in pull_mode before warning about previous connection blocks

Re: [Openvpn-devel] [PATCH 2/2] pf: clean up temporary files if plugin init fails

Re: [Openvpn-devel] [PATCH 1/2] Don't throw fatal errors from create_temp_file()

[Openvpn-devel] [PATCH 2/2] pf: clean up temporary files if plugin init fails

[Openvpn-devel] [PATCH 1/2] Don't throw fatal errors from create_temp_file()

[Openvpn-devel] Better error recognition and error/status reporting to the mgmt i/f

Re: [Openvpn-devel] [openvpn-devel] Forum upgrade problems

[Openvpn-devel] [openvpn-devel] Forum upgrade problems

8 matches

Site Navigation

Mail list logo

Footer information