Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added smartcard support with openssl-engine #464

Merged
merged 7 commits into from
Sep 19, 2019

Conversation

rmuehl
Copy link
Contributor

@rmuehl rmuehl commented Aug 23, 2019

Developed for Yubikey, but may work with other PIV enabled smartcards too.

@mrbaseman
Copy link
Collaborator

Thank you very much for this. I'll have a closer look as soon as I find the time to do so. I'm quite busy with other things right now.

src/http.c Outdated
@@ -45,6 +45,8 @@ static void url_encode(char *dest, const char *str)
if (isalnum(*str) || *str == '-' || *str == '_'
|| *str == '.' || *str == '~')
*dest++ = *str;
// else if (*str == ' ')
// *dest++ = '+';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a quick look. We can omit these two lines that are commented out anyway (actually we have removed them last week on the master branch).
I haven't found the time to test yet, but from quickly looking over the code changes that's the only thing that I can see.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - I've cleaned that up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing in a Virtualbox...

  • Fedora 28 and 29 - OK
  • SuSE Tumbleweed - OK
  • Suse Leap 15.0 and 15.1 - OK, but needs a newer version of libp11!
  • Ubuntu 16.04 LTS - OK
  • MacOS Mojave - NOK, it doesn't find the pkcs engine-by-id. Will have a deeper look on it.

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Aug 29, 2019

@rmuehl Gteat contribution!

Can you detail the version of libp11 that doesn't work (Suse 15.*) and the one that does? I think autoconf should check for the existence of the P11-KIT library and probably the required minimal version. I think it boils down to modifying configure.ac:

PKG_CHECK_MODULES([LIBP11], [libp11 >= 1.2.3], [], [AC_MSG_ERROR([Cannot find P11-KIT 1.2.3 or higher.])])

and Makefile.am:

openfortivpn_CPPFLAGS += $(LIBP11_CFLAGS) $(OPENSSL_CFLAGS) $(LIBSYSTEMD_CFLAGS)
openfortivpn_LDADD = $(LIBP11_LIBS) $(OPENSSL_LIBS) $(LIBSYSTEMD_LIBS)

@rmuehl
Copy link
Contributor Author

rmuehl commented Aug 30, 2019

Turned out it's not about the libp11 version. Looks like SuSE messed up the RPMs for 15.0 and 15.1. There's a missing build-dependency for p11-kit-devel:

BuildRequires: p11-kit-devel

and libp11 ignores this silently:

[pkcs11_module="$PKG_CONFIG --variable=proxy_module --silence-errors p11-kit-1"]

@mrbaseman
Copy link
Collaborator

Maybe we can pin that down to a particular header file that must be present and which we can check at configure time?

@rmuehl
Copy link
Contributor Author

rmuehl commented Sep 2, 2019

But that would be the wrong package. The pkcs11-engine from libp11 needs to be compiled with p11-kit-devel installed. It's not related to openfortivpn....
We would need a check against pkcs11.so to see if its compiled in.
Maybe something like this (on a working Fedora)

strings pkcs11.so |grep kit
/usr/lib64/p11-kit-proxy.so

But it's not linked in

ldd pkcs11.so
linux-vdso.so.1 (0x00007fff2bb3d000)
libcrypto.so.1.1 => /lib64/libcrypto.so.1.1 (0x00007f0ac6f0f000)
libdl.so.2 => /lib64/libdl.so.2 (0x00007f0ac6d0b000)
libc.so.6 => /lib64/libc.so.6 (0x00007f0ac694d000)
libz.so.1 => /lib64/libz.so.1 (0x00007f0ac6736000)
libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f0ac6517000)
/lib64/ld-linux-x86-64.so.2 (0x00007f0ac75b4000)

I have no idea a.t.m, but I will take a deeper look into it to see what's really going on there.

@mrbaseman
Copy link
Collaborator

Ah, I see. AC_TRY_RUN with a minimal C code example would be an option, but that's another obstacle for cross-compiling.
Well, we are talking about a broken build dependency of a different package for a specific distribution... maybe it's not worth putting too much effort into that issue.

I have just successfully compiled your branch on Ubuntu 16.04. Perhaps I can try OS X and FreeBSD the next days.

@mrbaseman
Copy link
Collaborator

I have built successfully on FreeBSD.
One question: the dependency on p11-kit-devel, is it something that breaks compilation, or does it just prevent smartcards from working correctly at run time?
I suspect the latter, but the existing features would still be working with these patches, right?
I didn't have the time yet to look at Mac OS X. BTW is it a compiling issue or a run time problem which you have encountered there?

@rmuehl
Copy link
Contributor Author

rmuehl commented Sep 6, 2019

There is no problem with compiling openfortivpn on all OSes at all. It's just a runtime problem when the pkcs11 engine is getting loaded, and that just happens if it is set in the config.
There are 2 possible errors on problematc libp11:

  1. could not find engine_by_id
  2. error loading pkcs11 module

@rmuehl
Copy link
Contributor Author

rmuehl commented Sep 6, 2019

The SuSE thing is fixable with a good version of the pkcs11-engine. But on macOSX I have no idea yet. Couldn't get it running in my Virtualbox yet.

@mrbaseman mrbaseman merged commit 320f95f into adrienverge:master Sep 19, 2019
@mrbaseman
Copy link
Collaborator

I have squashed and merged this into our master. Let's address the issue on Mac OSX with a separate commit.
As time permits I'm going to test with my Yubikey on the different platforms - maybe I can find the problem on OSX, then.
Maybe we should add a more detailed tutorial or link one on the wiki.
@rmuehl thanks a lot for this contribution so far, and if you happen to find the problem on OSX, feel free to open another pull request.

@mrbaseman
Copy link
Collaborator

this went into the 1.11.0 Release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants