[Freeipa-devel] [PATCH] Allow ipa-getkeytab to find server name from config file

[Simo Sorce]

Fixes #2203 by reading the server name from /etc/ipa/default.conf if not
provided on the command line.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
From 8dd8176147c46b2af559c61dec469dfff5b82059 Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Mon, 23 Nov 2015 14:50:04 -0500
Subject: [PATCH] Support sourcing the IPA server name from config

Use ding-libs to parse /etc/ipa/default.conf to find the IPA server
to contact by default.

Signed-off-by: Simo Sorce 

Ticket: https://fedorahosted.org/freeipa/ticket/2203
---
 ipa-client/Makefile.am |  4 ++
 ipa-client/configure.ac| 28 ++
 ipa-client/ipa-getkeytab.c | 93 +-
 3 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/ipa-client/Makefile.am b/ipa-client/Makefile.am
index 0da351c6a28a4e2a216bec3f85d0500e4aef47ff..6c426779530f216b11dff0764132221ea2793289 100644
--- a/ipa-client/Makefile.am
+++ b/ipa-client/Makefile.am
@@ -15,6 +15,7 @@ export AM_CFLAGS
 KRB5_UTIL_DIR=../util
 KRB5_UTIL_SRCS=$(KRB5_UTIL_DIR)/ipa_krb5.c
 ASN1_UTIL_DIR=../asn1
+IPA_CONF_FILE=$(sysconfdir)/ipa/default.conf
 
 AM_CPPFLAGS =			\
 	-I.			\
@@ -27,11 +28,13 @@ AM_CPPFLAGS =			\
 	-DLIBEXECDIR=\""$(libexecdir)"\"			\
 	-DDATADIR=\""$(datadir)"\"\
 	-DLOCALEDIR=\""$(localedir)"\"\
+	-DIPACONFFILE=\""$(IPA_CONF_FILE)"\"			\
 	$(KRB5_CFLAGS)		\
 	$(OPENLDAP_CFLAGS)	\
 	$(SASL_CFLAGS)		\
 	$(POPT_CFLAGS)		\
 	$(WARN_CFLAGS)		\
+	$(INI_CFLAGS)		\
 	$(NULL)
 
 sbin_PROGRAMS =			\
@@ -53,6 +56,7 @@ ipa_getkeytab_LDADD = 		\
 	$(SASL_LIBS)		\
 	$(POPT_LIBS)		\
 	$(LIBINTL_LIBS) \
+	$(INI_LIBS)		\
 	$(NULL)
 
 ipa_rmkeytab_SOURCES =		\
diff --git a/ipa-client/configure.ac b/ipa-client/configure.ac
index 78da8e6e413b8becbd4c75422abffb670050f446..943c3f1b62f6a4947335178e9bcf1d45434a7e90 100644
--- a/ipa-client/configure.ac
+++ b/ipa-client/configure.ac
@@ -192,6 +192,34 @@ LIBS="$SAVELIBS"
 AC_SUBST(LIBINTL_LIBS)
 
 dnl ---
+dnl - Check for libini_config
+dnl ---
+PKG_CHECK_MODULES([LIBINI_CONFIG], [ini_config >= 1.2.0], [have_libini_config=1], [have_libini_config=])
+if test x$have_libini_config = x; then
+AC_MSG_WARN([Could not find LIBINI_CONFIG headers])
+else
+INI_CONFIG_CFLAGS="`$PKG_CONFIG --cflags ini_config`"
+INI_CONFIG_LIBS="`$PKG_CONFIG --libs ini_config`"
+AC_CHECK_LIB(ini_config, ini_config_file_open, [],
+ [AC_MSG_WARN([ini_config library must support ini_config_file_open])],
+ [$INI_CONFIG_LIBS])
+AC_CHECK_LIB(ini_config, ini_config_augment, [],
+ [AC_MSG_WARN([ini_config library must support ini_config_augment])],
+ [$INI_CONFIG_LIBS])
+fi
+
+if test x$have_libini_config = x1; then
+INI_CFLAGS="$INI_CONFIG_CFLAGS"
+INI_LIBS="$INI_CONFIG_LIBS"
+else
+AC_MSG_ERROR([ini_config development packages not available])
+fi
+
+AC_SUBST(INI_LIBS)
+AC_SUBST(INI_CFLAGS)
+
+
+dnl ---
 dnl - Set the data install directory since we don't use pkgdatadir
 dnl ---
 
diff --git a/ipa-client/ipa-getkeytab.c b/ipa-client/ipa-getkeytab.c
index 15255d6a33c8c298f138868ac545d4ebea415fe5..9670d980fd1c2f00e28cf177425e9affee8c4097 100644
--- a/ipa-client/ipa-getkeytab.c
+++ b/ipa-client/ipa-getkeytab.c
@@ -36,6 +36,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "config.h"
 
@@ -596,6 +597,81 @@ static char *ask_password(krb5_context krbctx)
 return password;
 }
 
+struct ipa_config {
+const char *server_name;
+};
+
+static int config_from_file(struct ini_cfgobj *cfgctx)
+{
+struct ini_cfgfile *fctx = NULL;
+char **errors = NULL;
+int ret;
+
+ret = ini_config_file_open(IPACONFFILE, 0, );
+if (ret) {
+fprintf(stderr, _("Failed to open config file %s\n"), IPACONFFILE);
+return ret;
+}
+
+ret = ini_config_parse(fctx,
+   INI_STOP_ON_ANY,
+   INI_MS_MERGE | INI_MV1S_ALLOW | INI_MV2S_ALLOW,
+   INI_PARSE_NOWRAP,
+   cfgctx);
+if (ret) {
+fprintf(stderr, _("Failed to parse config file %s\n"), IPACONFFILE);
+if (ini_config_error_count(cfgctx)) {
+ini_config_get_errors(cfgctx, );
+if (errors) {
+ini_config_print_errors(stderr, errors);
+ini_config_free_errors(errors);
+}
+}
+ini_config_file_destroy(fctx);
+return ret;
+}
+
+ini_config_file_destroy(fctx);
+return 0;
+}
+
+int read_ipa_config(struct ipa_config **ipacfg)
+{
+struct ini_cfgobj *cfgctx = NULL;
+struct value_obj *obj = NULL;
+

Re: [Freeipa-devel] [PATCH 0354] Remove forgotten print in DNS plugin

[Petr Spacek]

On 20.11.2015 14:37, Martin Basti wrote:
> patch attached.

Obvous ACK.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 0042-0043 Avoid race condition in profile creation

[Jan Cholasta]

On 23.11.2015 06:46, Fraser Tweedale wrote:

The attached patch 0043 fixes #5269[1]: nondeterministic failure of
certificate profile creation during ipa-server-install.

[1] https://fedorahosted.org/freeipa/ticket/5269

The other patch 0042 is drive-by improvements of IPA install/upgrade
logging that I did while diagnosing the issue.

Thanks,
Fraser



Patch 0042: Does not apply on master.

Patch 0043: ACK

Honza

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0067, 0068] ipa-{cacert-renew, otptoken-import}: Fix connection to ldap.

[Jan Cholasta]

On 23.11.2015 08:53, David Kupka wrote:

On 20/11/15 08:29, Jan Cholasta wrote:

On 19.11.2015 17:28, David Kupka wrote:

https://fedorahosted.org/freeipa/ticket/5468


ipa-cacert-manage is not the only code which uses ldap2 this way.

It would be better to find the root cause of this rather than working
around it.



The root cause is that some scripts are creating custom connection to
LDAP and using api which is not connected to LDAP.
As we discussed personally ipa-cacert-manage and ipa-otptoken-import
have this issue.
Updated patch and new one for ipa-otptoken-import attached.


The patches do not apply on ipa-4-2.

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0064] Check if IPA is configured before attempting a winsync migration

[Martin Babinsky]

On 11/20/2015 07:10 PM, Gabe Alford wrote:

Thanks. Updated patch attached.


Gabe

On Fri, Nov 20, 2015 at 10:36 AM, Martin Babinsky > wrote:

On 11/20/2015 04:02 PM, Gabe Alford wrote:

Hello,

Fix for https://fedorahosted.org/freeipa/ticket/5470

Thanks,

Gabe


Hi Gabe,

patch looks good. IMHO it would be better if you moved the check
before API initialization like so:

"""
@@ -340,6 +340,12 @@ class WinsyncMigrate(admintool.AdminTool):
  the plumbing.
  """

+# Check if the IPA server is configured before attempting
to migrate
+try:
+installutils.check_server_configuration()
+except RuntimeError as e:
+sys.exit(e)
+
  # Finalize API
  api.bootstrap(in_server=True, context='server')
  api.finalize()
