Issue 8753 - Public key pinning support in libldap
Summary: Public key pinning support in libldap
Status: VERIFIED FIXED
Alias: None
Product: OpenLDAP
Classification: Unclassified
Component: libraries (show other issues)
Version: unspecified
Hardware: All All
: --- normal
Target Milestone: 2.5.0
Assignee: Ondřej Kuzník
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-10 10:43 UTC by Ondřej Kuzník
Modified: 2022-02-21 11:48 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Ondřej Kuzník 2017-10-10 10:43:41 UTC
Full_Name: Ondrej Kuznik
Version: master
OS: 
URL: https://github.com/mistotebe/openldap/tree/its8753
Submission from: (NULL) (82.10.24.68)


Some programs might want to pin the server's public key instead of/in addition
to certificate validation. The patch linked implements this option and provides
OpenSSL/GnuTLS support code.

A new libldap option LDAP_OPT_X_TLS_PEERKEY_HASH that accepts a string
'hashname/base64_hash_of_public_key'. If a TLS session is already present on the
main connection, it is also checked immediately.

It introduces a dependency on liblutil by depending on the symbol
lutil_b64_pton. Somehow, this breaks the build for the ldap* tools, not sure why
or how to fix that yet.
Comment 1 Ondřej Kuzník 2017-11-07 18:44:14 UTC
On Tue, Oct 10, 2017 at 10:43:42AM +0000, ondra@openldap.org wrote:
> URL: https://github.com/mistotebe/openldap/tree/its8753
> 
> A new libldap option LDAP_OPT_X_TLS_PEERKEY_HASH that accepts a string
> 'hashname/base64_hash_of_public_key'. If a TLS session is already present on the
> main connection, it is also checked immediately.
> 
> It introduces a dependency on liblutil by depending on the symbol
> lutil_b64_pton. Somehow, this breaks the build for the ldap* tools, not sure why
> or how to fix that yet.

A new version is now at the same place (see above), it moves the ldif.c
in-place base64 decoding into a separate function and reuses that.

Other changes:
- pin hash algorithm separator changes to ':'
- pin can now be set from the environment
- can now better deal with connection freeing and/or changes to the
  global ldap options

-- 
Ondřej Kuzník
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

Comment 2 Ryan Tandy 2019-08-26 22:18:49 UTC
The gnutls_digest_get_id function was added in GnuTLS 3.2.2:

https://gitlab.com/gnutls/gnutls/blob/gnutls_3_2_2/NEWS

That was released in 2013, so I think it's OK to depend on it by now.  
Please consider applying this patch to update the configure.in check:

https://github.com/openldap/openldap/compare/master...rtandy:its8753-gnutls-version.patch

Currently master fails to compile with older GnuTLS (e.g. on Debian 
7/wheezy) after configure succeeded.

Comment 3 Quanah Gibson-Mount 2020-03-23 17:14:43 UTC
Ondrej,

Can you look at comment#2 and handle accordingly if anything is left to be done here? thanks!
Comment 4 Quanah Gibson-Mount 2020-03-23 18:09:09 UTC
comment #2 was committed in 67f81dccc8ac4ad171f21d5719ada69717b57810
Comment 5 Quanah Gibson-Mount 2022-02-18 23:21:01 UTC
head:

  • a2a2ebba 
by Ondřej Kuzník at 2022-02-14T20:32:29+00:00 
ITS#8753 Document LDAP_OPT_X_TLS_PEERKEY_HASH
Comment 6 Quanah Gibson-Mount 2022-02-18 23:23:14 UTC
RE26:

  • 394f6ad5 
by Ondřej Kuzník at 2022-02-18T23:18:31+00:00 
ITS#8753 Document LDAP_OPT_X_TLS_PEERKEY_HASH
Comment 7 Quanah Gibson-Mount 2022-02-18 23:23:48 UTC
RE25:

  • 680affe6 
by Ondřej Kuzník at 2022-02-18T23:20:09+00:00 
ITS#8753 Document LDAP_OPT_X_TLS_PEERKEY_HASH
Comment 8 Michael Ströder 2022-02-19 08:26:47 UTC
What are valid values or is the format of the <hashalg> field?
Comment 9 Michael Ströder 2022-02-19 09:25:48 UTC
(In reply to Michael Ströder from comment #8)
> What are valid values or is the format of the <hashalg> field?

It seems crypto(7) should be referenced by ldap_set_option(3) in case of OpenSSL?

https://www.openssl.org/docs/manmaster/man7/crypto.html
Comment 10 Michael Ströder 2022-02-19 11:31:30 UTC
Is the key hash calculated over the raw public key? In which representation?

Why not use the TLS server cert's finger-print?
Comment 11 Ondřej Kuzník 2022-02-21 10:27:00 UTC
How about https://git.openldap.org/ondra/openldap/-/commit/e020f3ba26fd6d42ce0f91299b77ce2fbe1e7da0

It should be analogous to HTTP Public Key Pinning, that's why it's
working with keys, not certificates.
Comment 12 Michael Ströder 2022-02-21 10:40:01 UTC
(In reply to Ondřej Kuzník from comment #11)
> It should be analogous to HTTP Public Key Pinning, that's why it's
> working with keys, not certificates.

Ah, ok.

For python-ldap0 tests I've used for generation the SHA-256 hash:

openssl rsa -in tests/tls/localhost.key -outform der -pubout | openssl dgst -sha256 -binary | openssl enc -base64

But it does not work (with libldap 2.6.1):

ldap0.CONNECT_ERROR: {'result': -11, 'desc': b'Connect error', 'ctrls': [], 'info': b'error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed (self signed certificate in certificate chain)'}

See the (commented) lines in the test:

https://code.stroeder.com/pymod/python-ldap0/src/branch/main/tests/test_ldapobject.py#L1031

Assuming I got this right:

https://code.stroeder.com/pymod/python-ldap0/commit/1ec4ad7ada7388835d5df8c8dfe60791debaa8d0
Comment 13 Michael Ströder 2022-02-21 10:46:12 UTC
On 2/21/22 11:40, openldap-its@openldap.org wrote:
> See the (commented) lines in the test:
> https://code.stroeder.com/pymod/python-ldap0/src/branch/main/tests/test_ldapobject.py#L1031

Ok, I've looked into the tests for TLS_PEERKEY_HASHALG to make it work.

=> The correct values for hashalgo should be described in the man-page.
Comment 14 Ondřej Kuzník 2022-02-21 11:48:42 UTC
On Mon, Feb 21, 2022 at 10:46:12AM +0000, openldap-its@openldap.org wrote:
> => The correct values for hashalgo should be described in the man-page.

Since this depends entirely on the crypto library at runtime, not sure
how we could do any better than saying "it depends", which is what I did
in that linked commit, now at https://git.openldap.org/openldap/openldap/-/merge_requests/499

Can you suggest an alternate wording you think explains it better?

Thanks,