Re: [Freeipa-devel] [PATCH 0426] spec: add missing requires to python*-ipalib package

[Jan Cholasta]

Hi,

On 25.2.2016 18:05, Martin Basti wrote:

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

Patch attached.


NACK.

For python 3, the ldap module is provided by python3-pyldap.

Any reason for the random ordering? The requires are not alphabetically 
ordered, so I would prefer if you just appended the new ones.


There are missing as well as redundant requires in other packages, 
shouldn't we fix these too?


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 200] slapi-nis: update configuration to allow external members

[Jan Cholasta]

On 22.2.2016 19:56, Tomas Babej wrote:



On 02/22/2016 06:14 PM, Alexander Bokovoy wrote:

On Mon, 22 Feb 2016, Tomas Babej wrote:



On 02/22/2016 11:48 AM, Alexander Bokovoy wrote:

Hi,

attached patch should update compat tree configuration if it exist to
follow slapi-nis 0.55 which has support for external members of IPA
groups.

However, the real work is done in SSSD. These patches are not upstreamed
yet. We'll need to bump SSSD dependency in future once they come to
distros.





This looks good.

However, the new update file needs to be added to Makefile.am.
Additionally, patch adds a whitespace error.

Updated patch is attached.



ACK.

This should not be pushed until the dependency for SSSD can be bumped.


https://bodhi.fedoraproject.org/updates/FEDORA-2016-d872920f74

--
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 0011] Move freeipa certmonger helpers to libexecdir.

[Jan Cholasta]

On 25.2.2016 14:13, David Kupka wrote:

On 24/02/16 15:07, Rob Crittenden wrote:

David Kupka wrote:

On 23/02/16 16:41, Rob Crittenden wrote:

David Kupka wrote:

On 23/02/16 10:14, Martin Kosek wrote:

On 02/23/2016 09:47 AM, David Kupka wrote:

On 22/02/16 16:15, Martin Kosek wrote:

On 02/22/2016 04:04 PM, Jan Cholasta wrote:

On 22.2.2016 15:56, David Kupka wrote:

On 22/02/16 07:28, Jan Cholasta wrote:

On 18.2.2016 10:10, David Kupka wrote:

On 19/01/16 16:10, David Kupka wrote:

On 19/01/16 14:38, Jan Cholasta wrote:

On 19.1.2016 14:26, Martin Kosek wrote:

On 01/19/2016 01:47 PM, David Kupka wrote:

I've polished the patch attached to #5586 by Timo Aaltonen.

Thanks for the patch. I've fixed the path in specfile and
removed
unused import
but otherwise it works, ACK.

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


Won't this break existing certmonger requests depending on
the old
path?


It will, I don't see any upgrade code.



# getcert list | grep '/usr/lib64/ipa/certmonger'
pre-save command:
/usr/lib64/ipa/certmonger/stop_pkicad
post-save command:
/usr/lib64/ipa/certmonger/renew_ca_cert
"auditSigningCert
cert-pki-ca"
pre-save command:
/usr/lib64/ipa/certmonger/stop_pkicad
post-save command:
/usr/lib64/ipa/certmonger/renew_ca_cert
"ocspSigningCert
cert-pki-ca"
pre-save command:
/usr/lib64/ipa/certmonger/stop_pkicad
post-save command:
/usr/lib64/ipa/certmonger/renew_ca_cert
"subsystemCert
cert-pki-ca"
pre-save command:
/usr/lib64/ipa/certmonger/stop_pkicad
post-save command:
/usr/lib64/ipa/certmonger/renew_ca_cert
"caSigningCert
cert-pki-ca"
post-save command:
/usr/lib64/ipa/certmonger/renew_ra_cert
pre-save command:
/usr/lib64/ipa/certmonger/stop_pkicad
post-save command:
/usr/lib64/ipa/certmonger/renew_ca_cert
"Server-Cert
cert-pki-ca"
post-save command:
/usr/lib64/ipa/certmonger/restart_dirsrv
RHEL72
post-save command:
/usr/lib64/ipa/certmonger/restart_httpd






You're right it will break the upgrade. I haven't noticed that
Server-Cert for DS and HTTPD are not handled by
certificate_renewal_update (ipaserver.install.server.upgrade)
where all
the other trackings are stopped and then configured again
with the
paths.CERTMONGER_COMMAND_TEMPLATE already updated.

Thanks for the catch.



I've updated Timo's patch little more and added
start_tracking_certificates() for dsinstance and httpinstance.
Now the
upgrade works as expected.


The way the patches are split is kind of weird and apparently
confusing
(see the other thread). IMO there should be 2 patches: the first
should
add the ability to change DS and HTTP certmonger config during
upgrade
(i.e. the start_tracking_certificates() methods and
certificate_renewal_update() changes), the second should move
the
helpers (i.e. the actual move and certificate_renewal_update()
version
bump).


Honza, do I understand it correctly that the code is OK but I
did not
split it to the patches correctly?


Yes.


Before acking or pushing, can you please explain for me how the
upgrade of
certmonger tracking requests work? I want to make sure this is
right, so please
bear with me:

1) How does it edit existing tracking requests with the new helper
paths?

2) Does it go and try to edit the requests on every upgrade? Or is
there some
check that requests were updated?

Thanks,
Martin



Whole upgrade of renewal requests is done in
ipaserver/install/server/upgrade.py in
certificate_renewal_upgrade().

First there is version of requests and if it's the same as in state
file
upgrade is skipped.
Then every request is searched over certmonger's DBus interface and
if at least
one is not found it means that there was change in request
configuration. All
tracking requests are then stopped and started again with new
configuration.

So to answer you questions:
1) By stopping the old request with the old parameters (including
path) and
starting new with new parameters.

2) Only if version was bumped which happens only if some of the
requests changes.


Ah, so IIUC, if you bump the version, requests should be properly
updated. The
change looks fine then.



After discussion with Honza, we decided to drop the part comparing
only
base names of pre- and post-save commands and use it as whole. I've
also
split the patches so it's obvious what is going on.

Patches should be applied in this order:

freeipa-dkupka-0091.0


A cert could silently fail to be tracked in
start_tracking_certificates() if no serverid can be found.


In that case it also wouldn't be stopped. The behavior is the same as in
existing stop_tracking_certificates(). Should we rather raise and stop
the upgrade? I guess not but warning would be probably useful. What
solution would you prefer, Rob?


I don't know all the callers of this. It may be perfectly safe to assume
that a serverid is always there, but the implication if it isn't is that
some tracking cert won't be updated properly right? That potentially

Re: [Freeipa-devel] [REVIEW] Intial stab towards Authentication Indicators

[Nathaniel McCallum]

On Thu, 2016-02-25 at 10:49 -0500, Simo Sorce wrote:
> On Thu, 2016-02-25 at 10:32 -0500, Nathaniel McCallum wrote:
> > 
> > On Wed, 2016-02-24 at 09:55 -0500, Nathaniel McCallum wrote:
> > > 
> > > On Sun, 2016-02-21 at 20:50 -0500, Simo Sorce wrote:
> > > > 
> > > > 
> > > > On Sun, 2016-02-21 at 20:20 -0500, Nathaniel McCallum wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > https://github.com/npmccallum/freeipa/pull/1
> > > > > 
> > > > > The above (pseudo) pull request contains four patches against
> > > > > FreeIPA
> > > > > to enable the insertion of Authentication Indicators into
> > > > > Kerberos
> > > > > tickets. The basic flow looks like this.
> > > > > 
> > > > > First, we patch ipa-pwd-extop to return a control indicating
> > > > > what
> > > > > authentication method succeeded resulting in a successful
> > > > > bind.
> > > > > 
> > > > > Second, we patch ipa-otpd to check the returned control to
> > > > > ensure
> > > > > that
> > > > > the bind resulted from an otp validation.
> > > > > 
> > > > > Third, we patch ipa-kdb to enable the KDC to return either
> > > > > the
> > > > > encrypted timestamp or encrypted challenge preauth mechanism
> > > > > when
> > > > > the
> > > > > user is configured for optional 2FA logins. Clients can then
> > > > > decide
> > > > > whether to do 1FA or 2FA login (for kinit, sane behavior
> > > > > already
> > > > > exists).
> > > > > 
> > > > > Forth, we patch ipa-kdb again to insert hard-coded
> > > > > authentication
> > > > > indicators for either OTP or RADIUS.
> > > > > 
> > > > > Some explanation is required for the first two patches.
> > > > > Currently,
> > > > > it
> > > > > is possible to do a 1FA through the otp preauthentication
> > > > > mechanism
> > > > > if
> > > > > the user is configured for doing optional 2FA. However,
> > > > > because
> > > > > we
> > > > > want
> > > > > to insert an authentication indicator in this code path, we
> > > > > need
> > > > > to
> > > > > guarantee that a request going through the otp preauth
> > > > > mechanism
> > > > > actually validates an OTP. This is the purpose of the
> > > > > control.
> > > > > 
> > > > > Items still on the TODO list:
> > > > > 
> > > > >   * Authentication Indicator enforcement
> > > > > - Upstream libkrb5 needs to grow funcs for reading
> > > > > indicators
> > > > > - Schema change to add indicators multi-value attr to
> > > > > services
> > > > > - ipa-kdb needs to implement check_policy_tgs()
> > > > > 
> > > > > 
> > > > >   * SSSD needs to learn to handle optional 2FA
> > > > > 
> > > > > I will write up a project page for all of this tomorrow. But
> > > > > this
> > > > > small
> > > > > code basically amounts to my brainstorming. It is not ready
> > > > > for
> > > > > merge,
> > > > > just basic review.
> > > > > 
> > > > It looks mostly ok, however the LDAP control part needs to be
> > > > done
> > > > as
> > > > a
> > > > request/response pair.
> > > > A client that wishes to know what kind of authentication
> > > > happened
> > > > should
> > > > send a request control, and only in that case , the server will
> > > > send
> > > > the
> > > > associated reply control with the requested information.
> > > I just pushed a new version of the control (now merged into a
> > > single
> > > patch): https://github.com/npmccallum/freeipa/commit/a78191ee5d31
> > > e1de
> > > 39
> > > f28eb637f66199da7e9225
> > > 
> > > In this version the client sends a critical control with no
> > > content
> > > indicating that the server must validate an OTP. If the LDAP
> > > server
> > > doesn't support the control (for whatever reason), bind will
> > > fail. If
> > > the LDAP server doesn't validate an OTP (for whatever reason),
> > > bind
> > > will fail.
> > > 
> > > This approach is simpler and doesn't require a request/response
> > > control
> > > pair.
> > I need some design advice. My goal here is that we need a way to
> > expose
> > the authentication indicators to services in the FreeIPA UI/CLI.
> > 
> > Here is the good news: users can already set these values in
> > FreeIPA
> > using kadmin. They do this by simply setting the require_auth
> > string on
> > the target service principal. Our kdb plugin then encodes these
> > with
> > the rest of the tl_data into the krbExtraData attribute.
> > 
> > I see two approaches here. First, we can try to manipulate the
> > krbExtraData attribute directly. Second, we can create a separate
> > attribute for the authentication indicator strings and then
> > synthesize
> > the tl_data internally in kdb. We would have to do this for both
> > reads
> > and writes so as not to break existing kdb functionality.
> > 
> > The trade-off that I see is that the first method complicates the
> > python framework side where the second method complicates the kdb
> > plugin.
> > 
> > A third option, which I doubt is even possible, is to use kadmin to
> > manipulate this option rather than modifying LDAP directly.
> > 
> > Thoughts?
> We should 

Re: [Freeipa-devel] [PATCH 0425] pylint: suppress false positive no-member errors

[Martin Basti]

On 25.02.2016 15:48, Martin Basti wrote:

The last pylint 1.5 patch, \o/

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



self-NACK too broad disables
-- 
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] 953 advise: configure TLS in redhat_nss_pam_ldapd and redhat_nss_ldap plugins

[Petr Vobornik]

I did not add --enableldapstarttls to config_redhat_nss_ldap because I'm 
not sure if it is present on el5 (IMO it is not).


authconfig in:
* config_redhat_nss_ldap got
  * --enableldaptls

* config_redhat_nss_pam_ldapd got
  * --enableldaptls
  * --enableldapstarttls
options

https://fedorahosted.org/freeipa/ticket/5654
--
Petr Vobornik
From 9efeceb704bb53c3de39a2793faab4c58f80fc60 Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Thu, 25 Feb 2016 15:25:12 +0100
Subject: [PATCH] advise: configure TLS in redhat_nss_pam_ldapd and
 redhat_nss_ldap plugins

authconfig in:
* config_redhat_nss_ldap got
  * --enableldaptls

* config_redhat_nss_pam_ldapd got
  * --enableldaptls
  * --enableldapstarttls
options

https://fedorahosted.org/freeipa/ticket/5654
---
 ipaserver/advise/plugins/legacy_clients.py | 7 ---
 ipatests/test_integration/test_advise.py   | 8 +---
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/ipaserver/advise/plugins/legacy_clients.py b/ipaserver/advise/plugins/legacy_clients.py
index b6e1fc5a1549787fbe2805b0297d79211ae21d77..018175b560a69c418b2efa3a2247eb58c69f1dfe 100644
--- a/ipaserver/advise/plugins/legacy_clients.py
+++ b/ipaserver/advise/plugins/legacy_clients.py
@@ -195,8 +195,9 @@ class config_redhat_nss_pam_ldapd(config_base_legacy_client):
 
 self.log.comment('Use the authconfig to configure nsswitch.conf '
  'and the PAM stack')
-self.log.command('authconfig --updateall --enableldap '
- '--enableldapauth --ldapserver=%s --ldapbasedn=%s\n'
+self.log.command('authconfig --updateall --enableldap --enableldaptls '
+ '--enableldapstarttls --enableldapauth '
+ '--ldapserver=%s --ldapbasedn=%s\n'
  % (uri, base))
 
 def configure_ca_cert(self):
@@ -363,7 +364,7 @@ class config_redhat_nss_ldap(config_base_legacy_client):
 
 self.log.comment('Use the authconfig to configure nsswitch.conf '
  'and the PAM stack')
-self.log.command('authconfig --updateall --enableldap '
+self.log.command('authconfig --updateall --enableldap --enableldaptls '
  '--enableldapauth --ldapserver=%s --ldapbasedn=%s\n'
  % (uri, base))
 
diff --git a/ipatests/test_integration/test_advise.py b/ipatests/test_integration/test_advise.py
index 613096f1caed3efb7db33076da5e57bea58cfa13..e263316b254a26b0c9e5a02f9e970349d1047491 100644
--- a/ipatests/test_integration/test_advise.py
+++ b/ipatests/test_integration/test_advise.py
@@ -104,7 +104,8 @@ class TestAdvice(IntegrationTest):
 advice_regex = "\#\!\/bin\/sh.*" \
"yum[\s]+install[\s]+\-y[\s]+curl[\s]+openssl[\s]+nss_ldap" \
"[\s]+authconfig.*authconfig[\s]+\-\-updateall" \
-   "[\s]+\-\-enableldap[\s]+\-\-enableldapauth[\s]+" \
+   "[\s]+\-\-enableldap[\s]+\-\-enableldaptls"\
+   "[\s]+\-\-enableldapauth[\s]+" \
"\-\-ldapserver=.*[\s]+\-\-ldapbasedn=.*"
 raiseerr = True
 
@@ -116,8 +117,9 @@ class TestAdvice(IntegrationTest):
 advice_regex = "\#\!\/bin\/sh.*" \
"yum[\s]+install[\s]+\-y[\s]+curl[\s]+openssl[\s]+" \
"nss\-pam\-ldapd[\s]+pam_ldap[\s]+authconfig.*" \
-   "authconfig[\s]+\-\-updateall[\s]+" \
-   "\-\-enableldap[\s]+\-\-enableldapauth[\s]+" \
+   "authconfig[\s]+\-\-updateall[\s]+\-\-enableldap"\
+   "[\s]+\-\-enableldaptls[\s]+\-\-enableldapstarttls"\
+   "[\s]+\-\-enableldapauth[\s]+" \
"\-\-ldapserver=.*[\s]+\-\-ldapbasedn=.*"
 raiseerr = True
 
-- 
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] 0017 configure DNA shared config entry to allow connection with GSSAPI

[thierry bordaz]

On 02/25/2016 12:03 PM, Martin Babinsky wrote:

On 02/24/2016 04:30 PM, thierry bordaz wrote:

On 01/21/2016 05:04 PM, Martin Babinsky wrote:

On 01/21/2016 01:37 PM, thierry bordaz wrote:





Hi Thierry,

I have couple of comments to your patch:

1.)
there is a number of PEP8 errors in the patch
(http://paste.fedoraproject.org/313246/33893701), please fix them.

See http://www.freeipa.org/page/Python_Coding_Style for our
conventions used in Python code.

2.)
+DNA_BIND_METHOD   = "dnaRemoteBindMethod"
+DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
+DNA_PLUGIN_DN = 'cn=Distributed Numeric Assignment
Plugin,cn=plugins,cn=config'
+dna_config_base   = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN

Uppercase names are usually reserved for module-level constants. OTOH,
local variables should be lowercase. Also you can instantiate
dna_config_base directly as DN, using 2-member tuples, i. e:

"""
dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed Numeric
Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
"""

When passing DN object to the formatting functions/operators, it is
automatically converted to string so no need to hold string and DN
object separately. This is done in other places (see function/methods
in replication.py).

3.)

+for i in range(len(entries)) :
+
+mod = []
+if entries[i].single_value.get(DNA_BIND_METHOD) != method:
+mod.append((ldap.MOD_REPLACE, DNA_BIND_METHOD, 
method))

+
+if entries[i].single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+mod.append((ldap.MOD_REPLACE, DNA_CONN_PROTOCOL,
protocol))


please use idiomatic Python when handling list of entries, e.g.:

"""
for entry in entries:
   mod = []
   if entry.single_value.get(DNA_BIND_METHOD) != method:
   ...
"""

4.) I think that this method should in DSInstance class since it deals
with directory server configuration. Service is a parent object of all
other service installers/configurators and should contain only methods
common to more children.

5.) Since the method is called from every installer, it could be
beneficial to call it in DSInstance.__common_post_setup() as a part of
Directory server installation. Is there any reason why this is not the
case?

6.)

+while attempt != MAX_WAIT:
+try:
+entries = conn.get_entries(sharedcfgdn,
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
+break
+except errors.NotFound:
+root_logger.debug("So far enable not find DNA shared
config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn,
sharedcfgdn))
+attempt = attempt + 1
+time.sleep(2)
+continue
+
+# safety checking
+# there is no return, if there are several entries, as a
workaround of #5510
+if len(entries) != 1:

I am quite afraid what would happen if the server does not return any
entries until 30 s timeout. The code will then continue to the
condition which can potentially test an uninitialized variable and
blow up with 'NameError'. This should be handled more robustly, e. g.
raise an exception when a timeout is reached and no entries were
returned.

7.)

+if len(mod) > 0:

A Pythonic way to test for non-empty container is

"""
if mods:
   # do stuff
"""

since an empty list/dict/set evaluates to False and non-empty
containers are True.


8.)