"""

There's no point in initializing API if there is no server installed.

--
Martin^3 Babinsky



Thanks, ACK.

--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 0042-0043 Avoid race condition in profile creation

[Jan Cholasta]

On 23.11.2015 10:04, Jan Cholasta wrote:

On 23.11.2015 06:46, Fraser Tweedale wrote:

The attached patch 0043 fixes #5269[1]: nondeterministic failure of
certificate profile creation during ipa-server-install.

[1] https://fedorahosted.org/freeipa/ticket/5269

The other patch 0042 is drive-by improvements of IPA install/upgrade
logging that I did while diagnosing the issue.

Thanks,
Fraser



Patch 0042: Does not apply on master.

Patch 0043: ACK


I have rebased the patches on top of master, see attachment.

ACK and pushed to:
master: 5136cd6e4bd305d6f4b6bf22d22fb4abc365cfad
ipa-4-2: a8a666416201a7a7d6739f60854c5e5223b9ceb5

--
Jan Cholasta
From 9e0e735fa6bb042fbc5acd5b02d4d5da716c737d Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Fri, 20 Nov 2015 15:39:00 +1100
Subject: [PATCH 1/2] TLS and Dogtag HTTPS request logging improvements

Pretty printing the TLS peer certificate to logs on every request
introduces a lot of noise; do not log it (subject name, key usage
and validity are still logged).

Fix and tidy up some HTTP logging messages for Dogtag requests.

Part of: https://fedorahosted.org/freeipa/ticket/5269
---
 ipapython/dogtag.py | 9 -
 ipapython/nsslib.py | 3 ---
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/ipapython/dogtag.py b/ipapython/dogtag.py
index 51c2ec9..71de96d 100644
--- a/ipapython/dogtag.py
+++ b/ipapython/dogtag.py
@@ -324,7 +324,7 @@ def _httplib_request(
 if isinstance(host, unicode):
 host = host.encode('utf-8')
 uri = '%s://%s%s' % (protocol, ipautil.format_netloc(host, port), path)
-root_logger.debug('request %r', uri)
+root_logger.debug('request %s %s', method, uri)
 root_logger.debug('request body %r', request_body)
 
 headers = headers or {}
@@ -347,9 +347,8 @@ def _httplib_request(
 except Exception as e:
 raise NetworkError(uri=uri, error=str(e))
 
-root_logger.debug('request status %d',http_status)
-root_logger.debug('request reason_phrase %r', http_reason_phrase)
-root_logger.debug('request headers %s',   http_headers)
-root_logger.debug('request body %r',  http_body)
+root_logger.debug('response status %d %s', http_status, http_reason_phrase)
+root_logger.debug('response headers %s',   http_headers)
+root_logger.debug('response body %r',  http_body)
 
 return http_status, http_reason_phrase, http_headers, http_body
diff --git a/ipapython/nsslib.py b/ipapython/nsslib.py
index 5ae79b6..06e5329 100644
--- a/ipapython/nsslib.py
+++ b/ipapython/nsslib.py
@@ -48,9 +48,6 @@ def auth_certificate_callback(sock, check_sig, is_server, certdb):
 
 cert = sock.get_peer_certificate()
 
-root_logger.debug("auth_certificate_callback: check_sig=%s is_server=%s\n%s",
-  check_sig, is_server, str(cert))
-
 pin_args = sock.get_pkcs11_pin_arg()
 if pin_args is None:
 pin_args = ()
-- 
2.4.3

From 8103ab5efe32859678430145407cdb19dc77f542 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Fri, 20 Nov 2015 15:59:11 +1100
Subject: [PATCH 2/2] Avoid race condition caused by profile delete and
 recreate

When importing IPA-managed certificate profiles into Dogtag,
profiles with the same name (usually caIPAserviceCert) are removed,
then immediately recreated with the new profile data.  This causes a
race condition - Dogtag's LDAPProfileSystem profileChangeMonitor
thread could observe and process the deletion after the profile was
recreated, disappearing it again.

Update the profile instead of deleting and recreating it to avoid
this race condition.

Fixes: https://fedorahosted.org/freeipa/ticket/5269
---
 ipaserver/install/cainstance.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 1cbc0d0..448e42e 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -1970,8 +1970,7 @@ def _create_dogtag_profile(profile_id, profile_data):
 root_logger.debug(
 "Failed to disable profile '%s' "
 "(it is probably already disabled)")
-profile_api.delete_profile(profile_id)
-profile_api.create_profile(profile_data)
+profile_api.update_profile(profile_id, profile_data)
 
 # enable the profile
 try:
-- 
2.4.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Tomas Babej]

Hi,

If the code within the private_ccache contextmanager does not
set/removes the KRB5CCNAME, the pop method will raise KeyError, which
will cause unnecessary termination of the code flow.

Make sure the KRB5CCNAME is popped out of os.environ only if present.

Tomas
From 201bc398ec59920f7cd34de69de66cb3489f417d Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Mon, 23 Nov 2015 12:47:56 +0100
Subject: [PATCH] private_ccache: Harden the removal of KRB5CCNAME env variable

If the code within the private_ccache contextmanager does not
set/removes the KRB5CCNAME, the pop method will raise KeyError, which
will cause unnecessary termination of the code flow.

Make sure the KRB5CCNAME is popped out of os.environ only if present.
---
 ipapython/ipautil.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4acdd1a98818bf311a8fef103e7219cc62a28ec1..ec85786e18928838b51573c35e17986ff38f72d9 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1367,7 +1367,9 @@ def private_ccache(path=None):
 if original_value is not None:
 os.environ['KRB5CCNAME'] = original_value
 else:
-os.environ.pop('KRB5CCNAME')
+# No value was set originally, make sure no value is set now
+if 'KRB5CCNAME' in os.environ:
+os.environ.pop('KRB5CCNAME')
 
 if os.path.exists(path):
 os.remove(path)
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Jan Cholasta]

On 23.11.2015 12:53, Tomas Babej wrote:

Hi,

If the code within the private_ccache contextmanager does not
set/removes the KRB5CCNAME, the pop method will raise KeyError, which
will cause unnecessary termination of the code flow.

Make sure the KRB5CCNAME is popped out of os.environ only if present.

Tomas


NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.

Also I don't think the comment is necessary, it's quite obvious what the 
code does.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Tomas Babej]

On 11/23/2015 01:11 PM, Jan Cholasta wrote:
> On 23.11.2015 12:53, Tomas Babej wrote:
>> Hi,
>>
>> If the code within the private_ccache contextmanager does not
>> set/removes the KRB5CCNAME, the pop method will raise KeyError, which
>> will cause unnecessary termination of the code flow.
>>
>> Make sure the KRB5CCNAME is popped out of os.environ only if present.
>>
>> Tomas
> 
> NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.

Yeah, well, both ways are equivalent, I found the if statement more
explicit. We can go with the suggested version, if it's more pleasing
though - patch is attached.

> 
> Also I don't think the comment is necessary, it's quite obvious what the
> code does.
> 

I don't think an explanatory comment can hurt, ever. Worst thing that
happens is that somebody is assured that he understands the code correctly.

Tomas
From 65d4efa341ff73ef6a3a2da9d443577305aa82dd Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Mon, 23 Nov 2015 12:47:56 +0100
Subject: [PATCH] private_ccache: Harden the removal of KRB5CCNAME env variable

If the code within the private_ccache contextmanager does not
set/removes the KRB5CCNAME, the pop method will raise KeyError, which
will cause unnecessary termination of the code flow.

Make sure the KRB5CCNAME is popped out of os.environ only if present.
---
 ipapython/ipautil.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4acdd1a98818bf311a8fef103e7219cc62a28ec1..b4af0868c15178f75007b4f1e407070ae102ceb4 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1367,7 +1367,8 @@ def private_ccache(path=None):
 if original_value is not None:
 os.environ['KRB5CCNAME'] = original_value
 else:
-os.environ.pop('KRB5CCNAME')
+# No value was set originally, make sure no value is set now
+os.environ.pop('KRB5CCNAME', None)
 
 if os.path.exists(path):
 os.remove(path)
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0354] Remove forgotten print in DNS plugin

[Martin Basti]

On 23.11.2015 09:10, Petr Spacek wrote:

On 20.11.2015 14:37, Martin Basti wrote:

patch attached.

Obvous ACK.


Pushed to master: bf654aee1ca339b7807e80bd6a8dd42b008553c9

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0102] update idrange tests to reflect disabled modification of local ID ranges

[Martin Basti]

On 20.11.2015 18:41, Milan Kubík wrote:

On 11/20/2015 04:06 PM, Martin Babinsky wrote:
When I fixed https://fedorahosted.org/freeipa/ticket/4826 I forgot to 
fix the corresponding xmlrpc tests.


This oversight bit me today when I ran in-tree tests on my VM.

Here is the patch that makes idrange tests green and shiny again.


Tests are now passing, ACK


Pushed to:
master: 8909506a880719478dc1c346ee325566620a9d18
ipa-4-2: 92e00d11acbabcfbafca311b1dd286054ee8d7c3

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] First part of the replica promotion tests + testplan

[Martin Basti]

On 09.11.2015 17:21, Martin Basti wrote:



On 09.11.2015 15:09, Oleg Fayans wrote:

Hi guys,

Here are first two automated testcases from this (so far incomplete) 
testplan: http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan


Testplan review is highly appreciated






Hello,

I did not test patch, I just read the testplan, I have a few comments:

I'm not sure how test plan should look to cover QA needs, so maybe my 
following notes what is missing in test plan are not important.


1)
CA has been affected by replica promotion patches
1a)
test if ipa-ca-install works on replica with domain level 1 (test exists)
1b)
test if ipa-ca-install works on replica with domain level 0 (test exists)
1c)
test if ipa-ca-install works on master with domain level 1 (test exists)
1d)
test if ipa-ca-install works on master with domain level 0 (test exists)
1e)
test if ipa-ca-install with replica file fails with domain level 1 on 
replica

1f)
test if ipa-ca-install fails without replica file with domain level 0 
on replica

1g)
test if ipa-ca-install works on CA-less master with domain level 0 
(I'm not sure but probably tests exists)

1h)
test if ipa-ca-install works on CA-less master with domain level 1 
(I'm not sure but probably tests exists)


2)
KRA has been affected by replica promotion patches
2a)
test if ipa-kra-install works on replica with domain level 1 (test exists)
2b)
test if ipa-kra-install works on replica with domain level 0 (test exists)
2c)
test if ipa-krainstall works on master with domain level 1 (test exists)
2d)
test if ipa-kra-install works on master with domain level 0 (test exists)
2e)
test if ipa-kra-install with replica file fails with domain level 1 on 
replica

2f)
test if ipa-kra-install fails without replica file with domain level 0 
on replica


3) (not sure if this belongs to replica promotion or topology plugin 
testing)

ipa-replica-manage behaves differently with domain level 1
3a)
ipa-replica-manage connect should nto work with domain level 1
3b)
ipa-replica-manage disconnect should not work with domain level 1

4)
ipa-csreplica-manage behaves differently with domain level 1
4a)
ipa-csreplica-manage connect should not work with domain level 1
4b)
ipa-csreplica-manage disconnect should not work with domain level 1
4c)
ipa-csreplica-manage del should not work with domain level 1

5) (this is not related to replica so much, but we miss this test)
5a)
create new replica after master is restored from backup with domain 
level 1

5b)
create new replica after master is restored from backup with domain 
level 0


Martin^2



6) installation replica with a single command (client + replica install)
https://fedorahosted.org/freeipa/ticket/5310
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Martin Kosek]

On 11/23/2015 01:40 PM, Tomas Babej wrote:
> 
> 
> On 11/23/2015 01:31 PM, Jan Cholasta wrote:
>> On 23.11.2015 13:28, Tomas Babej wrote:
>>>
>>>
>>> On 11/23/2015 01:11 PM, Jan Cholasta wrote:
 On 23.11.2015 12:53, Tomas Babej wrote:
> Hi,
>
> If the code within the private_ccache contextmanager does not
> set/removes the KRB5CCNAME, the pop method will raise KeyError, which
> will cause unnecessary termination of the code flow.
>
> Make sure the KRB5CCNAME is popped out of os.environ only if present.
>
> Tomas

 NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.
>>>
>>> Yeah, well, both ways are equivalent, I found the if statement more
>>> explicit. We can go with the suggested version, if it's more pleasing
>>> though - patch is attached.
>>>

 Also I don't think the comment is necessary, it's quite obvious what the
 code does.

>>>
>>> I don't think an explanatory comment can hurt, ever. Worst thing that
>>> happens is that somebody is assured that he understands the code
>>> correctly.
>>
>> Actually the worst thing is that someone changes the code without
>> changing the comment. If the comment does not add any real value, it's
>> only a maintenance burden.
>>
> 
> Yep, that's a real issue in our code base (i.e. I recently removed such
> a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making
> sure the comments describe the implementation properly is on the
> author/reviewer though.
> 
> What's "added value" highly depends on your skill set, and familiarity
> with the code base. Particularly the familiarity with the code base can
> diminish over time even for the author, and those are the times where
> comments can come to the rescue.
> 
> For a newcomer to the project, even a trivial comment (from the point of
> view of the experienced developer) can be valuable.

While I agree with Tomas in general, in this particular case I also do not see
the value of the comment. All is said in the 4 lines of code, no deep FreeIPA
knowledge required:

 if original_value is not None:
 os.environ['KRB5CCNAME'] = original_value
 else:
-os.environ.pop('KRB5CCNAME')
+# No value was set originally, make sure no value is set now
+os.environ.pop('KRB5CCNAME', None)

I hope the discussion around the comment does not expand too much, there is
enough work in 4.3 to do...

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Jan Cholasta]

On 23.11.2015 13:40, Tomas Babej wrote:



On 11/23/2015 01:31 PM, Jan Cholasta wrote:

On 23.11.2015 13:28, Tomas Babej wrote:



On 11/23/2015 01:11 PM, Jan Cholasta wrote:

On 23.11.2015 12:53, Tomas Babej wrote:

Hi,

If the code within the private_ccache contextmanager does not
set/removes the KRB5CCNAME, the pop method will raise KeyError, which
will cause unnecessary termination of the code flow.

Make sure the KRB5CCNAME is popped out of os.environ only if present.

Tomas


NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.


Yeah, well, both ways are equivalent, I found the if statement more
explicit. We can go with the suggested version, if it's more pleasing
though - patch is attached.



Also I don't think the comment is necessary, it's quite obvious what the
code does.



I don't think an explanatory comment can hurt, ever. Worst thing that
happens is that somebody is assured that he understands the code
correctly.


Actually the worst thing is that someone changes the code without
changing the comment. If the comment does not add any real value, it's
only a maintenance burden.



Yep, that's a real issue in our code base (i.e. I recently removed such
a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making
sure the comments describe the implementation properly is on the
author/reviewer though.

What's "added value" highly depends on your skill set, and familiarity
with the code base. Particularly the familiarity with the code base can
diminish over time even for the author, and those are the times where
comments can come to the rescue.


Let's take a look at the code:

if original_value is not None:
os.environ['KRB5CCNAME'] = original_value
else:
os.environ.pop('KRB5CCNAME', None)

Can you tell me what in there couldn't be obvious to a person with even 
the most basic skill set?


IMHO a docstring for private_cccache would add some real value, but this 
comment alone does not.




For a newcomer to the project, even a trivial comment (from the point of
view of the experienced developer) can be valuable.


Following this logic, there should be a comment for every line of our 
code, which is ridiculous.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH 0385] replicainstall: Add possiblity to install client in one

[Tomas Babej]

Hi,

this patch implements the single command replica promotion
for #5310.

Tomas

https://fedorahosted.org/freeipa/ticket/5310
From 8dbb1f420533793f20160b7927e4a1e4d2bd9611 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Mon, 23 Nov 2015 12:46:15 +0100
Subject: [PATCH] replicainstall: Add possiblity to install client in one
 command

https://fedorahosted.org/freeipa/ticket/5310
---
 ipaserver/install/server/replicainstall.py | 70 +++---
 1 file changed, 65 insertions(+), 5 deletions(-)

diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 5ce9eb7092b5349cc9db13b465b3d5b033538ab6..0ef8380b33ff0f58ecd2f0d2a37489ee8376614f 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -755,6 +755,50 @@ def install(installer):
 
 
 @common_cleanup
+def ensure_enrolled(installer):
+config = installer._config
+
+# Perform only if we have the necessary options
+if not any([installer.principal and installer.admin_password,
+installer.keytab]):
+return
+
+# Call client install script
+service.print_msg("Configuring client side components")
+try:
+args = [paths.IPA_CLIENT_INSTALL, "--unattended"]
+if installer.domain_name:
+args.extend(["--domain", installer.domain_name])
+if installer.server_name:
+args.extend(["--server", installer.server_name])
+if installer.realm_name:
+args.extend(["--realm", installer.realm_name])
+if installer.host_name:
+args.extend(["--hostname", installer.host_name])
+
+if installer.principal:
+args.extend(["--principal", installer.principal])
+if installer.admin_password:
+args.extend(["--password", installer.admin_password])
+if installer.keytab:
+args.extend(["--keytab", installer.keytab])
+
+if installer.no_dns_sshfp:
+args.append("--no-dns-sshfp")
+if installer.ssh_trust_dns:
+args.append("--ssh-trust-dns")
+if installer.no_ssh:
+args.append("--no-ssh")
+if installer.no_sshd:
+args.append("--no-sshd")
+if installer.mkhomedir:
+args.append("--mkhomedir")
+ipautil.run(args)
+except Exception as e:
+sys.exit("Configuration of client side components failed!\n"
+ "ipa-client-install returned: " + str(e))
+
+@common_cleanup
 def promote_check(installer):
 options = installer
 
@@ -1112,9 +1156,6 @@ class Replica(BaseServer):
 description="a file generated by ipa-replica-prepare",
 )
 
-realm_name = None
-domain_name = None
-
 setup_ca = Knob(BaseServer.setup_ca)
 setup_kra = Knob(BaseServer.setup_kra)
 setup_dns = Knob(BaseServer.setup_dns)
@@ -1138,8 +1179,19 @@ class Replica(BaseServer):
 cli_short_name='w',
 )
 
+server_name = Knob(
+str, None,
+description="fully qualified name of IPA server to enrooll to",
+cli_name='server',
+)
+
+host_name = Knob(
+str, None,
+description="fully qualified name of this host",
+cli_name='hostname',
+)
+
 mkhomedir = Knob(BaseServer.mkhomedir)
-host_name = None
 no_host_dns = Knob(BaseServer.no_host_dns)
 no_ntp = Knob(BaseServer.no_ntp)
 no_pkinit = Knob(BaseServer.no_pkinit)
@@ -1157,10 +1209,17 @@ class Replica(BaseServer):
 principal = Knob(
 str, None,
 sensitive=True,
-description="User Principal allowed to promote replicas",
+description="User Principal allowed to promote replicas "
+"and join IPA realm",
 cli_short_name='P',
 )
 
+keytab = Knob(
+str, None,
+description="path to backed up keytab from previous enrollment",
+cli_name='keytab',
+)
+
 promote = False
 
 # ca
@@ -1213,6 +1272,7 @@ class Replica(BaseServer):
 @step()
 def main(self):
 if self.promote:
+ensure_enrolled(self)
 promote_check(self)
 yield
 promote(self)
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0102] update idrange tests to reflect disabled modification of local ID ranges

[Tomas Babej]

On 11/20/2015 06:41 PM, Milan Kubík wrote:
> On 11/20/2015 04:06 PM, Martin Babinsky wrote:
>> When I fixed https://fedorahosted.org/freeipa/ticket/4826 I forgot to
>> fix the corresponding xmlrpc tests.
>>
>> This oversight bit me today when I ran in-tree tests on my VM.
>>
>> Here is the patch that makes idrange tests green and shiny again.
>>
> Tests are now passing, ACK
> 

Tests are now passing, however, the code paths that have been checked by
these tests are no longer executed (as an modifications to the local
ranges are prohibited now).

This essentially means that we are testing the same thing multiple times
(i.e. local range cannot be dealt with), and the test names are
misleading currently.

I think we should either reduce the set of (effectively) duplicate
tests, or, better, introduce their replacements that are using allowed
range types, if possible.

Tomas

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Jan Cholasta]

On 23.11.2015 13:28, Tomas Babej wrote:



On 11/23/2015 01:11 PM, Jan Cholasta wrote:

On 23.11.2015 12:53, Tomas Babej wrote:

Hi,

If the code within the private_ccache contextmanager does not
set/removes the KRB5CCNAME, the pop method will raise KeyError, which
will cause unnecessary termination of the code flow.

Make sure the KRB5CCNAME is popped out of os.environ only if present.

Tomas


NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.


Yeah, well, both ways are equivalent, I found the if statement more
explicit. We can go with the suggested version, if it's more pleasing
though - patch is attached.



Also I don't think the comment is necessary, it's quite obvious what the
code does.



I don't think an explanatory comment can hurt, ever. Worst thing that
happens is that somebody is assured that he understands the code correctly.


Actually the worst thing is that someone changes the code without 
changing the comment. If the comment does not add any real value, it's 
only a maintenance burden.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Tomas Babej]

On 11/23/2015 01:31 PM, Jan Cholasta wrote:
> On 23.11.2015 13:28, Tomas Babej wrote:
>>
>>
>> On 11/23/2015 01:11 PM, Jan Cholasta wrote:
>>> On 23.11.2015 12:53, Tomas Babej wrote:
 Hi,

 If the code within the private_ccache contextmanager does not
 set/removes the KRB5CCNAME, the pop method will raise KeyError, which
 will cause unnecessary termination of the code flow.

 Make sure the KRB5CCNAME is popped out of os.environ only if present.

 Tomas
>>>
>>> NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.
>>
>> Yeah, well, both ways are equivalent, I found the if statement more
>> explicit. We can go with the suggested version, if it's more pleasing
>> though - patch is attached.
>>
>>>
>>> Also I don't think the comment is necessary, it's quite obvious what the
>>> code does.
>>>
>>
>> I don't think an explanatory comment can hurt, ever. Worst thing that
>> happens is that somebody is assured that he understands the code
>> correctly.
> 
> Actually the worst thing is that someone changes the code without
> changing the comment. If the comment does not add any real value, it's
> only a maintenance burden.
> 

Yep, that's a real issue in our code base (i.e. I recently removed such
a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making
sure the comments describe the implementation properly is on the
author/reviewer though.

What's "added value" highly depends on your skill set, and familiarity
with the code base. Particularly the familiarity with the code base can
diminish over time even for the author, and those are the times where
comments can come to the rescue.

For a newcomer to the project, even a trivial comment (from the point of
view of the experienced developer) can be valuable.

Tomas

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [patch 0025] Separated Tracker implementations into standalone package

[Lenka Doudova]

NACK - there's a "typo" in /tracker/user_plugin.py, line 17-18:

def get_user_dn(cn):

return DN(('cn', cn), api.env.container_user, api.env.basedn)


should be

def get_user_dn(uid):

return DN(('uid', uid), api.env.container_user, api.env.basedn)


Some tests may fail because of that.
Lenka


On 11/20/2015 08:54 PM, Aleš Mareček wrote:

Looks good. ACK.

- Original Message -

From: "Milan Kubík" 
To: "freeipa-devel" 
Cc: "Filip Skola" , "Ales Marecek" 
Sent: Friday, November 20, 2015 3:44:30 PM
Subject: [patch 0025] Separated Tracker implementations into standalone package

Fixes https://fedorahosted.org/freeipa/ticket/5467
Patches attached.

--
Milan Kubik




--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 0044-0045 Add profiles and default CA ACL on migration

[Jan Cholasta]

On 23.11.2015 06:54, Fraser Tweedale wrote:

Hi all,

The attached patches fix #5459[1]: Default CA ACL rule is not
created during ipa-replica-install.

These patches apply on branch ipa-4-2.  There is a (trivial)
conflict in imports when applying to master.


When a patch does not apply cleanly on all the target branches, you 
should attach a rebased patch as well.




I strongly recommend review / testing of these patches with patches
0042-0043[2] due to the prevalence of the other issue.

[1] https://fedorahosted.org/freeipa/ticket/5459
[2] https://www.redhat.com/archives/freeipa-devel/2015-November/msg00298.html


Patch 0044: ACK

Patch 0045:

1) The check in caacl_del could be better, please take a look at how the 
admins group is handled in ipalib/plugins/group.py for an example. You 
should at least raise ProtectedEntryError rather than ValidationError.


2) _add_default_caacl() should be located in 
ipaserver/install/cainstance.py.


3) Rather than calling the cainstance functions in 
replicainstall.install(), they should be called from 
CAInstance.configure_instance() to make them effective in ipa-ca-install 
and replica promotion as well.


Honza

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 0748 Handle encoding for ipautil.run

[Jan Cholasta]

On 23.11.2015 07:43, Jan Cholasta wrote:

On 19.11.2015 00:55, Petr Viktorin wrote:

On 11/03/2015 02:39 PM, Petr Viktorin wrote:

Hello,
Python 3's strings are Unicode, so data coming to or leaving a Python
program needs to be decoded/encoded if it's to be handled as a string.
One of the boundaries where encoding is necessary is external programs,
specifically, ipautil.run.
Unfortunately there's no one set of options that would work for all
run() invocations, so I went through them all and specified the
stdin/stdout/stderr encoding where necessary. I've also updated the call
sites to make it clearer if the return values are used or not.
If an encoding is not set, run() will accept/return bytes. (This is a
fail-safe setting, since it can't raise errors, and using bytes where
strings are expected generally fails loudly in py3.)

Note that the changes are not effective under Python 2.


ping,
Could someone look at this patch?


Looking.


1) I think a better approach would be to use str for stdin/stdout/stderr 
by default in both Python 2 and 3, i.e. do nothing in Python 2 and 
encode/decode using locale.getpreferredencoding() in Python 3. This is 
consistent with sys.std{in,out,err} in both Python 2 and 3. Using bytes 
or different encoding should be opt-in.


Note that different encoding should be used only if the LC_ALL or LANG 
variables are overriden in the command's environment.



2) The str() here is wrong:

+arg_string = nolog_replace(' '.join(str(shell_quote(a)) for a in 
args), nolog)


It converts b'data' to "b'data'" in Python 3.

Popen accepts both bytes and text in both Python 2 and 3, so there is 
nothing to be done for bytes arguments.



3) I think the "surrogateescape" error handler would be better for 
non-strict decoding, as the text can be encoded back to bytes without 
loss of information.



4) There are direct calls to subprocess.Popen() in a few places, have 
you checked them as well?



--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Tomas Babej]

On 11/23/2015 01:50 PM, Jan Cholasta wrote:
> On 23.11.2015 13:40, Tomas Babej wrote:
>>
>>
>> On 11/23/2015 01:31 PM, Jan Cholasta wrote:
>>> On 23.11.2015 13:28, Tomas Babej wrote:


 On 11/23/2015 01:11 PM, Jan Cholasta wrote:
> On 23.11.2015 12:53, Tomas Babej wrote:
>> Hi,
>>
>> If the code within the private_ccache contextmanager does not
>> set/removes the KRB5CCNAME, the pop method will raise KeyError, which
>> will cause unnecessary termination of the code flow.
>>
>> Make sure the KRB5CCNAME is popped out of os.environ only if present.
>>
>> Tomas
>
> NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.

 Yeah, well, both ways are equivalent, I found the if statement more
 explicit. We can go with the suggested version, if it's more pleasing
 though - patch is attached.

>
> Also I don't think the comment is necessary, it's quite obvious
> what the
> code does.
>

 I don't think an explanatory comment can hurt, ever. Worst thing that
 happens is that somebody is assured that he understands the code
 correctly.
>>>
>>> Actually the worst thing is that someone changes the code without
>>> changing the comment. If the comment does not add any real value, it's
>>> only a maintenance burden.
>>>
>>
>> Yep, that's a real issue in our code base (i.e. I recently removed such
>> a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making
>> sure the comments describe the implementation properly is on the
>> author/reviewer though.
>>
>> What's "added value" highly depends on your skill set, and familiarity
>> with the code base. Particularly the familiarity with the code base can
>> diminish over time even for the author, and those are the times where
>> comments can come to the rescue.
> 
> Let's take a look at the code:
> 
> if original_value is not None:
> os.environ['KRB5CCNAME'] = original_value
> else:
> os.environ.pop('KRB5CCNAME', None)
> 
> Can you tell me what in there couldn't be obvious to a person with even
> the most basic skill set?
> 
> IMHO a docstring for private_cccache would add some real value, but this
> comment alone does not.
> 
>>
>> For a newcomer to the project, even a trivial comment (from the point of
>> view of the experienced developer) can be valuable.
> 
> Following this logic, there should be a comment for every line of our
> code, which is ridiculous.
> 

Here's the version of the patch with the comment removed.

Tomas
From f87ae19e673d4fe2ae1330fb2b1e1a1dbaa55630 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Mon, 23 Nov 2015 12:47:56 +0100
Subject: [PATCH] private_ccache: Harden the removal of KRB5CCNAME env variable

If the code within the private_ccache contextmanager does not
set/removes the KRB5CCNAME, the pop method will raise KeyError, which
will cause unnecessary termination of the code flow.

Make sure the KRB5CCNAME is popped out of os.environ only if present.
---
 ipapython/ipautil.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4acdd1a98818bf311a8fef103e7219cc62a28ec1..a5545688d61354716745c7814b6bbf1ae316c13d 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1367,7 +1367,7 @@ def private_ccache(path=None):
 if original_value is not None:
 os.environ['KRB5CCNAME'] = original_value
 else:
-os.environ.pop('KRB5CCNAME')
+os.environ.pop('KRB5CCNAME', None)
 
 if os.path.exists(path):
 os.remove(path)
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0064] Check if IPA is configured before attempting a winsync migration

[Tomas Babej]

On 11/23/2015 12:11 PM, Martin Babinsky wrote:
> On 11/20/2015 07:10 PM, Gabe Alford wrote:
>> Thanks. Updated patch attached.
>>
>>
>> Gabe
>>
>> On Fri, Nov 20, 2015 at 10:36 AM, Martin Babinsky > > wrote:
>>
>> On 11/20/2015 04:02 PM, Gabe Alford wrote:
>>
>> Hello,
>>
>> Fix for https://fedorahosted.org/freeipa/ticket/5470
>>
>> Thanks,
>>
>> Gabe
>>
>>
>> Hi Gabe,
>>
>> patch looks good. IMHO it would be better if you moved the check
>> before API initialization like so:
>>
>> """
>> @@ -340,6 +340,12 @@ class WinsyncMigrate(admintool.AdminTool):
>>   the plumbing.
>>   """
>>
>> +# Check if the IPA server is configured before attempting
>> to migrate
>> +try:
>> +installutils.check_server_configuration()
>> +except RuntimeError as e:
>> +sys.exit(e)
>> +
>>   # Finalize API
>>   api.bootstrap(in_server=True, context='server')
>>   api.finalize()
>> """
>>
>> There's no point in initializing API if there is no server installed.
>>
>> --
>> Martin^3 Babinsky
>>
>>
> Thanks, ACK.
> 

