Re: [Freeipa-devel] [PATCH] cert-show: show subject alternative names

2016-07-18 Thread Jan Cholasta 

Hi,

On 14.7.2016 13:44, Fraser Tweedale wrote:

Hi all,

The attached patch includes SANs in cert-show output.  If you have
certs with esoteric altnames (especially any that are more than just
ASN.1 string types), please test with those certs.

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


I think it would be better to have a separate attribute for each 
supported SAN type rather than cramming everything into 
subject_alt_name. That way if you care only about a single specific type 
you won't have to go through all the values and parse them. Also it 
would allow you to use param types appropriate to the SAN types 
(DNSNameParam for DNS names, Principal for principal names, etc.)


Nitpick: please don't mix moving existing stuff and adding new stuff in 
a single patch.


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] 0001: Silence sshd messages during install

2016-07-18 Thread Jan Cholasta 

Hi,

On 9.7.2016 14:46, Ben Lipton wrote:

On 07/07/2016 11:19 AM, Ben Lipton wrote:


Thanks for the review! Comments below.


On 07/01/2016 07:42 AM, Martin Basti wrote:




On 29.06.2016 20:46, Ben Lipton wrote:

The attached patch silences some annoying messages I've been getting
when upgrading the freeipa-client package on F24:
"""
WARNING: 'UseLogin yes' is not supported in Fedora and may cause
several problems.

This will be fixed by openssh-7.2p2-9.fc24
(https://bugzilla.redhat.com/show_bug.cgi?id=1350347) so we probably
shouldn't worry about it.

Could not load host key: /etc/ssh/ssh_host_dsa_key

This is because by default sshd looks for all of
/etc/ssh/ssh_host_dsa_key, /etc/ssh/ssh_host_ecdsa_key,
/etc/ssh/ssh_host_ed25519_key and /etc/ssh/ssh_host_rsa_key, but
Fedora doesn't generate a DSA key by default.

"""

Since the script causing the message only looks at the return code
from sshd to determine the right options to use, I thought it might
be ok to discard the output. What do you think?

Ben




Hello, I don't like to hiding errors/warnings. Can you determine and
solve the root cause?


I definitely agree with this in principle, but in this case the
purpose of this code is to try different, potentially wrong,
parameters to sshd until it finds a combination that it accepts. It
seems like in some environments this would produce error messages that
aren't actionable and don't indicate any problem for package function,
which is why I didn't think these messages were necessarily worth
preserving.

On the other hand, if the code makes the wrong decision about sshd
version we might be interested in error logs that show why. Can we log
this to a file instead of the console, maybe?

If you'd prefer just addressing the root cause, a patch that prevents
the missing host key error is attached, but it won't stop the error
messages showing up when openssh is an older version.

Thanks,
Ben



Whoops, realized that my patch created a tempfile and didn't delete it.
Updated.


I think the first version of the patch was OK. sshd is called only to 
check which set of authorized keys options to use, we don't really care 
about anything else, so we can safely ignore whatever it puts to stderr.


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] [freeipa] #6002: Default CA can be used without an ACL

2016-07-18 Thread Jan Cholasta 

Hi,

On 4.7.2016 09:06, Fraser Tweedale wrote:

On Tue, Jun 28, 2016 at 01:47:23PM -, freeipa wrote:

#6002: Default CA can be used without an ACL

Comment (by ftweedal):

 This is expected behaviour; if a CA ACL does not reference any CAs,
 and does not have cacat=all, then it is assumed to refer to the
 default CA.  This is for backwards compatibility with existing
 CA ACLs, which do not reference any CAs but did (and still do)
 allow access to IPA CA.

 Leaving open for discussion about whether to break compatibility
 for a more consistent behaviour.


Didn't get any feedback in the ticket yet so raising on list for
visibility.  If people agree with current behaviour I can add a
clarification to caacl plugin help text and close out this ticket.


(Sorry for the late reply, I was on vacation the last 2 weeks.)

I would very much prefer if this was consistent with (literally) every 
other member list+category attribute, that is, no member and no category 
means the rule never matches.


While documenting this as an exception to the above rule is the easy way 
out, IMHO adhering to the rule is even better - anyone who touched HBAC 
or sudo in IPA would immediately know their way around CA ACLs without 
having to read the documentation at all, which is a win, because people 
don't generally read documentation until something goes wrong. The 
current behavior might surprise them, even if documented properly (it 
sure surprised me at first :-).


BTW I think this can be done without breaking compatibility, e.g. by 
using a new objectclass to distinguish between "old" (CA is always 
implicitly the top-level CA) and "new" (CAs are specified using the 
member and category attributes) CA ACLs.


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] 963 unite log file name of ipa-ca-install

2016-07-18 Thread Jan Cholasta 

Hi,

On 18.7.2016 18:50, Florence Blanc-Renaud wrote:

On 07/15/2016 04:29 PM, Petr Vobornik wrote:

ipa-ca-install said that it used
  /var/log/ipareplica-ca-install.log
but in fact it used
  /var/log/ipaserver-ca-install.log

This patch unites it to ipaserver-ca-install.log

It was chosen because ipa-ca-install can be also used on
master on CA-less -> CA conversion.

Term "server" is valid for both master and replica.

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





Looks good to me.
Ack


Does not look so good to me, "ipareplica-ca-install.log" is in fact the 
original file name used since ipa-ca-install was introduced (in commit 
8a32bb3746802a29b2655e4ad2cbbba8481e1eaf), so why the switch to 
"ipaserver-ca-install.log"?


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] 0089 caacl: expand plugin documentation

