Re: [Freeipa-devel] [PATCH] 0050 caacl: correctly handle full user principal name

2016-03-14 Thread Fraser Tweedale 
On Mon, Mar 14, 2016 at 03:10:55PM +0100, Martin Kosek wrote:
> On 03/14/2016 06:18 AM, Alexander Bokovoy wrote:
> > On Mon, 14 Mar 2016, Fraser Tweedale wrote:
> >> The attached patch fixes
> >> https://fedorahosted.org/freeipa/ticket/5733.  Thanks to Alexander
> >> for finding and reporting.
> >>
> >> Cheers,
> >> Fraser
> > 
> >> From 9bd7b74d9c928f386bd7dae59588580881ed1a9d Mon Sep 17 00:00:00 2001
> >> From: Fraser Tweedale 
> >> Date: Mon, 14 Mar 2016 14:49:47 +1100
> >> Subject: [PATCH] caacl: correctly handle full user principal name
> >>
> >> The caacl HBAC request is correct when just the username is given,
> >> but the full 'user@REALM' form was not handled correctly.
> >>
> >> Fixes: https://fedorahosted.org/freeipa/ticket/5733
> > A context might be helpful here: if you are using certmonger's -K option
> > to specify a user principal name to add to certificate, the name will
> > get normalized to include the realm. This is how it gets to caacl check.
> > 
> > ACK.
> 
> Seeing the patch, I am curious - is the realm validated anywhere pr is it just
> dropped and we just assume it is FreeIPA one?
> 
> I mean, do we make sure that REALM matches FreeIPA REALM and it is not trusted
> AD realm for example?
>
Martin, glad you asked.  We catch that situation elsewhere:

ftweedal% ipa cert-request --principal al...@notmydomain.org alice.csr
ipa: ERROR: The realm for the principal does not match the realm for this 
IPA server

Cheers,
Fraser

-- 
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] [Pki-devel] Design review request: RFC 2818 certificate compliance

2016-03-14 Thread Fraser Tweedale 
On Mon, Mar 14, 2016 at 09:29:37AM -0700, Christina Fu wrote:
> 
> 
> On 03/12/2016 11:51 PM, Fraser Tweedale wrote:
> >On Fri, Mar 11, 2016 at 10:20:49AM -0800, Christina Fu wrote:
> >>Hi Fraser,
> >>
> >>I think the general idea looks good.  If tested to work, I actually think
> >>you should have it replace the current caServerCert.cfg and make it the
> >>default server cert profile for Dogtag.  So I'd suggest you name things more
> >>generically.
> >>
> >Thanks Christina for the feedback.  W.r.t naming, can you clarify
> >what you think should be more generic and why?
> Actually it was more of a preemptive comment that was not specifically
> directed towards anything in your current design.
> I just took a closer look, and I think your new profile plugin name
> (|SubjectAltNameCopyCNDefault|) sounds good.
> 
> About replacing existing caServerCert.cfg, consider keeping it, but
> 1. name the new profile something like caServerSANCert.cfg
> 2. make caServerSANCert.cfg default (enable it), and disable
> caServerCert.cfg by default
> 
> Anyway, you get the idea.  The point is that I think we should fundamentally
> adhere to the standard in Dogtag, so such a fix should be part of the Dogtag
> default.
> 
> thanks,
> Christina
> 
Understood; thanks.  I'll file a ticket for the Dogtag profile
change.

> >
> >>Just for your reference, there is an implementation that injects SAN(s) into
> >>server certs at time of Dogtag instance creation.  It also allows one to put
> >>multiple SANs in one ssl server cert:
> >>https://fedorahosted.org/pki/ticket/1316#comment:14
> >>again, it's only limited to pkispawn option so it serves a different
> >>purpose.
> >>
> >>Christina
> >>
> >>On 03/10/2016 05:06 PM, Fraser Tweedale wrote:
> >>>On Mon, Mar 07, 2016 at 07:33:52AM +0100, Jan Cholasta wrote:
> Hi,
> 
> On 29.2.2016 07:59, Fraser Tweedale wrote:
> >Hi all (especially those interested in certificates),
> >
> >Please provide early review of my design for RFC 2818 compliance
> >which will address the following tickets:
> >
> >- #4970 Server certificate profile should always include a Subject 
> >Alternate name for the host
> >- #5706 [RFE] Support SAN-only certificates
> >
> >http://www.freeipa.org/page/V4/RFC_2818_certificate_compliance
> >
> >The design is a WIP and there is no code for it yet.  Looking for
> >feedback and (hopefully) validation of the approach before
> >committing cycles to implementing new profile components in Dogtag.
> 1) Do wildcard certificates need special handling? There is no mention of
> them in the design doc.
> 
> >>>No special handling of wildcard certs is needed but I've added some
> >>>commentary to the design page.
> >>>
> 2) Should we accept invalid CSR where CN length is greater than 64? I
> wouldn't be surprised if these existed in the wild.
> 
> >>>Good question.  I agree such CSRs probably exist.  There are various
> >>>ways to handle them:
> >>>
> >>>a) Reject request (with useful message; instruction to issue
> >>>SAN-only request instead)
> >>>
> >>>b) Issue non-compliant cert with overlong CN.  It will be helpful to
> >>>find out how important clients handle such certs.
> >>>
> >>>c) Accept the CSR but "promote" the overlong CN from CSR into a SAN
> >>>dnsName, and issue a SAN-only cert.  Some clients may not handle
> >>>such certs very well.
> >>>
> >>>Personally I like (c), because the user intent is clear but we still
> >>>issue a valid cert, however, I expect there are clients out there
> >>>(particularly in "enterprise" environments?) that will not handle it
> >>>well.
> >>>
> >>>I've copied pki-devel@ to solicit additional insights here :)
> >>>
> 3) Sometimes it is not clear which parts belong to Dogtag and which to IPA
> itself. For example the upgrade section - I assume Dogtag should update
> registry.cfg and IPA caIPAserviceCert profile, but it is not clearly 
> stated
> anywhere.
> 
> >>>Thanks, I've added clarifying remarks.  In brief: yes Dogtag should
> >>>update registry.cfg, but FreeIPA should update the profile.
> >>>
> >>>Thank you for your feedback, Jan.
> >>>Fraser
> >>>
> >>>___
> >>>Pki-devel mailing list
> >>>pki-de...@redhat.com
> >>>https://www.redhat.com/mailman/listinfo/pki-devel
> >>-- 
> >>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
> 