Pushed to:
master: 84e479edaaeb64567f4cfc847aa735bbd106220d
ipa-4-2: dbc442cc608a49726bcca9749b01dcd363b3ed8f

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 151-153] ipasam: fix wrong usage of talloc_new()

[Tomas Babej]

On 11/18/2015 12:59 PM, Alexander Bokovoy wrote:
> On Wed, 18 Nov 2015, Sumit Bose wrote:
>> Hi,
>>
>> please find attached 3 small patches for ipasam. The first fixes
>> https://fedorahosted.org/freeipa/ticket/5457 . The second is related
>> because if the compat tree is enabled the lookup will still fails
>> because the group is found twice.
>>
>> The third patch fixes an issue valgrind found while I was checking the
>> other issue.
> ACK for all three, they are obvious fixes (now ;).
> 
> We should also commit them to 4.1 and 4.2.
> 

Pushed to:
ipa-4-2: 181d2542a00faaff3d8773536d88901acdefefb9
ipa-4-1: 47df94943ab9fac75fce429078528a2113c463d3
master: 657cf958c6fc6767d09cfbd2d84046d5b84e9f80

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0002] Refactor test_group_plugin

[Filip Škola]

Found couple of issues (broke some dependencies).

NACK

F.

On Fri, 20 Nov 2015 13:56:36 +0100
Filip Škola  wrote:

> Another one.
> 
> F.


-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Rob Crittenden]

Tomas Babej wrote:
> 
> 
> On 11/23/2015 01:50 PM, Jan Cholasta wrote:
>> On 23.11.2015 13:40, Tomas Babej wrote:
>>>
>>>
>>> On 11/23/2015 01:31 PM, Jan Cholasta wrote:
 On 23.11.2015 13:28, Tomas Babej wrote:
>
>
> On 11/23/2015 01:11 PM, Jan Cholasta wrote:
>> On 23.11.2015 12:53, Tomas Babej wrote:
>>> Hi,
>>>
>>> If the code within the private_ccache contextmanager does not
>>> set/removes the KRB5CCNAME, the pop method will raise KeyError, which
>>> will cause unnecessary termination of the code flow.
>>>
>>> Make sure the KRB5CCNAME is popped out of os.environ only if present.
>>>
>>> Tomas
>>
>> NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.
>
> Yeah, well, both ways are equivalent, I found the if statement more
> explicit. We can go with the suggested version, if it's more pleasing
> though - patch is attached.
>
>>
>> Also I don't think the comment is necessary, it's quite obvious
>> what the
>> code does.
>>
>
> I don't think an explanatory comment can hurt, ever. Worst thing that
> happens is that somebody is assured that he understands the code
> correctly.

 Actually the worst thing is that someone changes the code without
 changing the comment. If the comment does not add any real value, it's
 only a maintenance burden.

