PR#13: Try harder to avoid password change replay errors and use p11-kit as default PKCS11 module [rawhide] - rpms/krb5

rpms / krb5

#13 Try harder to avoid password change replay errors and use p11-kit as default PKCS11 module [rawhide]

Merged 2 years ago by abbra. Opened 2 years ago by jrische.

rpms/ jrische/krb5 rawhide into rawhide

Use p11-kit as default PKCS11 module

Julien Rische • 2 years ago

c25a51c

Try harder to avoid password change replay errors

Julien Rische • 2 years ago

0451384

Add-configure-variable-for-default-PKCS-11-module.patch

file added

+201

		`@@ -0,0 +1,201 @@`
		`+ From 2a91dabd9752825b96faf3b25ea643d5282c5957 Mon Sep 17 00:00:00 2001`
		`+ From: Julien Rische <jrische@redhat.com>`
		`+ Date: Fri, 22 Apr 2022 14:12:37 +0200`
		`+ Subject: [PATCH] Add configure variable for default PKCS#11 module`
		`+`
		`+ [ghudson@mit.edu: added documentation of configure variable and doc`
		`+ substitution; shortened commit message]`
		`+`
		`+ ticket: 9058 (new)`
		`+ ---`
		`+ doc/admin/conf_files/krb5_conf.rst \| 2 +-`
		`+ doc/build/options2configure.rst \| 3 +++`
		`+ doc/conf.py \| 3 +++`
		`+ doc/mitK5defaults.rst \| 25 +++++++++++++------------`
		`+ src/configure.ac \| 8 ++++++++`
		`+ src/doc/Makefile.in \| 2 ++`
		`+ src/man/Makefile.in \| 4 +++-`
		`+ src/man/krb5.conf.man \| 2 +-`
		`+ src/plugins/preauth/pkinit/pkinit.h \| 1 -`
		`+ 9 files changed, 34 insertions(+), 16 deletions(-)`
		`+`
		`+ diff --git a/doc/admin/conf_files/krb5_conf.rst b/doc/admin/conf_files/krb5_conf.rst`
		`+ index adba8238d..3d25c9a12 100644`
		`+ --- a/doc/admin/conf_files/krb5_conf.rst`
		`+ +++ b/doc/admin/conf_files/krb5_conf.rst`
		`+ @@ -1020,7 +1020,7 @@ information for PKINIT is as follows:`
		`+ All keyword/values are optional. modname specifies the location`
		`+ of a library implementing PKCS #11. If a value is encountered`
		`+ with no keyword, it is assumed to be the modname. If no`
		+ - module-name is specified, the default is ``opensc-pkcs11.so``.
		`+ + module-name is specified, the default is \|pkcs11_modname\|.`
		+ ``slotid=`` and/or ``token=`` may be specified to force the use of
		`+ a particular smard card reader or token if there is more than one`
		+ available. ``certid=`` and/or ``certlabel=`` may be specified to
		`+ diff --git a/doc/build/options2configure.rst b/doc/build/options2configure.rst`
		`+ index a8959626d..8f8ac911c 100644`
		`+ --- a/doc/build/options2configure.rst`
		`+ +++ b/doc/build/options2configure.rst`
		`+ @@ -143,6 +143,9 @@ Environment variables`
		`+ This option allows one to specify libraries to be passed to the`
		+ linker (e.g., ``-l<library>``)
		`+`
		`+ +PKCS11_MODNAME=\ library`
		`+ + Override the built-in default PKCS11 library name.`
		`+ +`
		`+ SS_LIB=\ libs...`
		+ If ``-lss`` is not the correct way to link in your installed ss
		`+ library, for example if additional support libraries are needed,`
		`+ diff --git a/doc/conf.py b/doc/conf.py`
		`+ index a876fd633..252ab891a 100644`
		`+ --- a/doc/conf.py`
		`+ +++ b/doc/conf.py`
		`+ @@ -242,6 +242,7 @@ if 'mansubs' in tags:`
		+ ccache = '``@CCNAME@``'
		+ keytab = '``@KTNAME@``'
		+ ckeytab = '``@CKTNAME@``'
		+ + pkcs11_modname = '``@PKCS11MOD@``'
		`+ elif 'pathsubs' in tags:`
		`+ # Read configured paths from a file produced by the build system.`
		`+ exec(open("paths.py").read())`
		`+ @@ -255,6 +256,7 @@ else:`
		+ ccache = ':ref:`DEFCCNAME <paths>`'
		+ keytab = ':ref:`DEFKTNAME <paths>`'
		+ ckeytab = ':ref:`DEFCKTNAME <paths>`'
		+ + pkcs11_modname = ':ref:`PKCS11_MODNAME <paths>`'
		`+`
		`+ rst_epilog = '\n'`
		`+`
		`+ @@ -275,6 +277,7 @@ else:`
		`+ rst_epilog += '.. \|ccache\| replace:: %s\n' % ccache`
		`+ rst_epilog += '.. \|keytab\| replace:: %s\n' % keytab`
		`+ rst_epilog += '.. \|ckeytab\| replace:: %s\n' % ckeytab`
		`+ + rst_epilog += '.. \|pkcs11_modname\| replace:: %s\n' % pkcs11_modname`
		`+ rst_epilog += '''`
		+ .. \|krb5conf\| replace:: ``/etc/krb5.conf``
		+ .. \|defkeysalts\| replace:: ``aes256-cts-hmac-sha1-96:normal aes128-cts-hmac-sha1-96:normal``
		`+ diff --git a/doc/mitK5defaults.rst b/doc/mitK5defaults.rst`
		`+ index 74e69f4ad..aea7af3db 100644`
		`+ --- a/doc/mitK5defaults.rst`
		`+ +++ b/doc/mitK5defaults.rst`
		+ @@ -59,18 +59,19 @@ subdirectories of ``/usr/local``. When MIT krb5 is integrated into an
		`+ operating system, the paths are generally chosen to match the`
		`+ operating system's filesystem layout.`
		`+`
		`+ -========================== ============= =========================== ===========================`
		`+ -Description Symbolic name Custom build path Typical OS path`
		`+ -========================== ============= =========================== ===========================`
		+ -User programs BINDIR ``/usr/local/bin`` ``/usr/bin``
		+ -Libraries and plugins LIBDIR ``/usr/local/lib`` ``/usr/lib``
		+ -Parent of KDC state dir LOCALSTATEDIR ``/usr/local/var`` ``/var``
		+ -Parent of KDC runtime dir RUNSTATEDIR ``/usr/local/var/run`` ``/run``
		+ -Administrative programs SBINDIR ``/usr/local/sbin`` ``/usr/sbin``
		+ -Alternate krb5.conf dir SYSCONFDIR ``/usr/local/etc`` ``/etc``
		+ -Default ccache name DEFCCNAME ``FILE:/tmp/krb5cc_%{uid}`` ``FILE:/tmp/krb5cc_%{uid}``
		+ -Default keytab name DEFKTNAME ``FILE:/etc/krb5.keytab`` ``FILE:/etc/krb5.keytab``
		`+ -========================== ============= =========================== ===========================`
		`+ +========================== ============== =========================== ===========================`
		`+ +Description Symbolic name Custom build path Typical OS path`
		`+ +========================== ============== =========================== ===========================`
		+ +User programs BINDIR ``/usr/local/bin`` ``/usr/bin``
		+ +Libraries and plugins LIBDIR ``/usr/local/lib`` ``/usr/lib``
		+ +Parent of KDC state dir LOCALSTATEDIR ``/usr/local/var`` ``/var``
		+ +Parent of KDC runtime dir RUNSTATEDIR ``/usr/local/var/run`` ``/run``
		+ +Administrative programs SBINDIR ``/usr/local/sbin`` ``/usr/sbin``
		+ +Alternate krb5.conf dir SYSCONFDIR ``/usr/local/etc`` ``/etc``
		+ +Default ccache name DEFCCNAME ``FILE:/tmp/krb5cc_%{uid}`` ``FILE:/tmp/krb5cc_%{uid}``
		+ +Default keytab name DEFKTNAME ``FILE:/etc/krb5.keytab`` ``FILE:/etc/krb5.keytab``
		+ +Default PKCS11 module PKCS11_MODNAME ``opensc-pkcs11.so`` ``opensc-pkcs11.so``
		`+ +========================== ============== =========================== ===========================`
		`+`
		`+ The default client keytab name (DEFCKTNAME) typically defaults to`
		+ ``FILE:/usr/local/var/krb5/user/%{euid}/client.keytab`` for a custom
		`+ diff --git a/src/configure.ac b/src/configure.ac`
		`+ index 82b049af9..52e6563da 100644`
		`+ --- a/src/configure.ac`
		`+ +++ b/src/configure.ac`
		`+ @@ -1442,6 +1442,14 @@ AC_DEFINE_UNQUOTED(DEFKTNAME, ["$DEFKTNAME"], [Define to default keytab name])`
		`+ AC_DEFINE_UNQUOTED(DEFCKTNAME, ["$DEFCKTNAME"],`
		`+ [Define to default client keytab name])`
		`+`
		`+ +AC_ARG_VAR(PKCS11_MODNAME, [Default PKCS11 module name])`
		`+ +if test "${PKCS11_MODNAME+set}" != set; then`
		`+ + PKCS11_MODNAME=opensc-pkcs11.so`
		`+ +fi`
		`+ +AC_MSG_NOTICE([Default PKCS11 module name: $PKCS11_MODNAME])`
		`+ +AC_DEFINE_UNQUOTED(PKCS11_MODNAME, ["$PKCS11_MODNAME"],`
		`+ + [Default PKCS11 module name])`
		`+ +`
		`+ AC_CONFIG_FILES([build-tools/krb5-config], [chmod +x build-tools/krb5-config])`
		`+ AC_CONFIG_FILES([build-tools/kadm-server.pc`
		`+ build-tools/kadm-client.pc`
		`+ diff --git a/src/doc/Makefile.in b/src/doc/Makefile.in`
		`+ index 379bc3651..a1b0cff0a 100644`
		`+ --- a/src/doc/Makefile.in`
		`+ +++ b/src/doc/Makefile.in`
		`+ @@ -10,6 +10,7 @@ sysconfdir=@sysconfdir@`
		`+ DEFCCNAME=@DEFCCNAME@`
		`+ DEFKTNAME=@DEFKTNAME@`
		`+ DEFCKTNAME=@DEFCKTNAME@`
		`+ +PKCS11_MODNAME=@PKCS11_MODNAME@`
		`+`
		`+ RST_SOURCES= _static \`
		`+ _templates \`
		`+ @@ -118,6 +119,7 @@ paths.py:`
		+ echo 'ccache = "``$(DEFCCNAME)``"' >> $@
		+ echo 'keytab = "``$(DEFKTNAME)``"' >> $@
		+ echo 'ckeytab = "``$(DEFCKTNAME)``"' >> $@
		+ + echo 'pkcs11_modname = "``$(PKCS11_MODNAME)``"' >> $@
		`+`
		`+ # Dummy rule that man/Makefile can invoke`
		`+ version.py: $(docsrc)/version.py`
		`+ diff --git a/src/man/Makefile.in b/src/man/Makefile.in`
		`+ index 00b1b2de0..85cae0914 100644`
		`+ --- a/src/man/Makefile.in`
		`+ +++ b/src/man/Makefile.in`
		`+ @@ -8,6 +8,7 @@ sysconfdir=@sysconfdir@`
		`+ DEFCCNAME=@DEFCCNAME@`
		`+ DEFKTNAME=@DEFKTNAME@`
		`+ DEFCKTNAME=@DEFCKTNAME@`
		`+ +PKCS11_MODNAME=@PKCS11_MODNAME@`
		`+`
		`+ MANSUBS=k5identity.sub k5login.sub k5srvutil.sub kadm5.acl.sub kadmin.sub \`
		`+ kadmind.sub kdb5_ldap_util.sub kdb5_util.sub kdc.conf.sub \`
		`+ @@ -47,7 +48,8 @@ $(docsrc)/version.py: $(top_srcdir)/patchlevel.h`
		`+ -e 's\|@SYSCONFDIR@\|$(sysconfdir)\|g' \`
		`+ -e 's\|@CCNAME@\|$(DEFCCNAME)\|g' \`
		`+ -e 's\|@KTNAME@\|$(DEFKTNAME)\|g' \`
		`+ - -e 's\|@CKTNAME@\|$(DEFCKTNAME)\|g' $? > $@`
		`+ + -e 's\|@CKTNAME@\|$(DEFCKTNAME)\|g' \`
		`+ + -e 's\|@PKCS11MOD@\|$(PKCS11_MODNAME)\|g' $? > $@`
		`+`
		`+ all: $(MANSUBS)`
		`+`
		`+ diff --git a/src/man/krb5.conf.man b/src/man/krb5.conf.man`
		`+ index e993d5c09..42f5ea4f9 100644`
		`+ --- a/src/man/krb5.conf.man`
		`+ +++ b/src/man/krb5.conf.man`
		`+ @@ -1151,7 +1151,7 @@ user\(aqs certificate and private key.`
		`+ All keyword/values are optional. \fImodname\fP specifies the location`
		`+ of a library implementing PKCS #11. If a value is encountered`
		`+ with no keyword, it is assumed to be the \fImodname\fP\&. If no`
		`+ -module\-name is specified, the default is \fBopensc\-pkcs11.so\fP\&.`
		`+ +module\-name is specified, the default is \fB@PKCS11MOD@\fP\&.`
		`+ \fBslotid=\fP and/or \fBtoken=\fP may be specified to force the use of`
		`+ a particular smard card reader or token if there is more than one`
		`+ available. \fBcertid=\fP and/or \fBcertlabel=\fP may be specified to`
		`+ diff --git a/src/plugins/preauth/pkinit/pkinit.h b/src/plugins/preauth/pkinit/pkinit.h`
		`+ index b437fd53f..a2018cb10 100644`
		`+ --- a/src/plugins/preauth/pkinit/pkinit.h`
		`+ +++ b/src/plugins/preauth/pkinit/pkinit.h`
		`+ @@ -42,7 +42,6 @@`
		`+ #ifndef WITHOUT_PKCS11`
		`+ #include "pkcs11.h"`
		`+`
		`+ -#define PKCS11_MODNAME "opensc-pkcs11.so"`
		`+ #define PK_SIGLEN_GUESS 1000`
		`+ #define PK_NOSLOT 999999`
		`+ #endif`
		`+ --`
		`+ 2.35.1`
		`+`

Try-harder-to-avoid-password-change-replay-errors.patch

file added

+91

		`@@ -0,0 +1,91 @@`
		`+ From 1f706852ee759160e763c355a3053ad5e045fa06 Mon Sep 17 00:00:00 2001`
		`+ From: Greg Hudson <ghudson@mit.edu>`
		`+ Date: Fri, 4 Mar 2022 00:45:00 -0500`
		`+ Subject: [PATCH] Try harder to avoid password change replay errors`
		`+`
		`+ Commit d7b3018d338fc9c989c3fa17505870f23c3759a8 (ticket 7905) changed`
		`+ change_set_password() to prefer TCP. However, because UDP_LAST falls`
		`+ back to UDP after one second, we can still get a replay error due to a`
		`+ dropped packet, before the TCP layer has a chance to retry.`
		`+`
		`+ Instead, try k5_sendto() with NO_UDP, and only fall back to UDP after`
		`+ TCP fails completely without reaching a server. In sendto_kdc.c,`
		`+ implement an ONLY_UDP transport strategy to allow the UDP fallback.`
		`+`
		`+ ticket: 9037`
		`+ ---`
		`+ src/lib/krb5/os/changepw.c \| 9 ++++++++-`
		`+ src/lib/krb5/os/os-proto.h \| 1 +`
		`+ src/lib/krb5/os/sendto_kdc.c \| 12 ++++++++----`
		`+ 3 files changed, 17 insertions(+), 5 deletions(-)`
		`+`
		`+ diff --git a/src/lib/krb5/os/changepw.c b/src/lib/krb5/os/changepw.c`
		`+ index 9f968da7f..c59232586 100644`
		`+ --- a/src/lib/krb5/os/changepw.c`
		`+ +++ b/src/lib/krb5/os/changepw.c`
		`+ @@ -255,9 +255,16 @@ change_set_password(krb5_context context,`
		`+ callback_info.pfn_cleanup = kpasswd_sendto_msg_cleanup;`
		`+ krb5_free_data_contents(callback_ctx.context, &chpw_rep);`
		`+`
		`+ + /* UDP retransmits may be seen as replays. Only try UDP after other`
		`+ + * transports fail completely. */`
		`+ code = k5_sendto(callback_ctx.context, NULL, &creds->server->realm,`
		`+ - &sl, UDP_LAST, &callback_info, &chpw_rep,`
		`+ + &sl, NO_UDP, &callback_info, &chpw_rep,`
		`+ ss2sa(&remote_addr), &addrlen, NULL, NULL, NULL);`
		`+ + if (code == KRB5_KDC_UNREACH) {`
		`+ + code = k5_sendto(callback_ctx.context, NULL, &creds->server->realm,`
		`+ + &sl, ONLY_UDP, &callback_info, &chpw_rep,`
		`+ + ss2sa(&remote_addr), &addrlen, NULL, NULL, NULL);`
		`+ + }`
		`+ if (code)`
		`+ goto cleanup;`
		`+`
		`+ diff --git a/src/lib/krb5/os/os-proto.h b/src/lib/krb5/os/os-proto.h`
		`+ index a985f2aec..91d2791ce 100644`
		`+ --- a/src/lib/krb5/os/os-proto.h`
		`+ +++ b/src/lib/krb5/os/os-proto.h`
		`+ @@ -49,6 +49,7 @@ typedef enum {`
		`+ UDP_FIRST = 0,`
		`+ UDP_LAST,`
		`+ NO_UDP,`
		`+ + ONLY_UDP`
		`+ } k5_transport_strategy;`
		`+`
		`+ /* A single server hostname or address. */`
		`+ diff --git a/src/lib/krb5/os/sendto_kdc.c b/src/lib/krb5/os/sendto_kdc.c`
		`+ index 0eedec175..c7f5d861a 100644`
		`+ --- a/src/lib/krb5/os/sendto_kdc.c`
		`+ +++ b/src/lib/krb5/os/sendto_kdc.c`
		`+ @@ -802,11 +802,14 @@ resolve_server(krb5_context context, const krb5_data *realm,`
		`+ int err, result;`
		`+ char portbuf[PORT_LENGTH];`
		`+`
		`+ - /* Skip UDP entries if we don't want UDP. */`
		`+ + /* Skip entries excluded by the strategy. */`
		`+ if (strategy == NO_UDP && entry->transport == UDP)`
		`+ return 0;`
		`+ + if (strategy == ONLY_UDP && entry->transport != UDP &&`
		`+ + entry->transport != TCP_OR_UDP)`
		`+ + return 0;`
		`+`
		`+ - transport = (strategy == UDP_FIRST) ? UDP : TCP;`
		`+ + transport = (strategy == UDP_FIRST \|\| strategy == ONLY_UDP) ? UDP : TCP;`
		`+ if (entry->hostname == NULL) {`
		`+ /* Added by a module, so transport is either TCP or UDP. */`
		`+ ai.ai_socktype = socktype_for_transport(entry->transport);`
		`+ @@ -850,8 +853,9 @@ resolve_server(krb5_context context, const krb5_data *realm,`
		`+ }`
		`+`
		`+ /* For TCP_OR_UDP entries, add each address again with the non-preferred`
		`+ - * transport, unless we are avoiding UDP. Flag these as deferred. */`
		`+ - if (retval == 0 && entry->transport == TCP_OR_UDP && strategy != NO_UDP) {`
		`+ + * transport, if there is one. Flag these as deferred. */`
		`+ + if (retval == 0 && entry->transport == TCP_OR_UDP &&`
		`+ + (strategy == UDP_FIRST \|\| strategy == UDP_LAST)) {`
		`+ transport = (strategy == UDP_FIRST) ? TCP : UDP;`
		`+ for (a = addrs; a != 0 && retval == 0; a = a->ai_next) {`
		`+ a->ai_socktype = socktype_for_transport(transport);`
		`+ --`
		`+ 2.35.1`
		`+`

krb5.spec

file modified

+10 -1

		`@@ -42,7 +42,7 @@`
		`Summary: The Kerberos network authentication system`
		`Name: krb5`
		`Version: 1.19.2`
		`- Release: %{?zdpd}9%{?dist}`
		`+ Release: %{?zdpd}10%{?dist}`

		`# rharwood has trust path to signing key and verifies on check-in`
		`Source0: https://web.mit.edu/kerberos/dist/krb5/%{version}/krb5-%{version}%{?dashpre}.tar.gz`
		`@@ -97,6 +97,8 @@`
		`Patch37: Use-SHA256-instead-of-SHA1-for-PKINIT-CMS-digest.patch`
		`Patch38: krb5-krad-remote.patch`
		`Patch39: krb5-krad-larger-attrs.patch`
		`+ Patch40: Try-harder-to-avoid-password-change-replay-errors.patch`
		`+ Patch41: Add-configure-variable-for-default-PKCS-11-module.patch`

		`License: MIT`
		`URL: https://web.mit.edu/kerberos/www/`
		`@@ -288,6 +290,7 @@`
		`CFLAGS="$CFLAGS" \`
		`CPPFLAGS="$CPPFLAGS" \`
		`SS_LIB="-lss" \`
		`+ PKCS11_MODNAME="p11-kit-proxy.so" \`
		`--enable-shared \`
		`--runstatedir=/run \`
		`--localstatedir=%{_var}/kerberos \`
		`@@ -646,6 +649,12 @@`
		`%{_libdir}/libkadm5srv_mit.so.*`

		`%changelog`
		`+ * Mon May 2 2022 Julien Rische <jrische@redhat.com> - 1.19.2-10`
		`+ - Use p11-kit as default PKCS11 module`
		`+ - Resolves: rhbz#2073274`
		`+ - Try harder to avoid password change replay errors`
		`+ - Resolves: rhbz#2072059`
		`+`
		`* Tue Apr 05 2022 Alexander Bokovoy <abokovoy@redhat.com> - 1.19.2-9`
		`- Fix libkrad client cleanup`
		`- Fixes rhbz#2072059`

jrische commented 2 years ago

change_set_password() was changed to prefer TCP. However, because
UDP_LAST falls back to UDP after one second, we can still get a replay
error due to a dropped packet, before the TCP layer has a chance to
retry.

Instead, try k5_sendto() with NO_UDP, and only fall back to UDP after
TCP fails completely without reaching a server. In sendto_kdc.c,
implement an ONLY_UDP transport strategy to allow the UDP fallback.

Resolves: rhbz#2076965

Use p11-kit as a generic layer in front of PKCS11 implementation. This allows the user to switch to another smartcard support library without having to update the Kerberos configuration.

Resolves: rhbz#2073274

abbra commented 2 years ago

Test failed on ppc64le: https://kojipkgs.fedoraproject.org//work/tasks/7056/85977056/build.log

jrische commented 2 years ago

I have run a scratch build on ppc64le again, and tests are passing this time:
https://kojipkgs.fedoraproject.org/work/tasks/8165/86068165/build.log

abbra commented 2 years ago

Can you please force-push a patch (just try to do git commit --amend and change commit message, for example) so that a new build is triggered?

rebased onto 0451384

2 years ago

1 new commit added

Use p11-kit as default PKCS11 module

2 years ago

2 new commits added

Use p11-kit as default PKCS11 module
Try harder to avoid password change replay errors

2 years ago

abbra commented 2 years ago

Since we didn't have -10.fc37 built at all, could you please fold both changes under -10 instead of having individual changelog entries? These changes are part of the same merge request so no need to differentiate them.

2 new commits added