[PATCH] Split the providers into separate subpackages
by Jakub Hrozek
Hi,
the attached patch splits the previously monolithic sssd package into
sssd-common that contains the deamon and the responders and per-provider
packages such as sssd-ldap or sssd-ipa.
This split would benefit two parties:
1) security auditors who are often trying to find the smallest package
set including dependencies needed for the package to function. They
would be able to i.e. install sssd-ldap and not bother about
sssd-ipa or sssd-ad pulling in more dependencies.
2) 3rd party programs such as realmd or authconfig that would only
be able to require or install on demand the needed packages.
The patch addresses https://fedorahosted.org/sssd/ticket/1510 and must b
applied on the two specfile patches I sent earlier (the thread subject
included libsss_sudo).
9 years, 11 months
[PATCH] Alignment issues reported by clang.
by Michal Židek
https://fedorahosted.org/sssd/ticket/1359
This patch mostly silences some alignment related warnings reported by
clang, but also fixes some real alignment issues.
But not all warnings are suppressed in this patch. These cases still
generate warnings:
1)=====================================================
src/util/refcount.c:63:25: warning: cast from 'char *' to 'int *'
increases required alignment from 1 to 4 [-Wcast-align]
wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/util/refcount.c:82:25: warning: cast from 'char *' to 'int *'
increases required alignment from 1 to 4 [-Wcast-align]
wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment: This is not a problem, the data we access here is an integer
that is part of a structure, so there should be no alignment issues (C
compiler would not create a structure with inaccessible structure
member). I think suppressing this warning with some additional casts
would only confuse us later and make the code more obfuscated than it is.
2)=====================================================
src/providers/ldap/sdap_async_sudo_hostinfo.c:233:24: warning: cast from
'struct sockaddr *' to 'struct sockaddr_in *' increases
required alignment from 2 to 4 [-Wcast-align]
ip4_addr = (struct sockaddr_in*)(iface->ifa_addr);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/providers/ldap/sdap_async_sudo_hostinfo.c:267:27: warning: cast from
'struct sockaddr *' to 'struct sockaddr_in6 *' increases
required alignment from 2 to 4 [-Wcast-align]
ip6_network = (struct sockaddr_in6*)(iface->ifa_netmask);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...and other sockaddr related casts...
Comment: We only have to make sure that sockaddr is not allocated on
stack here. But we use the getifaddrs() function for this, so it should
be OK.
3)=====================================================
src/sss_client/nss_services.c:137:29: warning: cast from 'char *' to
'char **' increases required alignment from 1 to 8
[-Wcast-align]
sr->result->s_aliases = (char **) &(sr->buffer[i+pad]);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment: Well, there is nothing we can do about this warning. We just
make sure that the accessed data is aligned to sizeof(char **) using the
pad variable (in the original code, we aligned it to 32 bits, but that
was incorrect).
4)=====================================================
src/sss_client/nss_mc_group.c:67:22: warning: cast from 'char *' to
'char **' increases required alignment from 1 to 8 [-Wcast-align]
result->gr_mem = (char **)buffer;
^~~~~~~~~~~~~~~
src/monitor/monitor.c:1588:16: warning: cast from 'char *' to 'struct
inotify_event *' increases required alignment from 1 to 4
[-Wcast-align]
in_event = (struct inotify_event *)buf;
^~~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/nss/nsssrv_mmap_cache.c:281:22: warning: cast from 'char
*' to 'rel_ptr_t *' (aka 'unsigned int *') increases required
alignment from 1 to 4 [-Wcast-align]
name_ptr = *((rel_ptr_t *)rec->data);
^~~~~~~~~~~~~~~~~~~~~~
Comment: Here we access the beginnings of allocated memory spaces. So it
should be OK too. Adding some additional casts just to suppress the
warnings is here IMO inappropriate (it is then not so clear what data
type we use).
5)======================================================
src/responder/common/responder_packet.c:79:21: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->len = &((uint32_t *)packet->buffer)[0];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:80:21: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->cmd = &((uint32_t *)packet->buffer)[1];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:81:24: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->status = &((uint32_t *)packet->buffer)[2];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:82:26: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->reserved = &((uint32_t *)packet->buffer)[3];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:83:33: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:128:29: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->len = &((uint32_t *)packet->buffer)[0];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:129:29: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->cmd = &((uint32_t *)packet->buffer)[1];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:130:32: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->status = &((uint32_t *)packet->buffer)[2];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:131:34: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->reserved = &((uint32_t *)packet->buffer)[3];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:132:41: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
^~~~~~~~~~~~~~~~~~~~~~~~~~
Comment: Same as number 4, but we also use addresses that are multiples
of the sizeof(data_type_used) (relative to the beginning of the buffer).
Which is OK.
================================================================
The rest of the warnings are suppressed by this patch.
I also have one question. Does talloc guarantee that the allocated block
of memory (without the talloc related metadata) is aligned to the
sizeof(biggest_primitive_C_data_type)? I know malloc does this, but I am
not sure about talloc. I saw some places in the code, where we rely on
this feature, so my guess is that talloc does it. Can someone confirm
it? The above mentioned 3, 4 and 5 also rely on proper aligned memory block.
The patch is in attachment.
Thanks
Michal
10 years, 2 months
[PATCH] Do not grow sssd_nss memory footprint
by Jakub Hrozek
Hi,
the attached patch might seem simple, but it touches a delicate and
low-level part of the SSSD, so I'd like to request a detailed review.
I was notified by Bruce Aiesi of the Red Hat QE team that the sssd_nss
process grew its memory footprint when stress-tested by logging in many
users in parallel. I ran a couple of tests and then found out that the
responder context grew because we never freed the internal Data Provider
request. Attached is a patch that simple frees the "side request" after
all the callbacks have been invoked.
I didn't see the memory growth anymore during my testing and I didn't
see any other issues, either during my tests or with valgrind.
I hope I didn't miss any detail and the patch is OK.
10 years, 4 months
[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
10 years, 4 months