+entry = conn.get_entry(entries[i].dn)
+if entry.single_value.get(DNA_BIND_METHOD) != method:
+root_logger.error("Fail to set SASL/GSSAPI bind
method to %s" % (entries[i].dn))
+if entry.single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+root_logger.error("Fail to set LDAP protocol to
%s" % (entries[i].dn))

rather than re-fetching the modified entry and testing what happened,
you can just catch an exception raised by unsuccessfull mod and log an
error like this:

"""
try:
   conn.modify_s(entry.dn, mod)
except Exception as e:
   root_logger.error("Failed to modify entry {}: {}".format(entry, e))
"""

as a matter of fact, if the modify_s operation would fail for some
reason, an ldap exception would be raised and you checks would not
even be executed.

9.)
The debug message on line 219 should read "Unable to find DNA shared
config entry for dnaHostname=%s so far. Retry in 2 sec.". The errors
at the end of the method should have "Failed" instead of "Fail".


Hi Martin,

Finally tested... here is the updated patch. Thanks for you patience


thanks
thierry


Hi Thierry,

the patch works as expected. I have some more nitpicks though:

1.) Please fix the following pep8 errors:

http://paste.fedoraproject.org/329086/56397841/

you can check whether you recent commit conforms to PEP8 by running

"git show -U0 | pep8 --diff"

2.)
+self.step("update DNA shared config entry", 
self.update_dna_shared_config)


I 

[Freeipa-devel] [PATCH 0426] spec: add missing requires to python*-ipalib package

[Martin Basti]

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

Patch attached.
From 3f3cfeb7d26f0b775f2ad40650ba085763ebd86f Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 25 Feb 2016 17:47:10 +0100
Subject: [PATCH] spec: Add missing dependencies to python*-ipalib package

Standalone instalation of python*-ipalib packages does not pull all
required packages and results into import errors.

https://fedorahosted.org/freeipa/ticket/5680
---
 freeipa.spec.in | 8 
 1 file changed, 8 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 219c5ca2f13eaac14746ec4689ba611bbc6fc377..9a6cbfa7165735117c95876a8b27981c9d55b438 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -456,12 +456,14 @@ Requires: keyutils
 Requires: pyOpenSSL
 Requires: python-nss >= 0.16
 Requires: python-cryptography
+Requires: python-custodia
 Requires: python-lxml
 Requires: python-netaddr
 Requires: python-libipa_hbac
 Requires: python-qrcode-core >= 5.0.0
 Requires: python-pyasn1
 Requires: python-dateutil
+Requires: python-dns >= 1.11.1
 Requires: python-yubico >= 1.2.3
 Requires: python-sss-murmur
 Requires: dbus-python
@@ -469,6 +471,8 @@ Requires: python-setuptools
 Requires: python-six
 Requires: python-jwcrypto
 Requires: python-cffi
+Requires: python-ldap >= 2.4.15
+Requires: python-requests
 
 Conflicts: %{alt_name}-python < %{version}
 
@@ -500,12 +504,14 @@ Requires: keyutils
 Requires: python3-pyOpenSSL
 Requires: python3-nss >= 0.16
 Requires: python3-cryptography
+Requires: python3-custodia
 Requires: python3-lxml
 Requires: python3-netaddr
 Requires: python3-libipa_hbac
 Requires: python3-qrcode-core >= 5.0.0
 Requires: python3-pyasn1
 Requires: python3-dateutil
+Requires: python3-dns >= 1.11.1
 Requires: python3-yubico >= 1.2.3
 Requires: python3-sss-murmur
 Requires: python3-dbus
@@ -513,6 +519,8 @@ Requires: python3-setuptools
 Requires: python3-six
 Requires: python3-jwcrypto
 Requires: python3-cffi
+# Requires: python3-ldap >= 2.4.15 # uncomment when python3 package will be ready
+Requires: python3-requests
 
 %description -n python3-ipalib
 IPA is an integrated solution to provide centrally managed Identity (users,
-- 
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] [REVIEW] Intial stab towards Authentication Indicators

[Nathaniel McCallum]

On Thu, 2016-02-25 at 12:19 -0500, Nathaniel McCallum wrote:
> On Thu, 2016-02-25 at 10:49 -0500, Simo Sorce wrote:
> > 
> > On Thu, 2016-02-25 at 10:32 -0500, Nathaniel McCallum wrote:
> > > 
> > > 
> > > On Wed, 2016-02-24 at 09:55 -0500, Nathaniel McCallum wrote:
> > > > 
> > > > 
> > > > On Sun, 2016-02-21 at 20:50 -0500, Simo Sorce wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On Sun, 2016-02-21 at 20:20 -0500, Nathaniel McCallum wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > https://github.com/npmccallum/freeipa/pull/1
> > > > > > 
> > > > > > The above (pseudo) pull request contains four patches
> > > > > > against
> > > > > > FreeIPA
> > > > > > to enable the insertion of Authentication Indicators into
> > > > > > Kerberos
> > > > > > tickets. The basic flow looks like this.
> > > > > > 
> > > > > > First, we patch ipa-pwd-extop to return a control
> > > > > > indicating
> > > > > > what
> > > > > > authentication method succeeded resulting in a successful
> > > > > > bind.
> > > > > > 
> > > > > > Second, we patch ipa-otpd to check the returned control to
> > > > > > ensure
> > > > > > that
> > > > > > the bind resulted from an otp validation.
> > > > > > 
> > > > > > Third, we patch ipa-kdb to enable the KDC to return either
> > > > > > the
> > > > > > encrypted timestamp or encrypted challenge preauth
> > > > > > mechanism
> > > > > > when
> > > > > > the
> > > > > > user is configured for optional 2FA logins. Clients can
> > > > > > then
> > > > > > decide
> > > > > > whether to do 1FA or 2FA login (for kinit, sane behavior
> > > > > > already
> > > > > > exists).
> > > > > > 
> > > > > > Forth, we patch ipa-kdb again to insert hard-coded
> > > > > > authentication
> > > > > > indicators for either OTP or RADIUS.
> > > > > > 
> > > > > > Some explanation is required for the first two patches.
> > > > > > Currently,
> > > > > > it
> > > > > > is possible to do a 1FA through the otp preauthentication
> > > > > > mechanism
> > > > > > if
> > > > > > the user is configured for doing optional 2FA. However,
> > > > > > because
> > > > > > we
> > > > > > want
> > > > > > to insert an authentication indicator in this code path, we
> > > > > > need
> > > > > > to
> > > > > > guarantee that a request going through the otp preauth
> > > > > > mechanism
> > > > > > actually validates an OTP. This is the purpose of the
> > > > > > control.
> > > > > > 
> > > > > > Items still on the TODO list:
> > > > > > 
> > > > > >   * Authentication Indicator enforcement
> > > > > > - Upstream libkrb5 needs to grow funcs for reading
> > > > > > indicators
> > > > > > - Schema change to add indicators multi-value attr to
> > > > > > services
> > > > > > - ipa-kdb needs to implement check_policy_tgs()
> > > > > > 
> > > > > > 
> > > > > >   * SSSD needs to learn to handle optional 2FA
> > > > > > 
> > > > > > I will write up a project page for all of this tomorrow.
> > > > > > But
> > > > > > this
> > > > > > small
> > > > > > code basically amounts to my brainstorming. It is not ready
> > > > > > for
> > > > > > merge,
> > > > > > just basic review.
> > > > > > 
> > > > > It looks mostly ok, however the LDAP control part needs to be
> > > > > done
> > > > > as
> > > > > a
> > > > > request/response pair.
> > > > > A client that wishes to know what kind of authentication
> > > > > happened
> > > > > should
> > > > > send a request control, and only in that case , the server
> > > > > will
> > > > > send
> > > > > the
> > > > > associated reply control with the requested information.
> > > > I just pushed a new version of the control (now merged into a
> > > > single
> > > > patch): https://github.com/npmccallum/freeipa/commit/a78191ee5d
> > > > 31
> > > > e1de
> > > > 39
> > > > f28eb637f66199da7e9225
> > > > 
> > > > In this version the client sends a critical control with no
> > > > content
> > > > indicating that the server must validate an OTP. If the LDAP
> > > > server
> > > > doesn't support the control (for whatever reason), bind will
> > > > fail. If
> > > > the LDAP server doesn't validate an OTP (for whatever reason),
> > > > bind
> > > > will fail.
> > > > 
> > > > This approach is simpler and doesn't require a request/response
> > > > control
> > > > pair.
> > > I need some design advice. My goal here is that we need a way to
> > > expose
> > > the authentication indicators to services in the FreeIPA UI/CLI.
> > > 
> > > Here is the good news: users can already set these values in
> > > FreeIPA
> > > using kadmin. They do this by simply setting the require_auth
> > > string on
> > > the target service principal. Our kdb plugin then encodes these
> > > with
> > > the rest of the tl_data into the krbExtraData attribute.
> > > 
> > > I see two approaches here. First, we can try to manipulate the
> > > krbExtraData attribute directly. Second, we can create a separate
> > > attribute for the authentication indicator strings and then
> > > synthesize
> > > the tl_data 

Re: [Freeipa-devel] [REVIEW] Intial stab towards Authentication Indicators

[Simo Sorce]

On Thu, 2016-02-25 at 16:13 -0500, Nathaniel McCallum wrote:
> On Thu, 2016-02-25 at 12:19 -0500, Nathaniel McCallum wrote:
> > On Thu, 2016-02-25 at 10:49 -0500, Simo Sorce wrote:
> > > 
> > > On Thu, 2016-02-25 at 10:32 -0500, Nathaniel McCallum wrote:
> > > > 
> > > > 
> > > > On Wed, 2016-02-24 at 09:55 -0500, Nathaniel McCallum wrote:
> > > > > 
> > > > > 
> > > > > On Sun, 2016-02-21 at 20:50 -0500, Simo Sorce wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Sun, 2016-02-21 at 20:20 -0500, Nathaniel McCallum wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > https://github.com/npmccallum/freeipa/pull/1
> > > > > > > 
> > > > > > > The above (pseudo) pull request contains four patches
> > > > > > > against
> > > > > > > FreeIPA
> > > > > > > to enable the insertion of Authentication Indicators into
> > > > > > > Kerberos
> > > > > > > tickets. The basic flow looks like this.
> > > > > > > 
> > > > > > > First, we patch ipa-pwd-extop to return a control
> > > > > > > indicating
> > > > > > > what
> > > > > > > authentication method succeeded resulting in a successful
> > > > > > > bind.
> > > > > > > 
> > > > > > > Second, we patch ipa-otpd to check the returned control to
> > > > > > > ensure
> > > > > > > that
> > > > > > > the bind resulted from an otp validation.
> > > > > > > 
> > > > > > > Third, we patch ipa-kdb to enable the KDC to return either
> > > > > > > the
> > > > > > > encrypted timestamp or encrypted challenge preauth
> > > > > > > mechanism
> > > > > > > when
> > > > > > > the
> > > > > > > user is configured for optional 2FA logins. Clients can
> > > > > > > then
> > > > > > > decide
> > > > > > > whether to do 1FA or 2FA login (for kinit, sane behavior
> > > > > > > already
> > > > > > > exists).
> > > > > > > 
> > > > > > > Forth, we patch ipa-kdb again to insert hard-coded
> > > > > > > authentication
> > > > > > > indicators for either OTP or RADIUS.
> > > > > > > 
> > > > > > > Some explanation is required for the first two patches.
> > > > > > > Currently,
> > > > > > > it
> > > > > > > is possible to do a 1FA through the otp preauthentication
> > > > > > > mechanism
> > > > > > > if
> > > > > > > the user is configured for doing optional 2FA. However,
> > > > > > > because
> > > > > > > we
> > > > > > > want
> > > > > > > to insert an authentication indicator in this code path, we
> > > > > > > need
> > > > > > > to
> > > > > > > guarantee that a request going through the otp preauth
> > > > > > > mechanism
> > > > > > > actually validates an OTP. This is the purpose of the
> > > > > > > control.
> > > > > > > 
> > > > > > > Items still on the TODO list:
> > > > > > > 
> > > > > > >   * Authentication Indicator enforcement
> > > > > > > - Upstream libkrb5 needs to grow funcs for reading
> > > > > > > indicators
> > > > > > > - Schema change to add indicators multi-value attr to
> > > > > > > services
> > > > > > > - ipa-kdb needs to implement check_policy_tgs()
> > > > > > > 
> > > > > > > 
> > > > > > >   * SSSD needs to learn to handle optional 2FA
> > > > > > > 
> > > > > > > I will write up a project page for all of this tomorrow.
> > > > > > > But
> > > > > > > this
> > > > > > > small
> > > > > > > code basically amounts to my brainstorming. It is not ready
> > > > > > > for
> > > > > > > merge,
> > > > > > > just basic review.
> > > > > > > 
> > > > > > It looks mostly ok, however the LDAP control part needs to be
> > > > > > done
> > > > > > as
> > > > > > a
> > > > > > request/response pair.
> > > > > > A client that wishes to know what kind of authentication
> > > > > > happened
> > > > > > should
> > > > > > send a request control, and only in that case , the server
> > > > > > will
> > > > > > send
> > > > > > the
> > > > > > associated reply control with the requested information.
> > > > > I just pushed a new version of the control (now merged into a
> > > > > single
> > > > > patch): https://github.com/npmccallum/freeipa/commit/a78191ee5d
> > > > > 31
> > > > > e1de
> > > > > 39
> > > > > f28eb637f66199da7e9225
> > > > > 
> > > > > In this version the client sends a critical control with no
> > > > > content
> > > > > indicating that the server must validate an OTP. If the LDAP
> > > > > server
> > > > > doesn't support the control (for whatever reason), bind will
> > > > > fail. If
> > > > > the LDAP server doesn't validate an OTP (for whatever reason),
> > > > > bind
> > > > > will fail.
> > > > > 
> > > > > This approach is simpler and doesn't require a request/response
> > > > > control
> > > > > pair.
> > > > I need some design advice. My goal here is that we need a way to
> > > > expose
> > > > the authentication indicators to services in the FreeIPA UI/CLI.
> > > > 
> > > > Here is the good news: users can already set these values in
> > > > FreeIPA
> > > > using kadmin. They do this by simply setting the require_auth
> > > > string on
> > > > the target service principal. Our kdb plugin then encodes 

[Freeipa-devel] [FREEIPA INSTALL ISSUE] Recent Tomcat build from F23 updates-testing breaks Dogtag installation

[Martin Babinsky]

Hello everyone,

please note that package tomcat-8.0.32-3.fc23.noarch [1] messes with 
symlinks to Catalina classes used by Dogtag. This makes CA deployment 
blow up spectacularly during FreeIPA server/replica/etc installation. A 
bugzilla exists[2] for this issue and also mentions a workaround which 
makes the symlinks point to correct files.


An alternative is to downgrade tomcat to version 8.0.32-2.fc23 [2] or 
older. That should make Dogtag work again.


If you already encountered this issue give negative karma to the package 
at Bodhi[2].


[1] https://bodhi.fedoraproject.org/updates/FEDORA-2016-5e0bb2f21a
[2] http://koji.fedoraproject.org/koji/buildinfo?buildID=737537

--
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 0034] ipatests: extend permission plugin test with new expected output

[Milan Kubík]

On 02/24/2016 07:05 PM, Martin Basti wrote:



On 24.02.2016 08:34, Milan Kubík wrote:

On 02/18/2016 03:52 PM, Milan Kubík wrote:

On 02/15/2016 04:59 PM, Milan Kubík wrote:

Patch attached. Applies on ipa-4-3 as well.




Updated version of patch fixes test_old_permission_plugin as well.

--
Milan Kubik



Review bump.

--
Milan Kubik



NACK

[mbasti@dhcp129-96 freeipa-devel]$ git show -U0 | pep8 --diff
./ipatests/test_xmlrpc/test_old_permission_plugin.py:528:80: E501 line 
too long (95 > 79 characters)
./ipatests/test_xmlrpc/test_old_permission_plugin.py:586:1: E101 
indentation contains mixed spaces and tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:586:1: W191 
indentation contains tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:587:1: E101 
indentation contains mixed spaces and tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:587:80: E501 line 
too long (95 > 79 characters)
./ipatests/test_xmlrpc/test_old_permission_plugin.py:591:1: E101 
indentation contains mixed spaces and tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:591:1: W191 
indentation contains tabs
./ipatests/test_xmlrpc/test_permission_plugin.py:821:80: E501 line too 
long (99 > 79 characters)
./ipatests/test_xmlrpc/test_permission_plugin.py:884:80: E501 line too 
long (99 > 79 characters)



Sorry for that. Updated patch attached.

--
Milan Kubik

From ecaa2d4ef7a2e4a5d5b953c56e6e5bd760f35012 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= 
Date: Mon, 15 Feb 2016 16:54:34 +0100
Subject: [PATCH] ipatests: extend permission plugin test with new expected
 output

---
 ipatests/test_xmlrpc/test_old_permission_plugin.py | 14 ++
 ipatests/test_xmlrpc/test_permission_plugin.py | 18 ++
 2 files changed, 32 insertions(+)

diff --git a/ipatests/test_xmlrpc/test_old_permission_plugin.py b/ipatests/test_xmlrpc/test_old_permission_plugin.py
index 9e4b561a6f8ff4d6eac767f7f24e35ee4a910eff..09f43fee8c60e4e97d233d256f046fef44d31acf 100644
--- a/ipatests/test_xmlrpc/test_old_permission_plugin.py
+++ b/ipatests/test_xmlrpc/test_old_permission_plugin.py
@@ -524,6 +524,13 @@ class test_old_permission(Declarative):
 'subtree': u'ldap:///%s' % users_dn,
 },
 ],
+messages=({
+'message': (u'Search result has been truncated to '
+'configured search limit.'),
+'code': 13017,
+'type': u'warning',
+'name': u'SearchResultTruncated'
+},),
 ),
 ),
 
@@ -577,6 +584,13 @@ class test_old_permission(Declarative):
 DN(res['dn']).endswith(DN(api.env.container_permission,
   api.env.basedn)) and
 'ipapermission' in res['objectclass']],
+messages=({
+'message': (u'Search result has been truncated to '
+'configured search limit.'),
+'code': 13017,
+'type': u'warning',
+'name': u'SearchResultTruncated'
+},),
 ),
 ),
 
diff --git a/ipatests/test_xmlrpc/test_permission_plugin.py b/ipatests/test_xmlrpc/test_permission_plugin.py
index 01294665814fc667f932272ee8bc3077583b67df..8026e84366e3b9056ed6e2ff6d76c58bdc95140e 100644
--- a/ipatests/test_xmlrpc/test_permission_plugin.py
+++ b/ipatests/test_xmlrpc/test_permission_plugin.py
@@ -816,6 +816,15 @@ class test_permission(Declarative):
 'ipapermlocation': [users_dn],
 },
 ],
+messages=(
+{
+'message': (u'Search result has been truncated to '
+'configured search limit.'),
+'code': 13017,
+'type': u'warning',
+'name': u'SearchResultTruncated'
+},
+),
 ),
 ),
 
@@ -871,6 +880,15 @@ class test_permission(Declarative):
 DN(res['dn']).endswith(DN(api.env.container_permission,
   api.env.basedn)) and
 'ipapermission' in res['objectclass']],
+messages=(
+{
+'message': (u'Search result has been truncated to '
+'configured search limit.'),
+'code': 13017,
+'type': u'warning',
+'name': u'SearchResultTruncated'
+},
+),
 ),
 ),
 
-- 
2.7.1

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

Re: [Freeipa-devel] IPA client realm/domain autodiscovery improvements

[Petr Spacek]

On 24.2.2016 16:30, Sumit Bose wrote:
> On Wed, Feb 24, 2016 at 04:08:14PM +0100, David Kupka wrote:
>> On 24/02/16 15:55, Sumit Bose wrote:
>>> On Wed, Feb 24, 2016 at 03:30:40PM +0100, Martin Babinsky wrote:
 On 02/24/2016 03:20 PM, Sumit Bose wrote:
