Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
Apologies for the delay. On Mon, Jul 15, 2013 at 08:30:03PM +0300, Alexander Bokovoy wrote: Here is the logic: 0. Configuration is performed by setting schema-compat-lookup-sssd: user|group schema-compat-sssd-min-id: value in corresponding schema-compat plugin tree (cn=users and cn=groups). If schema-compat-sssd-min-id is not set, it will default to 1000. It is used to filter out attempts to fetch system users (1000 on Fedora by default). 1. On query, we parse query filter to identify what type of request is this: user or group lookup and then issue getpwnam_r()/getgrnam_r() and getsidbyid() for libsss_nss_idmap to fetch all needed information. SSSD caches these requests they should be relatively fast. 2. Once we served the request, it is cached in schema-compat cache map. The entry in the cache is currently not expired explicitly but I'm working on expiring it on wrong authentication -- if PAM stack returns a response telling there is no such user. 3. Authentication bind for cached entries is done via PAM service 'system-auth'. If HBAC rule 'allow_all' is disabled in FreeIPA, one needs to create a rule with service 'system-auth' and allow all users to access it on IPA masters. Since system-auth is never used explicitly by any application (it is always included through PAM stack and only top-level PAM service is used to drive the HBAC ruleset), there is no problem. PAM authentication code is taken from pam_passthru DS plugin. We cannot use it unchanged because pam_passthru expects that LDAP entry will exist in DS, while it is not true for these synthetic entries representing trusted domain users. On Fedora one needs pam-devel and libsss_nss_idmap-devel to build the plugin with new functionality. The bits about how to configure this facility need to be in the documentation somewhere. Right now there is none being added, and no new self-tests. diff --git a/configure.ac b/configure.ac index 8d7cbe1..4a47d36 100644 --- a/configure.ac +++ b/configure.ac @@ -309,6 +309,47 @@ AC_SUBST(ASYNCNS_CFLAGS) AC_SUBST(ASYNCNS_LIBS) fi +AC_ARG_WITH(sss_nss_idmap, + AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]), + use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO) +if pkg-config sss_nss_idmap 2 /dev/null ; then + if test x$use_sss_nss_idmap != xno ; then + AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have libsss_nss_idmap.]) + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) + else + SSS_NSS_IDMAP_CFLAGS= + SSS_NSS_IDMAP_LIBS= + fi +else + if test $use_sss_idmap = yes ; then Should this reference to $use_sss_idmap be referring to $use_sss_nss_idmap instead? + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) + else + SSS_NSS_IDMAP_CFLAGS= + SSS_NSS_IDMAP_LIBS= + fi +fi +AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x]) I suspect this'll need to quote SSS_NSS_IDMAP_LIBS here, in case its value ever starts to include whitespace. +if x$SSS_NSS_IDMAP_LIBS != x ; then Likewise here. + AC_CHECK_HEADERS(pam.h) + if test x$ac_cv_header_pam_h = xno ; then + use_pam=yes + else + use_pam=no + fi + + if test $use_pam = yes ; then + PAM_CFLAGS= + PAM_LIBS=-lpam + else + AC_ERROR([pam.h not found and it is required for SSSD mode]) + fi + AC_SUBST(PAM_CFLAGS) + AC_SUBST(PAM_LIBS) +fi Jakub already noted that this should be checking for security/pam_appl.h. @@ -401,6 +442,13 @@ AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_RDN_ATTR,$rdnattr, attrattr=schema-compat-entry-attribute AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_ATTR_ATTR,$attrattr, [Define to name of the attribute which is used to specify attributes to be used when constructing entries.]) +sssdattr=schema-compat-lookup-sssd +AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_SSSD_ATTR,$sssdattr, +[Define to name of the attribute which dictates whether or not SSSD on FreeIPA master is consulted about trusted domains' users.]) Is this a boolean attribute? diff --git a/src/back-sch-pam.c b/src/back-sch-pam.c new file mode 100644 index 000..3266261 --- /dev/null +++ b/src/back-sch-pam.c @@ -0,0 +1,361 @@ +/** BEGIN COPYRIGHT BLOCK + * This Program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation; version 2 of the License. + * + * This Program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. + * + * You should have received a copy of the GNU
Re: [Freeipa-devel] [PATCH] 436 Remove word 'field' from GECOS param label
On 07/23/2013 01:31 AM, Dmitri Pal wrote: On 07/22/2013 08:38 AM, Petr Vobornik wrote: On 07/19/2013 11:19 PM, Dmitri Pal wrote: On 07/19/2013 09:26 AM, Jan Pazdziora wrote: On Fri, Jul 19, 2013 at 03:17:49PM +0200, Petr Vobornik wrote: Disclaimer: I have no strong feelings in this matter, it just looks weird to me, so I'm OK with not doing it if it's general consensus. Originally we wanted to do this change in https://fedorahosted.org/freeipa/ticket/3569 but it was not done because of string freeze. I guess you can add field suffix to every field from /etc/password when you use it in a sentence but that doesn't necessary mean that You can. But gid exists as a concept beyond /etc/passwd. So does home directory. The GECOS field value does not, really. it's its name. man 5 passwd doesn't use word 'field' next to GECOS in fields description/list either. IMO our use case is the same. It says: GECOSThis field (sometimes called the comment field) [...] The gcos field in the password file was [...] Historically correct label would probably be 'GECOS identity' but that's not usable today as it's purpose is more general. Do we have tips in the UI? May be we should add them in future to provide extra information about meaning of the field or button. We do, but UX is not the best. 'doc' defined in .py files is displayed as a textbox tooltip. This feature is not apparent and for most of the fields the tooltip is the same as label. There is an idea to display a question mark icon next to textboxes to draw an attention to a presence of the doc text and provide it in a tooltip. I like the way how it's done in Alchemy UI: http://www.ui-alchemy.org/form You can notice there an additional information (string length limitation, example...) when field is focused. I went through open tickets and found few which touches the topic: - https://fedorahosted.org/freeipa/ticket/2912 [RFE] [Web UI] Use doc as field's tooltip instead of label * Seems to be already implemented. Then please close it and move it to the milestone where it was closed (if possible). - https://fedorahosted.org/freeipa/ticket/2413 [RFE]: add in UI examples of what is the requested field - https://fedorahosted.org/freeipa/ticket/3661 [RFE] IPA Web UI: When adding new reverse zone in DNS there could be an example of expected address format * Looks like duplicate of #2413 Right, please proceed with closing as duplicate then. I would prefer leaving only ticket 2413 open instead of changing description of 2912 though. Martin IMO we should close #2413 and maybe #2912 or change #2912 to cover the above mentioned proposals (in case of agreement). For now I think GECOS would probably be good enough. Adding field makes it more precise but looks weird. Sounds reasonable. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Tue, 23 Jul 2013, Nalin Dahyabhai wrote: Apologies for the delay. Thanks for the review! One short comment -- PAM code is from PAM pass-through plugin from 389-ds. That's the reason why its code doesn't follow slapi-nis way and why it has that license. I tried to keep it mostly intact to share changes but looking at git log it gets roughly two commits per year so maybe it is better to rework it completely. I'll address other comments and will send updated version for the review today. This was my first sizable SLAPI code so errors are inevitable. On Mon, Jul 15, 2013 at 08:30:03PM +0300, Alexander Bokovoy wrote: Here is the logic: 0. Configuration is performed by setting schema-compat-lookup-sssd: user|group schema-compat-sssd-min-id: value in corresponding schema-compat plugin tree (cn=users and cn=groups). If schema-compat-sssd-min-id is not set, it will default to 1000. It is used to filter out attempts to fetch system users (1000 on Fedora by default). 1. On query, we parse query filter to identify what type of request is this: user or group lookup and then issue getpwnam_r()/getgrnam_r() and getsidbyid() for libsss_nss_idmap to fetch all needed information. SSSD caches these requests they should be relatively fast. 2. Once we served the request, it is cached in schema-compat cache map. The entry in the cache is currently not expired explicitly but I'm working on expiring it on wrong authentication -- if PAM stack returns a response telling there is no such user. 3. Authentication bind for cached entries is done via PAM service 'system-auth'. If HBAC rule 'allow_all' is disabled in FreeIPA, one needs to create a rule with service 'system-auth' and allow all users to access it on IPA masters. Since system-auth is never used explicitly by any application (it is always included through PAM stack and only top-level PAM service is used to drive the HBAC ruleset), there is no problem. PAM authentication code is taken from pam_passthru DS plugin. We cannot use it unchanged because pam_passthru expects that LDAP entry will exist in DS, while it is not true for these synthetic entries representing trusted domain users. On Fedora one needs pam-devel and libsss_nss_idmap-devel to build the plugin with new functionality. The bits about how to configure this facility need to be in the documentation somewhere. Right now there is none being added, and no new self-tests. diff --git a/configure.ac b/configure.ac index 8d7cbe1..4a47d36 100644 --- a/configure.ac +++ b/configure.ac @@ -309,6 +309,47 @@ AC_SUBST(ASYNCNS_CFLAGS) AC_SUBST(ASYNCNS_LIBS) fi +AC_ARG_WITH(sss_nss_idmap, + AS_HELP_STRING([--with-sss-nss-idmap], [use libsss_nss_idmap]), + use_sss_nss_idmap=$withval,use_sss_nss_idmap=AUTO) +if pkg-config sss_nss_idmap 2 /dev/null ; then + if test x$use_sss_nss_idmap != xno ; then + AC_DEFINE(HAVE_SSS_NSS_IDMAP,1,[Define if you have libsss_nss_idmap.]) + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) + else + SSS_NSS_IDMAP_CFLAGS= + SSS_NSS_IDMAP_LIBS= + fi +else + if test $use_sss_idmap = yes ; then Should this reference to $use_sss_idmap be referring to $use_sss_nss_idmap instead? + PKG_CHECK_MODULES(SSS_NSS_IDMAP,sss_nss_idmap) + else + SSS_NSS_IDMAP_CFLAGS= + SSS_NSS_IDMAP_LIBS= + fi +fi +AM_CONDITIONAL([SSS_NSS_IDMAP], [test x$SSS_NSS_IDMAP_LIBS != x]) I suspect this'll need to quote SSS_NSS_IDMAP_LIBS here, in case its value ever starts to include whitespace. +if x$SSS_NSS_IDMAP_LIBS != x ; then Likewise here. + AC_CHECK_HEADERS(pam.h) + if test x$ac_cv_header_pam_h = xno ; then + use_pam=yes + else + use_pam=no + fi + + if test $use_pam = yes ; then + PAM_CFLAGS= + PAM_LIBS=-lpam + else + AC_ERROR([pam.h not found and it is required for SSSD mode]) + fi + AC_SUBST(PAM_CFLAGS) + AC_SUBST(PAM_LIBS) +fi Jakub already noted that this should be checking for security/pam_appl.h. @@ -401,6 +442,13 @@ AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_RDN_ATTR,$rdnattr, attrattr=schema-compat-entry-attribute AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_ATTR_ATTR,$attrattr, [Define to name of the attribute which is used to specify attributes to be used when constructing entries.]) +sssdattr=schema-compat-lookup-sssd +AC_DEFINE_UNQUOTED(SCH_CONTAINER_CONFIGURATION_SSSD_ATTR,$sssdattr, + [Define to name of the attribute which dictates whether or not SSSD on FreeIPA master is consulted about trusted domains' users.]) Is this a boolean attribute? diff --git a/src/back-sch-pam.c b/src/back-sch-pam.c new file mode 100644 index 000..3266261 --- /dev/null +++ b/src/back-sch-pam.c @@ -0,0 +1,361 @@ +/** BEGIN COPYRIGHT BLOCK + * This Program is
Re: [Freeipa-devel] DNSSEC support design considerations: key material handling
On 19.7.2013 19:55, Simo Sorce wrote: I will reply to the rest of the message later if necessary, still digesting some of your answers, but I wanted to address the following first. On Fri, 2013-07-19 at 18:29 +0200, Petr Spacek wrote: The most important question at the moment is What can we postpone? How fragile it can be for shipping it as part of Fedora 20? Could we declare DNSSEC support as technology preview/don't use it for anything serious? Until we figur out proper management in LDAP we will be a bit stuck, esp if we want to consider usin the 'somthing' that stores keys instead of toring them stright in LDAP. So maybe we can start with allowing just one server to do DNSSEC and source keys from files for now ? The problem is that DNSSEC deployment *on single domain* is 'all or nothing': All DNS servers have to support DNSSEC otherwise the validation on client side can fail randomly. Note that *parent* zone indicates that the particular child zone is secured with DNSSEC by sending DS (delegation signer) record to the client. Validation will fail if client receives DS record from the parent but no signatures are present in data from 'child' zone itself. This prevents downgrade (DNSSEC = plain DNS) attacks. As a result, we have only two options: One DNS server with DNSSEC enabled or arbitrary number DNS servers without DNSSEC, which is very unfortunate. as soon as we have that workign we should also have clearer plans about how we manage keys in LDAP (or elsewhere). -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0076] Use AD LDAP probing to create trusted domain ID range
This improved revision creates ranges of sizes that are multiples of default
range size (20).
Tomas
--
/ Alexander Bokovoy
From 629428d12fcfafdf2695dad2b2861980a18cceb4 Mon Sep 17 00:00:00 2001
From: Tomas Babej tba...@redhat.com
Date: Wed, 17 Jul 2013 15:55:36 +0200
Subject: [PATCH] Use AD LDAP probing to create trusted domain ID range
When creating a trusted domain ID range, probe AD DC to get
information about ID space leveraged by POSIX users already
defined in AD, and create an ID range with according parameters.
For more details:
http://www.freeipa.org/page/V3/Use_posix_attributes_defined_in_AD
https://fedorahosted.org/freeipa/ticket/3649
---
API.txt | 2 +-
VERSION | 2 +-
ipalib/plugins/trust.py | 111 +++---
ipaserver/dcerpc.py | 164 +-
ipaserver/install/installutils.py | 7 +-
5 files changed, 232 insertions(+), 54 deletions(-)
diff --git a/API.txt b/API.txt
index 44b3dd444964c8dac595177f8601c82d0235eabe..2773f3d5c88ffa05ab7587dd9f0df97b350e45ca 100644
--- a/API.txt
+++ b/API.txt
@@ -3283,7 +3283,7 @@ arg: Str('cn', attribute=True, cli_name='realm', multivalue=False, primary_key=T
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Int('base_id?', cli_name='base_id')
-option: Int('range_size?', autofill=True, cli_name='range_size', default=20)
+option: Int('range_size?', cli_name='range_size')
option: StrEnum('range_type?', cli_name='range_type', values=(u'ipa-ad-trust-posix', u'ipa-ad-trust'))
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('realm_admin?', cli_name='admin')
diff --git a/VERSION b/VERSION
index 8606d724e6c8c785ba9d554ed3effa905573e25f..8a36c6304d7cfe0452eae5dbdc7a5d2951ab 100644
--- a/VERSION
+++ b/VERSION
@@ -89,4 +89,4 @@ IPA_DATA_VERSION=2010061412
# #
IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=61
+IPA_API_VERSION_MINOR=62
diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 965ff76bb7968a8d2784e67478eb824dc3f0621b..b19a27ecabb62abdfbc3c7927a8f78e83ad6821d 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -20,9 +20,13 @@
from ipalib.plugins.baseldap import *
from ipalib.plugins.dns import dns_container_exists
+from ipapython.ipautil import realm_to_suffix
from ipalib import api, Str, StrEnum, Password, _, ngettext
from ipalib import Command
from ipalib import errors
+from ldap import SCOPE_SUBTREE
+from time import sleep
+
try:
import pysss_murmur #pylint: disable=F0401
_murmur_installed = True
@@ -292,8 +296,6 @@ sides.
Int('range_size?',
cli_name='range_size',
label=_('Size of the ID range reserved for the trusted domain'),
-default=DEFAULT_RANGE_SIZE,
-autofill=True
),
StrEnum('range_type?',
label=_('Range type'),
@@ -313,7 +315,7 @@ sides.
result = self.execute_ad(full_join, *keys, **options)
if not old_range:
-self.add_range(range_name, dom_sid, **options)
+self.add_range(range_name, dom_sid, *keys, **options)
trust_filter = cn=%s % result['value']
ldap = self.obj.backend
@@ -418,9 +420,7 @@ sides.
'Only the ipa-ad-trust and ipa-ad-trust-posix are '
'allowed values for --range-type when adding an AD '
'trust.'
-)
-
-)
+))
base_id = options.get('base_id')
range_size = options.get('range_size') != DEFAULT_RANGE_SIZE
@@ -468,9 +468,96 @@ sides.
return old_range, range_name, dom_sid
-def add_range(self, range_name, dom_sid, **options):
-base_id = options.get('base_id')
-if not base_id:
+def add_range(self, range_name, dom_sid, *keys, **options):
+
+First, we try to derive the parameters of the ID range based on the
+information contained in the Active Directory.
+
+If that was not successful, we go for our usual defaults (random base,
+range size 200 000, ipa-ad-trust range type).
+
+Any of these can be overriden by passing appropriate CLI options
+to the trust-add command.
+
+
+range_size = None
+range_type = None
+base_id = None
+
+# First, get information about ID space from AD
+# However, we skip this step if other than ipa-ad-trust-posix
+# range type is enforced
+
+if options.get('range_type', None) in (None, u'ipa-ad-trust-posix'):
+
+# Get the base dn
+domain = keys[-1]
+basedn = realm_to_suffix(domain)
+
+
Re: [Freeipa-devel] [PATCH] 436 Remove word 'field' from GECOS param label
On 07/22/2013 05:33 PM, Ana Krivokapic wrote:
On 07/22/2013 09:01 AM, Martin Kosek wrote:
On 07/19/2013 11:19 PM, Dmitri Pal wrote:
On 07/19/2013 09:26 AM, Jan Pazdziora wrote:
On Fri, Jul 19, 2013 at 03:17:49PM +0200, Petr Vobornik wrote:
Disclaimer: I have no strong feelings in this matter, it just looks
weird to me, so I'm OK with not doing it if it's general consensus.
Originally we wanted to do this change in
https://fedorahosted.org/freeipa/ticket/3569 but it was not done
because of string freeze.
I guess you can add field suffix to every field from /etc/password
when you use it in a sentence but that doesn't necessary mean that
You can. But gid exists as a concept beyond /etc/passwd. So does home
directory. The GECOS field value does not, really.
it's its name. man 5 passwd doesn't use word 'field' next to GECOS
in fields description/list either. IMO our use case is the same.
It says:
GECOS This field (sometimes called the comment field)
[...]
The gcos field in the password file was
[...]
Historically correct label would probably be 'GECOS identity' but
that's not usable today as it's purpose is more general.
Do we have tips in the UI?
May be we should add them in future to provide extra information about
meaning of the field or button.
For now I think GECOS would probably be good enough.
Adding field makes it more precise but looks weird.
+1 for just GECOS. Petr showed me both variants in the UI and GECOS field
really looked weird.
Martin
+1 for removing the word 'field'. The phrase 'GECOS field' also exists in the
following files:
install/ui/test/data/ipa_init_commands.json
install/ui/test/data/ipa_init_objects.json
install/ui/test/data/json_metadata.json
So it should be fixed there as well.
Fixed.
--
Petr Vobornik
From b0965b0c6cb056655a138205e0e205f21ddb2ee4 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Fri, 19 Jul 2013 13:35:17 +0200
Subject: [PATCH] Remove word 'field' from GECOS param label
No other param/field has 'field' in a label.
---
install/ui/test/data/ipa_init_commands.json | 12 ++--
install/ui/test/data/ipa_init_objects.json | 4 ++--
install/ui/test/data/json_metadata.json | 4 ++--
ipalib/plugins/user.py | 2 +-
4 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/install/ui/test/data/ipa_init_commands.json b/install/ui/test/data/ipa_init_commands.json
index f83059bcdb3eefa3b254c764c5d42234f697fcfd..3a812ef5143972c1b043bf5ef41be3e7dafe1a28 100644
--- a/install/ui/test/data/ipa_init_commands.json
+++ b/install/ui/test/data/ipa_init_commands.json
@@ -16976,9 +16976,9 @@
{
attribute: true,
class: Str,
-doc: GECOS field,
+doc: GECOS,
flags: [],
-label: GECOS field,
+label: GECOS,
name: gecos,
noextrawhitespace: true,
type: unicode
@@ -17350,9 +17350,9 @@
{
attribute: true,
class: Str,
-doc: GECOS field,
+doc: GECOS,
flags: [],
-label: GECOS field,
+label: GECOS,
name: gecos,
noextrawhitespace: true,
query: true,
@@ -17807,9 +17807,9 @@
{
attribute: true,
class: Str,
-doc: GECOS field,
+doc: GECOS,
flags: [],
-label: GECOS field,
+label: GECOS,
name: gecos,
noextrawhitespace: true,
type: unicode
diff --git a/install/ui/test/data/ipa_init_objects.json b/install/ui/test/data/ipa_init_objects.json
index 5d1fd65aaa2be6e1ed346ebb6072f618db944cf6..7d8baed33a073ea7a410fd44ac486360a627d3cf 100644
--- a/install/ui/test/data/ipa_init_objects.json
+++ b/install/ui/test/data/ipa_init_objects.json
@@ -7671,9 +7671,9 @@
},
{
class: Str,
-doc: GECOS field,
+doc: GECOS,
flags: [],
-label: GECOS field,
+label: GECOS,
name: gecos,
noextrawhitespace: true,
type: unicode
diff --git a/install/ui/test/data/json_metadata.json b/install/ui/test/data/json_metadata.json
index a3febc1ff9f779f3274174a3ec2e4f46fe4b3391..928e5eafd66158ab41c40ce3f5c3b1d486cdf13f 100644
--- a/install/ui/test/data/json_metadata.json
Re: [Freeipa-devel] [PATCH] 436 Remove word 'field' from GECOS param label
On 07/23/2013 12:58 PM, Petr Vobornik wrote: On 07/22/2013 05:33 PM, Ana Krivokapic wrote: On 07/22/2013 09:01 AM, Martin Kosek wrote: On 07/19/2013 11:19 PM, Dmitri Pal wrote: On 07/19/2013 09:26 AM, Jan Pazdziora wrote: On Fri, Jul 19, 2013 at 03:17:49PM +0200, Petr Vobornik wrote: Disclaimer: I have no strong feelings in this matter, it just looks weird to me, so I'm OK with not doing it if it's general consensus. Originally we wanted to do this change in https://fedorahosted.org/freeipa/ticket/3569 but it was not done because of string freeze. I guess you can add field suffix to every field from /etc/password when you use it in a sentence but that doesn't necessary mean that You can. But gid exists as a concept beyond /etc/passwd. So does home directory. The GECOS field value does not, really. it's its name. man 5 passwd doesn't use word 'field' next to GECOS in fields description/list either. IMO our use case is the same. It says: GECOSThis field (sometimes called the comment field) [...] The gcos field in the password file was [...] Historically correct label would probably be 'GECOS identity' but that's not usable today as it's purpose is more general. Do we have tips in the UI? May be we should add them in future to provide extra information about meaning of the field or button. For now I think GECOS would probably be good enough. Adding field makes it more precise but looks weird. +1 for just GECOS. Petr showed me both variants in the UI and GECOS field really looked weird. Martin +1 for removing the word 'field'. The phrase 'GECOS field' also exists in the following files: install/ui/test/data/ipa_init_commands.json install/ui/test/data/ipa_init_objects.json install/ui/test/data/json_metadata.json So it should be fixed there as well. Fixed. ACK -- Regards, Ana Krivokapic Associate Software Engineer FreeIPA team Red Hat Inc. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 143-147 Improve performance with large groups
On 07/19/2013 01:10 PM, Petr Vobornik wrote:
On 07/18/2013 05:29 PM, Jan Cholasta wrote:
On 18.7.2013 17:26, Martin Kosek wrote:
On 07/18/2013 05:22 PM, Jan Cholasta wrote:
On 18.7.2013 17:07, Martin Kosek wrote:
On 07/18/2013 04:53 PM, Jan Cholasta wrote:
Added patch which adds new hidden option no_members to suppress
membership
processing for commands of all objects that have member attributes.
This can be
used by the WebUI to prevent member lookups where they are not
necessary (as we
discussed off-line with Martin and Petr).
Honza
1) Should the new option really have exclude='webui'? I thought
that Web UI is
the main and only consumer of this option.
The 'webui' context doesn't actually exist and the only meaning of
this stanza
is that the option is not shown in the output of the show_mappings
command.
2) I would clearly state this is an internal option only, e.g.
+ doc=_('INTERNAL: suppress processing of membership attributes.'),
No other internal option has this kind of thing in its doc and nobody
will see
it anyway, so we might just leave it like that IMHO.
OK.
3) It would be nice to state that this option is mutually exclusive
with --all,
but given it is internal anyway and there is no central place to
define it, we
do not need to do that.
The options are not really mutually exclusive at this point, they can be
specified together, --all takes precedence.
Well, they can be specified together, but the option is then NOOP
which could
confuse users which may have different expectations. Being explicit
helps.
I agree.
But
as I said, in this case this is not a requirement.
I agree as well :-)
Honza
Functional ACK for Honza's patch (didn't break Web UI test suite)
Attaching Web UI patch.
It:
1) Removed --all from _find and _show commands used by search pages. All
displayed attributes should be already included in default attributes.
2) Removed search_all_attributes - Not needed since introduction of paging.
3) Added --no-members options to search _show commmands.
ACK. Pushed both Petr's and Honza's patch to master and ipa-3-2.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0109-0110 Support querying AD DC when establishing trust as HTTP/ipa.server principal
On Thu, 2013-07-18 at 18:37 +0300, Alexander Bokovoy wrote: Hi! Attached patches make possible to use HTTP/ipa.server@REALM to query AD DC over LDAP immediately after trust is established. We need this to get range discovery working prior to creating range for trusted domain. The patch 0109 makes KDC hostname cached on ipadb context to avoid resolving own hostname multiple times. The patch 0110 depends on ulc_casemap patches by Nathaniel and makes exception for HTTP/ipa.server@REALM when TGT is requested and MS-PAC is asked for -- we force refreshing list of trusted domains here. More details are available in the commit logs. I do not think that changing reinit interval is the right thing to do. I would rather pass a boolean that tells reinit to check if we have any trust info, and if not unconditionally try to reinit immediately. I see that you treat the interval sort of like a boolean but then you just race hoping the previous reload w/o trust info happened more than 1 second earlier. I think and explicit bool force_reload flag would be much clearer. Otherwise ack. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] Two minor IPA KDB MS-PAC fixes
clang found one branch with undefined variable return and one unused
variable.
From 09962a9a40cd589c4694ecab4b4faa3c39e8a4a3 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Tue, 23 Jul 2013 15:07:39 +0200
Subject: [PATCH 1/2] IPA KDB MS-PAC: return ENOMEM if allocation fails
---
daemons/ipa-kdb/ipa_kdb_mspac.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index
4ddf3e8662bb55bae36777bd95f62b4f7e60c154..12959a89308e800d01c6cbc81a9d0bb5239fd5d9
100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -1842,6 +1842,7 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
krb5_princ_component(context, ks_client_princ,
0)-length) == 0)) {
ipactx = ipadb_get_context(context);
if (!ipactx) {
+kerr = ENOMEM;
goto done;
}
if (ulc_casecmp(krb5_princ_component(context, ks_client_princ,
1)-data,
--
1.8.3.1
From fcefc88a34f8c02f75ad48f484f00438634b9d0e Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Tue, 23 Jul 2013 15:07:52 +0200
Subject: [PATCH 2/2] IPA KDB MS-PAC: remove unused variable
---
daemons/ipa-kdb/ipa_kdb_mspac.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index
12959a89308e800d01c6cbc81a9d0bb5239fd5d9..87a74909602f45fff7b3820f60eca560daaa2642
100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -1930,7 +1930,6 @@ static char *get_server_netbios_name(struct ipadb_context
*ipactx)
{
char hostname[MAXHOSTNAMELEN + 1]; /* NOTE: this is 64, too little ? */
char *p;
-int ret;
strncpy(hostname, ipactx-kdc_hostname, MAXHOSTNAMELEN);
/* May miss termination */
--
1.8.3.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0109-0110 Support querying AD DC when establishing trust as HTTP/ipa.server principal
On Tue, 23 Jul 2013, Simo Sorce wrote:
On Thu, 2013-07-18 at 18:37 +0300, Alexander Bokovoy wrote:
Hi!
Attached patches make possible to use HTTP/ipa.server@REALM to query AD
DC over LDAP immediately after trust is established. We need this to get
range discovery working prior to creating range for trusted domain.
The patch 0109 makes KDC hostname cached on ipadb context to avoid
resolving own hostname multiple times.
The patch 0110 depends on ulc_casemap patches by Nathaniel and makes
exception for HTTP/ipa.server@REALM when TGT is requested and MS-PAC is
asked for -- we force refreshing list of trusted domains here.
More details are available in the commit logs.
I do not think that changing reinit interval is the right thing to do.
I would rather pass a boolean that tells reinit to check if we have any
trust info, and if not unconditionally try to reinit immediately.
I see that you treat the interval sort of like a boolean but then you
just race hoping the previous reload w/o trust info happened more than 1
second earlier.
I think and explicit bool force_reload flag would be much clearer.
Otherwise ack.
Attached is modified patch that uses 'bool force_reinit' (as function is
called ipadb_reinit_mspac).
I tested it together with updated Tomas patch 0076 which relies on these
patches so I'm going to commit whole set together.
--
/ Alexander Bokovoy
From 620736888642102f32ad68f8a28a305488bcc401 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy aboko...@redhat.com
Date: Thu, 18 Jul 2013 13:32:42 +0300
Subject: [PATCH 2/4] ipa-kdb: reinit mspac on HTTP TGT acquisition to aid
trust-add case
When trust is established, we also create idrange for the trusted domain.
With FreeIPA 3.3 these ranges can have different types, and in order to
detect which one is to create, we need to do lookup at AD LDAP server.
Such lookup requires authenticated bind. We cannot bind as user because
IPA framework operates under constrained delegation using the user's
credentials and allowing HTTP/ipa.server@REALM to impersonate the user
against trusted domain's services would require two major things:
- first, as we don't really know exact AD LDAP server names (any AD DC
can be used), constrained delegation would have to be defined against
a wild-card
- second, constrained delegation requires that target principal exists
in IPA LDAP as DN.
These two together limit use of user's ticket for the purpose of IPA
framework looking up AD LDAP.
Additionally, immediately after trust is established, issuing TGT with
MS-PAC to HTTP/ipa.server@REALM may fail due to the fact that KDB driver
did not yet refreshed its list of trusted domains -- we have limited
refresh rate of 60 seconds by default.
This patch makes possible to force re-initialization of trusted domains'
view in KDB driver if we are asked for TGT for HTTP/ipa.server@REALM.
We will need to improve refresh of trusted domains' view in KDB driver
in future to notice changes in cn=etc,$SUFFIX tree automatically.
This improvement is tracked in https://fedorahosted.org/freeipa/ticket/1302 and
https://fedorahosted.org/freeipa/ticket/3626
Part of https://fedorahosted.org/freeipa/ticket/3649
---
daemons/ipa-kdb/ipa_kdb.c | 4 ++--
daemons/ipa-kdb/ipa_kdb.h | 2 +-
daemons/ipa-kdb/ipa_kdb_mspac.c | 29 ++---
3 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c
index 51b879c..5e4d047 100644
--- a/daemons/ipa-kdb/ipa_kdb.c
+++ b/daemons/ipa-kdb/ipa_kdb.c
@@ -393,8 +393,8 @@ int ipadb_get_connection(struct ipadb_context *ipactx)
goto done;
}
-/* get adtrust options */
-ret = ipadb_reinit_mspac(ipactx);
+/* get adtrust options using default refresh interval */
+ret = ipadb_reinit_mspac(ipactx, false);
if (ret ret != ENOENT) {
/* TODO: log that there is an issue with adtrust settings */
}
diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h
index a611bc2..f4d3555 100644
--- a/daemons/ipa-kdb/ipa_kdb.h
+++ b/daemons/ipa-kdb/ipa_kdb.h
@@ -250,7 +250,7 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
krb5_authdata **tgt_auth_data,
krb5_authdata ***signed_auth_data);
-krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx);
+krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx, bool
force_reinit);
void ipadb_mspac_struct_free(struct ipadb_mspac **mspac);
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index d6c4f9a..6ffab45 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -24,6 +24,7 @@
#include ipa_mspac.h
#include talloc.h
#include syslog.h
+#include unicase.h
#include util/time.h
#include gen_ndr/ndr_krb5pac.h
@@ -1282,7 +1283,8 @@ static struct ipadb_adtrusts
*get_domain_from_realm_update(krb5_context
Re: [Freeipa-devel] [PATCH] Two minor IPA KDB MS-PAC fixes
On Tue, 23 Jul 2013, Jakub Hrozek wrote: clang found one branch with undefined variable return and one unused variable. ACK. Pushed both to master together with my 0109-0111 and Tomas' 0076 as your patches are on top of mine. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 436 Remove word 'field' from GECOS param label
On 07/23/2013 09:02 AM, Martin Kosek wrote: On 07/23/2013 01:31 AM, Dmitri Pal wrote: On 07/22/2013 08:38 AM, Petr Vobornik wrote: On 07/19/2013 11:19 PM, Dmitri Pal wrote: On 07/19/2013 09:26 AM, Jan Pazdziora wrote: On Fri, Jul 19, 2013 at 03:17:49PM +0200, Petr Vobornik wrote: Disclaimer: I have no strong feelings in this matter, it just looks weird to me, so I'm OK with not doing it if it's general consensus. Originally we wanted to do this change in https://fedorahosted.org/freeipa/ticket/3569 but it was not done because of string freeze. I guess you can add field suffix to every field from /etc/password when you use it in a sentence but that doesn't necessary mean that You can. But gid exists as a concept beyond /etc/passwd. So does home directory. The GECOS field value does not, really. it's its name. man 5 passwd doesn't use word 'field' next to GECOS in fields description/list either. IMO our use case is the same. It says: GECOSThis field (sometimes called the comment field) [...] The gcos field in the password file was [...] Historically correct label would probably be 'GECOS identity' but that's not usable today as it's purpose is more general. Do we have tips in the UI? May be we should add them in future to provide extra information about meaning of the field or button. We do, but UX is not the best. 'doc' defined in .py files is displayed as a textbox tooltip. This feature is not apparent and for most of the fields the tooltip is the same as label. There is an idea to display a question mark icon next to textboxes to draw an attention to a presence of the doc text and provide it in a tooltip. I like the way how it's done in Alchemy UI: http://www.ui-alchemy.org/form You can notice there an additional information (string length limitation, example...) when field is focused. I went through open tickets and found few which touches the topic: - https://fedorahosted.org/freeipa/ticket/2912 [RFE] [Web UI] Use doc as field's tooltip instead of label * Seems to be already implemented. Then please close it and move it to the milestone where it was closed (if possible). - https://fedorahosted.org/freeipa/ticket/2413 [RFE]: add in UI examples of what is the requested field - https://fedorahosted.org/freeipa/ticket/3661 [RFE] IPA Web UI: When adding new reverse zone in DNS there could be an example of expected address format * Looks like duplicate of #2413 Right, please proceed with closing as duplicate then. I would prefer leaving only ticket 2413 open instead of changing description of 2912 though. Martin Resolution: * #3661, #2413 closed. * New ticket https://fedorahosted.org/freeipa/ticket/3812 for the UX enhancement. IMO we should close #2413 and maybe #2912 or change #2912 to cover the above mentioned proposals (in case of agreement). For now I think GECOS would probably be good enough. Adding field makes it more precise but looks weird. Sounds reasonable. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 436 Remove word 'field' from GECOS param label
On 07/23/2013 01:12 PM, Ana Krivokapic wrote: On 07/23/2013 12:58 PM, Petr Vobornik wrote: On 07/22/2013 05:33 PM, Ana Krivokapic wrote: On 07/22/2013 09:01 AM, Martin Kosek wrote: On 07/19/2013 11:19 PM, Dmitri Pal wrote: On 07/19/2013 09:26 AM, Jan Pazdziora wrote: On Fri, Jul 19, 2013 at 03:17:49PM +0200, Petr Vobornik wrote: snip For now I think GECOS would probably be good enough. Adding field makes it more precise but looks weird. +1 for just GECOS. Petr showed me both variants in the UI and GECOS field really looked weird. +1 for removing the word 'field'. The phrase 'GECOS field' also exists in the following files: install/ui/test/data/ipa_init_commands.json install/ui/test/data/ipa_init_objects.json install/ui/test/data/json_metadata.json So it should be fixed there as well. Fixed. ACK Pushed to master. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 430 Break long words in notification area
On 07/22/2013 05:19 PM, Ana Krivokapic wrote: On 07/18/2013 12:58 PM, Petr Vobornik wrote: Long words (ie. service principal) breaks out of notification area. It doesn't look good. Patch adds word-wrap to break them to multiple pieces. Reproduction: modify a service in Web UI ACK Pushed to master. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0109-0110 Support querying AD DC when establishing trust as HTTP/ipa.server principal
On Tue, 2013-07-23 at 16:11 +0300, Alexander Bokovoy wrote: On Tue, 23 Jul 2013, Simo Sorce wrote: On Thu, 2013-07-18 at 18:37 +0300, Alexander Bokovoy wrote: Hi! Attached patches make possible to use HTTP/ipa.server@REALM to query AD DC over LDAP immediately after trust is established. We need this to get range discovery working prior to creating range for trusted domain. The patch 0109 makes KDC hostname cached on ipadb context to avoid resolving own hostname multiple times. The patch 0110 depends on ulc_casemap patches by Nathaniel and makes exception for HTTP/ipa.server@REALM when TGT is requested and MS-PAC is asked for -- we force refreshing list of trusted domains here. More details are available in the commit logs. I do not think that changing reinit interval is the right thing to do. I would rather pass a boolean that tells reinit to check if we have any trust info, and if not unconditionally try to reinit immediately. I see that you treat the interval sort of like a boolean but then you just race hoping the previous reload w/o trust info happened more than 1 second earlier. I think and explicit bool force_reload flag would be much clearer. Otherwise ack. Attached is modified patch that uses 'bool force_reinit' (as function is called ipadb_reinit_mspac). I tested it together with updated Tomas patch 0076 which relies on these patches so I'm going to commit whole set together. LGTM, please proceed. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 143-147 Improve performance with large groups
On 07/23/2013 01:30 PM, Martin Kosek wrote:
On 07/19/2013 01:10 PM, Petr Vobornik wrote:
On 07/18/2013 05:29 PM, Jan Cholasta wrote:
On 18.7.2013 17:26, Martin Kosek wrote:
On 07/18/2013 05:22 PM, Jan Cholasta wrote:
On 18.7.2013 17:07, Martin Kosek wrote:
On 07/18/2013 04:53 PM, Jan Cholasta wrote:
Added patch which adds new hidden option no_members to suppress
membership
processing for commands of all objects that have member attributes.
This can be
used by the WebUI to prevent member lookups where they are not
necessary (as we
discussed off-line with Martin and Petr).
Honza
1) Should the new option really have exclude='webui'? I thought
that Web UI is
the main and only consumer of this option.
The 'webui' context doesn't actually exist and the only meaning of
this stanza
is that the option is not shown in the output of the show_mappings
command.
2) I would clearly state this is an internal option only, e.g.
+ doc=_('INTERNAL: suppress processing of membership attributes.'),
No other internal option has this kind of thing in its doc and nobody
will see
it anyway, so we might just leave it like that IMHO.
OK.
3) It would be nice to state that this option is mutually exclusive
with --all,
but given it is internal anyway and there is no central place to
define it, we
do not need to do that.
The options are not really mutually exclusive at this point, they can be
specified together, --all takes precedence.
Well, they can be specified together, but the option is then NOOP
which could
confuse users which may have different expectations. Being explicit
helps.
I agree.
But
as I said, in this case this is not a requirement.
I agree as well :-)
Honza
Functional ACK for Honza's patch (didn't break Web UI test suite)
Attaching Web UI patch.
It:
1) Removed --all from _find and _show commands used by search pages. All
displayed attributes should be already included in default attributes.
2) Removed search_all_attributes - Not needed since introduction of paging.
3) Added --no-members options to search _show commmands.
ACK. Pushed both Petr's and Honza's patch to master and ipa-3-2.
Martin
These patches sometimes add the attribute no_members to groups, which
results in 7 tests being broken like this:
==
FAIL: test_cli.TestCLIParsing.test_group_add
--
Traceback (most recent call last):
File /usr/lib/python2.7/site-packages/nose/case.py, line 197, in
runTest
self.test(*self.arg)
File
/var/lib/jenkins/workspace/freeipa-unittests-f19-devel/ipatests/test_cmdline/test_cli.py,
line 73, in test_group_add
version=API_VERSION)
File
/var/lib/jenkins/workspace/freeipa-unittests-f19-devel/ipatests/test_cmdline/test_cli.py,
line 24, in check_command
util.assert_deepequal(kw_expected, kw_got)
File
/var/lib/jenkins/workspace/freeipa-unittests-f19-devel/ipatests/util.py,
line 338, in assert_deepequal
doc, sorted(missing), sorted(extra), expected, got, stack
AssertionError: assert_deepequal: dict keys mismatch.
missing keys = []
extra keys = ['no_members']
expected = {'all': False, 'cn': u'tgroup1', 'raw': False, 'version':
u'2.62', 'external': False, 'nonposix': False, 'description': u'Test group'}
got = {'all': False, 'description': u'Test group', 'no_members':
False, 'raw': False, 'version': u'2.62', 'external': False, 'nonposix':
False, 'cn': u'tgroup1'}
path = ()
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] slapi-nis support for trusted domains
On Tue, Jul 23, 2013 at 10:15:47AM +0300, Alexander Bokovoy wrote:
On Tue, 23 Jul 2013, Nalin Dahyabhai wrote:
Apologies for the delay.
Thanks for the review!
One short comment -- PAM code is from PAM pass-through plugin from
389-ds. That's the reason why its code doesn't follow slapi-nis way and
why it has that license. I tried to keep it mostly intact to share
changes but looking at git log it gets roughly two commits per year so
maybe it is better to rework it completely.
That'd be my preference. Other than knowing how to map specific
PAM error codes to LDAP-level errors, there doesn't seem to be a lot of
magic that needs to be preserved in there.
I'll address other comments and will send updated version for the
review today. This was my first sizable SLAPI code so errors are
inevitable.
No worries. I think there's already a lot in there that's right.
I've been thinking about the patch some more, and I need to revise a
couple of my comments.
[snip]
+ slapi_entry_add_string(entry,
+ uid, user_name);
If is_uid is true, this is a numeric string. Intentional?
Given that group entries are being constructed using group member
information, which is always login names, I guess it isn't.
[snip]
+ for (i=0; grp.gr_mem[i]; i++) {
+ slapi_entry_add_string(entry, memberUid,
+ slapi_ch_smprintf(uid=%s,%s, grp.gr_mem[i],
sdn));
+ }
The memberUid attribute doesn't typically contain DNs. Did you mean
to use member here? Or to just use the user login name for the value?
This probably wants to set memberUid to the grp.gr_mem element's
value, because if we construct a member DN and expect the plugin's
configured logic to dereference it and pull out the UID value, and the
plugin attempts to read the entry with that DN by doing a search with
scope=base to find the entry, I don't think it'll trigger the logic that
would create that entry in the cache.
That, and in compat trees we're generally in the business of unrolling
group memberships anyway.
Cheers,
Nalin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 143-147 Improve performance with large groups
On 07/23/2013 04:54 PM, Petr Viktorin wrote:
On 07/23/2013 01:30 PM, Martin Kosek wrote:
On 07/19/2013 01:10 PM, Petr Vobornik wrote:
On 07/18/2013 05:29 PM, Jan Cholasta wrote:
On 18.7.2013 17:26, Martin Kosek wrote:
On 07/18/2013 05:22 PM, Jan Cholasta wrote:
On 18.7.2013 17:07, Martin Kosek wrote:
On 07/18/2013 04:53 PM, Jan Cholasta wrote:
Added patch which adds new hidden option no_members to suppress
membership
processing for commands of all objects that have member attributes.
This can be
used by the WebUI to prevent member lookups where they are not
necessary (as we
discussed off-line with Martin and Petr).
Honza
1) Should the new option really have exclude='webui'? I thought
that Web UI is
the main and only consumer of this option.
The 'webui' context doesn't actually exist and the only meaning of
this stanza
is that the option is not shown in the output of the show_mappings
command.
2) I would clearly state this is an internal option only, e.g.
+ doc=_('INTERNAL: suppress processing of membership attributes.'),
No other internal option has this kind of thing in its doc and nobody
will see
it anyway, so we might just leave it like that IMHO.
OK.
3) It would be nice to state that this option is mutually exclusive
with --all,
but given it is internal anyway and there is no central place to
define it, we
do not need to do that.
The options are not really mutually exclusive at this point, they
can be
specified together, --all takes precedence.
Well, they can be specified together, but the option is then NOOP
which could
confuse users which may have different expectations. Being explicit
helps.
I agree.
But
as I said, in this case this is not a requirement.
I agree as well :-)
Honza
Functional ACK for Honza's patch (didn't break Web UI test suite)
Attaching Web UI patch.
It:
1) Removed --all from _find and _show commands used by search pages. All
displayed attributes should be already included in default attributes.
2) Removed search_all_attributes - Not needed since introduction of
paging.
3) Added --no-members options to search _show commmands.
ACK. Pushed both Petr's and Honza's patch to master and ipa-3-2.
Martin
These patches sometimes add the attribute no_members to groups, which
results in 7 tests being broken like this:
==
FAIL: test_cli.TestCLIParsing.test_group_add
--
Traceback (most recent call last):
File /usr/lib/python2.7/site-packages/nose/case.py, line 197, in
runTest
self.test(*self.arg)
File
/var/lib/jenkins/workspace/freeipa-unittests-f19-devel/ipatests/test_cmdline/test_cli.py,
line 73, in test_group_add
version=API_VERSION)
File
/var/lib/jenkins/workspace/freeipa-unittests-f19-devel/ipatests/test_cmdline/test_cli.py,
line 24, in check_command
util.assert_deepequal(kw_expected, kw_got)
File
/var/lib/jenkins/workspace/freeipa-unittests-f19-devel/ipatests/util.py,
line 338, in assert_deepequal
doc, sorted(missing), sorted(extra), expected, got, stack
AssertionError: assert_deepequal: dict keys mismatch.
missing keys = []
extra keys = ['no_members']
expected = {'all': False, 'cn': u'tgroup1', 'raw': False, 'version':
u'2.62', 'external': False, 'nonposix': False, 'description': u'Test
group'}
got = {'all': False, 'description': u'Test group', 'no_members':
False, 'raw': False, 'version': u'2.62', 'external': False, 'nonposix':
False, 'cn': u'tgroup1'}
path = ()
Correction: they don't add the attribute to output. It's just that the
CLI tests need to be updated with the new option.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 431-434 Web UI integration tests continuation
On 07/18/2013 01:35 PM, Petr Vobornik wrote:
On 07/18/2013 01:34 PM, Petr Vobornik wrote:
[PATCH] 431 Web UI integration tests: Add trust tests
[PATCH] 432 Web UI integration tests: Add ui_driver method descriptions
[PATCH] 433 Web UI integration tests: Verify data after add and mod
[PATCH] 434 Web UI integration tests: Compute range sizes to avoid overlaps
Heavily inspired by code from xmlrpc tests.
To obtain ranges, this patch also adds method to execute FreeIPA command
through Web UI.
It uses Web UI instead of ipalib so it doesn't need to care about
authentication on a test-runner machine.
All: https://fedorahosted.org/freeipa/ticket/3744
And now the patches...
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Patch 431:
- In case 'ipa-adtrust-install' has not been run on the system, but 'has_trusts'
is configured in 'ui_test.conf', trust tests fail. I suggest checking for
'ipa-adtrust-install' with 'adtrust_is_enabled' command and then skipping tests
if trusts are not enabled.
Patch 432:
- Docstrings for 'has_ca()' and 'has_dns()' state that 'FreeIPA server was
installed *without* CA/DNS' when they shoud state *with*.
Patch 433:
- Please also add docstrings for newly added methods.
- The long 'if' statement in 'validate_fields()' can be shortened by using the
'in' keyword instead of individual string comparisons. For example:
elif ftype in ('textbox', 'password', 'combobox'):
actual = self.get_field_value(key, parent, 'input')
- IIUC, the code in validate_fields() compares lists irrespective of element
order. If this is the case, it can be replaced by simply 'sorted(expected) ==
sorted(actual)'.
- In 'basic_crud()', shouldn't validate_fields with 'add_v' be called right
after 'add_record'? The way it is now, data only gets validated in case of data
modification, but not after initial addition.
Patch 434:
- The if 'ipasecondarybaserid' in idrange block should be nested under the if
'ipasecondarybaserid' in idrange block, since it assumes that the 'base_rid'
variable is set.
- Please remove trailing semicolons.
General comments (these comments are by no means a requirement, and they are not
a reason for a NACK, just a nice to have):
- Some methods have unused parameters (e.g. validate_fields, basic_crud)
- Some methods define variables which shadow Python built-ins of the same name
(e.g. type, all). This can be a problem if you later want to use the built-in.
- It would be nice to make at least newly added code PEP8 compliant. The 'pep8'
utility can be used to easily check any python file for PEP8 compliance:
$ sudo yum install python-pep8
$ pep8 /path/to/script.py
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 431-434 Web UI integration tests continuation
On 07/23/2013 05:50 PM, Ana Krivokapic wrote:
On 07/18/2013 01:35 PM, Petr Vobornik wrote:
On 07/18/2013 01:34 PM, Petr Vobornik wrote:
[PATCH] 431 Web UI integration tests: Add trust tests
[PATCH] 432 Web UI integration tests: Add ui_driver method descriptions
[PATCH] 433 Web UI integration tests: Verify data after add and mod
[PATCH] 434 Web UI integration tests: Compute range sizes to avoid
overlaps
Heavily inspired by code from xmlrpc tests.
To obtain ranges, this patch also adds method to execute FreeIPA command
through Web UI.
It uses Web UI instead of ipalib so it doesn't need to care about
authentication on a test-runner machine.
All: https://fedorahosted.org/freeipa/ticket/3744
And now the patches...
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Patch 431:
- In case 'ipa-adtrust-install' has not been run on the system, but
'has_trusts' is configured in 'ui_test.conf', trust tests fail. I
suggest checking for 'ipa-adtrust-install' with 'adtrust_is_enabled'
command and then skipping tests if trusts are not enabled.
Patch 432:
- Docstrings for 'has_ca()' and 'has_dns()' state that 'FreeIPA server
was installed *without* CA/DNS' when they shoud state *with*.
Patch 433:
- Please also add docstrings for newly added methods.
- The long 'if' statement in 'validate_fields()' can be shortened by
using the 'in' keyword instead of individual string comparisons. For
example:
elif ftype in ('textbox', 'password', 'combobox'):
actual = self.get_field_value(key, parent, 'input')
- IIUC, the code in validate_fields() compares lists irrespective of
element order. If this is the case, it can be replaced by simply
'sorted(expected) == sorted(actual)'.
- In 'basic_crud()', shouldn't validate_fields with 'add_v' be called
right after 'add_record'? The way it is now, data only gets validated in
case of data modification, but not after initial addition.
Patch 434:
- The if 'ipasecondarybaserid' in idrange block should be nested under
the if 'ipasecondarybaserid' in idrange block, since it assumes that
the 'base_rid' variable is set.
- Please remove trailing semicolons.
General comments (these comments are by no means a requirement, and they
are not a reason for a NACK, just a nice to have):
- Some methods have unused parameters (e.g. validate_fields, basic_crud)
- Some methods define variables which shadow Python built-ins of the
same name (e.g. type, all). This can be a problem if you later want to
use the built-in.
- It would be nice to make at least newly added code PEP8 compliant. The
'pep8' utility can be used to easily check any python file for PEP8
compliance:
$ sudo yum install python-pep8
$ pep8 /path/to/script.py
It can also check only your changes:
git diff -U0 origin/master.. | pep8 --diff
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0047 Honor 'enabled' option for widgets
On 07/22/2013 04:46 PM, Ana Krivokapic wrote: On 07/18/2013 09:47 AM, Petr Vobornik wrote: On 07/17/2013 09:18 PM, Ana Krivokapic wrote: Hello, This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3793. Hello, 1) IMO we should not create attribute which is just a negation of another. 2) We should add set_enabled method to base widget. Existing set_enabled methods should use it and maintain widget output consistent with the attribute (ie. one should not directly set the attr and should use set_enabled instead). The method should be also callable when content is not yet created. get_enabled methods might become unnecessary - one can get the state form 'enabled' attribute. The attached updated patch implements the following changes: 1) set_enabled method has been added to the base widget class. 2) get_enabled/is_enabled methods have been removed. 3) Widget classes that inherit from the base widget class override the set_enabled method where necessary. 4) Using 'enabled: true/false' in the widget definition should now work correctly for all types of widgets. Thanks. 1. set_enabled method in input_widget uses `that.input`. Input widget is a base class which doesn't set the property and therefore we can't be certain that the descendant will set it. Also it may break when you call set_enabled(val) before create() . We should test for `that.input` presence. Same content-created test should be perform on other places: widget.js:1017,1147,2006 2. The changes in option_widget_base break disabling if user doesn't have write-rights. (can be reproduced when navigated (by manual change of url) to service in self-service. Note the differences between read_only, writable and enabled: * read_only - reflects metadata * writable - reflects ACL * enabled - context specific read_only and writable don't offer edit interface (label instead of textbox). Enabled controls disabled state of textbox. For some widgets the result might be the same (radios, checkboxes). option_widget_base.set_enabled should be changed. The mixin overwrites the original method and therefore doesn't set 'enabled' property. 3. multiple_choice_section.set_enabled should be renamed. It's related to individual choices and not the widget itself. And then new set_enabled should be added which would call the old one for each choice. 4. widget.js:3870 - not sure if it's needed but if so, one should also change `action_clicked` method. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 161 Use configured dogtag LDAP port instead of default one when renewing certs
On 22.7.2013 17:40, Simo Sorce wrote:
On Mon, 2013-07-22 at 17:36 +0200, Jan Cholasta wrote:
if nickname == 'subsystemCert cert-pki-ca':
-update_people_entry('pkidbuser', cert)
+update_people_entry(dogtag_uri, 'pkidbuser', cert)
This is probably wrong, there is no pkidbuser in old instances.
My subsystemCert has a subject of CN=CA Subsystem,O=REALM and this
cert is associated to an object named:
uid=CA-sevrver-name-9443,ou=people,o=ipaca
I think you need to search the db to find the right object(s) to update.
Right. Updated patch attached.
Honza
--
Jan Cholasta
From 46feab87d4fecc9f2fad283e8e5e1d360115dca2 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Tue, 23 Jul 2013 10:19:42 +
Subject: [PATCH] Fix certificate renewal scripts to work with separate CA DS
instance.
https://fedorahosted.org/freeipa/ticket/3805
---
install/restart_scripts/renew_ca_cert | 4 +--
install/restart_scripts/renew_ra_cert | 2 +-
ipaserver/install/cainstance.py | 60 ---
3 files changed, 44 insertions(+), 22 deletions(-)
diff --git a/install/restart_scripts/renew_ca_cert b/install/restart_scripts/renew_ca_cert
index 5768db3..94bf803 100644
--- a/install/restart_scripts/renew_ca_cert
+++ b/install/restart_scripts/renew_ca_cert
@@ -84,9 +84,7 @@ finally:
shutil.rmtree(tmpdir)
update_cert_config(nickname, cert)
-
-if nickname == 'subsystemCert cert-pki-ca':
-update_people_entry('pkidbuser', cert)
+update_people_entry(cert)
if nickname == 'auditSigningCert cert-pki-ca':
# Fix trust on the audit cert
diff --git a/install/restart_scripts/renew_ra_cert b/install/restart_scripts/renew_ra_cert
index e541e4b..596ca2b 100644
--- a/install/restart_scripts/renew_ra_cert
+++ b/install/restart_scripts/renew_ra_cert
@@ -41,7 +41,7 @@ db = certs.CertDB(api.env.realm)
dercert = db.get_cert_from_db('ipaCert', pem=False)
# Load it into dogtag
-update_people_entry('ipara', dercert)
+update_people_entry(dercert)
attempts = 0
updated = False
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index ca3ee69..7b79e17 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -40,6 +40,7 @@ import ConfigParser
from ipapython import dogtag
from ipapython.certdb import get_ca_nickname
from ipapython import certmonger
+from ipalib import api
from ipalib import pkcs10, x509
from ipalib import errors
from ipapython.dn import DN
@@ -1707,58 +1708,81 @@ def update_cert_config(nickname, cert):
base64.b64encode(cert),
quotes=False, separator='=')
-def update_people_entry(uid, dercert):
+def update_people_entry(dercert):
Update the userCerticate for an entry in the dogtag ou=People. This
is needed when a certificate is renewed.
-uid: uid of user to update
dercert: An X509.3 certificate in DER format
Logging is done via syslog
Returns True or False
-dn = DN(('uid',uid),('ou','People'),('o','ipaca'))
+base_dn = DN(('ou','People'), ('o','ipaca'))
serial_number = x509.get_serial_number(dercert, datatype=x509.DER)
subject = x509.get_subject(dercert, datatype=x509.DER)
issuer = x509.get_issuer(dercert, datatype=x509.DER)
attempts = 0
-dogtag_uri='ldap://localhost:%d' % DEFAULT_DSPORT
+configured_constants = dogtag.configured_constants(api)
+dogtag_uri = 'ldap://localhost:%d' % configured_constants.DS_PORT
updated = False
try:
dm_password = certmonger.get_pin('internaldb')
except IOError, e:
-syslog.syslog(syslog.LOG_ERR, 'Unable to determine PIN for CA instance: %s' % e)
+syslog.syslog(
+syslog.LOG_ERR, 'Unable to determine PIN for CA instance: %s' % e)
return False
while attempts 10:
conn = None
try:
conn = ldap2.ldap2(shared_instance=False, ldap_uri=dogtag_uri)
-conn.connect(bind_dn=DN(('cn', 'directory manager')),
-bind_pw=dm_password)
-(entry_dn, entry_attrs) = conn.get_entry(dn, ['usercertificate'])
-entry_attrs['usercertificate'].append(dercert)
-entry_attrs['description'] = '2;%d;%s;%s' % (serial_number, issuer,
-subject)
-conn.update_entry(dn, entry_attrs)
+conn.connect(
+bind_dn=DN(('cn', 'directory manager')), bind_pw=dm_password)
+
+filter = conn.make_filter(
+{'description': ';%s;%s' % (issuer, subject)},
+exact=False, trailing_wildcard=False)
+try:
+entries = conn.get_entries(base_dn, conn.SCOPE_SUBTREE, filter)
+except errors.NotFound:
+entries = []
+
updated = True
+
+for entry in entries:
+syslog.syslog(
+syslog.LOG_NOTICE, 'Updating entry %s' %
