Review Request: Remove licence-incompatible OpenSSL usage from OpenConnect support

Discussion:

David Woodhouse

2012-06-08 14:50:37 UTC

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105185/
-----------------------------------------------------------

Review request for Network Management.

Description
-------

OpenConnect: Eliminate OpenSSL dependencies, support new libopenconnect

Making libopenconnect support GnuTLS, required for licence compatibility
with KDE, means retrospectively changing the ABI so that the certificate
is treated as an opaque pointer. The library now provides a get_details()
method to get the certificate information in text form, so use that instead
of "knowing" that it's an OpenSSL X509 structure.

Cope with a couple of other minor API changes which the change of soname
gave us the opportunity to introduce.

Diffs
-----

vpnplugins/openconnect/CMakeLists.txt 3ea15bd5b343b05ac20eb2e94f012d4a14848a25
vpnplugins/openconnect/openconnectauth.h 417bd7eb3898011243e320b5ec095fcfc89db596
vpnplugins/openconnect/openconnectauth.cpp cd9504b0bd100d37fe3c7d09463893f966986239
vpnplugins/openconnect/openconnectauthworkerthread.h 7aa8dfc866a9ea554329c4b3d14a116da74fc0ea
vpnplugins/openconnect/openconnectauthworkerthread.cpp ec93ab2ca2481b8c2d58450a041414b75a1c8f6a

Diff: http://git.reviewboard.kde.org/r/105185/diff/

Testing
-------

Building against both old and new libopenconnect, ensuring that OpenSSL is not linked directly when building against new.

Thanks,

David Woodhouse

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120608/783da83b/attachment.html>

David Woodhouse

2012-06-09 08:41:25 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105185/
-----------------------------------------------------------

(Updated June 9, 2012, 8:41 a.m.)

Review request for Network Management.

Description
-------

OpenConnect: Eliminate OpenSSL dependencies, support new libopenconnect

Making libopenconnect support GnuTLS, required for licence compatibility
with KDE, means retrospectively changing the ABI so that the certificate
is treated as an opaque pointer. The library now provides a get_details()
method to get the certificate information in text form, so use that instead
of "knowing" that it's an OpenSSL X509 structure.

Cope with a couple of other minor API changes which the change of soname
gave us the opportunity to introduce.

Diffs
-----

vpnplugins/openconnect/CMakeLists.txt 3ea15bd5b343b05ac20eb2e94f012d4a14848a25
vpnplugins/openconnect/openconnectauth.h 417bd7eb3898011243e320b5ec095fcfc89db596
vpnplugins/openconnect/openconnectauth.cpp cd9504b0bd100d37fe3c7d09463893f966986239
vpnplugins/openconnect/openconnectauthworkerthread.h 7aa8dfc866a9ea554329c4b3d14a116da74fc0ea
vpnplugins/openconnect/openconnectauthworkerthread.cpp ec93ab2ca2481b8c2d58450a041414b75a1c8f6a

Diff: http://git.reviewboard.kde.org/r/105185/diff/

Testing (updated)
-------

Building against both old and new libopenconnect, ensuring that OpenSSL is not linked directly when building against new.

Connecting to VPN from KDE with new libopenconnect (linked against GnuTLS).

Thanks,

David Woodhouse

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120609/53ee20a0/attachment.html>

Lamarque Vieira Souza

2012-06-12 12:38:17 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105185/#review14655
-----------------------------------------------------------

vpnplugins/openconnect/openconnectauth.cpp
<http://git.reviewboard.kde.org/r/105185/#comment11585>

Doesn't exist a #define we can use instead of hardcoding the number 41 here?

vpnplugins/openconnect/openconnectauthworkerthread.cpp
<http://git.reviewboard.kde.org/r/105185/#comment11586>

here too.

- Lamarque Vieira Souza

Post by David Woodhouse
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/105185/
-----------------------------------------------------------
(Updated June 9, 2012, 8:41 a.m.)
Review request for Network Management.
Description
-------
OpenConnect: Eliminate OpenSSL dependencies, support new libopenconnect
Making libopenconnect support GnuTLS, required for licence compatibility
with KDE, means retrospectively changing the ABI so that the certificate
is treated as an opaque pointer. The library now provides a get_details()
method to get the certificate information in text form, so use that instead
of "knowing" that it's an OpenSSL X509 structure.
Cope with a couple of other minor API changes which the change of soname
gave us the opportunity to introduce.
Diffs
-----
vpnplugins/openconnect/CMakeLists.txt 3ea15bd5b343b05ac20eb2e94f012d4a14848a25
vpnplugins/openconnect/openconnectauth.h 417bd7eb3898011243e320b5ec095fcfc89db596
vpnplugins/openconnect/openconnectauth.cpp cd9504b0bd100d37fe3c7d09463893f966986239
vpnplugins/openconnect/openconnectauthworkerthread.h 7aa8dfc866a9ea554329c4b3d14a116da74fc0ea
vpnplugins/openconnect/openconnectauthworkerthread.cpp ec93ab2ca2481b8c2d58450a041414b75a1c8f6a
Diff: http://git.reviewboard.kde.org/r/105185/diff/
Testing
-------
Building against both old and new libopenconnect, ensuring that OpenSSL is not linked directly when building against new.
Connecting to VPN from KDE with new libopenconnect (linked against GnuTLS).
Thanks,
David Woodhouse

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120612/291f1ce8/attachment.html>

