Fix --management-external-key in 2.3alpha

PATCH In review Arne Schwabe 3 participants 10 messages Jun 28, 2012 – Jul 20, 2012

lore

Share & tools

lore

Arne Schwabe

author

2012-06-28 17:44 UTC #0 lore raw

PATCH 1/1 Fix --management-external-key in 2.3alpha

Just a quick patch which fixes --management-external-key, which I need for a cleaner way to access Android key store.

Arne

From 53c5018f953764221c0aa51daa8bfa74f146900f Mon Sep 17 00:00:00 2001
From: Arne Schwabe <[email protected]...>
Date: Thu, 28 Jun 2012 19:41:09 +0200
Subject: [PATCH] If --management-external-key is used do not check for
private key file existence

Fixes error: --key fails with EXTERNAL_PRIVATE_KEY: No such file or directory if --management-external-key is used

Signed-off-by Arne Schwabe <[email protected]...>

---
 src/openvpn/options.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

src/openvpn/options.c +3 -0

index 367c1bc..a5f323d 100644
@@ -2709,6 +2709,9 @@ options_postprocess_filechecks (struct options *options)
   errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->cert_file, R_OK, "--cert");
   errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->extra_certs_file, R_OK,
                              "--extra-certs");
+#ifdef MANAGMENT_EXTERNAL_KEY
+	if(!(options->management_flags | MF_EXTERNAL_KEY))
+#endif
   errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->priv_key_file, R_OK,
                              "--key");
   errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->pkcs12_file, R_OK,

Signature · 1 line

1.7.7.5 (Apple Git-26)

Alon Bar-Lev

2012-06-28 17:58 UTC #1 lore raw

Hmmm.... I discussed this before in list... the whole external key should be modified... the management should be able to support certificate + key, and key should probably not be limited to RSA only. I had plans to clean this one as well.

Quoted 19 lines · Arne Schwabe

Just a quick patch which fixes --management-external-key, which I need for a cleaner way to access Android key store.
Arne
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Arne Schwabe

author

2012-06-28 18:03 UTC #2 lore raw

Am 28.06.12 19:58, schrieb Alon Bar-Lev:

Quoted 5 lines

Hmmm.... I discussed this before in list... the whole external key should be modified... the management should be able to support certificate + key, and key should probably not be limited to RSA only. I had plans to clean this one as well.

Sure. But at the moment fixing a regression is important than adding new features.

Arne

Alon Bar-Lev

2012-06-28 18:03 UTC #3 lore raw

Quoted 8 lines · Arne Schwabe

Am 28.06.12 19:58, schrieb Alon Bar-Lev:
Quoted 5 lines
Hmmm.... I discussed this before in list... the whole external key should be modified... the management should be able to support certificate + key, and key should probably not be limited to RSA only. I had plans to clean this one as well.
Sure. But at the moment fixing a regression is important than adding new features.

Right. I did not reject this... :)

Alon

David Sommerseth

2012-06-28 19:33 UTC #4 lore raw

In reply to Arne Schwabe

Quoted 4 lines · Arne Schwabe

Just a quick patch which fixes --management-external-key, which I need for a cleaner way to access Android key store.

(please, don't attach the patches as attachments - rather send them as plain text mail, easier to review and far easier for me to process these patches afterwards when including them - git send-email is easy to set up, which does everything correct instantly)

I think I found a bug in your patch ...

src/openvpn/options.c +2 -0

index 367c1bc..a5f323d 100644
@@ -2709,6 +2709,9 @@ options_postprocess_filechecks (struct options *options)
   errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->cert_file, R_OK, "--cert");
   errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->extra_certs_file, R_OK,
                              "--extra-certs");
+#ifdef MANAGMENT_EXTERNAL_KEY
+	if(!(options->management_flags | MF_EXTERNAL_KEY))
                                      ^^^

I believe this should the '|' should be '&' instead. At least, it doesn't work so well when I don't change it. What do you think?

My tests are:

./src/openvpn/openvpn --dev tun --tls-client --ca sample/sample-keys/ca.crt --cert sample/sample-keys/client.crt --management 127.0.0.1 1234 --remote 127.0.0.1 --key non-existing-file
(this should fail with a "file not found" error)

and

  ./src/openvpn/openvpn --dev tun --tls-client --ca sample/sample-keys/ca.crt --cert sample/sample-keys/client.crt  --management 127.0.0.1 1234 --remote 127.0.0.1 --key non-existing-file --management-external-key

But ... is it expected to pass --key if you use --management-external-key? I probably don't know enough about this --management-external-key feature, so I might be wrong.

kind regards,

David Sommerseth

Arne Schwabe

author

2012-06-28 21:21 UTC #5 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 28.06.12 21:33, schrieb David Sommerseth:

Quoted 7 lines · Arne Schwabe

Quoted 4 lines
Just a quick patch which fixes --management-external-key, which I need for a cleaner way to access Android key store.
(please, don't attach the patches as attachments - rather send them as

plain text mail, easier to review and far easier for me to process these patches afterwards when including them - git send-email is easy to set up, which does everything correct instantly)

Quoted 6 lines

I think I found a bug in your patch ...
^^^
I believe this should the '|' should be '&' instead. At least, it

doesn't work so well when I don't change it.

Quoted 1 line

What do you think?

Yes you are right.

Quoted 4 lines