>>>
>>> Yep, that's a real issue in our code base (i.e. I recently removed such
>>> a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making
>>> sure the comments describe the implementation properly is on the
>>> author/reviewer though.
>>>
>>> What's "added value" highly depends on your skill set, and familiarity
>>> with the code base. Particularly the familiarity with the code base can
>>> diminish over time even for the author, and those are the times where
>>> comments can come to the rescue.
>>
>> Let's take a look at the code:
>>
>> if original_value is not None:
>> os.environ['KRB5CCNAME'] = original_value
>> else:
>> os.environ.pop('KRB5CCNAME', None)
>>
>> Can you tell me what in there couldn't be obvious to a person with even
>> the most basic skill set?
>>
>> IMHO a docstring for private_cccache would add some real value, but this
>> comment alone does not.
>>
>>>
>>> For a newcomer to the project, even a trivial comment (from the point of
>>> view of the experienced developer) can be valuable.
>>
>> Following this logic, there should be a comment for every line of our
>> code, which is ridiculous.
>>
> 
> Here's the version of the patch with the comment removed.

IMHO the comment should have been something like "ensure no KRB5CCNAME
otherwise it blows up in ..." If it took you 20 minutes to track down a
one-line change then a comment might save the next guy who changes the
behavior.

rob

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0067, 0068] ipa-{cacert-renew, otptoken-import}: Fix connection to ldap.

[David Kupka]

On 23/11/15 10:09, Jan Cholasta wrote:

On 23.11.2015 08:53, David Kupka wrote:

On 20/11/15 08:29, Jan Cholasta wrote:

On 19.11.2015 17:28, David Kupka wrote:

https://fedorahosted.org/freeipa/ticket/5468


ipa-cacert-manage is not the only code which uses ldap2 this way.

It would be better to find the root cause of this rather than working
around it.



The root cause is that some scripts are creating custom connection to
LDAP and using api which is not connected to LDAP.
As we discussed personally ipa-cacert-manage and ipa-otptoken-import
have this issue.
Updated patch and new one for ipa-otptoken-import attached.


The patches do not apply on ipa-4-2.



You're right, rebased patches attached.

--
David Kupka
From 3525252cf6efc8a8f0328f0c6696bf558767a3c1 Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Mon, 23 Nov 2015 06:38:17 +
Subject: [PATCH] ipa-cacert-renew: Fix connection to ldap.

https://fedorahosted.org/freeipa/ticket/5468
---
 ipaserver/install/ipa_cacert_manage.py | 32 ++--
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/ipaserver/install/ipa_cacert_manage.py b/ipaserver/install/ipa_cacert_manage.py
index 01ec805fc2094326d119827b4358c143f45f3ec4..8790b7066d7641864f8d83c6339cd0a73c620be0 100644
--- a/ipaserver/install/ipa_cacert_manage.py
+++ b/ipaserver/install/ipa_cacert_manage.py
@@ -105,9 +105,7 @@ class CACertManage(admintool.AdminTool):
 
 if ((command == 'renew' and options.external_cert_files) or
 command == 'install'):
-self.conn = self.ldap_connect()
-else:
-self.conn = None
+self.ldap_connect()
 
 try:
 if command == 'renew':
@@ -115,23 +113,21 @@ class CACertManage(admintool.AdminTool):
 elif command == 'install':
 rc = self.install()
 finally:
-if self.conn is not None:
-self.conn.disconnect()
+if api.Backend.ldap2.isconnected():
+api.Backend.ldap2.disconnect()
 
 return rc
 
 def ldap_connect(self):
-conn = ldap2(api)
-
 password = self.options.password
 if not password:
 try:
 ccache = krbV.default_context().default_ccache()
-conn.connect(ccache=ccache)
+api.Backend.ldap2.connect(ccache=ccache)
 except (krbV.Krb5Error, errors.ACIError):
 pass
 else:
-return conn
+return
 
 password = installutils.read_password(
 "Directory Manager", confirm=False, validate=False)
@@ -139,9 +135,8 @@ class CACertManage(admintool.AdminTool):
 raise admintool.ScriptError(
 "Directory Manager password required")
 
-conn.connect(bind_dn=DN(('cn', 'Directory Manager')), bind_pw=password)
+api.Backend.ldap2.connect(bind_dn=DN(('cn', 'Directory Manager')), bind_pw=password)
 