David Woodhouse

2012-06-14 00:35:02 UTC

Permalink

Post by David Woodhouse

vpnplugins/openconnect/openconnectauth.cpp, line 298
<http://git.reviewboard.kde.org/r/105185/diff/1/?file=66780#file66780line298>
Doesn't exist a #define we can use instead of hardcoding the number 41 here?

The problem is that the #defines belong to the crypto libraries ? GnuTLS or OpenSSL. And the libopenconnect library has been modified to avoid exposing them. If I added a #define to openconnect.h then using it would break the build against older libraries... unless we add more #ifdefs to cope with that.

On the whole, since the size of a SHA1 is never going to change, I'm most inclined to leave it as it is. It's quite clear what it's for, since the call to openconnect_get_cert_sha1() is within three lines in both cases.

Unless you strongly object, let's leave this as it is.

- David

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105185/#review14655
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120614/7653c13d/attachment-0001.html>

Lamarque Vieira Souza

2012-06-14 01:00:47 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105185/#review14719
-----------------------------------------------------------

vpnplugins/openconnect/openconnectauth.cpp
<http://git.reviewboard.kde.org/r/105185/#comment11617>

Looking in /usr/include/openconnect.h I am really not comfortable in not seeing any documentation about how big the buffer for openconnect_get_cert_sha1 should be. You could add a comment explaining that, then this patch is good to go.

- Lamarque Vieira Souza

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120614/7bf300b7/attachment.html>

David Woodhouse

2012-06-14 07:44:25 UTC

Permalink

Post by David Woodhouse

vpnplugins/openconnect/openconnectauth.cpp, line 298
<http://git.reviewboard.kde.org/r/105185/diff/1/?file=66780#file66780line298>
Looking in /usr/include/openconnect.h I am really not comfortable in not seeing any documentation about how big the buffer for openconnect_get_cert_sha1 should be. You could add a comment explaining that, then this patch is good to go.

I've pushed that change, and it'll appear in the imminent OpenConnect 4.0 release:
http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/ce8df71d2a

I'm working on improved documentation for the library, which started off as purely internal code with the GNOME auth-dialog being built as part of the openconnect package itself. Does it show? :)

Thanks.

- David

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105185/#review14719
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120614/9c8f15d4/attachment-0001.html>

Lamarque Vieira Souza

2012-06-14 14:06:28 UTC

Permalink

Post by David Woodhouse

http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/ce8df71d2a
I'm working on improved documentation for the library, which started off as purely internal code with the GNOME auth-dialog being built as part of the openconnect package itself. Does it show? :)
Thanks.

Ok, that is enough for me. Ship it :)

- Lamarque Vieira

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105185/#review14719
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120614/e4732da0/attachment.html>

Lamarque Vieira Souza

2012-06-14 14:07:03 UTC

Permalink

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120614/66bedb63/attachment.html>

David Woodhouse

2012-06-15 00:17:56 UTC

Permalink

Post by David Woodhouse

Post by Lamarque Vieira Souza
Ship It!

Thanks for the review. I'm not quite sure how to interpret the "Ship it!"... I don't have commit privs so if you could commit the patch or pull it from git://git.infradead.org/users/dwmw2/networkmanagement.git (or tell me what I need to do next) that would be much appreciated. (The repo has the same patch in both master and nm09 branches, and can be browsed by s/git/http/ in the URL)

- David

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105185/#review14738
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120615/4e417e25/attachment.html>

Commit Hook

2012-06-15 00:39:06 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105185/#review14749
-----------------------------------------------------------

This review has been submitted with commit 761233bc7c3f6d3c28e1ca5dbdc24b57a962cda0 by David Woodhouse to branch master.

- Commit Hook

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120615/26541c40/attachment-0001.html>

Commit Hook

2012-06-15 00:39:42 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105185/#review14750
-----------------------------------------------------------

This review has been submitted with commit 75dd611f94db439745332320f4734e9d7a715483 by David Woodhouse to branch nm09.

- Commit Hook

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120615/abd1f1c1/attachment.html>