Re: [Openvpn-devel] Adding support for AEAD cipher modes (AES-GCM, et al.)

[Joachim Schipper]

[My apologies for any formatting issues; my employer likes Outlook.]

James Yonan wrote:
> On 18/06/2013 01:23, Joachim Schipper wrote:
> > Joachim Schipper wrote [I'm continuing my own message here]:
> >> Kenny Root wrote:
> >>> I wrote a patch to add AEAD cipher modes to OpenVPN.
> >>>
> >>> It also doesn't appear PolarSSL has added AES-GCM to their main
> >>> crypto API yet.
> >>
> >> Cool to see support for GCM in the data channel!
> >>
> >> PolarSSL does support GCM (since 1.2, see include/polarssl/gcm.h),
> >> and indeed OpenVPN-NL exclusively uses a GCM SSL ciphersuite in the
> >> control channel. However, it would be very neat to also have it in
> >> the data channel, so I looked at your patch with great interest.
> >> It's really neat, but I do have some comments.
> >
> >> The prng_bytes() on src/openvpn/crypto.c:144 [...]
> >
> > Continuing this train of thought, both GCM and CCM need non-repeating
> > (but possibly predictable) IVs. As long as net_time and net_id are
> > unique to the current packet, your code works fine; and you should
> > still have 32 bits of randomness left to guard against Bad Things
> > happening to the clock. So this code is just fine.
> >
> >> I don't understand src/openvpn/crypto.c:194 either, but I'll take
> >> another look tonight.
> >
> > After another reading, I understand just fine. Consider adding "else
> > ASSERT(outlen == 0)", but ok.
> >
> >> Again, thanks for the patch! I'm no contributor, but I do think it's
> >> really cool.
> >
> > Let me reiterate: thanks! I've just taken a quick look, but it looks
> > quite nice.
> 
> Agreed -- adding AEAD support is great.  That gives us access to AES-
> GCM, which has spectacular performance when accelerated at the CPU
> level (i.e. AES-NI).
> 
> Testing the patch with AES-GCM, the packet crypto header looks like
> this (appearing to be loosely inspired by RFC 4106):
> 
> 7e7046bd 444a7e28 cc6387b1 64a4d6c1 0005 521c3b01 4308c041 380275a
> [ auth tag] [seq # ] [time_t] [random] [payload
...]
> [   used like HMAC signature  ] [   12-byte IV/nonce/AD  ]
> 
> This is running in static key mode.  The time_t field is only used in
> static key mode and is based on the daemon startup time.  It doesn't
> change unless (a) the daemon is restarted or (b) the sequence number
> field wraps around.  In TLS mode there would be a leading control byte,
> and the time_t field would be replaced by randomness.
> 
> The "12-byte IV/nonce/AD" is the "associated data" field meaning that
> it is integrity-checked but not encrypted.  This is so it can double as
> the IV.
> 
> I'm thinking there might be some possible efficiency gains here by
> building the 12-byte nonce differently for TLS mode, such as by pre-
> agreeing to 8 bytes of randomness for each key negotiation, then send 4
> additional bytes over the wire as a sequence number instead of 12.  The
> IV could be synthesized on each end by combining the sequence number
> with the 8 bytes of agreed randomness.  This would cut 8 bytes out of
> the packet because [time_t] [random] vanishes, while retaining the
> essential property that the IV never repeats for a given key.

That could work, yes, and has the advantage that TLS mode remains secure
even if the system clock goes really wonky.

You're probably aware of IPsec's Extended Sequence Numbers (RFC 4303 par.
2.2.1), of which 32 bits are implicitly negotiated and 32 bits are sent over
the wire. IPsec's sequence numbers are not an IV, but the GCM's IV is just
another counter, which offers some independent support for your plan.

> Can we get some more reviewers on this?

Yes, please. I wouldn't want to trust everything to one quick review by me.

> One question that applies to AEAD crypto in general and not
> specifically this patch, is what are the security implications of doing
> away with HMAC and trusting the cipher to do both encryption/decryption
> and integrity checking?  Is there general agreement in the crypto
> community that the integrity checking component of AEAD is as solid as
> encrypt-then-HMAC?  Because crypto history seems littered with the
> wreckage of attempts to pipe untrusted data directly into decryption
> algorithms.

Piping untrusted data into CBC mode, in particular, is usually disastrous;
but GCM is basically CTR mode plus a checksum, and CTR mode is a stream
cipher. Stream ciphers don't need padding and generally allow very simple
data processing, which severely cuts down on the possible side channels.

GCM is fairly well-regarded, although some people worry

Re: [Openvpn-devel] [PATCH] TLS versioning

[Joachim Schipper]

>From James Yonan :
> This is the TLS versioning patch as discussed in last Thursday's IRC
> meeting.
> 
> It combines these two patches:
> 
> https://github.com/jamesyonan/openvpn/commit/03a5599202bdc3ba07983dc4ef
> dae387fb8fb436
> 
> https://github.com/jamesyonan/openvpn/commit/d23005413b0e0f28a3c48a6342
> f494763d5c9b40

For completeness: looks good to me, I have no comments that have not already 
been seen on this list.

Joachim

Re: [Openvpn-devel] OpenVPN Versioning

[Joachim Schipper]

From: James Yonan <ja...@openvpn.net>:
> On 14/06/2013 02:47, Joachim Schipper wrote:
> >>From James Yonan <ja...@openvpn.net>:
> >> TLS Protocol
> >> 
> >>
> >> Since day 1, OpenVPN has used TLS 1.0 as a control channel and key
> >> exchange mechanism.  But now we have TLS 1.1 and 1.2, each of which
> >> addresses significant shortcomings in its predecessor.  Fortunately,
> >> SSL/TLS already includes dynamic version negotiation.  So I've put
> >> together a patch that leverages on this, by allowing OpenVPN client
> >> and server to negotiate the TLS version and use the highest version
> >> supported by both peers.  The patch also adds a new directive "tls-
> >> version-min" that allows either client or server to specify a
> minimum
> >> required TLS version from the peer.
> >>
> >>
> https://github.com/jamesyonan/openvpn/commit/6ee8faade224cc346d67a7f1
> >> 7
> >> 1
> >> 6df4012782999a

[snip: SSL negotiation. Thanks for the explanation; I agree that speaking
the highest supported protocol is good.]

> > I'm not sure what purpose the "or-highest" serves. Clients already
> connect with the newest protocol supported by both client and server;
> if you want to run a TLSv1.2-only network, just set min-version to 1.2
> on the server. (...)
>
> Suppose a server admin wants to upgrade to TLS 1.2 over some transition
> period, to allow time to upgrade existing clients in the field.

> The overall goal here is to provide tools that allow a controlled
> rollout of TLS 1.2 that doesn't break any clients during the rollout
> period, and to upgrade to 1.2 in a way that doesn't create the
> potential for MiTM version rollback attacks.

But no modern SSL protocol allows version rollback attacks: modern
implementations will always speak the highest supported/configured protocol
version between each other. (Only) SSLv2 allows rollback attacks, which is
why the only sane way to deal with SSLv2 in 2013 is to unconditionally
disable support.

> > The switch(tls_version_min) needs a default case, just compile the
> first case/default: unconditionally.
>
> There is a default case already -- it's right under "case
> TLS_VER_1_0:".

Yes, but that's #ifdef'ed. I'd be happier if the default case remained
present even if PolarSSL foolishly decides to drop TLSv1 support.

[snip: ciphersuites]
> > Negotiating ciphers might be fatal in some configurations that were a
> > bad idea to begin with. E.g. if you use OpenVPN with a static key and
> > --auth none (which is a bad idea!), adding this negotation could
> allow
> > an attacker to set the cipher to none or, more dangerously, to a very
> > weak cipher like DES (provided it is mutually supported). An attacker
> > could then bruteforce the key and use his knowledge of 56 bits of the
> > key to attack stronger protocols like AES or 3DES. (Or do we only
> > negotiate in SSL mode? I must admit to being fuzzy on the non-SSL
> > mode.)
>
> Static key mode has no negotiation.
>
> Agreed that any negotiation model must have constraints placed on the
> security of the negotiated cipher and HMAC digest.  You would probably
> want to disable "cipher none" or "auth none" on the presumption that
> users who want a cleartext tunnel would explicitly configure the client
> and server for this.

Sure. Just disabling negotiations unless you have an SSL'd control channel
works fine against this.

> One thing to keep in mind is that OpenVPN protocol negotiation occurs
> over the already-negotiated TLS session, so it is immune to being
> tampered with by a MiTM.  This is in contrast to SSL/TLS where a MiTM
> can affect the negotiation.

As above, MiTM attackers cannot effect the negotiation step in SSL/TLS
protocols after SSLv2, except trivially (by causing the negotiation to fail, 
possibly a couple of protocol steps later, e.g. by dropping all packets.)

Joachim

Re: [Openvpn-devel] Adding support for AEAD cipher modes (AES-GCM, et al.)

[Joachim Schipper]

Joachim Schipper wrote [I'm continuing my own message here]:
> > -Original Message-
> > From: Kenny Root [mailto:ke...@the-b.org]
> > Sent: dinsdag 4 juni 2013 2:15
> > To: openvpn-devel@lists.sourceforge.net
> > Subject: [Openvpn-devel] Adding support for AEAD cipher modes
> > (AES-GCM, et al.)
> >
> > I wrote a patch to add AEAD cipher modes to OpenVPN.
> >
> > It also doesn't appear PolarSSL has added AES-GCM to their main crypto
> > API yet.
> >
>
> Cool to see support for GCM in the data channel!
>
> PolarSSL does support GCM (since 1.2, see include/polarssl/gcm.h), and
> indeed OpenVPN-NL exclusively uses a GCM SSL ciphersuite in the control
> channel. However, it would be very neat to also have it in the data
> channel, so I looked at your patch with great interest. It's really
> neat, but I do have some comments.

> The prng_bytes() on src/openvpn/crypto.c:144 seems odd in various ways.
> I'll continue reviewing tonight and tomorrow morning, and I'll take a
> good look at this chunk of code then. (Sorry for the slow review; life
> and spotty internet in the train got in the way.)

Continuing this train of thought, both GCM and CCM need non-repeating (but
possibly predictable) IVs. As long as net_time and net_id are unique to the
current packet, your code works fine; and you should still have 32 bits of
randomness left to guard against Bad Things happening to the clock. So this
code is just fine.

> I don't understand src/openvpn/crypto.c:194 either, but I'll take
> another look tonight.

After another reading, I understand just fine. Consider adding "else
ASSERT(outlen == 0)", but ok.

> Again, thanks for the patch! I'm no contributor, but I do think it's
> really cool.

Let me reiterate: thanks! I've just taken a quick look, but it looks quite
nice.

Joachim

Re: [Openvpn-devel] Adding support for AEAD cipher modes (AES-GCM, et al.)

[Joachim Schipper]

> -Original Message-
> From: Kenny Root [mailto:ke...@the-b.org]
> Sent: dinsdag 4 juni 2013 2:15
> To: openvpn-devel@lists.sourceforge.net
> Subject: [Openvpn-devel] Adding support for AEAD cipher modes 
> (AES-GCM, et al.)
> 
> I wrote a patch to add AEAD cipher modes to OpenVPN. This is one of 
> NIST's recommended modes and newer Intel chips supporting the AES-NI 
> (with PCLMULQDQ) have excellent support for acceleration in this mode.
> I posted the initial version at
> https://community.openvpn.net/openvpn/ticket/301 and that ticket has 
> some of the performance numbers on an Ivy Bridge class chip.
> 
> It also doesn't appear PolarSSL has added AES-GCM to their main crypto 
> API yet.
> 
> (You can also comment at Github at
> https://github.com/kruton/openvpn/commit/a8c6701c8b47429c32da5c01f4398
> f
> 9e0b293c01)

Cool to see support for GCM in the data channel!

PolarSSL does support GCM (since 1.2, see include/polarssl/gcm.h), and
indeed OpenVPN-NL exclusively uses a GCM SSL ciphersuite in the control
channel. However, it would be very neat to also have it in the data channel,
so I looked at your patch with great interest. It's really neat, but I do
have some comments.

I don't really see the benefit of adding CCM support; I'm not aware of any
new protocol using it, since GCM is generally faster/nicer/more common, and
- on recent Intel chips - benefits from hardware support (GCLMULQDQ). That
said, it's not hard, so...

XTS seems rather unsuited for this application; it's an unauthenticated
tweakable block cipher used for disk encryption. The fact that it uses the
same API as GCM/CCM is yet another instance of OpenSSL perfectly aligning a
gun with your foot.

The prng_bytes() on src/openvpn/crypto.c:144 seems odd in various ways. I'll
continue reviewing tonight and tomorrow morning, and I'll take a good look
at this chunk of code then. (Sorry for the slow review; life and spotty
internet in the train got in the way.)

I don't understand src/openvpn/crypto.c:194 either, but I'll take another
look tonight.

Some further minor comments:

src/openvpn/crypto.c:311: MAX_HMAC_KEY_LENGTH is a rather strange name for
the length of a variable holding the authentication tag.

src/openvpn/crypto.c 322: whitespace error

src/openvpn/crypto.c 430: #ifndef ALLOW_NON_CBC_CIPHERS, we still allow
AEAD? That seems odd.

Src/openvpn/crypto_backend.h:271: "message authentication code". In general,
neither the function name nor the doxygen makes it obvious that this
function is valid only for AEAD modes.

Again, thanks for the patch! I'm no contributor, but I do think it's really
cool.

Joachim


smime.p7s
Description: S/MIME cryptographic signature

Re: [Openvpn-devel] OpenVPN Versioning

[Joachim Schipper]

>From James Yonan :
> TLS Protocol
> 
>
> Since day 1, OpenVPN has used TLS 1.0 as a control channel and key
> exchange mechanism.  But now we have TLS 1.1 and 1.2, each of which
> addresses significant shortcomings in its predecessor.  Fortunately,
> SSL/TLS already includes dynamic version negotiation.  So I've put
> together a patch that leverages on this, by allowing OpenVPN client
> and server to negotiate the TLS version and use the highest version
> supported by both peers.  The patch also adds a new directive "tls-
> version-min" that allows either client or server to specify a minimum
> required TLS version from the peer.
>
> https://github.com/jamesyonan/openvpn/commit/6ee8faade224cc346d67a7f17
> 1
> 6df4012782999a

Some comments on the design and implementation here. (I've just looked at the 
code, not tested it.)

I'm confused by your comments about TLS/SSL versions used. Our own builds using 
PolarSSL do negotiate TLSv1.1 and TLSv1.2 between themselves and when 
interoperating with OpenSSL, logging "Control Channel: TLSv1.1, cipher XXX" or 
even "Control Channel: TLSv1.2, cipher XXX". In general, the highest version 
supported by both client and server is used, as is normally the case with SSL. 
However, stock Ubuntu OpenVPN seems to negotiate TLSv1. Huh?

I'm not sure what purpose the "or-highest" serves. Clients already connect with 
the newest protocol supported by both client and server; if you want to run a 
TLSv1.2-only network, just set min-version to 1.2 on the server. If you're an 
especially competent and paranoid user, you might want to set min-version on 
the client, but in that case you hardly need an "or-highest", since you know 
which cipher suites your client supports.

Some of the whitespace looks odd/wrong.

Does the change to key_state_ssl_init() do anything?

The variables polar_{major,minor} in ssl_polarssl.c should be renamed, probably 
to something like ssl_{major,minor}_version - it's not a PolarSSL version.

The switch(tls_version_min) needs a default case, just compile the first 
case/default: unconditionally.

> OpenVPN Protocol
> 

> [Handshaking] could be used to negotiate other protocol capabilities
> such as cipher and HMAC digest:
>
>IV_CIPHER=BF-CBC,AES-128-CBC
>IV_AUTH=SHA1,SHA256,SHA512
>
> (...) There
> is also the issue of the size of the Client Capabilities List.  The
> IV_CIPHER and IV_AUTH lists might grow to be quite long (...) To
> reduce the size, we might
> consider:
>
> (a) use a single character designation for ciphers and HMAC digests,
> such as:
>
> Cipher Codes:
>   A : BF-CBC
>   B : AES-128-CBC
>   . . .
>
> Then communicate IV_CIPHER using the cipher codes:
>
>IV_CIPHER=AB
>
> This would require that the OpenVPN Project standardize on set of
> codes for ciphers and HMAC digests.

This is a good idea in general, but why not use SSL cipher suites (which 
already have standardized binary representations)? That has the added advantage 
of itting AEAD suites like AES-GCM, which are getting more important. (We may 
need to add some, but note that the { 0xff, X } "namespace" is available for 
"local and experimental" cipher suites.)

Negotiating ciphers might be fatal in some configurations that were a bad idea 
to begin with. E.g. if you use OpenVPN with a static key and --auth none (which 
is a bad idea!), adding this negotation could allow an attacker to set the 
cipher to none or, more dangerously, to a very weak cipher like DES (provided 
it is mutually supported). An attacker could then bruteforce the key and use 
his knowledge of 56 bits of the key to attack stronger protocols like AES or 
3DES. (Or do we only negotiate in SSL mode? I must admit to being fuzzy on the 
non-SSL mode.)

Joachim

Re: [Openvpn-devel] [PATCH] Fix for bug #49 for openvpn 2.2.2

[Joachim Schipper]

> Hi,
>
> our setup needs openvpn UDP/TLS tunnels with dynamic client IP addresses, so 
> I implemented a fix for the bug #49 that has been open for over two years. 
>
> The patch is for version 2.2.2 as I had trouble compiling the 2.3.x tarball 
> from openvpn.net. As the solution is rather simple (just two small utility 
> functions in mudp.c) I'd guess it could be comfortambly migrated to 2.3.x.
>
> Basically what the fix does is the following: incoming data channel UDP 
> packets from an unknown IP are matched against existing UDP/TLS sessions, and 
> if the packet passes the HMAC authentication against an existing TLS context 
> we know the client IP has changed and the session state will be instantly 
> updated accordingly.
>
> I have tested this fix to some extent, and the IP handover works impressively 
> smoothly in my test setup where I randomly switch between two routes from 
> client to server.
>
> Dynamic client IP's are enabled/disabled with --float in the server side.
>
> Please feel free to contact me for any questions etc.

This is not a full review of the patch, just a few quick remarks.

This patch seems to do (number of tunnels) HMAC's for any packet received from 
an unknown IP. If this is correct, couldn't a server with a couple thousand 
tunnels be brought just by sending it a few hundred packets a second?

authenticate_tls_packet() contains a for() loop, but I see no code path that 
actually loops - everything seems to return immediately?

memcmp() should be replaced by a constant-time function to prevent timing 
attacks (probably only realistic on a server with a single tunnel); even then, 
this patch allows determining the (approximate) number of tunnels in use by 
looking at processing time.

I haven't looked at how this interacts with other features; it's at least 
noteworthy that the floating behavior can't be specified on a per-connection 
basis.

Joachim

[Openvpn-devel] [PATCH] Add a basic --management-external-key client to contrib/

[Joachim Schipper]

A very simple client for --management-external-key based on an on-disk keyfile.
Useful for testing.

Signed-off-by: Joachim Schipper <joachim.schip...@fox-it.com>
---
 .gitignore |1 +
 contrib/management-external-key-client/Makefile|   12 +
 contrib/management-external-key-client/README  |   12 +
 .../management_external_key.c  |  349 
 4 files changed, 374 insertions(+)
 create mode 100644 contrib/management-external-key-client/Makefile
 create mode 100644 contrib/management-external-key-client/README
 create mode 100644 
contrib/management-external-key-client/management_external_key.c

diff --git a/.gitignore b/.gitignore
index a04afff..a8fb8a6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -48,6 +48,7 @@ version.sh
 msvc-env-local.bat
 config-msvc-local.h
 config-msvc-version.h
+contrib/management-external-key-client/management_external_key
 doc/openvpn.8.html
 distro/rpm/openvpn.spec
 tests/t_client.sh
diff --git a/contrib/management-external-key-client/Makefile 
b/contrib/management-external-key-client/Makefile
new file mode 100644
index 000..bad83b0
--- /dev/null
+++ b/contrib/management-external-key-client/Makefile
@@ -0,0 +1,12 @@
+.PHONY: clean
+
+#POLARSSL_INCLUDES?=-I/usr/local/include
+#POLARSSL_LDFLAGS?=-L/usr/local/lib -static
+CFLAGS=-O2 -g -W -Wall -Wextra -Wdeclaration-after-statement 
${POLARSSL_INCLUDES}
+LDFLAGS=${POLARSSL_LDFLAGS}
+
+management_external_key: management_external_key.c
+   ${CC} ${CFLAGS} ${LDFLAGS} -o management_external_key 
management_external_key.c -lpolarssl
+
+clean:
+   rm -f management_external_key
diff --git a/contrib/management-external-key-client/README 
b/contrib/management-external-key-client/README
new file mode 100644
index 000..78df077
--- /dev/null
+++ b/contrib/management-external-key-client/README
@@ -0,0 +1,12 @@
+When given the --management-external-key option, OpenVPN does not use a private
+key to sign SSL handshakes, but instead requests a signature over the
+management interface.
+
+This is a simple client for the management interface that uses a private key
+stored in a PEM file to create the signatures.
+
+You'll need PolarSSL to compile this code. Run management_external_key for
+instructions.
+
+Note that this is not production-ready code. It may, however, be useful for
+testing purposes.
diff --git a/contrib/management-external-key-client/management_external_key.c 
b/contrib/management-external-key-client/management_external_key.c
new file mode 100644
index 000..f57a084
--- /dev/null
+++ b/contrib/management-external-key-client/management_external_key.c
@@ -0,0 +1,349 @@
+/*
+ *  A simple client for openvpn --management-external-key.
+ *
+ *  Copyright (C) 2012 Fox Crypto B.V. <open...@fox-it.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program (see the file COPYING included with this
+ *  distribution); if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+/*
+ * This code depends only on PolarSSL.
+ */
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#define USAGE ("Usage: %s [-l] [-H host] port key\n" \
+"Client for openvpn --management-external-key\n" \
+"\n" \
+"  -H host  connect to host instead of localhost\n" \
+"  -l   request OpenVPN logs\n" \
+"  port port of the OpenVPN management interface\n" \
+"  key  keyfile (in PEM format) to use\n")
+
+/* Management interface requesting a signature */
+#define RSA_PROMPT_PREFIX ">RSA_SIGN:"
+#define RSA_RESP_PREFIX "rsa-sig\r\n"
+#define RSA_RESP_SUFFIX "\r\nEND\r\n"
+
+#ifndef __GNUC__
+#define __attribute__(x) /* gcc only */
+#endif
+
+int main(int argc, char **argv);
+/* Connect to management interface; returns socket or terminates program */
+static inline int open_sock(const char *host, const char *port);
+/* Communicate with OpenVPN */
+static inline void management_client(int management_sock, const char *keyfile,
+int request_logs) __attribute__((noreturn));
+/*
+ * Write some initial commands requesting logs etc. Returns 0 on success, or
+ * nonzero and sets errno.
+ */
+static inline int write_initial_commands(int manag

[Openvpn-devel] [PATCH] doc/management-notes.txt: fix typo

[Joachim Schipper]

Signed-off-by: Joachim Schipper <joachim.schip...@fox-it.com>
---
 doc/management-notes.txt |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/management-notes.txt b/doc/management-notes.txt
index 79e71ad..ef39b85 100644
--- a/doc/management-notes.txt
+++ b/doc/management-notes.txt
@@ -774,7 +774,7 @@ END
 Base64 encoded output of RSA_sign(NID_md5_sha1,... will provide a
 correct signature.
 
-This capability is intended to allow the use of arbitrarycryptographic
+This capability is intended to allow the use of arbitrary cryptographic
 service providers with OpenVPN via the management interface.
 
 
-- 
1.7.9.5

Re: [Openvpn-devel] [PATCH] Fix --show-pkcs11-ids

[Joachim Schipper]

Yes, that seems to be correct. Sorry!

Joachim

-Original Message-
From: David Sommerseth [mailto:openvpn.l...@topphemmelig.net] 
Sent: dinsdag 20 november 2012 10:19
To: Joachim Schipper
Cc: openvpn-devel@lists.sourceforge.net; Adriaan de Jong
Subject: Re: [Openvpn-devel] [PATCH] Fix --show-pkcs11-ids

On 14/11/12 10:03, Adriaan de Jong wrote:
> [PATCH] Fix --show-pkcs11-ids (Bug #239)
> 
> Broken by 75b49e406430299b187964744f82e50a9035a0d3.
> 
> Signed-off-by: Joachim Schipper <joachim.schip...@fox-it.com>
> ---
>  src/openvpn/pkcs11.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Hi Joachim,

Thanks a lot for you fix!  Just a little question.  You mention this commit as 
being the offending commit:

commit 75b49e406430299b187964744f82e50a9035a0d3
Author: Alon Bar-Lev <alon.bar...@gmail.com>
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Sun Apr 1 16:46:28 2012 +0300

cleanup: gc usage

Cleanup of "Use the garbage collector when retrieving x509 fields"
patch series.

Discussed at [1].

There should be an effort to produce common function prologue
and epilogue, so that cleanups will be done at single point.

[1] http://comments.gmane.org/gmane.network.openvpn.devel/5401

Signed-off-by: Alon Bar-Lev <alon.bar...@gmail.com>
Acked-by: Adriaan de Jong <dej...@fox-it.com>
Signed-off-by: David Sommerseth <dav...@redhat.com>


I'm just wondering how this could be related to you fix.  As I see it, the 
syntax of the if() statement you change didn't change, neither the behaviour of 
pkcs11_certificate_dn().

I see that commit 00b973f8af85c3ea8fa3cef80eec55e8dc139b27 changes the 
behaviour of pkcs11_certificate_dn() in both the OpenSSL and PolarSSL 
implementation.  So I'm suspecting you referenced the wrong commit.  Right?


--
kind regards,

David Sommerseth