> On Wed, Feb 24, 2016 at 01:31:55PM +0100, Petr Vobornik wrote:
>> On 02/16/2016 02:23 PM, Martin Babinsky wrote:
>>> Hi list,
>>>
>>> WARNING: huge brain dump ahead.
>>>
>>> During investigation of https://fedorahosted.org/freeipa/ticket/4305 me
>>> and Petr Spaced (CC'ed) came to a conclusion that the IPA realm
>>> autodiscovery code used by ipa-client-install is so convoluted, complex
>>> and hard to understand that the cost of fixing a bug/adding a behavioral
>>> change (there are some other tickets dealing with ipadiscovery, e.g. see
>>> https://fedorahosted.org/freeipa/ticket/5270
>>> https://fedorahosted.org/freeipa/ticket/3912 ) can potentially be higher
>>> that a more large-scale rewrite of the module and related codebase.
>>>
>>> Since we plan to do some general refactoring work anyway, I would like
>>> to discuss the possible course of action we may take to tackle this
>>> issue. I would like to present the following options we have been
>>> discussing so far:
>>>
>>> 1.) Do a substantial rewrite of existing code 
>>> ("ipaclient/ipadiscovery.py")
>>>
>>> We may take the existing IPADiscovery class and try rewrite it in order
>>> to get a more concise and deterministic code, which will:
>>>
>>> * rely more on python-dns and answers provided by resolver (e.g. we are
>>> directly parsing resolv.conf for available domains when we can ask the
>>> resolver to get domains for us)
>>> * be more pythonic (replace error codes with thrown exceptions, clean up
>>> numerous C-isms etc.)
>>> * not try to outsmart user when server/realm/domain is specified
>>> beforehand. Currently, even if you specify all three options, there is
>>> still some DNS discovery performed, hence bug #4305. I think that one
>>> you specify server(s), not magic should be performed and the discovery
>>> process should be reduced to checking whether they are IPA servers and
>>> pull all other info (like realm) from them.
>>>
>>> This may be a considerable effort requiring some time to implement and
>>> get right, but IMHO still comparable to the time spent iteratively
>>> placing 'if' statements into the existing code and hoping to not break
>>> anything. I would even argue it is not worth the effort because we can
>>>
>>> 2.) Use realmd dbus interface to do DNS discovery
>>>
>>> I have attached aquick and dirty script I have slapped together to
>>> leverage 'org.freedesktop.realmd.Discover' interface to do IPA realm
>>> discovery for us. This has a lot of appeal to me since we are leveraging
>>> existing codebase for DNS discovery and will have to handle only cases
>>> when the user manually specifies a list of IPA servers for the client.
>>>
>>> This however pulls in realmd as a (potentially circular) dependency for
>>> ipa client, and when we find bug in the discovery code, we will have to
>>> poke upstream (Stef or Sumit I think) to fix it.
>>>
>>> So from the long-term point of view, Jan Cholasta's (CC'ed) suggestion 
>>> to:
>>>
>>> 3.) Split out IPA discovery portion of realmd to a separate C library
>>> shared between IPA and realmd
>>>
>>> may be best. Both projects will have shared codebase (maintained either
>>> by us or realmd devs), which can be reused also by other people to
>>> create their own discovery/enrollment solution. This would however
>>> require sustained and concerted efforts of both teams to create the
>>> library and possibly rewrite realmd to accommodate this change.
>>>
>>> There may be some other options viable for us, if so please mention them
>>> in a reply. Also please correct any piece of information I got wrong.
>>>
>>> TL;DR: IPA realm/domain discovery is a mess, please suggest a way to fix
>>> it.
>>>
>>
>> #3 sounds good from long term perspective.
>>
>> In short term, i.e., #4305, we should skip discovery when --on-master is
>> used.
>
> I would prefer #2. Because as seen from the patch it is already working
> and can easily be used from python. I think this was also one of the
> reasons why Stef used a DBus service for this instead of a C library
> which then needs various bindings for python, Java, ruby, Go you name
> it.
>

 This approach is also my favorite. However, my (and several of my
 colleagues) concern is that
 https://bugzilla.redhat.com/show_bug.cgi?id=1271551 will break it in
 kickstart environment. I don't know how much of an issue is this, I guess 
 it
 

Re: [Freeipa-devel] [patch 0033] spec file: update the python-polib dependency name to python2-polib

[Milan Kubík]

On 02/15/2016 05:39 PM, Lukas Slebodnik wrote:

On (15/02/16 17:00), Petr Vobornik wrote:

On 02/15/2016 04:37 PM, Milan Kubík wrote:

Reflect the updated name of the package.


Seems to me as a packaging bug in python-polib. It should use python_provide
macro to handle the transition.

There is not a bug in python-polib

sh# rpm -q python2-polib
python2-polib-1.0.7-2.fc23.noarch

sh# rpm -q --provides python2-polib
python-polib = 1.0.7-2.fc23
python2-polib = 1.0.7-2.fc23

However it is a change in behaviour in dnf/yum.
You can see more details in BZ1291850 or better BZ1096506.

This a readon why "dnf builddep" will try to remove package.
(it's not downgrade from dnf point of view)

sh# dnf builddep freeipa.spec
Last metadata expiration check performed 0:17:37 ago on Mon Feb 15 16:19:14
2016.
Package python-setuptools-18.0.1-2.fc23.noarch is already installed, skipping.
Package systemd-222-10.fc23.x86_64 is already installed, skipping.
Package systemd-222-10.fc23.x86_64 is already installed, skipping.
Error: installed package python2-polib-1.0.7-2.fc23.noarch obsoletes
python-polib < 1.0.7-2.fc23 provided by python-polib-1.0.3-6.fc23.noarch
(try to add '--allowerasing' to command line to replace conflicting packages)


You might try to file a dnf BZ but mine 1291850 was two tiles closed as not a
but and then closed as a duplicate of another bug.

IMHO the simplest solution would to push the patch with better explanation
in's a workaround.

LSommit message becuase it's a workaround.

LS

Updated patch with reworded commit message.

--
Milan Kubik

From d55965f8662f335b7319a1868ab072075ed03077 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= 
Date: Mon, 15 Feb 2016 15:54:40 +0100
Subject: [PATCH] spec file: rename the python-polib dependency name to
 python2-polib

Trying to install the package depending on python-polib breaks
when the system has newer (and renamed) version python2-polib.

*This patch is an workaround* for the issue described in [1].
If a renamed package's provides is equal to an older package's
name, dnf tries to install the older package.
When the newer package is in the system, this leads to a conflict.

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1096506
---
 freeipa.spec.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 48fec974246dbc2933dd172318157f3e0e050a3b..caaef8803dd8625f24fa40005f7d83be7e6a6c66 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -77,7 +77,7 @@ BuildRequires:  python-gssapi >= 1.1.2
 BuildRequires:  python-rhsm
 BuildRequires:  pyOpenSSL
 BuildRequires:  pylint >= 1.0
-BuildRequires:  python-polib
+BuildRequires:  python2-polib
 BuildRequires:  python-libipa_hbac
 BuildRequires:  python-memcached
 BuildRequires:  python-lxml
@@ -562,7 +562,7 @@ Requires: python-nose
 Requires: pytest >= 2.6
 Requires: python-paste
 Requires: python-coverage
-Requires: python-polib
+Requires: python2-polib
 Requires: python-pytest-multihost >= 0.5
 Requires: python-pytest-sourceorder
 Requires: ldns-utils
-- 
2.7.1

-- 
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 0033] spec file: update the python-polib dependency name to python2-polib

[Milan Kubík]

On 02/25/2016 11:07 AM, Jan Cholasta wrote:

On 25.2.2016 11:03, Milan Kubík wrote:

On 02/15/2016 05:39 PM, Lukas Slebodnik wrote:

On (15/02/16 17:00), Petr Vobornik wrote:

On 02/15/2016 04:37 PM, Milan Kubík wrote:

Reflect the updated name of the package.


Seems to me as a packaging bug in python-polib. It should use
python_provide
macro to handle the transition.

There is not a bug in python-polib

sh# rpm -q python2-polib
python2-polib-1.0.7-2.fc23.noarch

sh# rpm -q --provides python2-polib
python-polib = 1.0.7-2.fc23
python2-polib = 1.0.7-2.fc23

However it is a change in behaviour in dnf/yum.
You can see more details in BZ1291850 or better BZ1096506.

This a readon why "dnf builddep" will try to remove package.
(it's not downgrade from dnf point of view)

sh# dnf builddep freeipa.spec
Last metadata expiration check performed 0:17:37 ago on Mon Feb 15
16:19:14
2016.
Package python-setuptools-18.0.1-2.fc23.noarch is already installed,
skipping.
Package systemd-222-10.fc23.x86_64 is already installed, skipping.
Package systemd-222-10.fc23.x86_64 is already installed, skipping.
Error: installed package python2-polib-1.0.7-2.fc23.noarch obsoletes
python-polib < 1.0.7-2.fc23 provided by 
python-polib-1.0.3-6.fc23.noarch

(try to add '--allowerasing' to command line to replace conflicting
packages)


You might try to file a dnf BZ but mine 1291850 was two tiles closed
as not a
but and then closed as a duplicate of another bug.

IMHO the simplest solution would to push the patch with better
explanation
in's a workaround.

LSommit message becuase it's a workaround.

LS

Updated patch with reworded commit message.


Please also add "workaround for 
https://bugzilla.redhat.com/show_bug.cgi?id=1096506; comment above the 
changed requires.



Done.

--
Milan Kubik

From 591eb0a92f79f234307cb3b7f4407bb1aa857ff5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= 
Date: Mon, 15 Feb 2016 15:54:40 +0100
Subject: [PATCH] spec file: rename the python-polib dependency name to
 python2-polib

Trying to install the package depending on python-polib breaks
when the system has newer (and renamed) version python2-polib.

*This patch is an workaround* for the issue described in [1].
If a renamed package's provides is equal to an older package's
name, dnf tries to install the older package.
When the newer package is in the system, this leads to a conflict.

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1096506
---
 freeipa.spec.in | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 48fec974246dbc2933dd172318157f3e0e050a3b..c8fd9db8fcdc14313eab724514b4f4a7f6095a37 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -77,7 +77,8 @@ BuildRequires:  python-gssapi >= 1.1.2
 BuildRequires:  python-rhsm
 BuildRequires:  pyOpenSSL
 BuildRequires:  pylint >= 1.0
-BuildRequires:  python-polib
+# workaround for https://bugzilla.redhat.com/show_bug.cgi?id=1096506
+BuildRequires:  python2-polib
 BuildRequires:  python-libipa_hbac
 BuildRequires:  python-memcached
 BuildRequires:  python-lxml
@@ -562,7 +563,8 @@ Requires: python-nose
 Requires: pytest >= 2.6
 Requires: python-paste
 Requires: python-coverage
-Requires: python-polib
+# workaround for https://bugzilla.redhat.com/show_bug.cgi?id=1096506
+Requires: python2-polib
 Requires: python-pytest-multihost >= 0.5
 Requires: python-pytest-sourceorder
 Requires: ldns-utils
-- 
2.7.1

-- 
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] IPA client realm/domain autodiscovery improvements

[Petr Spacek]

On 25.2.2016 11:02, Jan Cholasta wrote:
> On 25.2.2016 10:35, Petr Spacek wrote:
>> On 24.2.2016 16:30, Sumit Bose wrote:
>>> On Wed, Feb 24, 2016 at 04:08:14PM +0100, David Kupka wrote:
 On 24/02/16 15:55, Sumit Bose wrote:
> On Wed, Feb 24, 2016 at 03:30:40PM +0100, Martin Babinsky wrote:
>> On 02/24/2016 03:20 PM, Sumit Bose wrote:
>>> On Wed, Feb 24, 2016 at 01:31:55PM +0100, Petr Vobornik wrote:
 On 02/16/2016 02:23 PM, Martin Babinsky wrote:
> Hi list,
>
> WARNING: huge brain dump ahead.
>
> During investigation of https://fedorahosted.org/freeipa/ticket/4305 
> me
> and Petr Spaced (CC'ed) came to a conclusion that the IPA realm
> autodiscovery code used by ipa-client-install is so convoluted, 
> complex
> and hard to understand that the cost of fixing a bug/adding a 
> behavioral
> change (there are some other tickets dealing with ipadiscovery, e.g. 
> see
> https://fedorahosted.org/freeipa/ticket/5270
> https://fedorahosted.org/freeipa/ticket/3912 ) can potentially be 
> higher
> that a more large-scale rewrite of the module and related codebase.
>
> Since we plan to do some general refactoring work anyway, I would like
> to discuss the possible course of action we may take to tackle this
> issue. I would like to present the following options we have been
> discussing so far:
>
> 1.) Do a substantial rewrite of existing code
> ("ipaclient/ipadiscovery.py")
>
> We may take the existing IPADiscovery class and try rewrite it in 
> order
> to get a more concise and deterministic code, which will:
>
> * rely more on python-dns and answers provided by resolver (e.g. we 
> are
> directly parsing resolv.conf for available domains when we can ask the
> resolver to get domains for us)
> * be more pythonic (replace error codes with thrown exceptions, clean 
> up
> numerous C-isms etc.)
> * not try to outsmart user when server/realm/domain is specified
> beforehand. Currently, even if you specify all three options, there is
> still some DNS discovery performed, hence bug #4305. I think that one
> you specify server(s), not magic should be performed and the discovery
> process should be reduced to checking whether they are IPA servers and
> pull all other info (like realm) from them.
>
> This may be a considerable effort requiring some time to implement and
> get right, but IMHO still comparable to the time spent iteratively
> placing 'if' statements into the existing code and hoping to not break
> anything. I would even argue it is not worth the effort because we can
>
> 2.) Use realmd dbus interface to do DNS discovery
>
> I have attached aquick and dirty script I have slapped together to
> leverage 'org.freedesktop.realmd.Discover' interface to do IPA realm
> discovery for us. This has a lot of appeal to me since we are 
> leveraging
> existing codebase for DNS discovery and will have to handle only cases
> when the user manually specifies a list of IPA servers for the client.
>
> This however pulls in realmd as a (potentially circular) dependency 
> for
> ipa client, and when we find bug in the discovery code, we will have 
> to
> poke upstream (Stef or Sumit I think) to fix it.
>
> So from the long-term point of view, Jan Cholasta's (CC'ed)
> suggestion to:
>
> 3.) Split out IPA discovery portion of realmd to a separate C library
> shared between IPA and realmd
>
> may be best. Both projects will have shared codebase (maintained 
> either
> by us or realmd devs), which can be reused also by other people to
> create their own discovery/enrollment solution. This would however
> require sustained and concerted efforts of both teams to create the
> library and possibly rewrite realmd to accommodate this change.
>
> There may be some other options viable for us, if so please mention 
> them
> in a reply. Also please correct any piece of information I got wrong.
>
> TL;DR: IPA realm/domain discovery is a mess, please suggest a way to 
> fix
> it.
>

 #3 sounds good from long term perspective.

 In short term, i.e., #4305, we should skip discovery when --on-master 
 is
 used.
>>>
>>> I would prefer #2. Because as seen from the patch it is already working
>>> and can easily be used from python. I think this was also one of the
>>> reasons why Stef used a DBus 

[Freeipa-devel] [PATCH 0400] l10n: Remove Transifex configuration

[Tomas Babej]

Hi,

We're not using Transifex to manage our translations anymore.

Tomas
From 89b2da7d936b6c8aad115e05375c4dcdf8af11c5 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Wed, 20 Jan 2016 19:44:25 +0100
Subject: [PATCH] l10n: Remove Transifex configuration

We're not using Transifex to manage our translations anymore.
---
 .tx/config | 8 
 1 file changed, 8 deletions(-)
 delete mode 100644 .tx/config

diff --git a/.tx/config b/.tx/config
deleted file mode 100644
index 75461a9d46209b55616008a24d2c4e22b958f678..
--- a/.tx/config
+++ /dev/null
@@ -1,8 +0,0 @@
-[main]
-host = https://www.transifex.com
-
-[freeipa.ipa]
-file_filter = install/po/.po
-source_file = install/po/ipa.pot
-source_lang = en
-
-- 
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 0134] CI tests: use old schema when testing hostmask-based sudo rules

[Tomas Babej]

On 02/18/2016 10:32 AM, Martin Babinsky wrote:
> https://fedorahosted.org/freeipa/ticket/5625

ACK, works fine for me.

Thanks for the patch.

Pushed to master: 94a836dd46e5e041443b7da03e4ce8a7a7aaa7e3
Pushed to ipa-4-2: 61475631f64206d771e3fd243220be242f4bdd38

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 0423] fix duplicated except

[Martin Basti]

On 25.02.2016 12:03, David Kupka wrote:

On 25/02/16 11:40, Jan Cholasta wrote:

On 25.2.2016 11:25, David Kupka wrote:

On 24/02/16 17:56, Martin Basti wrote:

During my playing with pylint, I fixed this issue which allows us to
enable additional check in pylint (the nice one).

Patch attached, it should go only to master.



Works for me, ACK.

I always wonder how something like this can even get to the sources.
Fortunately the added check will prevent that in the future.


Before this is pushed, could you please check git history to verify that
these duplicate excepts are not symptomps of some actual problems?



Archaeology hat on.

The duplicate except statement in ipalib/plugins/automount.py was 
introduced by commit 0197ebbb and was there since 2010. All the time 
the second except was logically dead code.


The duplicate except statement in ipalib/backend.py was introduced by 
commit 01da4a8d converting StandardError to Exception. But the only 
difference is the message in raise error.


I should be save to push this.



Pushed to master: 0d39abddc27b0c348131dc3d0f3c5c538cd5f0b5

--
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] 0017 configure DNA shared config entry to allow connection with GSSAPI

[Martin Babinsky]

On 02/25/2016 12:17 PM, thierry bordaz wrote:

On 02/25/2016 12:03 PM, Martin Babinsky wrote:

On 02/24/2016 04:30 PM, thierry bordaz wrote:

On 01/21/2016 05:04 PM, Martin Babinsky wrote:

On 01/21/2016 01:37 PM, thierry bordaz wrote:





Hi Thierry,

I have couple of comments to your patch:

1.)
there is a number of PEP8 errors in the patch
(http://paste.fedoraproject.org/313246/33893701), please fix them.

See http://www.freeipa.org/page/Python_Coding_Style for our
conventions used in Python code.

2.)
+DNA_BIND_METHOD   = "dnaRemoteBindMethod"
+DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
+DNA_PLUGIN_DN = 'cn=Distributed Numeric Assignment
Plugin,cn=plugins,cn=config'
+dna_config_base   = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN

Uppercase names are usually reserved for module-level constants. OTOH,
local variables should be lowercase. Also you can instantiate
dna_config_base directly as DN, using 2-member tuples, i. e:

"""
dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed Numeric
Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
"""

When passing DN object to the formatting functions/operators, it is
automatically converted to string so no need to hold string and DN
object separately. This is done in other places (see function/methods
in replication.py).

3.)

+for i in range(len(entries)) :
+
+mod = []
+if entries[i].single_value.get(DNA_BIND_METHOD) != method:
+mod.append((ldap.MOD_REPLACE, DNA_BIND_METHOD,
method))
+
+if entries[i].single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+mod.append((ldap.MOD_REPLACE, DNA_CONN_PROTOCOL,
protocol))


