Re: [Freeipa-devel] Please review: V4/AD user short names design draft
On Tue, Feb 28, 2017 at 02:48:02PM +0200, Alexander Bokovoy wrote: > On ti, 28 helmi 2017, Martin Babinsky wrote: > > Hello list, > > > > I have put together a draft of design page describing server-side > > implementation of user short name -> fully-qualified name resolution.[1] > > > > In the end I have taken the liberty to change a few aspects of the > > design we have agreed on before and I will be grad if we can discuss > > them further. > > > > Me and Honza have discussed the object that should hold the domain > > resolution order and given the fact that IPA domain can also be a part > > of this list, we have decided that this information is no longer bound > > to trust configuration and should be a part of the global config > > instead. > > > > Also we have purposefully cut down the API only to a raw manipulation of > > the attribute using an option of `ipa config-mod`. The reasons for this > > are twofold: > > > > * the developer resources are quite scarce and it may be good to follow > > YAGNI[2] principle to implement the dumbest API now and not to invest > > into more high-level interface unless there is a demand for it > > > > * we can imagine that the manipulation of the domain resolution order > > is a rare operation (ideally only once all trusts are established), so I > > am not convinced that it is worth investing into designing higher-level > > API > > > > I propose we first develop the "dumber" parts first to unblock the SSSD > > part. If we have spare cycle afterwards then we can design and implement > > more bells-and-whistles afterwards. > Looks mostly OK, but there are few comments I have: > > - I do not see you mention how validation of the > ipaDomainResolutionOrder is done. This is important to avoid hard to > debug issues because SSSD will ignore domains it doesn't know about. > > - Space separator initially caused me to look up DNS RFCs as strictly > speaking domain names can contain any 8-bit octet (while host names > should follow LDH rule). But then [1] does explicitly say space is not > allowed in AD domain names. > > - "If ipaDomainResolutionOrder is empty then *all* users must use fully > qualified names." This is not correct with regards to the current > behavior. I think we should change this to "if > ipaDomainResolutionOrder is empty, then standard SSSD configuration > logic applies on each client." This would make current behavior > compatible with either empty or ipaDomainResolutionOrder value of > a single IPA domain name. Would it make sense to add ipaDomainResolutionOrder attribute during upgrade with the FreeIPA domain and have the behavior as proposed? That would ensure that users will be resolved the same way as before unless someone changes the attribute. > > - There are typos in the page. > > [1] > https://support.microsoft.com/en-us/help/909264/naming-conventions-in-active-directory-for-computers,-domains,-sites,-and-ous > > > -- > / 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 -- David Kupka signature.asc Description: PGP signature -- 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] Certificate Identity Mapping
Hello everyone! I would like to bring your attention to just published PRs implementing FreeIPA part of Certificate Identity Mapping feature [0]: - certmap plugin [1] by Flo - WebUI for certmap plugin [3] by Pavel - tests for certmap plugin [2] by me Also please think about names of the commands, parameters, entries and attributes. We've figured them somehow but if you have any suggestion that would improve the understanding please share. Please review them thoroughly, thanks! [0] https://www.freeipa.org/page/V4/Certificate_Identity_Mapping [1] https://github.com/freeipa/freeipa/pull/398 [2] https://github.com/freeipa/freeipa/pull/399 [3] https://github.com/freeipa/freeipa/pull/400 -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] Stageuser API
On 17/01/17 12:38, Christian Heimes wrote:
On 2017-01-16 15:52, David Kupka wrote:
Hello everyone!
I've noticed that our API for stageuser is missing some commands that
user has (stageuser-{add,remove}-{principal,cert}). I was wondering if
there is reason for it but after asking some fellows developers it seems
that there's none.
I understand the stageuser area as a place where user entry can be
created and amended during the hiring process in organization, example:
1. HR creates the entry with just basic informations (givenname,
surname, manager)
2. IT assigns basic account information (uid, gid)
3. based on to-be-employee manager's request IT adds additional group
membership (memberOf)
4. based on to-be-employee request IT adds login alias (krbPrincipalName)
5. Security Officer adds certificate from Smart Card assigned to the
to-be-employee
6. HR adds extra information to the account (address, marital status, ...)
7. Facilities update work place related information (seat number, phone
number, ...)
8. At the first day IT activates the user account.
Considering this work flow I think it might be useful to have the same
API for stageuser as for the user.
Does the example work flow make sense?
Should we provide the same set of commands for user and stageuser?
I see one potential issue in your proposal. A stage user does not
reserve its user name. The unique index on uid excludes the staging user
and deleted user branch. Therefore it is possible to create a user with
the same login name as a staging user.
We either have to ensure that this name clash does not cause any trouble
with certificates or we have to enforce uniqueness of uid across the
whole tree. For FreeIPA it's probably fine because we compare certs
bytes. Third party applications parse the cert subject instead and use
the subject to identify a user.
Christian
Hi Christian,
uniqueness of uid is not checked in staging area on purpose, it may be
changed multiple times before the stageuser is transformed into user
(activated). The uid uniqueness is then checked during activation.
Third party application that use FreeIPA's LDAP should:
1) search for users (and usercertificate attribute) only in
cn=users,cn=accounts
2) respect the value of nsAccountLock that is set to true for all staged
users.
But it would be nice to have this scenario (stageuser.uid == user.uid)
implemented as a part of [1].
[1] https://fedorahosted.org/freeipa/ticket/6615
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [DESIGN] IPA permission enforcement in Dogtag
On 13/01/17 08:07, Fraser Tweedale wrote: Related to design: http://www.freeipa.org/page/V4/Dogtag_GSS-API_Authentication Currently there are some operations that hit the CA that involve a number of privileged operations against the CA, but for which there is only one associated IPA permission. Deleting a CA is a good example (but it is one specific case of a more general issue). Summary of current ca-del behaviour: 1. Disable LWCA in Dogtag (uses RA Agent cert) 2. Delete LWCA in Dogtag (uses RA Agent cert) 3. Delete CA entry from IPA (requires "System: Delete CA" permission) So there are two things going on under the hood: a modify operation (disable CA) and the delete. When we implement proxy authentication to Dogtag, Dogtag will enforce the IPA permissions on its operations. Disable will map to "System: Modify CA" and delete to "System: Delete CA". So to delete a CA a user will need *both* permissions. Which could be surprising. There are a couple of reasonable approaches to this. 1. Decouple the disable and delete operations. If CA is not disabled, the user will be instructed to execute the ca-disable command separately before they can disable the CA. This introduces an additional manual step for operators. 2. Just improve the error reporting. In my WIP, for a user that has 'System: Delete CA' permission but not 'System: Modify CA', the reported failure is a 403 Authorization Error from Dogtag. We can add guards to fail more gracefully. I lean towards #2 because I guess the common case will be that users either get all CA admin permissions, or none, and we don't want to make more work (in the form of more commands to run) for users in the common case. I welcome alternative views and suggestions. Thanks, Fraser Hi Fraser, as a user with "System: Delete CA" permission calling "ca-del" command I would be really surprised that I don't have enough privileges to complete the action. I would expect: a) "Cannot delete active CA, disable it first" error. b) Delete will be completed successfully. All internal and to my sight hidden operations will be allowed just because I'm allowed to perform the delete operation. I think that b) might lead to strange exceptions in authorization checking and therefore to security issues. So I would prefer decoupling ca-disable and ca-del as you're describing in 1). -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] Stageuser API
Hello everyone!
I've noticed that our API for stageuser is missing some commands that
user has (stageuser-{add,remove}-{principal,cert}). I was wondering if
there is reason for it but after asking some fellows developers it seems
that there's none.
I understand the stageuser area as a place where user entry can be
created and amended during the hiring process in organization, example:
1. HR creates the entry with just basic informations (givenname,
surname, manager)
2. IT assigns basic account information (uid, gid)
3. based on to-be-employee manager's request IT adds additional group
membership (memberOf)
4. based on to-be-employee request IT adds login alias (krbPrincipalName)
5. Security Officer adds certificate from Smart Card assigned to the
to-be-employee
6. HR adds extra information to the account (address, marital status, ...)
7. Facilities update work place related information (seat number, phone
number, ...)
8. At the first day IT activates the user account.
Considering this work flow I think it might be useful to have the same
API for stageuser as for the user.
Does the example work flow make sense?
Should we provide the same set of commands for user and stageuser?
Thanks for your ideas and opinions!
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] NTP in FreeIPA
On 30/11/16 16:09, Rob Crittenden wrote: David Kupka wrote: On 29/11/16 18:10, Alexander Bokovoy wrote: Still, bug reports and users' complaints is the only external measure we have. There are close to nothing in complaints about NTP functionality, other than requests to support chronyd and a better discover of existing NTP setups. I don't think that requires dramatic action like removal of NTP support at all. As Petr already pointed out, since Fedora 16 chronyd is enabled by default and ipa-client-install doesn't configure time synchronization when chronyd is enabled. I believe that majority of users haven't used '--force-ntpd' and since it still worked they haven't filed any ticket. IMO in this case no bug reports means no users rather than no bugs or requests. Unfortunately, this is just my guess and AFAIK we don't have any data from users showing how they use FreeIPA. For argument's sake, let's say NTP configuration in the client is dropped and managed by the OS or other administrators. What implication does this have for configuring NTP server on masters? Would that be stopped as well? What about existing installs? I don't believe there is a precedence for removing a service from IPA. rob Well, everything was done for the first time at some point in history. I would prefer removing it from server too. I imagine it this way: 0. We agree that NTP as FreeIPA service will be dropped in 4.x 1. We add big fat warning to nearest release (currently 4.5) that FreeIPA will stop supporting NTP as its service on server and client and if NTP was configured by FreeIPA (we can tell from sysrestore) upgrade will revert those changes. 2. New installations of 4.x will not configure NTP on server nor client. Upgrades to 4.x will revert configuration if done by FreeIPA. I think it's actually that simple. The only hard part is reaching the agreement. While I understand that the value of FreeIPA is entirely in taking care of non-trivial services and orchestrating them in a way most comfortable for the administrator I think configuring NTP is: * reasonably easy (<5 lines on client, <10 lines on server), * unnecessary in most cases (distributions defaults or DHCP+NetworkManager just work) and so not worth keeping in FreeIPA. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] NTP in FreeIPA
On 29/11/16 18:10, Alexander Bokovoy wrote: On ti, 29 marras 2016, Petr Spacek wrote: On 29.11.2016 16:02, Rob Crittenden wrote: Petr Spacek wrote: On 29.11.2016 09:11, Jan Cholasta wrote: On 28.11.2016 20:57, Rob Crittenden wrote: David Kupka wrote: On 22/11/16 23:15, Gabe Alford wrote: I would say that it is worth keeping in FreeIPA. I know myself and some customers use its functionality by having the clients sync to the IPA servers and have the servers sync to the NTP source. This way if the NTP source ever gets disrupted for long periods of time (which has happened in my environment) the client time drifts with the authentication source. This is the way that AD often works and is configured. Hello Gabe, I agree that it's common practice to synchronize all nodes in network with single source in order to have the same time and save bandwidth. Also I understand that it's comfortable to let FreeIPA installer take care of it. But I don't think FreeIPA should do it IMO this is job for Ansible or similar tool. Also the problem is that in some situations FreeIPA installer makes it worse. Example: 1. Install FreeIPA server (ipa1.example.org) 2. Install FreeIPA client on all nodes in network 3. Install replica (ipa2.example.org) of FreeIPA server to increase redundancy Now all the clients have ipa1.example.org as the only server in /etc/ntp.conf. If the first FreeIPA server becomes unreachable all clients will be able to contact KDC on the other server thanks to DNS autodiscovery in libkrb5 but will be unable to synchronize time. Remember that the goal of IPA was to herd together a bunch of software to make hard things easier. This included dealing with the 5-minute Kerberos window so ntp was configured on the client and server (which is less of any issue now). When making changes you have to ask yourself who are you making this easier for: you or the user. Yes, getting NTP right is hard, but does it meet the 80/20 rule in terms of success? I'd think so. I If someone wants to configure it using Ansible they can use the --no-ntp. If they want to use different time servers they can pass in --ntp-server. But by default IMHO it should do something sane to give a good experience. I think to do something sane is exactly the point of this, and the sanest thing we can do is to not touch NTP configuration at all: * if the NTP configuration obtained via DHCP works, we can't make it any better by touching it, only worse, * if the default NTP configuration shipped with the distribution works, we again can't make it any better by touching it, * if we are running inside container, time is synchronized by other means and we should not touch NTP configuration at all, * if neither the default NTP configuration nor the NTP configuration obtained via DHCP works and we are not running inside container, we may attempt to fix the configuration, but it will not be permanent and will work only for this specific host. I think the first 3 points cover 99% of real-life deployments, and yet we are optimized towards the remaining 1%, with the potential of breaking the configuration for the 99%. This is far from sane IMHO. +1 for Honza's point. Current NTP code is works only for initial setup and silently breaks synchronization later on. Most importantly it breaks synchronization as soon as admin removes old replicas and replaces them with new ones - there is no mechanism to update the records in the client configuration (and SRV discovery is not supported by clients). I.e. when admin decommission replicas which were around at the time of client installation, the NTP on client will silently break. This would not happen if you did not touch it. (This also implicitly means that IPA-configured NTP is broken on all clients in topologies which were completely migrated from RHEL 6 to RHEL 7.) Either DHCP or default distro config would solve the problem better. That's fair but where are the huge pile of bugs, tickets and user e-mails complaining about time? Or has nobody noticed yet? Hard to say. There might be multiple reasons for this. E.g. - Starting with Fedora 16, there is Chronyd installed by default. IPA client installer does not configure Chronyd by default so there is nothing to break. - DHCP integration still modifies IPA-generated ntp.conf. - Users who care might use configuration management tool. Still, bug reports and users' complaints is the only external measure we have. There are close to nothing in complaints about NTP functionality, other than requests to support chronyd and a better discover of existing NTP setups. I don't think that requires dramatic action like removal of NTP support at all. As Petr already pointed out, since Fedora 16 chronyd is enabled by default and ipa-client-install doesn't configure time synchronization when chronyd is enabled. I believe that majority of users haven't used '--force-ntpd&#x
Re: [Freeipa-devel] NTP in FreeIPA
On 22/11/16 23:15, Gabe Alford wrote:
I would say that it is worth keeping in FreeIPA. I know myself and some
customers use its functionality by having the clients sync to the IPA
servers and have the servers sync to the NTP source. This way if the NTP
source ever gets disrupted for long periods of time (which has happened in
my environment) the client time drifts with the authentication source. This
is the way that AD often works and is configured.
Hello Gabe,
I agree that it's common practice to synchronize all nodes in network
with single source in order to have the same time and save bandwidth.
Also I understand that it's comfortable to let FreeIPA installer take
care of it.
But I don't think FreeIPA should do it IMO this is job for Ansible or
similar tool. Also the problem is that in some situations FreeIPA
installer makes it worse.
Example:
1. Install FreeIPA server (ipa1.example.org)
2. Install FreeIPA client on all nodes in network
3. Install replica (ipa2.example.org) of FreeIPA server to increase
redundancy
Now all the clients have ipa1.example.org as the only server in
/etc/ntp.conf. If the first FreeIPA server becomes unreachable all
clients will be able to contact KDC on the other server thanks to DNS
autodiscovery in libkrb5 but will be unable to synchronize time.
On Tue, Nov 22, 2016 at 7:05 AM, Jan Cholasta wrote:
On 22.11.2016 13:06, Petr Spacek wrote:
On 22.11.2016 12:15, David Kupka wrote:
Hello everyone!
Is it worth to keep configuring NTP in FreeIPA?
In usual environment there're no special requirements for time
synchronization
and the distribution default (be it ntpd, chrony or anything else) will
just
work. Any tampering with the configuration can't make it any better.
In environment with special requirements (network disconnected from
public
internet, nodes disconnected from topology for longer time, ...) time
synchronization must be taken care of accordingly by system
administrator and
FreeIPA simply can't help here.
Also there are problems and weird behavior with the current FreeIPA
installers:
* ipa-client-install replaces all servers in /etc/ntp.conf with the ones
specified by user or resolved from DNS. If none were provided nor
resolved the
FreeIPA server specified/resolved during installation it used. This
leads in
just single server in the configuration and no time synchronization when
this
server is down/decommissioned.
* ipa-client-install replaces the NTP configuration. If there was any
parts
previously edited by system administrator it's lost.
* ipa-server-install adds {0-4}.$PLATFORM.pool.ntp.org to /etc/ntp.conf.
What's the point in doing that? These servers're already in the
configuration
file installed with ntp package.
I have NTP-related WIP patches that solve some of the issues but in
general I
would prefer to remove the whole thing together with documenting "Please
make
sure that time on all FreeIPA servers and clients is synchronized. On
most
distributions this was already done during system installation."
Can we mark NTP options deprecated in 4.5 and remove them and stop
touching
any time syncing service in 4.6?
Considering that default config is just fine for normal cases, and given
how
poorly integrated it is into FreeIPA, I agree with David. FreeIPA should
get
out of configuration management business.
+1
--
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
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] Removing ipa.pot file from git tree
On 22/11/16 13:37, Martin Basti wrote: Hello list, we plan to remove ipa.pot file from git tree, as this is file can be generated from code during build time, and it is required only for pushing sources to Zanata. Does anybody remember reason why this file was added into git tree? Note: Translated strings (*.po files) will remain in git tree. If nobody is against it will be removed from git today, as it creates only huge diffs all the time without any extra value. Martin^2 Hi! +1, if we can generate it there's no reason to keep it in git. git log reveals that is was added back in 2010 when adding support for internalization: https://git.fedorahosted.org/cgit/freeipa.git/commit/?id=4461a74 -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] NTP in FreeIPA
Hello everyone!
Is it worth to keep configuring NTP in FreeIPA?
In usual environment there're no special requirements for time
synchronization and the distribution default (be it ntpd, chrony or
anything else) will just work. Any tampering with the configuration
can't make it any better.
In environment with special requirements (network disconnected from
public internet, nodes disconnected from topology for longer time, ...)
time synchronization must be taken care of accordingly by system
administrator and FreeIPA simply can't help here.
Also there are problems and weird behavior with the current FreeIPA
installers:
* ipa-client-install replaces all servers in /etc/ntp.conf with the ones
specified by user or resolved from DNS. If none were provided nor
resolved the FreeIPA server specified/resolved during installation it
used. This leads in just single server in the configuration and no time
synchronization when this server is down/decommissioned.
* ipa-client-install replaces the NTP configuration. If there was any
parts previously edited by system administrator it's lost.
* ipa-server-install adds {0-4}.$PLATFORM.pool.ntp.org to /etc/ntp.conf.
What's the point in doing that? These servers're already in the
configuration file installed with ntp package.
I have NTP-related WIP patches that solve some of the issues but in
general I would prefer to remove the whole thing together with
documenting "Please make sure that time on all FreeIPA servers and
clients is synchronized. On most distributions this was already done
during system installation."
Can we mark NTP options deprecated in 4.5 and remove them and stop
touching any time syncing service in 4.6?
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] Refactoring certificate-handling code: Staging branch and copr
Hello! If you're interested in certificate refactoring you can find our staging branch here [0] and builds in copr repository here [1]. Currently the branch is identical to master and copr contains some test builds but this will change soon. [0] https://github.com/dkupka/freeipa/tree/refactoring-certificates [1] https://copr.fedorainfracloud.org/coprs/dkupka/freeipa-refactoring-certificates/ -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] Building refactoring branches in copr
Hello everyone, as we already agreed we will have branches for our refactoring efforts somewhere and we will build them in copr so then can be easily consumed by CI and also everyone else willing to test them. I already put together simple script that pulls new patches makes srpm and submits it to copr build system. In order to avoid everyone else inventing the same wheel I can add all the refactoring branches, create copr repos and run the script with cron. If you would like me to take care about your refactoring branch just send me its location and consider it done. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] Feature branches for sub-team efforts
On 11/10/16 16:27, Alexander Bokovoy wrote:
On ti, 11 loka 2016, Petr Vobornik wrote:
On 10/11/2016 03:50 PM, Alexander Bokovoy wrote:
On ti, 11 loka 2016, Petr Vobornik wrote:
Hi List,
we discussed locally a proposal about creating a feature branch for
each
sub-team effort in our main git. Currently it would be for the 4
ongoing
refactoring efforts + Simo's work
Why?
It will allow each developer to create a pull request against the
feature branch and thus it will enable iterative development by
multiple
devs without affecting other sub-teams. When the
feature(refactoring) is
finished, the branch would be rebased on master and merged there. Note:
rebases can be done as needed - e.g. when other subteam finishes its
work.
Concerns:
1. Upstream git repo would be full of such branches.
- This can be mitigated by deleting the feature branches when their are
released or merged(up to discussion)
Don't put them in the upstream git repo. Let people decide where they
want to have them -- all Fedora contributors have access to
fedorapeople.org git hosting and there is github one button click
('Clone') away from the github mirror.
It is not a problem to keep a separate git branch published this way.
It is not a matter of making the code public. That can be done easily as
you write. Other alternative is own branch in GitHub fork.
May be I misunderstand you -- if you just want people to propose merge
requests on github with pre-defined names, then that's just fine.
Basically it's all about review.
Example use case: More than 1 devs are working on a same big effort.
This effort will probably consists of 10s of commits. The big effort is
divided into smaller ones which can be implemented and reviewed
separately. In our previous mode, the sub task would be merged to master
it is reviewed and ACKed. But now we cannot do that. We want to merge
the whole big task at once when it is finishes and tested.
One dev could probably have a branch on personal fork of FreeIPA on
GitHub which would work as the feature branch. Other team members would
create pull requests against it.
In such case we would loose mail notifications and would have to extend
our tooling because ipatool can use only one upstream on push.
So I still think this is not a problem. If people can agree which git
repo clone will be primary one and submit merge requests against it,
then there is no problem in having that git repo's branch to be
submitted as the pull request against the main git repo. This way you'll
get all the changes seen at the pull request sync time.
From my POV, when we create the refactoring branches in upstream we get
this for free:
* our minimal but convenient CI (lint + build on each PR)
* mail notifications
* tooling (ipatool pr-push XYZ -b refactoring-xyz just works)
When creating them elsewhere we get:
* confusion (no team-wide notifications, each effort in other fork)
* manual rebasing and pushing
So I think it's best to create the branches in upstream repo. I don't
care about names and also I don't care what happens with them after the
effort is done.
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0065] Fix ugly quit during external CA installation
On 23/08/16 13:58, Standa Laznicka wrote: https://fedorahosted.org/freeipa/ticket/6230 Thanks for the patch. This fixes the ugly error message and also the return code, ACK. Pushed to: master: 889f0863b80a0c13a14aa69cd8563b5adde984b2 ipa-4-4: 03a0f5a105f5625e6a4d373abb1f4d8b8044a026 -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] ca-less tests updated
Hi Oleg! As we discussed this morning there're still some issues [1][2] but I think the patch set is in reasonably good state and I'm not sure that the issues are bugs in IPA or in tests. This can be investigated and fixed later. ACK. Pushed to master: * bbac233b5ee487ab0e035cf0b861144769a0b738 tests: Fixed method failures during second call for the method * 2f6ffa326adb4d4e9152463ffa733d559f7be2af tests: Added basic constraints extension to the CA certs * 0c635686ddc6dbba54285a9c7844e12342083b9b tests: Added generation of missing certs * 38ad864342bb3fcbf65397b763c240f034f3e2c7 tests: Updated ipa server installation stdin text * c0e16aa3b9c380fb7936dc18c4bfc04e7f8327b5 tests: Create a method that cleans all ipa certs * 48ca465a12a91977eb57bb791cf0b098e5e5a4b3 tests: Added teardown methods for server and replica installation * 725d8d0cac16f6a41ad54ae319e3822d44047031 tests: Removed call for install method from parent class * fad6ec8256a97fbf06b3e4509d93af1d159e6b81 tests: Adapted installation methods to utilize methods from tasks * 84db13f676771746f9ab7769073837a29f6de464 tests: Fixed incorrect assert in verify_installation * a81d8472042f4909020c073ecb00a37d8e05ec33 tests: Applied correct teardown methods * 759bbcdfcbeade91c77b201c439c939d6477cd08 tests: Removed outdated command options test * d17d13d77a7c09cbc99c8bb0a3f7af3b72da8aca tests: Added necessary getkeytabs calls to fixtures * 24f218f4ebe203213eebede59ff79b89c657ee76 tests: Added necessary xfails * e0b67dfa7e957cc134043b265b87ed71bb09a7d3 tests: Updated master and replica installation methods to enable negative testing * 9217bcc871468615110c85b1131b62735f9e5092 tests: Made unapply_fixes call optional at master uninstallation * bb4205b582038669888544786a5611b18e52bf42 tests: Enabled negative testing for cleaning replication agreements * dbf0d141c5a4d1ccd1b681ac49cb57e47234aaa4 tests: Replaced hardcoded certutil with imported from paths * b8cf212e8bbe301f6d44551ecac063d12a042520 tests: Replaced unused setUp method with install * 804aae81966f23e307ec86364489f808fd0c3357 tests: fixed expects of incorrect error messages * 43994e669743bb8f54e32b52f2410ebde3660f04 tests: Fixed Usage of improper certs in ca-less tests * b8968d923cedbbeb931c3ed33b81b299a55baf4a tests: Implemented check for domainlevel before installation verification * 106f37c26f64492f771214409d229feb5d70a113 tests: Standardized replica_preparation in test_no_certs * 8be0906b04bbf995af6326b560151d15901b544e tests: added verbose assert to test_service_disable_doesnt_revoke * f1f94a7b9fe354d93f31ce8cd606d985dd44703b tests: fixed super method invocation * 7412f0cb20801e1608f8cf388210e57ef7d27497 tests: fixed certinstall method * 9870c5804a65ae320ebbcb313f8facb21963f710 tests: Reverted erroneous asserts in 4 tests * 47c808afa35f0708ca00ac8e98851c9f8d75badc tests: Fixed code styling in caless tests to make pep8 happy [1] https://fedorahosted.org/freeipa/ticket/6346 [2] https://fedorahosted.org/freeipa/ticket/6348 On 22/09/16 12:55, Oleg Fayans wrote: Fixed patch N 41 On 09/21/2016 04:21 PM, Oleg Fayans wrote: Patch-0076 rebased to current master On 09/21/2016 02:41 PM, Oleg Fayans wrote: Hi David, As per your comments the patches were once again refactored. I am attaching the full set of them, please ignore any previous versions The patches apply cleanly on master and pylint swallows the resulting code silently On 09/12/2016 09:51 AM, David Kupka wrote: Hi Oleg, thank you, now it's completely different game. Please add prefix to commit message summaries. Simply prepending "tests: " should be OK. 0041 - -h is deprecated in favor of -H. 0062 - 0068 - LGTM 0069 - I see 2 unrelated changes in the patch, please split them: - 1 - certutil - > paths.CERTUTIL - 2 - assert 0070 - I see 2 unrelated changes in the patch, please split them: - 1 - teardown - 2 - TestReplicaInstall.setUp -> TestReplicaInstall.install 0071 - typos in commit message, I see 5 unrelated changes in that patch: - 1 - error messages in assert - 2 - certificates used - 3 - verify_installation called only in DOMAIN_LEVEL_0. - 4 - TestCertinstall.install - 5 - TestCertinstall.certinstall 0072 - 0077 - LGTM On 09/09/16 15:22, Oleg Fayans wrote: Hi David, team According to your suggestions I've splitted my commits so that each commit addresses some particular problem. One patch (0071) still contains several unrelated fixes, but they mostly reflect changes in error messages and really small but numerous bugfixes that I did not consider worthy of a separate commit each. Please, whenever you have a free time take a look at this new bunch of patches. Thanks! On 09/06/2016 04:41 PM, David Kupka wrote: Hi Oleg! 0013 - It looks like there are two unrelated changes, addition of CRL distribution extension and creating certificate signed by no longer existing CA. Please create separate patch for each of the changes, and describe the change and rea
Re: [Freeipa-devel] [PATCH] ca-less tests updated
Hi Oleg, thank you, now it's completely different game. Please add prefix to commit message summaries. Simply prepending "tests: " should be OK. 0041 - -h is deprecated in favor of -H. 0062 - 0068 - LGTM 0069 - I see 2 unrelated changes in the patch, please split them: - 1 - certutil - > paths.CERTUTIL - 2 - assert 0070 - I see 2 unrelated changes in the patch, please split them: - 1 - teardown - 2 - TestReplicaInstall.setUp -> TestReplicaInstall.install 0071 - typos in commit message, I see 5 unrelated changes in that patch: - 1 - error messages in assert - 2 - certificates used - 3 - verify_installation called only in DOMAIN_LEVEL_0. - 4 - TestCertinstall.install - 5 - TestCertinstall.certinstall 0072 - 0077 - LGTM On 09/09/16 15:22, Oleg Fayans wrote: Hi David, team According to your suggestions I've splitted my commits so that each commit addresses some particular problem. One patch (0071) still contains several unrelated fixes, but they mostly reflect changes in error messages and really small but numerous bugfixes that I did not consider worthy of a separate commit each. Please, whenever you have a free time take a look at this new bunch of patches. Thanks! On 09/06/2016 04:41 PM, David Kupka wrote: Hi Oleg! 0013 - It looks like there are two unrelated changes, addition of CRL distribution extension and creating certificate signed by no longer existing CA. Please create separate patch for each of the changes, and describe the change and reason for it in commit messages. 0014 - Could you please split the patch to "numerous" commit each fixing one error? Please also describe each fix so everyone has at least vague idea about the patch without reading its code. Also why do you introduce global variable config, I don't see its used anywhere. 0039 - It looks like multiple different changes and commit message says nothing again. Please split and describe what did you change and why. 0041 - Looks like weird workaround to me. It would be better to investigate the root cause and fix it. Or at least describe the cause in commit message and code comment if it can't be fixed. Also "-h is deprecated in favor of -H" says man 1 ldapmodify. On 05/09/16 14:32, Oleg Fayans wrote: Hi guys, Finally the ca-less tests are stable. Here in the attachment is the full set of necessary patches. On 08/09/2016 10:57 AM, Oleg Fayans wrote: Hi all, Bump for the review of the 0013 patch. The script it addresses can be reused in some WebUI tests - one more reason to have it reviewed/merged The rest patches should be re-tested, since they were prepared a good while ago On 05/10/2016 05:08 PM, Oleg Fayans wrote: Hi David, After quite a while and some more struggles here comes the updated version of the patch together with other patches fixing things in ipatests/test_integration/tasks.py Server and replica installation was refactored in a way to utilize the code from tasks.py as much as it is possible The full set of necessary patches is attached On 04/20/2016 10:35 AM, David Kupka wrote: On 19/04/16 11:13, Oleg Fayans wrote: OK, that one, though passing lint, did not actually work. I gave up my attempts to define method decorators inside the class. Now it passes lint AND works:) Hi Oleg! 1) Current commit message is useless. Please use it to describe what is the point of the patch. 2) $ git show -U0 | pep8 --diff ./ipatests/test_integration/test_caless.py:66:1: E302 expected 2 blank lines, found 1 ./ipatests/test_integration/test_caless.py:74:1: E302 expected 2 blank lines, found 1 ./ipatests/test_integration/test_caless.py:820:5: E303 too many blank lines (2) ./ipatests/test_integration/test_caless.py:825:80: E501 line too long (80 > 79 characters) ./ipatests/test_integration/test_caless.py:1035:44: E225 missing whitespace around operator 3) Isn't there a way to do this with pytest's fixtures? +def server_install_teardown(func): +def wrapped(*args): +try: +func(*args) +finally: +args[0].uninstall_server() +return wrapped + +def replica_install_teardown(func): +def wrapped(*args): +try: +func(*args) +finally: +# Uninstall replica +replica = args[0].replicas[0] +tasks.kinit_admin(args[0].master) +args[0].uninstall_server(replica) +args[0].master.run_command(['ipa-replica-manage', 'del', +replica.hostname, '--force'], + raiseonerr=False) +args[0].master.run_command(['ipa', 'host-del', +replica.hostname], + raiseonerr=False) +return wrapped + There is a standard pytest method called 'method_teardown', that is indent to be executed after each test method,
Re: [Freeipa-devel] [PATCH] ca-less tests updated
Hi Oleg!
0013 - It looks like there are two unrelated changes, addition of CRL
distribution extension and creating certificate signed by no longer
existing CA. Please create separate patch for each of the changes, and
describe the change and reason for it in commit messages.
0014 - Could you please split the patch to "numerous" commit each fixing
one error? Please also describe each fix so everyone has at least vague
idea about the patch without reading its code. Also why do you introduce
global variable config, I don't see its used anywhere.
0039 - It looks like multiple different changes and commit message says
nothing again. Please split and describe what did you change and why.
0041 - Looks like weird workaround to me. It would be better to
investigate the root cause and fix it. Or at least describe the cause in
commit message and code comment if it can't be fixed. Also "-h is
deprecated in favor of -H" says man 1 ldapmodify.
On 05/09/16 14:32, Oleg Fayans wrote:
Hi guys,
Finally the ca-less tests are stable. Here in the attachment is the full
set of necessary patches.
On 08/09/2016 10:57 AM, Oleg Fayans wrote:
Hi all,
Bump for the review of the 0013 patch. The script it addresses can be
reused in some WebUI tests - one more reason to have it reviewed/merged
The rest patches should be re-tested, since they were prepared a good
while ago
On 05/10/2016 05:08 PM, Oleg Fayans wrote:
Hi David,
After quite a while and some more struggles here comes the updated
version of the patch together with other patches fixing things in
ipatests/test_integration/tasks.py
Server and replica installation was refactored in a way to utilize the
code from tasks.py as much as it is possible
The full set of necessary patches is attached
On 04/20/2016 10:35 AM, David Kupka wrote:
On 19/04/16 11:13, Oleg Fayans wrote:
OK, that one, though passing lint, did not actually work. I gave up my
attempts to define method decorators inside the class. Now it passes
lint AND works:)
Hi Oleg!
1) Current commit message is useless. Please use it to describe what is
the point of the patch.
2) $ git show -U0 | pep8 --diff
./ipatests/test_integration/test_caless.py:66:1: E302 expected 2 blank
lines, found 1
./ipatests/test_integration/test_caless.py:74:1: E302 expected 2 blank
lines, found 1
./ipatests/test_integration/test_caless.py:820:5: E303 too many blank
lines (2)
./ipatests/test_integration/test_caless.py:825:80: E501 line too long
(80 > 79 characters)
./ipatests/test_integration/test_caless.py:1035:44: E225 missing
whitespace around operator
3) Isn't there a way to do this with pytest's fixtures?
+def server_install_teardown(func):
+def wrapped(*args):
+try:
+func(*args)
+finally:
+args[0].uninstall_server()
+return wrapped
+
+def replica_install_teardown(func):
+def wrapped(*args):
+try:
+func(*args)
+finally:
+# Uninstall replica
+replica = args[0].replicas[0]
+tasks.kinit_admin(args[0].master)
+args[0].uninstall_server(replica)
+args[0].master.run_command(['ipa-replica-manage', 'del',
+replica.hostname, '--force'],
+ raiseonerr=False)
+args[0].master.run_command(['ipa', 'host-del',
+replica.hostname],
+ raiseonerr=False)
+return wrapped
+
There is a standard pytest method called 'method_teardown', that is
indent to be executed after each test method, but with our setup it does
not work.
4) Is it necessary to create the $TEST_DIR in the test? Isn't it
created
by the framework?
+host.transport.mkdir_recursive(host.config.test_dir)
Removed.
5) I don't think the comment match the code.
+# Remove CA cert in /etc/pki/nssdb, in case of failed
(un)install
+for host in cls.get_all_hosts():
+cls.uninstall_server(host)
+
super(CALessBase, cls).uninstall(mh)
Not actual anymore
6) No! Create list with one element, iterate that list and append every
item to the other list. Maybe there's better way (Hint: append).
I've seen this on multiple places.
if unattended:
args.extend(['-U'])
Agreed
7) Why don't you (extend and) use
ipatests.test_integaration.tasks.(un)install_{master,replica}?
This could be done pretty much all over the code.
host.run_command(['ipa-server-install', '--uninstall',
'-U'])
8) Use ipaplatform.paths for certutil and other binaries. If the binary
is not there feel free to add it.
I've seen this on multiple places.
+host.run_command(['certutil', '-d',
Re: [Freeipa-devel] [PATCH 0112-7] Speeding up cli help
On 17/08/16 14:17, Jan Cholasta wrote:
On 17.8.2016 13:21, David Kupka wrote:
On 08/08/16 13:26, Jan Cholasta wrote:
On 4.8.2016 16:32, David Kupka wrote:
On 03/08/16 16:33, Jan Cholasta wrote:
On 3.8.2016 16:23, David Kupka wrote:
On 21/07/16 10:12, Jan Cholasta wrote:
Hi,
On 20.7.2016 14:32, David Kupka wrote:
On 15/07/16 12:53, David Kupka wrote:
Hello!
After Honza introduced thin client that builds plugins and
commands
dynamically from schema client became much slower. This is only
logical,
instead of importing a module client now must fetch the schema
from
server, parse it and instantiate the commands using the data.
First step to speed it up was addition of schema cache to client.
That
removed the RTT and download time of fetching schema every time.
Now the most time consuming task became displaying help for
lists of
topics and command and displaying individual topics. This is
simply
because of the need to instantiate all the commands to find the
relations between topics and commands.
All the necessary bits for server commands and topics are
already in
the
schema cache so we can skip this part and generate help from it,
right?
Not so fast!
There are client plugins with commands and topics. So we can
generate
basic bits (list of all topics, list of all commands, list of
commands
for each topic) from schema and store it in cache. Then we need
to go
through all client plugins and get similar bits for client
plugins.
Then
we can merge and print.
Still the client response is not as fast as before and I this it
even
can't be. Also first time you display particular topic or list
takes
longer because it must be freshly generated and stored in cache
for
next
use. And this is what the attached patches do.
https://fedorahosted.org/freeipa/ticket/6048
Reimplemented so there is no need to distinguish client plugins and
remote plugins.
The main idea of this approach is to avoid creating instances of
the
commands just to get the information about topic, name and summary
needed for displaying help. Instead class properties are used to
access
the information directly in schema.
Patch 0112:
I think this would better be done in Schema.read_namespace_member,
because Schema is where all the state is.
(BTW does _SchemaNameSpace.__getitem__ raise KeyError for
non-existent
keys? It looks like it doesn't.)
Patch 0113:
How about setting _schema_cached to False in Schema.__init__()
rather
that getattr()-ing it in _ensure_cached()?
Patch 0116:
ClientCommand.doc should be a class property as well, otherwise
.summary
won't work on it correctly.
_SchemaCommand.doc should not be a property, as it's not needed for
.summary to work on it correctly.
Otherwise works fine for me.
Honza
Updated patches attached.
Thanks, ACK.
Pushed to master: 229e2a1ed9ea9877cb5e879fadd99f9040f77c96
I've made and reviewer overlooked some errors. Attached patches fix
them.
Shame on me.
Anyway,
1) When schema() raises SchemaUpToDate, the current code explodes:
ipa: ERROR: KeyError: 'fingerprint'
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/ipalib/cli.py", line 1348, in
run
api.finalize()
File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 707,
in finalize
self.__do_if_not_done('load_plugins')
File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 422,
in __do_if_not_done
getattr(self, name)()
File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 585,
in load_plugins
for package in self.packages:
File "/usr/lib/python2.7/site-packages/ipalib/__init__.py", line 919,
in packages
ipaclient.remote_plugins.get_package(self),
File
"/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/__init__.py",
line 92, in get_package
plugins = schema.get_package(api, server_info, client)
File
"/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/schema.py",
line 558, in get_package
fingerprint = str(schema['fingerprint'])
File
"/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/schema.py",
line 483, in __getitem__
return self._dict[key]
KeyError: 'fingerprint'
2) We read server info every time get_package() is called. It should be
cache similarly to how the schema is cached in the api object.
3) Some of the commands are still fully initialized during "ipa help
commands".
Honza
Updated patches attached.
Thanks, ACK.
Pushed to master: 6e6cbda036559e741ead0ab5ba18b0be0b41621e
When locale is not available setlocale() explodes. Handling this
exception in the same way as in ipalib/rpc.py to behave consistently.
--
David Kupka
From 083ad6016999eb244dc9915ec5fe6988f9053b0d Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 18 Aug 2016 10:59:09 +0200
Subject: [PATCH] schema cache: Fallback to 'en_us' when locale is not
ava
Re: [Freeipa-devel] [PATCH 0112-7] Speeding up cli help
On 08/08/16 13:26, Jan Cholasta wrote:
On 4.8.2016 16:32, David Kupka wrote:
On 03/08/16 16:33, Jan Cholasta wrote:
On 3.8.2016 16:23, David Kupka wrote:
On 21/07/16 10:12, Jan Cholasta wrote:
Hi,
On 20.7.2016 14:32, David Kupka wrote:
On 15/07/16 12:53, David Kupka wrote:
Hello!
After Honza introduced thin client that builds plugins and commands
dynamically from schema client became much slower. This is only
logical,
instead of importing a module client now must fetch the schema from
server, parse it and instantiate the commands using the data.
First step to speed it up was addition of schema cache to client.
That
removed the RTT and download time of fetching schema every time.
Now the most time consuming task became displaying help for lists of
topics and command and displaying individual topics. This is simply
because of the need to instantiate all the commands to find the
relations between topics and commands.
All the necessary bits for server commands and topics are already in
the
schema cache so we can skip this part and generate help from it,
right?
Not so fast!
There are client plugins with commands and topics. So we can
generate
basic bits (list of all topics, list of all commands, list of
commands
for each topic) from schema and store it in cache. Then we need
to go
through all client plugins and get similar bits for client plugins.
Then
we can merge and print.
Still the client response is not as fast as before and I this it
even
can't be. Also first time you display particular topic or list takes
longer because it must be freshly generated and stored in cache for
next
use. And this is what the attached patches do.
https://fedorahosted.org/freeipa/ticket/6048
Reimplemented so there is no need to distinguish client plugins and
remote plugins.
The main idea of this approach is to avoid creating instances of the
commands just to get the information about topic, name and summary
needed for displaying help. Instead class properties are used to
access
the information directly in schema.
Patch 0112:
I think this would better be done in Schema.read_namespace_member,
because Schema is where all the state is.
(BTW does _SchemaNameSpace.__getitem__ raise KeyError for non-existent
keys? It looks like it doesn't.)
Patch 0113:
How about setting _schema_cached to False in Schema.__init__() rather
that getattr()-ing it in _ensure_cached()?
Patch 0116:
ClientCommand.doc should be a class property as well, otherwise
.summary
won't work on it correctly.
_SchemaCommand.doc should not be a property, as it's not needed for
.summary to work on it correctly.
Otherwise works fine for me.
Honza
Updated patches attached.
Thanks, ACK.
Pushed to master: 229e2a1ed9ea9877cb5e879fadd99f9040f77c96
I've made and reviewer overlooked some errors. Attached patches fix them.
Shame on me.
Anyway,
1) When schema() raises SchemaUpToDate, the current code explodes:
ipa: ERROR: KeyError: 'fingerprint'
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/ipalib/cli.py", line 1348, in run
api.finalize()
File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 707,
in finalize
self.__do_if_not_done('load_plugins')
File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 422,
in __do_if_not_done
getattr(self, name)()
File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 585,
in load_plugins
for package in self.packages:
File "/usr/lib/python2.7/site-packages/ipalib/__init__.py", line 919,
in packages
ipaclient.remote_plugins.get_package(self),
File
"/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/__init__.py",
line 92, in get_package
plugins = schema.get_package(api, server_info, client)
File
"/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/schema.py",
line 558, in get_package
fingerprint = str(schema['fingerprint'])
File
"/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/schema.py",
line 483, in __getitem__
return self._dict[key]
KeyError: 'fingerprint'
2) We read server info every time get_package() is called. It should be
cache similarly to how the schema is cached in the api object.
3) Some of the commands are still fully initialized during "ipa help
commands".
Honza
Updated patches attached.
--
David Kupka
From 48c908746459f31b44a09045d53653ab503698ad Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 4 Aug 2016 16:02:24 +0200
Subject: [PATCH 01/10] schema cache: Do not reset ServerInfo dirty flag
Once dirty flag is set to True it must not be set back to False.
Otherwise changes are not written back to file.
https://fedorahosted.org/freeipa/ticket/6048
---
ipaclient/remote_plugins/__init__.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ipaclient/remote_plugins/__init_
Re: [Freeipa-devel] [PATCH 687] client: add missing output params to client-side commands
On 10/08/16 09:30, Jan Cholasta wrote: Hi, the attached patch fixes <https://fedorahosted.org/freeipa/ticket/6182>. Honza Works for me, ACK. Pushed to master: 20ee4a73e7b9e0ccdc7725ff4b87404d5543992e -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 685] parameters: move the `confirm` kwarg to Param
On 08/08/16 13:40, Jan Cholasta wrote:
On 8.8.2016 13:26, Martin Basti wrote:
On 08.08.2016 13:27, Jan Cholasta wrote:
Hi,
the attached patch fixes <https://fedorahosted.org/freeipa/ticket/6174>.
Honza
Please document this change in Param dosctring
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -418,6 +418,7 @@ class Param(ReadOnly):
('cli_metavar', str, None),
('no_convert', bool, False),
('deprecated', bool, False),
+('confirm', bool, True),
Martin^2
Works for me, ACK.
Pushed to master: e9c1d21b9fec17ab13894885eb1238631ecc43e5
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0112-7] Speeding up cli help
On 03/08/16 16:33, Jan Cholasta wrote:
On 3.8.2016 16:23, David Kupka wrote:
On 21/07/16 10:12, Jan Cholasta wrote:
Hi,
On 20.7.2016 14:32, David Kupka wrote:
On 15/07/16 12:53, David Kupka wrote:
Hello!
After Honza introduced thin client that builds plugins and commands
dynamically from schema client became much slower. This is only
logical,
instead of importing a module client now must fetch the schema from
server, parse it and instantiate the commands using the data.
First step to speed it up was addition of schema cache to client. That
removed the RTT and download time of fetching schema every time.
Now the most time consuming task became displaying help for lists of
topics and command and displaying individual topics. This is simply
because of the need to instantiate all the commands to find the
relations between topics and commands.
All the necessary bits for server commands and topics are already in
the
schema cache so we can skip this part and generate help from it,
right?
Not so fast!
There are client plugins with commands and topics. So we can generate
basic bits (list of all topics, list of all commands, list of commands
for each topic) from schema and store it in cache. Then we need to go
through all client plugins and get similar bits for client plugins.
Then
we can merge and print.
Still the client response is not as fast as before and I this it even
can't be. Also first time you display particular topic or list takes
longer because it must be freshly generated and stored in cache for
next
use. And this is what the attached patches do.
https://fedorahosted.org/freeipa/ticket/6048
Reimplemented so there is no need to distinguish client plugins and
remote plugins.
The main idea of this approach is to avoid creating instances of the
commands just to get the information about topic, name and summary
needed for displaying help. Instead class properties are used to access
the information directly in schema.
Patch 0112:
I think this would better be done in Schema.read_namespace_member,
because Schema is where all the state is.
(BTW does _SchemaNameSpace.__getitem__ raise KeyError for non-existent
keys? It looks like it doesn't.)
Patch 0113:
How about setting _schema_cached to False in Schema.__init__() rather
that getattr()-ing it in _ensure_cached()?
Patch 0116:
ClientCommand.doc should be a class property as well, otherwise .summary
won't work on it correctly.
_SchemaCommand.doc should not be a property, as it's not needed for
.summary to work on it correctly.
Otherwise works fine for me.
Honza
Updated patches attached.
Thanks, ACK.
Pushed to master: 229e2a1ed9ea9877cb5e879fadd99f9040f77c96
I've made and reviewer overlooked some errors. Attached patches fix them.
--
David Kupka
From ae1a9d024e37a153b6e9e4ada657f0e1e78300ef Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 4 Aug 2016 16:02:24 +0200
Subject: [PATCH 1/3] schema cache: Do not reset ServerInfo dirty flag
Once dirty flag is set to True it must not be set back to False.
Otherwise changes are not written back to file.
https://fedorahosted.org/freeipa/ticket/6048
---
ipaclient/remote_plugins/__init__.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ipaclient/remote_plugins/__init__.py b/ipaclient/remote_plugins/__init__.py
index 444651d30fd0cd96299fecb7ee7b5e4532b0abd4..976d6968724088d8e1fe8d3615990accf585ffeb 100644
--- a/ipaclient/remote_plugins/__init__.py
+++ b/ipaclient/remote_plugins/__init__.py
@@ -59,7 +59,8 @@ class ServerInfo(collections.MutableMapping):
return self._dict[key]
def __setitem__(self, key, value):
-self._dirty = key not in self._dict or self._dict[key] != value
+if key not in self._dict or self._dict[key] != value:
+self._dirty = True
self._dict[key] = value
def __delitem__(self, key):
--
2.7.4
From a6e9b17e0ad109f3237f4417828f7a3cfb5aa743 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 4 Aug 2016 16:07:08 +0200
Subject: [PATCH 2/3] schema cache: Write format information in cache
When format is not in cache '0' is assumed and client expecting higher
format refuses to load such cache.
https://fedorahosted.org/freeipa/ticket/6048
---
ipaclient/remote_plugins/schema.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index a215452ea0d2c1278a6121b3806b0daee02abd6e..13b1c365b111faae673c5ed78a7e5439a869c330 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -524,6 +524,7 @@ class Schema(object):
schema.writestr('_help',
json.dumps(self._generate_help(self._dict)))
+schema.writestr('format', json.dumps(FORMAT))
def _read(self, path):
with self._open_schema(self._fingerprint, 'r') as zf:
--
2.7.4
From c57a1b98ca518db8909d4d1176c99
Re: [Freeipa-devel] [PATCH 0112-7] Speeding up cli help
On 21/07/16 10:12, Jan Cholasta wrote:
Hi,
On 20.7.2016 14:32, David Kupka wrote:
On 15/07/16 12:53, David Kupka wrote:
Hello!
After Honza introduced thin client that builds plugins and commands
dynamically from schema client became much slower. This is only logical,
instead of importing a module client now must fetch the schema from
server, parse it and instantiate the commands using the data.
First step to speed it up was addition of schema cache to client. That
removed the RTT and download time of fetching schema every time.
Now the most time consuming task became displaying help for lists of
topics and command and displaying individual topics. This is simply
because of the need to instantiate all the commands to find the
relations between topics and commands.
All the necessary bits for server commands and topics are already in the
schema cache so we can skip this part and generate help from it, right?
Not so fast!
There are client plugins with commands and topics. So we can generate
basic bits (list of all topics, list of all commands, list of commands
for each topic) from schema and store it in cache. Then we need to go
through all client plugins and get similar bits for client plugins. Then
we can merge and print.
Still the client response is not as fast as before and I this it even
can't be. Also first time you display particular topic or list takes
longer because it must be freshly generated and stored in cache for next
use. And this is what the attached patches do.
https://fedorahosted.org/freeipa/ticket/6048
Reimplemented so there is no need to distinguish client plugins and
remote plugins.
The main idea of this approach is to avoid creating instances of the
commands just to get the information about topic, name and summary
needed for displaying help. Instead class properties are used to access
the information directly in schema.
Patch 0112:
I think this would better be done in Schema.read_namespace_member,
because Schema is where all the state is.
(BTW does _SchemaNameSpace.__getitem__ raise KeyError for non-existent
keys? It looks like it doesn't.)
Patch 0113:
How about setting _schema_cached to False in Schema.__init__() rather
that getattr()-ing it in _ensure_cached()?
Patch 0116:
ClientCommand.doc should be a class property as well, otherwise .summary
won't work on it correctly.
_SchemaCommand.doc should not be a property, as it's not needed for
.summary to work on it correctly.
Otherwise works fine for me.
Honza
Updated patches attached.
--
David Kupka
From bae93c2a57b51c8b5c11d481f9df2a62f330fb81 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Wed, 27 Jul 2016 10:46:40 +0200
Subject: [PATCH 1/6] schema: Speed up schema cache
Check presence of schema in cache (and download it if necessary) on
__init__ instead of with each __getitem__ call. Prefill internal
dictionary with empty record for each command to be able to quickly
determine if requested command exist in schema or not. Rest of schema
data are read from cache on first attempt to retrive them.
https://fedorahosted.org/freeipa/ticket/6048
https://fedorahosted.org/freeipa/ticket/6069
---
ipaclient/remote_plugins/schema.py | 297 ++---
1 file changed, 175 insertions(+), 122 deletions(-)
diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index cd1d5d607978899254325f634ccec91d2c92f59b..70cd7536d836ec113236bfdae8bbabb2d843725d 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -5,10 +5,8 @@
import collections
import errno
import fcntl
-import glob
import json
import os
-import re
import sys
import time
import types
@@ -65,8 +63,6 @@ USER_CACHE_PATH = (
'.cache'
)
)
-SCHEMA_DIR = os.path.join(USER_CACHE_PATH, 'ipa', 'schema')
-SERVERS_DIR = os.path.join(USER_CACHE_PATH, 'ipa', 'servers')
logger = log_mgr.get_logger(__name__)
@@ -274,15 +270,6 @@ class _SchemaObjectPlugin(_SchemaPlugin):
schema_key = 'classes'
-def _ensure_dir_created(d):
-try:
-os.makedirs(d)
-except OSError as e:
-if e.errno != errno.EEXIST:
-raise RuntimeError("Unable to create cache directory: {}"
- "".format(e))
-
-
class _LockedZipFile(zipfile.ZipFile):
""" Add locking to zipfile.ZipFile
Shared lock is used with read mode, exclusive with write mode.
@@ -308,7 +295,10 @@ class _SchemaNameSpace(collections.Mapping):
self._schema = schema
def __getitem__(self, key):
-return self._schema.read_namespace_member(self.name, key)
+try:
+return self._schema.read_namespace_member(self.name, key)
+except KeyError:
+raise KeyError(key)
def __iter__(self):
for key in self._schema.iter_namespace(self.name):
@@ -322,6 +312,62 @@ c
[Freeipa-devel] [PATCH 0114] vault: Catch correct exception in decrypt
Pushed under one-liner rule, attaching path for reference.
Pushed to master: 8ab0ad5b9ef59eca7b25a150baeb4a9bf8faa582
--
David Kupka
From bf61c2549ae98869ee9faaf808491b5a21af813d Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Wed, 3 Aug 2016 10:35:40 +0200
Subject: [PATCH] vault: Catch correct exception in decrypt
ValueError is raised when decryption fails.
https://fedorahosted.org/freeipa/ticket/6160
---
ipaclient/plugins/vault.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipaclient/plugins/vault.py b/ipaclient/plugins/vault.py
index e3a1ae3a0ad767bcee843b7fa3743a934e02d18b..73ad09b38316d55b466b7973dbeffefc1b7bb528 100644
--- a/ipaclient/plugins/vault.py
+++ b/ipaclient/plugins/vault.py
@@ -164,7 +164,7 @@ def decrypt(data, symmetric_key=None, private_key=None):
label=None
)
)
-except AssertionError:
+except ValueError:
raise errors.AuthenticationError(
message=_('Invalid credentials'))
--
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 0194] harden the check for trust namespace overlap in new principals
On 26/07/16 13:18, Martin Babinsky wrote: On 07/21/2016 12:56 PM, Martin Babinsky wrote: '*-add-principal' would crash with error if the trusted domains did not have any UPN suffixes or NETBIOS name associated with them. This patch fixes that. Big thanks to Milan who found and reported the issue during writing tests for the feature. https://fedorahosted.org/freeipa/ticket/6099 Bump for review. Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] PATCH: Improve on #2795 patches
On 20/07/16 12:11, Simo Sorce wrote: Attached patch introduces a helper function and avoids the questionable replace+delete operations where possible (still employed in the entry_to_mods function). Compiles and I am about to test it, but I'd like feedback on it if anyone wants to take a look. Simo. Looks and works good, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0112-7] Speeding up cli help
On 15/07/16 12:53, David Kupka wrote:
Hello!
After Honza introduced thin client that builds plugins and commands
dynamically from schema client became much slower. This is only logical,
instead of importing a module client now must fetch the schema from
server, parse it and instantiate the commands using the data.
First step to speed it up was addition of schema cache to client. That
removed the RTT and download time of fetching schema every time.
Now the most time consuming task became displaying help for lists of
topics and command and displaying individual topics. This is simply
because of the need to instantiate all the commands to find the
relations between topics and commands.
All the necessary bits for server commands and topics are already in the
schema cache so we can skip this part and generate help from it, right?
Not so fast!
There are client plugins with commands and topics. So we can generate
basic bits (list of all topics, list of all commands, list of commands
for each topic) from schema and store it in cache. Then we need to go
through all client plugins and get similar bits for client plugins. Then
we can merge and print.
Still the client response is not as fast as before and I this it even
can't be. Also first time you display particular topic or list takes
longer because it must be freshly generated and stored in cache for next
use. And this is what the attached patches do.
https://fedorahosted.org/freeipa/ticket/6048
Reimplemented so there is no need to distinguish client plugins and
remote plugins.
The main idea of this approach is to avoid creating instances of the
commands just to get the information about topic, name and summary
needed for displaying help. Instead class properties are used to access
the information directly in schema.
--
David Kupka
From 8eefe955f7e961e1a4e55b823772383243c029a5 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Tue, 19 Jul 2016 09:34:35 +0200
Subject: [PATCH 1/6] schema: Read data from cache only once
Keep data when read from storage to avoid unnecessary reads.
https://fedorahosted.org/freeipa/ticket/6048
---
ipaclient/remote_plugins/schema.py | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index cd1d5d607978899254325f634ccec91d2c92f59b..09f041592f04e65544cf65647f28f395a2f9e4a7 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -306,9 +306,17 @@ class _SchemaNameSpace(collections.Mapping):
def __init__(self, schema, name):
self.name = name
self._schema = schema
+self._dict = {}
def __getitem__(self, key):
-return self._schema.read_namespace_member(self.name, key)
+try:
+return self._dict[key]
+except KeyError:
+self._dict[key] = self._schema.read_namespace_member(
+self.name,
+key
+)
+return self._dict[key]
def __iter__(self):
for key in self._schema.iter_namespace(self.name):
--
2.7.4
From fe155db233456b2d6d917a010177eb382a3de58f Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Wed, 20 Jul 2016 09:54:45 +0200
Subject: [PATCH 2/6] schema: Speed up cache verification
Check schema only once for each api instance.
https://fedorahosted.org/freeipa/ticket/6048
---
ipaclient/remote_plugins/schema.py | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index 09f041592f04e65544cf65647f28f395a2f9e4a7..92430950f96be266ebaa2f238de226e1c02b8e6c 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -403,6 +403,9 @@ class Schema(object):
return (fp, exp)
def _ensure_cached(self):
+if getattr(self, '_schema_cached', False):
+return
+
no_info = False
try:
# pylint: disable=access-member-before-definition
@@ -422,13 +425,9 @@ class Schema(object):
logger.warning('Failed to load server properties: {}'
''.format(e))
-force_check = ((not getattr(self, '_schema_checked', False)) and
- self._api.env.force_schema_check)
-
-if (force_check or
+if (self._api.env.force_schema_check or
no_info or exp < time.time() or not Schema._in_cache(fp)):
(fp, exp) = self._get_schema()
-self._schema_checked = True
_ensure_dir_created(SERVERS_DIR)
try:
with self._open_server_info(self._api.env.server, 'w') as sc:
@@ -436,6 +435,7 @@ class Schema(object):
except Exception as e:
logger.warning('Failed to store server properties: {}'
'
[Freeipa-devel] [PATCH 0112-3] Speeding up cli help
Hello!
After Honza introduced thin client that builds plugins and commands
dynamically from schema client became much slower. This is only logical,
instead of importing a module client now must fetch the schema from
server, parse it and instantiate the commands using the data.
First step to speed it up was addition of schema cache to client. That
removed the RTT and download time of fetching schema every time.
Now the most time consuming task became displaying help for lists of
topics and command and displaying individual topics. This is simply
because of the need to instantiate all the commands to find the
relations between topics and commands.
All the necessary bits for server commands and topics are already in the
schema cache so we can skip this part and generate help from it, right?
Not so fast!
There are client plugins with commands and topics. So we can generate
basic bits (list of all topics, list of all commands, list of commands
for each topic) from schema and store it in cache. Then we need to go
through all client plugins and get similar bits for client plugins. Then
we can merge and print.
Still the client response is not as fast as before and I this it even
can't be. Also first time you display particular topic or list takes
longer because it must be freshly generated and stored in cache for next
use. And this is what the attached patches do.
https://fedorahosted.org/freeipa/ticket/6048
--
David Kupka
From e04b588df13286785aef53c59c41ea9c8935384f Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 14 Jul 2016 10:41:37 +0200
Subject: [PATCH 1/2] schema: Generate help for server plugins from schema and
store it in cache
https://fedorahosted.org/freeipa/ticket/6048
---
ipaclient/remote_plugins/schema.py | 159 -
1 file changed, 158 insertions(+), 1 deletion(-)
diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index cd1d5d607978899254325f634ccec91d2c92f59b..5c05a84e63fb9d04660d8113020bc3b11e4141a8 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -25,6 +25,7 @@ from ipapython.dn import DN
from ipapython.dnsutil import DNSName
from ipapython.ipa_log_manager import log_mgr
+
if six.PY3:
unicode = str
@@ -318,10 +319,136 @@ class _SchemaNameSpace(collections.Mapping):
return len(list(self._schema.iter_namespace(self.name)))
+class _MutableNameSpace(_SchemaNameSpace, collections.MutableMapping):
+
+def __setitem__(self, key, value):
+self._schema.add_namespace_member(self.name, key, value)
+
+def __delitem__(self, key):
+raise NotImplementedError("Droping individual pieces of cached data"
+ " makes no sense. At least for now.")
+
+
class NotAvailable(Exception):
pass
+class Help(object):
+def __init__(self, schema):
+self.schema = schema
+
+@staticmethod
+def _doc_to_summary(d):
+if d:
+return unicode(d).lstrip().split('\n', 1)[0]
+else:
+return u''
+
+def _command_is_visible(self, cmd_full_name):
+cmd = self.schema['commands'][cmd_full_name]
+if 'cli' in cmd.get('exclude', []):
+return False
+return True
+
+def _topic_is_visible(self, topic_full_name):
+topic_index = self.schema['topics_index'][topic_full_name]
+# super topics are always visible
+if topic_index['subtopics']:
+return True
+
+# if there is at least one cli visible command
+# topic is also visible
+topic_cmds = topic_index['commands']
+for cmd_full_name in topic_cmds:
+if self._command_is_visible(cmd_full_name):
+return True
+return False
+
+def _list(self, ns_name):
+ret = []
+
+try:
+help_ = self.schema['help'][ns_name]
+ret = help_['text']
+mcl = help_['mcl']
+except KeyError:
+for full_name in sorted(self.schema[ns_name]):
+if ((
+ns_name == 'commands' and
+not self._command_is_visible(full_name)
+ ) or (
+ns_name == 'topics' and
+not self._topic_is_visible(full_name)
+ )):
+continue
+
+obj = self.schema[ns_name][full_name]
+name = obj['name']
+summary = self._doc_to_summary(obj['doc'])
+ret.append((name, summary,))
+
+mcl = max([len(n[0]) for n in ret])
+
+self.schema['help'][ns_name] = {'text': ret, 'mcl': mcl}
+
+return (ret, mcl,)
+
+def commands(self):
Re: [Freeipa-devel] [PATCH 0149] help: Add dnsserver commands to help topic 'dns'
On 12/07/16 12:54, Petr Spacek wrote:
Hello,
help: Add dnsserver commands to help topic 'dns'
https://bugzilla.redhat.com/show_bug.cgi?id=1353888
Hi!
Your patch turns dnsserver topic to a subtopic of dns topic. I'm sorry I
gave you wrong advice. Attached patch makes dnsserver-* commands appear
in dns topic.
--
David Kupka
From 965e4b84a8b52e1760fc69745825362fc4ecf020 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Fri, 15 Jul 2016 11:55:19 +0200
Subject: [PATCH] help: Add dnsserver commands to help topic 'dns'
https://bugzilla.redhat.com/show_bug.cgi?id=1353888
---
ipaserver/plugins/dnsserver.py | 7 +++
1 file changed, 7 insertions(+)
diff --git a/ipaserver/plugins/dnsserver.py b/ipaserver/plugins/dnsserver.py
index beddec04230d810479fff9612721cf12260bbb3a..d635722a6b6aaea942d49456a04f5d0480d344c9 100644
--- a/ipaserver/plugins/dnsserver.py
+++ b/ipaserver/plugins/dnsserver.py
@@ -48,6 +48,8 @@ EXAMPLES:
register = Registry()
+topic = None
+
dnsserver_object_class = ['top', 'idnsServerConfigObject']
@register()
@@ -149,6 +151,7 @@ class dnsserver(LDAPObject):
@register()
class dnsserver_mod(LDAPUpdate):
__doc__ = _('Modify DNS server configuration')
+topic = 'dns'
msg_summary = _('Modified DNS server "%(value)s"')
@@ -156,6 +159,7 @@ class dnsserver_mod(LDAPUpdate):
@register()
class dnsserver_find(LDAPSearch):
__doc__ = _('Search for DNS servers.')
+topic = 'dns'
msg_summary = ngettext(
'%(count)d DNS server matched',
@@ -166,6 +170,7 @@ class dnsserver_find(LDAPSearch):
@register()
class dnsserver_show(LDAPRetrieve):
__doc__=_('Display configuration of a DNS server.')
+topic = 'dns'
@register()
@@ -175,6 +180,7 @@ class dnsserver_add(LDAPCreate, Local):
Be careful in future this will be transformed to public API call
"""
__doc__ = _('Add a new DNS server.')
+topic = 'dns'
msg_summary = _('Added new DNS server "%(value)s"')
@@ -186,5 +192,6 @@ class dnsserver_del(LDAPDelete, Local):
Be careful in future this will be transformed to public API call
"""
__doc__ = _('Delete a DNS server')
+topic = 'dns'
msg_summary = _('Deleted DNS server "%(value)s"')
--
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] [PATCH 0110] schema: Fix subtopic -> topic mapping
https://fedorahosted.org/freeipa/ticket/6069
--
David Kupka
From 3c4a505b041738312ae97e0dd26362523f985a7d Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 14 Jul 2016 10:15:59 +0200
Subject: [PATCH] schema: Fix subtopic -> topic mapping
https://fedorahosted.org/freeipa/ticket/6069
---
ipaserver/plugins/schema.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ipaserver/plugins/schema.py b/ipaserver/plugins/schema.py
index a82b357899a483fd3b3dc9f7407bd26a4c03aada..8fd7c6ba1c4ed8cd6e27cb8b1b04f48694a4f1ff 100644
--- a/ipaserver/plugins/schema.py
+++ b/ipaserver/plugins/schema.py
@@ -399,7 +399,8 @@ class topic_(MetaObject):
continue
if topic_value is not None:
topic_name = unicode(topic_value)
-topic['topic_topic'] = topic_full_name
+topic['topic_topic'] = '{}/{}'.format(topic_name,
+ topic_version)
else:
topic.pop('topic_topic', None)
--
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] pwpolicy: Do not expire passwords when maxlife is set to 0 (infinity).
On 01/07/16 11:22, thierry bordaz wrote:
On 07/01/2016 10:46 AM, David Kupka wrote:
Hello Thierry!
Thanks for looking into it. I will try to answer your questions and
comments inline.
On 01/07/16 10:26, thierry bordaz wrote:
Hi David,
The patch looks good but being not familiar with that code, my comments
may be absolutely wrong
In ipadb_get_pwd_expiration, if it is not 'self' we set
'*export=mod_time'.
If for some reason 'mod_time==0', it has now a specific meaning 'not
expiring' . Does it match the comment '* not 'self', so reset */'
mod_time should be timestamp of the modification operation. So
mod_time == 0 should happen only when the change was performed in the
very beginning of 70's.
Right. My fault I did not understand that code.
In ipadb_entry_to_mods, it deletes krbPasswordExpiration. But just
before it adds in the mods krbPasswordExpiration=0 or
krbPasswordExpiration=entry->pw_expiration
Could we skip those mods if entry->pw_expiration==0 or expire_time==0 ?
That is exactly what I thought. But in that part of code I have no
chance to check if the attribute is present in the entry or not. Also
I can't catch and ignore the resulting error when deleting nonexistent
attribute. Here only LDAPMod structures are filled but the execution
is done in an other part of code.
So I choose the easy path and always set the attribute and delete it
right after if necessary.
I think there is something a bit strange here.
To be able to delete the attribute we first need to set it to a specific
value then deleting the value we manage to delete the attribute.
I did not find a routine like ipa_get_ldap_mod_delattr. With such
routine I wonder if we could to something like:
if (entry->pw_expiration == 0) {
kerr = ipadb_get_ldap_mod_delattr(imods,
"krbPasswordExpiration");
} else {
kerr = ipadb_get_ldap_mod_time(imods,
"krbPasswordExpiration",
entry->pw_expiration,
mod_op);
}
Instead of
kerr = ipadb_get_ldap_mod_time(imods,
"krbPasswordExpiration",
entry->pw_expiration,
mod_op);
if (entry->pw_expiration == 0) {
kerr = ipadb_get_ldap_mod_time(imods,
"krbPasswordExpiration",
entry->pw_expiration, LDAP_MOD_DELETE);
}
At some point I have added such routine and tried same approach as you
do. But when the krbPasswordExpiration is not present in the entry (user
already has non-expiring password) there is an error later when the mods
are applied (err=16 ~ no such attribute).
When I found this I decided to always LDAP_MOD_REPLACE the attribute
(replace operation does not fail when the attribute is missing). And
then remove it when it shouldn't be there. After that I decided to
remove my _del_attr routine because I didn't want to add new unnecessary
code.
In ipapwd_SetPassword, ipapwd_post_modadd, same remark as above.
Something that I am not sure is what is the expected relation between
passwordexpirationtime and krbPasswordExpiration
I'm not sure either. I think we don't use passwordexpirationtime
attribute.
I think they should be somehow linked, in fact it is looking it is what
happen in ipapwd_write_krb_keys.
But it looks it happens only when the krb keys are created.
Probablby, I don't recall seeing passwordexpirationtime attribute used
anywhere.
thanks
thierry
On 06/30/2016 09:34 PM, David Kupka wrote:
On 04/05/16 17:22, Pavel Vomacka wrote:
On 05/04/2016 04:36 PM, Simo Sorce wrote:
On Wed, 2016-05-04 at 15:39 +0200, Martin Kosek wrote:
On 05/02/2016 02:28 PM, David Kupka wrote:
https://fedorahosted.org/freeipa/ticket/2795
That patch looks suspiciously short given the struggles I saw in
http://www.redhat.com/archives/freeipa-devel/2015-June/msg00198.html
:-)
Instead of setting to IPAPWD_END_OF_TIME, should we instead avoid
filling
"krbPasswordExpiration" attribute at all, i.e. have password
*without*
expiration? Or is krbPasswordExpiration mandatory?
So I looked at the MIT code, and it seem like they are coping just
fine
with a missing (ie value = 0 internally) pw_expiration attribute.
So if we make our code cope with omitting any expiration if the
attribute is missing then yes, we can mark no expiration with simply
removing (or not setting) the krbPasswordExpiration attribute.
The attribute itself is optional and can be omitted.
I think this is a good idea, and is definitely better than inventing
a a
magic value.
Simo.
Just a note: I tested David's patch and it actually doesn't work when
the new password policy for ipausers group is created (priority = 0,
which should be the highest priority). The maxlife and minlife values
are empty. Ev
Re: [Freeipa-devel] [PATCH] pwpolicy: Do not expire passwords when maxlife is set to 0 (infinity).
Hello Thierry! Thanks for looking into it. I will try to answer your questions and comments inline. On 01/07/16 10:26, thierry bordaz wrote: Hi David, The patch looks good but being not familiar with that code, my comments may be absolutely wrong In ipadb_get_pwd_expiration, if it is not 'self' we set '*export=mod_time'. If for some reason 'mod_time==0', it has now a specific meaning 'not expiring' . Does it match the comment '* not 'self', so reset */' mod_time should be timestamp of the modification operation. So mod_time == 0 should happen only when the change was performed in the very beginning of 70's. In ipadb_entry_to_mods, it deletes krbPasswordExpiration. But just before it adds in the mods krbPasswordExpiration=0 or krbPasswordExpiration=entry->pw_expiration Could we skip those mods if entry->pw_expiration==0 or expire_time==0 ? That is exactly what I thought. But in that part of code I have no chance to check if the attribute is present in the entry or not. Also I can't catch and ignore the resulting error when deleting nonexistent attribute. Here only LDAPMod structures are filled but the execution is done in an other part of code. So I choose the easy path and always set the attribute and delete it right after if necessary. In ipapwd_SetPassword, ipapwd_post_modadd, same remark as above. Something that I am not sure is what is the expected relation between passwordexpirationtime and krbPasswordExpiration I'm not sure either. I think we don't use passwordexpirationtime attribute. thanks thierry On 06/30/2016 09:34 PM, David Kupka wrote: On 04/05/16 17:22, Pavel Vomacka wrote: On 05/04/2016 04:36 PM, Simo Sorce wrote: On Wed, 2016-05-04 at 15:39 +0200, Martin Kosek wrote: On 05/02/2016 02:28 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/2795 That patch looks suspiciously short given the struggles I saw in http://www.redhat.com/archives/freeipa-devel/2015-June/msg00198.html :-) Instead of setting to IPAPWD_END_OF_TIME, should we instead avoid filling "krbPasswordExpiration" attribute at all, i.e. have password *without* expiration? Or is krbPasswordExpiration mandatory? So I looked at the MIT code, and it seem like they are coping just fine with a missing (ie value = 0 internally) pw_expiration attribute. So if we make our code cope with omitting any expiration if the attribute is missing then yes, we can mark no expiration with simply removing (or not setting) the krbPasswordExpiration attribute. The attribute itself is optional and can be omitted. I think this is a good idea, and is definitely better than inventing a a magic value. Simo. Just a note: I tested David's patch and it actually doesn't work when the new password policy for ipausers group is created (priority = 0, which should be the highest priority). The maxlife and minlife values are empty. Even if I set the new password policy maxlife and minlife to 0 the result was that password will expire in 90 days. The patch worked correctly when I changed value of maxlife and minlife to 0 in 'global_policy'. Then the password expiration was set to 2038-01-01. Hello! I hope I've finally find all the places in ipa-kdb and ipa-pwd-extop plugins to tickle in order to have password that don't expire. Updated patch attached. https://fedorahosted.org/freeipa/ticket/2795 -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0109] schema: Perform the check for schema update when, force_schema_check is True
On 01/07/16 07:59, David Kupka wrote:
https://fedorahosted.org/freeipa/ticket/4739
Offline NACK from Honza, attaching updated patch.
--
David Kupka
From 3d991e41e9e215c154994948e7d5360f82ea2e29 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Fri, 1 Jul 2016 07:50:08 +0200
Subject: [PATCH] schema: Perform the check for schema update when
force_schema_check is True
https://fedorahosted.org/freeipa/ticket/4739
---
ipaclient/remote_plugins/schema.py | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index a54d4eb777d3fa3d0a1dd6223eca2efeb8db4fbd..89aaeac516b9ee5afa0dd9b6d853d4ed1cf3b801 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -496,8 +496,13 @@ class Schema(object):
logger.warning('Failed to load server properties: {}'
''.format(e))
-if no_info or exp < time.time() or not Schema._in_cache(fp):
+force_check = ((not getattr(self, '_schema_checked', False)) and
+ self._api.env.force_schema_check)
+
+if (force_check or
+no_info or exp < time.time() or not Schema._in_cache(fp)):
(fp, exp) = self._get_schema()
+self._schema_checked = True
_ensure_dir_created(SERVERS_DIR)
try:
with self._open_server_info(self._api.env.server, 'w') as sc:
--
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] pwpolicy: Do not expire passwords when maxlife is set to 0 (infinity).
On 30/06/16 21:34, David Kupka wrote:
On 04/05/16 17:22, Pavel Vomacka wrote:
On 05/04/2016 04:36 PM, Simo Sorce wrote:
On Wed, 2016-05-04 at 15:39 +0200, Martin Kosek wrote:
On 05/02/2016 02:28 PM, David Kupka wrote:
https://fedorahosted.org/freeipa/ticket/2795
That patch looks suspiciously short given the struggles I saw in
http://www.redhat.com/archives/freeipa-devel/2015-June/msg00198.html
:-)
Instead of setting to IPAPWD_END_OF_TIME, should we instead avoid
filling
"krbPasswordExpiration" attribute at all, i.e. have password *without*
expiration? Or is krbPasswordExpiration mandatory?
So I looked at the MIT code, and it seem like they are coping just fine
with a missing (ie value = 0 internally) pw_expiration attribute.
So if we make our code cope with omitting any expiration if the
attribute is missing then yes, we can mark no expiration with simply
removing (or not setting) the krbPasswordExpiration attribute.
The attribute itself is optional and can be omitted.
I think this is a good idea, and is definitely better than inventing a a
magic value.
Simo.
Just a note: I tested David's patch and it actually doesn't work when
the new password policy for ipausers group is created (priority = 0,
which should be the highest priority). The maxlife and minlife values
are empty. Even if I set the new password policy maxlife and minlife to
0 the result was that password will expire in 90 days. The patch worked
correctly when I changed value of maxlife and minlife to 0 in
'global_policy'. Then the password expiration was set to 2038-01-01.
Hello!
I hope I've finally find all the places in ipa-kdb and ipa-pwd-extop
plugins to tickle in order to have password that don't expire. Updated
patch attached.
https://fedorahosted.org/freeipa/ticket/2795
Updated patch attached.
--
David Kupka
From af5e8516cf743544f529c2cd234af91e5251380e Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 30 Jun 2016 08:52:33 +0200
Subject: [PATCH] Allow unexpiring passwords
Treat maxlife=0 in password policy as "never expire". Delete
krbPasswordExpiration in user entry when password should never expire.
https://fedorahosted.org/freeipa/ticket/2795
---
daemons/ipa-kdb/ipa_kdb_passwords.c | 6 +-
daemons/ipa-kdb/ipa_kdb_principals.c | 11 +++
daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c | 22 --
daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 4
ipaserver/plugins/pwpolicy.py | 2 +-
5 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_passwords.c b/daemons/ipa-kdb/ipa_kdb_passwords.c
index ad57181d5049f36c69044bb2c9cfe183d7e4ea25..a3d4fe2436da60d081040754780d3e815acb1473 100644
--- a/daemons/ipa-kdb/ipa_kdb_passwords.c
+++ b/daemons/ipa-kdb/ipa_kdb_passwords.c
@@ -253,7 +253,11 @@ krb5_error_code ipadb_get_pwd_expiration(krb5_context context,
if (truexp) {
if (ied->pol) {
-*expire_time = mod_time + ied->pol->max_pwd_life;
+if (ied->pol->max_pwd_life) {
+*expire_time = mod_time + ied->pol->max_pwd_life;
+} else {
+*expire_time = 0;
+}
} else {
*expire_time = mod_time + IPAPWD_DEFAULT_PWDLIFE;
}
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index f1d3e9e89c2016b8a9ebad9c0c6fd46487a33a4b..6cdfa909452a4b55912b2a5a74648abd2053482a 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -1850,6 +1850,11 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
"krbPasswordExpiration",
entry->pw_expiration,
mod_op);
+if (entry->pw_expiration == 0) {
+kerr = ipadb_get_ldap_mod_time(imods,
+ "krbPasswordExpiration",
+ entry->pw_expiration, LDAP_MOD_DELETE);
+}
if (kerr) {
goto done;
}
@@ -2105,6 +2110,12 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
kerr = ipadb_get_ldap_mod_time(imods,
"krbPasswordExpiration",
expire_time, mod_op);
+if (expire_time == 0) {
+kerr = ipadb_get_ldap_mod_time(imods,
+ "krbPasswordExpiration",
+ expire_time, LDAP_MOD_DELETE);
+}
+
if (kerr) {
goto done;
}
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
index 5
Re: [Freeipa-devel] [WIP] Thin client
On 28/04/16 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hello! Patches: client: add support for pre-schema servers client: do not crash when overriding remote command as method brings compatibility with old servers into thin client (making it not so thin after all :-). I've tested it against 4.2 and 4.3 servers and the important parts work. There are some minor issues (e.g. dynamic defaults not working) but they can be addressed later, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH 0109] schema: Perform the check for schema update when, force_schema_check is True
https://fedorahosted.org/freeipa/ticket/4739
--
David Kupka
From 58685f92e8d4c1817f95a7b4042ce0fa4c95a704 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Fri, 1 Jul 2016 07:50:08 +0200
Subject: [PATCH] schema: Perform the check for schema update when
force_schema_check is True
https://fedorahosted.org/freeipa/ticket/4739
---
ipaclient/remote_plugins/schema.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index a54d4eb777d3fa3d0a1dd6223eca2efeb8db4fbd..78ca84484595ce28da12bc81561003894d7c8db3 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -496,7 +496,8 @@ class Schema(object):
logger.warning('Failed to load server properties: {}'
''.format(e))
-if no_info or exp < time.time() or not Schema._in_cache(fp):
+if (self._api.env.force_schema_check or
+no_info or exp < time.time() or not Schema._in_cache(fp)):
(fp, exp) = self._get_schema()
_ensure_dir_created(SERVERS_DIR)
try:
--
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] [PATCH 0108] schema: Decrease schema TTL to one hour
https://fedorahosted.org/freeipa/ticket/4739
--
David Kupka
From 796fd4291dd17128e7bdfecf2d14ae7b151987f5 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Fri, 1 Jul 2016 07:34:43 +0200
Subject: [PATCH] schema: Decrease schema TTL to one hour
Since checking schema is relatively cheap operation (one round-trip with
almost no data) we can do it offten to ensure schema will fetched by
client ASAP after it was updated on server.
https://fedorahosted.org/freeipa/ticket/4739
---
ipaserver/plugins/schema.py | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ipaserver/plugins/schema.py b/ipaserver/plugins/schema.py
index c7aa5f36c37d39a5131ca8ad915cb6e61bb748ec..a82b357899a483fd3b3dc9f7407bd26a4c03aada 100644
--- a/ipaserver/plugins/schema.py
+++ b/ipaserver/plugins/schema.py
@@ -21,7 +21,10 @@ from ipapython.version import API_VERSION
# Schema TTL sent to clients in response to schema call.
# Number of seconds before client should check for schema update.
-SCHEMA_TTL = 7*24*3600 # default: 7 days
+# This should be long enough to not slow down regular work or skripts
+# but also short enough to ensure schema will be retvieved soon after
+# it was updated
+SCHEMA_TTL = 3600 # default: 1 hour
__doc__ = _("""
API Schema
--
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] [WIP] Kerberos principal aliases pt. 2
On 28/06/16 20:08, Martin Babinsky wrote: On 06/24/2016 09:52 AM, Martin Babinsky wrote: Hi list, I am furiously working on tickets related to the proper support and API for managing kerberos principal aliases for hosts, users, and services[1-5]. To better track and comment on my progress, I have forked freeipa on git and created a branch for you to test and review. The link is here: https://github.com/martbab/freeipa/tree/krb5-principal-aliases Please be aware that I may force-push into the branch without warning when fixing issues we will discover during testing/review. [1] http://www.freeipa.org/page/V4/Kerberos_principal_aliases [2] https://fedorahosted.org/freeipa/ticket/3864 [3] https://fedorahosted.org/freeipa/ticket/3961 [4] https://fedorahosted.org/freeipa/ticket/1365 [5] https://fedorahosted.org/freeipa/ticket/5413 Based on Jan's suggestions I have reworked the code substantially and force-pushed it into the github branch. Please review. Hello! I have gone through the code and tested the functionality in basic use cases (server-install, upgrade, replica-install, adding/removing principals, getting ticket with alias, ...). Code looks good to me and everything* seems to work smoothly. condACK, if Pavel or Petr^1 (or anyone else who tried this) don't report any issue really soon. *except for https://fedorahosted.org/freeipa/ticket/6017 -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] pwpolicy: Do not expire passwords when maxlife is set to 0 (infinity).
On 04/05/16 17:22, Pavel Vomacka wrote:
On 05/04/2016 04:36 PM, Simo Sorce wrote:
On Wed, 2016-05-04 at 15:39 +0200, Martin Kosek wrote:
On 05/02/2016 02:28 PM, David Kupka wrote:
https://fedorahosted.org/freeipa/ticket/2795
That patch looks suspiciously short given the struggles I saw in
http://www.redhat.com/archives/freeipa-devel/2015-June/msg00198.html
:-)
Instead of setting to IPAPWD_END_OF_TIME, should we instead avoid
filling
"krbPasswordExpiration" attribute at all, i.e. have password *without*
expiration? Or is krbPasswordExpiration mandatory?
So I looked at the MIT code, and it seem like they are coping just fine
with a missing (ie value = 0 internally) pw_expiration attribute.
So if we make our code cope with omitting any expiration if the
attribute is missing then yes, we can mark no expiration with simply
removing (or not setting) the krbPasswordExpiration attribute.
The attribute itself is optional and can be omitted.
I think this is a good idea, and is definitely better than inventing a a
magic value.
Simo.
Just a note: I tested David's patch and it actually doesn't work when
the new password policy for ipausers group is created (priority = 0,
which should be the highest priority). The maxlife and minlife values
are empty. Even if I set the new password policy maxlife and minlife to
0 the result was that password will expire in 90 days. The patch worked
correctly when I changed value of maxlife and minlife to 0 in
'global_policy'. Then the password expiration was set to 2038-01-01.
Hello!
I hope I've finally find all the places in ipa-kdb and ipa-pwd-extop
plugins to tickle in order to have password that don't expire. Updated
patch attached.
https://fedorahosted.org/freeipa/ticket/2795
--
David Kupka
From dc2250869ca4f527b9b353913ed5a750a6f0f6e5 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 30 Jun 2016 08:52:33 +0200
Subject: [PATCH] Allow unexpiring passwords
Treat maxlife=0 in password policy as "never expire". Delete
krbPasswordExpiration in user entry when password should never expire.
https://fedorahosted.org/freeipa/ticket/2795
---
daemons/ipa-kdb/ipa_kdb_passwords.c | 6 +-
daemons/ipa-kdb/ipa_kdb_principals.c | 11 +++
daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c | 11 ++-
daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 4
ipaserver/plugins/pwpolicy.py | 2 +-
5 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_passwords.c b/daemons/ipa-kdb/ipa_kdb_passwords.c
index ad57181d5049f36c69044bb2c9cfe183d7e4ea25..a3d4fe2436da60d081040754780d3e815acb1473 100644
--- a/daemons/ipa-kdb/ipa_kdb_passwords.c
+++ b/daemons/ipa-kdb/ipa_kdb_passwords.c
@@ -253,7 +253,11 @@ krb5_error_code ipadb_get_pwd_expiration(krb5_context context,
if (truexp) {
if (ied->pol) {
-*expire_time = mod_time + ied->pol->max_pwd_life;
+if (ied->pol->max_pwd_life) {
+*expire_time = mod_time + ied->pol->max_pwd_life;
+} else {
+*expire_time = 0;
+}
} else {
*expire_time = mod_time + IPAPWD_DEFAULT_PWDLIFE;
}
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index f1d3e9e89c2016b8a9ebad9c0c6fd46487a33a4b..6cdfa909452a4b55912b2a5a74648abd2053482a 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -1850,6 +1850,11 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
"krbPasswordExpiration",
entry->pw_expiration,
mod_op);
+if (entry->pw_expiration == 0) {
+kerr = ipadb_get_ldap_mod_time(imods,
+ "krbPasswordExpiration",
+ entry->pw_expiration, LDAP_MOD_DELETE);
+}
if (kerr) {
goto done;
}
@@ -2105,6 +2110,12 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
kerr = ipadb_get_ldap_mod_time(imods,
"krbPasswordExpiration",
expire_time, mod_op);
+if (expire_time == 0) {
+kerr = ipadb_get_ldap_mod_time(imods,
+ "krbPasswordExpiration",
+ expire_time, LDAP_MOD_DELETE);
+}
+
if (kerr) {
goto done;
}
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
index 5dc606d22305cf63a16feca30aab2728bb20b80d..de295e0b4cbd8eeff556d96be3db
Re: [Freeipa-devel] [WIP] Thin client
On 28/04/16 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hello! Patch set: server: exclude Local commands from RPC client: add placeholders for required remote plugins client: ignore override errors in command overrides plugable: add option to ignore override errors cert: fix CLI output of cert_remove_hold frontend: do not ignore client-side output params user: add object plugin for user_status server: define missing virtual attributes contains mostly fixes for some bugs discovered in thin client. Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [WIP] Thin client
On 30/06/16 10:45, Jan Cholasta wrote: On 30.6.2016 10:32, Jan Cholasta wrote: On 29.6.2016 10:21, Jan Cholasta wrote: On 29.6.2016 09:22, David Kupka wrote: On 28/04/16 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hello! Commit "schema: fix Flag arguments on the client" fixes regression reported in https://fedorahosted.org/freeipa/ticket/6009, ACK. Thanks, pushed to master: a77e21cbca05be422fe5826857cfba7e0ba6e71f Attaching the patch for reference. Martin found a regression caused by this patch. The attached patch should fix it. Self-NACK, correct patch attached. Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [WIP] Thin client
On 28/04/16 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hello! Commit "schema: fix Flag arguments on the client" fixes regression reported in https://fedorahosted.org/freeipa/ticket/6009, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH 0107] test: cert: Reflect change in behavior in tests
-- David Kupka From 4331e9a62a3cea81b548c555001a7d7ed1127574 Mon Sep 17 00:00:00 2001 From: David Kupka Date: Tue, 28 Jun 2016 10:47:10 +0200 Subject: [PATCH] test: cert: Reflect change in behavior in tests Command cert-find with parameter sizelimit set to 0 no longer returns 0 certificates but returns all. More precise ConversionError is returned when parameter is not convertible to its type. https://fedorahosted.org/freeipa/ticket/5381 https://fedorahosted.org/freeipa/ticket/4739 --- ipatests/test_xmlrpc/test_cert_plugin.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ipatests/test_xmlrpc/test_cert_plugin.py b/ipatests/test_xmlrpc/test_cert_plugin.py index 1a6168da29393eb041273da3bbfa845858cdaf72..8127ef224b24a0b3a63c3d07ef72d4b53feda4be 100644 --- a/ipatests/test_xmlrpc/test_cert_plugin.py +++ b/ipatests/test_xmlrpc/test_cert_plugin.py @@ -429,8 +429,9 @@ class test_cert_find(XMLRPC_test): """ Search with a sizelimit of 0 """ +count_all = api.Command['cert_find']()['count'] res = api.Command['cert_find'](sizelimit=0) -assert 'count' in res and res['count'] == 0 +assert 'count' in res and res['count'] == count_all @raises(errors.ValidationError) def test_0028_find_negative_size(self): @@ -453,7 +454,7 @@ class test_cert_find(XMLRPC_test): res = api.Command['cert_find'](subject=u'ipatestcert.%s' % api.env.domain) assert 'count' in res and res['count'] >= 1 -@raises(errors.ValidationError) +@raises(errors.ConversionError) def test_0031_search_on_invalid_date(self): """ Search using invalid date format -- 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] [PATCH] API compatibility
Hello! Honza has pushed first patches needed for API compatibility into his GitHub repo [1]. I have reviewed patch set adding command versioning: frontend: forward command calls using full name schema: support plugin versioning plugable: support plugin versioning plugable: use plugin class as the key in API namespaces misc: generate `plugins` result directly in the command Works as expected, ACK. [1] https://github.com/jcholast/freeipa/commits/trac-4427 -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0139] DNS: Fix tests for realm domains integration with DNS zone ad
On 27/06/16 11:48, Petr Spacek wrote:
Hello,
DNS: Fix tests for realm domains integration with DNS zone add
We forgot to update tests after change in
22f4045f72daf182c44ce574291c0d8a7733713b.
https://fedorahosted.org/freeipa/ticket/5980
It should go to master, 4-3, and 4-2 as well (as the original change).
idnsforwarders needs to be changed on both occurrences. Fixed patch
attached. Petr^2 approved the change. Attached version works as expected
and the test is green again, ACK.
--
David Kupka
From c33e67cdfae7cf03a638fc27a69fa8cc39fc4301 Mon Sep 17 00:00:00 2001
From: Petr Spacek
Date: Mon, 27 Jun 2016 11:46:09 +0200
Subject: [PATCH] DNS: Fix tests for realm domains integration with DNS zone
add
We forgot to update tests after change in
22f4045f72daf182c44ce574291c0d8a7733713b.
https://fedorahosted.org/freeipa/ticket/5980
---
ipatests/test_xmlrpc/test_dns_realmdomains_integration.py | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/ipatests/test_xmlrpc/test_dns_realmdomains_integration.py b/ipatests/test_xmlrpc/test_dns_realmdomains_integration.py
index 9482ef6abf24d87f28447b1219f628662617528b..4b244e0cbba9eede6718d3497d6635d497127aa3 100644
--- a/ipatests/test_xmlrpc/test_dns_realmdomains_integration.py
+++ b/ipatests/test_xmlrpc/test_dns_realmdomains_integration.py
@@ -135,12 +135,12 @@ class test_dns_realmdomains_integration(Declarative):
),
dict(
-desc='Check realmdomain and TXT record do not get created '
- 'during dnszone_add for forwarded zone',
+desc='Check realmdomain and TXT record gets created '
+ 'during dnszone_add for master zone with a forwarder',
command=(
'dnszone_add', [dnszone_2], {
'idnssoarname': idnssoarname,
-'idnsforwarders': u'1.2.3.4',
+'idnsforwarders': u'198.18.19.20',
'idnsforwardpolicy': u'only',
}
),
@@ -162,7 +162,7 @@ class test_dns_realmdomains_integration(Declarative):
'idnsname': [DNSName(dnszone_2_absolute)],
'idnszoneactive': [u'TRUE'],
'idnssoamname': [self_server_ns_dnsname],
-'idnsforwarders': [u'1.2.3.4'],
+'idnsforwarders': [u'198.18.19.20'],
'idnsforwardpolicy': [u'only'],
'nsrecord': lambda x: True,
'idnssoarname': [DNSName(idnssoarname)],
@@ -182,7 +182,7 @@ class test_dns_realmdomains_integration(Declarative):
},
},
-extra_check=assert_realmdomain_and_txt_record_not_present,
+extra_check=assert_realmdomain_and_txt_record_present,
),
dict(
--
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] [WIP] Thin client
On 28/04/16 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hello! Patch set: schema: client-side cleanup automember: fix automember to work with thin client schema: do not crash in command_defaults if argument is None schema: fix param default value handling works* for me, ACK. *) xmlrpc test for automember_plugin test started failing with automember patch. Fix for test attached. -- David Kupka From e736f1d1c9e5247b95fab49a5ea8fa15d6077981 Mon Sep 17 00:00:00 2001 From: David Kupka Date: Mon, 27 Jun 2016 14:52:27 +0200 Subject: [PATCH] test: automember: Fix expected exception message --- ipatests/test_xmlrpc/test_automember_plugin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ipatests/test_xmlrpc/test_automember_plugin.py b/ipatests/test_xmlrpc/test_automember_plugin.py index 9060b69bd57aa8a8a2704dfd233ff3fa75da715a..ffbc91104ab504a98099babb024f9edab114ac5b 100644 --- a/ipatests/test_xmlrpc/test_automember_plugin.py +++ b/ipatests/test_xmlrpc/test_automember_plugin.py @@ -196,7 +196,7 @@ class TestNonexistentAutomember(XMLRPC_test): group1.ensure_missing() command = automember_group.make_delete_command() with raises_exact(errors.NotFound( -reason=u': Automember rule not found')): +reason=u'%s: Automember rule not found' % group1.cn)): command() def test_create_with_nonexistent_hostgroup(self, automember_hostgroup, @@ -214,7 +214,7 @@ class TestNonexistentAutomember(XMLRPC_test): hostgroup1.ensure_missing() command = automember_hostgroup.make_delete_command() with raises_exact(errors.NotFound( -reason=u': Automember rule not found')): +reason=u'%s: Automember rule not found' % hostgroup1.cn)): command() -- 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] [PATCHES 0069-0077] support for proper Kerberos principal canonicalization
On 22/06/16 18:56, Simo Sorce wrote:
On Wed, 2016-06-22 at 18:36 +0200, Martin Babinsky wrote:
On 06/22/2016 06:26 PM, Simo Sorce wrote:
On Wed, 2016-06-22 at 09:46 +0200, Martin Babinsky wrote:
On 10/05/2015 03:00 PM, Martin Babinsky wrote:
These patches implement the plumbing required to properly support
canonicalization of Kerberos principals (
https://fedorahosted.org/freeipa/ticket/3864).
Setting multiple principal aliases on hosts/services is beyond the scope
of this patchset and should be done after these patches are pushed.
I will try to send some tests for the patches later this week.
Please review the hell out of them.
Long time no see.
I am attaching rebased infrastructure patches which were reviewed and
tested by David a year ago :). Now that all related DS bugs were fixed
and the patches still work as expected, we may push them so that the
plumbing for further work (API for alias handling etc.) is in place.
If the patches were all reviewed and tested I say push them.
Simo.
There is one problem remaining, however, that when a user is kinit'ing
for the first name using his alias and has to change password, the
operation fails:
"""
[root@master1 ~]# kinit -C talias
Password for tal...@ipa.test:
kinit: KDC reply did not match expectations while getting initial
credentials
"""
This is the related snippet from KDC log:
"""
Jun 22 16:29:24 master1.ipa.test krb5kdc[31003](info): AS_REQ (6 etypes
{18 17 16 23 25 26}) 192.168.122.100: CLIENT KEY EXPIRED:
tal...@ipa.test for krbtgt/ipa.t...@ipa.test, Password has expired
Jun 22 16:29:24 master1.ipa.test krb5kdc[31003](info): closing down fd 12
Jun 22 16:29:24 master1.ipa.test krb5kdc[31003](info): AS_REQ (6 etypes
{18 17 16 23 25 26}) 192.168.122.100: NEEDED_PREAUTH: tal...@ipa.test
for kadmin/chang...@ipa.test, Additional pre-authentication required
Jun 22 16:29:24 master1.ipa.test krb5kdc[31003](info): closing down fd 12
Jun 22 16:29:28 master1.ipa.test krb5kdc[31003](info): AS_REQ (6 etypes
{18 17 16 23 25 26}) 192.168.122.100: ISSUE: authtime 1466612968, etypes
{rep=18 tkt=18 ses=18}, tal...@ipa.test for kadmin/chang...@ipa.test
Jun 22 16:29:28 master1.ipa.test krb5kdc[31003](info): closing down fd 12
"""
Here is the same command repeated with captured libkrb5 trace:
https://paste.fedoraproject.org/383358/14666131
If I use kinit with the canonical principal everything works as
expected, even with '-C' and '-'E' options. Subsequent kinits using
canonicalization work as expected.
Frankly I have no idea why this happens and I do not know how much this
error blocks us. We may need to investigate this before pizza orders arrive.
I guess the password changing code is making a request without
canonicalization flags set for the kpasswd service.
As you can see the kpasswd case is special because we are making an AS
REQ (not a TGS req) directly for that service, and that request needs to
use canonicalization as well, it probably isn't.
Simo.
Hello!
I've reviewed and tested the patches a year ago and now with current
master again. The only issue I've found is the problem Martin is
describing. User can not kinit with alias after password reset.
Since this is not regression I believe the patches can be pushed now.
and the issue can be solved together with the rest of missing
functionality. ACK!
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 551-552, 623-624] cert: add owner information, allow search by certificate
On 21/06/16 07:19, Jan Cholasta wrote: On 20.6.2016 15:31, Jan Cholasta wrote: On 20.6.2016 09:54, Jan Cholasta wrote: On 15.6.2016 12:33, Jan Cholasta wrote: On 14.6.2016 11:44, Jan Cholasta wrote: On 21.4.2016 09:11, Jan Cholasta wrote: On 6.4.2016 15:46, Pavel Vomacka wrote: On 03/16/2016 01:50 PM, Jan Cholasta wrote: Hi, the attached patches implement the server-side part of <https://fedorahosted.org/freeipa/ticket/5381>. Honza Hi, thank you for the patches. I tested them and they work well. But I would like to ask you whether would be possible to extend the response of 'basecert_find' method and probably also 'basecert_show' response. I think of these information: 1) information whether the certificate is issued by our CA or not. You can check for that by comparing the issuer name of the certificate to "CN=Certificate Authority,$SUBJECT_BASE". You can get subject base from config-show. 2) this probably wouldn't be possible (as we discussed), but I rather write it too - the information about revocation reason. The same as the 'cert_show' provides. Added --check-revocation flag to request this information. Currently it works only on certificates issued by our CA. 3) MD5 and SHA1 fingerprints as the 'cert_show' method returns Added, also included SHA-256. Thank you again. Updated patches attached. Updated and rebased patches attached. Requires Fraser's sub-CA patches. Attaching updated patch 623, which fixes these issues found by David: <https://paste.fedoraproject.org/378997/65913663/>. Updated and rebased patches attached. Attaching updated patches 552 and 623, which fix the --sizelimit option. Updated and rebased patches attached. The --revocation-reason option now works as expected. Hello! Thanks for patch set. Code looks good to me and works as expected. Pavel will test it with WebUI and the we can hopefully push it. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [WIP] Thin client
On 28/04/16 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hello! Patches: replica install: fix thin client regression schema: remove `no_cli` from command schema schema: remove redundant information schema: merge command args and options schema: remove output_params schema: add object class schema permission: handle ipapermright deprecated CLI alias on the client passwd: handle sort order of passwd argument on the client misc: skip `count` and `total` output in env.output_for_cli dns: do not rely on custom param fields in record attributes automember: add object plugin for automember_rebuild frontend: do not crash on missing output in output_for_cli frontend: skip `value` output in output_for_cli frontend: don't copy command arguments to output params makeaci, makeapi: use in-server API work for me, ACK. Note: There is one one known issue in automember output, will be fixed later. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH 0105] Remove unused locking "context manager"
Just dropping tiny piece of no longer used code.
--
David Kupka
From c493fd213690e10960b805900c62504a63740d43 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Fri, 17 Jun 2016 15:12:29 +0200
Subject: [PATCH] Remove unused locking "context manager"
Class ods_db_lock is unused since August 2015.
---
daemons/dnssec/ipa-ods-exporter | 13 -
1 file changed, 13 deletions(-)
diff --git a/daemons/dnssec/ipa-ods-exporter b/daemons/dnssec/ipa-ods-exporter
index 02e6b054abdd96dbeef82fc39339f5f6e98685af..9d610021a1fc56f8466e7b30d5dccc59b7c0e88a 100755
--- a/daemons/dnssec/ipa-ods-exporter
+++ b/daemons/dnssec/ipa-ods-exporter
@@ -19,7 +19,6 @@ from binascii import hexlify
from datetime import datetime
import dateutil.tz
import dns.dnssec
-import fcntl
from krbV import Krb5Error
import logging
import os
@@ -200,18 +199,6 @@ def ods2bind_timestamps(key_state, key_type, ods_times):
return bind_times
-class ods_db_lock(object):
-def __enter__(self):
-self.f = open(ODS_DB_LOCK_PATH, 'w')
-log.debug('waiting for lock %r', self.f)
-fcntl.lockf(self.f, fcntl.LOCK_EX)
-log.debug('acquired lock %r', self.f)
-
-def __exit__(self, *args):
-fcntl.lockf(self.f, fcntl.LOCK_UN)
-log.debug('released lock %r', self.f)
-self.f.close()
-
def get_ldap_zone(ldap, dns_base, name):
zone_names = ["%s." % name, name]
--
2.5.5
From e2276e08a52f979131a155436a78fad7ee7fc56d Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Fri, 17 Jun 2016 15:12:29 +0200
Subject: [PATCH] Remove unused locking "context manager"
Class ods_db_lock is unused since August 2015.
---
daemons/dnssec/ipa-ods-exporter | 13 -
1 file changed, 13 deletions(-)
diff --git a/daemons/dnssec/ipa-ods-exporter b/daemons/dnssec/ipa-ods-exporter
index 2aa936040c373e366e7e15539ed6e3413aac7d55..385764a6d087f04f5a4efe64cd7be72877f71f1e 100755
--- a/daemons/dnssec/ipa-ods-exporter
+++ b/daemons/dnssec/ipa-ods-exporter
@@ -20,7 +20,6 @@ from binascii import hexlify
from datetime import datetime
import dateutil.tz
import dns.dnssec
-import fcntl
from gssapi.exceptions import GSSError
import logging
import os
@@ -197,18 +196,6 @@ def ods2bind_timestamps(key_state, key_type, ods_times):
return bind_times
-class ods_db_lock(object):
-def __enter__(self):
-self.f = open(ODS_DB_LOCK_PATH, 'w')
-log.debug('waiting for lock %r', self.f)
-fcntl.lockf(self.f, fcntl.LOCK_EX)
-log.debug('acquired lock %r', self.f)
-
-def __exit__(self, *args):
-fcntl.lockf(self.f, fcntl.LOCK_UN)
-log.debug('released lock %r', self.f)
-self.f.close()
-
def get_ldap_zone(ldap, dns_base, name):
zone_names = ["%s." % name, name]
--
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] Schema caching for thin client
On 06/15/2016 08:15 PM, Petr Vobornik wrote: On 06/15/2016 02:36 PM, David Kupka wrote: Hello! Schema caching for thin client is available here: https://github.com/dkupka/freeipa/commits/schema_cache Comments and reviews welcome. Enjoy! Not doing proper review. I'll test by using it. But: 1. lint fails Pylint is running, please wait ... * Module ipaclient.remote_plugins.schema_cache ipaclient/remote_plugins/schema_cache.py:283: [W1612(unicode-builtin), _refresh_schema] unicode built-in referenced) Makefile:137: recipe for target 'lint' failed make: *** [lint] Error 1 I.e, you miss: import six if six.PY3: unicode = str Thanks for the catch, fixed version force-pushed. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH] Schema caching for thin client
Hello! Schema caching for thin client is available here: https://github.com/dkupka/freeipa/commits/schema_cache Comments and reviews welcome. Enjoy! -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [WIP] Thin client
On 06/15/2016 01:33 PM, David Kupka wrote: On 04/28/2016 02:45 PM, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hello! Next batch: schema: exclude local commands frontend: call `execute` rather than `forward` in Local dns, passwd: fix output validation error misc: fix CLI output of `env` and `plugins` commands ipalib: replace Any with Dict schema: generate client-side commands on demand plugable: initialize plugins on demand plugable: allow plugins to be non-classes There is known regression that arguments with dynamic defaults are not filled automatically. This will be fixed soon. Otherwise works for me, ACK. Fix for dynamic defaults regression is: schema: fix client-side dynamic defaults Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [WIP] Thin client
On 04/28/2016 02:45 PM, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hello! Next batch: schema: exclude local commands frontend: call `execute` rather than `forward` in Local dns, passwd: fix output validation error misc: fix CLI output of `env` and `plugins` commands ipalib: replace Any with Dict schema: generate client-side commands on demand plugable: initialize plugins on demand plugable: allow plugins to be non-classes There is known regression that arguments with dynamic defaults are not filled automatically. This will be fixed soon. Otherwise works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH 0103-4] installer: Fix single command replica install with --setup-dns
Should go into master, ipa-4-3 and ipa-4-2.
https://fedorahosted.org/freeipa/ticket/5945
--
David Kupka
From c980a7d0b06ec41f4151ff2b04244f1bc3c55b12 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 9 Jun 2016 07:57:43 +0200
Subject: [PATCH 1/2] installer: positional_arguments must be tuple or list of
strings
Setting string here was causing search for substring instead of search for value
in tuple or list.
https://fedorahosted.org/freeipa/ticket/5945
---
install/tools/ipa-replica-install | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index 10a10827ec03a375e5d3bece0a0c6e23df8cffc0..17fc957a583739bbda386676f44209e196282a9a 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -26,7 +26,7 @@ from ipaserver.install.server import Replica
ReplicaInstall = cli.install_tool(
Replica,
command_name='ipa-replica-install',
-positional_arguments='replica_file',
+positional_arguments=['replica_file'],
usage='%prog [options] REPLICA_FILE',
log_file_name=paths.IPAREPLICA_INSTALL_LOG,
debug_option=True,
--
2.7.4
From 2304b08d60a7255e020ffdefd0f0b05c129425de Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 9 Jun 2016 07:58:32 +0200
Subject: [PATCH 2/2] installer: index() raises ValueError
Expecting IndexError instead of ValueError led to traceback instead of correctly
reporting the error situation.
https://fedorahosted.org/freeipa/ticket/5945
---
ipapython/install/cli.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipapython/install/cli.py b/ipapython/install/cli.py
index aed0bc9fe12e0c56987a4e2f78d73f476dcfc2c8..9cd9ec76842b80b4f18b3282c80fb578ae09ba42 100644
--- a/ipapython/install/cli.py
+++ b/ipapython/install/cli.py
@@ -299,9 +299,9 @@ class ConfigureTool(admintool.AdminTool):
knob_cls = knob_classes[e.name]
try:
if self.positional_arguments is None:
-raise IndexError
+raise ValueError
index = self.positional_arguments.index(e.name)
-except IndexError:
+except ValueError:
cli_name = knob_cls.cli_name or e.name.replace('_', '-')
desc = "option --{0}".format(cli_name)
else:
--
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] [WIP] Thin client
On 08/06/16 14:44, Jan Cholasta wrote: On 8.6.2016 14:40, Martin Babinsky wrote: On 06/08/2016 02:11 PM, David Kupka wrote: On 28/04/16 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hello! Patch set: spec file: require correct packages to get API plugins schema: fix typo schema: fix topic command output replica install: use remote server API to create service entries This one is not complete since it breaks replica install w/ --setup-dns. Removing this line seems to fix this case: """ diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py index 41eee96..d695a15 100644 --- a/ipaserver/install/server/replicainstall.py +++ b/ipaserver/install/server/replicainstall.py @@ -1478,7 +1478,6 @@ def promote(installer): server_api.finalize() if options.setup_dns: -server_api.Backend.rpcclient.connect() server_api.Backend.ldap2.connect(autobind=True) dns.install(False, True, options, server_api """ Fixed. schema: do not validate unrequested params in command_defaults fixes some regressions introduced by previous patches. Works for me and I haven't met any new regression or bug, ACK. Thanks for the catch, Martin. Now it's really fixed and can be pushed. Obligatory keyword to trigger the push: ACK BTW, I've discovered other related issue [1] that is there since 4.2. I will send patch soon. [1] https://fedorahosted.org/freeipa/ticket/5945 -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] man: Decribe ipa-client-install workaround for broken D-Bus enviroment.
On 02/03/16 11:18, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/5694 Sending updated version crafted with Flo's help, thanks. -- David Kupka From a7d878e3922720b03b36a2a3b697f8c6c66cc383 Mon Sep 17 00:00:00 2001 From: David Kupka Date: Wed, 2 Mar 2016 11:08:19 +0100 Subject: [PATCH] man: Decribe ipa-client-install workaround for broken D-Bus enviroment. https://fedorahosted.org/freeipa/ticket/5694 --- client/man/ipa-client-install.1 | 11 +++ 1 file changed, 11 insertions(+) diff --git a/client/man/ipa-client-install.1 b/client/man/ipa-client-install.1 index 92ea77a4bda539f8614f3d47cac7b53faf57482c..7f490d153b12d714e7bda7a6abe72fb0756d520c 100644 --- a/client/man/ipa-client-install.1 +++ b/client/man/ipa-client-install.1 @@ -176,6 +176,17 @@ valid for the IPA domain. .TP \fB\-\-request\-cert\fR Request certificate for the machine. The certificate will be stored in /etc/ipa/nssdb under the nickname "Local IPA host". + +Using this option requires that D-Bus is properly configured or not configured +at all. In environment where this condition is not met (e.g. anaconda kickstart +chroot environment) set the system bus address to /dev/null to enable +workaround in ipa-client-install. + +# env DBUS_SYSTEM_BUS_ADDRESS=unix:path=/dev/null ipa-client-install --request-cert + +Note that requesting the certificate when certmonger is not running only +creates tracking request and the certmonger service must be started to be able +to track certificates. .TP \fB\-\-automount\-location\fR=\fILOCATION\fR Configure automount by running ipa\-client\-automount(1) with \fILOCATION\fR as -- 2.5.5 -- 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] [WIP] Thin client
On 28/04/16 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hello! Patch set: spec file: require correct packages to get API plugins schema: fix typo schema: fix topic command output replica install: use remote server API to create service entries schema: do not validate unrequested params in command_defaults fixes some regressions introduced by previous patches. Works for me and I haven't met any new regression or bug, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH 0102] test: test_cli: Do not expect defaults in kwargs.
https://fedorahosted.org/freeipa/ticket/4739
With this patch all but one test in test_cli.py will pass again. The one
failing is bug in the dns* commands prompt behavior and will be fixed soon.
--
David Kupka
From b3501bdc4a3195b8736a8d05992874ec72e7d97f Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Fri, 3 Jun 2016 09:21:08 +0200
Subject: [PATCH] test: test_cli: Do not expect defaults in kwargs.
Client is no longer forwarding in arguments with default values to the server.
https://fedorahosted.org/freeipa/ticket/4739
---
ipatests/test_cmdline/test_cli.py | 126 ++
1 file changed, 46 insertions(+), 80 deletions(-)
diff --git a/ipatests/test_cmdline/test_cli.py b/ipatests/test_cmdline/test_cli.py
index c2e051769550ae2b25b951731cc6f10bc05b2212..a6e6363cbc6d992743a1c07f5eb21c00370256b0 100644
--- a/ipatests/test_cmdline/test_cli.py
+++ b/ipatests/test_cmdline/test_cli.py
@@ -52,31 +52,18 @@ class TestCLIParsing(object):
self.check_command('ping', 'ping')
def test_user_show(self):
-self.check_command('user-show admin', 'user_show',
-uid=u'admin',
-rights=False,
-no_members=False,
-raw=False,
-all=False)
+self.check_command('user-show admin', 'user_show', uid=u'admin')
def test_user_show_underscore(self):
-self.check_command('user_show admin', 'user_show',
-uid=u'admin',
-rights=False,
-no_members=False,
-raw=False,
-all=False)
+self.check_command('user_show admin', 'user_show', uid=u'admin')
def test_group_add(self):
-self.check_command('group-add tgroup1 --desc="Test group"',
+self.check_command(
+'group-add tgroup1 --desc="Test group"',
'group_add',
cn=u'tgroup1',
description=u'Test group',
-nonposix=False,
-external=False,
-no_members=False,
-raw=False,
-all=False)
+)
def test_sudocmdgroup_add_member(self):
# Test CSV splitting is not done
@@ -86,53 +73,41 @@ class TestCLIParsing(object):
'sudocmdgroup_add_member',
cn=u'tcmdgroup1',
sudocmd=[u'ab,c', u'd'],
-no_members=False,
-raw=False,
-all=False)
+)
def test_group_add_nonposix(self):
-self.check_command('group-add tgroup1 --desc="Test group" --nonposix',
+self.check_command(
+'group-add tgroup1 --desc="Test group" --nonposix',
'group_add',
cn=u'tgroup1',
description=u'Test group',
nonposix=True,
-external=False,
-no_members=False,
-raw=False,
-all=False)
+)
def test_group_add_gid(self):
-self.check_command('group-add tgroup1 --desc="Test group" --gid=1234',
+self.check_command(
+'group-add tgroup1 --desc="Test group" --gid=1234',
'group_add',
cn=u'tgroup1',
description=u'Test group',
gidnumber=u'1234',
-nonposix=False,
-external=False,
-no_members=False,
-raw=False,
-all=False)
+)
def test_group_add_interactive(self):
with self.fake_stdin('Test group\n'):
-self.check_command('group-add tgroup1', 'group_add',
+self.check_command(
+'group-add tgroup1', 'group_add',
cn=u'tgroup1',
-nonposix=False,
-external=False,
-no_members=False,
-raw=False,
-all=False)
+)
def test_dnsrecord_add(self):
-self.check_command('dnsrecord-add %s ns --a-rec=1.2.3.4' % TEST_ZONE,
+self.check_command(
+'dnsrecord-add %s ns --a-rec=1.2.3.4' % TEST_ZONE,
'dnsrecord_add',
dnszoneidnsname=TEST_ZONE,
idnsname=u'ns',
arecord=u'1.2.3.4',
-structured=False,
-force=False,
-raw=False,
-all=False)
+)
def test_dnsrecord_del_all(self):
try:
@@ -144,19 +119,21 @@ class TestCLIParsing(object):
dnszoneidnsname=TEST_ZONE,
idnsname=u'ns', arecord=u'1.2.3.4', force=True)
Re: [Freeipa-devel] [WIP] Thin client
On 25/05/16 16:07, Jan Cholasta wrote: On 25.5.2016 15:03, David Kupka wrote: On 18/05/16 08:01, Jan Cholasta wrote: On 16.5.2016 16:35, Martin Basti wrote: On 09.05.2016 14:07, Jan Cholasta wrote: On 6.5.2016 14:32, Martin Basti wrote: On 28.04.2016 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza I did not test it yet, I just checked the code * automount: do not inherit automountlocation_tofiles from LDAPQuery * LGTM * dns: move all dnsrecord code called on client to a single class * LGTM * dns: do not rely on server data structures in code called on client * 1) you forgot to increment VERSION This was deliberate, as it will no longer be necessary to bump VERSION for backward compatible changes after this whole patchset is merged. But we're not there yet, so fixed. How we should handle VERSION after your patches? Otherwise LGTM * otptoken: fix import of DN * ACK * otptoken_yubikey: fix otptoken_add_yubikey arguments * 1) you forgot to increment VERSION Fixed. 2) Did you find out why this was issue? -del answer['value']# Why does this cause an error if omitted? -del answer['summary'] # Why does this cause an error if omitted? The command definition was not complete, it was missing has_output. Otherwise LGTM * vault: move client-side code to the module level * LGTM * vault: copy arguments of client commands from server counterparts * 1) you forgot to increment Version Fixed. * ipalib: use relative imports for cross-plugin imports * 1) Missing explanation for future generations why this change is needed in commit message Fixed. The other commits I will check later. Martin^2 Okay. Thanks. * frontend: remove the unused Command.soft_validate method LGTM * frontend: perform argument value validation only on server LGTM * frontend: do not forward argument defaults to server I'm not a fan of returning None in promt_param function, but I havent found anything better to use. It always returned None for unset params. LGTM * ipalib: optimize API.txt this contains a lot of black magic, but because this is mainly copy of current to proper places, LGTM It's actually mostly cut-n-paste. * makeaci: load additional plugins using API.add_module Looks good, but I miss explanation why is this change needed The next change would be impossible without this. * plugable: replace API.import_plugins with new API.add_package LGTM * ipalib, ipaserver: migrate all plugins to Registry-based registration ACK * ipalib, ipaserver: fix incorrect API.register calls in docstrings LGTM * plugable: remove the unused deprecated API.register method LGTM * plugable: switch API to Registry-based plugin discovery LGTM * frontend: merge baseldap.CallbackRegistry into Command LGTM *frontend: move the interactive_prompt callback type to Command LGTM Martin^2 Hello, first batch of 30 patches from Honza's trac-4739 branch (https://github.com/jcholast/freeipa/commits/trac-4739) is ready to be pushed into master. All upto "frontend: allow commands to have an argument named `name`" got over numerous test&fix cycles in last week+ and is working as expected now, ACK. Thanks for the review. Honzo, please rebase and push them, thanks! Attaching the patches for reference. Pushed to master: 2b50fc617024f18a81e97f30f75ed1b9221c4055 Hello, another patchset is ready. There are still some minor issues with interactive prompt in dns* commands but these can be fixed later. Otherwise all work as expected, ACK. Also it would be good to have tests for schema plugin, I have filled a ticket [1]. List of commits in Honza's trac-4739 branch: frontend: do not check API minor version of the client ipalib: move server-side plugins to ipaserver ipaclient: implement thin client misc: hide the unused --all option of `env` and `plugins` in CLI ipalib: move File command arguments to ipaclient ipactl: use server API client install: finalize API after CA certs are available rpc: do not validate command name in RPCClient.forward rpc: optimize JSON-RPC response handling rpc: specify connection options in API config rpc: allow overriding NSS DB directory in API config rpc: respect API config in RPCClient.create_connection ipalib: introduce API schema plugins ipalib: replace DeprecatedParam with `deprecated` Param argument parameters: introduce no_convert keyword argument parameters: introduce cli_metavar keyword argument ipalib: split off client-side plugin code into ipaclient dns: move code shared by client and server to separate module ipaclient: add client-side command override class frontend: turn Method attr
Re: [Freeipa-devel] [WIP] Thin client
On 18/05/16 08:01, Jan Cholasta wrote: On 16.5.2016 16:35, Martin Basti wrote: On 09.05.2016 14:07, Jan Cholasta wrote: On 6.5.2016 14:32, Martin Basti wrote: On 28.04.2016 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza I did not test it yet, I just checked the code * automount: do not inherit automountlocation_tofiles from LDAPQuery * LGTM * dns: move all dnsrecord code called on client to a single class * LGTM * dns: do not rely on server data structures in code called on client * 1) you forgot to increment VERSION This was deliberate, as it will no longer be necessary to bump VERSION for backward compatible changes after this whole patchset is merged. But we're not there yet, so fixed. How we should handle VERSION after your patches? Otherwise LGTM * otptoken: fix import of DN * ACK * otptoken_yubikey: fix otptoken_add_yubikey arguments * 1) you forgot to increment VERSION Fixed. 2) Did you find out why this was issue? -del answer['value']# Why does this cause an error if omitted? -del answer['summary'] # Why does this cause an error if omitted? The command definition was not complete, it was missing has_output. Otherwise LGTM * vault: move client-side code to the module level * LGTM * vault: copy arguments of client commands from server counterparts * 1) you forgot to increment Version Fixed. * ipalib: use relative imports for cross-plugin imports * 1) Missing explanation for future generations why this change is needed in commit message Fixed. The other commits I will check later. Martin^2 Okay. Thanks. * frontend: remove the unused Command.soft_validate method LGTM * frontend: perform argument value validation only on server LGTM * frontend: do not forward argument defaults to server I'm not a fan of returning None in promt_param function, but I havent found anything better to use. It always returned None for unset params. LGTM * ipalib: optimize API.txt this contains a lot of black magic, but because this is mainly copy of current to proper places, LGTM It's actually mostly cut-n-paste. * makeaci: load additional plugins using API.add_module Looks good, but I miss explanation why is this change needed The next change would be impossible without this. * plugable: replace API.import_plugins with new API.add_package LGTM * ipalib, ipaserver: migrate all plugins to Registry-based registration ACK * ipalib, ipaserver: fix incorrect API.register calls in docstrings LGTM * plugable: remove the unused deprecated API.register method LGTM * plugable: switch API to Registry-based plugin discovery LGTM * frontend: merge baseldap.CallbackRegistry into Command LGTM *frontend: move the interactive_prompt callback type to Command LGTM Martin^2 Hello, first batch of 30 patches from Honza's trac-4739 branch (https://github.com/jcholast/freeipa/commits/trac-4739) is ready to be pushed into master. All upto "frontend: allow commands to have an argument named `name`" got over numerous test&fix cycles in last week+ and is working as expected now, ACK. Honzo, please rebase and push them, thanks! -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [WIP] Thin client
On 28/04/16 14:45, Jan Cholasta wrote: Hi, I have pushed my thin client WIP branch to GitHub: <https://github.com/jcholast/freeipa/tree/trac-4739>. All commits up to "ipalib: use relative imports for cross-plugin imports" should be good for review. The rest is subject to change (WARNING: I will force push into this branch). Honza Hi, I have fought my way through the patch set up to "frontend: allow commands to have an argument named `name`" (including). The code looks good to me but I haven't dive into parts that was pure Cut&Paste. I assume that when it worked before it will still work. Some problems pop out in tests: user_plugin: http://paste.fedoraproject.org/367502/34888591/ This can be fixed with simple patch: http://paste.fedoraproject.org/367503/14634889/ With patch applied there are still some errors: http://paste.fedoraproject.org/367519/49007414/ -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0472] fix stageuser-find test
On 04/05/16 16:28, Martin Basti wrote: https://fedorahosted.org/freeipa/ticket/5281 I forgot to send patch that fixes also stageuser tests with current changes in has_keytab and has_password attributes. Patch attached Stageuser test suite is passing again, ACK. test_xmlrpc/test_stageuser_plugin.py . -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] Improving bug reporting
Hello everyone!
I often miss proper reproducer and other important info in trac tickets.
Asking for the missing info or guessing and trying is as ineffective as
it sounds and costs us a lot of time and effort. I believe we can
improve that.
We have guidelines for reporting a bug [1] but it obviously isn't
enough. I propose to prefill track ticket's description with following
(or similar) template and be strict on refusing (i.e. closing as
invalid) tickets that are incomplete.
Any thoughts, suggestions, agreement or disagreement?
[1] http://www.freeipa.org/page/Troubleshooting#Reporting_bugs
--8<- trac-ticket-template-proposal --->8--
Related SW versions:
On server:
{{{
$ rpm -q freeipa-server pki-base 389-ds-base bind samba krb5-server
certmonger
}}}
On client:
{{{
$ rpm -q freeipa-client krb5-workstation certmonger
}}}
Reproducible:
How often the issue occurs or what special condition is required to be met.
Examples:
Always / Happened X times of Y tries / Only at noon 29th February when
it's also Thursday / Only on Raspberry Pi
Steps to reproduce:
Precise description of all related steps you have done. List of commands
to run is the best form.
Example:
{{{
# ipa-server-install -a Secret123 -p Secret123 -r EXAMPLE.TEST -U
# echo Secret123 | kinit admin
}}}
Actual result:
Description of behavior you have observed (error, unexpected warning, ...).
Example:
{{{
kinit: Client 'ad...@example.test' not found in Kerberos database while
getting initial credentials
}}}
Expected result:
Description of behavior you have expected.
Example:
TGT for admin user is acquired.
--8<------->8--
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] MIT KRB5 uses 32bit time stamp
Hello! Recently I have touched password expiration code in ipa_kdb_password.c and noticed that we have IPAPW_END_OF_TIME set to January 1st, 2038. I thought that it's just old code that still assumes 32bit time stamp and that Kerberos surely moved to 64bit long time ago. I was really surprised when I opened /usr/include/krb5/krb5.h and found: typedef krb5_int32 krb5_timestamp; Is there a reason why not just replace the line above with: > typedef krb5_int64 krb5_timestamp; ? I know that it may seem that we have plenty of time to address it but I don't see a reason to wait. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH] pwpolicy: Do not expire passwords when maxlife is set to 0 (infinity).
https://fedorahosted.org/freeipa/ticket/2795
--
David Kupka
From 1f1d64210246cb2a968a7484d1ffcbaf48baa4ff Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Mon, 2 May 2016 13:56:39 +0200
Subject: [PATCH] pwpolicy: Do not expire passwords when maxlife is set to 0
(infinity).
Curently passwords that should expire after IPAPW_END_OF_TIME (Jan 1st 2038) are
set to expire at that time. We need to change MIT Kerberos to use 64bit time
stamps to allow real never-expiring password.
https://fedorahosted.org/freeipa/ticket/2795
---
daemons/ipa-kdb/ipa_kdb_passwords.c | 6 +-
ipalib/plugins/pwpolicy.py | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_passwords.c b/daemons/ipa-kdb/ipa_kdb_passwords.c
index ad57181d5049f36c69044bb2c9cfe183d7e4ea25..2dbdf6f29fce4cbc627ce888afc6f15f6f4e91b2 100644
--- a/daemons/ipa-kdb/ipa_kdb_passwords.c
+++ b/daemons/ipa-kdb/ipa_kdb_passwords.c
@@ -253,7 +253,11 @@ krb5_error_code ipadb_get_pwd_expiration(krb5_context context,
if (truexp) {
if (ied->pol) {
-*expire_time = mod_time + ied->pol->max_pwd_life;
+if (ied->pol->max_pwd_life == 0) {
+*expire_time = IPAPWD_END_OF_TIME; // 1 Jan 2038, 00:00 GMT
+} else {
+*expire_time = mod_time + ied->pol->max_pwd_life;
+}
} else {
*expire_time = mod_time + IPAPWD_DEFAULT_PWDLIFE;
}
diff --git a/ipalib/plugins/pwpolicy.py b/ipalib/plugins/pwpolicy.py
index 86c559b7dfeb7dffaa6c777876c6e65caab02075..0271854456b9ca2968ef58438c7925dfb21930c7 100644
--- a/ipalib/plugins/pwpolicy.py
+++ b/ipalib/plugins/pwpolicy.py
@@ -412,7 +412,7 @@ class pwpolicy(LDAPObject):
maxlife = int(existing_entry['krbmaxpwdlife'][0]) * 86400
if maxlife is not None and minlife is not None:
-if minlife > maxlife:
+if maxlife > 0 and minlife > maxlife: # maxlife = 0 => infinity
raise errors.ValidationError(
name='maxlife',
error=_('Maximum password life must be greater than minimum.'),
--
2.5.5
--
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] Possble FreeIPA Trac Malicious Link
On 25/04/16 15:32, Gabe Alford wrote: Hey all, This is something we may need to watch for. I noticed that a possible malicious link was added to the FreeIPA Trac start page. You can view it here: https://fedorahosted.org/freeipa/wiki/WikiStart?action=diff&version=22. I changed it back to the original text before the change. I know that the 389 Trac webpage had issues earlier this year with spam. Just an FYI. Gabe Hello Gabe, thanks for noticing and fixing. I've contacted Fedora infrastructure admin with request to ban the user and he informed me that the account was already blocked by antispam detection mechanism. Also he added few more rules for faster detection next time. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config
On 14/03/16 14:01, Martin Basti wrote:
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('{
Re: [Freeipa-devel] [PATCHES 0464-0468] always set hostname during installation
On 22/04/16 12:48, Martin Basti wrote: On 20.04.2016 17:49, Martin Basti wrote: https://fedorahosted.org/freeipa/ticket/5794 It requires my patch 441.2 Patches attached. Rebased patches attached, 441 has been pushed Martin^2 Thanks for patch set. Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0463] Performance: do not download password attributes in host/find-user command
On 22/04/16 10:58, Martin Basti wrote: On 21.04.2016 09:17, Martin Basti wrote: On 20.04.2016 16:57, Martin Basti wrote: https://fedorahosted.org/freeipa/ticket/5281 Patch attached. selfNACK Updated patch attached. Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0461] Limit max username lenght to 255 in config-mod
On 15/04/16 15:42, Martin Basti wrote:
On 15.04.2016 15:03, Petr Vobornik wrote:
On 04/15/2016 01:25 PM, Martin Basti wrote:
On 15.04.2016 13:18, David Kupka wrote:
On 14/04/16 17:47, Martin Basti wrote:
https://fedorahosted.org/freeipa/ticket/5774
Patch attached.
Hi,
works for me, ACK.
FYI: While trying this I've hit related issue
(https://fedorahosted.org/freeipa/ticket/5822) but I believe the
impact is minimal.
Pushed to master: 93871bf017e1bc5f2c176aa3419278d49fcc003b
API.txt and version was not updated:
-option: Int('ipamaxusernamelength', attribute=True, autofill=False,
cli_name='maxusername', minvalue=1, multivalue=False, required=False)
+option: Int('ipamaxusernamelength', attribute=True, autofill=False,
cli_name='maxusername', maxvalue=255, minvalue=1, multivalue=False,
required=False)
Sorry.
Patch attached.
Sorry, I haven't noticed this. I remember that previously this type of
errors aborted build. Why was this changed?
Anyway, the patch fixes it, ACK.
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] ca-less tests updated
On 20/04/16 10:35, David Kupka wrote:
On 19/04/16 11:13, Oleg Fayans wrote:
OK, that one, though passing lint, did not actually work. I gave up my
attempts to define method decorators inside the class. Now it passes
lint AND works:)
Hi Oleg!
1) Current commit message is useless. Please use it to describe what is
the point of the patch.
2) $ git show -U0 | pep8 --diff
./ipatests/test_integration/test_caless.py:66:1: E302 expected 2 blank
lines, found 1
./ipatests/test_integration/test_caless.py:74:1: E302 expected 2 blank
lines, found 1
./ipatests/test_integration/test_caless.py:820:5: E303 too many blank
lines (2)
./ipatests/test_integration/test_caless.py:825:80: E501 line too long
(80 > 79 characters)
./ipatests/test_integration/test_caless.py:1035:44: E225 missing
whitespace around operator
3) Isn't there a way to do this with pytest's fixtures?
+def server_install_teardown(func):
+def wrapped(*args):
+try:
+func(*args)
+finally:
+args[0].uninstall_server()
+return wrapped
+
+def replica_install_teardown(func):
+def wrapped(*args):
+try:
+func(*args)
+finally:
+# Uninstall replica
+replica = args[0].replicas[0]
+tasks.kinit_admin(args[0].master)
+args[0].uninstall_server(replica)
+args[0].master.run_command(['ipa-replica-manage', 'del',
+replica.hostname, '--force'],
+ raiseonerr=False)
+args[0].master.run_command(['ipa', 'host-del',
+replica.hostname],
+ raiseonerr=False)
+return wrapped
+
4) Is it necessary to create the $TEST_DIR in the test? Isn't it created
by the framework?
+host.transport.mkdir_recursive(host.config.test_dir)
5) I don't think the comment match the code.
+# Remove CA cert in /etc/pki/nssdb, in case of failed
(un)install
+for host in cls.get_all_hosts():
+cls.uninstall_server(host)
+
super(CALessBase, cls).uninstall(mh)
6) No! Create list with one element, iterate that list and append every
item to the other list. Maybe there's better way (Hint: append).
I've seen this on multiple places.
if unattended:
args.extend(['-U'])
7) Why don't you (extend and) use
ipatests.test_integaration.tasks.(un)install_{master,replica}?
This could be done pretty much all over the code.
host.run_command(['ipa-server-install', '--uninstall', '-U'])
8) Use ipaplatform.paths for certutil and other binaries. If the binary
is not there feel free to add it.
I've seen this on multiple places.
+host.run_command(['certutil', '-d', paths.NSS_DB_DIR, '-D',
+ '-n', 'External CA cert'],
+ raiseonerr=False)
+# A workaround forhttps://fedorahosted.org/freeipa/ticket/4639
+result = host.run_command(['certutil', '-L', '-d',
+ paths.HTTPD_ALIAS_DIR])
+for rawcert in result.stdout_text.split('\n')[4: -1]:
+cert = rawcert.split('')[0]
+host.run_command(['certutil', '-D', '-d',
paths.HTTPD_ALIAS_DIR,
+ '-n', cert])
9) certmonger is system service. You can check if is is .enabled() and
.running(). And IIUC the comment is negation of what the code does.
# Verify certmonger was not started
result = host.run_command(['getcert', 'list'],
raiseonerr=False)
-assert result > 0
-assert ('Please verify that the certmonger service has
been '
-'started.' in result.stdout_text),
result.stdout_text
+assert result.returncode == 0
10) What is the point of calling uninstall_server() when it will be
called in the finally block of server_install_teardown anyway?
+@server_install_teardown
def test_revoked_http(self):
"IPA server install with revoked HTTP certificate"
if result.returncode == 0:
+self.uninstall_server()
raise nose.SkipTest(
"Known CA-less installation defect, see "
+"https://fedorahosted.org/freeipa/ticket/4270";)
assert result.returncode > 0
Nitpick) Do not mix fixing typos/grammar/spelling/style with functional
changes.
-def test_incorect_http_pin(self):
+@pytest.mark.xfail(reason='freeipa ticket 5378')
+def test_incorrect_http_pin(self):
"Install
Re: [Freeipa-devel] [PATCH] ca-less tests updated
On 19/04/16 11:13, Oleg Fayans wrote:
OK, that one, though passing lint, did not actually work. I gave up my
attempts to define method decorators inside the class. Now it passes
lint AND works:)
Hi Oleg!
1) Current commit message is useless. Please use it to describe what is
the point of the patch.
2) $ git show -U0 | pep8 --diff
./ipatests/test_integration/test_caless.py:66:1: E302 expected 2 blank
lines, found 1
./ipatests/test_integration/test_caless.py:74:1: E302 expected 2 blank
lines, found 1
./ipatests/test_integration/test_caless.py:820:5: E303 too many blank
lines (2)
./ipatests/test_integration/test_caless.py:825:80: E501 line too long
(80 > 79 characters)
./ipatests/test_integration/test_caless.py:1035:44: E225 missing
whitespace around operator
3) Isn't there a way to do this with pytest's fixtures?
+def server_install_teardown(func):
+def wrapped(*args):
+try:
+func(*args)
+finally:
+args[0].uninstall_server()
+return wrapped
+
+def replica_install_teardown(func):
+def wrapped(*args):
+try:
+func(*args)
+finally:
+# Uninstall replica
+replica = args[0].replicas[0]
+tasks.kinit_admin(args[0].master)
+args[0].uninstall_server(replica)
+args[0].master.run_command(['ipa-replica-manage', 'del',
+replica.hostname, '--force'],
+ raiseonerr=False)
+args[0].master.run_command(['ipa', 'host-del',
+replica.hostname],
+ raiseonerr=False)
+return wrapped
+
4) Is it necessary to create the $TEST_DIR in the test? Isn't it created
by the framework?
+host.transport.mkdir_recursive(host.config.test_dir)
5) I don't think the comment match the code.
+# Remove CA cert in /etc/pki/nssdb, in case of failed (un)install
+for host in cls.get_all_hosts():
+cls.uninstall_server(host)
+
super(CALessBase, cls).uninstall(mh)
6) No! Create list with one element, iterate that list and append every
item to the other list. Maybe there's better way (Hint: append).
I've seen this on multiple places.
if unattended:
args.extend(['-U'])
7) Why don't you (extend and) use
ipatests.test_integaration.tasks.(un)install_{master,replica}?
This could be done pretty much all over the code.
host.run_command(['ipa-server-install', '--uninstall', '-U'])
8) Use ipaplatform.paths for certutil and other binaries. If the binary
is not there feel free to add it.
I've seen this on multiple places.
+host.run_command(['certutil', '-d', paths.NSS_DB_DIR, '-D',
+ '-n', 'External CA cert'],
+ raiseonerr=False)
+# A workaround forhttps://fedorahosted.org/freeipa/ticket/4639
+result = host.run_command(['certutil', '-L', '-d',
+ paths.HTTPD_ALIAS_DIR])
+for rawcert in result.stdout_text.split('\n')[4: -1]:
+cert = rawcert.split('')[0]
+host.run_command(['certutil', '-D', '-d', paths.HTTPD_ALIAS_DIR,
+ '-n', cert])
9) certmonger is system service. You can check if is is .enabled() and
.running(). And IIUC the comment is negation of what the code does.
# Verify certmonger was not started
result = host.run_command(['getcert', 'list'], raiseonerr=False)
-assert result > 0
-assert ('Please verify that the certmonger service has been '
-'started.' in result.stdout_text), result.stdout_text
+assert result.returncode == 0
10) What is the point of calling uninstall_server() when it will be
called in the finally block of server_install_teardown anyway?
+@server_install_teardown
def test_revoked_http(self):
"IPA server install with revoked HTTP certificate"
if result.returncode == 0:
+self.uninstall_server()
raise nose.SkipTest(
"Known CA-less installation defect, see "
+"https://fedorahosted.org/freeipa/ticket/4270";)
assert result.returncode > 0
Nitpick) Do not mix fixing typos/grammar/spelling/style with functional
changes.
-def test_incorect_http_pin(self):
+@pytest.mark.xfail(reason='freeipa ticket 5378')
+def test_incorrect_http_pin(self):
"Install new HTTP certificate with incorrect PKCS#12 password"
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [DESIGN] Kerberos principal alias handling
On 18/04/16 21:42, Simo Sorce wrote:
On Wed, 2016-04-13 at 07:50 +0200, David Kupka wrote:
On 08/04/16 17:10, Martin Babinsky wrote:
Hi list,
I have put together a draft [1] outlining the effort to reimplement the
handling of Kerberos principals in both backend and frontend layers of
FreeIPA so that we may have multiple aliases per user, host or service
and thus implement stuff like
https://fedorahosted.org/freeipa/ticket/3961 and
https://fedorahosted.org/freeipa/ticket/5413 .
Since much of the plumbing was already implemented,[2] the document
mainly describes what the patches do. Some parts required by other use
cases may be missing so please point these out.
I would also be happy if you could correct all factual inacurracies, I
did research on this issue a long time ago and my knowledge turned a bit
rusty.
[1] http://www.freeipa.org/page/V4/Kerberos_principal_aliases
[2]
https://www.redhat.com/archives/freeipa-devel/2015-October/msg00048.html
Hi,
after reading the designs following thoughts comes to my mind.
1) Just to be sure that I understand the new ticket obtaining process
correctly I'd like to summarize.
We need to always search all krbPrincipalName values, krbCanonicalName
and ipaKrbPrincipalAlias (for backward compatibility).
For TGT request case sensitivity of the search and principal in returned
ticket depends on canonicalization. When canonicalization is requested
the search is case-insensitive and krbCanonicalName is used otherwise
case-sensitive search is performed and principal from request is used.
Yes.
When requesting TGS search is always case-sensitive and principal from
request is used.
No, this sounds wrong to me, I think we want a case-insensitve search
for TGS requests.
In pseudo-code:
get_tgt(principal, secret, canonicalization)
if canonicalization
if principal case-insensitive-in {krbPrincipalName +
ipaKrbPrincipalAlias + krbCanonicalName}
# verify secret, perform various other checks...
return TGT(krbCanonicalName)
else
if principal case-sensitive-in {krbPrincipalName +
ipaKrbPrincipalAlias + krbCanonicalName}
# verify secret, perform various other checks...
return TGT(principal)
get_tgs(service_principal, TGT)
if service_principal case-sensitive-in {krbPrincipalName +
ipaKrbPrincipalAlias + krbCanonicalName}
# verify TGT, perform various other checks...
return TGS(service_principal)
Do I understand it right?
I do not think the TGS part is right.
A case insensitive search in TGS would allow to match upper case service
or host names which are sometime mistakenly used, especially in Windows
born software, given that the AD KDC is case insensitive.
Ok, thanks for correcting me.
2) I would like to add following constrains for
krb{Canonical,Principal}Name attributes:
when user/host/service is created krbCanonicalName is set to the same
value as krbPrincipalName
krbCanonicalName cannot be modified
krbPrincipalName with the same value as krbCanonicalName cannot be
removed/modified
krbPrincipalName must be case-insensitively unique in whole DB
krbPrincipalName attributes can be added and/or removed
+1
This will allow us to keep the first krbPrincipalName as RDN for
services/hosts and give the flexibility of adding/removing aliases.
'Change of username' use case is also solvable with this approach. When
username is changed we add krbPrincipalName with the new username. That
will allow user to login with either old or new name.
-1 for users a rename means we change the principal and the canonical
name and we do not retain any old principal name.
But this is inconsistent with the constrains above, especially
"krbCanonicalName cannot be modified". We have the following options:
1) Do not allow rename for hosts and services but allow it for users.
2) Allow renaming of all objects.
3) Do not allow renaming of anything.
I don't like 1) because it is inconsistent. User renaming is nice
feature so we probably don't want 3). Which leaves us with different set
of constrains:
there always needs to be krbPrincipalName with the same value as
krbCanonicalName
krbPrincipalName must be case-insensitively unique in whole DB
krbPrincipalName attributes can be added and/or removed
Is this what we want? Is it wise to allow renaming of hosts and
services? Is there a use case? Is there any potential danger?
3) ad CLI:
{user,host,service}-add - Can canonicalname be specified? Or will it
take principal argument/option value?
Can we add {user,host,service}-{add,remove}-principal set of commands
for principal manipulation? I really don't want to use
--{add,set,del}-attr unless necessary.
Will {user,host,service}-{show,find} display krbCanonicalName by default
or only with --all option?
4) ad Upgrade:
I think it would be worth to check and document what happens during
upgrade of multiple replicas. There
Re: [Freeipa-devel] [PATCH 0461] Limit max username lenght to 255 in config-mod
On 14/04/16 17:47, Martin Basti wrote: https://fedorahosted.org/freeipa/ticket/5774 Patch attached. Hi, works for me, ACK. FYI: While trying this I've hit related issue (https://fedorahosted.org/freeipa/ticket/5822) but I believe the impact is minimal. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0459] use python-netifaces for detection of the local ip addresses
On 13/04/16 17:43, Martin Basti wrote: https://fedorahosted.org/freeipa/ticket/5591 Patch attached. Hi, two nitpicks: 1) python-netifaces is missing in BuildRequires. 2) iproute package is no longer needed in Requires. Otherwise it works, ACK when freeipa.spec.in is fixed. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [DESIGN] Kerberos principal alias handling
On 08/04/16 17:10, Martin Babinsky wrote:
Hi list,
I have put together a draft [1] outlining the effort to reimplement the
handling of Kerberos principals in both backend and frontend layers of
FreeIPA so that we may have multiple aliases per user, host or service
and thus implement stuff like
https://fedorahosted.org/freeipa/ticket/3961 and
https://fedorahosted.org/freeipa/ticket/5413 .
Since much of the plumbing was already implemented,[2] the document
mainly describes what the patches do. Some parts required by other use
cases may be missing so please point these out.
I would also be happy if you could correct all factual inacurracies, I
did research on this issue a long time ago and my knowledge turned a bit
rusty.
[1] http://www.freeipa.org/page/V4/Kerberos_principal_aliases
[2]
https://www.redhat.com/archives/freeipa-devel/2015-October/msg00048.html
Hi,
after reading the designs following thoughts comes to my mind.
1) Just to be sure that I understand the new ticket obtaining process
correctly I'd like to summarize.
We need to always search all krbPrincipalName values, krbCanonicalName
and ipaKrbPrincipalAlias (for backward compatibility).
For TGT request case sensitivity of the search and principal in returned
ticket depends on canonicalization. When canonicalization is requested
the search is case-insensitive and krbCanonicalName is used otherwise
case-sensitive search is performed and principal from request is used.
When requesting TGS search is always case-sensitive and principal from
request is used.
In pseudo-code:
get_tgt(principal, secret, canonicalization)
if canonicalization
if principal case-insensitive-in {krbPrincipalName +
ipaKrbPrincipalAlias + krbCanonicalName}
# verify secret, perform various other checks...
return TGT(krbCanonicalName)
else
if principal case-sensitive-in {krbPrincipalName +
ipaKrbPrincipalAlias + krbCanonicalName}
# verify secret, perform various other checks...
return TGT(principal)
get_tgs(service_principal, TGT)
if service_principal case-sensitive-in {krbPrincipalName +
ipaKrbPrincipalAlias + krbCanonicalName}
# verify TGT, perform various other checks...
return TGS(service_principal)
Do I understand it right?
2) I would like to add following constrains for
krb{Canonical,Principal}Name attributes:
when user/host/service is created krbCanonicalName is set to the same
value as krbPrincipalName
krbCanonicalName cannot be modified
krbPrincipalName with the same value as krbCanonicalName cannot be
removed/modified
krbPrincipalName must be case-insensitively unique in whole DB
krbPrincipalName attributes can be added and/or removed
This will allow us to keep the first krbPrincipalName as RDN for
services/hosts and give the flexibility of adding/removing aliases.
'Change of username' use case is also solvable with this approach. When
username is changed we add krbPrincipalName with the new username. That
will allow user to login with either old or new name.
3) ad CLI:
{user,host,service}-add - Can canonicalname be specified? Or will it
take principal argument/option value?
Can we add {user,host,service}-{add,remove}-principal set of commands
for principal manipulation? I really don't want to use
--{add,set,del}-attr unless necessary.
Will {user,host,service}-{show,find} display krbCanonicalName by default
or only with --all option?
4) ad Upgrade:
I think it would be worth to check and document what happens during
upgrade of multiple replicas. There may be confusing behavior when
obtaining tickets. KDC behavior will differ among servers and since
autodiscovery is in use we don't know if we are talking to the old or
new server. I'm not sure what exactly will happen but I suspect it won't
be nice.
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0432, 0450] stageuser-activate: noralize manager value + tests
On 22/03/16 17:23, Martin Basti wrote: On 16.03.2016 09:13, Martin Basti wrote: On 15.03.2016 10:46, David Kupka wrote: On 08/03/16 12:02, Martin Basti wrote: https://fedorahosted.org/freeipa/ticket/5481 Patch attached. Works for me, ACK. Pushed to master: 4871cb5b549042f383ee883e527e773c0abe9d87 Pushed to ipa-4-3: 03743ba1d9191bf0d786116808dba4d7a3522b1f Fix for tests. Patch attached. Works for me, ACK. All-green tests are one step closer, yay! -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0015] use ipaplatform.paths in kdc.conf.template
On 23/03/16 00:30, Timo Aaltonen wrote: https://fedorahosted.org/freeipa/ticket/5343 Thanks for the patch, works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0012-0013] Improve ipaplatform.constants
On 23/03/16 07:17, Timo Aaltonen wrote:
22.03.2016, 21:10, Timo Aaltonen kirjoitti:
18.03.2016, 12:30, Timo Aaltonen kirjoitti:
Fix some hardcoded uid/gid strings to help with porting.
rebased and simplified against current master.
bah, the second patch needs to use constants.{ODS_USER,ODS_GROUP} now.
Hello, thanks for the patches. I've done few minor changes:
- using constants.ODS_{USER,GROUP} in second patch as you've mentioned
- added ticket URL to commit messages for future reference
- rebased the first patch to ipa-4-3 branch
Now it works for me, ACK.
--
David Kupka
--
David Kupka
From f8d4597106c06bec40c8c232671e2b8e7ba55203 Mon Sep 17 00:00:00 2001
From: Timo Aaltonen
Date: Fri, 18 Mar 2016 12:22:33 +0200
Subject: [PATCH 1/3] ipaplatform: Move remaining user/group constants to
ipaplatform.constants.
Use ipaplatform.constants in every corner instead of importing other bits or calling
some platform specific things, and remove most of the remaining hardcoded uid's.
https://fedorahosted.org/freeipa/ticket/5343
---
install/oddjob/com.redhat.idm.trust-fetch-domains | 3 ++-
ipaplatform/base/constants.py | 5 +
ipaplatform/base/services.py | 12 ---
ipaplatform/redhat/services.py| 26 ---
ipaserver/install/bindinstance.py | 2 +-
ipaserver/install/dns.py | 4 ++--
ipaserver/install/dnskeysyncinstance.py | 9
ipaserver/install/dogtaginstance.py | 1 -
ipaserver/install/httpinstance.py | 2 +-
ipaserver/install/odsexporterinstance.py | 5 +++--
ipaserver/install/opendnssecinstance.py | 15 +++--
11 files changed, 27 insertions(+), 57 deletions(-)
diff --git a/install/oddjob/com.redhat.idm.trust-fetch-domains b/install/oddjob/com.redhat.idm.trust-fetch-domains
index ea82e086ef5bade9be3b9f30ae50504c4fcd5db7..4c50c43065b365e7997f222d5e72041dfd32e034 100755
--- a/install/oddjob/com.redhat.idm.trust-fetch-domains
+++ b/install/oddjob/com.redhat.idm.trust-fetch-domains
@@ -8,6 +8,7 @@ from ipapython.dn import DN
from ipalib.config import Env
from ipalib.constants import DEFAULT_CONFIG
from ipapython.ipautil import kinit_keytab
+from ipaplatform.constants import constants
import sys
import os, pwd
@@ -30,7 +31,7 @@ def retrieve_keytab(api, ccache_name, oneway_keytab_name, oneway_principal):
raiseonerr=False)
# Make sure SSSD is able to read the keytab
try:
-sssd = pwd.getpwnam('sssd')
+sssd = pwd.getpwnam(constants.SSSD_USER)
os.chown(oneway_keytab_name, sssd[2], sssd[3])
except KeyError as e:
# If user 'sssd' does not exist, we don't need to chown from root to sssd
diff --git a/ipaplatform/base/constants.py b/ipaplatform/base/constants.py
index 52af12429d090dcc0d7eed14b76e8b651360f283..3e1c4c6f761444bf1e8d527691aa53282e46f17e 100644
--- a/ipaplatform/base/constants.py
+++ b/ipaplatform/base/constants.py
@@ -12,12 +12,17 @@ class BaseConstantsNamespace(object):
DS_GROUP = 'dirsrv'
HTTPD_USER = "apache"
IPA_DNS_PACKAGE_NAME = "freeipa-server-dns"
+KDCPROXY_USER = "kdcproxy"
NAMED_USER = "named"
+NAMED_GROUP = "named"
PKI_USER = 'pkiuser'
PKI_GROUP = 'pkiuser'
# ntpd init variable used for daemon options
NTPD_OPTS_VAR = "OPTIONS"
# quote used for daemon options
NTPD_OPTS_QUOTE = "\""
+ODS_USER = "ods"
+ODS_GROUP = "ods"
# nfsd init variable used to enable kerberized NFS
SECURE_NFS_VAR = "SECURE_NFS"
+SSSD_USER = "sssd"
diff --git a/ipaplatform/base/services.py b/ipaplatform/base/services.py
index 2ec84cdb21607cb51df6ad5fcd2ae515898bee44..9c1b30c0b3c536a58627d6a12f4632dfa4be5c6a 100644
--- a/ipaplatform/base/services.py
+++ b/ipaplatform/base/services.py
@@ -181,18 +181,6 @@ class PlatformService(object):
def get_config_dir(self, instance_name=""):
return
-def get_user_name(self, instance_name=""):
-return
-
-def get_group_name(self, instance_name=""):
-return
-
-def get_binary_path(self):
-return
-
-def get_package_name(self):
-return
-
class SystemdService(PlatformService):
SYSTEMD_SRV_TARGET = "%s.target.wants"
diff --git a/ipaplatform/redhat/services.py b/ipaplatform/redhat/services.py
index ca2a9481ef46b1dc22d898a583ed0fef98e306e1..4774dbf0deb3df50e1a3284353e47b2fb0bebc75 100644
--- a/ipaplatform/redhat/services.py
+++ b/ipaplatform/redhat/services.py
@@ -247,28 +247,6 @@ class RedHatCAService(RedHatService):
self.wait_until_running()
-class RedHatNamedService(RedHatService):
-def get_user_name(self):
-
[Freeipa-devel] [PATCH 0099] Look up HTTPD_USER's UID and GID during installation.
https://fedorahosted.org/freeipa/ticket/5712 -- David Kupka From 00959a382a34bfd77539443cd51b8033ca9c3ee1 Mon Sep 17 00:00:00 2001 From: David Kupka Date: Tue, 22 Mar 2016 09:40:43 +0100 Subject: [PATCH] Look up HTTPD_USER's UID and GID during installation. Those values differ among distributions and there is no guarantee that they're reserved. It's better to look them up based on HTTPD_USER's name. https://fedorahosted.org/freeipa/ticket/5712 --- install/share/custodia.conf.template | 4 ++-- ipaserver/install/custodiainstance.py | 6 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/install/share/custodia.conf.template b/install/share/custodia.conf.template index 688229a50854cd9521b0ae323f30a1c5b729b26f..d9de4d77f90931c089f2179731783430f85ed6f1 100644 --- a/install/share/custodia.conf.template +++ b/install/share/custodia.conf.template @@ -5,8 +5,8 @@ auditlog = $IPA_CUSTODIA_AUDIT_LOG [auth:simple] handler = custodia.httpd.authenticators.SimpleCredsAuth -uid = 48 -gid = 48 +uid = $UID +gid = $GID [auth:header] handler = custodia.httpd.authenticators.SimpleHeaderAuth diff --git a/ipaserver/install/custodiainstance.py b/ipaserver/install/custodiainstance.py index dbe36af6d7af23fa859dcb78f3dc24224fd8fd07..424e0797b682d312c07ebf86a13c27164cae6faf 100644 --- a/ipaserver/install/custodiainstance.py +++ b/ipaserver/install/custodiainstance.py @@ -3,6 +3,7 @@ from ipapython.secrets.kem import IPAKEMKeys from ipapython.secrets.client import CustodiaClient from ipaplatform.paths import paths +from ipaplatform.constants import constants from service import SimpleServiceInstance from ipapython import ipautil from ipapython.ipa_log_manager import root_logger @@ -14,6 +15,7 @@ from jwcrypto.common import json_decode import shutil import os import tempfile +import pwd class CustodiaInstance(SimpleServiceInstance): @@ -30,10 +32,12 @@ class CustodiaInstance(SimpleServiceInstance): def __config_file(self): template_file = os.path.basename(self.config_file) + '.template' template = os.path.join(ipautil.SHARE_DIR, template_file) +httpd_info = pwd.getpwnam(constants.HTTPD_USER) sub_dict = dict(IPA_CUSTODIA_CONF_DIR=paths.IPA_CUSTODIA_CONF_DIR, IPA_CUSTODIA_SOCKET=paths.IPA_CUSTODIA_SOCKET, IPA_CUSTODIA_AUDIT_LOG=paths.IPA_CUSTODIA_AUDIT_LOG, -LDAP_URI=installutils.realm_to_ldapi_uri(self.realm)) +LDAP_URI=installutils.realm_to_ldapi_uri(self.realm), +UID=httpd_info.pw_uid, GID=httpd_info.pw_gid) conf = ipautil.template_file(template, sub_dict) fd = open(self.config_file, "w+") fd.write(conf) -- 2.5.5 -- 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 0432] stageuser-activate: noralize manager value
On 08/03/16 12:02, Martin Basti wrote: https://fedorahosted.org/freeipa/ticket/5481 Patch attached. Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0432] use platform path for SSSD log directory
On 08/03/16 16:29, Martin Basti wrote: /var/log/SSSD is platform specific, thus should be added to ipaplatform module Patch attached. Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 550] certdb: never use the -r option of certutil
On 14/03/16 09:29, Jan Cholasta wrote: Hi, the attached patch fixes <https://fedorahosted.org/freeipa/ticket/5117> and <https://fedorahosted.org/freeipa/ticket/5720>. Honza Hi, thanks for the patch. I haven't found any distortion of affected use cases, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH 0095-0098] NTP: use augeas, configure chronyd, do not overwrite config
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.
--
David Kupka
From 5f9419f9a0e56119fc80f4cf1f7c2ffde15268a0 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Thu, 10 Mar 2016 22:33:15 +0100
Subject: [PATCH 1/4] augeas: add wrapper around python binding
python-augeas binding is mostly 1-1 mapping of C functions. Put a layer of
list- and dict-like classes to allow a slightly more comfortable work.
https://fedorahosted.org/freeipa/ticket/4920
---
freeipa.spec.in | 1 +
ipaplatform/base/paths.py | 1 +
ipapython/ipaaugeas.py| 269 ++
3 files changed, 271 insertions(+)
create mode 100644 ipapython/ipaaugeas.py
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 9e277020d70215e052ab6c905b1c6a29ae6cdd4d..a7cc91cec3aa660bc76b7873723d5f3241cdf1d9 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -171,6 +171,7 @@ Requires: systemd-python
Requires: %{etc_systemd_dir}
Requires: gzip
Requires: oddjob
+Requires: python-augeas > 0.5.0
Provides: %{alt_name}-server = %{version}
Conflicts: %{alt_name}-server
diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index bdff4f3934f3250bdfef3f913631b98d55d759b6..c0200122246e83edab2905ecf6c353b928ce88cf 100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -345,5 +345,6 @@ class BasePathNamespace(object):
IPA_CUSTODIA_SOCKET = '/run/httpd/ipa-custodia.sock'
IPA_CUSTODIA_AUDIT_LOG = '/var/log/ipa-custodia.audit.log'
IPA_GETKEYTAB = '/usr/sbin/ipa-getkeytab'
+IPA_CUSTOM_AUGEAS_LENSES_DIR = '/usr/share/augeas/lenses/ipa/'
path_namespace = BasePathNamespace
diff --git a/ipapython/ipaaugeas.py b/ipapython/ipaaugeas.py
new file mode 100644
index ..1e67ada447467c08e73e55c1576ac33233e4b64f
--- /dev/null
+++ b/ipapython/ipaaugeas.py
@@ -0,0 +1,269 @@
+#!/usr/bin/python
+
+import os
+import shutil
+import tempfile
+import collections
+from augeas import Augeas
+from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import root_logger
+
+
+class _tmp_path(object):
+"""Create a unique temporary path.
+
+Uniquenes depends on tempfile.mkdtemp ability to create unique directory
+path.
+The path is cleared when the object is deleted.
+"""
+def __init__(self, aug, label):
+self._aug = aug
+self._tmp = tempfile.mkdtemp()
+if label:
+self.path = os.path.join(self._tmp, label)
+else:
+self.path = self._tmp
+
+def __del__(self):
+self._aug.remove(self._tmp)
+shutil.rmtree(self._tmp)
+
+
+class aug_obj(object):
+"""python-augeas higher-level wrapper.
+Load single augeas lens and related configuration file.
+Allows working with augeas tree like dict(key:list(dict(...), ...), ...))
+
+Can be used in with statement in the same way as file does.
+"""
+
+_instance = None
+_loaded = False
+
+def __new__(cls, *args, **kwargs):
+if not cls._instance:
+cls._instance = super(aug_obj, cls).__new__(cls, *args, **kwargs)
+return cls._instance
+
+def __init__(self):
+if not hasattr(self, '_aug'):
+self._aug = Augeas(loadpath=paths.IPA_CUSTOM_AUGEAS_LENSES_DIR,
+ flags=Augeas.NO_MODL_AUTOLOAD | Augeas.NO_LOAD)
+self.lenses = []
+self.tree = {}
+
+def add_lens(self, lens, conf_files):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not add more'
+ 'lenses.')
+if lens in self.lenses:
+raise RuntimeError('Lens %s already added.' % lens)
+self.lenses.append(lens)
+load_path = '/augeas/load/{0}'.format(lens)
+
+self._aug.set(os.path.join(load_path, 'lens'), lens)
+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:]))
+
+def load(self):
+if self._loaded:
+raise RuntimeError('Augeas lenses was loaded. Could not add more'
+ 'lenses.')
+self._aug.load()
+errors = self._aug.match(os.path.join('//error'))
+if errors:
+err_msg = ""
+for error in errors:
+err_msg += ("{}: {}".format(e
Re: [Freeipa-devel] [PATCH 0430] remove unused argument from function update_ssh_keys
This patch is going to master branch only. Works for me, ACK. - Original Message - From: "Martin Basti" To: "freeipa-devel" Sent: Wednesday, March 2, 2016 5:33:18 PM Subject: [Freeipa-devel] [PATCH 0430] remove unused argument from function update_ssh_keys Patch attached, see commit message for details -- 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 0425] pylint: suppress false positive no-member errors
Tested with pylint-1.5.4-2, works for me, ACK. - Original Message - From: "Martin Basti" To: "freeipa-devel" Sent: Tuesday, March 1, 2016 5:55:54 PM Subject: Re: [Freeipa-devel] [PATCH 0425] pylint: suppress false positive no-member errors On 25.02.2016 17:50, Martin Basti wrote: On 25.02.2016 15:48, Martin Basti wrote: The last pylint 1.5 patch, \o/ https://fedorahosted.org/freeipa/ticket/5615 self-NACK too broad disables Updated patches attached. -- 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
[Freeipa-devel] [PATCH] man: Decribe ipa-client-install workaround for broken D-Bus enviroment.
https://fedorahosted.org/freeipa/ticket/5694From 0a7afc3042835935e2891032664afcead1f4bcea Mon Sep 17 00:00:00 2001 From: David Kupka Date: Wed, 2 Mar 2016 11:08:19 +0100 Subject: [PATCH] man: Decribe ipa-client-install workaround for broken D-Bus enviroment. https://fedorahosted.org/freeipa/ticket/5694 --- client/man/ipa-client-install.1 | 10 ++ 1 file changed, 10 insertions(+) diff --git a/client/man/ipa-client-install.1 b/client/man/ipa-client-install.1 index 92ea77a4bda539f8614f3d47cac7b53faf57482c..ce5258b3eac08b9a04bf5f4142635d2e21310f32 100644 --- a/client/man/ipa-client-install.1 +++ b/client/man/ipa-client-install.1 @@ -176,6 +176,16 @@ valid for the IPA domain. .TP \fB\-\-request\-cert\fR Request certificate for the machine. The certificate will be stored in /etc/ipa/nssdb under the nickname "Local IPA host". + +Using this option requires that D-Bus is properly configured or not configured +at all. In enviroment where this condition is not met (e.g. anaconda kickstart +chroot environment) set the system bus address to /dev/null to enable +workaround in ipa-client-install. + +# env DBUS_SYSTEM_BUS_ADDRESS=unix:path=/dev/null ipa-client-install --request-cert + +Note that the certmonger service requires a system reboot to start monitoring +the certificate obtained in this way. .TP \fB\-\-automount\-location\fR=\fILOCATION\fR Configure automount by running ipa\-client\-automount(1) with \fILOCATION\fR as -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0029] Move user/group constants for PKI and DS into ipaplatform
Hello Christian,
sorry for letting this patch rot for so long. I've forget about it the minute
Fraser replied.
To compensate a little I've fixed pep8 error, rebased it and attaching two
versions for master and for 4.3 branch.
I haven't found any missing cases and it works for me. If you're OK with the
modified patches it can be pushed.
David
- Original Message -
From: "Christian Heimes"
To: "Fraser Tweedale"
Cc: "freeipa-devel"
Sent: Wednesday, January 20, 2016 11:57:42 AM
Subject: Re: [Freeipa-devel] [PATCH 0029] Move user/group constants for PKI and
DS into ipaplatform
On 2016-01-20 02:54, Fraser Tweedale wrote:
> On Tue, Jan 19, 2016 at 02:20:27PM +0100, Christian Heimes wrote:
>> ipaplatform.constants has platform specific names for a couple of system
>> users like Apache HTTPD. The user names for PKI_USER, PKI_GROUP, DS_USER
>> and DS_GROUP are defined in other modules. Similar to #5587 the patch my
>> patch moves the constants into the platform module.
>>
>> https://fedorahosted.org/freeipa/ticket/5619
>
> I see a few remaining cases:
>
> ipaserver/install/dsinstance.py
> 712:pent = pwd.getpwnam("dirsrv")
>
> ipatests/test_integration/test_backup_and_restore.py
> 167:self.master.run_command(['userdel', 'dirsrv'])
> 168:self.master.run_command(['userdel', 'pkiuser'])
>
> ipaplatform/redhat/tasks.py
> 441:if name == 'pkiuser':
>
> When these are included, ACK.
Good catch!
My new patch takes care of remaining cases.
--
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/CodeFrom e5801f7a7b051ea1bd5ae3460e4011e871528126 Mon Sep 17 00:00:00 2001
From: Christian Heimes
Date: Tue, 19 Jan 2016 14:18:30 +0100
Subject: [PATCH] Move user/group constants for PKI and DS into ipaplatform
https://fedorahosted.org/freeipa/ticket/5619
---
install/share/copy-schema-to-ca.py | 8
ipaplatform/base/constants.py| 4
ipaplatform/redhat/tasks.py | 5 +++--
ipaserver/install/cainstance.py | 16
ipaserver/install/dogtaginstance.py | 5 +++--
ipaserver/install/dsinstance.py | 7 ---
ipaserver/install/ipa_backup.py | 4 ++--
ipaserver/install/ipa_restore.py | 16 +---
ipaserver/install/krainstance.py | 9 +
ipaserver/install/krbinstance.py | 4 ++--
ipaserver/install/server/upgrade.py | 3 ++-
ipatests/test_integration/test_backup_and_restore.py | 5 +++--
12 files changed, 49 insertions(+), 37 deletions(-)
diff --git a/install/share/copy-schema-to-ca.py b/install/share/copy-schema-to-ca.py
index ac49fcd59f179b9fc7b2d5ef5b6b7b91a7da892e..424670605e470d60f42f0dbc89177d83b2e968f6 100755
--- a/install/share/copy-schema-to-ca.py
+++ b/install/share/copy-schema-to-ca.py
@@ -19,9 +19,9 @@ from hashlib import sha1
from ipapython import ipautil, dogtag
from ipapython.ipa_log_manager import root_logger, standard_logging_setup
-from ipaserver.install.dsinstance import DS_USER, schema_dirname
-from ipaserver.install.cainstance import PKI_USER
+from ipaserver.install.dsinstance import schema_dirname
from ipalib import api
+from ipaplatform.constants import constants
try:
from ipaplatform import services
@@ -52,8 +52,8 @@ def _sha1_file(filename):
def add_ca_schema():
"""Copy IPA schema files into the CA DS instance
"""
-pki_pent = pwd.getpwnam(PKI_USER)
-ds_pent = pwd.getpwnam(DS_USER)
+pki_pent = pwd.getpwnam(constants.PKI_USER)
+ds_pent = pwd.getpwnam(constants.DS_USER)
for schema_fname in SCHEMA_FILENAMES:
source_fname = os.path.join(ipautil.SHARE_DIR, schema_fname)
target_fname = os.path.join(schema_dirname(SERVERID), schema_fname)
diff --git a/ipaplatform/base/constants.py b/ipaplatform/base/constants.py
index 50f8a3ed140aca0f6573231f2a7e5b20e2169919..52af12429d090dcc0d7eed14b76e8b651360f283 100644
--- a/ipaplatform/base/constants.py
+++ b/ipaplatform/base/constants.py
@@ -8,9 +8,13 @@ This base platform module exports platform dependant constants.
class BaseConstantsNamespace(object):
+DS_USER = 'dirsrv'
+DS_GROUP = 'dirsrv'
HTTPD_USER = "apache"
IPA_DNS_PACKAGE_NAME = "freeipa-server-dns"
NAMED_USER = "named"
+PKI_USER = 'pkiuser'
+PKI_GROUP = 'pkiuser'
# ntpd init variable used for daemon options
NTPD_OPTS_VAR = "OPTIONS"
# quote used for daemon options
diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py
index 6380486792bf62e3a7e607aba8658b0c519f67f8..7c29b51e1eb354f03acda815e89e552eea004a17 100644
--- a/ipaplatform/redhat/tasks.py
+++ b/ipaplatform/redhat/tasks.py
@@ -45,6 +45,7 @@ import ipapython.errors
from ipalib imp
Re: [Freeipa-devel] [PATCH 0011] Move freeipa certmonger helpers to libexecdir.
On 24/02/16 15:07, Rob Crittenden wrote: David Kupka wrote: On 23/02/16 16:41, Rob Crittenden wrote: David Kupka wrote: On 23/02/16 10:14, Martin Kosek wrote: On 02/23/2016 09:47 AM, David Kupka wrote: On 22/02/16 16:15, Martin Kosek wrote: On 02/22/2016 04:04 PM, Jan Cholasta wrote: On 22.2.2016 15:56, David Kupka wrote: On 22/02/16 07:28, Jan Cholasta wrote: On 18.2.2016 10:10, David Kupka wrote: On 19/01/16 16:10, David Kupka wrote: On 19/01/16 14:38, Jan Cholasta wrote: On 19.1.2016 14:26, Martin Kosek wrote: On 01/19/2016 01:47 PM, David Kupka wrote: I've polished the patch attached to #5586 by Timo Aaltonen. Thanks for the patch. I've fixed the path in specfile and removed unused import but otherwise it works, ACK. https://fedorahosted.org/freeipa/ticket/5586 Won't this break existing certmonger requests depending on the old path? It will, I don't see any upgrade code. # getcert list | grep '/usr/lib64/ipa/certmonger' pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "auditSigningCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "ocspSigningCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "subsystemCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "caSigningCert cert-pki-ca" post-save command: /usr/lib64/ipa/certmonger/renew_ra_cert pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "Server-Cert cert-pki-ca" post-save command: /usr/lib64/ipa/certmonger/restart_dirsrv RHEL72 post-save command: /usr/lib64/ipa/certmonger/restart_httpd You're right it will break the upgrade. I haven't noticed that Server-Cert for DS and HTTPD are not handled by certificate_renewal_update (ipaserver.install.server.upgrade) where all the other trackings are stopped and then configured again with the paths.CERTMONGER_COMMAND_TEMPLATE already updated. Thanks for the catch. I've updated Timo's patch little more and added start_tracking_certificates() for dsinstance and httpinstance. Now the upgrade works as expected. The way the patches are split is kind of weird and apparently confusing (see the other thread). IMO there should be 2 patches: the first should add the ability to change DS and HTTP certmonger config during upgrade (i.e. the start_tracking_certificates() methods and certificate_renewal_update() changes), the second should move the helpers (i.e. the actual move and certificate_renewal_update() version bump). Honza, do I understand it correctly that the code is OK but I did not split it to the patches correctly? Yes. Before acking or pushing, can you please explain for me how the upgrade of certmonger tracking requests work? I want to make sure this is right, so please bear with me: 1) How does it edit existing tracking requests with the new helper paths? 2) Does it go and try to edit the requests on every upgrade? Or is there some check that requests were updated? Thanks, Martin Whole upgrade of renewal requests is done in ipaserver/install/server/upgrade.py in certificate_renewal_upgrade(). First there is version of requests and if it's the same as in state file upgrade is skipped. Then every request is searched over certmonger's DBus interface and if at least one is not found it means that there was change in request configuration. All tracking requests are then stopped and started again with new configuration. So to answer you questions: 1) By stopping the old request with the old parameters (including path) and starting new with new parameters. 2) Only if version was bumped which happens only if some of the requests changes. Ah, so IIUC, if you bump the version, requests should be properly updated. The change looks fine then. After discussion with Honza, we decided to drop the part comparing only base names of pre- and post-save commands and use it as whole. I've also split the patches so it's obvious what is going on. Patches should be applied in this order: freeipa-dkupka-0091.0 A cert could silently fail to be tracked in start_tracking_certificates() if no serverid can be found. In that case it also wouldn't be stopped. The behavior is the same as in existing stop_tracking_certificates(). Should we rather raise and stop the upgrade? I guess not but warning would be probably useful. What solution would you prefer, Rob? I don't know all the callers of this. It may be perfectly safe to assume that a serverid is always there, but the i
Re: [Freeipa-devel] [PATCH 0423] fix duplicated except
On 25/02/16 11:40, Jan Cholasta wrote: On 25.2.2016 11:25, David Kupka wrote: On 24/02/16 17:56, Martin Basti wrote: During my playing with pylint, I fixed this issue which allows us to enable additional check in pylint (the nice one). Patch attached, it should go only to master. Works for me, ACK. I always wonder how something like this can even get to the sources. Fortunately the added check will prevent that in the future. Before this is pushed, could you please check git history to verify that these duplicate excepts are not symptomps of some actual problems? Archaeology hat on. The duplicate except statement in ipalib/plugins/automount.py was introduced by commit 0197ebbb and was there since 2010. All the time the second except was logically dead code. The duplicate except statement in ipalib/backend.py was introduced by commit 01da4a8d converting StandardError to Exception. But the only difference is the message in raise error. I should be save to push this. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0424] Pylint: add missing attributes of exceptions to definition in pylint plugin
On 24/02/16 18:54, Martin Basti wrote: Pylint is not able to handle IPA errors objects, because attributes are added into objects dynamically, and pylint 1.5 reports them as no-member errors. https://fedorahosted.org/freeipa/ticket/5615 Patch attached. It would be better to define all errors with all unrecoverable members but this is sufficient start. More can be add when it's needed. Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0423] fix duplicated except
On 24/02/16 17:56, Martin Basti wrote: During my playing with pylint, I fixed this issue which allows us to enable additional check in pylint (the nice one). Patch attached, it should go only to master. Works for me, ACK. I always wonder how something like this can even get to the sources. Fortunately the added check will prevent that in the future. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] IPA client realm/domain autodiscovery improvements
to the ipa-client package. And as long as you only use the Discover method realmd would not try to call ipa-client-install, so there is no logical circle either. To avoid an un-needed second discovery when ipa-client-install is not run from the command line but from realmd directly ipa-client-install can skip the realmd call if domain and realm are already given on the command line because realmd will set both options when calling ipa-client-install. And if there is a bug you have to poke the maintainer of the library or realmd either way. bye, Sumit -- 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 -- Martin^3 Babinsky -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 546] client: stop using /etc/pki/nssdb
On 22/02/16 16:06, Jan Cholasta wrote: Hi, the attached patch fixes <https://fedorahosted.org/freeipa/ticket/5592>. Honza Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0084-0086] CI: Add double circle topology
On 24/02/16 08:27, David Kupka wrote:
On 23/02/16 17:54, Martin Basti wrote:
On 23.02.2016 17:33, Martin Basti wrote:
On 23.02.2016 17:30, Martin Basti wrote:
On 18.02.2016 10:14, David Kupka wrote:
On 12/02/16 16:52, Martin Basti wrote:
On 12.02.2016 13:03, Milan Kubík wrote:
On 02/12/2016 10:59 AM, David Kupka wrote:
Sending one more topology test. This one creates a M groups
consisting N (N>=2) servers.
First two servers in each group are used to connect with nearest
four
groups and also with the other servers inside the group (when N>2).
Servers inside the group (not connecting to other groups) are
connected with each other.
The patch set needs freeipa-dkupka-8{1,2,3} applied.
ACK
I cannot apply patches, please rebase
[mbasti@dhcp129-96 freeipa-devel]$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
[mbasti@dhcp129-96 freeipa-devel]$ git am freeipa-dkupka-008* -3
Applying: CI: Add '2-connected' topology generator.
Applying: CI: Add simple replication test in 2-connected topology.
Applying: CI: Add test for 2-connected topology generator.
Applying: CI: Add double circle topology.
Applying: CI: Add replication test utilizing double-circle topology.
Applying: CI: Add test for double-circle topology generator.
error: invalid object 100644 e12d141391840cc7f9150a178875393a96dd469b
for 'ipatests/test_integration/test_topologies.py'
fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0006 CI: Add test for double-circle topology
generator.
The copy of the patch that failed is found in:
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am
--abort".
Martin^2
Git fails to apply patches because wrong version of
freeipa-dkupka-008{1,2,3} was pushed. Attached patches should fix it.
Sorry my bad.
ACK
Pushed to:
ipa-4-3: ffe3731ae7813fcc3246a83e37d62fc2754bb4ca
master: acdabba6ec0f68841f02c1e6ad65232de81bb505
New test:
Pushed to:
master: a1e582b33c42bcc8a708777afb975e7dc571ee3d
ipa-4-3: 2efa60637111e40a9ac9459d507d9f55a2ae301a
Revert?
IMO this will not work on python3
* Module ipatests.test_integration.test_topologies
ipatests/test_integration/test_topologies.py:124:
[W1638(range-builtin-not-iterating), test_topology_double_circle_topo]
range built-in referenced when not iterating)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:949: [W0110(deprecated-lambda),
double_circle_topo] map/filter on lambda could be replaced by
comprehension)
ipatests/test_integration/tasks.py:949:
[W1636(map-builtin-not-iterating), double_circle_topo] map built-in
referenced when not iterating)
ipatests/test_integration/tasks.py:949:
[W1637(zip-builtin-not-iterating), double_circle_topo] zip built-in
referenced when not iterating)
You can revert it and I will send fixed patches or you can just apply
attached patch.
Updated patch attached.
--
David Kupka
From 1e12eb7d207e5980fb3653d5818968b129f4a1c9 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Wed, 24 Feb 2016 08:15:51 +0100
Subject: [PATCH] CI: Make double circle topology python3 compatible
---
ipatests/test_integration/tasks.py | 2 +-
ipatests/test_integration/test_topologies.py | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 9d9a78bb10b463016c646aa921dde722e882da93..60e9e82391077ce6b997d0ed4ad4f2ff19f43d1e 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -946,7 +946,7 @@ def double_circle_topo(master, replicas, site_size=6):
# split servers into sites
it = [iter(servers)] * site_size
-sites = map(lambda x: (x[0], x[1], x[2:]), zip(*it))
+sites = [(x[0], x[1], x[2:]) for x in zip(*it)]
num_sites = len(sites)
for i in range(num_sites):
diff --git a/ipatests/test_integration/test_topologies.py b/ipatests/test_integration/test_topologies.py
index a0a1b9d6222779a9ce67b2fa8a29052747eae8f9..4618b44fe3b6cd69149a5fb55b2b392c5383a5c2 100644
--- a/ipatests/test_integration/test_topologies.py
+++ b/ipatests/test_integration/test_topologies.py
@@ -121,7 +121,7 @@ def test_topology_two_connected():
def test_topology_double_circle_topo():
topo = tasks.get_topo('double-circle')
assert topo == tasks.double_circle_topo
-assert list(topo('M', range(1, 30))) == [
+assert list(topo('M', list(range(1, 30 == [
('M', 1),
(1, 6),
(1, 12),
--
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 0084-0086] CI: Add double circle topology
On 23/02/16 17:54, Martin Basti wrote:
On 23.02.2016 17:33, Martin Basti wrote:
On 23.02.2016 17:30, Martin Basti wrote:
On 18.02.2016 10:14, David Kupka wrote:
On 12/02/16 16:52, Martin Basti wrote:
On 12.02.2016 13:03, Milan Kubík wrote:
On 02/12/2016 10:59 AM, David Kupka wrote:
Sending one more topology test. This one creates a M groups
consisting N (N>=2) servers.
First two servers in each group are used to connect with nearest
four
groups and also with the other servers inside the group (when N>2).
Servers inside the group (not connecting to other groups) are
connected with each other.
The patch set needs freeipa-dkupka-8{1,2,3} applied.
ACK
I cannot apply patches, please rebase
[mbasti@dhcp129-96 freeipa-devel]$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
[mbasti@dhcp129-96 freeipa-devel]$ git am freeipa-dkupka-008* -3
Applying: CI: Add '2-connected' topology generator.
Applying: CI: Add simple replication test in 2-connected topology.
Applying: CI: Add test for 2-connected topology generator.
Applying: CI: Add double circle topology.
Applying: CI: Add replication test utilizing double-circle topology.
Applying: CI: Add test for double-circle topology generator.
error: invalid object 100644 e12d141391840cc7f9150a178875393a96dd469b
for 'ipatests/test_integration/test_topologies.py'
fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0006 CI: Add test for double-circle topology
generator.
The copy of the patch that failed is found in:
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am
--abort".
Martin^2
Git fails to apply patches because wrong version of
freeipa-dkupka-008{1,2,3} was pushed. Attached patches should fix it.
Sorry my bad.
ACK
Pushed to:
ipa-4-3: ffe3731ae7813fcc3246a83e37d62fc2754bb4ca
master: acdabba6ec0f68841f02c1e6ad65232de81bb505
New test:
Pushed to:
master: a1e582b33c42bcc8a708777afb975e7dc571ee3d
ipa-4-3: 2efa60637111e40a9ac9459d507d9f55a2ae301a
Revert?
IMO this will not work on python3
* Module ipatests.test_integration.test_topologies
ipatests/test_integration/test_topologies.py:124:
[W1638(range-builtin-not-iterating), test_topology_double_circle_topo]
range built-in referenced when not iterating)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:949: [W0110(deprecated-lambda),
double_circle_topo] map/filter on lambda could be replaced by
comprehension)
ipatests/test_integration/tasks.py:949:
[W1636(map-builtin-not-iterating), double_circle_topo] map built-in
referenced when not iterating)
ipatests/test_integration/tasks.py:949:
[W1637(zip-builtin-not-iterating), double_circle_topo] zip built-in
referenced when not iterating)
You can revert it and I will send fixed patches or you can just apply
attached patch.
--
David Kupka
From a46ba01172e666d8afdac5e0605f32110138ea39 Mon Sep 17 00:00:00 2001
From: David Kupka
Date: Wed, 24 Feb 2016 08:15:51 +0100
Subject: [PATCH] CI: Make double circle topology python3 compatible
---
ipatests/test_integration/tasks.py | 2 +-
ipatests/test_integration/test_topologies.py | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 9d9a78bb10b463016c646aa921dde722e882da93..60e9e82391077ce6b997d0ed4ad4f2ff19f43d1e 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -946,7 +946,7 @@ def double_circle_topo(master, replicas, site_size=6):
# split servers into sites
it = [iter(servers)] * site_size
-sites = map(lambda x: (x[0], x[1], x[2:]), zip(*it))
+sites = [(x[0], x[1], x[2:]) for x in zip(*it)]
num_sites = len(sites)
for i in range(num_sites):
diff --git a/ipatests/test_integration/test_topologies.py b/ipatests/test_integration/test_topologies.py
index a0a1b9d6222779a9ce67b2fa8a29052747eae8f9..4bdff44a162e188c0549b6ecca12ae658839e5cd 100644
--- a/ipatests/test_integration/test_topologies.py
+++ b/ipatests/test_integration/test_topologies.py
@@ -121,7 +121,7 @@ def test_topology_two_connected():
def test_topology_double_circle_topo():
topo = tasks.get_topo('double-circle')
assert topo == tasks.double_circle_topo
-assert list(topo('M', range(1, 30))) == [
+assert list(topo('M', [r for r in range(1, 30)])) == [
('M', 1),
(1, 6),
(1, 12),
--
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 0011] Move freeipa certmonger helpers to libexecdir.
On 23/02/16 16:41, Rob Crittenden wrote: David Kupka wrote: On 23/02/16 10:14, Martin Kosek wrote: On 02/23/2016 09:47 AM, David Kupka wrote: On 22/02/16 16:15, Martin Kosek wrote: On 02/22/2016 04:04 PM, Jan Cholasta wrote: On 22.2.2016 15:56, David Kupka wrote: On 22/02/16 07:28, Jan Cholasta wrote: On 18.2.2016 10:10, David Kupka wrote: On 19/01/16 16:10, David Kupka wrote: On 19/01/16 14:38, Jan Cholasta wrote: On 19.1.2016 14:26, Martin Kosek wrote: On 01/19/2016 01:47 PM, David Kupka wrote: I've polished the patch attached to #5586 by Timo Aaltonen. Thanks for the patch. I've fixed the path in specfile and removed unused import but otherwise it works, ACK. https://fedorahosted.org/freeipa/ticket/5586 Won't this break existing certmonger requests depending on the old path? It will, I don't see any upgrade code. # getcert list | grep '/usr/lib64/ipa/certmonger' pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "auditSigningCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "ocspSigningCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "subsystemCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "caSigningCert cert-pki-ca" post-save command: /usr/lib64/ipa/certmonger/renew_ra_cert pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "Server-Cert cert-pki-ca" post-save command: /usr/lib64/ipa/certmonger/restart_dirsrv RHEL72 post-save command: /usr/lib64/ipa/certmonger/restart_httpd You're right it will break the upgrade. I haven't noticed that Server-Cert for DS and HTTPD are not handled by certificate_renewal_update (ipaserver.install.server.upgrade) where all the other trackings are stopped and then configured again with the paths.CERTMONGER_COMMAND_TEMPLATE already updated. Thanks for the catch. I've updated Timo's patch little more and added start_tracking_certificates() for dsinstance and httpinstance. Now the upgrade works as expected. The way the patches are split is kind of weird and apparently confusing (see the other thread). IMO there should be 2 patches: the first should add the ability to change DS and HTTP certmonger config during upgrade (i.e. the start_tracking_certificates() methods and certificate_renewal_update() changes), the second should move the helpers (i.e. the actual move and certificate_renewal_update() version bump). Honza, do I understand it correctly that the code is OK but I did not split it to the patches correctly? Yes. Before acking or pushing, can you please explain for me how the upgrade of certmonger tracking requests work? I want to make sure this is right, so please bear with me: 1) How does it edit existing tracking requests with the new helper paths? 2) Does it go and try to edit the requests on every upgrade? Or is there some check that requests were updated? Thanks, Martin Whole upgrade of renewal requests is done in ipaserver/install/server/upgrade.py in certificate_renewal_upgrade(). First there is version of requests and if it's the same as in state file upgrade is skipped. Then every request is searched over certmonger's DBus interface and if at least one is not found it means that there was change in request configuration. All tracking requests are then stopped and started again with new configuration. So to answer you questions: 1) By stopping the old request with the old parameters (including path) and starting new with new parameters. 2) Only if version was bumped which happens only if some of the requests changes. Ah, so IIUC, if you bump the version, requests should be properly updated. The change looks fine then. After discussion with Honza, we decided to drop the part comparing only base names of pre- and post-save commands and use it as whole. I've also split the patches so it's obvious what is going on. Patches should be applied in this order: freeipa-dkupka-0091.0 A cert could silently fail to be tracked in start_tracking_certificates() if no serverid can be found. In that case it also wouldn't be stopped. The behavior is the same as in existing stop_tracking_certificates(). Should we rather raise and stop the upgrade? I guess not but warning would be probably useful. What solution would you prefer, Rob? freeipa-dkupka-0087.1 freeipa-dkupka-0088.1 freeipa-tjaalton-0011.2 freeipa-dkupka-0092.0 -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0011] Move freeipa certmonger helpers to libexecdir.
On 23/02/16 10:14, Martin Kosek wrote: On 02/23/2016 09:47 AM, David Kupka wrote: On 22/02/16 16:15, Martin Kosek wrote: On 02/22/2016 04:04 PM, Jan Cholasta wrote: On 22.2.2016 15:56, David Kupka wrote: On 22/02/16 07:28, Jan Cholasta wrote: On 18.2.2016 10:10, David Kupka wrote: On 19/01/16 16:10, David Kupka wrote: On 19/01/16 14:38, Jan Cholasta wrote: On 19.1.2016 14:26, Martin Kosek wrote: On 01/19/2016 01:47 PM, David Kupka wrote: I've polished the patch attached to #5586 by Timo Aaltonen. Thanks for the patch. I've fixed the path in specfile and removed unused import but otherwise it works, ACK. https://fedorahosted.org/freeipa/ticket/5586 Won't this break existing certmonger requests depending on the old path? It will, I don't see any upgrade code. # getcert list | grep '/usr/lib64/ipa/certmonger' pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "auditSigningCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "ocspSigningCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "subsystemCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "caSigningCert cert-pki-ca" post-save command: /usr/lib64/ipa/certmonger/renew_ra_cert pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "Server-Cert cert-pki-ca" post-save command: /usr/lib64/ipa/certmonger/restart_dirsrv RHEL72 post-save command: /usr/lib64/ipa/certmonger/restart_httpd You're right it will break the upgrade. I haven't noticed that Server-Cert for DS and HTTPD are not handled by certificate_renewal_update (ipaserver.install.server.upgrade) where all the other trackings are stopped and then configured again with the paths.CERTMONGER_COMMAND_TEMPLATE already updated. Thanks for the catch. I've updated Timo's patch little more and added start_tracking_certificates() for dsinstance and httpinstance. Now the upgrade works as expected. The way the patches are split is kind of weird and apparently confusing (see the other thread). IMO there should be 2 patches: the first should add the ability to change DS and HTTP certmonger config during upgrade (i.e. the start_tracking_certificates() methods and certificate_renewal_update() changes), the second should move the helpers (i.e. the actual move and certificate_renewal_update() version bump). Honza, do I understand it correctly that the code is OK but I did not split it to the patches correctly? Yes. Before acking or pushing, can you please explain for me how the upgrade of certmonger tracking requests work? I want to make sure this is right, so please bear with me: 1) How does it edit existing tracking requests with the new helper paths? 2) Does it go and try to edit the requests on every upgrade? Or is there some check that requests were updated? Thanks, Martin Whole upgrade of renewal requests is done in ipaserver/install/server/upgrade.py in certificate_renewal_upgrade(). First there is version of requests and if it's the same as in state file upgrade is skipped. Then every request is searched over certmonger's DBus interface and if at least one is not found it means that there was change in request configuration. All tracking requests are then stopped and started again with new configuration. So to answer you questions: 1) By stopping the old request with the old parameters (including path) and starting new with new parameters. 2) Only if version was bumped which happens only if some of the requests changes. Ah, so IIUC, if you bump the version, requests should be properly updated. The change looks fine then. After discussion with Honza, we decided to drop the part comparing only base names of pre- and post-save commands and use it as whole. I've also split the patches so it's obvious what is going on. Patches should be applied in this order: freeipa-dkupka-0091.0 freeipa-dkupka-0087.1 freeipa-dkupka-0088.1 freeipa-tjaalton-0011.2 freeipa-dkupka-0092.0 -- David Kupka From 3e43c00c9d90752c28b5e81ddb7827ba00f12eba Mon Sep 17 00:00:00 2001 From: David Kupka Date: Wed, 17 Feb 2016 15:18:04 +0100 Subject: [PATCH 2/5] dsinstance: add start_tracking_certificates method Configure certmonger to start tracing certificate for DS. https://fedorahosted.org/freeipa/ticket/5586 --- ipaserver/install/dsinstance.py | 10 ++ ipaserver/install/server/upgrade.py | 19 +-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/ipaserver/install/dsinstance.py b/
Re: [Freeipa-devel] [PATCH 0011] Move freeipa certmonger helpers to libexecdir.
On 22/02/16 16:15, Martin Kosek wrote: On 02/22/2016 04:04 PM, Jan Cholasta wrote: On 22.2.2016 15:56, David Kupka wrote: On 22/02/16 07:28, Jan Cholasta wrote: On 18.2.2016 10:10, David Kupka wrote: On 19/01/16 16:10, David Kupka wrote: On 19/01/16 14:38, Jan Cholasta wrote: On 19.1.2016 14:26, Martin Kosek wrote: On 01/19/2016 01:47 PM, David Kupka wrote: I've polished the patch attached to #5586 by Timo Aaltonen. Thanks for the patch. I've fixed the path in specfile and removed unused import but otherwise it works, ACK. https://fedorahosted.org/freeipa/ticket/5586 Won't this break existing certmonger requests depending on the old path? It will, I don't see any upgrade code. # getcert list | grep '/usr/lib64/ipa/certmonger' pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "auditSigningCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "ocspSigningCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "subsystemCert cert-pki-ca" pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "caSigningCert cert-pki-ca" post-save command: /usr/lib64/ipa/certmonger/renew_ra_cert pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert "Server-Cert cert-pki-ca" post-save command: /usr/lib64/ipa/certmonger/restart_dirsrv RHEL72 post-save command: /usr/lib64/ipa/certmonger/restart_httpd You're right it will break the upgrade. I haven't noticed that Server-Cert for DS and HTTPD are not handled by certificate_renewal_update (ipaserver.install.server.upgrade) where all the other trackings are stopped and then configured again with the paths.CERTMONGER_COMMAND_TEMPLATE already updated. Thanks for the catch. I've updated Timo's patch little more and added start_tracking_certificates() for dsinstance and httpinstance. Now the upgrade works as expected. The way the patches are split is kind of weird and apparently confusing (see the other thread). IMO there should be 2 patches: the first should add the ability to change DS and HTTP certmonger config during upgrade (i.e. the start_tracking_certificates() methods and certificate_renewal_update() changes), the second should move the helpers (i.e. the actual move and certificate_renewal_update() version bump). Honza, do I understand it correctly that the code is OK but I did not split it to the patches correctly? Yes. Before acking or pushing, can you please explain for me how the upgrade of certmonger tracking requests work? I want to make sure this is right, so please bear with me: 1) How does it edit existing tracking requests with the new helper paths? 2) Does it go and try to edit the requests on every upgrade? Or is there some check that requests were updated? Thanks, Martin Whole upgrade of renewal requests is done in ipaserver/install/server/upgrade.py in certificate_renewal_upgrade(). First there is version of requests and if it's the same as in state file upgrade is skipped. Then every request is searched over certmonger's DBus interface and if at least one is not found it means that there was change in request configuration. All tracking requests are then stopped and started again with new configuration. So to answer you questions: 1) By stopping the old request with the old parameters (including path) and starting new with new parameters. 2) Only if version was bumped which happens only if some of the requests changes. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
