don't mix settings when crl-verify option appears more than once

clr-verify can be specified multiple times in the config file and the expected behaviour is that the last occurrence should be used.

Therefore, reset the optional flags everytime a new crl-verify option is found.

Signed-off-by Antonio Quartulli <[email protected]...>

---
 src/openvpn/options.c | 8 ++++++++
 1 file changed, 8 insertions(+)

src/openvpn/options.c +1 -0

index 281ef0b..5192198 100644
@@ -6966,6 +6966,14 @@ add_option (struct options *options,
 		  || (p[2] && streq (p[1], INLINE_FILE_TAG) ) || !p[2]) && !p[3])
     {
       VERIFY_PERMISSION (OPT_P_GENERAL);
+      /*

Hi,

Quoted 20 lines · Antonio Quartulli

clr-verify can be specified multiple times in the config file and the expected behaviour is that the last occurrence should be used.

Therefore, reset the optional flags everytime a new crl-verify option is found.

Signed-off-by Antonio Quartulli <[email protected]...>

---
 src/openvpn/options.c | 8 ++++++++
 1 file changed, 8 insertions(+)

src/openvpn/options.c +1 -0

index 281ef0b..5192198 100644
@@ -6966,6 +6966,14 @@ add_option (struct options *options,
 		  || (p[2] && streq (p[1], INLINE_FILE_TAG) ) || !p[2]) && !p[3])
     {
       VERIFY_PERMISSION (OPT_P_GENERAL);
+      /*

src/openvpn/options.c add_option (struct options *options, · line 6979

Quoted 10 lines

src/openvpn/options.c +7 -0

+       * If crl-verify appears more than once in the config file, we have to
+       * to keep settings belonging to the last occurrence only.
+       * Reset optional settings each time.
+       */
+      options->ssl_flags &= ~SSLF_CRL_VERIFY_DIR;
+      options->crl_file_inline = NULL;
+
       if (p[2] && streq(p[2], "dir"))
 	options->ssl_flags |= SSLF_CRL_VERIFY_DIR;
       options->crl_file = p[1];

Feature-ACK. I think this is a great idea. This allows us to do an if(inline_file), instead of doing a strcmp against the 'magic inline string' everywhere.

However, I think it would be better to keep all inline file handling consistent. So I would prefer a patch that does this for all inline options, and replaces the strcmp()s at various places with if(inline).

(Thinking about refactoring this, wouldn't it be even prettier to change crl_file_inline to a bool, indicating whether crl_file contains a file path or the file contents? Might be a matter of style though, so feel free to ignore if you disagree.)

I was planning to do the rest of the refactoring in a second patch.

At this point I think it would make sense to reject this patch and wait for a full patchset set where I will:

1) "fix" every inline option to be sure that we don't carry around bogus settings
2) switch any *_inline field to bool
3) refactor the code to avoid strcmp()

Quoted 9 lines

I was planning to do the rest of the refactoring in a second patch.

At this point I think it would make sense to reject this patch and wait for a full patchset set where I will:

1) "fix" every inline option to be sure that we don't carry around bogus settings
2) switch any *_inline field to bool
3) refactor the code to avoid strcmp()

Am 26.10.16 um 14:48 schrieb Steffan Karger:

Quoted 6 lines · Antonio Quartulli

Quoted 5 lines

I was planning to do the rest of the refactoring in a second patch.

At this point I think it would make sense to reject this patch and wait for a full patchset set where I will:

Quoted 5 lines

Quoted 4 lines

1) "fix" every inline option to be sure that we don't carry around bogus settings
2) switch any *_inline field to bool
3) refactor the code to avoid strcmp()

Beware that the [[inline]] also serves as a filename and you have to add extra logic everywhere a file name is printed.