My tests are:
./src/openvpn/openvpn --dev tun --tls-client --ca

sample/sample-keys/ca.crt --cert sample/sample-keys/client.crt --management 127.0.0.1 1234 --remote 127.0.0.1 --key non-existing-file

Quoted 5 lines

(this should fail with a "file not found" error)
and
./src/openvpn/openvpn --dev tun --tls-client --ca

sample/sample-keys/ca.crt --cert sample/sample-keys/client.crt --management 127.0.0.1 1234 --remote 127.0.0.1 --key non-existing-file --management-external-key

Quoted 2 lines

But ... is it expected to pass --key if you use

--management-external-key? I probably don't know enough about this --management-external-key feature, so I might be wrong.
No. --key is not passed but --managment-external-key sets --key filename to EXTERNAL_KEY (like inline TAG for files).

Arne

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk/syvcACgkQe8+cMNS4zRcXkwCfZZg6JIM+S9pMBZzhwmHrgqZi GFAAn3kn1uudf28fgm94+WuIQM6OlGfy
=BmbV
-----END PGP SIGNATURE-----

David Sommerseth

2012-06-29 07:49 UTC #6 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 6 lines · Arne Schwabe

Quoted 3 lines
But ... is it expected to pass --key if you use --management-external-key? I probably don't know enough about this --management-external-key feature, so I might be wrong.
No. --key is not passed but --managment-external-key sets --key filename to EXTERNAL_KEY (like inline TAG for files).

Ahh, okay, that makes sense. Should we consider to make --key and
- --management-external-key exclusive? So they it will choke if both is used? Just to make the usage a bit clearer.

kind regards,

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

iEYEARECAAYFAk/tXgEACgkQDC186MBRfrr/RACfX/v8arl5/kfCQRNo/ZIDbzDh rCgAnRVJAhmobwOFPn8ddgvufiXhE9u6
=V2AG
-----END PGP SIGNATURE-----

Arne Schwabe

author

2012-06-29 11:36 UTC #7 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 29.06.12 09:49, schrieb David Sommerseth:

Quoted 11 lines · Arne Schwabe

Quoted 6 lines
Quoted 3 lines
But ... is it expected to pass --key if you use --management-external-key? I probably don't know enough about this --management-external-key feature, so I might be wrong.
No. --key is not passed but --managment-external-key sets --key filename to EXTERNAL_KEY (like inline TAG for files).
Ahh, okay, that makes sense. Should we consider to make --key and --management-external-key exclusive? So they it will choke if both is used? Just to make the usage a bit clearer.

Yes. I will prepare a new patch.

Arne
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk/tkzEACgkQe8+cMNS4zRfExwCfY2YGdAHW7zCYcbCZ0P66buQO /cgAoLtWhsorAvh1HhBNS+qE3/3sLn7t
=7tId
-----END PGP SIGNATURE-----

Arne Schwabe

author

2012-07-20 23:02 UTC #8 lore raw

PATCH 1/1 If --management-external-key is used do not check for private key file existence

Fixes error: --key fails with EXTERNAL_PRIVATE_KEY: No such file or directory if --management-external-key is used

Signed-off-by Arne Schwabe <[email protected]...>

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

src/openvpn/options.c +12 -1

index cd1cb1c..7041e94 100644
@@ -1930,6 +1930,15 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
   if ((options->management_client_user || options->management_client_group)
       && !(options->management_flags & MF_UNIX_SOCK))
     msg (M_USAGE, "--management-client-(user|group) can only be used on unix domain sockets");
+#ifdef MANAGMENT_EXTERNAL_KEY
+  if(options->management_flags & MF_EXTERNAL_KEY) {
+	  if(options->priv_key_file)
+		  msg (M_USAGE, "--key and --management-external-key are mutually exclusive");
+	  /* set a filename for nicer output in the logs */
+	  options->priv_key_file = "EXTERNAL_PRIVATE_KEY";
+  }
+#endif
+
 #endif
 
   /*

@@ -2627,6 +2636,9 @@ options_postprocess_filechecks (struct options *options)
   errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->cert_file, R_OK, "--cert");
   errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->extra_certs_file, R_OK,
                              "--extra-certs");
+#ifdef MANAGMENT_EXTERNAL_KEY
+  if(!options->management_flags & MF_EXTERNAL_KEY)
+#endif
   errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->priv_key_file, R_OK,
                              "--key");
   errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->pkcs12_file, R_OK,

@@ -4141,7 +4153,6 @@ add_option (struct options *options,
     {
       VERIFY_PERMISSION (OPT_P_GENERAL);
       options->management_flags |= MF_EXTERNAL_KEY;
-      options->priv_key_file = "EXTERNAL_PRIVATE_KEY";
     }
 #endif
 #ifdef MANAGEMENT_DEF_AUTH

Signature · 1 line

1.7.9.5

Arne Schwabe

author

2012-07-20 23:15 UTC #9 lore raw

Re: [Openvpn-devel] [PATCH] If --management-external-key is used do not check for private key file existence

Am 21.07.12 01:02, schrieb Arne Schwabe:

Quoted 3 lines

Fixes error: --key fails with EXTERNAL_PRIVATE_KEY: No such file or directory if --management-external-key is used

Ignore that patch version. It did not compile. I am too tired today. I will post the correct version tommorow. Sorry for the noise.

Arne