-return conn
 
 def renew(self):
 ca = cainstance.CAInstance(api.env.realm, certs.NSS_DIR)
@@ -202,9 +197,10 @@ class CACertManage(admintool.AdminTool):
   "--external-cert-file=/path/to/external_ca_certificate")
 
 def renew_external_step_2(self, ca, old_cert):
-print "Importing the renewed CA certificate, please wait"
+print("Importing the renewed CA certificate, please wait")
 
 options = self.options
+conn = api.Backend.ldap2
 cert_file, ca_file = installutils.load_external_cert(
 options.external_cert_files, x509.subject_base())
 
@@ -273,21 +269,21 @@ class CACertManage(admintool.AdminTool):
 except RuntimeError:
 break
 certstore.put_ca_cert_nss(
-self.conn, api.env.basedn, ca_cert, nickname, ',,')
+conn, api.env.basedn, ca_cert, nickname, ',,')
 
 dn = DN(('cn', self.cert_nickname), ('cn', 'ca_renewal'),
 ('cn', 'ipa'), ('cn', 'etc'), api.env.basedn)
 try:
-entry = self.conn.get_entry(dn, ['usercertificate'])
+entry = conn.get_entry(dn, ['usercertificate'])
 entry['usercertificate'] = [cert]
-self.conn.update_entry(entry)
+conn.update_entry(entry)
 except errors.NotFound:
-entry = self.conn.make_entry(
+entry = conn.make_entry(
 dn,
 objectclass=['top', 'pkiuser', 'nscontainer'],
 cn=[self.cert_nickname],
 usercertificate=[cert])
-self.conn.add_entry(entry)
+conn.add_entry(entry)
 except errors.EmptyModlist:
 pass
 
@@ -362,7 +358,7 @@ class CACertManage(admintool.AdminTool):
 
 try:
 certstore.put_ca_cert_nss(
-self.conn, api.env.basedn, cert, nickname, trust_flags)
+

Re: [Freeipa-devel] [PATCHES 509-514] replica promotion: use host credentials when setting up replication

[Jan Cholasta]

On 23.11.2015 15:34, Simo Sorce wrote:

On Mon, 2015-11-23 at 08:54 +0100, Jan Cholasta wrote:

On 20.11.2015 17:58, Simo Sorce wrote:

On Fri, 2015-11-20 at 16:49 +0100, Jan Cholasta wrote:

On 19.11.2015 17:43, Simo Sorce wrote:

[..]

On the patches
--
509:
- commit says only: "aci: add IPA servers host group 'ipaservers'"
but it does other things like changing how CA renewal certificate acis
are added, I think that must be separated in another patch.

- Why "Manage Host Keytab"  aci has been changed to exclude ipaservers ?


To allow only members of the admins group to manage keytabs of IPA
servers. This is analogous to how only members of the admins group can
change passwords of other admins.


I guess there is an interaction between ACIs I am not getting here, can
you give a small overview of how we end up here ?


This ACI allows admins to modify keytab of any host:

aci: (targetattr = "krbPrincipalKey || krbLastPwdChange")(target =
"ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX;)(version 3.0;acl
"Admins can manage host keytab";allow (write) groupdn =
"ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX;;)

The original "Manage Host Keytab" ACI allows any user with the
corresponding permission to modify keytab of any host.

The modified "Manage Host Keytab" ACI allows any user with the
correcponding permission to modify keytab of any host except for members
of ipaservers.

The result is that the user has to be member of admins to be able to
modify keytab of IPA servers.


If IPAservers are not supposed to access those attributes, why are they
added to the permissions at all ?


The effect of the change is not to limit access of IPA servers, but to
limit access of non-admin users *to* IPA servers.


Ah, thanks for explaining, this makes a lot of sense.


Wouldn't it make sense to have 2 set of permissions ?
One for admin type users (with access to all attributes) and one for
less privileged users ?


There already is a separate ACI which allows admins full access to the
attributes, see above.




- Why adding the host to the ipaservers group is done in the
krbinstance ? I would expect LDAP ops to be mostly done in the
DSinstance.


It is the same place where the host entry is created.


I will translate this with "historical" :-)


- I do not see where the host is added to the ipaservers group on
upgrade.


It's in install/updates/20-ipaservers_hostgroup.update:

add: member: fqdn=$FQDN,cn=computers,cn=accounts,$SUFFIX


Ahh, I missed that, thanks!


510:
- We should probably tightenup the ACI to allos host X to only add
memberPrincipal = X and no other value, also the host should not be
allowed to change the memberPrincipal attribute only the keys.
If we can't express this in ACIs we can live with the ones you propose
though.


I think this can be done.

memberPrincipal is included in the ACI because KEMLdap.set_key() touches
it. Perhaps it is not intentional?


Yeah, it is a bug in set_key(), of course we need to add memberPrincipal
when creating a whole new entry, but we should not modify it on existing
entries.


I though so, I will fix that.




511:
ack, but I think you should catch potential exceptions from the
os.rmdir, as it will throw if there is some other file in the dir (for
whatever reason). The exception can be safely ignored, we just do not
want to blow up with a traceback here.


OK.



512:
- I do not think this is correct, it seem to me you are overriding the
local ccache (if any) with the host keytab. If I read this right, this
means that if you run kinit admin && ipa-replica-install then you will
kind your admin credentials gone and a host TGT instead. Am I reading
this change correctly ?


No. :-) Temporary ccache is now used for the install and the host is
kinited into it. The initial ccache is used only for connection check
(and adding the host to ipaservers in patch 514). Your kinit admin will
not be lost.


Ok, got it.



513:
Nack
- Should be folded into 510


These ACIs are required only for patch 514.


Yes, but they are close and will give a full picture of the change
required for the feature if they are together. They won't hurt anything
even if unused until a few patches later (IIUC).


OK, makes sense.




- Should not allow all hosts in the domain but only ipaservers for aal
three changes


If the host isn't member of ipaservers before ipa-replica-install is
run, the "Check that we don't already have a replication agreement",
"Detect if the other master can handle replication managers" and "Find
if any server has a CA" steps will fail without these ACIs, which breaks
patch 514.


Yes I understood that, but it is intentional, these are administrative
checks, they need to be done as admin or with a host account that has
been pre-added to the ipaservers group. We do not want to expose
information to all other hosts in the domain as replication agreements
can include passwords in some cases, or other access information the
admin may not want to make available to random 

Re: [Freeipa-devel] [PATCHES 509-514] replica promotion: use host credentials when setting up replication

[Simo Sorce]

On Mon, 2015-11-23 at 08:54 +0100, Jan Cholasta wrote:
> On 20.11.2015 17:58, Simo Sorce wrote:
> > On Fri, 2015-11-20 at 16:49 +0100, Jan Cholasta wrote:
> >> On 19.11.2015 17:43, Simo Sorce wrote:
> > [..]
> >>> On the patches
> >>> --
> >>> 509:
> >>> - commit says only: "aci: add IPA servers host group 'ipaservers'"
> >>> but it does other things like changing how CA renewal certificate acis
> >>> are added, I think that must be separated in another patch.
> >>>
> >>> - Why "Manage Host Keytab"  aci has been changed to exclude ipaservers ?
> >>
> >> To allow only members of the admins group to manage keytabs of IPA
> >> servers. This is analogous to how only members of the admins group can
> >> change passwords of other admins.
> >
> > I guess there is an interaction between ACIs I am not getting here, can
> > you give a small overview of how we end up here ?
> 
> This ACI allows admins to modify keytab of any host:
> 
> aci: (targetattr = "krbPrincipalKey || krbLastPwdChange")(target = 
> "ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX;)(version 3.0;acl 
> "Admins can manage host keytab";allow (write) groupdn = 
> "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX;;)
> 
> The original "Manage Host Keytab" ACI allows any user with the 
> corresponding permission to modify keytab of any host.
> 
> The modified "Manage Host Keytab" ACI allows any user with the 
> correcponding permission to modify keytab of any host except for members 
> of ipaservers.
> 
> The result is that the user has to be member of admins to be able to 
> modify keytab of IPA servers.
> 
> > If IPAservers are not supposed to access those attributes, why are they
> > added to the permissions at all ?
> 
> The effect of the change is not to limit access of IPA servers, but to 
> limit access of non-admin users *to* IPA servers.

Ah, thanks for explaining, this makes a lot of sense.

> > Wouldn't it make sense to have 2 set of permissions ?
> > One for admin type users (with access to all attributes) and one for
> > less privileged users ?
> 
> There already is a separate ACI which allows admins full access to the 
> attributes, see above.
> 
> >
> >>> - Why adding the host to the ipaservers group is done in the
> >>> krbinstance ? I would expect LDAP ops to be mostly done in the
> >>> DSinstance.
> >>
> >> It is the same place where the host entry is created.
> >
> > I will translate this with "historical" :-)
> >
> >>> - I do not see where the host is added to the ipaservers group on
> >>> upgrade.
> >>
> >> It's in install/updates/20-ipaservers_hostgroup.update:
> >>
> >> add: member: fqdn=$FQDN,cn=computers,cn=accounts,$SUFFIX
> >
> > Ahh, I missed that, thanks!
> >
> >>> 510:
> >>> - We should probably tightenup the ACI to allos host X to only add
> >>> memberPrincipal = X and no other value, also the host should not be
> >>> allowed to change the memberPrincipal attribute only the keys.
> >>> If we can't express this in ACIs we can live with the ones you propose
> >>> though.
> >>
> >> I think this can be done.
> >>
> >> memberPrincipal is included in the ACI because KEMLdap.set_key() touches
> >> it. Perhaps it is not intentional?
> >
> > Yeah, it is a bug in set_key(), of course we need to add memberPrincipal
> > when creating a whole new entry, but we should not modify it on existing
> > entries.
> 
> I though so, I will fix that.
> 
> >
> >>> 511:
> >>> ack, but I think you should catch potential exceptions from the
> >>> os.rmdir, as it will throw if there is some other file in the dir (for
> >>> whatever reason). The exception can be safely ignored, we just do not
> >>> want to blow up with a traceback here.
> >>
> >> OK.
> >>
> >>>
> >>> 512:
> >>> - I do not think this is correct, it seem to me you are overriding the
> >>> local ccache (if any) with the host keytab. If I read this right, this
> >>> means that if you run kinit admin && ipa-replica-install then you will
> >>> kind your admin credentials gone and a host TGT instead. Am I reading
> >>> this change correctly ?
> >>
> >> No. :-) Temporary ccache is now used for the install and the host is
> >> kinited into it. The initial ccache is used only for connection check
> >> (and adding the host to ipaservers in patch 514). Your kinit admin will
> >> not be lost.
> >
> > Ok, got it.
> >
> >>>
> >>> 513:
> >>> Nack
> >>> - Should be folded into 510
> >>
> >> These ACIs are required only for patch 514.
> >
> > Yes, but they are close and will give a full picture of the change
> > required for the feature if they are together. They won't hurt anything
> > even if unused until a few patches later (IIUC).
> 
> OK, makes sense.
> 
> >
> >>> - Should not allow all hosts in the domain but only ipaservers for aal
> >>> three changes
> >>
> >> If the host isn't member of ipaservers before ipa-replica-install is
> >> run, the "Check that we don't already have a replication agreement",
> >> "Detect if the other master can handle replication managers" and "Find
> >> if any server 

