[SSSD] Re: Session Recording control options
On Mon, Jun 06, 2016 at 06:24:53PM +0300, Nikolai Kondrashov wrote: > On 06/06/2016 06:20 PM, Sumit Bose wrote: > > On Mon, Jun 06, 2016 at 04:24:35PM +0300, Nikolai Kondrashov wrote: > > > Hi everyone, > > > > > > After a little discussion with Dmitri and Sumit we decided that we'll need > > > options for controlling session recording in sssd.conf, after all. > > > > > > The options should be something like this: > > > > > > record_sessions - string, one of: none/some/all, default is > > > "none" > > > record_sessions_users - string, space-separated list of users to > > > record > > > record_sessions_groups - string, space-separated list of groups to > > > record > > > > > > I'm not sure where we should put them. They can't be put into "nss" or > > > "pam" > > > sections alone, as they concern both (nss fakes the shell, pam adds > > > enviroment > > > variables). I would rather put them into the global "sssd" section and > > > have > > > fully-qualified usernames listed there, but I see that there is very > > > little > > > options there otherwise, so I suspect they wouldn't be welcome. > > > Otherwise, we > > > can put them into domain sections, but that would mean duplicating the > > > "record_sessions" option in every one of them, which is inconvenient. > > > > I would suggest to put them into [nss] and let the pam responder read > > them form there as well. My reasoning is that the faked shell returned > > e.g. by 'getent passwd user_name' is the most user visible change. And > > if anyone is irritated by this it would be good if the options > > responsible for this can be found in the configuration of the related > > responder. > > This seems reasonable from the point of figuring out where the shell came > from, but if I wanted to turn the recording on, why would I look into the nss > section documentation? For some features we have a separate section in the manpages (for example for failover or ID mapping) and for some larger features we have even a separate manpage (sssd-sudo). Would either make sense for the recording? > > OTOH, if we can't put them into the general "sssd" section, then "nss" is > better than putting them into every domain. > > Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Session Recording control options
On 06/06/2016 05:24 PM, Nikolai Kondrashov wrote: On 06/06/2016 06:20 PM, Sumit Bose wrote: On Mon, Jun 06, 2016 at 04:24:35PM +0300, Nikolai Kondrashov wrote: Hi everyone, After a little discussion with Dmitri and Sumit we decided that we'll need options for controlling session recording in sssd.conf, after all. The options should be something like this: record_sessions - string, one of: none/some/all, default is "none" record_sessions_users - string, space-separated list of users to record record_sessions_groups - string, space-separated list of groups to record I'm not sure where we should put them. They can't be put into "nss" or "pam" sections alone, as they concern both (nss fakes the shell, pam adds enviroment variables). I would rather put them into the global "sssd" section and have fully-qualified usernames listed there, but I see that there is very little options there otherwise, so I suspect they wouldn't be welcome. Otherwise, we can put them into domain sections, but that would mean duplicating the "record_sessions" option in every one of them, which is inconvenient. I would suggest to put them into [nss] and let the pam responder read them form there as well. My reasoning is that the faked shell returned e.g. by 'getent passwd user_name' is the most user visible change. And if anyone is irritated by this it would be good if the options responsible for this can be found in the configuration of the related responder. This seems reasonable from the point of figuring out where the shell came from, but if I wanted to turn the recording on, why would I look into the nss section documentation? We don't need to keep our hands tied, we can also introduce new section e.g. [tlog] or [session]. OTOH, if we can't put them into the general "sssd" section, then "nss" is better than putting them into every domain. Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Design document - sssctl
On Mon, Jun 06, 2016 at 02:09:57PM +0200, Pavel Březina wrote: > On 03/22/2016 12:42 PM, Pavel Reichl wrote: > > Hello, > > > > Pavel Březina and I have prepared the 1st draft of design document. We > > mostly focused on summing up its future functionality and its interface. > > > > Please comment if you miss some essential functionality or if you would > > prefer some different interface. > > > > Thanks! > > > > https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl > > I just updated the design page to latest version. Would list-domains also include the domain online status or whether the domain is autoconfigured subdomain or a main domain? ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Design document - sssctl
On 06/07/2016 11:36 AM, Jakub Hrozek wrote: On Mon, Jun 06, 2016 at 02:09:57PM +0200, Pavel Březina wrote: On 03/22/2016 12:42 PM, Pavel Reichl wrote: Hello, Pavel Březina and I have prepared the 1st draft of design document. We mostly focused on summing up its future functionality and its interface. Please comment if you miss some essential functionality or if you would prefer some different interface. Thanks! https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl I just updated the design page to latest version. Would list-domains also include the domain online status or whether the domain is autoconfigured subdomain or a main domain? It is of course possible. But my idea was to print just the domain names so the caller known what to pass to 'sssctl domain-status $domain' to get more information. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] TOOLS: Prevent dereference of null pointer
On (06/06/16 18:55), Lukas Slebodnik wrote:
>On (15/03/16 12:31), Lukas Slebodnik wrote:
>>On (15/03/16 11:26), Pavel Březina wrote:
>>>On 03/07/2016 01:33 PM, Lukas Slebodnik wrote:
On (07/03/16 12:12), Pavel Březina wrote:
>On 03/07/2016 10:14 AM, Lukas Slebodnik wrote:
>>ehlo,
>>
>>simple aptch is attached.
>
>When there, can you also talloc_free(attrs) on error? Thanks.
See updated patch
>>>
>>>Some time has passed now so I take it as you won't implement Sumit's
>>>suggestion.
>>I will but I have tasks with higher priority :-)
>>
>Updated patch is attached.
>
>LS
>From e616ea9e8e58d0ed70b56edc338184d783597004 Mon Sep 17 00:00:00 2001
>From: Lukas Slebodnik
>Date: Mon, 6 Jun 2016 18:15:44 +0200
>Subject: [PATCH] TOOLS: Prevent dereference of null pointer
>
>VAR_CHECK is called with (var, EOK, ...)
>EOK would be returned in case of "var != EOK"
>and output argument _attrs would not be initialized.
>Therefore there could be dereference of null pointer
>after calling function usermod_build_attrs.
>---
> src/tools/sss_sync_ops.c | 62 +---
> 1 file changed, 27 insertions(+), 35 deletions(-)
>
>diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c
>index
>5468929b691c6539cdf55f59be3560412e398f21..e47aef37d2b89b28b7ff18555473136bdf7596cf
> 100644
>--- a/src/tools/sss_sync_ops.c
>+++ b/src/tools/sss_sync_ops.c
>-if (lock == DO_UNLOCK) {
>+if (ret == EOK && lock == DO_UNLOCK) {
>+attr_name = SYSDB_DISABLED;
> /* PAM code checks for 'false' value in SYSDB_DISABLED attribute */
> ret = sysdb_attrs_add_string(attrs,
>- SYSDB_DISABLED,
>+ attr_name,
> "false");
>-VAR_CHECK(ret, EOK, SYSDB_DISABLED,
>- "Could not add attribute to changeset\n");
>+}
>+
>+if (ret != EOK) {
>+DEBUG(SSSDBG_CRIT_FAILURE,
>+ "Could not add attribute [%s] to changeset.\n", attr_name);
I forgot to return error here.
> }
>
> *_attrs = attrs;
Upodated patch is attached.
LS
>From 6b190337908f8167ce328cb763967a4c98f34cec Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik
Date: Mon, 6 Jun 2016 18:15:44 +0200
Subject: [PATCH] TOOLS: Prevent dereference of null pointer
VAR_CHECK is called with (var, EOK, ...)
EOK would be returned in case of "var != EOK"
and output argument _attrs would not be initialized.
Therefore there could be dereference of null pointer
after calling function usermod_build_attrs.
---
src/tools/sss_sync_ops.c | 63 +---
1 file changed, 28 insertions(+), 35 deletions(-)
diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c
index
5468929b691c6539cdf55f59be3560412e398f21..ee5c9bb2fca65488d651d909e575b32b406923e6
100644
--- a/src/tools/sss_sync_ops.c
+++ b/src/tools/sss_sync_ops.c
@@ -37,13 +37,6 @@
#define ATTR_NAME_SEP '='
#define ATTR_VAL_SEP ','
-#define VAR_CHECK(var, val, attr, msg) do { \
-if (var != (val)) { \
-DEBUG(SSSDBG_CRIT_FAILURE, msg" attribute: %s\n", attr); \
-return val; \
-} \
-} while(0)
-
static int attr_name_val_split(TALLOC_CTX *mem_ctx, const char *nameval,
char **_name, char ***_values, int *_nvals)
{
@@ -200,8 +193,9 @@ static int usermod_build_attrs(TALLOC_CTX *mem_ctx,
int lock,
struct sysdb_attrs **_attrs)
{
-int ret;
+int ret = EOK;
struct sysdb_attrs *attrs;
+const char *attr_name = NULL;
attrs = sysdb_new_attrs(mem_ctx);
if (attrs == NULL) {
@@ -209,60 +203,59 @@ static int usermod_build_attrs(TALLOC_CTX *mem_ctx,
}
if (shell) {
+attr_name = SYSDB_SHELL;
ret = sysdb_attrs_add_string(attrs,
- SYSDB_SHELL,
+ attr_name,
shell);
-VAR_CHECK(ret, EOK, SYSDB_SHELL,
- "Could not add attribute to changeset\n");
}
-if (home) {
+if (ret == EOK && home) {
+attr_name = SYSDB_HOMEDIR;
ret = sysdb_attrs_add_string(attrs,
- SYSDB_HOMEDIR,
+ attr_name,
home);
-VAR_CHECK(ret, EOK, SYSDB_HOMEDIR,
- "Could not add attribute to changeset\n");
}
-if (gecos) {
+if (ret == EOK && gecos) {
+attr_name = SYSDB_GECOS;
ret = sysdb_attrs_add_string(attrs,
- SYSDB_GECOS,
+ attr_name,
gecos);
-VAR_CHECK(ret, EOK, SYSDB_GECOS,
- "Could not add attribute to changeset\n");
}
-if (uid) {
+if (ret == EOK &&
[SSSD] Re: [PATCH] libwbclient: wbcSidsToUnixIds() don't fail on errors
On Mon, Jun 06, 2016 at 03:22:22PM +0300, Alexander Bokovoy wrote: > ACK. master: 52f1093ef3d7c44132ec10c57436865b2cbb19d7 sssd-1-13: 15ad5f603a5797c61a01f67365c2581c7bddcdfa ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Design document - sssctl
On Tue, Jun 07, 2016 at 11:53:55AM +0200, Pavel Březina wrote: > On 06/07/2016 11:36 AM, Jakub Hrozek wrote: > > On Mon, Jun 06, 2016 at 02:09:57PM +0200, Pavel Březina wrote: > > > On 03/22/2016 12:42 PM, Pavel Reichl wrote: > > > > Hello, > > > > > > > > Pavel Březina and I have prepared the 1st draft of design document. We > > > > mostly focused on summing up its future functionality and its interface. > > > > > > > > Please comment if you miss some essential functionality or if you would > > > > prefer some different interface. > > > > > > > > Thanks! > > > > > > > > https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl > > > > > > I just updated the design page to latest version. > > > > Would list-domains also include the domain online status or whether the > > domain is autoconfigured subdomain or a main domain? > > It is of course possible. But my idea was to print just the domain names so > the caller known what to pass to 'sssctl domain-status $domain' to get more > information. Yes, that's better. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCHES] ipa: add support for certificate overrides
On Mon, Jun 06, 2016 at 11:06:06AM +0200, Sumit Bose wrote:
> On Fri, Jun 03, 2016 at 02:56:08PM +0200, Jakub Hrozek wrote:
> > On Fri, May 20, 2016 at 09:13:29PM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > this set of patches should resolve
> > > https://fedorahosted.org/sssd/ticket/2897 "Smart Cards: Certificate in
> > > the ID View" and cover all other use cases from
> > > https://fedorahosted.org/sssd/wiki/DesignDocs/LookupUsersByCertificatePart2
> > > as well. So basically certificates can be read from IPA and local
> > > overrides and from AD with direct in indirect integration.
> > >
> > > The patches are all about lookups, so Smartcards and authentication is
> > > not needed to test them. All is needed is a certificate which can be
> > > added to an AD user object or an override object and then try to lookup
> > > the user with
> > >
> > > dbus-send --system --print-reply --dest=org.freedesktop.sssd.infopipe \
> > > /org/freedesktop/sssd/infopipe/Users \
> > > org.freedesktop.sssd.infopipe.Users.FindByCertificate
> > > string:"BASE64_CERTIFICATE_STRING"
> > >
> > > from a IPA client, IPA server or AD client with AD provier.
> > >
> > > If the certificate is store in the AD user object and the lookup is
> > > started on an IPA client a patch for the IPA server is needed, because
> > > the request has to run via the extdom plugin. I'll send a patch to
> > > freeipa-devel which will use the sss_nss_getnamebycert() call added by
> > > one of the patches to allow the extdom plugin to do lookups by
> > > certificate. This means that SSSD on the IPA server must used the
> > > attached patches as well.
> > >
> > > bye,
> > > Sumit
> >
> > Hi,
> >
> > so far I read the patches, see comments inline. I haven't tested them
> > yet, feel free to postpone sending new patches until the final ack/nack.
>
> Thank you for reviewing and testing.
>
> >
> > > From b081fc781a4bb22f3fdc6200256086f6b2cb811f Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose
> > > Date: Wed, 6 Apr 2016 11:12:30 +0200
> > > Subject: [PATCH 02/12] sysdb: add searches by certificate with overrides
> > > +/* If there are views we have to check if override values must be
> > > added to
> > > + * the original object. */
> > > +if (DOM_HAS_VIEWS(domain) && orig_obj->count == 1) {
> > > +ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],
> > > + override_obj == NULL ? NULL :
> > > override_obj->msgs[0],
> > > + NULL);
> >
> > As a style nitpick, I would prefer to use:
> > if (ret == ENOENT) {
> > } else if (ret != EOK) {
> > }
>
> fixed
>
> >
> > > +if (ret != EOK && ret != ENOENT) {
> > > +DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object
> > > failed.\n");
> > > +goto done;
> > > +}
> >
> >
> > > From 2a2a021aa6ff9f3651c389201292c4066af4319d Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose
> > > Date: Fri, 8 Apr 2016 13:22:24 +0200
> > > Subject: [PATCH 09/12] sss_override: add certificate support
> > >
> > > ---
> > > src/db/sysdb.h | 1 +
> > > src/tests/intg/ldap_local_override_test.py | 8 +++
> > > src/tools/sss_override.c | 38
> > > ++
> >
> > It would be nice to amend the sss_override manpage, too.
>
> fixed
>
> >
> >
> > > From d564862024bb208614b2f3b547d99b72a9787a45 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose
> > > Date: Mon, 25 Apr 2016 16:09:48 +0200
> > > Subject: [PATCH 11/12] NSS: add SSS_NSS_GETNAMEBYCERT request
> >
> > > +static void ifp_users_find_by_cert_done(struct tevent_req *req);
> >
> > I guess using the ifp prefix here is a copy-and-paste error?
>
> fixed
>
> >
> >
> > > From f7647dbf154ce5cb082b391269f9b674ca47e712 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose
> > > Date: Tue, 26 Apr 2016 13:13:43 +0200
> > > Subject: [PATCH 12/12] nss-idmap: add sss_nss_getnamebycert()
> > >
> > > ---
> > > Makefile.am| 2 +-
> > > src/python/pysss_nss_idmap.c | 47
> > > --
> > > src/responder/nss/nsssrv_cmd.c | 1 +
> > > src/sss_client/idmap/sss_nss_idmap.c | 26 -
> > > src/sss_client/idmap/sss_nss_idmap.exports | 6
> > > src/sss_client/idmap/sss_nss_idmap.h | 15 ++
> > > 6 files changed, 93 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Makefile.am b/Makefile.am
> > > index
> > > d20a10fa61f8a2fc296f839318503960382a9879..58255b0dc766e3755d70c12ee312a9aa52d6a724
> > > 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -989,7 +989,7 @@ libsss_nss_idmap_la_LIBADD = \
> > > $(CLIENT_LIBS)
> > > libsss_nss_idmap_la_LDFLAGS = \
> > >
> > > -Wl,--version-script,$(srcdir)/src/sss_client/idmap/sss_nss_idmap.exports
> > > \
> > > --version-info 1:0:1
> > > +-version-info 2:0:2
> >
> > The ch
[SSSD] Re: [PATCH] AD_PROVIDER: Fix constant char *
On (03/06/16 13:09), Sumit Bose wrote: >On Fri, Jun 03, 2016 at 09:38:43AM +0200, Sumit Bose wrote: >> On Fri, Jun 03, 2016 at 08:22:10AM +0200, Petr Cech wrote: >> > bump >> >> obvious ACK, just waiting for the CI to finish. > >There are 2 failures in >http://sssd-ci.duckdns.org/logs/job/44/38/summary.html but they look >like CI issues and not related to the patch. > >ACK > master: * 9a6ff0851fc707f21165818f66ae926fa14d7226 LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] PAM: add pam_sss option allow_missing_name
On Mon, Jun 06, 2016 at 10:56:52AM +0200, Sumit Bose wrote: > On Fri, Jun 03, 2016 at 05:56:45PM +0200, Jakub Hrozek wrote: > > On Wed, Jun 01, 2016 at 06:31:29PM +0200, Sumit Bose wrote: > > > Hi, > > > > > > that attached two patches would allow to use the Smartcard support in > > > gdm with SSSD. To use it you should replace pam_pkcs11 in > > > /etc/pam.d/smartcard-auth in the auth section by > > > > > > authsufficient pam_sss.so allow_missing_name > > > > > > and drop the password section completely. > > > > > > To enable the Smartcard support in gdm the easiest way is to use > > > dconf-editor: > > > > > > DCONF_PROFILE=gdm dconf-editor > > > > > > In the org/gnome/login-screen section you can switch the Smartcard > > > support on and off. Additionally you might want to tune the removal > > > action in org/gnome/settings-daemon/peripherals/smartcard . > > > > > > If now a Smartcard is inserted gdm should register it, call > > > /etc/pam.d/gdm-smartcard which calls /etc/pam.d/smartcard-auth without a > > > user name. With the new option from the first patch pam_sss will accept > > > this and send it to the pam responder. The pam responder can handle this > > > if Smartcard authentication is enabled, tries to read the certificate > > > from the Smartcard, tries to find and matching user and if successful, > > > returns the user name to pam_sss which puts it on the PAM stack and > > > continues with the authentication. > > > > > > It would be nice if someone can review the code even without testing the > > > functionality. In this case I will ask someone else with access to > > > Smartcards and reader to do some functional testing. > > > > > > I think these patches are candidates for the pam wrapper based tests > > > Jakub has for review on the list. I'll start reviewing those and add > > > tests when they are in master. > > > > The code looks good to me with some minor nitpicks (see inline) but at > > least for me, the tests are failing: > > [ RUN ] test_pam_offline_chauthtok_prelim > > [ ERROR ] --- 0x2 != 0x3 > > [ LINE ] --- > > /home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_pam_srv.c:641: error: > > Failure! > > [ FAILED ] test_pam_offline_chauthtok_prelim > > [ RUN ] test_pam_offline_chauthtok > > [ ERROR ] --- 0x2 != 0x3 > > [ LINE ] --- > > /home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_pam_srv.c:641: error: > > Failure! > > [ FAILED ] test_pam_offline_chauthtok > > > > Do I need some other patches applied as well? > > Not that I'm aware of. So far I was not able to reproduce the error > locally not with CI > http://sssd-ci.duckdns.org/logs/job/44/49/summary.html . Do you maybe > have your pam wrapper patches applied to check for regressions? Of course this was the case :-) Good excuse to rebase my tests atop these patches.. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCHES] ipa: add support for certificate overrides
On Tue, Jun 07, 2016 at 12:04:12PM +0200, Jakub Hrozek wrote:
> On Mon, Jun 06, 2016 at 11:06:06AM +0200, Sumit Bose wrote:
> > On Fri, Jun 03, 2016 at 02:56:08PM +0200, Jakub Hrozek wrote:
> > > On Fri, May 20, 2016 at 09:13:29PM +0200, Sumit Bose wrote:
> > > > Hi,
> > > >
> > > > this set of patches should resolve
> > > > https://fedorahosted.org/sssd/ticket/2897 "Smart Cards: Certificate in
> > > > the ID View" and cover all other use cases from
> > > > https://fedorahosted.org/sssd/wiki/DesignDocs/LookupUsersByCertificatePart2
> > > > as well. So basically certificates can be read from IPA and local
> > > > overrides and from AD with direct in indirect integration.
> > > >
> > > > The patches are all about lookups, so Smartcards and authentication is
> > > > not needed to test them. All is needed is a certificate which can be
> > > > added to an AD user object or an override object and then try to lookup
> > > > the user with
> > > >
> > > > dbus-send --system --print-reply --dest=org.freedesktop.sssd.infopipe \
> > > > /org/freedesktop/sssd/infopipe/Users \
> > > > org.freedesktop.sssd.infopipe.Users.FindByCertificate
> > > > string:"BASE64_CERTIFICATE_STRING"
> > > >
> > > > from a IPA client, IPA server or AD client with AD provier.
> > > >
> > > > If the certificate is store in the AD user object and the lookup is
> > > > started on an IPA client a patch for the IPA server is needed, because
> > > > the request has to run via the extdom plugin. I'll send a patch to
> > > > freeipa-devel which will use the sss_nss_getnamebycert() call added by
> > > > one of the patches to allow the extdom plugin to do lookups by
> > > > certificate. This means that SSSD on the IPA server must used the
> > > > attached patches as well.
> > > >
> > > > bye,
> > > > Sumit
> > >
> > > Hi,
> > >
> > > so far I read the patches, see comments inline. I haven't tested them
> > > yet, feel free to postpone sending new patches until the final ack/nack.
> >
> > Thank you for reviewing and testing.
> >
> > >
> > > > From b081fc781a4bb22f3fdc6200256086f6b2cb811f Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose
> > > > Date: Wed, 6 Apr 2016 11:12:30 +0200
> > > > Subject: [PATCH 02/12] sysdb: add searches by certificate with overrides
> > > > +/* If there are views we have to check if override values must be
> > > > added to
> > > > + * the original object. */
> > > > +if (DOM_HAS_VIEWS(domain) && orig_obj->count == 1) {
> > > > +ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],
> > > > + override_obj == NULL ? NULL :
> > > > override_obj->msgs[0],
> > > > + NULL);
> > >
> > > As a style nitpick, I would prefer to use:
> > > if (ret == ENOENT) {
> > > } else if (ret != EOK) {
> > > }
> >
> > fixed
> >
> > >
> > > > +if (ret != EOK && ret != ENOENT) {
> > > > +DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object
> > > > failed.\n");
> > > > +goto done;
> > > > +}
> > >
> > >
> > > > From 2a2a021aa6ff9f3651c389201292c4066af4319d Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose
> > > > Date: Fri, 8 Apr 2016 13:22:24 +0200
> > > > Subject: [PATCH 09/12] sss_override: add certificate support
> > > >
> > > > ---
> > > > src/db/sysdb.h | 1 +
> > > > src/tests/intg/ldap_local_override_test.py | 8 +++
> > > > src/tools/sss_override.c | 38
> > > > ++
> > >
> > > It would be nice to amend the sss_override manpage, too.
> >
> > fixed
> >
> > >
> > >
> > > > From d564862024bb208614b2f3b547d99b72a9787a45 Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose
> > > > Date: Mon, 25 Apr 2016 16:09:48 +0200
> > > > Subject: [PATCH 11/12] NSS: add SSS_NSS_GETNAMEBYCERT request
> > >
> > > > +static void ifp_users_find_by_cert_done(struct tevent_req *req);
> > >
> > > I guess using the ifp prefix here is a copy-and-paste error?
> >
> > fixed
> >
> > >
> > >
> > > > From f7647dbf154ce5cb082b391269f9b674ca47e712 Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose
> > > > Date: Tue, 26 Apr 2016 13:13:43 +0200
> > > > Subject: [PATCH 12/12] nss-idmap: add sss_nss_getnamebycert()
> > > >
> > > > ---
> > > > Makefile.am| 2 +-
> > > > src/python/pysss_nss_idmap.c | 47
> > > > --
> > > > src/responder/nss/nsssrv_cmd.c | 1 +
> > > > src/sss_client/idmap/sss_nss_idmap.c | 26 -
> > > > src/sss_client/idmap/sss_nss_idmap.exports | 6
> > > > src/sss_client/idmap/sss_nss_idmap.h | 15 ++
> > > > 6 files changed, 93 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index
> > > > d20a10fa61f8a2fc296f839318503960382a9879..58255b0dc766e3755d70c12ee312a9aa52d6a724
> > > > 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefi
[SSSD] Re: Session Recording control options
On 06/07/2016 11:16 AM, Pavel Březina wrote: On 06/06/2016 05:24 PM, Nikolai Kondrashov wrote: On 06/06/2016 06:20 PM, Sumit Bose wrote: On Mon, Jun 06, 2016 at 04:24:35PM +0300, Nikolai Kondrashov wrote: Hi everyone, After a little discussion with Dmitri and Sumit we decided that we'll need options for controlling session recording in sssd.conf, after all. The options should be something like this: record_sessions - string, one of: none/some/all, default is "none" record_sessions_users - string, space-separated list of users to record record_sessions_groups - string, space-separated list of groups to record I'm not sure where we should put them. They can't be put into "nss" or "pam" sections alone, as they concern both (nss fakes the shell, pam adds enviroment variables). I would rather put them into the global "sssd" section and have fully-qualified usernames listed there, but I see that there is very little options there otherwise, so I suspect they wouldn't be welcome. Otherwise, we can put them into domain sections, but that would mean duplicating the "record_sessions" option in every one of them, which is inconvenient. I would suggest to put them into [nss] and let the pam responder read them form there as well. My reasoning is that the faked shell returned e.g. by 'getent passwd user_name' is the most user visible change. And if anyone is irritated by this it would be good if the options responsible for this can be found in the configuration of the related responder. This seems reasonable from the point of figuring out where the shell came from, but if I wanted to turn the recording on, why would I look into the nss section documentation? We don't need to keep our hands tied, we can also introduce new section e.g. [tlog] or [session]. Alright, how about we introduce a new section in the configuration file, put these options into it: [session_recording] scope - string, one of: none/selected/all, default is "none" users - string, space-separated list of users to record, only valid if "scope = selected" groups - string, space-separated list of groups to record, only valid if "scope = selected" and add a section to sssd.conf(5), or a new manpage, e.g. sssd-session-recording(5)? Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] PAM: add pam_sss option allow_missing_name
On Tue, Jun 07, 2016 at 12:18:17PM +0200, Jakub Hrozek wrote: > On Mon, Jun 06, 2016 at 10:56:52AM +0200, Sumit Bose wrote: > > On Fri, Jun 03, 2016 at 05:56:45PM +0200, Jakub Hrozek wrote: > > > On Wed, Jun 01, 2016 at 06:31:29PM +0200, Sumit Bose wrote: > > > > Hi, > > > > > > > > that attached two patches would allow to use the Smartcard support in > > > > gdm with SSSD. To use it you should replace pam_pkcs11 in > > > > /etc/pam.d/smartcard-auth in the auth section by > > > > > > > > authsufficient pam_sss.so allow_missing_name > > > > > > > > and drop the password section completely. > > > > > > > > To enable the Smartcard support in gdm the easiest way is to use > > > > dconf-editor: > > > > > > > > DCONF_PROFILE=gdm dconf-editor > > > > > > > > In the org/gnome/login-screen section you can switch the Smartcard > > > > support on and off. Additionally you might want to tune the removal > > > > action in org/gnome/settings-daemon/peripherals/smartcard . > > > > > > > > If now a Smartcard is inserted gdm should register it, call > > > > /etc/pam.d/gdm-smartcard which calls /etc/pam.d/smartcard-auth without a > > > > user name. With the new option from the first patch pam_sss will accept > > > > this and send it to the pam responder. The pam responder can handle this > > > > if Smartcard authentication is enabled, tries to read the certificate > > > > from the Smartcard, tries to find and matching user and if successful, > > > > returns the user name to pam_sss which puts it on the PAM stack and > > > > continues with the authentication. > > > > > > > > It would be nice if someone can review the code even without testing the > > > > functionality. In this case I will ask someone else with access to > > > > Smartcards and reader to do some functional testing. > > > > > > > > I think these patches are candidates for the pam wrapper based tests > > > > Jakub has for review on the list. I'll start reviewing those and add > > > > tests when they are in master. > > > > > > The code looks good to me with some minor nitpicks (see inline) but at > > > least for me, the tests are failing: > > > [ RUN ] test_pam_offline_chauthtok_prelim > > > [ ERROR ] --- 0x2 != 0x3 > > > [ LINE ] --- > > > /home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_pam_srv.c:641: > > > error: Failure! > > > [ FAILED ] test_pam_offline_chauthtok_prelim > > > [ RUN ] test_pam_offline_chauthtok > > > [ ERROR ] --- 0x2 != 0x3 > > > [ LINE ] --- > > > /home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_pam_srv.c:641: > > > error: Failure! > > > [ FAILED ] test_pam_offline_chauthtok > > > > > > Do I need some other patches applied as well? > > > > Not that I'm aware of. So far I was not able to reproduce the error > > locally not with CI > > http://sssd-ci.duckdns.org/logs/job/44/49/summary.html . Do you maybe > > have your pam wrapper patches applied to check for regressions? > > Of course this was the case :-) Good excuse to rebase my tests atop > these patches.. ACK to both your patches by the way. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Session Recording control options
On 06/07/2016 02:29 PM, Nikolai Kondrashov wrote: On 06/07/2016 11:16 AM, Pavel Březina wrote: On 06/06/2016 05:24 PM, Nikolai Kondrashov wrote: On 06/06/2016 06:20 PM, Sumit Bose wrote: On Mon, Jun 06, 2016 at 04:24:35PM +0300, Nikolai Kondrashov wrote: Hi everyone, After a little discussion with Dmitri and Sumit we decided that we'll need options for controlling session recording in sssd.conf, after all. The options should be something like this: record_sessions - string, one of: none/some/all, default is "none" record_sessions_users - string, space-separated list of users to record record_sessions_groups - string, space-separated list of groups to record I'm not sure where we should put them. They can't be put into "nss" or "pam" sections alone, as they concern both (nss fakes the shell, pam adds enviroment variables). I would rather put them into the global "sssd" section and have fully-qualified usernames listed there, but I see that there is very little options there otherwise, so I suspect they wouldn't be welcome. Otherwise, we can put them into domain sections, but that would mean duplicating the "record_sessions" option in every one of them, which is inconvenient. I would suggest to put them into [nss] and let the pam responder read them form there as well. My reasoning is that the faked shell returned e.g. by 'getent passwd user_name' is the most user visible change. And if anyone is irritated by this it would be good if the options responsible for this can be found in the configuration of the related responder. This seems reasonable from the point of figuring out where the shell came from, but if I wanted to turn the recording on, why would I look into the nss section documentation? We don't need to keep our hands tied, we can also introduce new section e.g. [tlog] or [session]. Alright, how about we introduce a new section in the configuration file, put these options into it: [session_recording] scope - string, one of: none/selected/all, default is "none" users - string, space-separated list of users to record, only valid if "scope = selected" groups - string, space-separated list of groups to record, only valid if "scope = selected" and add a section to sssd.conf(5), or a new manpage, e.g. sssd-session-recording(5)? So it can be, for example: [session_recording] scope = selected users = john mary groups = sysops Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Session Recording control options
On 06/07/2016 01:39 PM, Nikolai Kondrashov wrote: On 06/07/2016 02:29 PM, Nikolai Kondrashov wrote: On 06/07/2016 11:16 AM, Pavel Březina wrote: On 06/06/2016 05:24 PM, Nikolai Kondrashov wrote: On 06/06/2016 06:20 PM, Sumit Bose wrote: On Mon, Jun 06, 2016 at 04:24:35PM +0300, Nikolai Kondrashov wrote: Hi everyone, After a little discussion with Dmitri and Sumit we decided that we'll need options for controlling session recording in sssd.conf, after all. The options should be something like this: record_sessions - string, one of: none/some/all, default is "none" record_sessions_users - string, space-separated list of users to record record_sessions_groups - string, space-separated list of groups to record I'm not sure where we should put them. They can't be put into "nss" or "pam" sections alone, as they concern both (nss fakes the shell, pam adds enviroment variables). I would rather put them into the global "sssd" section and have fully-qualified usernames listed there, but I see that there is very little options there otherwise, so I suspect they wouldn't be welcome. Otherwise, we can put them into domain sections, but that would mean duplicating the "record_sessions" option in every one of them, which is inconvenient. I would suggest to put them into [nss] and let the pam responder read them form there as well. My reasoning is that the faked shell returned e.g. by 'getent passwd user_name' is the most user visible change. And if anyone is irritated by this it would be good if the options responsible for this can be found in the configuration of the related responder. This seems reasonable from the point of figuring out where the shell came from, but if I wanted to turn the recording on, why would I look into the nss section documentation? We don't need to keep our hands tied, we can also introduce new section e.g. [tlog] or [session]. Alright, how about we introduce a new section in the configuration file, put these options into it: [session_recording] scope - string, one of: none/selected/all, default is "none" users - string, space-separated list of users to record, only valid if "scope = selected" groups - string, space-separated list of groups to record, only valid if "scope = selected" I think we mostly use comma-separated list in sssd. and add a section to sssd.conf(5), or a new manpage, e.g. sssd-session-recording(5)? It depends on the amount of information... If there will be lots of text on the session recording than I think it would be better as an individual man page otherwise we'll fine with sssd.conf section. But yes, I think creating a new section is much better. So it can be, for example: [session_recording] scope = selected users = john mary groups = sysops Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCHES] ipa: add support for certificate overrides
On Tue, Jun 07, 2016 at 12:28:22PM +0200, Sumit Bose wrote: > sure, here you are. > > bye, > Sumit Hmm, are these the correct patches? /home/remote/jhrozek/devel/sssd/src/db/sysdb_views.c: In function 'sysdb_search_override_by_cert': /home/remote/jhrozek/devel/sssd/src/db/sysdb_views.c:880:11: error: too many arguments to function 'sss_cert_derb64_to_ldap_filter' ret = sss_cert_derb64_to_ldap_filter(tmp_ctx, cert, SYSDB_USER_CERT, NULL, ^ In file included from /home/remote/jhrozek/devel/sssd/src/db/sysdb_views.c:23:0: /home/remote/jhrozek/devel/sssd/src/util/cert.h:40:9: note: declared here errno_t sss_cert_derb64_to_ldap_filter(TALLOC_CTX *mem_ctx, const char *derb64, ^ I applied: $ git rev-list origin/master..HEAD 91fe4dec3d70759a84ff48c4cc7502327f717894 8df9665f49a4b669299504cdaa5e745f12336de6 5ac83ad804b06fb6678f46cd289269a1bccfe4ad 2976137caa3d57e6b87b736ab1040c778af4d44b e57eea3aaad72138596343c540f84e705f4f0e17 61a5346d0efaabe7b3bbacc5c202dc797c2684a7 d0911e8fb6c5748cac8267d978971022cbc0c859 330f21d2f0df5b9b6c9c54e685774c722a629efd eecf83cb6a79dcea1f99e4363e76c1758dcd69fb 98c2123924dc6e1e9d695fa7b45292b1c635dee3 7faa5fb5c0006a6d77a0b5b735846c61b533e7ff f506e3a307e5899d35b3ad8ff55d9a02d5a71ade ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Session Recording control options
On 06/07/2016 02:45 PM, Pavel Březina wrote: On 06/07/2016 01:39 PM, Nikolai Kondrashov wrote: On 06/07/2016 02:29 PM, Nikolai Kondrashov wrote: On 06/07/2016 11:16 AM, Pavel Březina wrote: On 06/06/2016 05:24 PM, Nikolai Kondrashov wrote: On 06/06/2016 06:20 PM, Sumit Bose wrote: On Mon, Jun 06, 2016 at 04:24:35PM +0300, Nikolai Kondrashov wrote: Hi everyone, After a little discussion with Dmitri and Sumit we decided that we'll need options for controlling session recording in sssd.conf, after all. The options should be something like this: record_sessions - string, one of: none/some/all, default is "none" record_sessions_users - string, space-separated list of users to record record_sessions_groups - string, space-separated list of groups to record I'm not sure where we should put them. They can't be put into "nss" or "pam" sections alone, as they concern both (nss fakes the shell, pam adds enviroment variables). I would rather put them into the global "sssd" section and have fully-qualified usernames listed there, but I see that there is very little options there otherwise, so I suspect they wouldn't be welcome. Otherwise, we can put them into domain sections, but that would mean duplicating the "record_sessions" option in every one of them, which is inconvenient. I would suggest to put them into [nss] and let the pam responder read them form there as well. My reasoning is that the faked shell returned e.g. by 'getent passwd user_name' is the most user visible change. And if anyone is irritated by this it would be good if the options responsible for this can be found in the configuration of the related responder. This seems reasonable from the point of figuring out where the shell came from, but if I wanted to turn the recording on, why would I look into the nss section documentation? We don't need to keep our hands tied, we can also introduce new section e.g. [tlog] or [session]. Alright, how about we introduce a new section in the configuration file, put these options into it: [session_recording] scope - string, one of: none/selected/all, default is "none" users - string, space-separated list of users to record, only valid if "scope = selected" groups - string, space-separated list of groups to record, only valid if "scope = selected" I think we mostly use comma-separated list in sssd. Ah, OK, that's also fine. and add a section to sssd.conf(5), or a new manpage, e.g. sssd-session-recording(5)? It depends on the amount of information... If there will be lots of text on the session recording than I think it would be better as an individual man page otherwise we'll fine with sssd.conf section. Alright, we can start with a section in sssd.conf(5) and if it grows, move it to a separate page. But yes, I think creating a new section is much better. Great! I like it too, although it feels a bit like an overkill for just these three options. Let's see what others will say. Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCHES] p11: add no_verification option
On Mon, May 30, 2016 at 04:32:20PM +0200, Sumit Bose wrote: > > oops, yes I guess this would be a good idea. I'll send a new patch. > > > > new version attached. > > bye, > Sumit One last question, do we want to add the ocsp_default_responder and ocsp_default_responder_signing_cert options to configAPI? ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCHES] ipa: add support for certificate overrides
On Tue, Jun 07, 2016 at 01:56:10PM +0200, Jakub Hrozek wrote:
> On Tue, Jun 07, 2016 at 12:28:22PM +0200, Sumit Bose wrote:
> > sure, here you are.
> >
> > bye,
> > Sumit
>
> Hmm, are these the correct patches?
>
> /home/remote/jhrozek/devel/sssd/src/db/sysdb_views.c: In function
> 'sysdb_search_override_by_cert':
> /home/remote/jhrozek/devel/sssd/src/db/sysdb_views.c:880:11: error: too many
> arguments to function 'sss_cert_derb64_to_ldap_filter'
> ret = sss_cert_derb64_to_ldap_filter(tmp_ctx, cert, SYSDB_USER_CERT,
> NULL,
>^
> In file included from
> /home/remote/jhrozek/devel/sssd/src/db/sysdb_views.c:23:0:
> /home/remote/jhrozek/devel/sssd/src/util/cert.h:40:9: note: declared here
> errno_t sss_cert_derb64_to_ldap_filter(TALLOC_CTX *mem_ctx, const char
> *derb64,
> ^
ah, sorry, I picked the patches from a wrong branch.
Please try the new version.
bye,
Sumit
>
> I applied:
> $ git rev-list origin/master..HEAD
> 91fe4dec3d70759a84ff48c4cc7502327f717894
> 8df9665f49a4b669299504cdaa5e745f12336de6
> 5ac83ad804b06fb6678f46cd289269a1bccfe4ad
> 2976137caa3d57e6b87b736ab1040c778af4d44b
> e57eea3aaad72138596343c540f84e705f4f0e17
> 61a5346d0efaabe7b3bbacc5c202dc797c2684a7
> d0911e8fb6c5748cac8267d978971022cbc0c859
> 330f21d2f0df5b9b6c9c54e685774c722a629efd
> eecf83cb6a79dcea1f99e4363e76c1758dcd69fb
> 98c2123924dc6e1e9d695fa7b45292b1c635dee3
> 7faa5fb5c0006a6d77a0b5b735846c61b533e7ff
> f506e3a307e5899d35b3ad8ff55d9a02d5a71ade
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
From 309a477d08314f849609bd61b5b18a738a1724c6 Mon Sep 17 00:00:00 2001
From: Sumit Bose
Date: Mon, 30 Nov 2015 12:14:16 +0100
Subject: [PATCH 01/12] sysdb: add sysdb_attrs_add_base64_blob()
---
src/db/sysdb.c | 22 ++
src/db/sysdb.h | 2 ++
src/tests/cmocka/test_sysdb_utils.c | 37 +
3 files changed, 61 insertions(+)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index
3c888a42ca6f3b3e37b9f63e35c31bb7d5ffc367..d7540f0ccbe7de38f840f84dbe8f51f4793821bc
100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -23,6 +23,7 @@
#include "util/util.h"
#include "util/strtonum.h"
#include "util/sss_utf8.h"
+#include "util/crypto/sss_crypto.h"
#include "db/sysdb_private.h"
#include "confdb/confdb.h"
#include
@@ -634,6 +635,27 @@ int sysdb_attrs_add_mem(struct sysdb_attrs *attrs, const
char *name,
return sysdb_attrs_add_val(attrs, name, &v);
}
+int sysdb_attrs_add_base64_blob(struct sysdb_attrs *attrs, const char *name,
+const char *base64_str)
+{
+struct ldb_val v;
+int ret;
+
+if (base64_str == NULL) {
+return EINVAL;
+}
+
+v.data = sss_base64_decode(attrs, base64_str, &v.length);
+if (v.data == NULL) {
+DEBUG(SSSDBG_OP_FAILURE, "sss_base64_decode failed.\n");
+return ENOMEM;
+}
+
+ret = sysdb_attrs_add_val(attrs, name, &v);
+talloc_free(v.data);
+return ret;
+}
+
int sysdb_attrs_add_bool(struct sysdb_attrs *attrs,
const char *name, bool value)
{
diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index
11b680f3f67319abd8856ea42045f6b8f0cef4ba..ec398496d67f0d32351a1fa974b2ab17dc9af88c
100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -322,6 +322,8 @@ int sysdb_attrs_add_lower_case_string(struct sysdb_attrs
*attrs, bool safe,
const char *name, const char *str);
int sysdb_attrs_add_mem(struct sysdb_attrs *attrs, const char *name,
const void *mem, size_t size);
+int sysdb_attrs_add_base64_blob(struct sysdb_attrs *attrs, const char *name,
+const char *base64_str);
int sysdb_attrs_add_bool(struct sysdb_attrs *attrs,
const char *name, bool value);
int sysdb_attrs_add_long(struct sysdb_attrs *attrs,
diff --git a/src/tests/cmocka/test_sysdb_utils.c
b/src/tests/cmocka/test_sysdb_utils.c
index
b791f14b7fdfdd4e55343fc1c03a7fd55d8ed101..42abed3c52463893525c35e563044d430e9a6a8b
100644
--- a/src/tests/cmocka/test_sysdb_utils.c
+++ b/src/tests/cmocka/test_sysdb_utils.c
@@ -103,6 +103,42 @@ static void test_sysdb_handle_original_uuid(void **state)
talloc_free(dest_attrs);
}
+#define TEST_BASE64_ABC "YWJj"
+#define TEST_BASE64_123 "AQID"
+static void test_sysdb_attrs_add_base64_blob(void **state)
+{
+struct sysdb_attrs *attrs;
+struct ldb_message_element *el;
+char zero[] = {'\1', '\2', '\3'};
+int ret;
+
+attrs = sysdb_new_attrs(NULL);
+assert_non_null(attrs);
+
+ret = sysdb_attrs_add_base64_blob(attrs, "testAttrABC", TEST_BASE64_ABC);
+assert_int_equal(ret, EOK);
+
+ret = sysdb_attrs_add_base64_blob(attrs, "testAttr000", TEST_BASE64_123);
+assert_int_equal(ret, EOK);
+
+ret = sy
[SSSD] Re: [PATCH] ssh: skip invalid certificates
On Fri, Jun 03, 2016 at 08:17:01PM +0200, Sumit Bose wrote:
> Hi,
>
> currently the code which generates ssh key from the public keys in the
> user certificates fails if one certificate cannot be validated and
> terminates the whole request. It is of course valid that the user entry
> might contain certificates which SSSD cannot validate and since we just
> won't generate a ssh-key in this case SSSD should just skip those
> entires and return ssh-keys for every valid certificate.
>
> You can test the patch even without a real certificate by e.g. adding a
> ssh-key to an IPA user object. Then 'sss_ssh_authorizedkeys username'
> should return this key. If you now add some random data the the
> userCertificate object of the same user, call 'sss_cache -E' and call
> 'sss_ssh_authorizedkeys username' again, you get nothing because the
> random data cannot be validated and hence the whole request is aborted.
> With the attached patch sss_ssh_authorizedkeys should return the ssh-key
> again.
>
> bye,
> Sumit
> From 540c69184a128bb840c7f41cabfb0cfe62f344a7 Mon Sep 17 00:00:00 2001
> From: Sumit Bose
> Date: Thu, 2 Jun 2016 18:22:03 +0200
> Subject: [PATCH] ssh: skip invalid certificates
>
> Current an invalid certificate cause the whole ssh key lookup request to
> abort. Since it is possible that e.g. the LDAP user entry contains
> certificates where the client does not have the needed CA certificates
> for validation we should just ignore invalid certificates.
>
> Resolves https://fedorahosted.org/sssd/ticket/2977
> ---
> src/responder/ssh/sshsrv_cmd.c | 171
> ++---
> 1 file changed, 126 insertions(+), 45 deletions(-)
>
> diff --git a/src/responder/ssh/sshsrv_cmd.c b/src/responder/ssh/sshsrv_cmd.c
> index
> 5954cec1ba3a688ae2d14f2304d722d91c26d592..51922d59a8150ceb7cd96e728c1c482b373639a8
> 100644
> --- a/src/responder/ssh/sshsrv_cmd.c
> +++ b/src/responder/ssh/sshsrv_cmd.c
> @@ -781,9 +781,94 @@ ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx)
> return EOK;
> }
>
> +static errno_t get_valid_certs_keys(TALLOC_CTX *mem_ctx,
> +struct ssh_cmd_ctx *cmd_ctx,
> +struct ldb_message_element *el_cert,
> +struct ssh_ctx *ssh_ctx,
> +struct ldb_message_element **_el_res)
> +{
> +TALLOC_CTX *tmp_ctx;
> +uint8_t *key;
> +size_t key_len;
> +char *cert_verification_opts;
> +struct cert_verify_opts *cert_verify_opts;
> +int ret;
> +struct ldb_message_element *el_res;
> +struct cli_ctx *cctx = cmd_ctx->cctx;
> +size_t d;
> +
> +if (el_cert == NULL) {
> +DEBUG(SSSDBG_TRACE_ALL, "Mssing element, nothing to do.\n");
> +return EOK;
> +}
> +
> +tmp_ctx = talloc_new(NULL);
> +if (tmp_ctx == NULL) {
> +DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
> +return ENOMEM;
> +}
> +
[...]
> +if (el_res->num_values == 0) {
> +*_el_res = NULL;
> +} else {
> +*_el_res = talloc_steal(mem_ctx, el_res);
> +}
> +
> +return EOK;
You need to free tmp_ctx in this function.
> +}
> +
The rest of the patch LGTM.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCHES] p11: add no_verification option
On Tue, Jun 07, 2016 at 02:42:56PM +0200, Jakub Hrozek wrote: > On Mon, May 30, 2016 at 04:32:20PM +0200, Sumit Bose wrote: > > > oops, yes I guess this would be a good idea. I'll send a new patch. > > > > > > > new version attached. > > > > bye, > > Sumit > > One last question, do we want to add the ocsp_default_responder and > ocsp_default_responder_signing_cert options to configAPI? No, because I think the configAPI is currently not capable of this because both are only allowed options to certificate_verification as e.g. no_ocsp or no_verification. bye, Sumit > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCHES] p11: add no_verification option
On Tue, Jun 07, 2016 at 03:11:49PM +0200, Sumit Bose wrote: > On Tue, Jun 07, 2016 at 02:42:56PM +0200, Jakub Hrozek wrote: > > On Mon, May 30, 2016 at 04:32:20PM +0200, Sumit Bose wrote: > > > > oops, yes I guess this would be a good idea. I'll send a new patch. > > > > > > > > > > new version attached. > > > > > > bye, > > > Sumit > > > > One last question, do we want to add the ocsp_default_responder and > > ocsp_default_responder_signing_cert options to configAPI? > > No, because I think the configAPI is currently not capable of this > because both are only allowed options to certificate_verification as > e.g. no_ocsp or no_verification. OK, makes sense. Thank you, ACK. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] DEBUG: Add `debug` alias for debug_level
On Fri, Jun 03, 2016 at 09:34:46AM -0400, Stephen Gallagher wrote: > On 04/28/2016 09:30 AM, Lukas Slebodnik wrote: > > On (27/04/16 15:18), Stephen Gallagher wrote: > >> On 04/27/2016 05:57 AM, Pavel Březina wrote: > >>> On 04/26/2016 05:08 PM, Stephen Gallagher wrote: > Our users constantly make the mistake of typing `debug = 9` in the > sssd.conf > instead of `debug_level = 9` as would be correct. This happens > frequently-enough > that we should just alias it rather than continue to have people make > mistakes. > > Resolves: > https://fedorahosted.org/sssd/ticket/2999 > >>> > >>> I don't really oppose but I'd rather print a warning instead of aliasing > >>> it, > >>> otherwise we can end up aliasing everything. It may be done as part of > >>> configuration check patches that should detect typos.' > >> > >> > >> Yeah, I don't want this to become a common thing (we shouldn't really be > >> aliasing anything), but this is such a *common* mistake that it's > >> bordering on > >> ridiculous not to just make an exception here. > >> > >> When you get right down to it, most projects use the more abbreviated term > >> "debug" anyway, so we're kind of the outlier. > >> > >> (You will notice I intentionally didn't add it to the manual; this is > >> meant to > >> be a hidden convenience feature, not the primary method. Also, > >> `debug_level` > >> will always overrule `debug` if both are present.) > >> > > I don't prefer undocumeted options. > > If we really want to have an alias then it should be documented. > > Otherwise users might wonder why it magicaly works. > > > > The option/alias will need to be added to the list of valid options anyway > > with the config validation (coming soon) > > > > Ok, I added documentation and tests for it. See attached patch. > I'm fine with the patch and since another user got confused about debug vs debug_level on #sssd recently, I would like to merge the patch. Petr already ack-ed the previous versions and I'm fine with the amendments. Anyone against pushing the patch? If not, I'll push it.. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] ssh: skip invalid certificates
On Tue, Jun 07, 2016 at 03:08:36PM +0200, Jakub Hrozek wrote:
> On Fri, Jun 03, 2016 at 08:17:01PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > currently the code which generates ssh key from the public keys in the
> > user certificates fails if one certificate cannot be validated and
> > terminates the whole request. It is of course valid that the user entry
> > might contain certificates which SSSD cannot validate and since we just
> > won't generate a ssh-key in this case SSSD should just skip those
> > entires and return ssh-keys for every valid certificate.
> >
> > You can test the patch even without a real certificate by e.g. adding a
> > ssh-key to an IPA user object. Then 'sss_ssh_authorizedkeys username'
> > should return this key. If you now add some random data the the
> > userCertificate object of the same user, call 'sss_cache -E' and call
> > 'sss_ssh_authorizedkeys username' again, you get nothing because the
> > random data cannot be validated and hence the whole request is aborted.
> > With the attached patch sss_ssh_authorizedkeys should return the ssh-key
> > again.
> >
> > bye,
> > Sumit
>
> > From 540c69184a128bb840c7f41cabfb0cfe62f344a7 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose
> > Date: Thu, 2 Jun 2016 18:22:03 +0200
> > Subject: [PATCH] ssh: skip invalid certificates
> >
> > Current an invalid certificate cause the whole ssh key lookup request to
> > abort. Since it is possible that e.g. the LDAP user entry contains
> > certificates where the client does not have the needed CA certificates
> > for validation we should just ignore invalid certificates.
> >
> > Resolves https://fedorahosted.org/sssd/ticket/2977
> > ---
> > src/responder/ssh/sshsrv_cmd.c | 171
> > ++---
> > 1 file changed, 126 insertions(+), 45 deletions(-)
> >
> > diff --git a/src/responder/ssh/sshsrv_cmd.c b/src/responder/ssh/sshsrv_cmd.c
> > index
> > 5954cec1ba3a688ae2d14f2304d722d91c26d592..51922d59a8150ceb7cd96e728c1c482b373639a8
> > 100644
> > --- a/src/responder/ssh/sshsrv_cmd.c
> > +++ b/src/responder/ssh/sshsrv_cmd.c
> > @@ -781,9 +781,94 @@ ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx)
> > return EOK;
> > }
> >
> > +static errno_t get_valid_certs_keys(TALLOC_CTX *mem_ctx,
> > +struct ssh_cmd_ctx *cmd_ctx,
> > +struct ldb_message_element *el_cert,
> > +struct ssh_ctx *ssh_ctx,
> > +struct ldb_message_element **_el_res)
> > +{
> > +TALLOC_CTX *tmp_ctx;
> > +uint8_t *key;
> > +size_t key_len;
> > +char *cert_verification_opts;
> > +struct cert_verify_opts *cert_verify_opts;
> > +int ret;
> > +struct ldb_message_element *el_res;
> > +struct cli_ctx *cctx = cmd_ctx->cctx;
> > +size_t d;
> > +
> > +if (el_cert == NULL) {
> > +DEBUG(SSSDBG_TRACE_ALL, "Mssing element, nothing to do.\n");
> > +return EOK;
> > +}
> > +
> > +tmp_ctx = talloc_new(NULL);
> > +if (tmp_ctx == NULL) {
> > +DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
> > +return ENOMEM;
> > +}
> > +
>
> [...]
>
> > +if (el_res->num_values == 0) {
> > +*_el_res = NULL;
> > +} else {
> > +*_el_res = talloc_steal(mem_ctx, el_res);
> > +}
> > +
> > +return EOK;
>
> You need to free tmp_ctx in this function.
ah. sorry, I added it to the caller and forgot to add it to the new
function. New version attached.
bye,
Sumit
> > +}
> > +
>
> The rest of the patch LGTM.
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
From 6c8776a370fde6caae653a322a7ca358f31e4de3 Mon Sep 17 00:00:00 2001
From: Sumit Bose
Date: Thu, 2 Jun 2016 18:22:03 +0200
Subject: [PATCH] ssh: skip invalid certificates
Current an invalid certificate cause the whole ssh key lookup request to
abort. Since it is possible that e.g. the LDAP user entry contains
certificates where the client does not have the needed CA certificates
for validation we should just ignore invalid certificates.
Resolves https://fedorahosted.org/sssd/ticket/2977
---
src/responder/ssh/sshsrv_cmd.c | 179 ++---
1 file changed, 134 insertions(+), 45 deletions(-)
diff --git a/src/responder/ssh/sshsrv_cmd.c b/src/responder/ssh/sshsrv_cmd.c
index
5954cec1ba3a688ae2d14f2304d722d91c26d592..ba3b694d97821696ca0218c279c1f9d9859ab13e
100644
--- a/src/responder/ssh/sshsrv_cmd.c
+++ b/src/responder/ssh/sshsrv_cmd.c
@@ -781,9 +781,102 @@ ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx)
return EOK;
}
+static errno_t get_valid_certs_keys(TALLOC_CTX *mem_ctx,
+struct ssh_cmd_ctx *cmd_ctx,
+struct ldb_message_element *el_cert,
+
[SSSD] Re: [PATCHES] ipa: add support for certificate overrides
On Tue, Jun 07, 2016 at 02:55:40PM +0200, Sumit Bose wrote: > On Tue, Jun 07, 2016 at 01:56:10PM +0200, Jakub Hrozek wrote: > > On Tue, Jun 07, 2016 at 12:28:22PM +0200, Sumit Bose wrote: > > > sure, here you are. > > > > > > bye, > > > Sumit > > > > Hmm, are these the correct patches? > > > > /home/remote/jhrozek/devel/sssd/src/db/sysdb_views.c: In function > > 'sysdb_search_override_by_cert': > > /home/remote/jhrozek/devel/sssd/src/db/sysdb_views.c:880:11: error: too > > many arguments to function 'sss_cert_derb64_to_ldap_filter' > > ret = sss_cert_derb64_to_ldap_filter(tmp_ctx, cert, SYSDB_USER_CERT, > > NULL, > >^ > > In file included from > > /home/remote/jhrozek/devel/sssd/src/db/sysdb_views.c:23:0: > > /home/remote/jhrozek/devel/sssd/src/util/cert.h:40:9: note: declared here > > errno_t sss_cert_derb64_to_ldap_filter(TALLOC_CTX *mem_ctx, const char > > *derb64, > > ^ > > ah, sorry, I picked the patches from a wrong branch. > > Please try the new version. OK, this looks better, but there CI still complains on Debian: /bin/bash ./libtool --tag=CC --mode=link gcc -Wall -Wshadow -Wstrict-prototypes -Wpointer-arith -Wcast-qual -Wcast-align -Wwrite-strings -Wundef -Werror-implicit-function-declaration -Winit-self -Wmissing-include-dirs -fno-strict-aliasing -std=gnu99 -g3 -O2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -o proxy_child src/providers/proxy/proxy_child-proxy_child.o src/providers/proxy_child-data_provider_iface_generated.o -lpam -ltalloc -ltevent -ltalloc -lpopt -lldb -ldbus-1 -lpcre -lini_config -lbasicobjects -lref_array -lcollection -lcollection -ldhash -llber -lldap -lselinux -ltdb libsss_util.la libsss_crypt.la libsss_debug.la libsss_child.la /usr/bin/ld: src/responder/nss/nsssrv_cmd.o: undefined reference to symbol 'sss_cert_derb64_to_pem' //var/lib/jenkins/workspace/ci/label/debian_testing/ci-build-debug/.libs/libsss_cert.so: //error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status Makefile:10585: recipe for target 'sssd_nss' failed make[2]: *** [sssd_nss] Error 1 make[2]: *** Waiting for unfinished jobs... CI link: http://sssd-ci.duckdns.org/logs/job/44/61/debian_testing/ci-build-debug/ci-make-tests.log ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] systemtap-based performance probes
On Mon, Jun 06, 2016 at 05:50:03PM +0200, Lukas Slebodnik wrote:
> >diff --git a/src/external/systemtap.m4 b/src/external/systemtap.m4
> >new file mode 100644
> >index
> >..d1caa2017f0730394339f0a439046df6b56cb2ba
> >--- /dev/null
> >+++ b/src/external/systemtap.m4
> >@@ -0,0 +1,35 @@
> >+dnl A macro to check the availability of systemtap user-space probes
> >+AC_DEFUN([AM_CHECK_SYSTEMTAP],
> >+[
> >+AC_ARG_ENABLE([systemtap],
> >+[AS_HELP_STRING([--enable-systemtap],
> >+[Enable inclusion of systemtap trace
> >support])],
> >+[ENABLE_SYSTEMTAP="${enableval}"], [ENABLE_SYSTEMTAP='no'])
> >+
> >+if test "x${ENABLE_SYSTEMTAP}" = xyes; then
> >+AC_CHECK_PROGS(DTRACE, dtrace)
> >+if test -z "$DTRACE"; then
> >+AC_MSG_ERROR([dtrace not found])
> >+fi
> >+
> >+AC_CHECK_HEADER([sys/sdt.h], [SDT_H_FOUND='yes'],
> ^^^
> this variable is not used anywhere.
> it would be better to use default action
> "[]"
Fixed
> >+[SDT_H_FOUND='no';
> >+AC_MSG_ERROR([systemtap support needs sys/sdt.h
> >header])])
> e.g. AC_CHECK_HEADERS([check.h],,AC_MSG_ERROR([Could not find CHECK headers]))
> >+
> >+AC_DEFINE([HAVE_SYSTEMTAP], [1], [Define to 1 if systemtap is
> >enabled])
> >+HAVE_SYSTEMTAP=1
> ^^
> it should already be set to 1 by AC_DEFINE.
Didn't seem so, I thought AC_DEFINE really only sets the config.h
variable. When I removed this line, the AC_CONDITIONAL wasn't evaluated.
> >+
> >+AC_ARG_WITH([tapset-install-dir],
> >+[AS_HELP_STRING([--with-tapset-install-dir],
> ^
>It would be good to append here string "=DIR"
>So it will be clear that there is expected argument
>We already have it for "--with-systemdunitdir"
Fixed
> >+[The absolute path where the tapset dir
> >will be installed])],
> >+[if test "x${withval}" = x; then
> >+tapset_dir="\$(datadir)/systemtap/tapset"
> >+else
> >+tapset_dir="${withval}"
> >+fi],
> It can be simplified to 'tapset_dir="${withval}"' after
> updating help string.
Fixed
>
> >+[tapset_dir="\$(datadir)/systemtap/tapset"])
>
>this default value could be mentioned in
>help string as well.
>@see output of ./configure --help
>(with-pubconf-path, with-pipe-path ...)
Fixed
>
> >From c1a4fba84679bb4ce10badcc9458088e4a94d281 Mon Sep 17 00:00:00 2001
> >From: Jakub Hrozek
> >Date: Mon, 29 Feb 2016 13:20:28 +0100
> >Subject: [PATCH 04/10] SYSDB: Add systemtap probes to track sysdb
> >transactions
> >
> >Actually adds marks for sysdb transactions that receive the transaction
> >nesting level as an argument. The nesting is passed on from probes to
> >marks along with a human-friendly description.
> >
> >The transaction commit is decorated with two probes, before and after.
> >This would allow the caller to distinguish between the time we spend in
> >the transaction (which might be important, because if a transaction is
> >active on an ldb context, even the readers are blocked before the
> >transaction completes) and the time we spend commiting the transaction
> >(which is important because that's when the disk writes occur)
> >
> >The probes would be installed into /usr/share/systemtap/tapset on RHEL
> >and Fedora. This is in line with systemtap's paths which are described
> >in detail in "man 7 stappaths".
> >---
> > Makefile.am | 4 +++-
> > configure.ac| 1 +
> > src/db/sysdb.c | 8
> > src/systemtap/sssd.stp.in | 32
> > src/systemtap/sssd_probes.d | 4
> > 5 files changed, 48 insertions(+), 1 deletion(-)
> > create mode 100644 src/systemtap/sssd.stp.in
> >
> >diff --git a/Makefile.am b/Makefile.am
> >index
> >33930759ab82b65643a9a0a071fd92b025dab145..1b4ba3cc651b29c55f7b43c90bc479f584a911e2
> > 100644
> >--- a/Makefile.am
> >+++ b/Makefile.am
> >@@ -81,6 +81,7 @@ krb5rcachedir = @krb5rcachedir@
> > sudolibdir = @sudolibpath@
> > polkitdir = @polkitdir@
> > pamconfdir = $(sysconfdir)/pam.d
> >+systemtap_tapdir = @tapset_dir@
> >
> > UNICODE_LIBS=@UNICODE_LIBS@
> >
> >@@ -1081,6 +1082,8 @@ SYSTEMTAP_PROBES = \