please use idiomatic Python when handling list of entries, e.g.:

"""
for entry in entries:
   mod = []
   if entry.single_value.get(DNA_BIND_METHOD) != method:
   ...
"""

4.) I think that this method should in DSInstance class since it deals
with directory server configuration. Service is a parent object of all
other service installers/configurators and should contain only methods
common to more children.

5.) Since the method is called from every installer, it could be
beneficial to call it in DSInstance.__common_post_setup() as a part of
Directory server installation. Is there any reason why this is not the
case?

6.)

+while attempt != MAX_WAIT:
+try:
+entries = conn.get_entries(sharedcfgdn,
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
+break
+except errors.NotFound:
+root_logger.debug("So far enable not find DNA shared
config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn,
sharedcfgdn))
+attempt = attempt + 1
+time.sleep(2)
+continue
+
+# safety checking
+# there is no return, if there are several entries, as a
workaround of #5510
+if len(entries) != 1:

I am quite afraid what would happen if the server does not return any
entries until 30 s timeout. The code will then continue to the
condition which can potentially test an uninitialized variable and
blow up with 'NameError'. This should be handled more robustly, e. g.
raise an exception when a timeout is reached and no entries were
returned.

7.)

+if len(mod) > 0:

A Pythonic way to test for non-empty container is

"""
if mods:
   # do stuff
"""

since an empty list/dict/set evaluates to False and non-empty
containers are True.


8.)

+entry = conn.get_entry(entries[i].dn)
+if entry.single_value.get(DNA_BIND_METHOD) != method:
+root_logger.error("Fail to set SASL/GSSAPI bind
method to %s" % (entries[i].dn))
+if entry.single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+root_logger.error("Fail to set LDAP protocol to
%s" % (entries[i].dn))

rather than re-fetching the modified entry and testing what happened,
you can just catch an exception raised by unsuccessfull mod and log an
error like this:

"""
try:
   conn.modify_s(entry.dn, mod)
except Exception as e:
   root_logger.error("Failed to modify entry {}: {}".format(entry, e))
"""

as a matter of fact, if the modify_s operation would fail for some
reason, an ldap exception would be raised and you checks would not
even be executed.

9.)
The debug message on line 219 should read "Unable to find DNA shared
config entry for dnaHostname=%s so far. Retry in 2 sec.". The errors
at the end of the method should have "Failed" instead of "Fail".


Hi Martin,

Finally tested... here is the updated patch. Thanks for you patience


thanks
thierry


Hi Thierry,

the patch works as expected.


8-)

I have some more nitpicks though:

1.) Please fix the following pep8 errors:

http://paste.fedoraproject.org/329086/56397841/

you can check whether you recent commit conforms to PEP8 by running

"git show -U0 | pep8 --diff"


I did it and fixed most of them but what 

Re: [Freeipa-devel] [PATCH 0424] Pylint: add missing attributes of exceptions to definition in pylint plugin

[Martin Basti]

On 25.02.2016 11:29, David Kupka wrote:

On 24/02/16 18:54, Martin Basti wrote:

Pylint is not able to handle IPA errors objects, because attributes are
added into objects dynamically, and pylint 1.5 reports them as no-member
errors.

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

Patch attached.




It would be better to define all errors with all unrecoverable members 
but this is sufficient start. More can be add when it's needed.


Works for me, ACK.


Pushed to:
master: 5c33edcd11c466df59dbd13aac5e1b42ffa6fbb7
ipa-4-3: a7fc12b3abc1d6e1bd8d817f6358455202e17fad
ipa-4-2: a27f7dfa483fa1f088b46544f323fe3801a08b56

--
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 0011] Move freeipa certmonger helpers to libexecdir.

[David Kupka]

On 24/02/16 15:07, Rob Crittenden wrote:

David Kupka wrote:

On 23/02/16 16:41, Rob Crittenden wrote:

David Kupka wrote:

On 23/02/16 10:14, Martin Kosek wrote:

On 02/23/2016 09:47 AM, David Kupka wrote:

On 22/02/16 16:15, Martin Kosek wrote:

On 02/22/2016 04:04 PM, Jan Cholasta wrote:

On 22.2.2016 15:56, David Kupka wrote:

On 22/02/16 07:28, Jan Cholasta wrote:

On 18.2.2016 10:10, David Kupka wrote:

On 19/01/16 16:10, David Kupka wrote:

On 19/01/16 14:38, Jan Cholasta wrote:

On 19.1.2016 14:26, Martin Kosek wrote:

On 01/19/2016 01:47 PM, David Kupka wrote:

I've polished the patch attached to #5586 by Timo Aaltonen.

Thanks for the patch. I've fixed the path in specfile and
removed
unused import
but otherwise it works, ACK.

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


Won't this break existing certmonger requests depending on
the old
path?


It will, I don't see any upgrade code.



# getcert list | grep '/usr/lib64/ipa/certmonger'
pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
post-save command:
/usr/lib64/ipa/certmonger/renew_ca_cert
"auditSigningCert
cert-pki-ca"
pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
post-save command:
/usr/lib64/ipa/certmonger/renew_ca_cert
"ocspSigningCert
cert-pki-ca"
pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
post-save command:
/usr/lib64/ipa/certmonger/renew_ca_cert
"subsystemCert
cert-pki-ca"
pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
post-save command:
/usr/lib64/ipa/certmonger/renew_ca_cert
"caSigningCert
cert-pki-ca"
post-save command:
/usr/lib64/ipa/certmonger/renew_ra_cert
pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
post-save command:
/usr/lib64/ipa/certmonger/renew_ca_cert
"Server-Cert
cert-pki-ca"
post-save command:
/usr/lib64/ipa/certmonger/restart_dirsrv
RHEL72
post-save command:
/usr/lib64/ipa/certmonger/restart_httpd






You're right it will break the upgrade. I haven't noticed that
Server-Cert for DS and HTTPD are not handled by
certificate_renewal_update (ipaserver.install.server.upgrade)
where all
the other trackings are stopped and then configured again
with the
paths.CERTMONGER_COMMAND_TEMPLATE already updated.

Thanks for the catch.



I've updated Timo's patch little more and added
start_tracking_certificates() for dsinstance and httpinstance.
Now the
upgrade works as expected.


The way the patches are split is kind of weird and apparently
confusing
(see the other thread). IMO there should be 2 patches: the first
should
add the ability to change DS and HTTP certmonger config during
upgrade
(i.e. the start_tracking_certificates() methods and
certificate_renewal_update() changes), the second should move the
helpers (i.e. the actual move and certificate_renewal_update()
version
bump).


Honza, do I understand it correctly that the code is OK but I
did not
split it to the patches correctly?


Yes.


Before acking or pushing, can you please explain for me how the
upgrade of
certmonger tracking requests work? I want to make sure this is
right, so please
bear with me:

1) How does it edit existing tracking requests with the new helper
paths?

2) Does it go and try to edit the requests on every upgrade? Or is
there some
check that requests were updated?

Thanks,
Martin



Whole upgrade of renewal requests is done in
ipaserver/install/server/upgrade.py in certificate_renewal_upgrade().

First there is version of requests and if it's the same as in state
file
upgrade is skipped.
Then every request is searched over certmonger's DBus interface and
if at least
one is not found it means that there was change in request
configuration. All
tracking requests are then stopped and started again with new
configuration.

So to answer you questions:
1) By stopping the old request with the old parameters (including
path) and
starting new with new parameters.

2) Only if version was bumped which happens only if some of the
requests changes.


Ah, so IIUC, if you bump the version, requests should be properly
updated. The
change looks fine then.



After discussion with Honza, we decided to drop the part comparing only
base names of pre- and post-save commands and use it as whole. I've also
split the patches so it's obvious what is going on.

Patches should be applied in this order:

freeipa-dkupka-0091.0


A cert could silently fail to be tracked in
start_tracking_certificates() if no serverid can be found.


In that case it also wouldn't be stopped. The behavior is the same as in
existing stop_tracking_certificates(). Should we rather raise and stop
the upgrade? I guess not but warning would be probably useful. What
solution would you prefer, Rob?


I don't know all the callers of this. It may be perfectly safe to assume
that a serverid is always there, but the implication if it isn't is that
some tracking cert won't be updated properly right? That potentially
could mean no renewal.

So the 

[Freeipa-devel] [PATCH 0401] ipa-adtrust-install: Allow dash in the NETBIOS name

[Tomas Babej]

Hi,

Dash should be one of the allowed characters in the netbios names,
so relax the too strict validation.

Note: the set of allowed characters might expand in the future

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

Tomas
From eab57f7d15758bd998d944b33f338a35a57de218 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Thu, 25 Feb 2016 14:02:08 +0100
Subject: [PATCH] ipa-adtrust-install: Allow dash in the NETBIOS name

Dash should be one of the allowed characters in the netbios names,
so relax the too strict validation.

Note: the set of allowed characters might expand in the future

https://fedorahosted.org/freeipa/ticket/5286
---
 install/tools/ipa-adtrust-install| 19 ---
 ipaserver/install/adtrustinstance.py | 15 +--
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index d2cec17891976e911d42869d5c871bf4f2805db7..ada11e076f7dec6f47afc02dc178a5306fb53daf 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -74,17 +74,22 @@ def parse_options():
 return safe_options, options
 
 def netbios_name_error(name):
-print "\nIllegal NetBIOS name [%s].\n" % name
-print "Up to 15 characters and only uppercase ASCII letter and digits are allowed."
+print(
+"\nIllegal NetBIOS name [{}].\n\n"
+"Up to 15 characters and only uppercase ASCII letters, "
+"digits and dashes are allowed.".format(name)
+)
 
 def read_netbios_name(netbios_default):
 netbios_name = ""
 
-print "Enter the NetBIOS name for the IPA domain."
-print "Only up to 15 uppercase ASCII letters and digits are allowed."
-print "Example: EXAMPLE."
-print ""
-print ""
+print(
+"Enter the NetBIOS name for the IPA domain.\n"
+"Only up to 15 uppercase ASCII letters, digits and "
+"dashes are allowed.\n"
+"Example: EXAMPLE\n\n"
+)
+
 if not netbios_default:
 netbios_default = "EXAMPLE"
 while True:
diff --git a/ipaserver/install/adtrustinstance.py b/ipaserver/install/adtrustinstance.py
index a890f141b5ea5d79511cbd7eb3d24c73cf04f3b5..9fa1c37b6e5a23cf2f6da3dc084e1c3f050ac721 100644
--- a/ipaserver/install/adtrustinstance.py
+++ b/ipaserver/install/adtrustinstance.py
@@ -44,7 +44,7 @@ from ipaplatform.paths import paths
 from ipaplatform.tasks import tasks
 
 
-ALLOWED_NETBIOS_CHARS = string.ascii_uppercase + string.digits
+ALLOWED_NETBIOS_CHARS = string.ascii_uppercase + string.digits + '-'
 
 UPGRADE_ERROR = """
 Entry %(dn)s does not exist.
@@ -83,13 +83,16 @@ def ipa_smb_conf_exists():
 return False
 
 
-def check_netbios_name(s):
+def check_netbios_name(name):
 # NetBIOS names may not be longer than 15 allowed characters
-if not s or len(s) > 15 or \
-   ''.join([c for c in s if c not in ALLOWED_NETBIOS_CHARS]):
-return False
+invalid_netbios_name = any([
+not name,
+len(name) > 15,
+''.join([c for c in name if c not in ALLOWED_NETBIOS_CHARS])
+])
+
+return not invalid_netbios_name
 
-return True
 
 def make_netbios_name(s):
 return ''.join([c for c in s.split('.')[0].upper() \
-- 
2.5.0

From 853e3e5759a12214a72c27c7d76ab3fe9ed8df9a Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Thu, 25 Feb 2016 14:09:48 +0100
Subject: [PATCH] ipa-adtrust-install: Allow dash in the NETBIOS name

Dash should be one of the allowed characters in the netbios names,
so relax the too strict validation.

Note: the set of allowed characters might expand in the future

https://fedorahosted.org/freeipa/ticket/5286
---
 install/tools/ipa-adtrust-install|  6 --
 ipaserver/install/adtrustinstance.py | 15 +--
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index 8b3fce2db8535ac2124a347c6ca2f63f7c6e3e42..f972835ec89c18d0453e659142d4434aaa571dd5 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -88,13 +88,15 @@ def parse_options():
 
 def netbios_name_error(name):
 print("\nIllegal NetBIOS name [%s].\n" % name)
-print("Up to 15 characters and only uppercase ASCII letter and digits are allowed.")
+print("Up to 15 characters and only uppercase ASCII letters, digits "
+  "and dashes are allowed.")
 
 def read_netbios_name(netbios_default):
 netbios_name = ""
 
 print("Enter the NetBIOS name for the IPA domain.")
-print("Only up to 15 uppercase ASCII letters and digits are allowed.")
+print("Only up to 15 uppercase ASCII letters, digits "
+  "and dashes are allowed.")
 print("Example: EXAMPLE.")
 print("")
 print("")
diff --git a/ipaserver/install/adtrustinstance.py b/ipaserver/install/adtrustinstance.py
index 9e7e001f7c505d09d5a61164399e9ed256ae9865..54a23bc15d818864f0987b02294244414ed9883f 100644
--- 

Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI

[thierry bordaz]

On 02/25/2016 01:56 PM, Martin Babinsky wrote:

On 02/25/2016 12:17 PM, thierry bordaz wrote:

On 02/25/2016 12:03 PM, Martin Babinsky wrote:

On 02/24/2016 04:30 PM, thierry bordaz wrote:

On 01/21/2016 05:04 PM, Martin Babinsky wrote:

On 01/21/2016 01:37 PM, thierry bordaz wrote:





Hi Thierry,

I have couple of comments to your patch:

1.)
there is a number of PEP8 errors in the patch
(http://paste.fedoraproject.org/313246/33893701), please fix them.

See http://www.freeipa.org/page/Python_Coding_Style for our
conventions used in Python code.

2.)
+DNA_BIND_METHOD   = "dnaRemoteBindMethod"
+DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
+DNA_PLUGIN_DN = 'cn=Distributed Numeric Assignment
Plugin,cn=plugins,cn=config'
+dna_config_base   = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN

Uppercase names are usually reserved for module-level constants. 
OTOH,

local variables should be lowercase. Also you can instantiate
dna_config_base directly as DN, using 2-member tuples, i. e:

"""
dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed Numeric
Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
"""

When passing DN object to the formatting functions/operators, it is
automatically converted to string so no need to hold string and DN
object separately. This is done in other places (see function/methods
in replication.py).

3.)

+for i in range(len(entries)) :
+
+mod = []
+if entries[i].single_value.get(DNA_BIND_METHOD) != 
method:

+mod.append((ldap.MOD_REPLACE, DNA_BIND_METHOD,
method))
+
+if entries[i].single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+mod.append((ldap.MOD_REPLACE, DNA_CONN_PROTOCOL,
protocol))


please use idiomatic Python when handling list of entries, e.g.:

"""
for entry in entries:
   mod = []
   if entry.single_value.get(DNA_BIND_METHOD) != method:
   ...
"""

4.) I think that this method should in DSInstance class since it 
deals
with directory server configuration. Service is a parent object of 
all
other service installers/configurators and should contain only 
methods

common to more children.

5.) Since the method is called from every installer, it could be
beneficial to call it in DSInstance.__common_post_setup() as a 
part of
Directory server installation. Is there any reason why this is not 
the

case?

6.)

+while attempt != MAX_WAIT:
+try:
+entries = conn.get_entries(sharedcfgdn,
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
+break
+except errors.NotFound:
+root_logger.debug("So far enable not find DNA shared
config entry for dnaHostname=%s under %s. Retry in 2sec" % 
(self.fqdn,

sharedcfgdn))
+attempt = attempt + 1
+time.sleep(2)
+continue
+
+# safety checking
+# there is no return, if there are several entries, as a
workaround of #5510
+if len(entries) != 1:

I am quite afraid what would happen if the server does not return any
entries until 30 s timeout. The code will then continue to the
condition which can potentially test an uninitialized variable and
blow up with 'NameError'. This should be handled more robustly, e. g.
raise an exception when a timeout is reached and no entries were
returned.

7.)

+if len(mod) > 0:

A Pythonic way to test for non-empty container is

"""
if mods:
   # do stuff
"""

since an empty list/dict/set evaluates to False and non-empty
containers are True.


8.)

+entry = conn.get_entry(entries[i].dn)
+if entry.single_value.get(DNA_BIND_METHOD) != 
method:

+root_logger.error("Fail to set SASL/GSSAPI bind
method to %s" % (entries[i].dn))
+if entry.single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+root_logger.error("Fail to set LDAP protocol to
%s" % (entries[i].dn))

rather than re-fetching the modified entry and testing what happened,
you can just catch an exception raised by unsuccessfull mod and 
log an

error like this:

"""
try:
   conn.modify_s(entry.dn, mod)
except Exception as e:
   root_logger.error("Failed to modify entry {}: {}".format(entry, 
e))

