[PATCH 0/4] Create and use an auth token object
by Simo Sorce
The current way we handle with auth token is manual and very error prone.
The semanthics are also confusing and do not make clear how tokens are stored
such that manipulating them is difficult. For example it was unclar in the
code whether password tokens where 0 terminated and whether the length would
incliude the null termination byte or not.
This code creates a standard structure called sss_auth_token that has a full
set of getters and setters.
Simo.
Note: I wanted to make this structure completely opaque but it would have
required a lot more allocations and pointers, and made the patchset larger.
Fixes: https://fedorahosted.org/sssd/ticket/1586
Simo Sorce (4):
Code can only check for cached passwords
Add function to safely wipe memory.
Add authtok utility functions.
Change pam data auth tokens.
Makefile.am | 4 +
src/db/sysdb.h | 3 +-
src/db/sysdb_ops.c | 13 +-
src/providers/data_provider.h | 9 +-
src/providers/dp_auth_util.c | 77 ++++++----
src/providers/dp_pam_data_util.c | 113 ++++++++-------
src/providers/ipa/ipa_auth.c | 6 +-
src/providers/krb5/krb5_auth.c | 45 +++---
src/providers/krb5/krb5_child.c | 148 ++++++++++---------
src/providers/krb5/krb5_child_handler.c | 59 ++++++--
.../krb5/krb5_delayed_online_authentication.c | 50 ++++---
src/providers/krb5/krb5_renew_tgt.c | 18 +--
src/providers/ldap/ldap_auth.c | 90 +++++-------
src/providers/ldap/sdap_async.c | 4 +-
src/providers/ldap/sdap_async.h | 7 +-
src/providers/ldap/sdap_async_connection.c | 85 ++++++-----
src/providers/proxy/proxy.h | 7 +-
src/providers/proxy/proxy_auth.c | 14 +-
src/providers/proxy/proxy_child.c | 51 ++++---
src/responder/pam/pam_LOCAL_domain.c | 52 +++----
src/responder/pam/pamsrv_cmd.c | 159 ++++++++++++---------
src/tests/krb5_child-test.c | 13 +-
src/tests/sysdb-tests.c | 6 +-
src/util/authtok.c | 146 +++++++++++++++++++
src/util/authtok.h | 137 ++++++++++++++++++
src/util/util.c | 9 ++
src/util/util.h | 10 ++
27 files changed, 856 insertions(+), 479 deletions(-)
create mode 100644 src/util/authtok.c
create mode 100644 src/util/authtok.h
--
1.7.11.4
11 years, 3 months
On talloc_new(NULL) (ab)use in SSSD
by Simo Sorce
I've always had a beef with the use of this construct in recent sssd
code:
fn()
{
tmp_ctx = talloc_new(NULL);
use tmp_ctx ...
steal stuff on mem_ctx;
talloc_free(tmp_ctx);
return
}
The main reason this style was adopted is the claim that using
allocation on NULL makes it easier to valgrind stuff and find leaks, and
that leaks are easier to spot than huge memory growths.
I agree that this code makes that specific task easier, but it comes at
a cost, a cost we always bear while instead this code is useufl only
once in a while when we run valgrind.
It makes memory leaks much more dangerous, because if you forget to free
tmp_ctx it really is leaked
It requires a malloc() for even tiny functions and the context on it's
own uses quite a bit of space (iirc 80 bytes or so + malloc overhead).
Malloc is not free and can be quite expensive when some library tricks
glibc to go in paranoid threadsafe mode (which is surprisingly easy to
do) and mutexes start to be used for every allocation/free.
I'd like to propose a compromise going forward where we use a couple of
macros to avoid the cost of a temp context when not necessary.
These are the macros to me called when we enter and exit the functions:
SSS_MEM_CTX(loc_ctx, main_ctx) do { \
if (debug_level & SSSDBG_TRACE_INTERNAL) { \
loc_ctx = talloc_new(NULL); \
} else { \
loc_ctx = main_ctx; \
} \
} while(0)
SSS_FREE_MEM_CTX(loc_ctx, main_ctx) \
if (loc_ctx != main_ctx) talloc_free(loc_ctx)
Basically what we do here is that if we set debug level to
SSSDBG_TRACE_INTERNAL then we create brand new contexts every time,
otherwise we use the passed in memory context and save the overhead.
This is just to avoid the useless overhead and avoid minor mem leaks in
actual production when they happen (as the memory will eventually be
freed by the main context) but still allow for them to be foiund easily
with valgrind by simply setting the debug level to SSSDBG_TRACE_INTERNAL
when running under valgrind.
IF the function needs a temporary memory context because it handles a
lot of memory allocations for stuff it need not return it would *Still*
use a local tmp_ctx, but it would be something like a local structure
then explicitly freed or worst case tmp_ctx = talloc_new(loc_ctx) [ NOT
NULL! ].
You also keep stealing from loc_ctx to main_ctx, it will be a no-op when
they coincide.
So this would be an example:
CURRENT STYLE:
char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
{
TALLOC_CTX *tmp_ctx;
char *bar;
tmp_ctx = talloc_new(NULL);
bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
talloc_steal(mem_ctx, bar);
talloc_free(tmp_ctx);
return bar;
}
NEW STYLE:
char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
{
TALLOC_CTX *loc_ctx;
char *bar;
SSS_MEM_CTX(loc_ctx, mem_ctx);
bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
talloc_steal(mem_ctx, bar);
SSS_FREE_MEM_CTX(loc_ctx, mem_ctx);
return bar;
}
Discuss.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11 years, 3 months
[PATCH] Fix faulty usn reinit
by Simo Sorce
While looking at some code my eye fell on the fact that sdap_reinit.c
was including sysdb_private.h
That's a no-no on its own, you don't get to use private headers
liberally, or I wouldn't have marked them "private" in the first place!
However besides the abuse of the private headers I found also that the
function was broken because it wasn't doing what it was trying to do
(limit cleanups to users, groups and services).
Instead it would search the whole tree (3 times) and later remove all
entries w/o a USN.
I think this could cause the code to remove *everything* not directly
downloaded from the IPA tree (for example subdomain users) that lacks
the SYSDB_USN attribute for example.
I haven't tested the patch yet tbh, but I do not have the setup right
now, if someone has a 2 servers setup ready and can force sssd to
reconnect to the second and step through the cleanup to make sure it
runs as it should I would be grateful.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11 years, 3 months
[PATCH] AD: Add user as a direct member of his primary group
by Jakub Hrozek
There is one quirk that comes with using tokenGroups - the user is
reported as a member of groups that are parent groups of his primary
SID/GID as well. These group memberships are reflected in the database
with member/memberof links.
Later on, when the groups are resolved using LDAP calls (for instance
during id $user or simply getent group), the member/memberof links are
lost and not established until another initgroups call.
The attached patch changes the behaviour of the AD provider (and AD
provider only) such that when saving groups, a direct member link is
also established between the user and the group with GID same as user's
primary GID number. Then, the ghost/member links are propagated in the
hierarchy.
There is one unwanted side-effect of this change - the user would be
returned as a member of his primary group, too, when running
getgrnam(primary-group). If these patches were accepted, we'd have to
document this change in a manual page.
11 years, 3 months
[PATCH] Add a default section to a swtich-statement
by Sumit Bose
Hi,
a switch statement which was introduced with "Remove unhelpful vtable
from sss_cache" has a missing default section which results in a
compiler warning about an uninitialized variable. The attached patch
should fix it.
bye,
Sumit
11 years, 3 months