-- 
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 0141] ipa-replica-manage: print traceback on unexpected error when in verbose mode

2016-03-14 Thread Martin Basti 



On 10.03.2016 15:59, Martin Babinsky wrote:

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




NACK

1)
Maybe we should print traceback in verbose mode for RuntimeError as well.

2)
IMO would be better to print traceback first and then, print error

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] [Pki-devel] Design review request: RFC 2818 certificate compliance

2016-03-14 Thread Christina Fu 



On 03/12/2016 11:51 PM, Fraser Tweedale wrote:

On Fri, Mar 11, 2016 at 10:20:49AM -0800, Christina Fu wrote:

Hi Fraser,

I think the general idea looks good.  If tested to work, I actually think
you should have it replace the current caServerCert.cfg and make it the
default server cert profile for Dogtag.  So I'd suggest you name things more
generically.


Thanks Christina for the feedback.  W.r.t naming, can you clarify
what you think should be more generic and why?
Actually it was more of a preemptive comment that was not specifically 
directed towards anything in your current design.
I just took a closer look, and I think your new profile plugin name 
(|SubjectAltNameCopyCNDefault|) sounds good.


About replacing existing caServerCert.cfg, consider keeping it, but
1. name the new profile something like caServerSANCert.cfg
2. make caServerSANCert.cfg default (enable it), and disable 
caServerCert.cfg by default


Anyway, you get the idea.  The point is that I think we should 
fundamentally adhere to the standard in Dogtag, so such a fix should be 
part of the Dogtag default.


thanks,
Christina




Just for your reference, there is an implementation that injects SAN(s) into
server certs at time of Dogtag instance creation.  It also allows one to put
multiple SANs in one ssl server cert:
https://fedorahosted.org/pki/ticket/1316#comment:14
again, it's only limited to pkispawn option so it serves a different
purpose.

Christina

On 03/10/2016 05:06 PM, Fraser Tweedale wrote:

On Mon, Mar 07, 2016 at 07:33:52AM +0100, Jan Cholasta wrote:

Hi,

On 29.2.2016 07:59, Fraser Tweedale wrote:

Hi all (especially those interested in certificates),

Please provide early review of my design for RFC 2818 compliance
which will address the following tickets:

- #4970 Server certificate profile should always include a Subject Alternate 
name for the host
- #5706 [RFE] Support SAN-only certificates

http://www.freeipa.org/page/V4/RFC_2818_certificate_compliance

The design is a WIP and there is no code for it yet.  Looking for
feedback and (hopefully) validation of the approach before
committing cycles to implementing new profile components in Dogtag.

1) Do wildcard certificates need special handling? There is no mention of
them in the design doc.


No special handling of wildcard certs is needed but I've added some
commentary to the design page.


2) Should we accept invalid CSR where CN length is greater than 64? I
wouldn't be surprised if these existed in the wild.


Good question.  I agree such CSRs probably exist.  There are various
ways to handle them:

a) Reject request (with useful message; instruction to issue
SAN-only request instead)

b) Issue non-compliant cert with overlong CN.  It will be helpful to
find out how important clients handle such certs.

c) Accept the CSR but "promote" the overlong CN from CSR into a SAN
dnsName, and issue a SAN-only cert.  Some clients may not handle
such certs very well.

Personally I like (c), because the user intent is clear but we still
issue a valid cert, however, I expect there are clients out there
(particularly in "enterprise" environments?) that will not handle it
well.

I've copied pki-devel@ to solicit additional insights here :)


3) Sometimes it is not clear which parts belong to Dogtag and which to IPA
itself. For example the upgrade section - I assume Dogtag should update
registry.cfg and IPA caIPAserviceCert profile, but it is not clearly stated
anywhere.


Thanks, I've added clarifying remarks.  In brief: yes Dogtag should
update registry.cfg, but FreeIPA should update the profile.

Thank you for your feedback, Jan.
Fraser

___
Pki-devel mailing list
pki-de...@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel

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


-- 
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 0024] ipa-replica-manage: added --suffix option for certain commands

2016-03-14 Thread Petr Vobornik 

On 03/14/2016 04:55 PM, Jan Cholasta wrote:

On 14.3.2016 16:26, Petr Vobornik wrote:

On 03/14/2016 12:57 PM, Jan Cholasta wrote:

On 14.3.2016 12:50, Martin Basti wrote:



On 14.03.2016 12:05, Jan Cholasta wrote:

Hi,

On 11.3.2016 10:39, Stanislav Laznicka wrote:

Hi,

Please see the patch attached. Contrary to the discussion at
https://fedorahosted.org/freeipa/ticket/4987 I also added the suffix
option for clean_ruv command. If this command is available for normal
RUVs, it should probably be available for CS-RUVs as well (or
deprecated
for both with advised use of clean_dangling_ruv).


ipa-csreplica-manage is used to manage the CA suffix, so
ipa-csreplica-manage should be extended instead of adding --suffix
option to ipa-replica-manage. Having half of the CA suffix managed by
ipa-replica-manage and the other half by ipa-replica-manage is
confusing.

Honza


There is a design document about deprecating ipa-csreplica-manage and
move part of its responsibilities to ipa-replica-manage.

http://www.freeipa.org/page/V4/Manage_replication_topology_4_4#ipa.28cs.29replica_manange_changes





So patch is compatible with design.


The design is wrong then.


I don't agree.



Either do it in ipa-csreplica-manage, or make *all* ipa-replica-manage
sub-commands respect the --suffix option. Anything else is inconsistent
mess.


That's the idea for domain level 1. There is little value in extending
behavior(managing replication agreements) in domain level 0.


Domain level 0 is still relevant, it won't go away anytime soon.



Main idea is to not care about suffixes and work with all suffixes right
away. This is reflected in clean-dangling-ruv command and these
extensions are its counterpart - to enable disabling the run. We mostly
care about replica IDs not suffixes they belong to. IMO --suffix option
is not necessary and is mostly for debugging.

One of the reasons why we have all the RUV commands is a mess after
uninstallation when somebody forgets/ignores to run
`ipa-csreplica-manage del $server` or also `ipa-replica-manage del
$server` before uninstallation of replica. Users then usually run
`ipa-replica-manage del $server` --force --clean` but
`ipa-csreplica-manage del $server` can't be run after it.  Changes in
4.3 and 4.4 tries to prevent this situation (e.g. by calling equivalent
of `ipa-cs+replica-manage del` from `ipa-server-install  --uninstall`).
But until then mess is cleaned on all servers, we should deal with it
with the most convenient way - hiding implementation details.



This is actually exposing implementation details by forcing the user to
use a different command based on the domain level.


What different commands?


Please explain to me how any of the above requires us to introduce
additional inconsistencies and bad UX to IPA.


What bad UX?

It is supposed to be used in following way:
  ipa-replica-manage clean-dangling-ruvs

If from whatever reason some clean ruv task is not finished then:
  ipa-replica-manage list-clean-ruv
[all running task for all suffixes]
  ipa-replica-manage abort-clean-ruv REPLICATION_ID

Nothing else. Works for both domain levels and suffixes from a single 
tool. Again, --suffix option is not important.


Note: clean-ruv subcommand could be probably marked as deprecated or be 
discouraged to use.


If the patch doesn't implement it, then it's wrong.
--
Petr Vobornik

--
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 0024] ipa-replica-manage: added --suffix option for certain commands

2016-03-14 Thread Jan Cholasta 

On 14.3.2016 16:26, Petr Vobornik wrote:

On 03/14/2016 12:57 PM, Jan Cholasta wrote:

On 14.3.2016 12:50, Martin Basti wrote:



On 14.03.2016 12:05, Jan Cholasta wrote:

Hi,

On 11.3.2016 10:39, Stanislav Laznicka wrote:

Hi,

Please see the patch attached. Contrary to the discussion at
https://fedorahosted.org/freeipa/ticket/4987 I also added the suffix
option for clean_ruv command. If this command is available for normal
RUVs, it should probably be available for CS-RUVs as well (or
deprecated
for both with advised use of clean_dangling_ruv).


ipa-csreplica-manage is used to manage the CA suffix, so
ipa-csreplica-manage should be extended instead of adding --suffix
option to ipa-replica-manage. Having half of the CA suffix managed by
ipa-replica-manage and the other half by ipa-replica-manage is
confusing.

Honza


There is a design document about deprecating ipa-csreplica-manage and
move part of its responsibilities to ipa-replica-manage.

http://www.freeipa.org/page/V4/Manage_replication_topology_4_4#ipa.28cs.29replica_manange_changes




So patch is compatible with design.


The design is wrong then.


I don't agree.



Either do it in ipa-csreplica-manage, or make *all* ipa-replica-manage
sub-commands respect the --suffix option. Anything else is inconsistent
mess.


That's the idea for domain level 1. There is little value in extending
behavior(managing replication agreements) in domain level 0.


Domain level 0 is still relevant, it won't go away anytime soon.



Main idea is to not care about suffixes and work with all suffixes right
away. This is reflected in clean-dangling-ruv command and these
extensions are its counterpart - to enable disabling the run. We mostly
care about replica IDs not suffixes they belong to. IMO --suffix option
is not necessary and is mostly for debugging.

One of the reasons why we have all the RUV commands is a mess after
uninstallation when somebody forgets/ignores to run
`ipa-csreplica-manage del $server` or also `ipa-replica-manage del
$server` before uninstallation of replica. Users then usually run
`ipa-replica-manage del $server` --force --clean` but
`ipa-csreplica-manage del $server` can't be run after it.  Changes in
4.3 and 4.4 tries to prevent this situation (e.g. by calling equivalent
of `ipa-cs+replica-manage del` from `ipa-server-install  --uninstall`).
But until then mess is cleaned on all servers, we should deal with it
with the most convenient way - hiding implementation details.



This is actually exposing implementation details by forcing the user to 
use a different command based on the domain level.


Please explain to me how any of the above requires us to introduce 
additional inconsistencies and bad UX to IPA.


--
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 0024] ipa-replica-manage: added --suffix option for certain commands

2016-03-14 Thread Petr Vobornik 

On 03/14/2016 12:57 PM, Jan Cholasta wrote:

On 14.3.2016 12:50, Martin Basti wrote:



On 14.03.2016 12:05, Jan Cholasta wrote:

Hi,

On 11.3.2016 10:39, Stanislav Laznicka wrote:

Hi,

Please see the patch attached. Contrary to the discussion at
https://fedorahosted.org/freeipa/ticket/4987 I also added the suffix
option for clean_ruv command. If this command is available for normal
RUVs, it should probably be available for CS-RUVs as well (or
deprecated
for both with advised use of clean_dangling_ruv).


ipa-csreplica-manage is used to manage the CA suffix, so
ipa-csreplica-manage should be extended instead of adding --suffix
option to ipa-replica-manage. Having half of the CA suffix managed by
ipa-replica-manage and the other half by ipa-replica-manage is
confusing.

Honza


There is a design document about deprecating ipa-csreplica-manage and
move part of its responsibilities to ipa-replica-manage.

http://www.freeipa.org/page/V4/Manage_replication_topology_4_4#ipa.28cs.29replica_manange_changes



So patch is compatible with design.


The design is wrong then.


I don't agree.



Either do it in ipa-csreplica-manage, or make *all* ipa-replica-manage
sub-commands respect the --suffix option. Anything else is inconsistent
mess.


That's the idea for domain level 1. There is little value in extending 
behavior(managing replication agreements) in domain level 0.


Main idea is to not care about suffixes and work with all suffixes right 
away. This is reflected in clean-dangling-ruv command and these 
extensions are its counterpart - to enable disabling the run. We mostly 
care about replica IDs not suffixes they belong to. IMO --suffix option 
is not necessary and is mostly for debugging.


One of the reasons why we have all the RUV commands is a mess after 
uninstallation when somebody forgets/ignores to run 
`ipa-csreplica-manage del $server` or also `ipa-replica-manage del 
$server` before uninstallation of replica. Users then usually run 
`ipa-replica-manage del $server` --force --clean` but 
`ipa-csreplica-manage del $server` can't be run after it.  Changes in 
4.3 and 4.4 tries to prevent this situation (e.g. by calling equivalent 
of `ipa-cs+replica-manage del` from `ipa-server-install  --uninstall`). 
But until then mess is cleaned on all servers, we should deal with it 
with the most convenient way - hiding implementation details.


--
Petr Vobornik

--
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] 0050 caacl: correctly handle full user principal name

2016-03-14 Thread Martin Kosek 
On 03/14/2016 06:18 AM, Alexander Bokovoy wrote:
> On Mon, 14 Mar 2016, Fraser Tweedale wrote:
>> The attached patch fixes
>> https://fedorahosted.org/freeipa/ticket/5733.  Thanks to Alexander
>> for finding and reporting.
>>
>> Cheers,
>> Fraser
> 
>> From 9bd7b74d9c928f386bd7dae59588580881ed1a9d Mon Sep 17 00:00:00 2001
>> From: Fraser Tweedale 
>> Date: Mon, 14 Mar 2016 14:49:47 +1100
>> Subject: [PATCH] caacl: correctly handle full user principal name
>>
>> The caacl HBAC request is correct when just the username is given,
>> but the full 'user@REALM' form was not handled correctly.
>>
>> Fixes: https://fedorahosted.org/freeipa/ticket/5733
> A context might be helpful here: if you are using certmonger's -K option
> to specify a user principal name to add to certificate, the name will
> get normalized to include the realm. This is how it gets to caacl check.
> 
> ACK.

Seeing the patch, I am curious - is the realm validated anywhere pr is it just
dropped and we just assume it is FreeIPA one?

I mean, do we make sure that REALM matches FreeIPA REALM and it is not trusted
AD realm for example?

-- 
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 0434] log: add timestamp to filename of logs

2016-03-14 Thread Jan Cholasta 

On 14.3.2016 13:56, Rob Crittenden wrote:

Jan Cholasta wrote:

On 11.3.2016 15:56, Gabe Alford wrote:

On Fri, Mar 11, 2016 at 7:35 AM, Petr Vobornik > wrote:

 On 03/11/2016 03:00 PM, Rob Crittenden wrote:

 Martin Kosek wrote:

 On 03/11/2016 09:55 AM, Jan Cholasta wrote:

 On 11.3.2016 09:33, Martin Kosek wrote:

 On 03/08/2016 07:07 PM, Martin Basti wrote:



 On 08.03.2016 16:37, Martin Basti wrote:



 On 08.03.2016 16:31, Martin Basti wrote:


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

 Patch attached.


 Rebased patch attached.



 self-NACK

 Scripts print to CLI unformatted strings, it
 should not be so easy.
 See /var/log/ipaupgrade-{timestamp}.log for more
 information


 second-NACK. We cannot break existing log file
 paths. The paths are mentioned
 in a documentation and there may be also automation
 around that (gathering log
 files). So there should be always symlink from the
 well known location to the
 newest timestampe'd log.


 Sorry, but this is absurd. What's the point of
 maintaining backward
 compatibility with obsolete documentation? Following
 this logic, we would not
 be able to change anything ever. What we should actually
 do is update the
 documentation. Ditto for automation.


 +1 for updating the automation and documentation. But some
 backward
 compatibility will need to be retained, at least for the
 stable systems like
 RHEL where *other* people may have some automation or
 documentation around it,
 not just us.


 Or you could just also create a symlink to the old name and it
will
 always just work.

 rob


 Aren't the symlinks what Martin2 mentioned in second-NACK?

 These new timestamped logs should be combined with the Gabe's
 patches: #5728 (renamed to command name) and #5724 (move to
 /var/log/ipa directory).

 So that there will be e.g.:
 /var/log/ipaserver-install.log ->
 /var/log/ipa-server-install-{timestamp}.log

 /var/log/ipa/ipa-server-install.log ->
 /var/log/ipa-server-install-{timestamp}.log


I wonder if it would be simpler/better to always write to the *.log
file, and then have old logs timestamped rather than write directly to a
timestamped log file?
Then just symlink the original log file in /var/log/ to the new log file
name/location in /var/log/ipa.

For example:
/var/log/ipaserver-install.log ->
/var/log/ipa/ipa-server-install.log<-- We write to this
log (current)

/var/log/ipa-server-install-{timestamp}.log   <-- Old log with some date

/var/log/ipa-server-install-{timestamp}.log   <-- Older log with some
date

/var/log/ipa-server-install-{timestamp}.log   <-- Oldest log with some
date


This is way too overengineered for something that should actually be
really simple. I don't care if it is done this way or not, but IMHO it
would be a waste of time. Logs are not API and should not be treated as
such. If it needs to be done differently on RHEL, it should be handled
downstream.


Sure logs are not API but they have been named the same way since
inception (nearly 8 years now). I don't think symlinking to the old
names is a big deal.


It kind of is, since you have to keep the symlink up to date, handle the 
case when there is a regular file in place of the symlink, and they 
won't work properly for commands which currently append to their log 
files rather than overwrite them anyway.


To do this properly, you have to add a new FileHandler with proper 
options for each old log file. IMHO there is no benefit in doing this 
upstream, but it is relatively straightforward and isolated to be done 
downstream.


--
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 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

2016-03-14 Thread Martin Basti 



On 14.03.2016 13:46, Martin Babinsky wrote:

On 03/11/2016 09:16 AM, David Kupka wrote:

Current version (0.5.0) of python-augeas is missing copy() method. Use
dkupka/python-augeas copr repo before new version it's build and
available in the official repos.




Hi David,

TLDR: NACK :D.

Here are high-level remarks to discuss:

Maybe it would be a good idea to move ipaaugeas/changeconf and ntp to 
ipaplatform since it is dealing with (sorta) platform specific 
behavior (ntp vs. chrony vs. whatever we will have for timesync in the 
future). CC'ing Jan for thoughts.


Also regarding patches 0096-0097, we could have platform specific 
TimeDateService object that could wrap NTP/chrony management. for 
example, the task namespace functions in Pathc 0096 could be 
reimplemented as a methods of the service (RedhatTimeDateService, 
FedoraTimeDateService etc.) and then called in a platform-agnostic 
manner.


Here are some comments regarding code:

Patch 0095:

1.)
+IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'

Do not forget to add this directory to %install and %files spection of 
the spec file so that it is correctly added to RPM build.


2.)

please separate import of system-wide and IPA-specific modules by 
blank line


+import collections
+from augeas import Augeas
+from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import root_logger

3.) the call to parent's __new__ should have signature 'super(aug_obj, 
cls).__new__(*args, **kwargs)'


+cls._instance = super(aug_obj, cls).__new__(cls, *args, 
**kwargs)


4.)

+raise RuntimeError('Augeas lenses was loaded. Could not 
add more'

+   'lenses.')

Should be 'Augeas lenses _were_ loaded'

5.)

+if lens in self.lenses:
+raise RuntimeError('Lens %s already added.' % lens)
+self.lenses.append(lens)
+load_path = '/augeas/load/{0}'.format(lens


Shouldn't the following code use os.path,join to construct the load_path?

6.) I would prefer the following indentation style in the add_lens() 
method


@@ -65,9 +65,9 @@ class aug_obj(object):
 for conf_file in conf_files:
 self._aug.set(os.path.join(load_path, 'incl[0]'), conf_file)
 self.tree['old'] = self.tree.get(conf_file, None)
-self.tree[conf_file] = aug_node(self._aug,
- os.path.join('/files',
- conf_file[1:]))
+self.tree[conf_file] = aug_node(
+self._aug, os.path.join('/files', conf_file[1:])
+)

7.) I would also prefer if the hardcoded paths like '/augeas/load', 
'files', and '//error' would be made into either module variables or 
class members.


8.)

+def load(self):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not 
add more'

+   'lenses.')

Fix the excpetion text in the same way as in 4.)

9.)

+errors = self._aug.match(os.path.join('//error'))

is the os.path.join necessary here?

10.) I guess you can rewrite the error message in load() method using 
list comprehension:


@@ -76,9 +76,9 @@ class aug_obj(object):
 self._aug.load()
 errors = self._aug.match(os.path.join('//error'))
 if errors:
-err_msg = ""
-for error in errors:
-err_msg += ("{}: {}".format(error, 
self._aug.get(error)))

+err_msg = '\n'.join(
+["{}: {}".format(e, self._aug.get(e)) for e in errors]
+)
 raise RuntimeError(err_msg)
 self._loaded = True

11.)

+class aug_node(collections.MutableMapping):
+""" Single augeas node.
+Can be handled as python dict().
+"""
+def __init__(self, aug, path):
+self._aug = aug
+if path and os.path.isabs(path):
+self._path = path
+else:
+self._tmp = _tmp_path(aug, path)
+self._path = self._tmp.path

Isn't it better to change signature of __init__ to:

def __init__(self, aug, path=None):

and then test whether path is None?

12.)

def __setitem__(self, key, node):
+target = os.path.join(self._path, key)
+end = '{0}[0]'.format(os.path.join(self._path, key))
+if self._aug.match(target):
+self._aug.remove(target)
+target_list = aug_list(self._aug, target)
+for src_node in aug_list(node._aug, node._path):
+target_list.append(src_node)

The 'end' variable is declared but not used.

13.)

+
+def __len__(self):
+return len(self._aug.match('{0}/*'.format(self._path)))
+
+def __iter__(self):
+for key in self._aug.match('{0}/*'.format(self._path)):
+yield self._aug.label(key)
+raise StopIteration()
+

Shouldn't we construct paths using os.path.join for the sake of 
consistency?


14.)

+def __bool__(self):
+return (bool(len(self)) or bool(self.value))

The parentheses around the boolean expression are

Re: [Freeipa-devel] [PATCH 550] certdb: never use the -r option of certutil

2016-03-14 Thread Rob Crittenden 
Jan Cholasta wrote:
> Hi,
> 
> the attached patch fixes 
> and .
> 

IMHO you should file a bug against nss as well.

rob

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

Re: [Freeipa-devel] [PATCH 0138] only search for Kerberos SRV records when autodiscovery was requested

2016-03-14 Thread Martin Basti 



On 07.03.2016 18:02, Martin Babinsky wrote:

A quick fix for https://fedorahosted.org/freeipa/ticket/4305

I'm aware that we were talking about putting realmd discovery into 
IPADiscovery class and stuff, but that is a bit beyond the scope of 
this ticket.


I will open ticket(s) tracking:

1.) Offload at least the IPA realm DNS discovery to realmd D-Bus 
interface
2.) rewrite (not refactoring: the stuff also needs a substantial 
functional redesign IMHO) of the DNS discovery in the client-side 
installers.


I will make sure that this effort is not forgotten and we are not left 
with yet-another temporary fix.





ACK

Pushed to:
ipa-4-3: b81f333c2c043c763f3534a8ef96a605bf04c343
master: 8290d4b4cba8cf0b9ca517f7f09db2ee81606899
-- 
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 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config

2016-03-14 Thread Martin Babinsky 

On 03/11/2016 09:16 AM, David Kupka wrote:

Current version (0.5.0) of python-augeas is missing copy() method. Use
dkupka/python-augeas copr repo before new version it's build and
available in the official repos.




Hi David,

TLDR: NACK :D.

Here are high-level remarks to discuss:

Maybe it would be a good idea to move ipaaugeas/changeconf and ntp to 
ipaplatform since it is dealing with (sorta) platform specific behavior 
(ntp vs. chrony vs. whatever we will have for timesync in the future). 
CC'ing Jan for thoughts.


Also regarding patches 0096-0097, we could have platform specific 
TimeDateService object that could wrap NTP/chrony management. for 
example, the task namespace functions in Pathc 0096 could be 
reimplemented as a methods of the service (RedhatTimeDateService, 
FedoraTimeDateService etc.) and then called in a platform-agnostic manner.


Here are some comments regarding code:

Patch 0095:

1.)
+IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'

Do not forget to add this directory to %install and %files spection of 
the spec file so that it is correctly added to RPM build.


2.)

please separate import of system-wide and IPA-specific modules by blank line

+import collections
+from augeas import Augeas
+from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import root_logger

3.) the call to parent's __new__ should have signature 'super(aug_obj, 
cls).__new__(*args, **kwargs)'


+cls._instance = super(aug_obj, cls).__new__(cls, *args, 
**kwargs)


4.)

+raise RuntimeError('Augeas lenses was loaded. Could not add 
more'

+   'lenses.')

Should be 'Augeas lenses _were_ loaded'

5.)

+if lens in self.lenses:
+raise RuntimeError('Lens %s already added.' % lens)
+self.lenses.append(lens)
+load_path = '/augeas/load/{0}'.format(lens


Shouldn't the following code use os.path,join to construct the load_path?

6.) I would prefer the following indentation style in the add_lens() method

@@ -65,9 +65,9 @@ class aug_obj(object):
 for conf_file in conf_files:
 self._aug.set(os.path.join(load_path, 'incl[0]'), conf_file)
 self.tree['old'] = self.tree.get(conf_file, None)
-self.tree[conf_file] = aug_node(self._aug,
-os.path.join('/files',
- conf_file[1:]))
+self.tree[conf_file] = aug_node(
+self._aug, os.path.join('/files', conf_file[1:])
+)

7.) I would also prefer if the hardcoded paths like '/augeas/load', 
'files', and '//error' would be made into either module variables or 
class members.


8.)

+def load(self):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not add 
more'

+   'lenses.')

Fix the excpetion text in the same way as in 4.)

9.)

+errors = self._aug.match(os.path.join('//error'))

is the os.path.join necessary here?

10.) I guess you can rewrite the error message in load() method using 
list comprehension:


@@ -76,9 +76,9 @@ class aug_obj(object):
 self._aug.load()
 errors = self._aug.match(os.path.join('//error'))
 if errors:
-err_msg = ""
-for error in errors:
-err_msg += ("{}: {}".format(error, self._aug.get(error)))
+err_msg = '\n'.join(
+["{}: {}".format(e, self._aug.get(e)) for e in errors]
+)
 raise RuntimeError(err_msg)
 self._loaded = True

11.)

+class aug_node(collections.MutableMapping):
+""" Single augeas node.
+Can be handled as python dict().
+"""
+def __init__(self, aug, path):
+self._aug = aug
+if path and os.path.isabs(path):
+self._path = path
+else:
+self._tmp = _tmp_path(aug, path)
+self._path = self._tmp.path

Isn't it better to change signature of __init__ to:

def __init__(self, aug, path=None):

and then test whether path is None?

12.)

def __setitem__(self, key, node):
+target = os.path.join(self._path, key)
+end = '{0}[0]'.format(os.path.join(self._path, key))
+if self._aug.match(target):
+self._aug.remove(target)
+target_list = aug_list(self._aug, target)
+for src_node in aug_list(node._aug, node._path):
+target_list.append(src_node)

The 'end' variable is declared but not used.

13.)

+
+def __len__(self):
+return len(self._aug.match('{0}/*'.format(self._path)))
+
+def __iter__(self):
+for key in self._aug.match('{0}/*'.format(self._path)):
+yield self._aug.label(key)
+raise StopIteration()
+

Shouldn't we construct paths using os.path.join for the sake of consistency?

14.)

+def __bool__(self):
+return (bool(len(self)) or bool(self.value))

The

Re: [Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands

2016-03-14 Thread Jan Cholasta 

On 14.3.2016 12:50, Martin Basti wrote:



On 14.03.2016 12:05, Jan Cholasta wrote:

Hi,

On 11.3.2016 10:39, Stanislav Laznicka wrote:

Hi,

Please see the patch attached. Contrary to the discussion at
https://fedorahosted.org/freeipa/ticket/4987 I also added the suffix
option for clean_ruv command. If this command is available for normal
RUVs, it should probably be available for CS-RUVs as well (or deprecated
for both with advised use of clean_dangling_ruv).


ipa-csreplica-manage is used to manage the CA suffix, so
ipa-csreplica-manage should be extended instead of adding --suffix
option to ipa-replica-manage. Having half of the CA suffix managed by
ipa-replica-manage and the other half by ipa-replica-manage is confusing.

Honza


There is a design document about deprecating ipa-csreplica-manage and
move part of its responsibilities to ipa-replica-manage.

http://www.freeipa.org/page/V4/Manage_replication_topology_4_4#ipa.28cs.29replica_manange_changes


So patch is compatible with design.


The design is wrong then.

Either do it in ipa-csreplica-manage, or make *all* ipa-replica-manage 
sub-commands respect the --suffix option. Anything else is inconsistent 
mess.


--
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 0024] ipa-replica-manage: added --suffix option for certain commands

2016-03-14 Thread Martin Basti 



On 14.03.2016 12:05, Jan Cholasta wrote:

Hi,

On 11.3.2016 10:39, Stanislav Laznicka wrote:

Hi,

Please see the patch attached. Contrary to the discussion at
https://fedorahosted.org/freeipa/ticket/4987 I also added the suffix
option for clean_ruv command. If this command is available for normal
RUVs, it should probably be available for CS-RUVs as well (or deprecated
for both with advised use of clean_dangling_ruv).


ipa-csreplica-manage is used to manage the CA suffix, so 
ipa-csreplica-manage should be extended instead of adding --suffix 
option to ipa-replica-manage. Having half of the CA suffix managed by 
ipa-replica-manage and the other half by ipa-replica-manage is confusing.


Honza

There is a design document about deprecating ipa-csreplica-manage and 
move part of its responsibilities to ipa-replica-manage.


http://www.freeipa.org/page/V4/Manage_replication_topology_4_4#ipa.28cs.29replica_manange_changes

So patch is compatible with design.

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 0024] ipa-replica-manage: added --suffix option for certain commands

2016-03-14 Thread Jan Cholasta 

Hi,

On 11.3.2016 10:39, Stanislav Laznicka wrote:

Hi,

Please see the patch attached. Contrary to the discussion at
https://fedorahosted.org/freeipa/ticket/4987 I also added the suffix
option for clean_ruv command. If this command is available for normal
RUVs, it should probably be available for CS-RUVs as well (or deprecated
for both with advised use of clean_dangling_ruv).


ipa-csreplica-manage is used to manage the CA suffix, so 
ipa-csreplica-manage should be extended instead of adding --suffix 
option to ipa-replica-manage. Having half of the CA suffix managed by 
ipa-replica-manage and the other half by ipa-replica-manage is confusing.


Honza

--
Jan Cholasta

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

[Freeipa-devel] [PATCH 550] certdb: never use the -r option of certutil

2016-03-14 Thread Jan Cholasta 

Hi,

the attached patch fixes  
and .


Honza

--
Jan Cholasta
From efd94957c00021f08560fd67eeb083ee2c2a260e Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Thu, 10 Mar 2016 13:16:41 +0100
Subject: [PATCH] certdb: never use the -r option of certutil

The -r option makes certutil output certificates in DER. If there are
multiple certificates sharing the same nickname, certutil will output
them concatenated into a single blob. The blob is not a valid DER
anymore and causes failures further in the code.

Use the -a option instead to output the certificates in PEM and convert
them to DER on demand.

https://fedorahosted.org/freeipa/ticket/5117
https://fedorahosted.org/freeipa/ticket/5720
---
 ipapython/certdb.py | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/ipapython/certdb.py b/ipapython/certdb.py
index aea50a8..e19f712 100644
--- a/ipapython/certdb.py
+++ b/ipapython/certdb.py
@@ -425,19 +425,17 @@ class NSSDatabase(object):
 "Setting trust on %s failed" % root_nickname)
 
 def get_cert(self, nickname, pem=False):
-args = ['-L', '-n', nickname]
-if pem:
-args.append('-a')
-else:
-args.append('-r')
+args = ['-L', '-n', nickname, '-a']
 try:
-result = self.run_certutil(args, capture_output=pem)
+result = self.run_certutil(args, capture_output=True)
 except ipautil.CalledProcessError:
 raise RuntimeError("Failed to get %s" % nickname)
-if pem:
-return result.output
-else:
-return result.raw_output
+cert = result.output
+if not pem:
+(cert, start) = find_cert_from_txt(cert, start=0)
+cert = x509.strip_header(cert)
+cert = base64.b64decode(cert)
+return cert
 
 def has_nickname(self, nickname):
 try:
-- 
2.5.0

From 84ce28a1e7983e5f4169be772a3f041ae64525f2 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Thu, 10 Mar 2016 13:16:41 +0100
Subject: [PATCH] certdb: never use the -r option of certutil

The -r option makes certutil output certificates in DER. If there are
multiple certificates sharing the same nickname, certutil will output
them concatenated into a single blob. The blob is not a valid DER
anymore and causes failures further in the code.

Use the -a option instead to output the certificates in PEM and convert
them to DER on demand.

https://fedorahosted.org/freeipa/ticket/5117
https://fedorahosted.org/freeipa/ticket/5720
---
 ipapython/certdb.py | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ipapython/certdb.py b/ipapython/certdb.py
index 5a6e494..63dc458 100644
--- a/ipapython/certdb.py
+++ b/ipapython/certdb.py
@@ -395,15 +395,15 @@ class NSSDatabase(object):
 "Setting trust on %s failed" % root_nickname)
 
 def get_cert(self, nickname, pem=False):
-args = ['-L', '-n', nickname]
-if pem:
-args.append('-a')
-else:
-args.append('-r')
+args = ['-L', '-n', nickname, '-a']
 try:
 cert, err, returncode = self.run_certutil(args)
 except ipautil.CalledProcessError:
 raise RuntimeError("Failed to get %s" % nickname)
+if not pem:
+(cert, start) = find_cert_from_txt(cert, start=0)
+cert = x509.strip_header(cert)
+cert = base64.b64decode(cert)
 return cert
 
 def has_nickname(self, nickname):
-- 
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 0434] log: add timestamp to filename of logs

2016-03-14 Thread Jan Cholasta 

On 11.3.2016 15:56, Gabe Alford wrote:

On Fri, Mar 11, 2016 at 7:35 AM, Petr Vobornik > wrote:

On 03/11/2016 03:00 PM, Rob Crittenden wrote:

Martin Kosek wrote:

On 03/11/2016 09:55 AM, Jan Cholasta wrote:

On 11.3.2016 09:33, Martin Kosek wrote:

On 03/08/2016 07:07 PM, Martin Basti wrote:



On 08.03.2016 16:37, Martin Basti wrote:



On 08.03.2016 16:31, Martin Basti wrote:

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

Patch attached.


Rebased patch attached.



self-NACK

Scripts print to CLI unformatted strings, it
should not be so easy.
See /var/log/ipaupgrade-{timestamp}.log for more
information


second-NACK. We cannot break existing log file
paths. The paths are mentioned
in a documentation and there may be also automation
around that (gathering log
files). So there should be always symlink from the
well known location to the
newest timestampe'd log.


Sorry, but this is absurd. What's the point of
maintaining backward
compatibility with obsolete documentation? Following
this logic, we would not
be able to change anything ever. What we should actually
do is update the
documentation. Ditto for automation.


+1 for updating the automation and documentation. But some
backward
compatibility will need to be retained, at least for the
stable systems like
RHEL where *other* people may have some automation or
documentation around it,
not just us.


Or you could just also create a symlink to the old name and it will
always just work.

rob


Aren't the symlinks what Martin2 mentioned in second-NACK?

These new timestamped logs should be combined with the Gabe's
patches: #5728 (renamed to command name) and #5724 (move to
/var/log/ipa directory).

So that there will be e.g.:
/var/log/ipaserver-install.log ->
/var/log/ipa-server-install-{timestamp}.log

/var/log/ipa/ipa-server-install.log ->
/var/log/ipa-server-install-{timestamp}.log


I wonder if it would be simpler/better to always write to the *.log
file, and then have old logs timestamped rather than write directly to a
timestamped log file?
Then just symlink the original log file in /var/log/ to the new log file
name/location in /var/log/ipa.

For example:
/var/log/ipaserver-install.log ->
/var/log/ipa/ipa-server-install.log<-- We write to this
log (current)

/var/log/ipa-server-install-{timestamp}.log   <-- Old log with some date

/var/log/ipa-server-install-{timestamp}.log   <-- Older log with some date

/var/log/ipa-server-install-{timestamp}.log   <-- Oldest log with some date


This is way too overengineered for something that should actually be 
really simple. I don't care if it is done this way or not, but IMHO it 
would be a waste of time. Logs are not API and should not be treated as 
such. If it needs to be done differently on RHEL, it should be handled 
downstream.


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