"""

as a matter of fact, if the modify_s operation would fail for some
reason, an ldap exception would be raised and you checks would not
even be executed.

9.)
The debug message on line 219 should read "Unable to find DNA shared
config entry for dnaHostname=%s so far. Retry in 2 sec.". The errors
at the end of the method should have "Failed" instead of "Fail".


Hi Martin,

Finally tested... here is the updated patch. Thanks for you patience


thanks
thierry


Hi Thierry,

the patch works as expected.


8-)

I have some more nitpicks though:

1.) Please fix the following pep8 errors:

http://paste.fedoraproject.org/329086/56397841/

you can check whether you recent commit conforms to PEP8 by running

"git 

Re: [Freeipa-devel] [PATCH 0001] Add new parameter --ssh-update to ipa-client-install

[Martin Basti]

On 22.02.2016 22:13, Martin Štefany wrote:

Hi,

please, review the attached patch which adds --ssh-update to ipa-client-
install.

Ticket: https://fedorahosted.org/freeipa/ticket/2655

Hello,
thank you for your patch.
Please attach a patch as a file next time.

I have doubts that this should be part of ipa-client-install, this needs 
a broader discussion.


Code comments inline:


---
Martin

 From 4974a57f48a0cd48b83724297ae2af572bc530eb Mon Sep 17 00:00:00 2001
From: Martin Stefany 
Date: Mon, 22 Feb 2016 20:58:13 +
Subject: [PATCH] Add new parameter --ssh-update to ipa-client-install

Add a new parameter '--ssh-update' which can be used later after freeipa
client is installed to update SSH hostkeys and SSHFP DNS records for
host.

https://fedorahosted.org/freeipa/ticket/2655
---
  ipa-client/ipa-install/ipa-client-install | 102
+-
  1 file changed, 99 insertions(+), 3 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-
install/ipa-client-install
index
789ff591591673744ee3b922e5c0181233ad553c..97adfb6b449fb441bddada89a3b151
33e398ca50 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -71,6 +71,7 @@ CLIENT_INSTALL_ERROR = 1
  CLIENT_NOT_CONFIGURED = 2
  CLIENT_ALREADY_CONFIGURED = 3
  CLIENT_UNINSTALL_ERROR = 4 # error after restoring files/state
+CLIENT_SSHUPDATE_ERROR = 5 # error during update of SSH public keys
  
  def parse_options():

  def validate_ca_cert_file_option(option, opt, value, parser):
@@ -215,6 +216,12 @@ def parse_options():
"be run with --unattended
option")
  parser.add_option_group(uninstall_group)
  
+sshupdate_group = OptionGroup(parser, "SSH key update options")

+sshupdate_group.add_option("--ssh-update", dest="ssh_update",
+  action="store_true", default=False,
+  help="update local host's SSH public keys in host
entry and DNS.")
+parser.add_option_group(sshupdate_group)
+
  options, args = parser.parse_args()
  safe_opts = parser.get_safe_opts(options)
  
@@ -840,6 +847,92 @@ def uninstall(options, env):
  
  return rv
  
+def sshupdate(options, env):

+if not is_ipa_client_installed():
+root_logger.error("IPA client is not configured on this
system.")
+return CLIENT_NOT_CONFIGURED
+
+api.bootstrap(context='cli_installer', debug=options.debug)
+api.finalize()
+if 'config_loaded' not in api.env:
+root_logger.error("Failed to initialize IPA API.")
+return CLIENT_SSHUPDATE_ERROR
+
+# Now, let's try to connect to the server's RPC interface
+connected = False
+try:
+api.Backend.rpcclient.connect()
+connected = True
+root_logger.debug("Try RPC connection")
+api.Backend.rpcclient.forward('ping')
+except errors.KerberosError as e:
+if connected:
+api.Backend.rpcclient.disconnect()
+root_logger.info(
+"Cannot connect to the server due to Kerberos error: %s. "
+"Trying with delegate=True", e)
+try:
+api.Backend.rpcclient.connect(delegate=True)
+root_logger.debug("Try RPC connection")
+api.Backend.rpcclient.forward('ping')
+
+root_logger.info("Connection with delegate=True
successful")
+
+# The remote server is not capable of Kerberos S4U2Proxy
+# delegation. This features is implemented in IPA server
+# version 2.2 and higher
+root_logger.warning(
+"Target IPA server has a lower version than the
enrolled "
+"client")
+root_logger.warning(
+"Some capabilities including the ipa command capability
"
+"may not be available")
+except errors.PublicError as e2:
+root_logger.warning(
+"Second connect with delegate=True also failed: %s",
e2)
+root_logger.error(
+"Cannot connect to the IPA server RPC interface: %s",
e2)
+return CLIENT_SSHUPDATE_ERROR
+except errors.PublicError as e:
+root_logger.error(
+"Cannot connect to the server due to generic error: %s", e)
+return CLIENT_SSHUPDATE_ERROR
I think you should be kinited with client keytab, client is allowed to 
modify its SSHpublic keys in ldap. I'd only require to be root to 
execute it.


kinit -kt /etc/krb5.keytab host/`hostname`
ipa host-mod `hostname` --sshpubkey="something"

Also this rpcconnection looks to me too much complicated, I think it 
should be just simple rpcconnect



+
+# We need to pull IPA server address from default.conf
+try:
+parser = RawConfigParser()
+parser.read(paths.IPA_DEFAULT_CONF)
+cli_realm = parser.get('global', 'realm')
+hostname = parser.get('global', 'host')
+# TODO: consult with review team
+# 

Re: [Freeipa-devel] [PATCH 0001] Add new parameter --ssh-update to ipa-client-install

[Jan Cholasta]

Hi,

On 25.2.2016 14:23, Martin Basti wrote:



On 22.02.2016 22:13, Martin Štefany wrote:

Hi,

please, review the attached patch which adds --ssh-update to ipa-client-
install.

Ticket:https://fedorahosted.org/freeipa/ticket/2655

Hello,
thank you for your patch.
Please attach a patch as a file next time.

I have doubts that this should be part of ipa-client-install, this needs
a broader discussion.


+1, I think it should be a separate command (ignore my earlier 
suggestion from Trac to incorporate this into ipa-client-install, I was 
young and stupid).


See client/ipa-certupdate and ipaclient/ipa_certupdate.py for an example 
of how such a command should be implemented.




Code comments inline:


---
Martin

>From 4974a57f48a0cd48b83724297ae2af572bc530eb Mon Sep 17 00:00:00 2001
From: Martin Stefany 
Date: Mon, 22 Feb 2016 20:58:13 +
Subject: [PATCH] Add new parameter --ssh-update to ipa-client-install

Add a new parameter '--ssh-update' which can be used later after freeipa
client is installed to update SSH hostkeys and SSHFP DNS records for
host.

https://fedorahosted.org/freeipa/ticket/2655
---
  ipa-client/ipa-install/ipa-client-install | 102
+-
  1 file changed, 99 insertions(+), 3 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-
install/ipa-client-install
index
789ff591591673744ee3b922e5c0181233ad553c..97adfb6b449fb441bddada89a3b151
33e398ca50 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -71,6 +71,7 @@ CLIENT_INSTALL_ERROR = 1
  CLIENT_NOT_CONFIGURED = 2
  CLIENT_ALREADY_CONFIGURED = 3
  CLIENT_UNINSTALL_ERROR = 4 # error after restoring files/state
+CLIENT_SSHUPDATE_ERROR = 5 # error during update of SSH public keys

  def parse_options():
  def validate_ca_cert_file_option(option, opt, value, parser):
@@ -215,6 +216,12 @@ def parse_options():
"be run with --unattended
option")
  parser.add_option_group(uninstall_group)

+sshupdate_group = OptionGroup(parser, "SSH key update options")
+sshupdate_group.add_option("--ssh-update", dest="ssh_update",
+  action="store_true", default=False,
+  help="update local host's SSH public keys in host
entry and DNS.")
+parser.add_option_group(sshupdate_group)
+
  options, args = parser.parse_args()
  safe_opts = parser.get_safe_opts(options)

@@ -840,6 +847,92 @@ def uninstall(options, env):

  return rv

+def sshupdate(options, env):
+if not is_ipa_client_installed():
+root_logger.error("IPA client is not configured on this
system.")
+return CLIENT_NOT_CONFIGURED
+
+api.bootstrap(context='cli_installer', debug=options.debug)
+api.finalize()
+if 'config_loaded' not in api.env:
+root_logger.error("Failed to initialize IPA API.")
+return CLIENT_SSHUPDATE_ERROR
+
+# Now, let's try to connect to the server's RPC interface
+connected = False
+try:
+api.Backend.rpcclient.connect()
+connected = True
+root_logger.debug("Try RPC connection")
+api.Backend.rpcclient.forward('ping')
+except errors.KerberosError as e:
+if connected:
+api.Backend.rpcclient.disconnect()
+root_logger.info(
+"Cannot connect to the server due to Kerberos error: %s. "
+"Trying with delegate=True", e)
+try:
+api.Backend.rpcclient.connect(delegate=True)
+root_logger.debug("Try RPC connection")
+api.Backend.rpcclient.forward('ping')
+
+root_logger.info("Connection with delegate=True
successful")
+
+# The remote server is not capable of Kerberos S4U2Proxy
+# delegation. This features is implemented in IPA server
+# version 2.2 and higher
+root_logger.warning(
+"Target IPA server has a lower version than the
enrolled "
+"client")
+root_logger.warning(
+"Some capabilities including the ipa command capability
"
+"may not be available")
+except errors.PublicError as e2:
+root_logger.warning(
+"Second connect with delegate=True also failed: %s",
e2)
+root_logger.error(
+"Cannot connect to the IPA server RPC interface: %s",
e2)
+return CLIENT_SSHUPDATE_ERROR
+except errors.PublicError as e:
+root_logger.error(
+"Cannot connect to the server due to generic error: %s", e)
+return CLIENT_SSHUPDATE_ERROR

I think you should be kinited with client keytab, client is allowed to
modify its SSHpublic keys in ldap. I'd only require to be root to
execute it.

kinit -kt /etc/krb5.keytab host/`hostname`
ipa host-mod `hostname` --sshpubkey="something"

Also this rpcconnection looks to me too much complicated, I think it
should be just simple 

[Freeipa-devel] [TESTS][PATCH 0011] WebUI: Creating user without private group

[Lenka Doudova]

Hi,

here's a patch for webUI tests that provides test for creating user 
without private group.

Related to ticket https://fedorahosted.org/freeipa/ticket/4986

Since the option to specify GID when creating a user is not available 
https://fedorahosted.org/freeipa/ticket/5505 the test creates a new 
posix group, makes it a default user group instead of 'ipausers' and 
then attemps to create the user without private group. Returning default 
user group value to 'ipausers' is provided even for cases when the test 
fails so it would not block other tests from performing properly.


Lenka
From b9d0d7d5887e339de24d9878b733b45a0618bb9b Mon Sep 17 00:00:00 2001
From: Lenka Doudova 
Date: Thu, 25 Feb 2016 15:00:49 +0100
Subject: [PATCH] WebUI: Test creating user without private group

Test for option to create a user without private group in web UI.

Covers ticket https://fedorahosted.org/freeipa/ticket/4986
---
 ipatests/test_webui/data_user.py | 11 +++
 ipatests/test_webui/test_user.py | 31 +++
 2 files changed, 42 insertions(+)

diff --git a/ipatests/test_webui/data_user.py b/ipatests/test_webui/data_user.py
index 79a53898035a2723c1e79c459ba4aef3ffa67723..50945c1e95634654ca3a8f0dabd09e6d7e2d0452 100644
--- a/ipatests/test_webui/data_user.py
+++ b/ipatests/test_webui/data_user.py
@@ -63,3 +63,14 @@ DATA2 = {
 ('textbox', 'sn', 'OtherSurname2'),
 ],
 }
+
+PKEY3 = 'itest-user3'
+DATA3 = {
+'pkey': PKEY3,
+'add': [
+('textbox', 'uid', PKEY3),
+('textbox', 'givenname', 'Name3'),
+('textbox', 'sn', 'Surname3'),
+('checkbox', 'noprivate', None),
+]
+}
diff --git a/ipatests/test_webui/test_user.py b/ipatests/test_webui/test_user.py
index b216125b226230ab268de3f86ec7ac6f785c6e16..713df5dc929c0b4e1761cab5c1a1a1675cc00669 100644
--- a/ipatests/test_webui/test_user.py
+++ b/ipatests/test_webui/test_user.py
@@ -222,6 +222,37 @@ class test_user(UI_driver):
 self.delete(user.ENTITY, [user.DATA])
 self.delete(group.ENTITY, [group.DATA])
 
+@screenshot
+def test_noprivate(self):
+"""
+User without private group
+"""
+self.init_app()
+self.add_record(group.ENTITY, group.DATA2)
+
+try:
+self.set_ipadefaultprimarygroup(group.PKEY2)
+self.add_record(user.ENTITY, user.DATA3)
+self.delete(user.ENTITY, [user.DATA3])
+except:
+self.dialog_button_click('cancel')  # exit error dialog
+self.dialog_button_click('cancel')  # exit add user dialog
+raise
+finally:
+self.set_ipadefaultprimarygroup('ipausers')
+self.delete(group.ENTITY, [group.DATA2])
+
+def set_ipadefaultprimarygroup(self, group):
+"""
+Set ipa config "Default users group" field
+"""
+self.navigate_to_entity('config')
+self.mod_record('config', {
+'mod': [
+('combobox', 'ipadefaultprimarygroup', group),
+]
+})
+
 def set_ipapwdexpadvnotify(self, days):
 """
 Set ipa config "Password Expiration Notification (days)" field
-- 
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] Locations design v2: LDAP schema & user interface

[Petr Spacek]

On 24.2.2016 15:25, Simo Sorce wrote:
> On Wed, 2016-02-24 at 10:00 +0100, Martin Kosek wrote:
>> On 02/23/2016 06:59 PM, Petr Spacek wrote:
>>> On 23.2.2016 18:14, Simo Sorce wrote:
>> ...
 More seriously I think it is a great idea, but too premature to get all
 the way there now. We need to build schema and CLI that will allow us to
 get there without having to completely change interfaces if at all
 possible or minimizing any disruption in the tools.
>>>
>>> Actually the backwards compatibility is the main worry which led to this 
>>> idea
>>> with links.
>>>
>>> If we release first version of locations with custom priorities etc. we will
>>> have support the schema (which will be different) and API (which will be 
>>> later
>>> unnecessary) forever.
>>>
>>> If we skip this intermediate phase with hand-made configuration we can save
>>> all the headache with upgrades to more automatic solution later on.
>>>
>>>
>>> Maybe we should invert the order:
>>> Start with locations + links with administrative metric and add 
>>> hand-tweaking
>>> capabilities later (if necessary).
>>>
>>> IMHO locations + links with administrative metric will be easier to 
>>> implement
>>> than the first version.
>>>
>>> Just thinking aloud ...
>>
>> Makes sense to me, I would have the same worry as Petr, that we would break
>> something if we decide moving to links based solution later.
> 
> Maybe I am missing something, but in order to generate the proper SRV
> records we need priority and weights anyway, either by entering them
> manually or by autogenerating them from some other piece of information
> in the framework. So given this information is needed anyway why would
> it become a problem to retain it in the future if we enable a tool the
> simply autogenerates this information ?

Let me clarify this:
You are right, in the end we always somehow get to priorities and weights.

TL;DR version
=
The difference is subtle details how we get priorities and if we store them in
LDAP and represent them in API (or not). It will simplify things if we do not
expose them. I'm not convinced that we *need* to expose them in the first round.


TL version
==

In the high level the process is always as follows:
1. input tuples (location, server, weight) for all primary servers assigned to
locations
2. input or derive (location, server, priority) for all backups
3. generate SRV records using priority groups combined from the previous two 
steps

Now we are trying to decide if step (2) is "input" OR "derive" priorities for
backup servers.


Variants


Variant A
-
If we let the user to do everything manually (no links etc.) we need to
provide following schema + API + user interface:
[first step - same in both variants]
* create locations
* assign 'main' (aka 'primary' aka 'home') servers to locations
++ specify weights for the 'main' servers in given location, i.e. manually
input (server, weight) tuples

[second step]
* specify backup servers for each location
++ assign (server, priority, weight) information for each non-main server
++ for S servers and L locations we need to represent up to
   S * L tuples (server, priority, weight) and provide means to manage it
++ most importantly, maintenance complexity of backups grows any time you add
one of (server OR location)
++ this would be a nightmare to manage. For simple cases this require some
'include' mechanism to declare one location as backup for another location.
This include complicates things significantly as it has a lot of corner cases
and requires different LDAP schema when compared to direct servers assignment.



Variant B
-
If we let the user only specify locations + links with costs we need to
provide following schema + API + user interface:
[first step - no change from variant A]
* create locations
* assign 'main' (aka 'primary' aka 'home') servers to locations
++ specify weights for the 'main' servers in given location, i.e. manually
input (server, weight) tuples

[second step]
* create links between locations
++ manually assign point-to-point information + administrative cost
++ for S servers and L locations we need to represent up to
   L^2 tuples (from, to, cost) and provide means to manage it
++ storage can be optimized to great extent if there is a lot of links with
equal cost, typically a full-mesh interconnections can be represented by
single object in LDAP
* generate backups (i.e. priority assignment) using usual routing algorithms.
Priority does not need to be neither exposed to user nor stored in LDAP at all.
++ most importantly, maintenance complexity of backups grows while you add
locations *but* you do not need to manually go though backup configuration for
(potentially) all locations every time as you add/change/remove servers in
existing locations (which you have to do with variant A, unless you use some
smart includes ...).


Please note that variant B with (links, costs) do not use explicit priority

Re: [Freeipa-devel] Locations design v2: LDAP schema & user interface

[Simo Sorce]

On Thu, 2016-02-25 at 14:45 +0100, Petr Spacek wrote:
> Variant C
> -
> An alternative is to be lazy and dumb. Maybe it would be enough for
> the first
> round ...
> 
> We would retain
> [first step - no change from variant A]
> * create locations
> * assign 'main' (aka 'primary' aka 'home') servers to locations
> ++ specify weights for the 'main' servers in given location, i.e.
> manually
> input (server, weight) tuples
> 
> Then, backups would be auto-generated set of all remaining servers
> from all
> other locations.
> 
> Additional storage complexity: 0
> 
> This covers the scenario "always prefer local servers and use remote
> only as
> fallback" easily. It does not cover any other scenario.
> 
> This might be sufficient for the first run and would allow us to
> gather some
> feedback from the field.
> 
> Now I'm inclined to this variant :-)

To be honest, this is all I always had in mind, for the first step.

To recap:
- define a location with the list of servers (perhaps location is a
property of server objects so you can have only one location per server,
and if you remove the server it is automatically removed from the
location w/o additional work or referential integrity necessary), if
weight is not defined (default) then they all have the same weight.

- Allow to specify backup locations in the location object, priorities
are calculated automatically and all backup locations have same weight.

- Define a *default* location, which is the backup for any other
location but always with lower priority to any other explicitly defined
backup locations.

- Weights for backup location servers are the same as the weight defined
within the backup location itself, so no additional weights are defined
for backups.

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 0413] fix permission: Read Replication Agreements

[Jan Cholasta]

On 24.2.2016 15:43, Martin Basti wrote:



On 24.02.2016 13:36, Jan Cholasta wrote:

On 24.2.2016 13:07, Martin Basti wrote:



On 24.02.2016 10:45, Jan Cholasta wrote:

On 23.2.2016 17:20, Martin Basti wrote:



On 22.02.2016 09:00, Jan Cholasta wrote:

Hi,

On 17.2.2016 14:49, Martin Basti wrote:

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

Patch attached (for master, 4.3, 4.2)


1) All the replication agreement permission ACIs should be located in
the same entry. Currently "Read Replication Agreements" is in
"cn=config" and everything else in "cn=mapping tree,cn=config", so I
guess "cn=mapping tree,cn=config" makes more sense.


2) Instead of literal DN('cn=permissions,cn=pbac'), use
api.env.container_permissions.


3) IMO the removal of managed permission attributes could be a little
bit more robust. You should check that the original entry contains
all
the required values before touching it (objectclass=ipapermissionv2,
ipapermissiontype=V2, ipapermissiontype=MANAGED) and remove only the
values that need to be removed, instead of just overwriting
everything.


Honza


Updated patch attached.


The patch does not apply on ipa-4-2.


I will send it later.


Also this bit in replica-acis.ldif is redundant:

+
+dn: cn=mapping tree,cn=config
+changetype: modify
+add: aci

All related ACIs to replication are in both replica-acis.ldif and
20-aci.update.
I just do not want to mess it more than it is.


What I'm trying to say is that:

dn: cn=mapping tree,cn=config
changetype: modify
add: aci
aci: $ACI1

dn: cn=mapping tree,cn=config
changetype: modify
add: aci
aci: $ACI2

is the same as:

dn: cn=mapping tree,cn=config
changetype: modify
add: aci
aci: $ACI1
aci: $ACI2

. You actually have it right in 20-aci.update, but not in
replica-acis.ldif.


I made it in that way to keep consistency in the replica-acis.ldif file.


I see. I missed that.



Patch for 4-2 added


Thanks, ACK.

Pushed to:
master: bba2355631c4cbadfb5089663c2a3af65a817fb7
ipa-4-2: de7ec77ea8811a6add2eab5d0853686484ae732c
ipa-4-3: 2bac05a18720c4ab84bc1de5573d3d96e73ddc55

--
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 0033] spec file: update the python-polib dependency name to python2-polib

[Jan Cholasta]

On 25.2.2016 11:03, Milan Kubík wrote:

On 02/15/2016 05:39 PM, Lukas Slebodnik wrote:

On (15/02/16 17:00), Petr Vobornik wrote:

On 02/15/2016 04:37 PM, Milan Kubík wrote:

Reflect the updated name of the package.


Seems to me as a packaging bug in python-polib. It should use
python_provide
macro to handle the transition.

There is not a bug in python-polib

