For a long time I felt constrained using just the error values that
are available in system headers, they do not express the full range
of errors we need to express within SSSD.
So this is a RFC patchset that demonstrates how we could about getting
a richer set of error messages for SSSD that should also simplify some
of our most convoluted code when it comes to returning errors.
The first patch just introduces a very simple error number space and 2
basic errors we always missed. ERR_INTERNAL is what we wanted to use in
many places where we currently return EIO or EFAULT for example.
Patch 2 shows how we can dramatically improve error reporting for our
offline caching authentication code with just a few more targeted errors.
The main thing to note here is that now we can safely distinguish from
a real system error about a missing file (ENOENT) and actually missing
Patch 3 is just an example of fixing EIO in a commonly used macro.
Patch 4 is the most complex example of converting a whole subsystem
that is really complex in its error handling.
Even after the recent refactoring patchesi, krb5_child error reporting
remained quite complex as it uses all of errno, krb5 and pam status
The result is that we have to track multiple different error variables
and that can easily lead to hard to read code as well as bugs where one
error code does not reflect into others.
With this patch I think we show how we are able to reduce the complexity
of error handling and at the same time return precise information on what
NOTE: This patchset depends on my previous patches for refactoring the
krb5_child as well as the patches that kix the krb5_auth style and the
patches that introduce the authtoken structure.
Simo Sorce (4):
Add SSSD specific error codes and definitions
Use SSSD specific errors for offline auth
Return ERR_INTERNAL instead of EIO
Cleanup error message handling for krb5 child
Makefile.am | 4 +-
src/db/sysdb_ops.c | 17 +-
src/providers/krb5/krb5_auth.c | 157 +++++++++-------
src/providers/krb5/krb5_child.c | 388 ++++++++++++++++-----------------------
src/tests/auth-tests.c | 6 +-
src/tests/sysdb-tests.c | 12 +-
src/util/auth_utils.h | 22 ++-
src/util/util.h | 10 +-
src/util/util_errors.c | 50 +++++
src/util/util_errors.h | 85 +++++++++
10 files changed, 417 insertions(+), 334 deletions(-)
create mode 100644 src/util/util_errors.c
create mode 100644 src/util/util_errors.h
There was another bug in pam_pwd_expiration warning. If it was set to 0
which means "always display whatever server sends", the condition
wouldn't hit and effectivelly the option would be set to -1 (=use
since the SSSD development started, the codebase used only error codes
from the standard errno range (see man errno.h). Recently it has became
apparent that it's not really sufficient -- the errno codes are too tied
to glibc interface and system calls and do not really reflect what is
going on in the SSSD. Quite often, we would misuse "EIO" or "EFAULT" for
"something is wrong".
There are cases where we want to accuratelly report the particular error
code to an external interface (such as pam_sss). Previously we would just
special case some errno codes, but that turned out to be too buggy and
hard to follow. See upstream bugs like #1310 or #1699 for an example.
To solve this issue, Simo proposed introducing an internal range or SSSD
specific error codes:
I think his patches are very nice in general. I reviewed the patches in
person this weekend on http://devconf.cz and when I run some more tests
I'd like to push them.
To sum up our discussion for the other developers, here are some guidelies
to follow. In general, after the patches are pushed, the new code should
use the SSSD specific error codes, not errno internally.
* If the current error codes do not fit, the developer should
introduce new ones. The new error codes should be specifically
called out in the commit message or even split into a separate
patch as appropriate. In general, the error codes should converge
over time to a stable set that wouldn't grow any more.
* When introducing new error codes, the developer should make sure
that consumers of the interface being introduced or converted
does not special case some old errno-codes (we often
special-case EAGAIN to denote "request still in progress" for
* the error codes would not be split into any ranges. If the
caller needs to check N error codes, it should check N separate
error codes (or refactored)
Please raise any concerns you might have over the plan.
This was a though one, but here it is at last.
You can test it nicely with the autofs test client. If you have many
mount points in one map (I was operating with 323), you can see that
number of returned entries to the client is random.
-----BEGIN PGP SIGNED MESSAGE-----
There is a large amount of duplicated code being linked into multiple
SSSD binaries. Instead of statically linking this code throughout the
SSSD, we should instead create private shared libraries for them and
drop this code on the system only once.
The installed size of the Fedora RPMs decreased from 13MB to 5.1MB.
This is definitely a significant gain, both in disk space and memory
usage (since the code will only need to be loaded into memory once).
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
-----END PGP SIGNATURE-----
other than using kinit -r afterwards, how can sssd/pam be configured to invoke kinit -r
Before migrated to IPA, I install kstart for kerberos clients and run krenew -K 60 to have
krenew checking every 60 minutes in the background to extend ticket when due to expire.
It does not work with IPA client because it seems IPA client can not obtain a renewable
ticket by default at login. Can this be changed?
Senior Systems Administrator
M6-624 Research Computing
Sunnybrook Health Sciences Centre
2075 Bayview Ave.
Toronto, Ontario, M4N 3M5
(416) 480-6100 x3263
While working on a different refactoring patch I came across the
krb5_child code and found it very hard to read and follow due to the
growth it has gone through in the past.
I couldn't force myself not to refactor it to make it more linear and
easier to follow and read. A bunch of dead stuff that really confused me
initially also got removed.
(I propose this for master only)
Simo Sorce * Red Hat, Inc * New York