Re: [Freeipa-devel] [PATCHES 509-514] replica promotion: use host credentials when setting up replication

[Simo Sorce]

On Mon, 2015-11-23 at 15:37 +0100, Jan Cholasta wrote:
> 
> Ad alternative is to add the host to ipaservers before the checks are 
> done and remove it again if any of them fail.

Too error prone, I am ok with the current way in your patches
until/unless I can think of a fail safe way. :-)

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] Allow ipa-getkeytab to find server name from config file

[Jan Cholasta]

On 23.11.2015 21:18, Simo Sorce wrote:

Fixes #2203 by reading the server name from /etc/ipa/default.conf if not
provided on the command line.

Simo.


Just a thought: it would be nice if we had libipaconfig and used it 
everywhere (the framework, ipa-getkeytab, certmonger, ...). I don't like 
that there are at least 3 ipa config parser implementations now, each 
with its own quirks.


Honza

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 0044-0045 Add profiles and default CA ACL on migration

[Fraser Tweedale]

On Mon, Nov 23, 2015 at 10:05:32AM +0100, Jan Cholasta wrote:
> On 23.11.2015 06:54, Fraser Tweedale wrote:
> >Hi all,
> >
> >The attached patches fix #5459[1]: Default CA ACL rule is not
> >created during ipa-replica-install.
> >
> >These patches apply on branch ipa-4-2.  There is a (trivial)
> >conflict in imports when applying to master.
> 
> When a patch does not apply cleanly on all the target branches, you should
> attach a rebased patch as well.
> 
> >
> >I strongly recommend review / testing of these patches with patches
> >0042-0043[2] due to the prevalence of the other issue.
> >
> >[1] https://fedorahosted.org/freeipa/ticket/5459
> >[2] https://www.redhat.com/archives/freeipa-devel/2015-November/msg00298.html
> 
> Patch 0044: ACK
> 
> Patch 0045:
> 
> 1) The check in caacl_del could be better, please take a look at how the
> admins group is handled in ipalib/plugins/group.py for an example. You
> should at least raise ProtectedEntryError rather than ValidationError.
> 
> 2) _add_default_caacl() should be located in
> ipaserver/install/cainstance.py.
> 
> 3) Rather than calling the cainstance functions in replicainstall.install(),
> they should be called from CAInstance.configure_instance() to make them
> effective in ipa-ca-install and replica promotion as well.
> 
> Honza
> 
Updated patches for ipa-4-2 and master branches attached.

The new patch 0045 is somewhat more intrusive; I have tested server
install, replica install (with CA) from 3.0 and 4.2 master and
ipa-ca-install with replica file from 3.0 master... but more testing
would be no bad thing!

I'll be offline for a few hours but will check back later tonight;
see where things are at.

Thanks,
Fraser
From 354796196f029865e4f5a652900288e043d9c03a Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Mon, 23 Nov 2015 12:09:32 +1100
Subject: [PATCH 44/45] Do not erroneously reinit NSS in Dogtag interface

The Dogtag interface always attempts to (re)init NSS, which can fail
with SEC_ERROR_BUSY.  Do not reinitialise NSS when it has already
been initialised with the given dbdir.

Part of: https://fedorahosted.org/freeipa/ticket/5459
---
 ipapython/dogtag.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ipapython/dogtag.py b/ipapython/dogtag.py
index 
71de96dc6889ca677db2cf7bb4f2d7b56df832f6..0436d5f464aac20c6b3c7db7b1c3a972d3533823
 100644