sh# rpm -q python2-polib
python2-polib-1.0.7-2.fc23.noarch

sh# rpm -q --provides python2-polib
python-polib = 1.0.7-2.fc23
python2-polib = 1.0.7-2.fc23

However it is a change in behaviour in dnf/yum.
You can see more details in BZ1291850 or better BZ1096506.

This a readon why "dnf builddep" will try to remove package.
(it's not downgrade from dnf point of view)

sh# dnf builddep freeipa.spec
Last metadata expiration check performed 0:17:37 ago on Mon Feb 15
16:19:14
2016.
Package python-setuptools-18.0.1-2.fc23.noarch is already installed,
skipping.
Package systemd-222-10.fc23.x86_64 is already installed, skipping.
Package systemd-222-10.fc23.x86_64 is already installed, skipping.
Error: installed package python2-polib-1.0.7-2.fc23.noarch obsoletes
python-polib < 1.0.7-2.fc23 provided by python-polib-1.0.3-6.fc23.noarch
(try to add '--allowerasing' to command line to replace conflicting
packages)


You might try to file a dnf BZ but mine 1291850 was two tiles closed
as not a
but and then closed as a duplicate of another bug.

IMHO the simplest solution would to push the patch with better
explanation
in's a workaround.

LSommit message becuase it's a workaround.

LS

Updated patch with reworded commit message.


Please also add "workaround for 
https://bugzilla.redhat.com/show_bug.cgi?id=1096506; comment above the 
changed requires.


--
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 0423] fix duplicated except

[David Kupka]

On 24/02/16 17:56, Martin Basti wrote:

During my playing with pylint, I fixed this issue which allows us to
enable additional check in pylint (the nice one).

Patch attached, it should go only to master.



Works for me, ACK.

I always wonder how something like this can even get to the sources. 
Fortunately the added check will prevent that in the future.


--
David Kupka

--
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] [FREEIPA INSTALL ISSUE] Recent Tomcat build from F23 updates-testing breaks Dogtag installation

[Martin Babinsky]

On 02/25/2016 10:18 AM, Martin Babinsky wrote:

Hello everyone,

please note that package tomcat-8.0.32-3.fc23.noarch [1] messes with
symlinks to Catalina classes used by Dogtag. This makes CA deployment
blow up spectacularly during FreeIPA server/replica/etc installation. A
bugzilla exists[2] for this issue and also mentions a workaround which
makes the symlinks point to correct files.

An alternative is to downgrade tomcat to version 8.0.32-2.fc23 [2] or
older. That should make Dogtag work again.

If you already encountered this issue give negative karma to the package
at Bodhi[2].

[1] https://bodhi.fedoraproject.org/updates/FEDORA-2016-5e0bb2f21a
[2] http://koji.fedoraproject.org/koji/buildinfo?buildID=737537



Actually you need to downgrade to tomcat-8.0.26-2.fc23 from 'updates' 
repo to make things work again.


--
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] IPA client realm/domain autodiscovery improvements

[Jan Cholasta]

On 25.2.2016 10:35, Petr Spacek wrote:

On 24.2.2016 16:30, Sumit Bose wrote:

On Wed, Feb 24, 2016 at 04:08:14PM +0100, David Kupka wrote:

On 24/02/16 15:55, Sumit Bose wrote:

On Wed, Feb 24, 2016 at 03:30:40PM +0100, Martin Babinsky wrote:

On 02/24/2016 03:20 PM, Sumit Bose wrote:

On Wed, Feb 24, 2016 at 01:31:55PM +0100, Petr Vobornik wrote:

On 02/16/2016 02:23 PM, Martin Babinsky wrote:

Hi list,

WARNING: huge brain dump ahead.

During investigation of https://fedorahosted.org/freeipa/ticket/4305 me
and Petr Spaced (CC'ed) came to a conclusion that the IPA realm
autodiscovery code used by ipa-client-install is so convoluted, complex
and hard to understand that the cost of fixing a bug/adding a behavioral
change (there are some other tickets dealing with ipadiscovery, e.g. see
https://fedorahosted.org/freeipa/ticket/5270
https://fedorahosted.org/freeipa/ticket/3912 ) can potentially be higher
that a more large-scale rewrite of the module and related codebase.

Since we plan to do some general refactoring work anyway, I would like
to discuss the possible course of action we may take to tackle this
issue. I would like to present the following options we have been
discussing so far:

1.) Do a substantial rewrite of existing code ("ipaclient/ipadiscovery.py")

We may take the existing IPADiscovery class and try rewrite it in order
to get a more concise and deterministic code, which will:

* rely more on python-dns and answers provided by resolver (e.g. we are
directly parsing resolv.conf for available domains when we can ask the
resolver to get domains for us)
* be more pythonic (replace error codes with thrown exceptions, clean up
numerous C-isms etc.)
* not try to outsmart user when server/realm/domain is specified
beforehand. Currently, even if you specify all three options, there is
still some DNS discovery performed, hence bug #4305. I think that one
you specify server(s), not magic should be performed and the discovery
process should be reduced to checking whether they are IPA servers and
pull all other info (like realm) from them.

This may be a considerable effort requiring some time to implement and
get right, but IMHO still comparable to the time spent iteratively
placing 'if' statements into the existing code and hoping to not break
anything. I would even argue it is not worth the effort because we can

2.) Use realmd dbus interface to do DNS discovery

I have attached aquick and dirty script I have slapped together to
leverage 'org.freedesktop.realmd.Discover' interface to do IPA realm
discovery for us. This has a lot of appeal to me since we are leveraging
existing codebase for DNS discovery and will have to handle only cases
when the user manually specifies a list of IPA servers for the client.

This however pulls in realmd as a (potentially circular) dependency for
ipa client, and when we find bug in the discovery code, we will have to
poke upstream (Stef or Sumit I think) to fix it.

So from the long-term point of view, Jan Cholasta's (CC'ed) suggestion to:

3.) Split out IPA discovery portion of realmd to a separate C library
shared between IPA and realmd

may be best. Both projects will have shared codebase (maintained either
by us or realmd devs), which can be reused also by other people to
create their own discovery/enrollment solution. This would however
require sustained and concerted efforts of both teams to create the
library and possibly rewrite realmd to accommodate this change.

There may be some other options viable for us, if so please mention them
in a reply. Also please correct any piece of information I got wrong.

TL;DR: IPA realm/domain discovery is a mess, please suggest a way to fix
it.



#3 sounds good from long term perspective.

In short term, i.e., #4305, we should skip discovery when --on-master is
used.


I would prefer #2. Because as seen from the patch it is already working
and can easily be used from python. I think this was also one of the
reasons why Stef used a DBus service for this instead of a C library
which then needs various bindings for python, Java, ruby, Go you name
it.



This approach is also my favorite. However, my (and several of my
colleagues) concern is that
https://bugzilla.redhat.com/show_bug.cgi?id=1271551 will break it in
kickstart environment. I don't know how much of an issue is this, I guess it
completely precludes automatic enrollment during machine provisioning.


realmd has the --dbus-peer option. So I guess it might be possible that
realmd can be started by ipa-client-install directly in this case and
allow communication over a socket pair. I'm not sure how hard or easy
this would be in python.

Stef, do you have some pointers how to use dbus-peer from python?

bye,
Sumit



We have already done something similar with certmonger. You can look into
ipapython/certmonger.py


Great, so it should be possible to use realmd in the same way in a
kickstart environment.


Then I vote for #2 as it is easy enough.

If necessary, realmd 

Re: [Freeipa-devel] [PATCH 0423] fix duplicated except

[Jan Cholasta]

On 25.2.2016 11:25, David Kupka wrote:

On 24/02/16 17:56, Martin Basti wrote:

During my playing with pylint, I fixed this issue which allows us to
enable additional check in pylint (the nice one).

Patch attached, it should go only to master.



Works for me, ACK.

I always wonder how something like this can even get to the sources.
Fortunately the added check will prevent that in the future.


Before this is pushed, could you please check git history to verify that 
these duplicate excepts are not symptomps of some actual problems?


--
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 0421] Make PTR records check optional for IPA installation

[Petr Spacek]

On 24.2.2016 15:13, Martin Basti wrote:
> https://fedorahosted.org/freeipa/ticket/5686
> 
> Patch attached.

LGTM, ACK if it passes QE testing.

-- 
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 0424] Pylint: add missing attributes of exceptions to definition in pylint plugin

[David Kupka]

On 24/02/16 18:54, Martin Basti wrote:

Pylint is not able to handle IPA errors objects, because attributes are
added into objects dynamically, and pylint 1.5 reports them as no-member
errors.

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

Patch attached.




It would be better to define all errors with all unrecoverable members 
but this is sufficient start. More can be add when it's needed.


Works for me, ACK.

--
David Kupka

--
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 0423] fix duplicated except

[David Kupka]

On 25/02/16 11:40, Jan Cholasta wrote:

On 25.2.2016 11:25, David Kupka wrote:

On 24/02/16 17:56, Martin Basti wrote:

During my playing with pylint, I fixed this issue which allows us to
enable additional check in pylint (the nice one).

Patch attached, it should go only to master.



Works for me, ACK.

I always wonder how something like this can even get to the sources.
Fortunately the added check will prevent that in the future.


Before this is pushed, could you please check git history to verify that
these duplicate excepts are not symptomps of some actual problems?



Archaeology hat on.

The duplicate except statement in ipalib/plugins/automount.py was 
introduced by commit 0197ebbb and was there since 2010. All the time the 
second except was logically dead code.


The duplicate except statement in ipalib/backend.py was introduced by 
commit 01da4a8d converting StandardError to Exception. But the only 
difference is the message in raise error.


I should be save to push this.


--
David Kupka

--
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] 0017 configure DNA shared config entry to allow connection with GSSAPI

[Martin Babinsky]

On 02/24/2016 04:30 PM, thierry bordaz wrote:

On 01/21/2016 05:04 PM, Martin Babinsky wrote:

On 01/21/2016 01:37 PM, thierry bordaz wrote:





Hi Thierry,

I have couple of comments to your patch:

1.)
there is a number of PEP8 errors in the patch
(http://paste.fedoraproject.org/313246/33893701), please fix them.

See http://www.freeipa.org/page/Python_Coding_Style for our
conventions used in Python code.

2.)
+DNA_BIND_METHOD   = "dnaRemoteBindMethod"
+DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
+DNA_PLUGIN_DN = 'cn=Distributed Numeric Assignment
Plugin,cn=plugins,cn=config'
+dna_config_base   = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN

Uppercase names are usually reserved for module-level constants. OTOH,
local variables should be lowercase. Also you can instantiate
dna_config_base directly as DN, using 2-member tuples, i. e:

"""
dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed Numeric
Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
"""

When passing DN object to the formatting functions/operators, it is
automatically converted to string so no need to hold string and DN
object separately. This is done in other places (see function/methods
in replication.py).

3.)

+for i in range(len(entries)) :
+
+mod = []
+if entries[i].single_value.get(DNA_BIND_METHOD) != method:
+mod.append((ldap.MOD_REPLACE, DNA_BIND_METHOD, method))
+
+if entries[i].single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+mod.append((ldap.MOD_REPLACE, DNA_CONN_PROTOCOL,
protocol))


please use idiomatic Python when handling list of entries, e.g.:

"""
for entry in entries:
   mod = []
   if entry.single_value.get(DNA_BIND_METHOD) != method:
   ...
"""

4.) I think that this method should in DSInstance class since it deals
with directory server configuration. Service is a parent object of all
other service installers/configurators and should contain only methods
common to more children.

5.) Since the method is called from every installer, it could be
beneficial to call it in DSInstance.__common_post_setup() as a part of
Directory server installation. Is there any reason why this is not the
case?

6.)

+while attempt != MAX_WAIT:
+try:
+entries = conn.get_entries(sharedcfgdn,
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
+break
+except errors.NotFound:
+root_logger.debug("So far enable not find DNA shared
config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn,
sharedcfgdn))
+attempt = attempt + 1
+time.sleep(2)
+continue
+
+# safety checking
+# there is no return, if there are several entries, as a
workaround of #5510
+if len(entries) != 1:

I am quite afraid what would happen if the server does not return any
entries until 30 s timeout. The code will then continue to the
condition which can potentially test an uninitialized variable and
blow up with 'NameError'. This should be handled more robustly, e. g.
raise an exception when a timeout is reached and no entries were
returned.

7.)

+if len(mod) > 0:

A Pythonic way to test for non-empty container is

"""
if mods:
   # do stuff
"""

since an empty list/dict/set evaluates to False and non-empty
containers are True.


8.)

+entry = conn.get_entry(entries[i].dn)
+if entry.single_value.get(DNA_BIND_METHOD) != method:
+root_logger.error("Fail to set SASL/GSSAPI bind
method to %s" % (entries[i].dn))
+if entry.single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+root_logger.error("Fail to set LDAP protocol to
%s" % (entries[i].dn))

rather than re-fetching the modified entry and testing what happened,
you can just catch an exception raised by unsuccessfull mod and log an
error like this:

"""
try:
   conn.modify_s(entry.dn, mod)
except Exception as e:
   root_logger.error("Failed to modify entry {}: {}".format(entry, e))
"""

as a matter of fact, if the modify_s operation would fail for some
reason, an ldap exception would be raised and you checks would not
even be executed.

9.)
The debug message on line 219 should read "Unable to find DNA shared
config entry for dnaHostname=%s so far. Retry in 2 sec.". The errors
at the end of the method should have "Failed" instead of "Fail".


Hi Martin,

Finally tested... here is the updated patch. Thanks for you patience


thanks
thierry


Hi Thierry,

the patch works as expected. I have some more nitpicks though:

1.) Please fix the following pep8 errors:

http://paste.fedoraproject.org/329086/56397841/

you can check whether you recent commit conforms to PEP8 by running

"git show -U0 | pep8 --diff"

2.)
+self.step("update DNA shared config entry", 
self.update_dna_shared_config)


I would rather change the message to "Updating DNA 

Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI

[thierry bordaz]

On 02/25/2016 12:03 PM, Martin Babinsky wrote:

On 02/24/2016 04:30 PM, thierry bordaz wrote:

On 01/21/2016 05:04 PM, Martin Babinsky wrote:

On 01/21/2016 01:37 PM, thierry bordaz wrote:





Hi Thierry,

I have couple of comments to your patch:

1.)
there is a number of PEP8 errors in the patch
(http://paste.fedoraproject.org/313246/33893701), please fix them.

See http://www.freeipa.org/page/Python_Coding_Style for our
conventions used in Python code.

2.)
+DNA_BIND_METHOD   = "dnaRemoteBindMethod"
+DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
+DNA_PLUGIN_DN = 'cn=Distributed Numeric Assignment
Plugin,cn=plugins,cn=config'
+dna_config_base   = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN

Uppercase names are usually reserved for module-level constants. OTOH,
local variables should be lowercase. Also you can instantiate
dna_config_base directly as DN, using 2-member tuples, i. e:

"""
dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed Numeric
Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
"""

When passing DN object to the formatting functions/operators, it is
automatically converted to string so no need to hold string and DN
object separately. This is done in other places (see function/methods
in replication.py).

3.)

+for i in range(len(entries)) :
+
+mod = []
+if entries[i].single_value.get(DNA_BIND_METHOD) != method:
+mod.append((ldap.MOD_REPLACE, DNA_BIND_METHOD, 
method))

+
+if entries[i].single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+mod.append((ldap.MOD_REPLACE, DNA_CONN_PROTOCOL,
protocol))


please use idiomatic Python when handling list of entries, e.g.:

"""
for entry in entries:
   mod = []
   if entry.single_value.get(DNA_BIND_METHOD) != method:
   ...
"""

4.) I think that this method should in DSInstance class since it deals
with directory server configuration. Service is a parent object of all
other service installers/configurators and should contain only methods
common to more children.

5.) Since the method is called from every installer, it could be
beneficial to call it in DSInstance.__common_post_setup() as a part of
Directory server installation. Is there any reason why this is not the
case?

6.)

+while attempt != MAX_WAIT:
+try:
+entries = conn.get_entries(sharedcfgdn,
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
+break
+except errors.NotFound:
+root_logger.debug("So far enable not find DNA shared
config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn,
sharedcfgdn))
+attempt = attempt + 1
+time.sleep(2)
+continue
+
+# safety checking
+# there is no return, if there are several entries, as a
workaround of #5510
+if len(entries) != 1:

I am quite afraid what would happen if the server does not return any
entries until 30 s timeout. The code will then continue to the
condition which can potentially test an uninitialized variable and
blow up with 'NameError'. This should be handled more robustly, e. g.
raise an exception when a timeout is reached and no entries were
returned.

7.)

+if len(mod) > 0:

A Pythonic way to test for non-empty container is

"""
if mods:
   # do stuff
"""

since an empty list/dict/set evaluates to False and non-empty
containers are True.


8.)

+entry = conn.get_entry(entries[i].dn)
+if entry.single_value.get(DNA_BIND_METHOD) != method:
+root_logger.error("Fail to set SASL/GSSAPI bind
method to %s" % (entries[i].dn))
+if entry.single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+root_logger.error("Fail to set LDAP protocol to
%s" % (entries[i].dn))

rather than re-fetching the modified entry and testing what happened,
you can just catch an exception raised by unsuccessfull mod and log an
error like this:

"""
try:
   conn.modify_s(entry.dn, mod)
except Exception as e:
   root_logger.error("Failed to modify entry {}: {}".format(entry, e))
"""

as a matter of fact, if the modify_s operation would fail for some
reason, an ldap exception would be raised and you checks would not
even be executed.

9.)
The debug message on line 219 should read "Unable to find DNA shared
config entry for dnaHostname=%s so far. Retry in 2 sec.". The errors
at the end of the method should have "Failed" instead of "Fail".


Hi Martin,

Finally tested... here is the updated patch. Thanks for you patience


thanks
thierry


Hi Thierry,

the patch works as expected. 


8-)

I have some more nitpicks though:

1.) Please fix the following pep8 errors:

http://paste.fedoraproject.org/329086/56397841/

you can check whether you recent commit conforms to PEP8 by running

"git show -U0 | pep8 --diff"


I did it and fixed most of them but what remains is long line. I fixed 
some of them 

[Freeipa-devel] [PATCH 0425] pylint: suppress false positive no-member errors

[Martin Basti]

The last pylint 1.5 patch, \o/

https://fedorahosted.org/freeipa/ticket/5615
From 785fb245fbe04b4cc630e6bc1c1ee670d6eca6a8 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 25 Feb 2016 13:46:33 +0100
Subject: [PATCH] pylint: supress false positive no-member errors

pylint 1.5 prints many false positive no-member errors which are
supressed by this commit.

https://fedorahosted.org/freeipa/ticket/5615
---
 install/tools/ipactl | 4 ++--
 ipalib/krb_utils.py  | 2 +-
 ipalib/plugins/batch.py  | 9 +++--
 ipalib/plugins/server.py | 5 +++--
 ipapython/ipaldap.py | 2 +-
 ipapython/ipautil.py | 2 +-
 ipapython/nsslib.py  | 7 +--
 ipaserver/install/installutils.py| 9 +++--
 ipaserver/install/ipa_otptoken_import.py | 4 +++-
 ipaserver/install/server/install.py  | 2 ++
 ipaserver/install/service.py | 3 ++-
 ipatests/test_xmlrpc/xmlrpc_test.py  | 2 ++
 12 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/install/tools/ipactl b/install/tools/ipactl