2016-07-18 Thread Fraser Tweedale 
On Mon, Jul 18, 2016 at 09:55:21AM +0200, Martin Basti wrote:
> 
> 
> On 13.07.2016 18:34, Petr Vobornik wrote:
> > On 07/12/2016 08:45 AM, Alexander Bokovoy wrote:
> > > On Tue, 12 Jul 2016, Fraser Tweedale wrote:
> > > > Attached patch is a doc change, addressing
> > > > https://fedorahosted.org/freeipa/ticket/6002.
> > > > 
> > > > Thanks,
> > > > Fraser
> > > >  From 19c5fc60391d37c9d0500feb5d5d5a6628bc4d27 Mon Sep 17 00:00:00 2001
> > > > From: Fraser Tweedale 
> > > > Date: Tue, 12 Jul 2016 15:11:11 +1000
> > > > Subject: [PATCH] caacl: expand plugin documentation
> > > > 
> > > > Expand the 'caacl' plugin documentation to explain some common
> > > > confusions including the fact that CA ACLs apply to the target
> > > > subject principal (not necessarily the principal requesting the
> > > > cert), and the fact that CA-less CA ACL implies the 'ipa' CA.
> > > > 
> > > > Fixes: https://fedorahosted.org/freeipa/ticket/6002
> > > > ---
> > > > ipaserver/plugins/caacl.py | 34 --
> > > > 1 file changed, 28 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/ipaserver/plugins/caacl.py b/ipaserver/plugins/caacl.py
> > > > index
> > > > 9a60f7e27809c4f41b160647efafde94dbe90bf0..d316cc7c48cf2997d6be6b052dc1efa6d6fcdb6a
> > > > 100644
> > > > --- a/ipaserver/plugins/caacl.py
> > > > +++ b/ipaserver/plugins/caacl.py
> > > > @@ -23,14 +23,36 @@ if six.PY3:
> > > > __doc__ = _("""
> > > > Manage CA ACL rules.
> > > > 
> > > > -This plugin is used to define rules governing which principals are
> > > > -permitted to have certificates issued using a given certificate
> > > > -profile.
> > > > +This plugin is used to define rules governing which CAs and profiles
> > > > +may be used to issue certificates to particular principals or groups
> > > > +of principals.
> > > > 
> > > > -PROFILE ID SYNTAX:
> > > > +SUBJECT PRINCIPAL SCOPE:
> > > > 
> > > > -A Profile ID is a string without spaces or punctuation starting with
> > > > a letter
> > > > -and followed by a sequence of letters, digits or underscore ("_").
> > > > +For a certificate request to be allowed, the principal(s) that are
> > > > +the subject of a certificate request (not necessarily the principal
> > > > +actually requesting the certificate) must be included in the scope
> > > > +of a CA ACL that also includes the target CA and profile.
> > > > +
> > > > +Users can be included by name, group or the "all users" category.
> > > > +Hosts can be included by name, hostgroup or the "all hosts"
> > > > +category.  Services can be included by service name or the "all
> > > > +services" category.  CA ACLs may be associated with a single type of
> > > > +principal, or multiple types.
> > > > +
> > > > +CERTIFICATE AUTHORITY SCOPE:
> > > > +
> > > > +A CA ACL can be associated with one or more CAs by name, or by the
> > > > +"all CAs" category.  For compatibility reasons, a CA ACL with no CA
> > > > +association implies an association with the 'ipa' CA (and only this
> > > > +CA).
> > > > +
> > > > +PROFILE SCOPE:
> > > > +
> > > > +A CA ACL can be associated with one or more profiles by Profile ID.
> > > > +The Profile ID is a string without spaces or punctuation starting
> > > > +with a letter and followed by a sequence of letters, digits or
> > > > +underscore ("_").
> > > > 
> > > > EXAMPLES:
> > > > 
> > > ACK. Reads well.
> > > 
> > Pushed to master: 8cd87d12d53a98a8e386c06a7c5fddb1d38d990d
> > 
> Please note for future, that long string should be splitted, to make life of
> translators easier
> 
> http://www.freeipa.org/page/Coding_Best_Practices#Split_long_translatable_strings
> 
> Martin^2
>
I see; thanks for pointing this out Martin.

-- 
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 0003] Fix several small typos

2016-07-18 Thread Lukas Slebodnik 
On (18/07/16 16:38), Petr Spacek wrote:
>On 14.7.2016 16:11, Ben Lipton wrote:
>> On 07/14/2016 04:09 AM, Alexander Bokovoy wrote:
>>> On Wed, 13 Jul 2016, Ben Lipton wrote:
 Nothing too exciting, just fixes a few typos I've noticed in comments.
>>> ACK. However, please file a ticket and mention it in the commit message.
>
>Is it worth the bureaucracy? I do not think so. We will certainly not backport
>typo fixes in comments to older branches so I would say that ticket is useless.
>
>Just my two cents.
>
+1

LS

-- 
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] Using RPZ to overcome multi Kerberos domains and multiple DNS authorities.

2016-07-18 Thread Jim Glenz 
IPA DNS configuration using Response Policy Zone (RPZ).

IPA utilizes DNS extensively to locate service records (SRV) and text
records (TXT) associated with the Kerberos realm.
IPA also heavily require DNS A records and PTR records to function
correctly.
Normally all A,SRV,TXT,PTR records are part of the same DNS domain zone.

The following shows how to decouple IPA "TXT and SRV" records only, and
pass (forward) all other records to another internal DNS server when
required to have all records (except SRV and TXT) records in the other DNS
system.

Note: Below is very customized for specific environment, your environment
may be different. Just wanted to pass on this DNS trick.
Methodology used was to implement a BIND instance on at least two servers
and then configuring a Response Policy Zone (RPZ).
The RPZ is configured to respond to specific DNS records and forward other
DNS records onward to next hop DNS.

All A and PTR records should exist in the next hop DNS authoritative server.
As mentioned, the following solution is very specific to a unique
environment.

IPA members and clinet servers must have their primary/secondary DNS
resolvers set to the DNS RPZ BIND servers.


Steps
 Create your Master and Slave Bind DNS where RPZ will be used (can be your
IPA server or any other server having Bind DNS installed)
 Create Response Policy Zone (RPZ) files.
 Test configuration.

Search below for " 
nslookup  

nslookup . 
nslookup . 

nslookup  
nslookup . 
nslookup -type=TXT _kerberos.  
nslookup -type=SRV _kerberos-master._tcp.  
nslookup -type=SRV _kerberos-master._udp.  
nslookup -type=SRV _kerberos._tcp. 
nslookup -type=SRV _kerberos._udp. 
nslookup -type=SRV _kpasswd._tcp.  
nslookup -type=SRV _kpasswd._udp.  
nslookup -type=SRV _ldap._tcp. 

nslookup  
nslookup . 
nslookup -type=TXT _kerberos.  
nslookup -type=SRV _kerberos-master._tcp.  
nslookup -type=SRV _kerberos-master._udp.  
nslookup -type=SRV _kerberos._tcp. 
nslookup -type=SRV _kerberos._udp. 
nslookup -type=SRV _kpasswd._tcp.  
nslookup -type=SRV _kpasswd._udp.  
nslookup -type=SRV _ldap._tcp. 

nslookup google.com 
nslookup google.com 

nslookup -type=ptr  
nslookup -type=ptr  
nslookup -type=ptr  
nslookup -type=ptr  


Will be referencing reverse.arpa zone 10.x.x.x internal network. Adjust as
necessary for your environment.

Appendix A: Primary IPA DNS /etc/named.conf file
# cat named.conf
options {
// Put files that named is allowed to write in the data/ directory:
directory "/var/named"; // the default
dump-file   "data/cache_dump.db";
statistics-file "data/named_stats.txt";
memstatistics-file  "data/named_mem_stats.txt";
#forward first;
forwarders {
;
;
};
response-policy {zone ""; };

// Any host is permitted to issue recursive queries
allow-recursion { any; };

  tkey-gssapi-credential "DNS/";
  tkey-domain "";
};

/* If you want to enable debugging, eg. using the 'rndc trace' command,
 * By default, SELinux policy does not allow named to modify the /var/named
directory,
 * so put the default debug log file in data/ :
 */
logging {
channel default_debug {
file "data/named.run";
severity dynamic;
};
};

zone "" IN {
type master;
file "";
also-notify {;};
};

zone "10.in-addr.arpa" IN {
 type forward;
forwarders {
;
;
};
};

include "/etc/named.rfc1912.zones";

# dynamic not used, remark out.
#dynamic-db "ipa" {
#library "ldap.so";
#};

Appendix B: Primary IPA DNS /var/named/ file
$ORIGIN .
$TTL 86400  ; 1 day
.rpz IN SOA  localhost. root.localhost. (
201505162150 ; serial
3600   ; refresh (1 hour)
1800   ; retry (30 minutes)
604800 ; expire (1 week)
86400  ; minimum (1 day)
)
NS  localhost.
$ORIGIN .rpz.
_kerberos   TXT ""
_ntp_udpSRV 0 100 123 ".
$ORIGIN _tcp..rpz.
_kerberos   SRV 0 100 88 ".
SRV 0 100 88 ".
_kerberos-masterSRV 0 100 88 ".
SRV 0 100 88 ".
_kpasswdSRV 0 100 464 ".
SRV 0 100 464 ".
_ldap   SRV 0 100 389 ".
SRV 0 100 389 ".
$ORIGIN _udp..rpz.
_kerberos   SRV 0 100 88 ".
SRV 0 100 88 ".
_kerberos-masterSRV 0 100 88 ".
SRV 0 100 88 ".
_kpasswdSRV 0 100 464 ".
SRV

Re: [Freeipa-devel] [PATCH] 963 unite log file name of ipa-ca-install

2016-07-18 Thread Florence Blanc-Renaud 

On 07/15/2016 04:29 PM, Petr Vobornik wrote:

ipa-ca-install said that it used
  /var/log/ipareplica-ca-install.log
but in fact it used
  /var/log/ipaserver-ca-install.log

This patch unites it to ipaserver-ca-install.log

It was chosen because ipa-ca-install can be also used on
master on CA-less -> CA conversion.

Term "server" is valid for both master and replica.

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





Looks good to me.
Ack

--
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 0026][Tests] RFE: Support UPN for trusted domains

2016-07-18 Thread Martin Babinsky 

On 07/14/2016 03:39 PM, Lenka Doudova wrote:



On 07/13/2016 06:04 PM, Martin Babinsky wrote:

On 07/01/2016 04:45 PM, Lenka Doudova wrote:



On 07/01/2016 03:04 PM, Martin Babinsky wrote:

On 07/01/2016 11:13 AM, Lenka Doudova wrote:

And, of course, a patch file :)


On 07/01/2016 11:09 AM, Lenka Doudova wrote:

Hi all,

here's patch with basic test suite for support of UPN.

Note: it needs to be applied on top of my patch 0025.2 (or later, if
there's will be more fixes to that patch).


Lenka







Hi Lenka,

test data such as usernames, etc. should be stored either in separate
resource files or at least as class attributes like this:

diff --git a/ipatests/test_integration/test_trust.py
b/ipatests/test_integration/test_trust.py
index e8fdc6b..86ba7cc 100644
--- a/ipatests/test_integration/test_trust.py
+++ b/ipatests/test_integration/test_trust.py
@@ -394,28 +394,33 @@ class TestTrustWithUPN(ADTrustBase):
 """
 Test support of UPN for trusted domains
 """
+upn_suffix = 'UPNsuffix.com'
+upn_username = 'upnuser'
+upn_princ = '{}@{}'.format(upn_username, upn_suffix)
+upn_password = 'Secret123456'
+
 def test_upn_in_nonposix_trust(self):
 """ Check that UPN is listed as trust attribute """
 result = self.master.run_command(['ipa', 'trust-show',
self.ad_domain,
   '--all', '--raw'])

-assert "ipantadditionalsuffixes: UPNsuffix.com" in
result.stdout_text
+assert ("ipantadditionalsuffixes:
{}".format(self.upn_suffix) in
+result.stdout_text)

 def test_upn_user_resolution_in_nonposix_trust(self):
 """ Check that user with UPN can be resolved """
-upnuser = 'upnu...@upnsuffix.com'
-result = self.master.run_command(['getent', 'passwd',
upnuser])
+result = self.master.run_command(['getent', 'passwd',
self.upn_princ])

 # result will contain AD domain, not UPN
-upnuser_regex = "^upnuser@{0}:\*:(\d+):(\d+):UPN
User:/:$".format(
-self.ad_domain)
+upnuser_regex = "^{}@{}:\*:(\d+):(\d+):UPN User:/:$".format(
+self.upn_username, self.ad_domain)
 assert re.search(upnuser_regex, result.stdout_text)

 def test_upn_user_authentication(self):
 """ Check that AD user with UPN can authenticate in IPA """
 self.master.run_command(['systemctl', 'restart', 'krb5kdc'])
-self.master.run_command(['kinit', '-C', '-E',
'upnu...@upnsuffix.com'],
-stdin_text='Secret123456')
+self.master.run_command(['kinit', '-C', '-E', self.upn_princ],
+stdin_text=self.upn_password)

otherwise LGTM.


Thanks for review, fixed patch attached.

Few notes:
1. mbabinsky's suggestion to store testdata as class attributes or
separate resource file: I decided to use the class attribute approach.
The separate resource file is a nice idea, which I have already put on
my "to do" list - there's a lot of hardcoded stuff in the trust tests,
even in the original ones (before my patches), so when there's time I'll
work on a way how to dynamically provide this data as test configuration
2. previous discussion about getent vs. pwd.getpwnam(): I'll leave the
getent command, since according to mbasti the alternative would not work
in CI.

Lenka


Hi Lenka,

I am not sure 'test_all_trustdomains_found' should be run as a part of
this test suite. Maybe yes, I'm not sure.

Also I would add a 60 second sleep after KDC restart in
'test_upn_user_authentication' so that MS-PAC cache gets refreshed
before trying to kinit as enterprise principal.

Two of the tests fail on my setup but that is probably due to
https://fedorahosted.org/freeipa/ticket/6082 .


Hi,

the "test_all_trustdomains_found" method is inherited from parent class,
and I believe it cannot hurt to have it there.
I added the sleep as you requested.
I also tried to run the tests with two way trust and since everything
was fine then, the failures you experienced are really most likely due
to the oddjob issue you linked. Of course the patch still contains tests
with one way trusts, so until the oddjob issue is solved, the tests will
probably fail, and should be fine once the fix is provided.

Fixed patch attached.
Lenka


Yes I think that the failing tests are due to bugs in trust, not the 
test code. ACK.


--
Martin^3 Babinsky

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

Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust

2016-07-18 Thread Martin Babinsky 

On 07/18/2016 04:59 PM, Lenka Doudova wrote:



On 07/18/2016 04:55 PM, Martin Babinsky wrote:

On 07/14/2016 11:42 AM, Lenka Doudova wrote:



On 07/13/2016 05:40 PM, Martin Babinsky wrote:

On 07/01/2016 04:15 PM, Lenka Doudova wrote:



On 07/01/2016 02:38 PM, Martin Babinsky wrote:

On 07/01/2016 06:36 AM, Lenka Doudova wrote:



On 06/30/2016 05:01 PM, Martin Babinsky wrote:

On 06/30/2016 03:47 PM, Lenka Doudova wrote:

Hi,

attaching patch with some basic coverage for external trust
feature. Bit
more detailed info in commit message.

Since the feature requires me to run commands previously used
only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable
me to
reuse existing function without copy-pasting them for one variable
change.


Lenka





Hi Lenka,

I have a few comments:

1.)

-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(),
subdomain=False):

This modification seems extremely kludgy to me.

I would rather change the function signature to:

-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):

and pass the domain name itself to the function instead of doing
magic
inside. The same goes with `remove trust_with_ad`. You may want to
fix
the calls to them in existing code so that the domain is passed
instead of the whole trust object.

2.)

+def sync_time_hostname(host, srv_hostname):
+"""
+Syncs time with the remote server given by its hostname.
+Please note that this function leaves ntpd stopped.
+"""
+host.run_command(['systemctl', 'stop', 'ntpd'])
+host.run_command(['ntpdate', srv_hostname])
+
+

this looks like a duplicate of the function above it, why is it
even
needed?

3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+"""
+Test establishing external trust with subdomain
+"""
+
+@classmethod
+def install(cls, mh):
+super(ADTrustBase, cls).install(mh)
+cls.ad = cls.ad_domains[0].ads[0]
+cls.install_adtrust()
+cls.check_sid_generation()
+
+# Determine whether the subdomain AD is available
+try:
+child_ad =
cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+cls.ad_subdomain = None
+
+cls.configure_dns_and_time()

Please use proper OOP practices when overriding the behavior of the
base test class.

For starters you could rewrite the install method like this:

+@classmethod
+def install(cls, mh):
+# Determine whether the subdomain AD is available
+try:
+cls.child_ad =
cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+raise nose.SkipTest("AFAIK this will skip the whole
test
class")
+super(ADTrustBase, cls).install(mh)

with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+def configure_dns_and_time(cls):
+tasks.configure_dns_for_trust(cls.master,
cls.ad_subdomain)
 # no need to re-implement the function just to get to the
child
AD DC hostname now
+tasks.sync_time(cls.master, cls.child_ad.hostname)

You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.


Hi,
thanks for review, fixed patch attached.

Lenka


Hi Lenka,

I am still not happy about the patch underutilizing the potential for
a proper inheritance hierarchy and instead relying on a staggering
amounts of copy-pasting for implementation.

I am attaching a quick untested patch illustrating how the
implementation should look like.

Also you have some leftovers from previous revision, please fix them:

Pylint is running, please wait ...
* Module ipatests.test_integration.test_trust
ipatests/test_integration/test_trust.py:236: [E1101(no-member),
TestExternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
ipatests/test_integration/test_trust.py:243:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_establish_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:279:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:330: [E1101(no-member),
TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time

Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust

2016-07-18 Thread Lenka Doudova 



On 07/18/2016 04:55 PM, Martin Babinsky wrote:

On 07/14/2016 11:42 AM, Lenka Doudova wrote:



On 07/13/2016 05:40 PM, Martin Babinsky wrote:

On 07/01/2016 04:15 PM, Lenka Doudova wrote:



On 07/01/2016 02:38 PM, Martin Babinsky wrote:

On 07/01/2016 06:36 AM, Lenka Doudova wrote:



On 06/30/2016 05:01 PM, Martin Babinsky wrote:

On 06/30/2016 03:47 PM, Lenka Doudova wrote:

Hi,

attaching patch with some basic coverage for external trust
feature. Bit
more detailed info in commit message.

Since the feature requires me to run commands previously used
only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable
me to
reuse existing function without copy-pasting them for one variable
change.


Lenka





Hi Lenka,

I have a few comments:

1.)

-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(),
subdomain=False):

This modification seems extremely kludgy to me.

I would rather change the function signature to:

-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):

and pass the domain name itself to the function instead of doing
magic
inside. The same goes with `remove trust_with_ad`. You may want to
fix
the calls to them in existing code so that the domain is passed
instead of the whole trust object.

2.)

+def sync_time_hostname(host, srv_hostname):
+"""
+Syncs time with the remote server given by its hostname.
+Please note that this function leaves ntpd stopped.
+"""
+host.run_command(['systemctl', 'stop', 'ntpd'])
+host.run_command(['ntpdate', srv_hostname])
+
+

this looks like a duplicate of the function above it, why is it 
even

needed?

3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+"""
+Test establishing external trust with subdomain
+"""
+
+@classmethod
+def install(cls, mh):
+super(ADTrustBase, cls).install(mh)
+cls.ad = cls.ad_domains[0].ads[0]
+cls.install_adtrust()
+cls.check_sid_generation()
+
+# Determine whether the subdomain AD is available
+try:
+child_ad = 
cls.host_by_role(cls.optional_extra_roles[0])

+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+cls.ad_subdomain = None
+
+cls.configure_dns_and_time()

Please use proper OOP practices when overriding the behavior of the
base test class.

For starters you could rewrite the install method like this:

+@classmethod
+def install(cls, mh):
+# Determine whether the subdomain AD is available
+try:
+cls.child_ad =
cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+raise nose.SkipTest("AFAIK this will skip the whole 
test

class")
+super(ADTrustBase, cls).install(mh)

with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+def configure_dns_and_time(cls):
+tasks.configure_dns_for_trust(cls.master, 
cls.ad_subdomain)
 # no need to re-implement the function just to get to the 
child

AD DC hostname now
+tasks.sync_time(cls.master, cls.child_ad.hostname)

You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.


Hi,
thanks for review, fixed patch attached.

Lenka


Hi Lenka,

I am still not happy about the patch underutilizing the potential for
a proper inheritance hierarchy and instead relying on a staggering
amounts of copy-pasting for implementation.

I am attaching a quick untested patch illustrating how the
implementation should look like.

Also you have some leftovers from previous revision, please fix them:

Pylint is running, please wait ...
* Module ipatests.test_integration.test_trust
ipatests/test_integration/test_trust.py:236: [E1101(no-member),
TestExternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
ipatests/test_integration/test_trust.py:243:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_establish_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:279:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:330: [E1101(no-member),
TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
Makefile:137: reci

Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust

2016-07-18 Thread Martin Babinsky 

On 07/14/2016 11:42 AM, Lenka Doudova wrote:



On 07/13/2016 05:40 PM, Martin Babinsky wrote:

On 07/01/2016 04:15 PM, Lenka Doudova wrote:



On 07/01/2016 02:38 PM, Martin Babinsky wrote:

On 07/01/2016 06:36 AM, Lenka Doudova wrote:



On 06/30/2016 05:01 PM, Martin Babinsky wrote:

On 06/30/2016 03:47 PM, Lenka Doudova wrote:

Hi,

attaching patch with some basic coverage for external trust
feature. Bit
more detailed info in commit message.

Since the feature requires me to run commands previously used
only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable
me to
reuse existing function without copy-pasting them for one variable
change.


Lenka





Hi Lenka,

I have a few comments:

1.)

-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(),
subdomain=False):

This modification seems extremely kludgy to me.

I would rather change the function signature to:

-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):

and pass the domain name itself to the function instead of doing
magic
inside. The same goes with `remove trust_with_ad`. You may want to
fix
the calls to them in existing code so that the domain is passed
instead of the whole trust object.

2.)

+def sync_time_hostname(host, srv_hostname):
+"""
+Syncs time with the remote server given by its hostname.
+Please note that this function leaves ntpd stopped.
+"""
+host.run_command(['systemctl', 'stop', 'ntpd'])
+host.run_command(['ntpdate', srv_hostname])
+
+

this looks like a duplicate of the function above it, why is it even
needed?

3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+"""
+Test establishing external trust with subdomain
+"""
+
+@classmethod
+def install(cls, mh):
+super(ADTrustBase, cls).install(mh)
+cls.ad = cls.ad_domains[0].ads[0]
+cls.install_adtrust()
+cls.check_sid_generation()
+
+# Determine whether the subdomain AD is available
+try:
+child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+cls.ad_subdomain = None
+
+cls.configure_dns_and_time()

Please use proper OOP practices when overriding the behavior of the
base test class.

For starters you could rewrite the install method like this:

+@classmethod
+def install(cls, mh):
+# Determine whether the subdomain AD is available
+try:
+cls.child_ad =
cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+raise nose.SkipTest("AFAIK this will skip the whole test
class")
+super(ADTrustBase, cls).install(mh)

with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+def configure_dns_and_time(cls):
+tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
 # no need to re-implement the function just to get to the child
AD DC hostname now
+tasks.sync_time(cls.master, cls.child_ad.hostname)

You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.


Hi,
thanks for review, fixed patch attached.

Lenka


Hi Lenka,

I am still not happy about the patch underutilizing the potential for
a proper inheritance hierarchy and instead relying on a staggering
amounts of copy-pasting for implementation.

I am attaching a quick untested patch illustrating how the
implementation should look like.

Also you have some leftovers from previous revision, please fix them:

Pylint is running, please wait ...
* Module ipatests.test_integration.test_trust
ipatests/test_integration/test_trust.py:236: [E1101(no-member),
TestExternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
ipatests/test_integration/test_trust.py:243:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_establish_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:279:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:330: [E1101(no-member),
TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
Makefile:137: recipe for target 'lint' failed
make: *** [lint] Error 1
sendin

Re: [Freeipa-devel] [PATCH 0003] Fix several small typos

2016-07-18 Thread Petr Spacek 
On 14.7.2016 16:11, Ben Lipton wrote:
> On 07/14/2016 04:09 AM, Alexander Bokovoy wrote:
>> On Wed, 13 Jul 2016, Ben Lipton wrote:
>>> Nothing too exciting, just fixes a few typos I've noticed in comments.
>> ACK. However, please file a ticket and mention it in the commit message.

Is it worth the bureaucracy? I do not think so. We will certainly not backport
typo fixes in comments to older branches so I would say that ticket is useless.

Just my two cents.

-- 
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] 0010 Show full error message for selinuxusermap-add-hostgroup

2016-07-18 Thread Florence Blanc-Renaud 

On 07/18/2016 08:20 AM, Jan Cholasta wrote:

Hi,

On 7.7.2016 16:40, Florence Blanc-Renaud wrote:

On 07/07/2016 01:23 PM, Petr Vobornik wrote:

On 07/05/2016 02:38 PM, Florence Blanc-Renaud wrote:

Hi,

the output of ipa selinuxusermap-add-hostgroup and
selinuxusermap-add-user does not display any more the host/host
group or
user/group that could not be added. This patch fixes this regression by
adding the labels host/hostgroup/user/group to the list of
_failed_member_output_params of the class ClientMethod.


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



I've a feeling that this issue is more general and multiple commands
regressed. Would be good to check other member options, e.g. also in
user plugin.


Hi Petr,

you are right, a lot of other commands regressed. So far I checked only
user and sudocmd but it is likely to be a long task. Are there
regression tests that could help me make sure that the fix is exhaustive?

Flo


See attachment for a patch with an universal fix.

Honza


Hi Honza,

the patch fixes most of the issues. I still see some CLI that do not 
print everything (while they used to before the regression):

ipa servicedelegationrule-add-member
ipa servicedelegationrule-remove-member
ipa servicedelegationtarget-add-member
ipa servicedelegationtarget-remove-member

And the following CLI do not print the failed members (but they never did):
ipa automember-add-condition
ipa automember-remove-condition
ipa sudorule-add-allow-command
ipa sudorule-remove-allow-command
ipa sudorule-add-deny-command
ipa sudorule-remove-deny-command

It is probably ok to commit this patch and investigate in another ticket 
the remaining issues,

Flo.

--
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 190] expose `--secret` option in radiusproxy-* commands

2016-07-18 Thread Martin Babinsky 

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

--
Martin^3 Babinsky
From fd0821d5a55dc71fea650fb0167414fd7702ad25 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 18 Jul 2016 10:45:48 +0200
Subject: [PATCH] expose `--secret` option in radiusproxy-* commands

Option `--secret` was hidden from radiusproxy CLI preventing setting a secret
on existing server or searching by secret. Since thin client implementation it
was also not recognized by the interactive prompt code in CLI frontend since
it never got there.

https://fedorahosted.org/freeipa/ticket/6078
---
 ipaserver/plugins/radiusproxy.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ipaserver/plugins/radiusproxy.py b/ipaserver/plugins/radiusproxy.py
index 44d87b9ae1337278bb6237d471f64693b0eac3db..5657e002c1ce66335b7697b98f95a49207c61d87 100644
--- a/ipaserver/plugins/radiusproxy.py
+++ b/ipaserver/plugins/radiusproxy.py
@@ -126,7 +126,6 @@ class radiusproxy(LDAPObject):
 label=_('Secret'),
 doc=_('The secret used to encrypt data'),
 confirm=True,
-flags=['no_option'],
 ),
 Int('ipatokenradiustimeout?',
 cli_name='timeout',
-- 
2.7.4

-- 
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] 0210 frontend: fix output validation for multiple type choices

2016-07-18 Thread Martin Babinsky 

On 07/18/2016 12:29 PM, Martin Babinsky wrote:

On 07/18/2016 10:01 AM, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, Alexander Bokovoy wrote:

Hi,

I had some time and was blocked by these bugs to do my tickets so I
actually fixed these three problems that are assigned to Martin
Babinsky. Hopefully, Martin wouldn't be offended by that. :)

--

Output entry elements may have multiple types allowed. We need to check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),) tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show
(and automember-set-default in #6037) claims to return the primary key
of the object in the 'value' output field, but the object does not have
a primary key, so the client rightfully expects None.

A proper fix would be to set "has_output = output.simple_value" for
these commands (all of automember_default_group_{set,remove,show},
trustconfig_{mod,show}).

Honza



The problem is that these commands do not return a simple boolean in
'result' but a full entry dict, so 'simple_value' won't do the trick in
this case.

But I agree, we should rather fix misbehaving commands rather than bend
the framework to accomodate their idiosyncracies, we have enough of that
already.

I am attaching a patch that adds a custom shim for misbehaving commands 
so that they work as before. There is also a big fat warning added to 
discourage implementation of such commands.


--
Martin^3 Babinsky
From 1f3dd199bb1d0c02c70d099884d13dde6277253c Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 18 Jul 2016 13:18:44 +0200
Subject: [PATCH] allow 'value' output param in commands without primary key

`PrimaryKey` output param works only for API objects that have primary keys,
otherwise it expects None (nothing is associated with this param). Since the
validation of command output was tightened durng thin client effort, some
commands not honoring this contract began to fail output validation.

A custom output was implemented for them to restore their functionality. It
should however be considered as a fix for broken commands and not used
further.

https://fedorahosted.org/freeipa/ticket/6037
https://fedorahosted.org/freeipa/ticket/6061
---
 API.txt |  8 
 VERSION |  4 ++--
 ipalib/output.py| 10 ++
 ipaserver/plugins/automember.py |  3 +++
 ipaserver/plugins/trust.py  |  1 +
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/API.txt b/API.txt
index eb33c1fb7f94f5af45ec0b38fc7e45e484a1044e..cbe23f4bde3a29cf3f28a9e361f83e176ede08e0 100644
--- a/API.txt
+++ b/API.txt
@@ -144,7 +144,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_set/1
 args: 0,6,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -155,7 +155,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_show/1
 args: 0,4,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -164,7 +164,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_del/1
 args: 1,2,3
 arg: Str('cn+', cli_name='automember_rule')
@@ -5584,7 +5584,7 @@ option: StrEnum('trust_type', autofill=True, cli_name='type', default=u'ad', val
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: trustdomain_add/1
 args: 2,8,3
 arg: Str('trustcn', cli_name='trust')
diff --git a/VERSION b/VERSION
index 0559741451a858dd0adfa99a8bf653261d771601..ca489965050f32d2d8987dfd251ec2b2a0ba1768 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=2010061412
 #  #
 
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=210
-# Last change: Add --ca option to cert-status
+IPA_API_VERSION_MINOR=211
+# Last change: mbabinsk: allow 'value' output param in commands without primary key
diff --git a/ipalib/output.py b/ipalib/output.py
index 19dd9adadeb8521caf9f0dc52981ce57a7f0c8b6..b

Re: [Freeipa-devel] CI DNS locations: basic test for SRV records

2016-07-18 Thread Martin Basti 



On 18.07.2016 13:18, Petr Spacek wrote:

On 8.7.2016 14:01, Martin Basti wrote:

See commit message for details. Patch attached.


This test does not cover:

* NTP service records

* ipa-ca A/ records

* ADTrust records

Should I open tickets to cover cases above?

ACK


Pushed to master: 72b2c8a54de09d6e5c1cc82c951d5bfd06938e88

--
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] CI DNS locations: basic test for SRV records

2016-07-18 Thread Petr Spacek 
On 8.7.2016 14:01, Martin Basti wrote:
> See commit message for details. Patch attached.
> 
> 
> This test does not cover:
> 
> * NTP service records
> 
> * ipa-ca A/ records
> 
> * ADTrust records
> 
> Should I open tickets to cover cases above?

ACK

-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCH 0552] Vault: enable client side plugins CLI

2016-07-18 Thread Jan Cholasta 

Hi,

On 12.7.2016 16:03, Petr Vobornik wrote:

On 07/12/2016 02:06 PM, Martin Babinsky wrote:

On 07/08/2016 04:36 PM, Alexander Bokovoy wrote:

On Fri, 08 Jul 2016, Martin Basti wrote:

Patch attached.
https://fedorahosted.org/freeipa/ticket/6035



From 2c97c316c1db49daeda15c709f082ee083a741ad Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 8 Jul 2016 15:53:25 +0200
Subject: [PATCH] Enable vault-* commands on client

Client plugins fot vault commands were disabled by NO_CLI=True,
inherited from vault_add_interal, that is always NO_CLI=True.
Introduced by this commit 8278da6967dbe425b4e0c6cf37dc1c53052525b2

Removed NO_CLI=True from client side plugins for vault.

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

ACK.

I haven't tested it but the change is obvious.



And it works as expected, so ACK also from me.



master:
* 9feeaca9fb552229638ce98086aa75905a45b48d Enable vault-* commands on client


The intent of the NO_CLI properties was to hide the commands when the 
server does not support them. It didn't exactly work for vault commands, 
but must be done anyway.


Ticket: 

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] 0210 frontend: fix output validation for multiple type choices

2016-07-18 Thread Martin Babinsky 

On 07/18/2016 10:01 AM, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, Alexander Bokovoy wrote:

Hi,

I had some time and was blocked by these bugs to do my tickets so I
actually fixed these three problems that are assigned to Martin
Babinsky. Hopefully, Martin wouldn't be offended by that. :)

--

Output entry elements may have multiple types allowed. We need to check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),) tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show
(and automember-set-default in #6037) claims to return the primary key
of the object in the 'value' output field, but the object does not have
a primary key, so the client rightfully expects None.

A proper fix would be to set "has_output = output.simple_value" for
these commands (all of automember_default_group_{set,remove,show},
trustconfig_{mod,show}).

Honza



The problem is that these commands do not return a simple boolean in 
'result' but a full entry dict, so 'simple_value' won't do the trick in 
this case.


But I agree, we should rather fix misbehaving commands rather than bend 
the framework to accomodate their idiosyncracies, we have enough of that 
already.


--
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] 0211-0212 Make sure --raw option works for trust-add

2016-07-18 Thread Martin Babinsky 

On 07/16/2016 12:50 PM, Alexander Bokovoy wrote:

Hi,


I had some time and was blocked by these bugs to do my tickets so I
actually fixed these three problems that are assigned to Martin
Babinsky. Hopefully, Martin wouldn't be offended by that. :)

Note that this fix (patch 0211) has potential for a break but also
introduces a correct behavior in my view as we should not really have
non-lower cased keys in LDAP dictionaries in entry_to_dict() for both
normal and --raw modes.

- 0211

Since commit a8dd7aa337f25abd938a582d0fcba51d3b356410 if IPA command
is called with --raw option, a retrieved LDAP entry's attribute
names aren't normalized to lower case when converting the entry
to a dictionary. This breaks overall assumption that dictionary
keys are in lower case.

Partially fixes 'ipa trust-add --raw' issues.

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

- 0212

Make sure we display raw values for 'trust-add --raw' case.

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






Hi Alexander,

I am CC'ing Jan since I hope he knows why 
a8dd7aa337f25abd938a582d0fcba51d3b356410 was implemented in this way. I 
think there was a reason behind this decision and we should not revert 
it without further discussion.


I had the fix for trust-add ready on Friday but was unable to test it 
properly because of some issues with my VMs. I am attaching it for 
reference, since it is similar to your patch 212 but relies on 
attrs_list passed in to LDAP search in order to fetch lowercased attributes.


--
Martin^3 Babinsky
From c3c197822364c01d62626acdc3d69274a14ca291 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Fri, 15 Jul 2016 12:38:00 +0200
Subject: [PATCH] trust-add: handle `--all/--raw` options properly

`trust-add` command did not handle these options correctly often resulting in
internal errors or mangled output. This patch implements a behavior which is
more in-line with the rest of the API commands.

https://fedorahosted.org/freeipa/ticket/6059
---
 ipaserver/plugins/trust.py | 41 +++--
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/ipaserver/plugins/trust.py b/ipaserver/plugins/trust.py
index 8536202b9b785507bd27b3c7b1896b721f8c5927..18a7cfd3447143393fb087f840edb406e98285a4 100644
--- a/ipaserver/plugins/trust.py
+++ b/ipaserver/plugins/trust.py
@@ -710,6 +710,25 @@ sides.
 msg_summary = _('Added Active Directory trust for realm "%(value)s"')
 msg_summary_existing = _('Re-established trust to domain "%(value)s"')
 
+def _format_trust_attrs(self, result, **options):
+
+# Format the output into human-readable values
+attributes = int(result['result'].get('ipanttrustattributes', [0])[0])
+
+if not options.get('raw', False):
+result['result']['trusttype'] = [trust_type_string(
+result['result']['ipanttrusttype'][0], attributes)]
+result['result']['trustdirection'] = [trust_direction_string(
+result['result']['ipanttrustdirection'][0])]
+result['result']['truststatus'] = [trust_status_string(
+result['verified'])]
+
+if attributes:
+result['result'].pop('ipanttrustattributes', None)
+
+result['result'].pop('ipanttrustauthoutgoing', None)
+result['result'].pop('ipanttrustauthincoming', None)
+
 def execute(self, *keys, **options):
 ldap = self.obj.backend
 
@@ -729,10 +748,15 @@ sides.
 else:
 created_range_type = old_range['result']['iparangetype'][0]
 
+attrs_list = self.obj.default_attributes
+if options.get('all', False):
+attrs_list.append('*')
+
 trust_filter = "cn=%s" % result['value']
 (trusts, truncated) = ldap.find_entries(
  base_dn=DN(self.api.env.container_trusts, self.api.env.basedn),
- filter=trust_filter)
+ filter=trust_filter,
+ attrs_list=attrs_list)
 
 result['result'] = entry_to_dict(trusts[0], **options)
 
@@ -761,20 +785,9 @@ sides.
 # add_new_domains_from_trust() on its own.
 fetch_trusted_domains_over_dbus(self.api, self.log, result['value'])
 
-# Format the output into human-readable values
-attributes = int(result['result'].get('ipanttrustattributes', [0])[0])
-result['result']['trusttype'] = [trust_type_string(
-result['result']['ipanttrusttype'][0], attributes)]
-result['result']['trustdirection'] = [trust_direction_string(
-result['result']['ipanttrustdirection'][0])]
-result['result']['truststatus'] = [trust_status_string(
-result['verified'])]
-if attributes:
-result['result'].pop('ipanttrustattributes', None)
-
+# Format the output into human-readable values unless `--raw` is given
+self._format_trust_attrs(result, **options)
 del result['ver

Re: [Freeipa-devel] [PATCH 031] RedHatCAService should wait for local Dogtag instance

2016-07-18 Thread Martin Basti 



On 12.07.2016 16:41, Christian Heimes wrote:

On 2016-07-07 14:54, Martin Basti wrote:

Patch needs changes in ipa-4-3 branch

My patch? Do you want me to submit a patch for 4.3 branch?

Christian


The ticket is in 4.3.2 milestone, so we need patch for ipa-4-3 branch 
too. Current patch does not apply (too much conflicts)


Martin^2

--
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 0187] Use server API in com.redhat.idm.trust-fetch-domains oddjob helper

2016-07-18 Thread Alexander Bokovoy 

On Mon, 18 Jul 2016, Martin Babinsky wrote:

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

--
Martin^3 Babinsky



From 990f29cbfb457c6179ffc0bed452dc358ba30d21 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 14 Jul 2016 09:31:22 +0200
Subject: [PATCH] Use server API in com.redhat.idm.trust-fetch-domains oddjob
helper

https://fedorahosted.org/freeipa/ticket/6082
---
install/oddjob/com.redhat.idm.trust-fetch-domains | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/oddjob/com.redhat.idm.trust-fetch-domains 
b/install/oddjob/com.redhat.idm.trust-fetch-domains
index 
a6b87cde917cfa5bfedf28442a6d1b2b512706f9..7c948fd53bd54bf3638ef3cc4407576b9011f4fb
 100755
--- a/install/oddjob/com.redhat.idm.trust-fetch-domains
+++ b/install/oddjob/com.redhat.idm.trust-fetch-domains
@@ -76,7 +76,7 @@ env._bootstrap(debug=options.debug, log=None)
env._finalize_core(**dict(DEFAULT_CONFIG))

# Initialize the API with the proper debug level
-api.bootstrap(debug=env.debug, log=None)
+api.bootstrap(in_server=True, debug=env.debug, log=None)
api.finalize()

# Only import trust plugin after api is initialized or internal imports
ACK. 
--

/ Alexander Bokovoy

--
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 0187] Use server API in com.redhat.idm.trust-fetch-domains oddjob helper

2016-07-18 Thread Martin Babinsky 

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

--
Martin^3 Babinsky
From 990f29cbfb457c6179ffc0bed452dc358ba30d21 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 14 Jul 2016 09:31:22 +0200
Subject: [PATCH] Use server API in com.redhat.idm.trust-fetch-domains oddjob
 helper

https://fedorahosted.org/freeipa/ticket/6082
---
 install/oddjob/com.redhat.idm.trust-fetch-domains | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/oddjob/com.redhat.idm.trust-fetch-domains b/install/oddjob/com.redhat.idm.trust-fetch-domains
index a6b87cde917cfa5bfedf28442a6d1b2b512706f9..7c948fd53bd54bf3638ef3cc4407576b9011f4fb 100755
--- a/install/oddjob/com.redhat.idm.trust-fetch-domains
+++ b/install/oddjob/com.redhat.idm.trust-fetch-domains
@@ -76,7 +76,7 @@ env._bootstrap(debug=options.debug, log=None)
 env._finalize_core(**dict(DEFAULT_CONFIG))
 
 # Initialize the API with the proper debug level
-api.bootstrap(debug=env.debug, log=None)
+api.bootstrap(in_server=True, debug=env.debug, log=None)
 api.finalize()
 
 # Only import trust plugin after api is initialized or internal imports
-- 
2.7.4

-- 
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] Please use https:// url for freeipa.git repo

2016-07-18 Thread Martin Babinsky 
It seems that access to upstream freeipa.git repo through Git protocol 
does not work (or was deliberately disabled by Fedora infra).


Please use HTTPS for fetching/cloning/pulling/etc., so replace

git://git.fedorahosted.org/git/freeipa.git

with

https://git.fedorahosted.org/git/freeipa.git

in the repo config.

--
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] 0210 frontend: fix output validation for multiple type choices

2016-07-18 Thread Alexander Bokovoy 

On Mon, 18 Jul 2016, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, Alexander Bokovoy wrote:

Hi,

I had some time and was blocked by these bugs to do my tickets so I
actually fixed these three problems that are assigned to Martin
Babinsky. Hopefully, Martin wouldn't be offended by that. :)

--

Output entry elements may have multiple types allowed. We need to check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),) tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show 
(and automember-set-default in #6037) claims to return the primary key 
of the object in the 'value' output field, but the object does not 
have a primary key, so the client rightfully expects None.

Why did it work before introducing thin client?

Aside from that, comparing "(type(None),) is None" will never give you
True on the thin client side. At the server side we have "None is None"
and that works. So the question is also why there is a change like that
between client and server sides?

A proper fix would be to set "has_output = output.simple_value" for 
these commands (all of automember_default_group_{set,remove,show}, 
trustconfig_{mod,show}).


Honza

--
Jan Cholasta


--
/ Alexander Bokovoy

--
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] 0210 frontend: fix output validation for multiple type choices

2016-07-18 Thread Jan Cholasta 

Hi,

On 16.7.2016 12:46, Alexander Bokovoy wrote:

Hi,

I had some time and was blocked by these bugs to do my tickets so I
actually fixed these three problems that are assigned to Martin
Babinsky. Hopefully, Martin wouldn't be offended by that. :)

--

Output entry elements may have multiple types allowed. We need to check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),) tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show 
(and automember-set-default in #6037) claims to return the primary key 
of the object in the 'value' output field, but the object does not have 
a primary key, so the client rightfully expects None.


A proper fix would be to set "has_output = output.simple_value" for 
these commands (all of automember_default_group_{set,remove,show}, 
trustconfig_{mod,show}).


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] 0089 caacl: expand plugin documentation

2016-07-18 Thread Martin Basti 



On 13.07.2016 18:34, Petr Vobornik wrote:

On 07/12/2016 08:45 AM, Alexander Bokovoy wrote:

On Tue, 12 Jul 2016, Fraser Tweedale wrote:

Attached patch is a doc change, addressing
https://fedorahosted.org/freeipa/ticket/6002.

Thanks,
Fraser
 From 19c5fc60391d37c9d0500feb5d5d5a6628bc4d27 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Tue, 12 Jul 2016 15:11:11 +1000
Subject: [PATCH] caacl: expand plugin documentation

Expand the 'caacl' plugin documentation to explain some common
confusions including the fact that CA ACLs apply to the target
subject principal (not necessarily the principal requesting the
cert), and the fact that CA-less CA ACL implies the 'ipa' CA.

Fixes: https://fedorahosted.org/freeipa/ticket/6002
---
ipaserver/plugins/caacl.py | 34 --
1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/ipaserver/plugins/caacl.py b/ipaserver/plugins/caacl.py
index
9a60f7e27809c4f41b160647efafde94dbe90bf0..d316cc7c48cf2997d6be6b052dc1efa6d6fcdb6a
100644
--- a/ipaserver/plugins/caacl.py
+++ b/ipaserver/plugins/caacl.py
@@ -23,14 +23,36 @@ if six.PY3:
__doc__ = _("""
Manage CA ACL rules.

-This plugin is used to define rules governing which principals are
-permitted to have certificates issued using a given certificate
-profile.
+This plugin is used to define rules governing which CAs and profiles
+may be used to issue certificates to particular principals or groups
+of principals.

-PROFILE ID SYNTAX:
+SUBJECT PRINCIPAL SCOPE:

-A Profile ID is a string without spaces or punctuation starting with
a letter
-and followed by a sequence of letters, digits or underscore ("_").
+For a certificate request to be allowed, the principal(s) that are
+the subject of a certificate request (not necessarily the principal
+actually requesting the certificate) must be included in the scope
+of a CA ACL that also includes the target CA and profile.
+
+Users can be included by name, group or the "all users" category.
+Hosts can be included by name, hostgroup or the "all hosts"
+category.  Services can be included by service name or the "all
+services" category.  CA ACLs may be associated with a single type of
+principal, or multiple types.
+
+CERTIFICATE AUTHORITY SCOPE:
+
+A CA ACL can be associated with one or more CAs by name, or by the
+"all CAs" category.  For compatibility reasons, a CA ACL with no CA
+association implies an association with the 'ipa' CA (and only this
+CA).
+
+PROFILE SCOPE:
+
+A CA ACL can be associated with one or more profiles by Profile ID.
+The Profile ID is a string without spaces or punctuation starting
+with a letter and followed by a sequence of letters, digits or
+underscore ("_").

EXAMPLES:


ACK. Reads well.


Pushed to master: 8cd87d12d53a98a8e386c06a7c5fddb1d38d990d

Please note for future, that long string should be splitted, to make 
life of translators easier


http://www.freeipa.org/page/Coding_Best_Practices#Split_long_translatable_strings

Martin^2

--
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