--- a/ipapython/dogtag.py
+++ b/ipapython/dogtag.py
@@ -265,7 +265,8 @@ def https_request(host, port, url, secdir, password, 
nickname,
 """
 
 def connection_factory(host, port):
-conn = nsslib.NSSConnection(host, port, dbdir=secdir,
+no_init = secdir == nsslib.current_dbdir
+conn = nsslib.NSSConnection(host, port, dbdir=secdir, no_init=no_init,
 tls_version_min=api.env.tls_version_min,
 tls_version_max=api.env.tls_version_max)
 conn.set_debuglevel(0)
-- 
2.4.3

From c0234a507fada45eb64a00e1d3e6ed39321564bc Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Mon, 23 Nov 2015 12:09:32 +1100
Subject: [PATCH 44/45] Do not erroneously reinit NSS in Dogtag interface

The Dogtag interface always attempts to (re)init NSS, which can fail
with SEC_ERROR_BUSY.  Do not reinitialise NSS when it has already
been initialised with the given dbdir.

Part of: https://fedorahosted.org/freeipa/ticket/5459
---
 ipapython/dogtag.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ipapython/dogtag.py b/ipapython/dogtag.py
index 
26b2de6ca77202fa9ccc61ee16ed7623e10ecb5f..8996902ba92f0fdd6106e2650c2decde375c593b
 100644
--- a/ipapython/dogtag.py
+++ b/ipapython/dogtag.py
@@ -255,7 +255,8 @@ def https_request(host, port, url, secdir, password, 
nickname,
 """
 
 def connection_factory(host, port):
-conn = nsslib.NSSConnection(host, port, dbdir=secdir,
+no_init = secdir == nsslib.current_dbdir
+conn = nsslib.NSSConnection(host, port, dbdir=secdir, no_init=no_init,
 tls_version_min=api.env.tls_version_min,
 tls_version_max=api.env.tls_version_max)
 conn.set_debuglevel(0)
-- 
2.4.3

From 81d939ad6755ab80b7db2e592d08730d88f1e06b Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Mon, 23 Nov 2015 12:09:32 +1100
Subject: [PATCH 45/45] Add profiles and default CA ACL on migration

Profiles and the default CA ACL were not being added during replica
install from pre-4.2 servers.  Update ipa-replica-install to add
these if they are missing.

Also update the caacl plugin to prevent deletion of the default CA
ACL and instruct the administrator to disable it instead.

To ensure that the cainstance installation can add profiles, supply
the RA certificate as part of the instance configuration.
Certmonger renewal setup is avoided at this point because the NSSDB
gets reinitialised later in installation procedure.

Re: [Freeipa-devel] [PATCH 0067, 0068] ipa-{cacert-renew, otptoken-import}: Fix connection to ldap.

[Jan Cholasta]

On 23.11.2015 15:17, David Kupka wrote:

On 23/11/15 10:09, Jan Cholasta wrote:

On 23.11.2015 08:53, David Kupka wrote:

On 20/11/15 08:29, Jan Cholasta wrote:

On 19.11.2015 17:28, David Kupka wrote:

https://fedorahosted.org/freeipa/ticket/5468


ipa-cacert-manage is not the only code which uses ldap2 this way.

It would be better to find the root cause of this rather than working
around it.



The root cause is that some scripts are creating custom connection to
LDAP and using api which is not connected to LDAP.
As we discussed personally ipa-cacert-manage and ipa-otptoken-import
have this issue.
Updated patch and new one for ipa-otptoken-import attached.


The patches do not apply on ipa-4-2.



You're right, rebased patches attached.


Thanks, ACK.

Pushed to:
master: 2ef1eb0ae75270d37dcbb106e431a98eb02f0993
ipa-4-2: 8d59f7752c2539378d4383871f13a17b048edcc6

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0002] Refactor test_group_plugin

[Filip Škola]

Sending updated patch.

F.

On Mon, 23 Nov 2015 14:59:34 +0100
Filip Škola  wrote:

> Found couple of issues (broke some dependencies).
> 
> NACK
> 
> F.
> 
> On Fri, 20 Nov 2015 13:56:36 +0100
> Filip Škola  wrote:
> 
> > Another one.
> > 
> > F.
> 
> 

>From d6e30ee42ea427e9a2d5a85a787eddffd557eeba Mon Sep 17 00:00:00 2001
From: Filip Skola 
Date: Mon, 9 Nov 2015 16:48:55 +0100
Subject: [PATCH] Refactor test_group_plugin, use GroupTracker for tests

---
 ipatests/test_xmlrpc/test_group_plugin.py| 1881 +-
 ipatests/test_xmlrpc/tracker/__init__.py |   22 +
 ipatests/test_xmlrpc/tracker/group_plugin.py |  303 +
 3 files changed, 927 insertions(+), 1279 deletions(-)
 create mode 100644 ipatests/test_xmlrpc/tracker/__init__.py
 create mode 100644 ipatests/test_xmlrpc/tracker/group_plugin.py

diff --git a/ipatests/test_xmlrpc/test_group_plugin.py b/ipatests/test_xmlrpc/test_group_plugin.py
index ed38c696e643a394510b6cbf7988f8c17520daf4..4350f128bad6306ac37492a8f5fdbb74b64478ab 100644
--- a/ipatests/test_xmlrpc/test_group_plugin.py
+++ b/ipatests/test_xmlrpc/test_group_plugin.py
@@ -1,6 +1,7 @@
 # Authors:
 #   Rob Crittenden 
 #   Pavel Zuna 
+#   Filip Skola 
 #
 # Copyright (C) 2008  Red Hat
 # see file 'COPYING' for use and warranty information
@@ -26,1325 +27,647 @@ import pytest
 
 from ipalib import api, errors
 from ipatests.test_xmlrpc import objectclasses
-from xmlrpc_test import (Declarative, fuzzy_digits, fuzzy_uuid, fuzzy_set_ci,
- add_sid, add_oc, XMLRPC_test, raises_exact)
+from ipatests.test_xmlrpc.xmlrpc_test import (
+fuzzy_digits, fuzzy_uuid, fuzzy_set_ci, add_sid, add_oc,
+XMLRPC_test, raises_exact
+)
 from ipapython.dn import DN
-from ipatests.test_xmlrpc.test_user_plugin import get_user_result
 
-from ipatests.test_xmlrpc.ldaptracker import Tracker
-from ipatests.test_xmlrpc.test_user_plugin import UserTracker
+from ipatests.test_xmlrpc.tracker.group_plugin import GroupTracker
+from ipatests.test_xmlrpc.test_user_plugin import UserTracker, user, user_npg
 from ipatests.util import assert_deepequal
 
 
-group1 = u'testgroup1'
-group2 = u'testgroup2'
-group3 = u'testgroup3'
-renamedgroup1 = u'testgroup'
-user1 = u'tuser1'
+notagroup = u'notagroup'
+renamedgroup1 = u'renamedgroup'
+invalidgroup1 = u'+tgroup1'
+external_sid1 = u'S-1-1-123456-789-1'
 
-invalidgroup1=u'+tgroup1'
-
-# When adding external SID member to a group we can't test
-# it fully due to possibly missing Samba 4 python bindings
-# and/or not configured AD trusts. Thus, we'll use incorrect
-# SID value to merely test that proper exceptions are raised
-external_sid1=u'S-1-1-123456-789-1'
 
 def get_group_dn(cn):
 return DN(('cn', cn), api.env.container_group, api.env.basedn)
 
 
-@pytest.mark.tier1
-class test_group(Declarative):
-cleanup_commands = [
-('group_del', [group1], {}),
-('group_del', [group2], {}),
-('group_del', [group3], {}),
-('group_del', [renamedgroup1], {}),
-('user_del', [user1], {}),
-]
-
-tests = [
-
-
-# create group1:
-dict(
-desc='Try to retrieve non-existent %r' % group1,
-command=('group_show', [group1], {}),
-expected=errors.NotFound(reason=u'%s: group not found' % group1),
-),
-
-
-dict(
-desc='Try to update non-existent %r' % group1,
-command=('group_mod', [group1], dict(description=u'Foo')),
-expected=errors.NotFound(reason=u'%s: group not found' % group1),
-),
-
-
-dict(
-desc='Try to delete non-existent %r' % group1,
-command=('group_del', [group1], {}),
-expected=errors.NotFound(reason=u'%s: group not found' % group1),
-),
-
-
-dict(
-desc='Try to rename non-existent %r' % group1,
-command=('group_mod', [group1], dict(setattr=u'cn=%s' % renamedgroup1)),
-expected=errors.NotFound(reason=u'%s: group not found' % group1),
-),
-
-
-dict(
-desc='Create non-POSIX %r' % group1,
-command=(
-'group_add', [group1], dict(description=u'Test desc 1',nonposix=True)
-),
-expected=dict(
-value=group1,
-summary=u'Added group "testgroup1"',
-result=dict(
-cn=[group1],
-description=[u'Test desc 1'],
-objectclass=objectclasses.group,
-ipauniqueid=[fuzzy_uuid],
-dn=get_group_dn('testgroup1'),
-),
-),
-),
-
-
-dict(
-desc='Try to create duplicate %r' % group1,
-command=(
-'group_add', [group1], dict(description=u'Test desc 1')
-),
-

Re: [Freeipa-devel] [PATCH 0385] replicainstall: Add possiblity to install client in one

[Jan Cholasta]

Hi,

On 23.11.2015 12:50, Tomas Babej wrote:

Hi,

this patch implements the single command replica promotion
for #5310.

Tomas

https://fedorahosted.org/freeipa/ticket/5310


1) ensure_enrolled() should be called from promote_check() after the 
client check is done:


client_fstore = sysrestore.FileStore(paths.IPA_CLIENT_SYSRESTORE)
if not client_fstore.has_files():
ensure_enrolled(installer)


2)

+server_name = Knob(
+str, None,
+description="fully qualified name of IPA server to enrooll to",
+cli_name='server',
+)

Please use the same identifier ipa-client-install uses, i.e. 
s/server_name/server/.


Also there is typo in the description: "enrooll".


3)

+host_name = Knob(
+str, None,
+description="fully qualified name of this host",
+cli_name='hostname',
+)

This knob is already defined in BaseServer, there's no need to redefine 
it here (just remove the "host_name = None").


If you want to change the description, change it in BaseServer.


4)

+keytab = Knob(
+str, None,
+description="path to backed up keytab from previous enrollment",
+cli_name='keytab',
+)

ipa-client-install uses the short name -k for the keytab option, it 
should be used here as well.



5) The replica file argument conflicts with the --realm, --domain, 
--server, --admin-password and --principal options. This should be 
checked in Replica.__init__().


The --hostname option should either be conflicting as well or be 
implemented properly for legacy replica install.



6) I think --admin-password should be renamed to --password and the 
description changed, since it now also allows OTP enrollment.



Honza

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH] Remove des3/arcfour from default enctypes

[Simo Sorce]

Note, this does not touch the trust code because apparently we use only
arcfour there.

CCing Alexander to give me a comment about that, probably worth opening
a ticket specific to trusts.

Otherwise addresses #4740

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
From 70b4c8971ca623aa51e8e7d1f0e5d245a05c7396 Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Mon, 23 Nov 2015 13:40:42 -0500
Subject: [PATCH] Use only AES enctypes by default

Remove des3 and arcfour from the defaults for new installs.

NOTE: the ipasam/dcerpc code sill uses arcfour

Signed-off-by: Simo Sorce 

Ticket: https://fedorahosted.org/freeipa/ticket/4740
---
 daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c | 14 +++---
 install/share/kerberos.ldif  |  2 --
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
index 1a8ef47b0fc6a932a4115dfa05ecf1a39c8e762f..5dc606d22305cf63a16feca30aab2728bb20b80d 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
@@ -55,18 +55,10 @@ extern const char *ipa_realm_dn;
 extern const char *ipa_etc_config_dn;
 extern const char *ipa_pwd_config_dn;
 
-/* These are the default enc:salt types if nothing is defined.
- * TODO: retrieve the configure set of ecntypes either from the
- * kfc.conf file or by synchronizing the file content into
- * the directory */
+/* These are the default enc:salt types if nothing is defined in LDAP */
 static const char *ipapwd_def_encsalts[] = {
-"des3-hmac-sha1:normal",
-/*"arcfour-hmac:normal",
-"des-hmac-sha1:normal",
-"des-cbc-md5:normal", */
-"des-cbc-crc:normal",
-/*"des-cbc-crc:v4",
-"des-cbc-crc:afs3", */
+"aes256-cts:special",
+"aes128-cts:special",
 NULL
 };
 
diff --git a/install/share/kerberos.ldif b/install/share/kerberos.ldif
index 41e77952adafaf28bfaa96b4c1f1a81ef96348be..1f556382e262ec1b71eb0f4267de0a987952d84d 100644
--- a/install/share/kerberos.ldif
+++ b/install/share/kerberos.ldif
@@ -30,8 +30,6 @@ krbMaxTicketLife: 86400
 krbMaxRenewableAge: 604800
 krbDefaultEncSaltTypes: aes256-cts:special
 krbDefaultEncSaltTypes: aes128-cts:special
-krbDefaultEncSaltTypes: des3-hmac-sha1:special
-krbDefaultEncSaltTypes: arcfour-hmac:special
 
 # Default password Policy
 dn: cn=global_policy,cn=$REALM,cn=kerberos,$SUFFIX
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [IPAQE][REVIEW-REQUEST][TEST PLAN] Replica promotion

[Oleg Fayans]

Hi all,

Here is a draft of the Replica Promotion test plan
http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan


--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code