index ff5ea5a50a291da35e895d5674bdc8ea76ed48d4..d27ada56556c58c42242cf0add11ef47d4440f56 100755
--- a/install/tools/ipactl
+++ b/install/tools/ipactl
@@ -290,7 +290,7 @@ def ipa_start(options):
 
 if isinstance(e, IpactlError):
 # do not display any other error message
-raise IpactlError(rval=e.rval)
+raise IpactlError(rval=e.rval)  # pylint: disable=no-member
 else:
 raise IpactlError()
 
@@ -387,7 +387,7 @@ def ipa_restart(options):
 pass
 if isinstance(e, IpactlError):
 # do not display any other error message
-raise IpactlError(rval=e.rval)
+raise IpactlError(rval=e.rval)  # pylint: disable=no-member
 else:
 raise IpactlError()
 
diff --git a/ipalib/krb_utils.py b/ipalib/krb_utils.py
index b33e4b7c82cf08c68220531ebacca309117ad770..e6e277c7a0926187ebcde7ba08e45ebb56ad865e 100644
--- a/ipalib/krb_utils.py
+++ b/ipalib/krb_utils.py
@@ -160,7 +160,7 @@ def get_credentials(name=None, ccache_name=None):
 try:
 return gssapi.Credentials(usage='initiate', name=name, store=store)
 except gssapi.exceptions.GSSError as e:
-if e.min_code == KRB5_FCC_NOFILE:
+if e.min_code == KRB5_FCC_NOFILE:  # pylint: disable=no-member
 raise ValueError('"%s", ccache="%s"' % (e.message, ccache_name))
 raise
 
diff --git a/ipalib/plugins/batch.py b/ipalib/plugins/batch.py
index 626ba2835bd5387df3d49d66293284f40e4b0d42..2da7b7ca811fc67b22c43655352ace539488ce0d 100644
--- a/ipalib/plugins/batch.py
+++ b/ipalib/plugins/batch.py
@@ -114,11 +114,16 @@ class batch(Command):
 if isinstance(e, errors.RequirementError) or \
 isinstance(e, errors.CommandError):
 self.info(
-'%s: batch: %s', context.principal, e.__class__.__name__
+'%s: batch: %s',
+context.principal,  # pylint: disable=no-member
+e.__class__.__name__
 )
 else:
 self.info(
-'%s: batch: %s(%s): %s', context.principal, name, ', '.join(api.Command[name]._repr_iter(**params)),  e.__class__.__name__
+'%s: batch: %s(%s): %s',
+context.principal, name,  # pylint: disable=no-member
+', '.join(api.Command[name]._repr_iter(**params)),
+e.__class__.__name__
 )
 if isinstance(e, errors.PublicError):
 reported_error = e
diff --git a/ipalib/plugins/server.py b/ipalib/plugins/server.py
index e31def77cc649e6392ea8527040e61a56a83ff2f..93ced8b73049b61fe274c15d84150a892cd34529 100644
--- a/ipalib/plugins/server.py
+++ b/ipalib/plugins/server.py
@@ -228,8 +228,9 @@ class server_conncheck(crud.PKQuery):
 privilege = u'Replication Administrators'
 privilege_dn = self.api.Object.privilege.get_dn(privilege)
 ldap = self.obj.backend
