Skip to contentSkip to conversation

Check whether in pull_mode before warning about previous connection blocks

PATCH Superseded [email protected]... 4 participants 12 messages Sep 15, 2017 – Sep 30, 2017

lore
Share & tools
lore

[email protected]...

author
#0 lore raw

PATCH 1/1 Check whether in pull_mode before warning about previous connection blocks

From: Selva Nair <[email protected]...>

Eliminate the confusing message that says "explicit-exit-notify is ignored by previous <connection> blocks" when the option is pushed.
Reported by: Eike Lohmann [email protected]...
https://www.mail-archive.com/[email protected]/msg04052.html

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
src/openvpn/options.c +1 -1
index 3a5bccf..d302bd4 100644
@@ -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]);
Signature · 1 line
2.6.2
#1 lore raw

Hi,

Quoted 6 lines · On Fri, Sep 15, 2017 at 12:41:03AM -0400, [email protected]... wrote:

From: Selva Nair <[email protected]...>

Eliminate the confusing message that says "explicit-exit-notify is ignored by previous <connection> blocks" when the option is pushed.
Reported by: Eike Lohmann [email protected]...
https://www.mail-archive.com/[email protected]/msg04052.html

Not sure I understand that patch. If I read it right (and I might not), won't this just make this option non-pushable, thus suppressing the error message by disabling the option?

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
author
#2 lore raw
Quoted 7 lines · Gert Doering

Hi,

Quoted 3 lines · On Fri, Sep 15, 2017 at 12:41:03AM -0400, [email protected]... wrote:

From: Selva Nair <[email protected]...>

Eliminate the confusing message that says "explicit-exit-notify is

ignored by

Quoted 9 lines
Quoted 3 lines

previous <connection> blocks" when the option is pushed. Reported by: Eike Lohmann [email protected]...
https://www.mail-archive.com/[email protected]...

sourceforge.net/msg04052.html

Not sure I understand that patch. If I read it right (and I might not), won't this just make this option non-pushable, thus suppressing the error message by disabling the option?

No it doesn't change any functionality. By not including OPT_P_CONNECTION in the mask sent to verify permission the previous connection blacks check is disabled. And this is done only when pull_mode is True.

So the only effect is just to suppress the message about previous connection blocks when this option is parsed as a pushed option.

The key here is that check pull_mode to know that we are processing a pushed option.

Selva

Quoted 11 lines

gert
--
USENET is *not* the non-clickable part of WWW!

                                                           //

www.muc.de/~gert/
Gert Doering - Munich, Germany
[email protected]...
fax: +49-89-35655025 [email protected]...
muenchen.de

#3 lore raw

Hi,

Quoted 3 lines · Selva

No it doesn't change any functionality. By not including OPT_P_CONNECTION in the mask sent to verify permission the previous connection blacks check is disabled. And this is done only when pull_mode is True.

I was totally misreading the code, apologies :-) - now I read up on what OPT_P_CONNECTION *does*, and it's not a "normal" flag for "this is not an acceptable option in this mode" but a "warning if..." one.

[..]

Quoted 2 lines

The key here is that check pull_mode to know that we are processing a pushed option.

What about moving the pull_mode check into verify_permission() so it applies to all pushed option that could trigger the warning (as Arne pointed out)?

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
author
#4 lore raw

Hi,

Quoted 13 lines · Gert Doering

Hi,

Quoted 2 lines · Selva

No it doesn't change any functionality. By not including OPT_P_CONNECTION in the mask sent to verify permission the previous connection blacks

check

Quoted 1 line

is disabled. And this is done only when pull_mode is True.

I was totally misreading the code, apologies :-) - now I read up on what OPT_P_CONNECTION *does*, and it's not a "normal" flag for "this is not an acceptable option in this mode" but a "warning if..." one.

Quoted 7 lines

[..]

Quoted 2 lines

The key here is that check pull_mode to know that we are processing a pushed option.

What about moving the pull_mode check into verify_permission() so it applies to all pushed option that could trigger the warning (as Arne pointed out)?

I suppose Arne's statement is "theoretical" -- I could not find any other option that could be in a connection block and pushable. Are there any?

Anyway, the reason I did not add the check in verify_permission() is that pull_mode is not as such defined there -- easily fixed at the expense of code repetition and associated "risks.."

But pull_mode = allowed & OPT_P_PLL_MODE is probably never going to change, so this may be its a lame excuse. I'll do a v2 if that's the preferred way.

Selva

#5 lore raw
In reply to [email protected]...
Quoted 6 lines · On 9/15/17 6:41 AM, [email protected]... wrote:

From: Selva Nair <[email protected]...>

Eliminate the confusing message that says "explicit-exit-notify is ignored by previous <connection> blocks" when the option is pushed.
Reported by: Eike Lohmann [email protected]...
https://www.mail-archive.com/[email protected]/msg04052.html

