Re: [Openvpn-devel] [PATCH 1/2] Skip expired certificates in Windows certificate store

2018-04-02 Thread Steffan Karger 
Hi,

On 02-04-18 16:58, Selva Nair wrote:
> On Mon, Apr 2, 2018 at 8:37 AM, Steffan Karger  wrote:
>> Also, this looks like a somewhat unrelated fix.  I would have personally
>> preferred it in a separate patch (so we can e.g. backport it easily even
>> if we decide not not backport the functional change).
> 
> The original did the search based on subject and hash separately,
> which is now combined into a loop below this parsing chunk. So a
> return, in place of break, is required here. The warning on parse error
> is new (instead of silently returning NULL). But the end result of that
> is still a FATAL error later in the code, as before.
> 
> I'm ok to leave out the warning from this patch, though..

Ah, ok.  If it's not independent, let's just keep it in this patch.

-Steffan

--
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] Skip expired certificates in Windows certificate store

2018-04-02 Thread Steffan Karger 
Hi,

One comment based on stare-at-code only:

On 12-03-18 02:17, selva.n...@gmail.com wrote:
> @@ -636,6 +640,8 @@ find_certificate_in_store(const char *cert_prop, 
> HCERTSTORE cert_store)
>  }
>  if (!*++p)  /* unexpected end of string */
>  {
> +msg(M_WARN, "WARNING: cryptoapicert: error parsing <%s>.", 
> cert_prop);
> +return NULL;
>  break;
>  }

The break after the return looks a bit strange, maybe remove it?

Also, this looks like a somewhat unrelated fix.  I would have personally
preferred it in a separate patch (so we can e.g. backport it easily even
if we decide not not backport the functional change).

Otherwise the code and functional changes look good to me.  I'll try to
find some time to compile this and test it on windows later.

-Steffan

--
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] Skip expired certificates in Windows certificate store

2018-03-12 Thread Gert Doering 
Hi Selva,

On Sun, Mar 11, 2018 at 09:17:58PM -0400, selva.n...@gmail.com wrote:
> From: Selva Nair 
> 
> Have the cryptoapicert option find the first matching certificate
> in store that is valid at the present time. Currently the first
> found item, even if expired, is returned.

Are these two intended for master only or master+2.4?

(I admit that I am too lazy right now to go and actually look at the
surrounding code :-) - but with all the recent work wrt cryptoapi and
external management key, I lost track which bits are considered "new
goodies for master only")

Functionality-wise this makes sense (feature-ACK), and it also makes
sense for 2.4 - because "if there are two certificates, an expired and
a valid one, and we take the expired one" smells very much like a bug
to me :-)

gert
-- 
now what should I write here...

Gert Doering - Munich, Germany g...@greenie.muc.de


signature.asc
Description: PGP 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

[Openvpn-devel] [PATCH 1/2] Skip expired certificates in Windows certificate store

2018-03-11 Thread selva . nair 
From: Selva Nair 

Have the cryptoapicert option find the first matching certificate
in store that is valid at the present time. Currently the first
found item, even if expired, is returned.

This makes it possible to update certifiates in store without having
to delete old ones. As a side effect, if only expired certificates are
found, the connection fails.

Also remove some unnecessary casts.

Tested on Windows 10.
Trac #966

Signed-off-by: Selva Nair 
---
 src/openvpn/cryptoapi.c | 41 ++---
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 11b971f..a579854 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -601,27 +601,31 @@ find_certificate_in_store(const char *cert_prop, 
HCERTSTORE cert_store)
  * SUBJ:
  * THUMB:, e.g.
  * THUMB:f6 49 24 41 01 b4 fb 44 0c ce f4 36 ae d0 c4 c9 df 7a b6 28
+ * The first matching certificate that has not expired is returned.
  */
 const CERT_CONTEXT *rv = NULL;
+DWORD find_type;
+const void *find_param;
+unsigned char hash[255];
+CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
 
 if (!strncmp(cert_prop, "SUBJ:", 5))
 {
 /* skip the tag */
-cert_prop += 5;
-rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | 
PKCS_7_ASN_ENCODING,
-0, CERT_FIND_SUBJECT_STR_A, cert_prop, 
NULL);
-
+find_param = cert_prop + 5;
+find_type = CERT_FIND_SUBJECT_STR_A;
 }
 else if (!strncmp(cert_prop, "THUMB:", 6))
 {
-unsigned char hash[255];
-char *p;
+const char *p;
 int i, x = 0;
-CRYPT_HASH_BLOB blob;
+find_type = CERT_FIND_HASH;
+find_param = 
 
 /* skip the tag */
 cert_prop += 6;
-for (p = (char *) cert_prop, i = 0; *p && i < sizeof(hash); i++) {
+for (p = cert_prop, i = 0; *p && i < sizeof(hash); i++)
+{
 if (*p >= '0' && *p <= '9')
 {
 x = (*p - '0') << 4;
@@ -636,6 +640,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE 
cert_store)
 }
 if (!*++p)  /* unexpected end of string */
 {
+msg(M_WARN, "WARNING: cryptoapicert: error parsing <%s>.", 
cert_prop);
+return NULL;
 break;
 }
 if (*p >= '0' && *p <= '9')
@@ -657,10 +663,23 @@ find_certificate_in_store(const char *cert_prop, 
HCERTSTORE cert_store)
 }
 }
 blob.cbData = i;
-blob.pbData = (unsigned char *) 
+}
+while(true)
+{
+int validity = 1;
+/* this frees previous rv, if not NULL */
 rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | 
PKCS_7_ASN_ENCODING,
-0, CERT_FIND_HASH, , NULL);
-
+0, find_type, find_param, rv);
+if (rv)
+{
+validity = CertVerifyTimeValidity(NULL, rv->pCertInfo);
+}
+if (!rv || validity == 0)
+{
+break;
+}
+msg(M_WARN, "WARNING: cryptoapicert: ignoring certificate in store 
%s.",
+validity < 0 ? "not yet valid" : "that has expired");
 }
 
 return rv;
-- 
2.1.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