-filter = ldap.make_filter(
-{'krbprincipalname': context.principal, 'memberof': privilege_dn},
+filter = ldap.make_filter({
+'krbprincipalname': context.principal,  # pylint: disable=no-member
+'memberof': privilege_dn},
 rules=ldap.MATCH_ALL)
 try:
 ldap.find_entries(base_dn=self.api.env.basedn, filter=filter)
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 7522c504b5b8901002776521f05f4ebab8c35ec8..2965ba4a5509eb16589bc9dde9485147d9114032 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -208,7 +208,7 @@ class LDAPEntry(collections.MutableMapping):
 
 Keyword arguments can be used to override values of specific attributes.
  

Re: [Freeipa-devel] [PATCH 0390] Fix build with GCC 4.9+

[Petr Spacek]

On 19.2.2016 13:55, Petr Spacek wrote:
> Hello,
> 
> Fix build with GCC 4.9+.
> 
> GCC 4.9+ is too aggressive when optimizing functions with nonnull
> attributes. This removes most of asserts() in the plugin.
> GCC 6 adds warnings for these cases.
> 
> We are disabling the unwanted condition pruning by adding
> -fno-delete-null-pointer-checks argument.
> BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a.
> 
> Additionally we silence warnings to prevent build failures when -Werror
> is used.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1307346

Updated version is attached. It contains less autotools magic because it
enables attribute nonnull only under Clang static analyzer and Coverity - as a
result we do not have to silence GCC warnings from -Wnonnull.

Please review so I can fix build in Fedora 24.

Thank you.

-- 
Petr^2 Spacek
From 4732fe9f4e525c44b46e7ed0734ccaec94fba49e Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Fri, 19 Feb 2016 13:39:27 +0100
Subject: [PATCH] Fix build with GCC 4.9+.

GCC 4.9+ is too aggressive when optimizing functions with nonnull
attributes. This removes most of asserts() in the plugin.
GCC 6 adds warnings for these cases.

We are disabling the unwanted condition pruning by adding
-fno-delete-null-pointer-checks argument.
BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a.

Additionally we enable nonnull attribute only when the build is running under
Clang static analyzer or Coverity.

https://bugzilla.redhat.com/show_bug.cgi?id=1307346
---
 configure.ac | 13 +
 src/util.h   |  8 ++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index a06708b1a5ee64bb64c80272c10ed1a35670c8d0..a0123ac0a62b5acd5238f028d8c42e83af4060db 100644
--- a/configure.ac
+++ b/configure.ac
@@ -39,6 +39,19 @@ AC_TRY_COMPILE([
 [CFLAGS="$SAVED_CFLAGS"
  AC_MSG_RESULT([no])])
 
+# Check if build chain supports -fno-delete-null-pointer-checks
+# this flag avoids too agressive optimizations which would remove some asserts
+# BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a
+AC_MSG_CHECKING([for -fno-delete-null-pointer-checks compiler flag])
+SAVED_CFLAGS="$CFLAGS"
+CFLAGS="$CFLAGS -fno-delete-null-pointer-checks"
+AC_TRY_COMPILE([
+	extern int fdef(void);
+],[],
+[AC_MSG_RESULT([yes])],
+[CFLAGS="$SAVED_CFLAGS"
+ AC_MSG_RESULT([no])])
+
 # Get CFLAGS from isc-config.sh
 AC_ARG_VAR([BIND9_CFLAGS],
[C compiler flags for bind9, overriding isc-config.sh])
diff --git a/src/util.h b/src/util.h
index 9849ff9b6c38ec1c6dd143440d5b5e584b2ecd51..402503c339a5ab6ca5273cae420e743b9fc252ab 100644
--- a/src/util.h
+++ b/src/util.h
@@ -103,11 +103,15 @@ extern isc_boolean_t verbose_checks; /* from settings.c */
 /* If no argument index list is given to the nonnull attribute,
  * all pointer arguments are marked as non-null. */
 #define ATTR_NONNULLS ATTR_NONNULL()
-#ifdef __GNUC__
+#if defined(__COVERITY__) || defined(__clang_analyzer__)
 #define ATTR_NONNULL(...) __attribute__((nonnull(__VA_ARGS__)))
-#define ATTR_CHECKRESULT __attribute__((warn_unused_result))
 #else
 #define ATTR_NONNULL(...)
+#endif
+
+#if defined(__GNUC__)
+#define ATTR_CHECKRESULT __attribute__((warn_unused_result))
+#else
 #define ATTR_CHECKRESULT
 #endif
 
-- 
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 0001] Add new parameter --ssh-update to ipa-client-install

[Jan Cholasta]

On 25.2.2016 15:59, Petr Spacek wrote:

On 25.2.2016 14:36, Jan Cholasta wrote:

Hi,

On 25.2.2016 14:23, Martin Basti wrote:



On 22.02.2016 22:13, Martin Štefany wrote:

Hi,

please, review the attached patch which adds --ssh-update to ipa-client-
install.

Ticket:https://fedorahosted.org/freeipa/ticket/2655

Hello,
thank you for your patch.
Please attach a patch as a file next time.

I have doubts that this should be part of ipa-client-install, this needs
a broader discussion.


+1, I think it should be a separate command (ignore my earlier suggestion from
Trac to incorporate this into ipa-client-install, I was young and stupid).

See client/ipa-certupdate and ipaclient/ipa_certupdate.py for an example of
how such a command should be implemented.


Or we can have idempotent client installer as we have ipa-dns-install ...


We can, but don't. Patches are welcome of course.

Nonetheless, you should be able to reinstall just the SSH keys, as 
opposed to the full client, which is exactly what the separate command 
should accomplish.




Petr^2 Spacek


Code comments inline:


---
Martin

>From 4974a57f48a0cd48b83724297ae2af572bc530eb Mon Sep 17 00:00:00 2001
From: Martin Stefany 
Date: Mon, 22 Feb 2016 20:58:13 +
Subject: [PATCH] Add new parameter --ssh-update to ipa-client-install

Add a new parameter '--ssh-update' which can be used later after freeipa
client is installed to update SSH hostkeys and SSHFP DNS records for
host.

https://fedorahosted.org/freeipa/ticket/2655
---
   ipa-client/ipa-install/ipa-client-install | 102
+-
   1 file changed, 99 insertions(+), 3 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-
install/ipa-client-install
index
789ff591591673744ee3b922e5c0181233ad553c..97adfb6b449fb441bddada89a3b151
33e398ca50 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -71,6 +71,7 @@ CLIENT_INSTALL_ERROR = 1
   CLIENT_NOT_CONFIGURED = 2
   CLIENT_ALREADY_CONFIGURED = 3
   CLIENT_UNINSTALL_ERROR = 4 # error after restoring files/state
+CLIENT_SSHUPDATE_ERROR = 5 # error during update of SSH public keys

   def parse_options():
   def validate_ca_cert_file_option(option, opt, value, parser):
@@ -215,6 +216,12 @@ def parse_options():
 "be run with --unattended
option")
   parser.add_option_group(uninstall_group)

+sshupdate_group = OptionGroup(parser, "SSH key update options")
+sshupdate_group.add_option("--ssh-update", dest="ssh_update",
+  action="store_true", default=False,
+  help="update local host's SSH public keys in host
entry and DNS.")
+parser.add_option_group(sshupdate_group)
+
   options, args = parser.parse_args()
   safe_opts = parser.get_safe_opts(options)

@@ -840,6 +847,92 @@ def uninstall(options, env):

   return rv

+def sshupdate(options, env):
+if not is_ipa_client_installed():
+root_logger.error("IPA client is not configured on this
system.")
+return CLIENT_NOT_CONFIGURED
+
+api.bootstrap(context='cli_installer', debug=options.debug)
+api.finalize()
+if 'config_loaded' not in api.env:
+root_logger.error("Failed to initialize IPA API.")
+return CLIENT_SSHUPDATE_ERROR
+
+# Now, let's try to connect to the server's RPC interface
+connected = False
+try:
+api.Backend.rpcclient.connect()
+connected = True
+root_logger.debug("Try RPC connection")
+api.Backend.rpcclient.forward('ping')
+except errors.KerberosError as e:
+if connected:
+api.Backend.rpcclient.disconnect()
+root_logger.info(
+"Cannot connect to the server due to Kerberos error: %s. "
+"Trying with delegate=True", e)
+try:
+api.Backend.rpcclient.connect(delegate=True)
+root_logger.debug("Try RPC connection")
+api.Backend.rpcclient.forward('ping')
+
+root_logger.info("Connection with delegate=True
successful")
+
+# The remote server is not capable of Kerberos S4U2Proxy
+# delegation. This features is implemented in IPA server
+# version 2.2 and higher
+root_logger.warning(
+"Target IPA server has a lower version than the
enrolled "
+"client")
+root_logger.warning(
+"Some capabilities including the ipa command capability
"
+"may not be available")
+except errors.PublicError as e2:
+root_logger.warning(
+"Second connect with delegate=True also failed: %s",
e2)
+root_logger.error(
+"Cannot connect to the IPA server RPC interface: %s",
e2)
+return CLIENT_SSHUPDATE_ERROR
+except errors.PublicError as e:
+root_logger.error(
+"Cannot connect to the server due to 

Re: [Freeipa-devel] [PATCH 0001] Add new parameter --ssh-update to ipa-client-install

[Petr Spacek]

On 25.2.2016 14:36, Jan Cholasta wrote:
> Hi,
> 
> On 25.2.2016 14:23, Martin Basti wrote:
>>
>>
>> On 22.02.2016 22:13, Martin Štefany wrote:
>>> Hi,
>>>
>>> please, review the attached patch which adds --ssh-update to ipa-client-
>>> install.
>>>
>>> Ticket:https://fedorahosted.org/freeipa/ticket/2655
>> Hello,
>> thank you for your patch.
>> Please attach a patch as a file next time.
>>
>> I have doubts that this should be part of ipa-client-install, this needs
>> a broader discussion.
> 
> +1, I think it should be a separate command (ignore my earlier suggestion from
> Trac to incorporate this into ipa-client-install, I was young and stupid).
> 
> See client/ipa-certupdate and ipaclient/ipa_certupdate.py for an example of
> how such a command should be implemented.

Or we can have idempotent client installer as we have ipa-dns-install ...

Petr^2 Spacek

>> Code comments inline:
>>>
>>> ---
>>> Martin
>>>
>>> >From 4974a57f48a0cd48b83724297ae2af572bc530eb Mon Sep 17 00:00:00 2001
>>> From: Martin Stefany 
>>> Date: Mon, 22 Feb 2016 20:58:13 +
>>> Subject: [PATCH] Add new parameter --ssh-update to ipa-client-install
>>>
>>> Add a new parameter '--ssh-update' which can be used later after freeipa
>>> client is installed to update SSH hostkeys and SSHFP DNS records for
>>> host.
>>>
>>> https://fedorahosted.org/freeipa/ticket/2655
>>> ---
>>>   ipa-client/ipa-install/ipa-client-install | 102
>>> +-
>>>   1 file changed, 99 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-
>>> install/ipa-client-install
>>> index
>>> 789ff591591673744ee3b922e5c0181233ad553c..97adfb6b449fb441bddada89a3b151
>>> 33e398ca50 100755
>>> --- a/ipa-client/ipa-install/ipa-client-install
>>> +++ b/ipa-client/ipa-install/ipa-client-install
>>> @@ -71,6 +71,7 @@ CLIENT_INSTALL_ERROR = 1
>>>   CLIENT_NOT_CONFIGURED = 2
>>>   CLIENT_ALREADY_CONFIGURED = 3
>>>   CLIENT_UNINSTALL_ERROR = 4 # error after restoring files/state
>>> +CLIENT_SSHUPDATE_ERROR = 5 # error during update of SSH public keys
>>>
>>>   def parse_options():
>>>   def validate_ca_cert_file_option(option, opt, value, parser):
>>> @@ -215,6 +216,12 @@ def parse_options():
>>> "be run with --unattended
>>> option")
>>>   parser.add_option_group(uninstall_group)
>>>
>>> +sshupdate_group = OptionGroup(parser, "SSH key update options")
>>> +sshupdate_group.add_option("--ssh-update", dest="ssh_update",
>>> +  action="store_true", default=False,
>>> +  help="update local host's SSH public keys in host
>>> entry and DNS.")
>>> +parser.add_option_group(sshupdate_group)
>>> +
>>>   options, args = parser.parse_args()
>>>   safe_opts = parser.get_safe_opts(options)
>>>
>>> @@ -840,6 +847,92 @@ def uninstall(options, env):
>>>
>>>   return rv
>>>
>>> +def sshupdate(options, env):
>>> +if not is_ipa_client_installed():
>>> +root_logger.error("IPA client is not configured on this
>>> system.")
>>> +return CLIENT_NOT_CONFIGURED
>>> +
>>> +api.bootstrap(context='cli_installer', debug=options.debug)
>>> +api.finalize()
>>> +if 'config_loaded' not in api.env:
>>> +root_logger.error("Failed to initialize IPA API.")
>>> +return CLIENT_SSHUPDATE_ERROR
>>> +
>>> +# Now, let's try to connect to the server's RPC interface
>>> +connected = False
>>> +try:
>>> +api.Backend.rpcclient.connect()
>>> +connected = True
>>> +root_logger.debug("Try RPC connection")
>>> +api.Backend.rpcclient.forward('ping')
>>> +except errors.KerberosError as e:
>>> +if connected:
>>> +api.Backend.rpcclient.disconnect()
>>> +root_logger.info(
>>> +"Cannot connect to the server due to Kerberos error: %s. "
>>> +"Trying with delegate=True", e)
>>> +try:
>>> +api.Backend.rpcclient.connect(delegate=True)
>>> +root_logger.debug("Try RPC connection")
>>> +api.Backend.rpcclient.forward('ping')
>>> +
>>> +root_logger.info("Connection with delegate=True
>>> successful")
>>> +
>>> +# The remote server is not capable of Kerberos S4U2Proxy
>>> +# delegation. This features is implemented in IPA server
>>> +# version 2.2 and higher
>>> +root_logger.warning(
>>> +"Target IPA server has a lower version than the
>>> enrolled "
>>> +"client")
>>> +root_logger.warning(
>>> +"Some capabilities including the ipa command capability
>>> "
>>> +"may not be available")
>>> +except errors.PublicError as e2:
>>> +root_logger.warning(
>>> +"Second connect with delegate=True also failed: %s",
>>> e2)
>>> +root_logger.error(
>>> +"Cannot connect to the IPA 

Re: [Freeipa-devel] [PATCH] 0005 webui: topology graph: canvas resizes itself according to the window size

[Pavel Vomacka]

On 02/17/2016 06:29 PM, Petr Vobornik wrote:

On 02/15/2016 04:20 PM, Pavel Vomacka wrote:



On 02/12/2016 01:52 PM, Pavel Vomacka wrote:



On 02/11/2016 12:31 PM, Pavel Vomacka wrote:

Hello,

The canvas of the graph had static size. This patch fixes this issue
and from now the graph canvas is resized according to the window size.

Pavel Vomacka



Because of changes in previous patch I'm sending also this one again.
Plus I fixed some jslint warnings.

And again a link to the ticket:
https://fedorahosted.org/freeipa/ticket/5647 .

--
Pavel^3 Vomacka



And another change in the code. This patch adds checking whether a svg
element even exists. And don't add 'col-sm-12' class to the svg element
any more. This class just added useless paddings to the element.

--
Pavel^3 Vomacka



Hi,

thanks for the patch.

Hi,

thank you for reviewing.


1. I don't like the fact that the resize handler registered in 
initialize method is active forever, even when viewing other facets.
I moved the handler to the topology graph facet. It is also removed 
after hide event is emited.
2. The code will probably fail if there is other svg element present 
on the page.


$('svg') searches for all svg elements in DOM, such search is usually 
slow and undeterministic. It is better to use a stored reference(if 
possible) or limit the search to some parent element, e.g. TopoGraph 
can store and then use its container.


Would be funny if there were 2 graphs.

Yep, you are right. I avoid using this type of searching in this patch.



3. Why is there the toFixed(1) call? Or more specifically on that 
position? It hides the fact that toFixed transforms Number to String 
and then '-' operator with Number on the right casts it back to Number.
The toFixed(1) was used just because we don't need so accurate numbers, 
but in this patch this function is not used any more.


4. width could be just: this._svg.parent().width()
The width is now solved by using this.content.width() in topology graph 
facet. I think that the calculating of width and height should be at the 
same place. That is why I didn't put calculating of width into the 
TopoGraph.


5. Your approach for bottom padding works well but I don't like that 
the component assumes that there is some col-sm-12 element on a page 
whose right padding is actually equal to space on the left of the svg.

I agree, fixed.


#1 and #5 makes me think that the resize logic should be moved 
topology facet. Something like:


* register resize handler on facet's 'show' event
* unregister resize handler on facet's 'hide' event (will solve #1)
* on window resize, compute the size in topology facet, call new 
.resize(width, height) method of TopoGraph


Then, we wouldn't have to search whole DOM for 'svg' elements to check 
if page is visible. The bottom padding can be obtained by: 
parseInt(this.content.css('paddingLeft')) where 'this' is facet.



I followed these tips and here is a new patch.

--
Pavel^3 Vomacka
>From 597997b97bd48d1ec92c977fc5429c5c2711a8c1 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Thu, 25 Feb 2016 15:02:04 +0100
Subject: [PATCH] Resize topology graph canvas according to window size

The size of svg element is calculated when the topology graph facet is load
and then every time when the window is resized. The resize event listener
is removed after the topology graph facet emits hide event.

https://fedorahosted.org/freeipa/ticket/5647
---
 install/ui/src/freeipa/topology.js   | 41 
 install/ui/src/freeipa/topology_graph.js | 15 ++--
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/install/ui/src/freeipa/topology.js b/install/ui/src/freeipa/topology.js
index 6e67484cc7542765056f0887928a64e4dc576836..a01296ee4b9a991c4655a004f947177bee6bc64f 100644
--- a/install/ui/src/freeipa/topology.js
+++ b/install/ui/src/freeipa/topology.js
@@ -431,14 +431,39 @@ topology.TopologyGraphFacet = declare([Facet, ActionMixin, HeaderMixin], {
 init: function(spec) {
 this.inherited(arguments);
 var graph = this.get_widget('topology-graph');
+var listener = this.resize_listener.bind(this, graph);
 
 on(this, 'show', lang.hitch(this, function(args) {
-graph.update();
+var size = this.calculate_canvas_size();
+graph.update(size.height, size.width);
+
+$(window).on('resize', null, listener);
 }));
 
 on(graph, 'link-selected', lang.hitch(this, function(data) {
 this.set_selected_link(data.link);
 }));
+
+on(this, 'hide', function () {
+$(window).off('resize', null, listener);
+});
+},
+
+resize_listener: function(graph) {
+var size = this.calculate_canvas_size();
+
+graph.resize(size.height, size.width);
+},
+
+calculate_canvas_size: function() {
+var space = parseInt(this.content.css('paddingLeft'), 10);
+var height = 

Re: [Freeipa-devel] Locations design v2: LDAP schema & user interface

[Petr Spacek]

On 25.2.2016 15:28, Simo Sorce wrote:
> On Thu, 2016-02-25 at 14:45 +0100, Petr Spacek wrote:
>> Variant C
>> -
>> An alternative is to be lazy and dumb. Maybe it would be enough for
>> the first
>> round ...
>>
>> We would retain
>> [first step - no change from variant A]
>> * create locations
>> * assign 'main' (aka 'primary' aka 'home') servers to locations
>> ++ specify weights for the 'main' servers in given location, i.e.
>> manually
>> input (server, weight) tuples
>>
>> Then, backups would be auto-generated set of all remaining servers
>> from all
>> other locations.
>>
>> Additional storage complexity: 0
>>
>> This covers the scenario "always prefer local servers and use remote
>> only as
>> fallback" easily. It does not cover any other scenario.
>>
>> This might be sufficient for the first run and would allow us to
>> gather some
>> feedback from the field.
>>
>> Now I'm inclined to this variant :-)
> 
> To be honest, this is all I always had in mind, for the first step.
> 
> To recap:
> - define a location with the list of servers (perhaps location is a
> property of server objects so you can have only one location per server,
> and if you remove the server it is automatically removed from the
> location w/o additional work or referential integrity necessary), if
> weight is not defined (default) then they all have the same weight.

Agreed.


> - Allow to specify backup locations in the location object, priorities
> are calculated automatically and all backup locations have same weight.

Hmm, weights have to be inherited form the original location in all cases. Did
you mean that all backup locations have the same *priority*?

Anyway, explicit configuration of backup locations is introducing API and
schema for variant A and that is what I'm questioning above. It is hard to
make it extensible so we do not have headache in future when somebody decides
that more flexibility is needed OR that link-based approach is better.

In other words, for doing what you propose above we would have to design
complete schema and API for variant A anyway to make sure we do not lock
ourselves, so we are not getting any saving by doing so.


> - Define a *default* location, which is the backup for any other
> location but always with lower priority to any other explicitly defined
> backup locations.

I would rather *always* use the default location as backup for all other
locations. It does not require any API or schema (as it equals to "all
servers" except "servers in this location" which can be easily calculated on 
fly).

This can be later on extended in whatever direction we want without any
upgrade/migration problem.

More importantly, all the schema and API will be common for all other variants
anyway so we can start doing so and see how much time is left when it is done.


> - Weights for backup location servers are the same as the weight defined
> within the backup location itself, so no additional weights are defined
> for backups.

Yes, that was somehow implied in the variant A. Sorry for not mentioning it.
Weight is always relative number for servers inside one location.

-- 
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 0034] ipatests: extend permission plugin test with new expected output

[Martin Basti]

On 25.02.2016 09:52, Milan Kubík wrote:

On 02/24/2016 07:05 PM, Martin Basti wrote:



On 24.02.2016 08:34, Milan Kubík wrote:

On 02/18/2016 03:52 PM, Milan Kubík wrote:

On 02/15/2016 04:59 PM, Milan Kubík wrote:

Patch attached. Applies on ipa-4-3 as well.




Updated version of patch fixes test_old_permission_plugin as well.

--
Milan Kubik



Review bump.

--
Milan Kubik



NACK

[mbasti@dhcp129-96 freeipa-devel]$ git show -U0 | pep8 --diff
./ipatests/test_xmlrpc/test_old_permission_plugin.py:528:80: E501 
line too long (95 > 79 characters)
./ipatests/test_xmlrpc/test_old_permission_plugin.py:586:1: E101 
indentation contains mixed spaces and tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:586:1: W191 
indentation contains tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:587:1: E101 
indentation contains mixed spaces and tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:587:80: E501 
line too long (95 > 79 characters)
./ipatests/test_xmlrpc/test_old_permission_plugin.py:591:1: E101 
indentation contains mixed spaces and tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:591:1: W191 
indentation contains tabs
./ipatests/test_xmlrpc/test_permission_plugin.py:821:80: E501 line 
too long (99 > 79 characters)
./ipatests/test_xmlrpc/test_permission_plugin.py:884:80: E501 line 
too long (99 > 79 characters)



Sorry for that. Updated patch attached.

--
Milan Kubik

ACK

Pushed to:
master: b32c9d639ef8e3fa852fb07f9385ae7e7b48e00e
ipa-4-3: 5ae12641426427552406af89feb15e9bcbad8db3

-- 
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] [REVIEW] Intial stab towards Authentication Indicators

[Simo Sorce]

On Thu, 2016-02-25 at 10:32 -0500, Nathaniel McCallum wrote:
> On Wed, 2016-02-24 at 09:55 -0500, Nathaniel McCallum wrote:
> > On Sun, 2016-02-21 at 20:50 -0500, Simo Sorce wrote:
> > > 
> > > On Sun, 2016-02-21 at 20:20 -0500, Nathaniel McCallum wrote:
> > > > 
> > > > 
> > > > https://github.com/npmccallum/freeipa/pull/1
> > > > 
> > > > The above (pseudo) pull request contains four patches against
> > > > FreeIPA
> > > > to enable the insertion of Authentication Indicators into
> > > > Kerberos
> > > > tickets. The basic flow looks like this.
> > > > 
> > > > First, we patch ipa-pwd-extop to return a control indicating what
> > > > authentication method succeeded resulting in a successful bind.
> > > > 
> > > > Second, we patch ipa-otpd to check the returned control to ensure
> > > > that
> > > > the bind resulted from an otp validation.
> > > > 
> > > > Third, we patch ipa-kdb to enable the KDC to return either the
> > > > encrypted timestamp or encrypted challenge preauth mechanism when
> > > > the
> > > > user is configured for optional 2FA logins. Clients can then
> > > > decide
> > > > whether to do 1FA or 2FA login (for kinit, sane behavior already
> > > > exists).
> > > > 
> > > > Forth, we patch ipa-kdb again to insert hard-coded authentication
> > > > indicators for either OTP or RADIUS.
> > > > 
> > > > Some explanation is required for the first two patches.
> > > > Currently,
> > > > it
> > > > is possible to do a 1FA through the otp preauthentication
> > > > mechanism
> > > > if
> > > > the user is configured for doing optional 2FA. However, because
> > > > we
> > > > want
> > > > to insert an authentication indicator in this code path, we need
> > > > to
> > > > guarantee that a request going through the otp preauth mechanism
> > > > actually validates an OTP. This is the purpose of the control.
> > > > 
> > > > Items still on the TODO list:
> > > > 
> > > >   * Authentication Indicator enforcement
> > > > - Upstream libkrb5 needs to grow funcs for reading indicators
> > > > - Schema change to add indicators multi-value attr to
> > > > services
> > > > - ipa-kdb needs to implement check_policy_tgs()
> > > > 
> > > > 
> > > >   * SSSD needs to learn to handle optional 2FA
> > > > 
> > > > I will write up a project page for all of this tomorrow. But this
> > > > small
> > > > code basically amounts to my brainstorming. It is not ready for
> > > > merge,
> > > > just basic review.
> > > > 
> > > It looks mostly ok, however the LDAP control part needs to be done
> > > as
> > > a
> > > request/response pair.
> > > A client that wishes to know what kind of authentication happened
> > > should
> > > send a request control, and only in that case , the server will
> > > send
> > > the
> > > associated reply control with the requested information.
> > I just pushed a new version of the control (now merged into a single
> > patch): https://github.com/npmccallum/freeipa/commit/a78191ee5d31e1de
> > 39
> > f28eb637f66199da7e9225
> > 
> > In this version the client sends a critical control with no content
> > indicating that the server must validate an OTP. If the LDAP server
> > doesn't support the control (for whatever reason), bind will fail. If
> > the LDAP server doesn't validate an OTP (for whatever reason), bind
> > will fail.
> > 
> > This approach is simpler and doesn't require a request/response
> > control
> > pair.
> 
> I need some design advice. My goal here is that we need a way to expose
> the authentication indicators to services in the FreeIPA UI/CLI.
> 
> Here is the good news: users can already set these values in FreeIPA
> using kadmin. They do this by simply setting the require_auth string on
> the target service principal. Our kdb plugin then encodes these with
> the rest of the tl_data into the krbExtraData attribute.
> 
> I see two approaches here. First, we can try to manipulate the
> krbExtraData attribute directly. Second, we can create a separate
> attribute for the authentication indicator strings and then synthesize
> the tl_data internally in kdb. We would have to do this for both reads
> and writes so as not to break existing kdb functionality.
> 
> The trade-off that I see is that the first method complicates the
> python framework side where the second method complicates the kdb
> plugin.
> 
> A third option, which I doubt is even possible, is to use kadmin to
> manipulate this option rather than modifying LDAP directly.
> 
> Thoughts?

We should translate it, we need that to allow to delegate access only to
the specific attribute via our standard means.

We already do this for other tl_data entries.

The krbExtraData access cannot always be delegated because it would be
open ended. also it is really obnoxious to have to manipulate ASN.1
stuff in the framework.

kadmin could be used at some point, but we'd still want to have this
attribute extracted in order to be able to grant access control
individually, as our ACL system and delegation system is more fine

Re: [Freeipa-devel] [REVIEW] Intial stab towards Authentication Indicators

[Nathaniel McCallum]

On Wed, 2016-02-24 at 09:55 -0500, Nathaniel McCallum wrote:
> On Sun, 2016-02-21 at 20:50 -0500, Simo Sorce wrote:
> > 
> > On Sun, 2016-02-21 at 20:20 -0500, Nathaniel McCallum wrote:
> > > 
> > > 
> > > https://github.com/npmccallum/freeipa/pull/1
> > > 
> > > The above (pseudo) pull request contains four patches against
> > > FreeIPA
> > > to enable the insertion of Authentication Indicators into
> > > Kerberos
> > > tickets. The basic flow looks like this.
> > > 
> > > First, we patch ipa-pwd-extop to return a control indicating what
> > > authentication method succeeded resulting in a successful bind.
> > > 
> > > Second, we patch ipa-otpd to check the returned control to ensure
> > > that
> > > the bind resulted from an otp validation.
> > > 
> > > Third, we patch ipa-kdb to enable the KDC to return either the
> > > encrypted timestamp or encrypted challenge preauth mechanism when
> > > the
> > > user is configured for optional 2FA logins. Clients can then
> > > decide
> > > whether to do 1FA or 2FA login (for kinit, sane behavior already
> > > exists).
> > > 
> > > Forth, we patch ipa-kdb again to insert hard-coded authentication
> > > indicators for either OTP or RADIUS.
> > > 
> > > Some explanation is required for the first two patches.
> > > Currently,
> > > it
> > > is possible to do a 1FA through the otp preauthentication
> > > mechanism
> > > if
> > > the user is configured for doing optional 2FA. However, because
> > > we
> > > want
> > > to insert an authentication indicator in this code path, we need
> > > to
> > > guarantee that a request going through the otp preauth mechanism
> > > actually validates an OTP. This is the purpose of the control.
> > > 
> > > Items still on the TODO list:
> > > 
> > >   * Authentication Indicator enforcement
> > > - Upstream libkrb5 needs to grow funcs for reading indicators
> > > - Schema change to add indicators multi-value attr to
> > > services
> > > - ipa-kdb needs to implement check_policy_tgs()
> > > 
> > > 
> > >   * SSSD needs to learn to handle optional 2FA
> > > 
> > > I will write up a project page for all of this tomorrow. But this
> > > small
> > > code basically amounts to my brainstorming. It is not ready for
> > > merge,
> > > just basic review.
> > > 
> > It looks mostly ok, however the LDAP control part needs to be done
> > as
> > a
> > request/response pair.
> > A client that wishes to know what kind of authentication happened
> > should
> > send a request control, and only in that case , the server will
> > send
> > the
> > associated reply control with the requested information.
> I just pushed a new version of the control (now merged into a single
> patch): https://github.com/npmccallum/freeipa/commit/a78191ee5d31e1de
> 39
> f28eb637f66199da7e9225
> 
> In this version the client sends a critical control with no content
> indicating that the server must validate an OTP. If the LDAP server
> doesn't support the control (for whatever reason), bind will fail. If
> the LDAP server doesn't validate an OTP (for whatever reason), bind
> will fail.
> 
> This approach is simpler and doesn't require a request/response
> control
> pair.

I need some design advice. My goal here is that we need a way to expose
the authentication indicators to services in the FreeIPA UI/CLI.

Here is the good news: users can already set these values in FreeIPA
using kadmin. They do this by simply setting the require_auth string on
the target service principal. Our kdb plugin then encodes these with
the rest of the tl_data into the krbExtraData attribute.

I see two approaches here. First, we can try to manipulate the
krbExtraData attribute directly. Second, we can create a separate
attribute for the authentication indicator strings and then synthesize
the tl_data internally in kdb. We would have to do this for both reads
and writes so as not to break existing kdb functionality.

The trade-off that I see is that the first method complicates the
python framework side where the second method complicates the kdb
plugin.

A third option, which I doubt is even possible, is to use kadmin to
manipulate this option rather than modifying LDAP directly.

Thoughts?

-- 
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] Locations design v2: LDAP schema & user interface

[Simo Sorce]

On Thu, 2016-02-25 at 15:54 +0100, Petr Spacek wrote:
> On 25.2.2016 15:28, Simo Sorce wrote:
> > On Thu, 2016-02-25 at 14:45 +0100, Petr Spacek wrote:
> >> Variant C
> >> -
> >> An alternative is to be lazy and dumb. Maybe it would be enough for
> >> the first
> >> round ...
> >>
> >> We would retain
> >> [first step - no change from variant A]
> >> * create locations
> >> * assign 'main' (aka 'primary' aka 'home') servers to locations
> >> ++ specify weights for the 'main' servers in given location, i.e.
> >> manually
> >> input (server, weight) tuples
> >>
> >> Then, backups would be auto-generated set of all remaining servers
> >> from all
> >> other locations.
> >>
> >> Additional storage complexity: 0
> >>
> >> This covers the scenario "always prefer local servers and use remote
> >> only as
> >> fallback" easily. It does not cover any other scenario.
> >>
> >> This might be sufficient for the first run and would allow us to
> >> gather some
> >> feedback from the field.
> >>
> >> Now I'm inclined to this variant :-)
> > 
> > To be honest, this is all I always had in mind, for the first step.
> > 
> > To recap:
> > - define a location with the list of servers (perhaps location is a
> > property of server objects so you can have only one location per server,
> > and if you remove the server it is automatically removed from the
> > location w/o additional work or referential integrity necessary), if
> > weight is not defined (default) then they all have the same weight.
> 
> Agreed.
> 
> 
> > - Allow to specify backup locations in the location object, priorities
> > are calculated automatically and all backup locations have same weight.
> 
> Hmm, weights have to be inherited form the original location in all cases. Did
> you mean that all backup locations have the same *priority*?

Yes, sorry.

> Anyway, explicit configuration of backup locations is introducing API and
> schema for variant A and that is what I'm questioning above. It is hard to
> make it extensible so we do not have headache in future when somebody decides
> that more flexibility is needed OR that link-based approach is better.

I think no matter we do we'll need to allow admins to override backup
locations, in future if we can calculate them automatically admins will
simply not set any backup location explicitly (or set some special value
like "autogenerate" and the system will do it for them.

Forcing admins to mentally calculate weights to force the system to
autogenerate the configuration they want would be a bad experience, I
personally would find it very annoying.

> In other words, for doing what you propose above we would have to design
> complete schema and API for variant A anyway to make sure we do not lock
> ourselves, so we are not getting any saving by doing so.

A seemed much more complicated to me, as you wanted to define a ful
matrix for weights of servers when they are served as backups and all
that.

> > - Define a *default* location, which is the backup for any other
> > location but always with lower priority to any other explicitly defined
> > backup locations.
> 
> I would rather *always* use the default location as backup for all other
> locations. It does not require any API or schema (as it equals to "all
> servers" except "servers in this location" which can be easily calculated on 
> fly).

We can start with this, but it works well only in a stellar topology
where you have a central location all other location connect to.
As soon as you have a super-stellar topology where you have hub location
to which regional locations connect to, then this is wasteful.

> This can be later on extended in whatever direction we want without any
> upgrade/migration problem.
> 
> More importantly, all the schema and API will be common for all other variants
> anyway so we can start doing so and see how much time is left when it is done.

I am ok with this for the first step.
After all location is mostly about the "normal" case where clients want
to reach the local servers, the backup part is only an additional
feature we can keep simple for now. It's a degraded mode of operation
anyway so it is probably ok to have just one default backup location as
a starting point.

> > - Weights for backup location servers are the same as the weight defined
> > within the backup location itself, so no additional weights are defined
> > for backups.
> 
> Yes, that was somehow implied in the variant A. Sorry for not mentioning it.
> Weight is always relative number for servers inside one location.

Ok it looked a lot more complex from your description.

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