The problem exists for all connection block options that are pushable. We rather need to signal when options are pushed (or do a dirty streq(file, [pushed-options]) and then only ignore the warning. The warning is just bogus for pushed options.

Arne

author
#6 lore raw
Quoted 8 lines · Arne Schwabe
Quoted 3 lines · On 9/15/17 6:41 AM, [email protected]... wrote:

From: Selva Nair <[email protected]...>

Eliminate the confusing message that says "explicit-exit-notify is

ignored by

Quoted 3 lines

previous <connection> blocks" when the option is pushed. Reported by: Eike Lohmann [email protected]...
https://www.mail-archive.com/[email protected]...

Quoted 7 lines

.net/msg04052.html

The problem exists for all connection block options that are pushable. We rather need to signal when options are pushed (or do a dirty streq(file, [pushed-options]) and then only ignore the warning. The warning is just bogus for pushed options.

Isn't that what the variable pull_mode is for? Its True when we are processing a pushed option False otherwise.

Selva

Quoted 2 lines

Arne

author
#7 lore raw

PATCH v2 1/1 Check whether in pull_mode before warning about previous connection blocks

In reply to [email protected]...

From: Selva Nair <[email protected]...>

In particular, this eliminates the message that says "explicit-exit-notify is ignored by previous <connection> blocks" when the option is pushed.

Note: pull_mode is identified as "allowed & OPT_P_PULL_MODE" matching with the definition in add_options().

Reported by: Eike Lohmann [email protected]...
https://www.mail-archive.com/[email protected]/msg04052.html

v2: move the check to verify_permissions() as suggested by Gert <[email protected]...>

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/options.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
src/openvpn/options.c +3 -1
index 3a5bccf..b4613df 100644
@@ -4824,11 +4824,13 @@ verify_permission(const char *name,
 #ifndef ENABLE_SMALL
     /* Check if this options is allowed in connection block,
      * but we are currently not in a connection block
+     * unless this is a pushed option.
      * Parsing a connection block uses a temporary options struct without
      * connection_list
      */
 
-    if ((type & OPT_P_CONNECTION) && options->connection_list)
+    if ((type & OPT_P_CONNECTION) && options->connection_list
+        && !(allowed & OPT_P_PULL_MODE))
     {
         if (file)
         {
Signature · 1 line
2.6.2
author
#8 lore raw

Hi,

Quoted 13 lines · On Fri, Sep 15, 2017 at 2:46 PM, <[email protected]...> wrote:

From: Selva Nair <[email protected]...>

In particular, this eliminates the message that says "explicit-exit-notify is ignored by previous <connection> blocks" when the option is pushed.

Note: pull_mode is identified as "allowed & OPT_P_PULL_MODE" matching with the
definition in add_options().

Reported by: Eike Lohmann [email protected]...
https://www.mail-archive.com/[email protected]...
sourceforge.net/msg04052.html

Quoted 8 lines

v2: move the check to verify_permissions() as suggested by Gert <[email protected]...>

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/options.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Does this look good now?

Selva

#9 lore raw
Quoted 20 lines · Selva

Hi,

Quoted 16 lines · On Fri, Sep 15, 2017 at 2:46 PM, <[email protected]...> wrote:

From: Selva Nair <[email protected]...>

In particular, this eliminates the message that says "explicit-exit-notify is ignored by previous <connection> blocks" when the option is pushed.

Note: pull_mode is identified as "allowed & OPT_P_PULL_MODE" matching with the
definition in add_options().

Reported by: Eike Lohmann [email protected]...
https://www.mail-archive.com/[email protected]...
sourceforge.net/msg04052.html

v2: move the check to verify_permissions() as suggested by Gert <[email protected]...>

Quoted 8 lines
Quoted 5 lines
Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/options.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Does this look good now?

pull_mode is already computed in add_options() and could just be passed to verify_permission(). However, adding another parameter to the function call is probably not worth the gain.

Personally, I think this patch looks good now.

Cheers,

Signature · 1 line
Antonio Quartulli
author
#10 lore raw

Hi,

Quoted 34 lines · Antonio Quartulli
Quoted 7 lines · Selva

Hi,

Quoted 3 lines · On Fri, Sep 15, 2017 at 2:46 PM, <[email protected]...> wrote:

From: Selva Nair <[email protected]...>

In particular, this eliminates the message that says

"explicit-exit-notify

Quoted 3 lines
Quoted 3 lines

is ignored by previous <connection> blocks" when the option is pushed.

Note: pull_mode is identified as "allowed & OPT_P_PULL_MODE" matching

with

Quoted 10 lines
Quoted 10 lines

the
definition in add_options().

Reported by: Eike Lohmann [email protected]...
https://www.mail-archive.com/[email protected]...
sourceforge.net/msg04052.html

v2: move the check to verify_permissions() as suggested by Gert <[email protected]...>

Quoted 8 lines
Quoted 5 lines
Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/options.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Does this look good now?

Quoted 9 lines

pull_mode is already computed in add_options() and could just be passed to verify_permission(). However, adding another parameter to the function call is probably not worth the gain.

Personally, I think this patch looks good now.

Cheers,

Thanks :)

Selva

#11 lore raw
In reply to [email protected]...

Thanks. Small and to the point :-) (and though I trust Antonio's ACK, I have quick-tested it and does what it says :) ).

Your patch has been applied to the master and release/2.4 branch.

commit 422ecdac4a2738cd269361e048468d8b58793c4e (master) commit 90812e9879ba47ff0275f20efd653ae3381308b8 (release/2.4)

Author: Selva Nair
Date:   Fri Sep 15 14:46:15 2017 -0400
     Check whether in pull_mode before warning about previous connection blocks

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Antonio Quartulli <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg15452.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering