Re: [Freeipa-devel] Please review: V4/AD user short names design draft

2017-03-08 Thread Jan Cholasta

On 8.3.2017 10:30, Martin Babinsky wrote:

On Tue, Feb 28, 2017 at 01:29:50PM +0100, 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.

[1] https://www.freeipa.org/page/V4/AD_User_Short_Names
[2] https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

--
Martin^3 Babinsky

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


I have updated the design page[1] and incorporated most of the comments from all
reviewers. The most dramatic change is that I have expanded the discussion by
the possibility for overriding global domain resolution order by ID
view-specific settings. I have also expanded How-To section accordingly.

Please try to review and comment during today as the window for development is
quickly closing.


LGTM.



[1] http://www.freeipa.org/page/V4/AD_User_Short_Names




--
Jan Cholasta

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


Re: [Freeipa-devel] Please review: V4/AD user short names design draft

2017-03-07 Thread Jan Cholasta

On 7.3.2017 15:14, Simo Sorce wrote:

On Tue, 2017-03-07 at 09:38 +0100, Martin Babinsky wrote:

On 03/06/2017 01:48 PM, Simo Sorce wrote:

On Mon, 2017-03-06 at 07:47 +0100, Martin Babinsky wrote:

On 03/02/2017 02:54 PM, Simo Sorce wrote:

On Thu, 2017-03-02 at 08:10 +0100, Martin Babinsky wrote:

In this case it would probably be a good idea to think about "forward
compatibility" and define a new AUX objectclass bringing in
'ipaDomainResolutionOrder' instead of extending two separate
objectclasses. In this way we may the just extend whathever object we
desire to carry the override in an easy and clean way.


I agree.
Simo.



Now the most difficult question remains... How to name this objectclass.
I personally am out of ideas but will try my best to come up with
something meaningful.


Try to describe what the option ultimately does with as few words as
possible.

Simo.




I was thinking about this and since we are performing name qualification
(short-name -> fully-qualified name incl. domain/realm part), I would
like to propose the following naming schema:

objectlasses: ( OID_TBD NAME ipaNameQualificationData Desc 'data used
for short name qualification data' SUP top AUXILIARY MAY
(ipaNameQualificationDomainList) X-ORIGIN 'IPA 4.5' )

attributeTypes: ( OID_TBD NAME 'ipaNameQualificationDomainList' DESC
'List of domains used to qualify user short name' EQUALITY
caseIgnoreIA5Match SINGLE-VALUE SYNTAX 1.3.6.1.4.1.1466.115.121.1.26
X-ORIGIN 'IPA v4.5' )

Let me know if you are ok with this or am I overengineering the names?

I would like to solve this quickly so that I can finish the design and
start implementation.


I was thinking that we can use acronyms here to make it less of a
mouthful and also more easily recognizable:
My idea is:
- ipaNameQualificationData -> ipaFQDNPolicies
- ipaNameQualificationDomainList -> ipaFQDNCheckOrder


TBH I liked ipaDomainResolutionOrder the best, both 
ipaNameQualificationDomainList and ipaFQDNCheckOrder sound 
overengineered to me :-)


If ipaDomainResolutionOrder is not good enough, we could draw some 
inspiration from resolv.conf and use e.g. ipaDomainSearchList.


--
Jan Cholasta

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


Re: [Freeipa-devel] Please review: V4/AD user short names design draft

2017-03-01 Thread Jan Cholasta

On 1.3.2017 14:58, Alexander Bokovoy wrote:

On ke, 01 maalis 2017, Jan Cholasta wrote:

On 1.3.2017 14:05, Alexander Bokovoy wrote:

On ke, 01 maalis 2017, Jan Cholasta wrote:

On 1.3.2017 13:39, Martin Babinsky wrote:

Alexander,

thank you for your comments. Replies inline:

On 02/28/2017 01:48 PM, 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.



The validation is described in a Design section as follows:

"""
Finally, any modification of the domain resolution order must ensure
that each of the specified domain names corresponds either to that of
FreeIPA domain or to one of the trusted AD domains stored in LDAP
backend. In the case of trusted domains, the domain must not be marked
as disabled.
"""

Is this sufficient or is a more thorough validation required? Shall I
split the whole section into sub-sections for easier navigation?


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



I have discussed this with Jan and consulted the same document that
you
cited, that's why I have arrived to the conclusion to use
whitespace as
separator. Jakub/Fabiano, is this ok with the way SSSD decodes domain
names or should we consider other options to avoid breakage with more
exotic domain names?


Actually I would prefer something else than whitespace as a separator.
A ':' maybe?

or ',' or ';'. Any would work.


I have considered a empty attribute value to be a distinct state from
the missing attribute and assigned a different semantic meaning to it.

The reasoning is as follows: if the attribute is not set, SSSD will
not
retrieve it and this signals that it should continue operate in usual
way.

If the attribute is present but is empty, the semantics change
slightly
as now we consider *no* domains during short name resolution
(extension
of the missing domain behavior to the case of all domains are missing
from list).


It doesn't have to be literally empty (LDAP character string syntaxes
don't allow it anyway IIRC), there can be a value which denotes an
empty list of domain (e.g. the separator alone).

I don't see *why* there should be this distinction. The deciding party
is SSSD. Whether this attirbute exists and empty or does not exist at
all does not change anything. Changing how SSSD interprets own defaults
depending on absense or emptiness of certain attribute in IPA config
object is not user friendly at all.

SSSD default behavior should stay the same whether it finds missing or
empty attribute because the attribute will not be known to older SSSD
anyway. Missing or empty attribute should, in my opinion, be equal to
older SSSD behavior.



"No value is set in configuration => use built-in default / some value
is set configuration => use the value" is perfectly user friendly and
pretty much common virtually everywhere I believe, much more so than
"empty value is set in configuration => ignore the value even if the
user deliberately set it empty and use the default value instead".

I'm not arguing with "no value is set in configuration -> use built-in
default". I do argue on having empty but present attribute because it
does not add anything us

Re: [Freeipa-devel] Please review: V4/AD user short names design draft

2017-03-01 Thread Jan Cholasta

On 1.3.2017 14:05, Alexander Bokovoy wrote:

On ke, 01 maalis 2017, Jan Cholasta wrote:

On 1.3.2017 13:39, Martin Babinsky wrote:

Alexander,

thank you for your comments. Replies inline:

On 02/28/2017 01:48 PM, 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.



The validation is described in a Design section as follows:

"""
Finally, any modification of the domain resolution order must ensure
that each of the specified domain names corresponds either to that of
FreeIPA domain or to one of the trusted AD domains stored in LDAP
backend. In the case of trusted domains, the domain must not be marked
as disabled.
"""

Is this sufficient or is a more thorough validation required? Shall I
split the whole section into sub-sections for easier navigation?


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



I have discussed this with Jan and consulted the same document that you
cited, that's why I have arrived to the conclusion to use whitespace as
separator. Jakub/Fabiano, is this ok with the way SSSD decodes domain
names or should we consider other options to avoid breakage with more
exotic domain names?


Actually I would prefer something else than whitespace as a separator.
A ':' maybe?

or ',' or ';'. Any would work.


I have considered a empty attribute value to be a distinct state from
the missing attribute and assigned a different semantic meaning to it.

The reasoning is as follows: if the attribute is not set, SSSD will not
retrieve it and this signals that it should continue operate in usual
way.

If the attribute is present but is empty, the semantics change slightly
as now we consider *no* domains during short name resolution (extension
of the missing domain behavior to the case of all domains are missing
from list).


It doesn't have to be literally empty (LDAP character string syntaxes
don't allow it anyway IIRC), there can be a value which denotes an
empty list of domain (e.g. the separator alone).

I don't see *why* there should be this distinction. The deciding party
is SSSD. Whether this attirbute exists and empty or does not exist at
all does not change anything. Changing how SSSD interprets own defaults
depending on absense or emptiness of certain attribute in IPA config
object is not user friendly at all.

SSSD default behavior should stay the same whether it finds missing or
empty attribute because the attribute will not be known to older SSSD
anyway. Missing or empty attribute should, in my opinion, be equal to
older SSSD behavior.



"No value is set in configuration => use built-in default / some value 
is set configuration => use the value" is perfectly user friendly and 
pretty much common virtually everywhere I believe, much more so than 
"empty value is set in configuration => ignore the value even if the 
user deliberately set it empty and use the default value instead".


--
Jan Cholasta

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


Re: [Freeipa-devel] Please review: V4/AD user short names design draft

2017-03-01 Thread Jan Cholasta

On 1.3.2017 13:39, Martin Babinsky wrote:

Alexander,

thank you for your comments. Replies inline:

On 02/28/2017 01:48 PM, 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.



The validation is described in a Design section as follows:

"""
Finally, any modification of the domain resolution order must ensure
that each of the specified domain names corresponds either to that of
FreeIPA domain or to one of the trusted AD domains stored in LDAP
backend. In the case of trusted domains, the domain must not be marked
as disabled.
"""

Is this sufficient or is a more thorough validation required? Shall I
split the whole section into sub-sections for easier navigation?


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



I have discussed this with Jan and consulted the same document that you
cited, that's why I have arrived to the conclusion to use whitespace as
separator. Jakub/Fabiano, is this ok with the way SSSD decodes domain
names or should we consider other options to avoid breakage with more
exotic domain names?


Actually I would prefer something else than whitespace as a separator. A 
':' maybe?





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



I have considered a empty attribute value to be a distinct state from
the missing attribute and assigned a different semantic meaning to it.

The reasoning is as follows: if the attribute is not set, SSSD will not
retrieve it and this signals that it should continue operate in usual way.

If the attribute is present but is empty, the semantics change slightly
as now we consider *no* domains during short name resolution (extension
of the missing domain behavior to the case of all domains are missing
from list).


It doesn't have to be literally empty (LDAP character string syntaxes 
don't allow it anyway IIRC), there can be a value which denotes an empty 
list of domain (e.g. the separator alone).




That is however open to discussion and I think we can even get away from
this by letting SSSD guys to decide how to handle this case.


- There are typos in the page.



I know there was not much proofreading involved in this iteration. I
have already tried to fix them.


[1]
https://support.microsoft.com/en-us/help/909264/naming-conventions-in-active-directory-for-computers,-domains,-sites,-and-ous










--
Jan Cholasta

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


Re: [Freeipa-devel] MD5 certificate fingerprints removal

2017-02-23 Thread Jan Cholasta

On 23.2.2017 19:06, Martin Basti wrote:



On 23.02.2017 15:09, Tomas Krizek wrote:

On 02/22/2017 01:44 PM, Fraser Tweedale wrote:

On Wed, Feb 22, 2017 at 01:41:22PM +0100, Tomas Krizek wrote:

On 02/22/2017 12:28 AM, Fraser Tweedale wrote:

On Tue, Feb 21, 2017 at 05:23:07PM +0100, Standa Laznicka wrote:

On 02/21/2017 04:24 PM, Tomas Krizek wrote:

On 02/21/2017 03:23 PM, Rob Crittenden wrote:

Standa Laznicka wrote:

Hello,

Since we're trying to make FreeIPA work in FIPS we got to the point
where we need to do something with MD5 fingerprints in the cert plugin.
Eventually we came to a realization that it'd be best to get rid of them
as a whole. These are counted by the framework and are not stored
anywhere. Note that alongside with these fingerprints SHA1 fingerprints
are also counted and those are there to stay.

The question for this ML is, then - is it OK to remove these or would
you rather have them replaced with SHA-256 alongside the SHA-1? MD5 is a
grandpa and I think it should go.

I based the values displayed on what certutil displayed at the time (7
years ago). I don't know that anyone uses these fingerprints. The
OpenSSL equivalent doesn't include them by default.

You may be able to deprecate fingerprints altogether.

rob

I think it's useful to display the certificate's fingerprint. I'm in
favor of removing md5 and adding sha256 instead.


Rob, thank you for sharing the information of where the cert fingerprints
are originated! `certutil` shipped with nss-3.27.0-1.3 currently displays
SHA-256 and SHA1 fingerprints for certificates so I propose going that way
too.


IMO we should remove MD5 and SHA-1, and add SHA-256.  But we should
also make no API stability guarantee w.r.t. the fingerprint
attributes, i.e. to allow us to move to newer digests in future (and
remove broken/no-longer-secure ones).  We should advise that if a
customer has a hard requirement on a particular digest that they
should compute it themselves from the certificate.

Cheers,
Fraser

What is the motivation to remove SHA-1? Are there any attacks besides
theoretical ones on SHA-1?

Do other libraries already deprecate SHA-1?


Come to think of it, I was thinking about SHA-1 signatures (which
are completely forbidden in the public PKI nowadays).  But for
fingerprints it is not so bad (for now).

Thanks,
Fraser

Actually, there's been a practical SHA1 attack just published [1].
Computational complexity was
9,223,372,036,854,775,808 SHA1 computations, which takes about 110 years
on a single GPU.

Therefore, I'm in favor to deprecate SHA1 as well and provide only SHA256.

[1] - https://shattered.io/





I think we should wait with removal SHA1, don't remove it prematurely.
As MD5 is deprecated for very long time, SHA1 is not and we are not
using it for any cryptographic operation nor certificates. It is just
informational fingerprint.


+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


Re: [Freeipa-devel] Certificate Identity Mapping - new API to retrieve matching users

2017-02-22 Thread Jan Cholasta

On 22.2.2017 11:28, Sumit Bose wrote:

On Wed, Feb 22, 2017 at 10:02:24AM +0100, Petr Vobornik wrote:

On 02/22/2017 12:43 AM, Fraser Tweedale wrote:

On Tue, Feb 21, 2017 at 06:12:23PM +0100, Petr Vobornik wrote:

On 02/21/2017 05:15 PM, Florence Blanc-Renaud wrote:

Hi,

related to the Certificate Identity Mapping feature, a new CLI will be
needed to find all the users matching a given certificate.

I propose to provide this as:

ipa certmaptest --certificate 
---
2 users matched
---
  Matched user login: test1
  Matched user login: test2

Number of entries returned 2



Please provide any comments, suggestions on the CLI or the output.
Thanks,
Flo.



Thanks Flo for sharing it.

I don't like the command name. It is not self explanatory. It says it is
testing something, it is not clear what and the actual result is users who
match the map configuration or have the cert in their user's entry.

Better would be:
  $ ipa certmap-match --certificate


How about `ipa certmap-find-user ...'?  Doesn't get more obvious
than that, IMO.


Was thinking about that as well but I think that the command might, in
future, return also something else then user object, e.g. ID override.


No, since the ID override is related to a user the user should be
returned not the override.


"user" in IPA means IPA user, so there will be a difference between IPA 
users and external users, which I think was Petr's point. I agree with 
him that certmap-find-user is not the right name for the command, 
because it suggests that it returns only IPA users.




bye,
Sumit







Pasting user story to give context if somebody is not familiar with it:
"""
As a Security Officer, I want to present IdM Server with an Employee Smart
Card certificate and list all Employees with a matching role account, so
that I can validate the configuration is correct

Note: In FreeIPA 4.4, user-find --certificate can already find users linked
with a certificate blob

Acceptance criteria:
* I can perform the administrative task both via IdM Web UI and CLI
* When asking IdM for the information, I should always receive the same list
that would be matched in client authentication workflows (by SSSD)
* The list of users should include both users linked via standard
certificate blob and other generically mapped users
"""
--
Petr Vobornik

Associate Manager, Engineering, Identity Management
Red Hat, Inc.

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



--
Petr Vobornik

Associate Manager, Engineering, Identity Management
Red Hat, Inc.

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





--
Jan Cholasta

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


Re: [Freeipa-devel] [DESIGN] IPA permission enforcement in Dogtag

2017-02-07 Thread Jan Cholasta

On 8.2.2017 08:06, Fraser Tweedale wrote:

On Wed, Feb 08, 2017 at 08:02:18AM +0100, Jan Cholasta wrote:

On 8.2.2017 07:29, Fraser Tweedale wrote:

On Mon, Feb 06, 2017 at 10:24:31AM +0100, Jan Cholasta wrote:

On 17.1.2017 08:57, David Kupka wrote:

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


IMO having to disable the CA before deletion is an implementation detail and
should not be exposed to the user at all. Why do we have to disable the CA
from IPA in ca-del? I would expect Dogtag to disable it itself internally
when it's being deleted.


The CA requiring disablement before deletion is a property of how
Dogtag Lightweight CAs are implement.  I don't intend to change this
(besides, it might need to be this way for Common Criteria; a
similar restriction exists for profiles).


OK.



We could make it so that in IPA context, delete permission implies
disable permission.  Currently (in Dogtag) permission to
enable/disable is the 'modify' permission.  So to do this without
implying that someone with 'delete' permission as 'modify'
permission, I'd need to add an explicit 'enable/disable ca'
permission.


+1



This is a good idea, but it is more work to add the required ACLs
(which will need to be done during IPA upgrade or installation).
I'll shelve https://github.com/freeipa/freeipa/pull/415 for now, but
keep the patch in my working branch and code it out later, if
there's time before release.  Otherwise we might need to keep it
until there's time for the proper fix, so that things don't break.


OK. I can give you a hand with the ACLs if you want.


Thanks.  The ACLs are part of Dogtag actually; so when we upgrade to
a verison of Dogtag with the new permissions, new ACLs will need to
be added.  There will be two versions of the ACLs: one set for use
with RA Agent cert authn, and one set for use with externally
authenticated FreeIPA principals.

There are a handful of similar "new ACLs to chase Dogtag changes"
that will be part of the GSS-API work.  I have a good understanding
of what needs to happen.


I see. I though you meant ACIs on IPA side.

Are we not going to rely on our ACIs for access control in Dogtag + GSSAPI?



Cheers,
Fraser




--
Jan Cholasta

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


Re: [Freeipa-devel] [DESIGN] IPA permission enforcement in Dogtag

2017-02-07 Thread Jan Cholasta

On 8.2.2017 07:29, Fraser Tweedale wrote:

On Mon, Feb 06, 2017 at 10:24:31AM +0100, Jan Cholasta wrote:

On 17.1.2017 08:57, David Kupka wrote:

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


IMO having to disable the CA before deletion is an implementation detail and
should not be exposed to the user at all. Why do we have to disable the CA
from IPA in ca-del? I would expect Dogtag to disable it itself internally
when it's being deleted.


The CA requiring disablement before deletion is a property of how
Dogtag Lightweight CAs are implement.  I don't intend to change this
(besides, it might need to be this way for Common Criteria; a
similar restriction exists for profiles).


OK.



We could make it so that in IPA context, delete permission implies
disable permission.  Currently (in Dogtag) permission to
enable/disable is the 'modify' permission.  So to do this without
implying that someone with 'delete' permission as 'modify'
permission, I'd need to add an explicit 'enable/disable ca'
permission.


+1



This is a good idea, but it is more work to add the required ACLs
(which will need to be done during IPA upgrade or installation).
I'll shelve https://github.com/freeipa/freeipa/pull/415 for now, but
keep the patch in my working branch and code it out later, if
there's time before release.  Otherwise we might need to keep it
until there's time for the proper fix, so that things don't break.


OK. I can give you a hand with the ACLs if you want.



Thanks,
Fraser




--
Jan Cholasta

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


Re: [Freeipa-devel] CSR autogeneration next steps

2017-02-07 Thread Jan Cholasta

On 4.2.2017 16:40, Ben Lipton wrote:

On 01/12/2017 04:35 AM, Jan Cholasta wrote:

On 11.1.2017 00:38, Ben Lipton wrote:


On 01/10/2017 01:58 AM, Jan Cholasta wrote:

On 19.12.2016 21:59, Ben Lipton wrote:


On 12/15/2016 11:11 PM, Ben Lipton wrote:


On 12/12/2016 03:52 AM, Jan Cholasta wrote:

On 5.12.2016 16:48, Ben Lipton wrote:

Hi Jan, thanks for the comments.


On 12/05/2016 04:25 AM, Jan Cholasta wrote:

Hi Ben,

On 3.11.2016 00:12, Ben Lipton wrote:

Hi everybody,

Soon I'm going to have to reduce the amount of time I spend on
new
development work for the CSR autogeneration project, and I
want to
leave
the project in as organized a state as possible. So, I'm taking
inventory of the work I've done in order to make sure that what's
ready
for review can get reviewed and the ideas that have been
discussed
get
prototyped or at least recorded so they won't be forgotten.


Thanks, I have some questions and comments, see below.



Code that's ready for review (I will continue to put in as much
time as
needed to help get these ready for submission):

- Current PR: https://github.com/freeipa/freeipa/pull/10


How hard would it be to update the PR to use the "new" interface
from
the design thread? By this I mean that currently there is a
command
(cert_get_requestdata), which creates a CSR from profile id +
principal + helper, but in the design we discussed a command which
creates a CertificationRequestInfo from profile id + principal +
public key.

Internally it could use the OpenSSL helper, no need to
implement the
full "new" design. With your build_requestinfo.c code below it
looks
like it should be pretty straightforward.


This is probably doable with the cffi, but I'm concerned about
usability. A user can run the current command to get a (reusable)
script, and run the script to get a CSR. It works with keys in
both PEM
files and NSS databases already. If we change to outputting a
CertificationRequestInfo, in order to make this usable on the
command
line, we'll need:
- An additional tool to sign a CSR given a CertificationRequestInfo
(for
both types of key storage).
- A way to extract a SubjectPublicKeyInfo structure from a key
within
the ipa command (like [1] but we need it for both types of key
storage)
Since as far as I know there's no standard encoding for files
containing
only a CertificationRequestInfo or a SubjectPublicKeyInfo, we'll be
writing and distributing these ourselves. I think that's where
most of
the extra work will come in.


For PEM files, this is easily doable using python-cryptography (to
extract SubjectPublicKeyInfo and sign CertificationRequestInfo) and
PyASN1 (to create a CSR from the CertificationRequestInfo and the
signature).


I didn't realize that python-cryptography knew about
SubjectPublicKeyInfo structures, but indeed this seems to be pretty
straightforward:

key = load_pem_private_key(key_bytes, None, default_backend())
pubkey_info = key.public_key().public_bytes(Encoding.DER,
PublicFormat.SubjectPublicKeyInfo)

Thanks for letting me know this functionality already existed.


I'm currently working on the step of signing the
CertificationRequestInfo and creating a CSR from it. I think I have it
working with pyasn1, but of course the "signature algorithm" for the CSR
needs to be specified and implemented within the code since I'm not
using a library that understands CSRs natively. The code I have
currently always produces CSRs with the sha256WithRSAEncryption
algorithm (DER-encode request info, SHA256, PKCS #1v1.5 padding, RSA
encryption), and the OID for that algorithm is hardcoded in the output
CSR. Is this ok or will we need more flexibility than that?


IMO it's OK for starters.



For NSS databases, this will be trickier and will require calling C
functions, as neither certutil nor python-nss provide a way to a)
address existing keys in the database by key ID b) get
SubjectPublicKeyInfo for a given key.


This can be worked around by:

1. Generating a key + temporary certificate:

n=$(head -c 40 /dev/urandom | base32)
certutil -S -n $n -s CN=$n -x -t ,,

2. Extracting the public key from the certificate:

certutil -L -n $n -a >temp.crt
(extract the public key using python-cryptography)

3. Deleting the temporary certificate:

certutil -D -n $n

4. Importing the newly issued certificate:

certutil -A -n $n -t ,, -a 
Oof, thanks, I'm not sure I would have been able to come up with that.
Can you generate a key without a temporary certificate if you use the
NSS API, or does their model require every key to belong to a cert?


I'm pretty sure it's possible, but it certainly won't be as simple as
this. I gave up after a few hours of digging into NSS source code and
not being able to figure out how.



As for encoding, the obvious choice is DER. It does not really
matter
there is no standard file format, as we won't be transferring these
as files anyway.


Agreed. I just meant there aren't tools already because th

Re: [Freeipa-devel] [DESIGN] IPA permission enforcement in Dogtag

2017-02-06 Thread Jan Cholasta

On 17.1.2017 08:57, David Kupka wrote:

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


IMO having to disable the CA before deletion is an implementation detail 
and should not be exposed to the user at all. Why do we have to disable 
the CA from IPA in ca-del? I would expect Dogtag to disable it itself 
internally when it's being deleted.


--
Jan Cholasta

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


Re: [Freeipa-devel] [DESIGN] Dogtag GSS-API Authentication

2017-02-06 Thread Jan Cholasta

On 11.1.2017 02:09, Fraser Tweedale wrote:

On Tue, Jan 10, 2017 at 10:48:08AM +0100, Martin Babinsky wrote:

Hi Fraser,

I have some rather inane comments. I guess Jan cholasta will do a more
thorough review of your design. See below:

On 01/06/2017 09:08 AM, Fraser Tweedale wrote:

Hi comrades,

I have written up the high-level details of the FreeIPA->Dogtag
GSS-API authentication design.  The goal is improve security by
removing an egregious privilege separation violation: the RA Agent
cert.

There is a fair bit of work still to do on the Dogtag side but
things are shaping up there and it's time to work out the IPA
aspects.  The design is at:

  http://www.freeipa.org/page/V4/Dogtag_GSS-API_Authentication


first of all, you link a internal document from publicly available design
page. you should prepare a publicly visible version of the Dogtag-side
design and link that.


Will do; thanks.


It would also be nice to have a high-level graphical representation of the
proposed CSR processing workflow. I think you can re-use the one that is in
the Dogtag part, omit the Dogtag internals and add IPA-specific parts.


I will definitely do this a bit later, once more details of IPA
design are established.



Right now, I need feedback about the Domain Level aspects: whether
it is the right approach, whether there are mechanisms to perform
update steps (specifically: LDAP updates and/or api calls) alongside
a DL bump, or if there aren't, how to deal with that (implement such
a mechanism, make admins do extra steps, ???).



Is the DL bump really necessary? Are you sure we really can not just update
the profile configuration and let older Dogtag installation handle it
gracefully? IIRC we have done some profile inclusion work in 4.2 development
and on and never really bothered about older Dogtag understanding them.


The problem is that the new profiles will refer to plugins (i.e.
classes) that do not exist in older versions of Dogtag.  Profile
config is replicated, so if we upgrade profile config with old
versions of Dogtag in the topology, it breaks them.

I considered a mechanism where multiple versions of a profile exist
in LDAP (i.e. multiple attribute values), and Dogtag picks the one
that's "right" for it.  (An example of how to do this might be
attribute tagging where tag indicates minimum version of Dogtag
containing components used in that profile version, and Dogag picks
the highest that it supports).  The advantage of such a mechanism is
that we could use it for any future scenario where we introduce new
profile components that we want to use in IPA.  The downside is that
it significantly complicates profile management (including for
administrators), and can result in the same profile having different
behaviour on different Dogtag instances, which could be confusing
and make it harder to diagnose issues.  Given the tradeoffs, I think
a DL bump is preferable.


I don't like the prospect of having to bump DL every time a new 
component is introduced. This time it might be OK, because the new DL is 
apparently required for the RA -> GSSAPI change, but IMHO in general a 
change in a certificate profile does not warrant a DL bump.


I agree that maintaining multiple versions of a profile is not the way 
to go, but I think there are other options:


* Change `auth.instance_id` from `raCertAuth` to a new, IPA-specific 
`ipaAuth`. Configure `auths.instance.ipaAuth` in CS.cfg to behave 
exactly the same as `raCertAuth`. This will have to be done on all 
masters, including old ones, which can receive the change in a bug fix 
update (4.4.x). Then, on upgrade to new IPA with GSSAPI enabled Dogtag, 
change `auths.instance.ipaAuth` to use external script for 
authentication. Similar thing could be done for other profile components.


* Do not care about old masters. Update the profile and let certificate 
requests on old masters fail. This should be fine, as the situation 
where there are different version masters should be only temporary until 
all masters are upgraded. If an appropriate error is returned from 
cert-request, automated requests via certmonger will be re-attempted and 
will succeed once all masters are upgraded.





Anyway I guess we can call `certprofile-import' to load
ExternalProcessConstraint-enabled profile upon setting domain level to 2, we
just have to know where on the FS it is located.


Of course, any other general or specific feedback is welcome.

Thanks,
Fraser



So if I understand correctly there will be no change in CA ACL management
interface and only the code which evaluates them will be factored out into
'ipa-pki-validate-cert-request' command? Also, wouldn't it simpler if the CA
ACL evaluation was delegated to a separate API command instead?
ExternalProcessConstraint would then only ask IPA JSON api and process the
response.


There are no changes to CA ACL management interface as part of this
design, but there are proposals to extend/rework it in future, e

Re: [Freeipa-devel] [design] add nsupdate output format to dns-update-system-records

2017-01-29 Thread Jan Cholasta

On 27.1.2017 22:45, Petr Vobornik wrote:

On 01/27/2017 01:37 PM, Martin Basti wrote:

Hello all,

I'm working on adding support of nsupdate format to output of `ipa
dns-update-system-records` command, that will allow to copy output
to nsupdate utility and update IPA DNS records on external server. I
propose following:

1) new option --nsupdate-format

This option will replace standard output with nsupdate format output.
Feel free to propose better name.


2) client or server side plugin?

I'd like to implement this behavior only at client side. But this will
not work in cases when only server api or RPC calls are used. Do you
think that we should support this uses case (in server side plugin)?


Martin




The main ideal was to allow piping into nsupdate. i don't remember if
server side returns in human readable form or in structured way. If in
structured, then implementation in client side plugin would make better
sense.

+1 to Alexander on '--out nsupdate' option


+1 to both --out and client-side implementation.

--
Jan Cholasta

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


Re: [Freeipa-devel] CSR autogeneration next steps

2017-01-12 Thread Jan Cholasta

On 11.1.2017 00:38, Ben Lipton wrote:


On 01/10/2017 01:58 AM, Jan Cholasta wrote:

On 19.12.2016 21:59, Ben Lipton wrote:


On 12/15/2016 11:11 PM, Ben Lipton wrote:


On 12/12/2016 03:52 AM, Jan Cholasta wrote:

On 5.12.2016 16:48, Ben Lipton wrote:

Hi Jan, thanks for the comments.


On 12/05/2016 04:25 AM, Jan Cholasta wrote:

Hi Ben,

On 3.11.2016 00:12, Ben Lipton wrote:

Hi everybody,

Soon I'm going to have to reduce the amount of time I spend on new
development work for the CSR autogeneration project, and I want to
leave
the project in as organized a state as possible. So, I'm taking
inventory of the work I've done in order to make sure that what's
ready
for review can get reviewed and the ideas that have been discussed
get
prototyped or at least recorded so they won't be forgotten.


Thanks, I have some questions and comments, see below.



Code that's ready for review (I will continue to put in as much
time as
needed to help get these ready for submission):

- Current PR: https://github.com/freeipa/freeipa/pull/10


How hard would it be to update the PR to use the "new" interface
from
the design thread? By this I mean that currently there is a command
(cert_get_requestdata), which creates a CSR from profile id +
principal + helper, but in the design we discussed a command which
creates a CertificationRequestInfo from profile id + principal +
public key.

Internally it could use the OpenSSL helper, no need to implement the
full "new" design. With your build_requestinfo.c code below it looks
like it should be pretty straightforward.


This is probably doable with the cffi, but I'm concerned about
usability. A user can run the current command to get a (reusable)
script, and run the script to get a CSR. It works with keys in
both PEM
files and NSS databases already. If we change to outputting a
CertificationRequestInfo, in order to make this usable on the command
line, we'll need:
- An additional tool to sign a CSR given a CertificationRequestInfo
(for
both types of key storage).
- A way to extract a SubjectPublicKeyInfo structure from a key within
the ipa command (like [1] but we need it for both types of key
storage)
Since as far as I know there's no standard encoding for files
containing
only a CertificationRequestInfo or a SubjectPublicKeyInfo, we'll be
writing and distributing these ourselves. I think that's where
most of
the extra work will come in.


For PEM files, this is easily doable using python-cryptography (to
extract SubjectPublicKeyInfo and sign CertificationRequestInfo) and
PyASN1 (to create a CSR from the CertificationRequestInfo and the
signature).


I didn't realize that python-cryptography knew about
SubjectPublicKeyInfo structures, but indeed this seems to be pretty
straightforward:

key = load_pem_private_key(key_bytes, None, default_backend())
pubkey_info = key.public_key().public_bytes(Encoding.DER,
PublicFormat.SubjectPublicKeyInfo)

Thanks for letting me know this functionality already existed.


I'm currently working on the step of signing the
CertificationRequestInfo and creating a CSR from it. I think I have it
working with pyasn1, but of course the "signature algorithm" for the CSR
needs to be specified and implemented within the code since I'm not
using a library that understands CSRs natively. The code I have
currently always produces CSRs with the sha256WithRSAEncryption
algorithm (DER-encode request info, SHA256, PKCS #1v1.5 padding, RSA
encryption), and the OID for that algorithm is hardcoded in the output
CSR. Is this ok or will we need more flexibility than that?


IMO it's OK for starters.



For NSS databases, this will be trickier and will require calling C
functions, as neither certutil nor python-nss provide a way to a)
address existing keys in the database by key ID b) get
SubjectPublicKeyInfo for a given key.


This can be worked around by:

1. Generating a key + temporary certificate:

n=$(head -c 40 /dev/urandom | base32)
certutil -S -n $n -s CN=$n -x -t ,,

2. Extracting the public key from the certificate:

certutil -L -n $n -a >temp.crt
(extract the public key using python-cryptography)

3. Deleting the temporary certificate:

certutil -D -n $n

4. Importing the newly issued certificate:

certutil -A -n $n -t ,, -a 
Oof, thanks, I'm not sure I would have been able to come up with that.
Can you generate a key without a temporary certificate if you use the
NSS API, or does their model require every key to belong to a cert?


I'm pretty sure it's possible, but it certainly won't be as simple as 
this. I gave up after a few hours of digging into NSS source code and 
not being able to figure out how.




As for encoding, the obvious choice is DER. It does not really matter
there is no standard file format, as we won't be transferring these
as files anyway.


Agreed. I just meant there aren't tools already because this isn't a
type of file one often needs to process.




Would it be ok to stick w

Re: [Freeipa-devel] CSR autogeneration next steps

2017-01-09 Thread Jan Cholasta

On 19.12.2016 21:59, Ben Lipton wrote:


On 12/15/2016 11:11 PM, Ben Lipton wrote:


On 12/12/2016 03:52 AM, Jan Cholasta wrote:

On 5.12.2016 16:48, Ben Lipton wrote:

Hi Jan, thanks for the comments.


On 12/05/2016 04:25 AM, Jan Cholasta wrote:

Hi Ben,

On 3.11.2016 00:12, Ben Lipton wrote:

Hi everybody,

Soon I'm going to have to reduce the amount of time I spend on new
development work for the CSR autogeneration project, and I want to
leave
the project in as organized a state as possible. So, I'm taking
inventory of the work I've done in order to make sure that what's
ready
for review can get reviewed and the ideas that have been discussed
get
prototyped or at least recorded so they won't be forgotten.


Thanks, I have some questions and comments, see below.



Code that's ready for review (I will continue to put in as much
time as
needed to help get these ready for submission):

- Current PR: https://github.com/freeipa/freeipa/pull/10


How hard would it be to update the PR to use the "new" interface from
the design thread? By this I mean that currently there is a command
(cert_get_requestdata), which creates a CSR from profile id +
principal + helper, but in the design we discussed a command which
creates a CertificationRequestInfo from profile id + principal +
public key.

Internally it could use the OpenSSL helper, no need to implement the
full "new" design. With your build_requestinfo.c code below it looks
like it should be pretty straightforward.


This is probably doable with the cffi, but I'm concerned about
usability. A user can run the current command to get a (reusable)
script, and run the script to get a CSR. It works with keys in both PEM
files and NSS databases already. If we change to outputting a
CertificationRequestInfo, in order to make this usable on the command
line, we'll need:
- An additional tool to sign a CSR given a CertificationRequestInfo
(for
both types of key storage).
- A way to extract a SubjectPublicKeyInfo structure from a key within
the ipa command (like [1] but we need it for both types of key storage)
Since as far as I know there's no standard encoding for files
containing
only a CertificationRequestInfo or a SubjectPublicKeyInfo, we'll be
writing and distributing these ourselves. I think that's where most of
the extra work will come in.


For PEM files, this is easily doable using python-cryptography (to
extract SubjectPublicKeyInfo and sign CertificationRequestInfo) and
PyASN1 (to create a CSR from the CertificationRequestInfo and the
signature).


I didn't realize that python-cryptography knew about
SubjectPublicKeyInfo structures, but indeed this seems to be pretty
straightforward:

key = load_pem_private_key(key_bytes, None, default_backend())
pubkey_info = key.public_key().public_bytes(Encoding.DER,
PublicFormat.SubjectPublicKeyInfo)

Thanks for letting me know this functionality already existed.


For NSS databases, this will be trickier and will require calling C
functions, as neither certutil nor python-nss provide a way to a)
address existing keys in the database by key ID b) get
SubjectPublicKeyInfo for a given key.


This can be worked around by:

1. Generating a key + temporary certificate:

n=$(head -c 40 /dev/urandom | base32)
certutil -S -n $n -s CN=$n -x -t ,,

2. Extracting the public key from the certificate:

certutil -L -n $n -a >temp.crt
(extract the public key using python-cryptography)

3. Deleting the temporary certificate:

certutil -D -n $n

4. Importing the newly issued certificate:

certutil -A -n $n -t ,, -a 

As for encoding, the obvious choice is DER. It does not really matter
there is no standard file format, as we won't be transferring these
as files anyway.


Agreed. I just meant there aren't tools already because this isn't a
type of file one often needs to process.




Would it be ok to stick with the current design in this PR? I'd feel
much better if we could get the basic functionality into the repo and
then iterate on it rather than changing the plan at this point. I can
create a separate PR to change cert_get_requestdata to this new
interface and at the same time add the necessary adapters (bullet
points
above) to make it user-friendly.


Works for me.


Updated the PR to fix conflicts with master. Had some trouble with CI
but creating a new PR with the same commits fixed it
(https://github.com/freeipa/freeipa/pull/337). Not sure if it's fixed
permanently, so I guess I'll just keep the two PRs synchronized now,
or we could close the old one.


You can close the old one.

Just to make sure we are on the same page, you want this PR to be merged 
before submitting additional PRs built on top of it?






I would probably just implement the adapters within the
cert_build/cert_request client code unless you think having standalone
tools is valuable. I suppose certmonger is going to need these features
too, but I don't know how well sharing code between them is going to
work.



Re: [Freeipa-devel] Certificate Identity Mapping

2017-01-08 Thread Jan Cholasta

On 6.1.2017 10:48, Sumit Bose wrote:

On Fri, Jan 06, 2017 at 08:40:31AM +0100, Jan Cholasta wrote:

On 5.1.2017 13:15, Sumit Bose wrote:

On Mon, Jan 02, 2017 at 08:06:04AM +0100, Jan Cholasta wrote:

On 19.12.2016 12:13, Sumit Bose wrote:

On Mon, Dec 19, 2016 at 10:02:58AM +0100, Jan Cholasta wrote:

I agree with *almost* everything Sumit said. See my inline comments below.

On 16.12.2016 11:53, Sumit Bose wrote:

On Tue, Dec 06, 2016 at 04:39:10PM +0100, Florence Blanc-Renaud wrote:

Hi,

I have started a feature description for the Certificate Identity Mapping at
the following location:
http://www.freeipa.org/page/V4/Certificate_Identity_Mapping

This is a first step, focusing on the interface we would like to provide. It
still contains open questions, some of which are linked to the corresponding
design on SSSD side:
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates
https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardsAndMultipleIdentities

Comments, concerns and suggestions are welcome. Thanks!


Hi Flo,

thank you very much for setting up the page.

My comments are mostly about the commands.

certmappingconfig-mod:

* --enable=Boolean: if this option is 'False' SSSD will basically show
  the current behavior and just look up the certificates directly. But I
  wonder if the option is needed at all because not adding any mapping
  rules would have the same effect.

  What is the scope here, only the IPA domain, or all trusted domains as
  well? If it is for trusted domains as well will the certmappingrule-*
  commands and user-{add/remove}-certmapping return an error?

  So, in general I see an overlap with the mapping rules and I think it
  would be clearer to drop this option and do the lookups according to
  the mapping rules.

* --prompt-username=Boolean: the description implies that this option is
  synonymous to 1:1 mapping, but it is not. On Linux authentication in
  most cases use a user name either by directly asking (e.g. /bin/login)
  or using the current user name (e.g. sudo). So, according to its name
  it would only control if gdm is allowed to ask for an (optional) user
  name.

  If the option is renamed to e.g. --force-1-to-1-mapping to really
  enforce a 1:1 mapping then it would make sense to derived to gdm
  behavior. I.e. if 1:1 mapping is enforce it makes no sense for gdm to
  ask for a user name and if it is not enforced then it makes sense to
  offer and optional user name input field.

* --enable-username-mismatch=Boolean: I think this option can be
  dropped. My test so far show that if a non-matching hint is given on a
  Windows client authentication fails.

* --alternate-attribute=STRING: I think this option isn't needed as
  well. For IPA server-side we should decide on an attribute name and
  add it to the schema for user objects. On the client side the
  attribute name can be taken from the mapping rule.A


certmappingrule.*:

* ISSUERDN: it looks like you want to use issuerName here. In
  certificateRecord it it used with LDAP ordering and I would prefer
  LDAP ordering at all points where we have a choice. Unfortunately in the
  issuer-subject mapping AD dictates X.500 ordering.


LDAP ordering should indeed be preferred, as it is used everywhere else in
IPA. We can convert to/from X.500 ordering where necessary, when possible.



* DOMAINDN: does this refer to the nsslapd-certmap-basedn attribute in
  the example? My intention in the SSSD design-page was to specify the
  domain (as in DNS domain/IPA domain/trusted domain) where the matching
  user should be searched. Different domains might certificates from
  different issuers and some domains might not even use certificates.
  With this information SSSD does not have to search any domain trusted
  by IPA from a given certificate, but look only at domains listed here
  (the attribute should be a multi-value one).

  There are objects in the LDAP tree for each trusted domain which are
  used by SSSD so using a DN syntax would be valid here.


We use domain names rather than DNs to refer to domains everywhere else in
the framework. I don't think this place should be an exception.


I'm fine with domain names as well. In fact I didn't thought of using
DNs for this before I read DOMAINDN on the design page.





* LDAPSEARCHFILTER: I think a separate option is not need. LDAP search
  filters should just be a special kind of mapping rules. I can image in
  syntax like: <LDAPFILTER:(&(cn=%A)(email=%B)(authType=pkinit))>. I
  think the difficult part with the LDAP filters will to define sensible
  templates.


I'm not sure I understand. Could you please elaborate a little bit?


A LDAP search filter which would cover the AD behavior would look like:

(|(altSecurityIdentities=%A%B)(userPrincipalName=%C)(samAccountName=%D))

where

%A: must be replaced with the issuer of the certificate in X.500 order
%B: must be replaced with the subject of the certificate in X.500 order

it would b

Re: [Freeipa-devel] [RFC] Matching and Mapping Certificates

2017-01-08 Thread Jan Cholasta

On 6.1.2017 10:30, Sumit Bose wrote:

On Fri, Jan 06, 2017 at 08:50:14AM +0100, Jan Cholasta wrote:

On 5.1.2017 10:39, Sumit Bose wrote:

On Mon, Jan 02, 2017 at 09:18:47AM +0100, Jan Cholasta wrote:

On 18.10.2016 07:34, Jan Cholasta wrote:

On 17.10.2016 16:50, Rob Crittenden wrote:

Jan Cholasta wrote:

Hi,

On 13.10.2016 18:52, Sumit Bose wrote:

= Issuer specific matching =
Although the MIT Kerberos rules allow to select the issuer of a
certificate there are use cases where a more specific selection is
needed. E.g. if there are some default matching rules for all issuers
and some other issuer specific rules where the default rules should
not apply. To make this possible with the above scheme the default
rules must have an  clause which matches all but the issuer
with the specific rules. Writing regular-expressions to not match a
specific string or a list of strings is at least error-prone if not
impossible.

To make it easier to define issuer specific rules and default rules at
the same time and optional issuer string can be added to the rule to
indicate that for the given issuer only those rules should be
considered. Given the use-case I think it is acceptable to require
that the full issuer must be specified here in LDAP order (see below)
and case-sensitive matching is used.


This could also be solved by adding priority to rules - if two rules
match, the one with higher priority (the issuer specific rule) is
preferred over the one with lower priority (the default rule). IMO this
is better than an optional issuer string as it offers greater
flexibility.


The use cases I've seen haven't had to do with priority, though that
would be a nice enhancement, but with only allowing certificates issued
by a specific CA to be allowed (this is pretty common in web servers).
Being able to say "only do the matching on certificates issued by foo"
is valuable.


Sure, I'm not suggesting that matching by issuer should be removed, only
that rule precedence should not be determined by the issuer field setting.



Bump. Sumit, what is your opinion on this?


I'm fine with an optional(?) priority as well. Since priorities are
already used in the pwpolicies this should be already known to the
experienced admin. I guess we just have stick with "A lower value
indicates a higher priority" to not confuse users. That's why I think
that the priority should be optional here and a missing value indicates
the lowest priority (default rules).


In pwpolicy and sudorule, the priority is required and has to be unique.
Maybe we should do this for certificate mapping rules as well?


I think there is no requirement that only a single rule should match
hence I think the priority here must not be unique.


Is there a requirement that it must be possible for multiple rules to 
match? If not, we could begin with a simpler (for admin) solution with 
unique priorities and lift the restriction later when/if it becomes 
necessary. But I don't have a strong opinion on this.








Are you thinking of using the CoS scheme here as well would a priority
attribute be sufficient because we do not want to reference internal
objects in the mapping rules?


I'm not sure how CoS would be helpful here, I think a simple interger
attribute would be perfectly sufficient.


I agree.

bye,
Sumit





bye,
Sumit



--
Jan Cholasta



--
Jan Cholasta



--
Jan Cholasta

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


Re: [Freeipa-devel] [RFC] Matching and Mapping Certificates

2017-01-05 Thread Jan Cholasta

On 5.1.2017 10:39, Sumit Bose wrote:

On Mon, Jan 02, 2017 at 09:18:47AM +0100, Jan Cholasta wrote:

On 18.10.2016 07:34, Jan Cholasta wrote:

On 17.10.2016 16:50, Rob Crittenden wrote:

Jan Cholasta wrote:

Hi,

On 13.10.2016 18:52, Sumit Bose wrote:

= Issuer specific matching =
Although the MIT Kerberos rules allow to select the issuer of a
certificate there are use cases where a more specific selection is
needed. E.g. if there are some default matching rules for all issuers
and some other issuer specific rules where the default rules should
not apply. To make this possible with the above scheme the default
rules must have an  clause which matches all but the issuer
with the specific rules. Writing regular-expressions to not match a
specific string or a list of strings is at least error-prone if not
impossible.

To make it easier to define issuer specific rules and default rules at
the same time and optional issuer string can be added to the rule to
indicate that for the given issuer only those rules should be
considered. Given the use-case I think it is acceptable to require
that the full issuer must be specified here in LDAP order (see below)
and case-sensitive matching is used.


This could also be solved by adding priority to rules - if two rules
match, the one with higher priority (the issuer specific rule) is
preferred over the one with lower priority (the default rule). IMO this
is better than an optional issuer string as it offers greater
flexibility.


The use cases I've seen haven't had to do with priority, though that
would be a nice enhancement, but with only allowing certificates issued
by a specific CA to be allowed (this is pretty common in web servers).
Being able to say "only do the matching on certificates issued by foo"
is valuable.


Sure, I'm not suggesting that matching by issuer should be removed, only
that rule precedence should not be determined by the issuer field setting.



Bump. Sumit, what is your opinion on this?


I'm fine with an optional(?) priority as well. Since priorities are
already used in the pwpolicies this should be already known to the
experienced admin. I guess we just have stick with "A lower value
indicates a higher priority" to not confuse users. That's why I think
that the priority should be optional here and a missing value indicates
the lowest priority (default rules).


In pwpolicy and sudorule, the priority is required and has to be unique. 
Maybe we should do this for certificate mapping rules as well?




Are you thinking of using the CoS scheme here as well would a priority
attribute be sufficient because we do not want to reference internal
objects in the mapping rules?


I'm not sure how CoS would be helpful here, I think a simple interger 
attribute would be perfectly sufficient.




bye,
Sumit



--
Jan Cholasta



--
Jan Cholasta

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


Re: [Freeipa-devel] Certificate Identity Mapping

2017-01-05 Thread Jan Cholasta

On 5.1.2017 13:15, Sumit Bose wrote:

On Mon, Jan 02, 2017 at 08:06:04AM +0100, Jan Cholasta wrote:

On 19.12.2016 12:13, Sumit Bose wrote:

On Mon, Dec 19, 2016 at 10:02:58AM +0100, Jan Cholasta wrote:

I agree with *almost* everything Sumit said. See my inline comments below.

On 16.12.2016 11:53, Sumit Bose wrote:

On Tue, Dec 06, 2016 at 04:39:10PM +0100, Florence Blanc-Renaud wrote:

Hi,

I have started a feature description for the Certificate Identity Mapping at
the following location:
http://www.freeipa.org/page/V4/Certificate_Identity_Mapping

This is a first step, focusing on the interface we would like to provide. It
still contains open questions, some of which are linked to the corresponding
design on SSSD side:
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates
https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardsAndMultipleIdentities

Comments, concerns and suggestions are welcome. Thanks!


Hi Flo,

thank you very much for setting up the page.

My comments are mostly about the commands.

certmappingconfig-mod:

* --enable=Boolean: if this option is 'False' SSSD will basically show
  the current behavior and just look up the certificates directly. But I
  wonder if the option is needed at all because not adding any mapping
  rules would have the same effect.

  What is the scope here, only the IPA domain, or all trusted domains as
  well? If it is for trusted domains as well will the certmappingrule-*
  commands and user-{add/remove}-certmapping return an error?

  So, in general I see an overlap with the mapping rules and I think it
  would be clearer to drop this option and do the lookups according to
  the mapping rules.

* --prompt-username=Boolean: the description implies that this option is
  synonymous to 1:1 mapping, but it is not. On Linux authentication in
  most cases use a user name either by directly asking (e.g. /bin/login)
  or using the current user name (e.g. sudo). So, according to its name
  it would only control if gdm is allowed to ask for an (optional) user
  name.

  If the option is renamed to e.g. --force-1-to-1-mapping to really
  enforce a 1:1 mapping then it would make sense to derived to gdm
  behavior. I.e. if 1:1 mapping is enforce it makes no sense for gdm to
  ask for a user name and if it is not enforced then it makes sense to
  offer and optional user name input field.

* --enable-username-mismatch=Boolean: I think this option can be
  dropped. My test so far show that if a non-matching hint is given on a
  Windows client authentication fails.

* --alternate-attribute=STRING: I think this option isn't needed as
  well. For IPA server-side we should decide on an attribute name and
  add it to the schema for user objects. On the client side the
  attribute name can be taken from the mapping rule.A


certmappingrule.*:

* ISSUERDN: it looks like you want to use issuerName here. In
  certificateRecord it it used with LDAP ordering and I would prefer
  LDAP ordering at all points where we have a choice. Unfortunately in the
  issuer-subject mapping AD dictates X.500 ordering.


LDAP ordering should indeed be preferred, as it is used everywhere else in
IPA. We can convert to/from X.500 ordering where necessary, when possible.



* DOMAINDN: does this refer to the nsslapd-certmap-basedn attribute in
  the example? My intention in the SSSD design-page was to specify the
  domain (as in DNS domain/IPA domain/trusted domain) where the matching
  user should be searched. Different domains might certificates from
  different issuers and some domains might not even use certificates.
  With this information SSSD does not have to search any domain trusted
  by IPA from a given certificate, but look only at domains listed here
  (the attribute should be a multi-value one).

  There are objects in the LDAP tree for each trusted domain which are
  used by SSSD so using a DN syntax would be valid here.


We use domain names rather than DNs to refer to domains everywhere else in
the framework. I don't think this place should be an exception.


I'm fine with domain names as well. In fact I didn't thought of using
DNs for this before I read DOMAINDN on the design page.





* LDAPSEARCHFILTER: I think a separate option is not need. LDAP search
  filters should just be a special kind of mapping rules. I can image in
  syntax like: <LDAPFILTER:(&(cn=%A)(email=%B)(authType=pkinit))>. I
  think the difficult part with the LDAP filters will to define sensible
  templates.


I'm not sure I understand. Could you please elaborate a little bit?


A LDAP search filter which would cover the AD behavior would look like:

(|(altSecurityIdentities=%A%B)(userPrincipalName=%C)(samAccountName=%D))

where

%A: must be replaced with the issuer of the certificate in X.500 order
%B: must be replaced with the subject of the certificate in X.500 order

it would be possible of course to use a specific template here which
would generate the complete search attri

Re: [Freeipa-devel] [RFC] Matching and Mapping Certificates

2017-01-02 Thread Jan Cholasta

On 18.10.2016 07:34, Jan Cholasta wrote:

On 17.10.2016 16:50, Rob Crittenden wrote:

Jan Cholasta wrote:

Hi,

On 13.10.2016 18:52, Sumit Bose wrote:

= Issuer specific matching =
Although the MIT Kerberos rules allow to select the issuer of a
certificate there are use cases where a more specific selection is
needed. E.g. if there are some default matching rules for all issuers
and some other issuer specific rules where the default rules should
not apply. To make this possible with the above scheme the default
rules must have an  clause which matches all but the issuer
with the specific rules. Writing regular-expressions to not match a
specific string or a list of strings is at least error-prone if not
impossible.

To make it easier to define issuer specific rules and default rules at
the same time and optional issuer string can be added to the rule to
indicate that for the given issuer only those rules should be
considered. Given the use-case I think it is acceptable to require
that the full issuer must be specified here in LDAP order (see below)
and case-sensitive matching is used.


This could also be solved by adding priority to rules - if two rules
match, the one with higher priority (the issuer specific rule) is
preferred over the one with lower priority (the default rule). IMO this
is better than an optional issuer string as it offers greater
flexibility.


The use cases I've seen haven't had to do with priority, though that
would be a nice enhancement, but with only allowing certificates issued
by a specific CA to be allowed (this is pretty common in web servers).
Being able to say "only do the matching on certificates issued by foo"
is valuable.


Sure, I'm not suggesting that matching by issuer should be removed, only
that rule precedence should not be determined by the issuer field setting.



Bump. Sumit, what is your opinion on this?

--
Jan Cholasta

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


Re: [Freeipa-devel] Certificate Identity Mapping

2017-01-02 Thread Jan Cholasta

On 16.12.2016 09:34, Florence Blanc-Renaud wrote:

On 12/06/2016 04:39 PM, Florence Blanc-Renaud wrote:

Hi,

I have started a feature description for the Certificate Identity
Mapping at the following location:
http://www.freeipa.org/page/V4/Certificate_Identity_Mapping

This is a first step, focusing on the interface we would like to
provide. It still contains open questions, some of which are linked to
the corresponding design on SSSD side:
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates


https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardsAndMultipleIdentities



Comments, concerns and suggestions are welcome. Thanks!

Flo.



Hi,

the design page for Certificate Identity Mapping [1] has been updated
with a schema proposal and an example of configuration data.

Please share your comments, concerns, suggestions before January 7, so
that we can finalize the API and start the implementation.
Thanks,
Flo.


1) I'm not fan of host-mod --certmapping-prompt-username. IMO it would 
be better to base this on group membership, which would allow automember 
to be used.


A possible solution would be to introduce a CoS-based policy object, 
similar to pwpolicy, but for hosts:


certmappolicy-mod [HOSTGROUP] --prompt-username=Boolean
certmappolicy-add HOSTGROUP --prompt-username=Boolean
certmappolicy-del HOSTGROUP

HOSTGROUP can be ommited in certmappolicy-mod, in which case the default 
policy is modified. This would allow removing --prompt-username and 
--enable-local-prompt-policy from certmappingconfig.



2) Nitpick: could we please rename certmapping* to certmap*? Not only 
would it be quicker to type in the command line, but also named 
consistently with selinuxusermap.



--
Jan Cholasta

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


Re: [Freeipa-devel] Certificate Identity Mapping

2017-01-01 Thread Jan Cholasta

On 19.12.2016 12:13, Sumit Bose wrote:

On Mon, Dec 19, 2016 at 10:02:58AM +0100, Jan Cholasta wrote:

I agree with *almost* everything Sumit said. See my inline comments below.

On 16.12.2016 11:53, Sumit Bose wrote:

On Tue, Dec 06, 2016 at 04:39:10PM +0100, Florence Blanc-Renaud wrote:

Hi,

I have started a feature description for the Certificate Identity Mapping at
the following location:
http://www.freeipa.org/page/V4/Certificate_Identity_Mapping

This is a first step, focusing on the interface we would like to provide. It
still contains open questions, some of which are linked to the corresponding
design on SSSD side:
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates
https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardsAndMultipleIdentities

Comments, concerns and suggestions are welcome. Thanks!


Hi Flo,

thank you very much for setting up the page.

My comments are mostly about the commands.

certmappingconfig-mod:

* --enable=Boolean: if this option is 'False' SSSD will basically show
  the current behavior and just look up the certificates directly. But I
  wonder if the option is needed at all because not adding any mapping
  rules would have the same effect.

  What is the scope here, only the IPA domain, or all trusted domains as
  well? If it is for trusted domains as well will the certmappingrule-*
  commands and user-{add/remove}-certmapping return an error?

  So, in general I see an overlap with the mapping rules and I think it
  would be clearer to drop this option and do the lookups according to
  the mapping rules.

* --prompt-username=Boolean: the description implies that this option is
  synonymous to 1:1 mapping, but it is not. On Linux authentication in
  most cases use a user name either by directly asking (e.g. /bin/login)
  or using the current user name (e.g. sudo). So, according to its name
  it would only control if gdm is allowed to ask for an (optional) user
  name.

  If the option is renamed to e.g. --force-1-to-1-mapping to really
  enforce a 1:1 mapping then it would make sense to derived to gdm
  behavior. I.e. if 1:1 mapping is enforce it makes no sense for gdm to
  ask for a user name and if it is not enforced then it makes sense to
  offer and optional user name input field.

* --enable-username-mismatch=Boolean: I think this option can be
  dropped. My test so far show that if a non-matching hint is given on a
  Windows client authentication fails.

* --alternate-attribute=STRING: I think this option isn't needed as
  well. For IPA server-side we should decide on an attribute name and
  add it to the schema for user objects. On the client side the
  attribute name can be taken from the mapping rule.A


certmappingrule.*:

* ISSUERDN: it looks like you want to use issuerName here. In
  certificateRecord it it used with LDAP ordering and I would prefer
  LDAP ordering at all points where we have a choice. Unfortunately in the
  issuer-subject mapping AD dictates X.500 ordering.


LDAP ordering should indeed be preferred, as it is used everywhere else in
IPA. We can convert to/from X.500 ordering where necessary, when possible.



* DOMAINDN: does this refer to the nsslapd-certmap-basedn attribute in
  the example? My intention in the SSSD design-page was to specify the
  domain (as in DNS domain/IPA domain/trusted domain) where the matching
  user should be searched. Different domains might certificates from
  different issuers and some domains might not even use certificates.
  With this information SSSD does not have to search any domain trusted
  by IPA from a given certificate, but look only at domains listed here
  (the attribute should be a multi-value one).

  There are objects in the LDAP tree for each trusted domain which are
  used by SSSD so using a DN syntax would be valid here.


We use domain names rather than DNs to refer to domains everywhere else in
the framework. I don't think this place should be an exception.


I'm fine with domain names as well. In fact I didn't thought of using
DNs for this before I read DOMAINDN on the design page.





* LDAPSEARCHFILTER: I think a separate option is not need. LDAP search
  filters should just be a special kind of mapping rules. I can image in
  syntax like: <LDAPFILTER:(&(cn=%A)(email=%B)(authType=pkinit))>. I
  think the difficult part with the LDAP filters will to define sensible
  templates.


I'm not sure I understand. Could you please elaborate a little bit?


A LDAP search filter which would cover the AD behavior would look like:

(|(altSecurityIdentities=%A%B)(userPrincipalName=%C)(samAccountName=%D))

where

%A: must be replaced with the issuer of the certificate in X.500 order
%B: must be replaced with the subject of the certificate in X.500 order

it would be possible of course to use a specific template here which
would generate the complete search attribute value.

%C: must be replaced by the principal from AD's SAN
szOID_NT_PRINCIPAL_NAME
%D: must b

Re: [Freeipa-devel] Certificate Identity Mapping

2016-12-19 Thread Jan Cholasta
t any type of mapping,
  or a more user-friendly API ...'. I would not say 'or' but 'and' and
  implement both

* ipaCertMappingEnableMismatch and ipaCertMappingAlternateIdAttribute; I
  think both are note needed, see above

* altSecurityIdentities: I would prefer to use a different name and OID.
  Using the same definition as AD would imo imply that it can be used in
  the same way as in AD. But e.g. AD also supports other content like
  KERBEROS:alternative_user_principal@AD.DOMAIN which we will not
  support.

* issuerName vs ipaCAIssuerDN: I would prefer issuerName because it is
  general UTF-8 and not DN syntax (1.3.6.1.4.1.1466.115.121.1.12). Since
  the issuer DN in general will not be a DN from the local LDAP tree I
  think the UTF-8 version fits better.


I think it's worth mentioning that if the attribute used DN syntax and 
matching, we wouldn't have to worry about normalizing the issuer name 
before searching for it, as DS would do that for us.




* nsslapd-certmap-basedn, see DOMAINDN above

* altSecurityIdentities example: X.500 ordering is used by AD here and
  unfortunately I think we have to adopt it at least for this specific
  usage, here is an ldapsearch output from AD:

altSecurityIdentities:
X509:DC=devel,DC=ad,CN=ad-AD-SERVER-CADC=devel,DC
 =ad,CN=Users,CN=t u,E=test.user@email.domain
altSecurityIdentities: X509:O=Red Hat,OU=prod,CN=Certificate
AuthorityDC
 =com,DC=redhat,OU=users,OID.0.9.2342.19200300.100.1.1=sbose,E=sb...@redhat.co
 m,CN=Sumit Bose Sumit Bose

* Certificate Mapping Administrators or re-use Certificate
  Administrators: I would prefer a new 'Certificate Mapping
  Administrators'

* Users can manage their own X.509 certificate mappings? I'm not sure
  here, at the first glance I would say no. How are OTP tokens handled?
  Maybe this would be a candidate for certmappingconfig-* option?


I think a better question is "How is userCertificate handled?"

Anyway, self-service permissions can be enabled/disabled, so there is 
really no need for a new certmappingconfig option.




That's all :-)

bye,
Sumit




--
Jan Cholasta

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


Re: [Freeipa-devel] CSR autogeneration next steps

2016-12-12 Thread Jan Cholasta

On 12.12.2016 13:49, Fraser Tweedale wrote:

(This is a tangential discussion, but...)

On Mon, Dec 12, 2016 at 09:52:02AM +0100, Jan Cholasta wrote:

IMO profile ID should default to caIPAserviceCert on the client as well.


NACK.  Default profile (although fixed at the present time) should
be considered server-side policy.  If we eventually make it
configurable, we don't want older clients overriding it.


I didn't mean the default value should be overriden on the clients, just 
that profile ID should stay optional on the client and use the default 
profile ID when unspecified.




Thanks,
Fraser




--
Jan Cholasta

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


Re: [Freeipa-devel] CSR autogeneration next steps

2016-12-12 Thread Jan Cholasta

On 5.12.2016 16:48, Ben Lipton wrote:

Hi Jan, thanks for the comments.


On 12/05/2016 04:25 AM, Jan Cholasta wrote:

Hi Ben,

On 3.11.2016 00:12, Ben Lipton wrote:

Hi everybody,

Soon I'm going to have to reduce the amount of time I spend on new
development work for the CSR autogeneration project, and I want to leave
the project in as organized a state as possible. So, I'm taking
inventory of the work I've done in order to make sure that what's ready
for review can get reviewed and the ideas that have been discussed get
prototyped or at least recorded so they won't be forgotten.


Thanks, I have some questions and comments, see below.



Code that's ready for review (I will continue to put in as much time as
needed to help get these ready for submission):

- Current PR: https://github.com/freeipa/freeipa/pull/10


How hard would it be to update the PR to use the "new" interface from
the design thread? By this I mean that currently there is a command
(cert_get_requestdata), which creates a CSR from profile id +
principal + helper, but in the design we discussed a command which
creates a CertificationRequestInfo from profile id + principal +
public key.

Internally it could use the OpenSSL helper, no need to implement the
full "new" design. With your build_requestinfo.c code below it looks
like it should be pretty straightforward.


This is probably doable with the cffi, but I'm concerned about
usability. A user can run the current command to get a (reusable)
script, and run the script to get a CSR. It works with keys in both PEM
files and NSS databases already. If we change to outputting a
CertificationRequestInfo, in order to make this usable on the command
line, we'll need:
- An additional tool to sign a CSR given a CertificationRequestInfo (for
both types of key storage).
- A way to extract a SubjectPublicKeyInfo structure from a key within
the ipa command (like [1] but we need it for both types of key storage)
Since as far as I know there's no standard encoding for files containing
only a CertificationRequestInfo or a SubjectPublicKeyInfo, we'll be
writing and distributing these ourselves. I think that's where most of
the extra work will come in.


For PEM files, this is easily doable using python-cryptography (to 
extract SubjectPublicKeyInfo and sign CertificationRequestInfo) and 
PyASN1 (to create a CSR from the CertificationRequestInfo and the 
signature).


For NSS databases, this will be trickier and will require calling C 
functions, as neither certutil nor python-nss provide a way to a) 
address existing keys in the database by key ID b) get 
SubjectPublicKeyInfo for a given key.


As for encoding, the obvious choice is DER. It does not really matter 
there is no standard file format, as we won't be transferring these as 
files anyway.




Would it be ok to stick with the current design in this PR? I'd feel
much better if we could get the basic functionality into the repo and
then iterate on it rather than changing the plan at this point. I can
create a separate PR to change cert_get_requestdata to this new
interface and at the same time add the necessary adapters (bullet points
above) to make it user-friendly.


Works for me.



I would probably just implement the adapters within the
cert_build/cert_request client code unless you think having standalone
tools is valuable. I suppose certmonger is going to need these features
too, but I don't know how well sharing code between them is going to work.


cert-request is exactly the place where it should be :-) I wouldn't 
bother with certmonger until we have a server-side csrgen.






- Allow some fields to be specified by the user at creation time:
https://github.com/LiptonB/freeipa/commits/local-user-data


Good idea :-)



- Automation for the full process from getting CSR data to requesting
cert: https://github.com/LiptonB/freeipa/commits/local-cert-build


LGTM, although I would prefer if this was a client-side extension of
cert-request rather than a completely new command.


I did try that at first, but I struggled to figure out the interface for
the modified cert-request. (Not that the current solution is so great,
what with the copying of options from cert_request and certreq.) If I
remember correctly, I was uncertain how to implement parameters that are
required/invalid based on other parameters: the current cert-request
takes a signed CSR (required), a principal (required), and a profile ID;
the new cert-request (what I implemented as cert-build) takes a
principal (required), a profile ID (required), and a key location
(required). I can't remember if that was the only problem, but I'll try
again to merge the commands and get back to you.


To make the CSR argument optional on the client, you can do this:

def get_options(self):
for option in super(cert_request, self).get_options():
if option.name == 'csr':
option = option.clone(required=False)
yield

IMO profi

Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument

2016-12-06 Thread Jan Cholasta

On 21.11.2016 17:08, Standa Laznicka wrote:

On 10/10/2016 08:47 AM, Standa Laznicka wrote:

On 10/10/2016 07:53 AM, Jan Cholasta wrote:

On 7.10.2016 12:23, Standa Laznicka wrote:

On 10/07/2016 08:31 AM, Jan Cholasta wrote:

On 17.8.2016 13:47, Stanislav Laznicka wrote:

On 08/11/2016 02:59 PM, Stanislav Laznicka wrote:

On 08/11/2016 07:49 AM, Jan Cholasta wrote:

On 2.8.2016 13:47, Stanislav Laznicka wrote:

On 07/19/2016 09:20 AM, Jan Cholasta wrote:

Hi,

On 14.7.2016 14:36, Stanislav Laznicka wrote:

Hello,

This patch fixes https://fedorahosted.org/freeipa/ticket/5640.

With not so much experience with the framework, it raises
question
in my
head whether ipaldap.get_entries is used properly throughout the
system
- does it always assume that it gets ALL the requested
entries or
just a
few of those as configured by the 'ipaSearchRecordsLimit'
attribute of
ipaConfig.etc which it actually gets?


That depends. If you call get_entries() on the ldap2 plugin
(which is
usually the case in the framework), then ipaSearchRecordsLimit is
used. If you call it on some arbitrary LDAPClient instance, the
hardcoded default (= unlimited) is used.



One spot that I know the get_entries method was definitely
not used
properly before this patch is in the
baseldap.LDAPObject.get_memberindirect() method:

 692 result = self.backend.get_entries(
 693 self.api.env.basedn,
 694 filter=filter,
 695 attrs_list=['member'],
 696 size_limit=-1, # paged search will get
everything
anyway
 697 paged_search=True)

which to me seems kind of important if the environment
size_limit
is not
set properly :) The patch does not fix the non-propagation of
the
paged_search, though.


Why do you think size_limit is not used properly here?

AFAIU it is desired that the search is unlimited. However, due
to the
fact that neither size_limit nor paged_search are passed from
ldap2.get_entries() to ldap2.find_entries() (methods inherited
from
LDAPClient), only the number of records specified by
ipaSearchRecordsLimit is returned. That could eventually cause
problems
should ipaSearchRecordsLimit be set to a low value as in the
ticket.


I see. This is *not* intentional, the **kwargs of get_entries()
should be passed to find_entries(). This definitely needs to be
fixed.



Anyway, this ticket is not really easily fixable without more
profound
changes. Often, multiple LDAP searches are done during command
execution. What do you do with the size limit then? Do you
pass the
same size limit to all the searches? Do you subtract the
result size
from the size limit after each search? Do you do something
else with
it? ... The answer is that it depends on the purpose of each
individual LDAP search (like in get_memberindirect() above, we
have to
do unlimited search, otherwise the resulting entry would be
incomplete), and fixing this accross the whole framework is a
non-trivial task.


I do realize that the proposed fix for the permission plugin is
not
perfect, it would probably be better to subtract the number of
currently
loaded records from the sizelimit, although in the end the
number of
returned values will not be higher than the given size_limit.
However,
it seems reasonable that if get_entries is passed a size limit, it
should apply it over current ipaSearchRecordsLimit rather than
ignoring
it. Then, any use of get_entries could be fixed accordingly if
someone
sees fit.



Right. Anyway, this is a different issue than above, so please put
this into a separate commit.


Please see the attached patches, then.


Self-NACK, with Honza's help I found there was a mistake in the
code. I
also found an off-by-one bug which I hope I could stick to the
other two
patches (attaching only the modified and new patches).


Works for me, but this bit in patch 0064 looks suspicious to me:

+if max_entries > 0 and len(entries) ==
max_entries:

Shouldn't it rather be:

+if max_entries > 0 and len(entries) >=
max_entries:

?


The length of entries list should not exceed max_entries if size_limit
is properly implemented. Therefore the list you get from execute()
should not have more then max_entries entries. You shouldn't also be
able to append a legacy entry to entries list if this check is the
first
thing you do.


That's a lot of shoulds :-) I would expect at least an assert
statement to make sure everything is right.



That being said, >= could be used but then some popping of entries from
entries list would be necessary. But it's perhaps safer to do, although
if there's a bug, it won't be that obvious :)


OK, nevermind then, just add the assert please, to make bugs *very*
obvious.


An assert seems like a very good idea, attached is an asserting patch.




Attached is the patch rebased on the current master. Renumbered it a bit
as previous numbers could've been confusing so I also omitted the
revision number.



Re: [Freeipa-devel] [RFC] Matching and Mapping Certificates

2016-12-05 Thread Jan Cholasta

On 25.11.2016 15:55, Sumit Bose wrote:

On Fri, Nov 25, 2016 at 02:19:10PM +0100, Jan Cholasta wrote:

Bump, Sumit, have you seen my comments? I haven't heard back from you.


Yes, I've seen it and added a comment about it on the page
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates#Matching-alternativeRFC4523syntax
To cut it short I would prefer to use a standard, but I think RFC4523
currently does nit meet out needs. But I would be happy if there are
ways to mitigate my concerns.


What I actually had in mind was not to use the full RFC 4523 syntax, but 
rather re-use the concepts used in it - for example, instead of using 
regular expressions to match subject names, we could use a scheme based 
on name constraints, where the subject name is matched using base + 
minimum distance + maximum distance, which could look like this, written 
down using glob-like syntax:


directoryName=CN=a,O=b
(base = CN=a,O=b, minimum distance = 0, maximum distance = 0)

directoryName=*,O=b
(base = O=b, minimum distance = 1, maximum distance = 1)

directoryName=*,*,O=b
(base = O=b, minimum distance = 2, maximum distance = 2)

directoryName=**,*,O=b
(base = O=b, minimum distance = 1, maximum distance unspecified)



I'm working on updating and changing other sections as well and planned
to reply when I'm done with the other sections as well.


OK, thanks for the heads up.



bye,
Sumit



On 17.10.2016 09:50, Jan Cholasta wrote:

Hi,

On 13.10.2016 18:52, Sumit Bose wrote:

On Tue, Oct 11, 2016 at 01:37:09PM +0200, Sumit Bose wrote:

On Thu, Oct 06, 2016 at 12:49:30PM +0200, Sumit Bose wrote:

Hi,

I've started to write a SSSD design page about enhancing the current
mapping of certificates to users and how to select/match a suitable
certificate if multiple certificates are on a Smartcard.

My currently thoughts and idea and be found at
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates

and for your convenience below as well.

Comments and suggestions are welcome. Please let me know about
concerns,
alternatives and missing use-cases/user-stories.

bye,
Sumit



Hi,

Rob, Fraser, Alexander, thank you for your comments. I think both the
issuer specific matching and the OID in the SUBJECT matching are good
ideas. I updated the design page accordingly. The changes can be shown
with
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates?action=diff=9_version=6


The updated version can be found below as well. Of course more
comments and
suggestions are still very welcome.



I did another update. A "Compatibility with Active Director" section is
added which made me realize that there are use-cases for using the
issuer in the mapping as well and the sub-strings in LDAP search filters
might be useful as well.

The changes can be seen with
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates?action=diff=10_version=9


Please let me know your comments and suggestions.

bye,
Sumit

= Matching and Mapping Certificates =

Related ticket(s):
 *
http://www.freeipa.org/page/V4/User_Certificates#Certificate_Identity_Mapping


=== Problem statement ===
 Mapping 
Currently it is required that a certificate used for authentication is
either stored in the LDAP user entry or in a matching override. This
might not always be applicable and other ways are needed to relate a
user with a certificate.

 Matching 
Even if SSSD will support multiple certificates on a Smartcard in the
context of https://fedorahosted.org/sssd/ticket/3050 it might be
necessary to restrict (or relax) the current certificate selection in
certain environments.

=== Use cases ===
 Mapping 
In some environments it might not be possible or would cause unwanted
effort to add certificates to the LDAP entry of the users to allow
Smartcard based authentication. Reasons might be:
* Certificates/Smartcards are issued externally
* LDAP schema extension is not possible or not allowed

 Matching 
A user might have multiple certificate on a Smartcard which are
suitable for authentication. But on some host in the environment only
certificates from a specific CA (while all other CAs are trusted as
well) or with some special extension should be valid for login.

=== Overview of the solution ===
To match a certificate a language/syntax has to be defined which
allows to reference items from the certificate and compare the values
with the expected data. To map the certificates to a user the
language/syntax should allow to relate certificate items with LDAP
attributes so that the value(s) from the certificate item can be used
in a LDAP search filter.


Note that in some cases it might be possible to map a certificate to a
user without having to do an extra LDAP search, for example when the
certificate contains the principal name of the user. Does the design
allow this? Or is there no extra LDAP search?




=== Implementati

Re: [Freeipa-devel] CSR autogeneration next steps

2016-12-05 Thread Jan Cholasta

Hi Ben,

On 3.11.2016 00:12, Ben Lipton wrote:

Hi everybody,

Soon I'm going to have to reduce the amount of time I spend on new
development work for the CSR autogeneration project, and I want to leave
the project in as organized a state as possible. So, I'm taking
inventory of the work I've done in order to make sure that what's ready
for review can get reviewed and the ideas that have been discussed get
prototyped or at least recorded so they won't be forgotten.


Thanks, I have some questions and comments, see below.



Code that's ready for review (I will continue to put in as much time as
needed to help get these ready for submission):

- Current PR: https://github.com/freeipa/freeipa/pull/10


How hard would it be to update the PR to use the "new" interface from 
the design thread? By this I mean that currently there is a command 
(cert_get_requestdata), which creates a CSR from profile id + principal 
+ helper, but in the design we discussed a command which creates a 
CertificationRequestInfo from profile id + principal + public key.


Internally it could use the OpenSSL helper, no need to implement the 
full "new" design. With your build_requestinfo.c code below it looks 
like it should be pretty straightforward.




- Allow some fields to be specified by the user at creation time:
https://github.com/LiptonB/freeipa/commits/local-user-data


Good idea :-)



- Automation for the full process from getting CSR data to requesting
cert: https://github.com/LiptonB/freeipa/commits/local-cert-build


LGTM, although I would prefer if this was a client-side extension of 
cert-request rather than a completely new command.




Other prototypes and design ideas that aren't ready for submission yet:

- Utility written in C to build a CertificationRequestInfo from a
SubjectPublicKeyInfo and an openssl-style config file. The purpose of
this is to take a config that my code already knows how to generate, and
put it in a form that certmonger can use. This is nearly done and
available at:
https://github.com/LiptonB/freeipa-prototypes/blob/master/build_requestinfo.c


Nice! As I said above, this could really make implementing the "new" 
csrgen interface simple.





- Ideally it should be possible to use this tool to reimplement the full
cert-request automation (local-cert-build branch) without a dependency
on the certutil/openssl tools. However, I don't think any of the python
crypto libraries have bindings for the functions that deal with
CertificationRequestInfo objects, so I don't think I can do this in the
short term.


You can use python-cffi to write your own minimal bindings. It's fairly 
straightforward, take a look at FreeIPA commit 500ee7e2 for an example 
of how to port C code to Python with python-cffi.




- Certmonger "helper" program that takes in the CertificationRequestInfo
that certmonger generates, calls out to IPA for profile-specific data,
and returns an updated CertificationRequestInfo built from the data.
Certmonger doesn't currently support this type of helper, but (if I
understood correctly) this is the architecture Nalin believed would be
simplest to fit in. This is not done yet, but I intend to complete it
soon - it shouldn't require much code beyond what's in build_requestinfo.c.


To me this sounds like it should be a new operation of the current 
helper rather than a completely new helper.


Anyway, the ultimate goal is to move the csrgen code to the server, 
which means everything the helper will have to do is call a command over 
RPC.




- Tool to convert an XER-encoded cert extension to DER, given the ASN.1
description of the extension. This would unblock Jan Cholasta's idea of
using XSLT for templates rather than text-based formatting. I should be
able to implement the conversion tool, but it may be a while before I
have time to demo the full XSLT idea.


Was there any progress on this?



So: currently on my to do list are the certmonger helper and the
XER->DER conversion tool. Do you have any comments about these plans,
and is there anything else I can do to wrap up the project neatly?

Thanks,
Ben



Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] NTP in FreeIPA

2016-11-30 Thread Jan Cholasta

On 30.11.2016 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 think there should be no implication, the server is a completely 
different thing.


The only thing I would maybe do is to detect if there is an existing NTP 
server configuration and if there is, do not touch it.




I don't believe there is a precedence for removing a service from IPA.

rob




--
Jan Cholasta

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


Re: [Freeipa-devel] NTP in FreeIPA

2016-11-29 Thread Jan Cholasta

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.




There don't seem to be a ton of NTP tickets and I don't recall a lot of
user's pressing for it to go away (the reverse, many times their
problems revolve around time not being synced). I wonder if a survey on
freeipa-users would be in order to see how hot an issue this really is.

rob




--
Jan Cholasta

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


Re: [Freeipa-devel] [RFC] Matching and Mapping Certificates

2016-11-25 Thread Jan Cholasta

Bump, Sumit, have you seen my comments? I haven't heard back from you.

On 17.10.2016 09:50, Jan Cholasta wrote:

Hi,

On 13.10.2016 18:52, Sumit Bose wrote:

On Tue, Oct 11, 2016 at 01:37:09PM +0200, Sumit Bose wrote:

On Thu, Oct 06, 2016 at 12:49:30PM +0200, Sumit Bose wrote:

Hi,

I've started to write a SSSD design page about enhancing the current
mapping of certificates to users and how to select/match a suitable
certificate if multiple certificates are on a Smartcard.

My currently thoughts and idea and be found at
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates

and for your convenience below as well.

Comments and suggestions are welcome. Please let me know about
concerns,
alternatives and missing use-cases/user-stories.

bye,
Sumit



Hi,

Rob, Fraser, Alexander, thank you for your comments. I think both the
issuer specific matching and the OID in the SUBJECT matching are good
ideas. I updated the design page accordingly. The changes can be shown
with
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates?action=diff=9_version=6


The updated version can be found below as well. Of course more
comments and
suggestions are still very welcome.



I did another update. A "Compatibility with Active Director" section is
added which made me realize that there are use-cases for using the
issuer in the mapping as well and the sub-strings in LDAP search filters
might be useful as well.

The changes can be seen with
https://fedorahosted.org/sssd/wiki/DesignDocs/MatchingAndMappingCertificates?action=diff=10_version=9


Please let me know your comments and suggestions.

bye,
Sumit

= Matching and Mapping Certificates =

Related ticket(s):
 *
http://www.freeipa.org/page/V4/User_Certificates#Certificate_Identity_Mapping


=== Problem statement ===
 Mapping 
Currently it is required that a certificate used for authentication is
either stored in the LDAP user entry or in a matching override. This
might not always be applicable and other ways are needed to relate a
user with a certificate.

 Matching 
Even if SSSD will support multiple certificates on a Smartcard in the
context of https://fedorahosted.org/sssd/ticket/3050 it might be
necessary to restrict (or relax) the current certificate selection in
certain environments.

=== Use cases ===
 Mapping 
In some environments it might not be possible or would cause unwanted
effort to add certificates to the LDAP entry of the users to allow
Smartcard based authentication. Reasons might be:
* Certificates/Smartcards are issued externally
* LDAP schema extension is not possible or not allowed

 Matching 
A user might have multiple certificate on a Smartcard which are
suitable for authentication. But on some host in the environment only
certificates from a specific CA (while all other CAs are trusted as
well) or with some special extension should be valid for login.

=== Overview of the solution ===
To match a certificate a language/syntax has to be defined which
allows to reference items from the certificate and compare the values
with the expected data. To map the certificates to a user the
language/syntax should allow to relate certificate items with LDAP
attributes so that the value(s) from the certificate item can be used
in a LDAP search filter.


Note that in some cases it might be possible to map a certificate to a
user without having to do an extra LDAP search, for example when the
certificate contains the principal name of the user. Does the design
allow this? Or is there no extra LDAP search?




=== Implementation details ===
 Matching 
The pkinit plugin of MIT Kerberos must find a suitable certificate
from a Smartcard as well and has defined the following syntax (see the
pkinit_cert_match section of the krb5.conf man page or
http://web.mit.edu/Kerberos/krb5-1.14/doc/admin/conf_files/krb5_conf.html
for details). The main components are

* regular-expression
* regular-expression
* regular-expression
* extended-key-usage-list
* key-usage-list

and can be grouped together with a prefixed '&&' (and) or '`||`' (or)
operator ('&&' is the default). If multiple rules are given they are
iterated with the order in the config file as long as a rule matches
exactly one certificate.

'''Question: MIT Kerberos use case-sensitive matching and POSIX
Extended Regular Expression syntax, shall we do the same?'''

While  and  are (imo) already quite flexible I can
see some potential extensions for the other components.


I don't think regular expressions are a particularly good choice for DN
matching. It is difficult to express assertions which are quite natural
for DNs (matching multi-attribute RDNs, matching the same attribute type
by different identifiers, respecting the defined matching rules of
attribute types) and at the same time it is easy to express assertions
which do not make much sense for DNs (matching substrings in attribute
names, matching ac

Re: [Freeipa-devel] client-only FreeIPA build

2016-11-22 Thread Jan Cholasta

On 22.11.2016 18:10, Petr Vobornik wrote:

On 11/22/2016 05:25 PM, Rob Crittenden wrote:

Lukas Slebodnik wrote:

On (22/11/16 16:29), Petr Spacek wrote:

On 22.11.2016 16:27, Jan Cholasta wrote:

Hi,

On 22.11.2016 16:04, Petr Spacek wrote:

Hello,

the recent changes with regard to
http://www.freeipa.org/page/V4/Integration_Improvements
beg a question whether we should invest into supporting client-only builds in
FreeIPA build system.


Note that the Integration efforts don't really apply. The client-only
install is for doing client enrollment and integration can mean lots of
things.



Right now, FreeIPA can be built on all architectures we care about so there is
no incentive to invest into client-only build - this applies to binary/RPM
builds.


Client-only build lowers the barrier for porting IPA to new platforms (porting
only client code is *much* easier than porting the whole thing), so I would
very much prefer if we kept it.


Understood.


Agree about portability

But upstream spec file needn't have such relicts.
The upstream spec file is pure fedora specific.


The upstream spec is what is used to document and verify that the
client-only build actually works.

I also think it is a worthy goal to maintain.


Wondering out loud: What prevents the "porter" from doing full build and then
packaging only client bits? Yes, he has to install come of the dependencies
for the build to pass but still, it is way easier than actually making server
fully functional.


It is not an insignificant amount of dependencies to build all of IPA.


Petr, are you going to allocate time for this soonish or should I open a
ticket and forget about it for now?


IMHO this should be covered under the build refactoring to avoid
regressions.


+1



rob



I think we should not implement it. I see no need. Fedora, Debian, RHEL
all work with server build. Difference between arches is not issue as well.


This assumes that these are the only ports out there, which I know for 
fact they are not [1].




If somebody would want it for porting IPA on other distro then fine. But
at that stage there will be more stuff to figure out so writing the
patch can wait for that eventuality.


[1] https://aur.archlinux.org/packages/freeipa-client/

--
Jan Cholasta

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


Re: [Freeipa-devel] client-only FreeIPA build

2016-11-22 Thread Jan Cholasta

On 22.11.2016 16:59, Lukas Slebodnik wrote:

On (22/11/16 16:29), Petr Spacek wrote:

On 22.11.2016 16:27, Jan Cholasta wrote:

Hi,

On 22.11.2016 16:04, Petr Spacek wrote:

Hello,

the recent changes with regard to
http://www.freeipa.org/page/V4/Integration_Improvements
beg a question whether we should invest into supporting client-only builds in
FreeIPA build system.

Right now, FreeIPA can be built on all architectures we care about so there is
no incentive to invest into client-only build - this applies to binary/RPM
builds.


Client-only build lowers the barrier for porting IPA to new platforms (porting
only client code is *much* easier than porting the whole thing), so I would
very much prefer if we kept it.


Understood.


Agree about portability

But upstream spec file needn't have such relicts.


I like to think about the upstream spec file as sort of a template for 
porting, so I can't say I agree. There is no other definitive, 
up-to-date  source of information about what are the dependencies, how 
to properly build IPA for downstream packaging and what needs to be 
executed on package install and upgrade.



The upstream spec file is pure fedora specific.


Almost :-) The actual downstream Fedora spec file differs slightly, and 
the upstream spec file is actually usable on RHEL as well.





Wondering out loud: What prevents the "porter" from doing full build and then
packaging only client bits? Yes, he has to install come of the dependencies
for the build to pass but still, it is way easier than actually making server
fully functional.


The issue with this is that some of the dependencies might not had been 
ported as well, which would leave the porters to either do it 
themselves, which might not be a trivial task, or wait for someone else 
to do it, which might take ages.


Speaking from my own experience, when I was porting IPA client to Arch 
Linux [1], I had to port authconfig first. I had hard time doing it, 
harder than porting IPA client itself. I can't imagine how much harder 
would it be if I had to first port DS and Samba 4 with MIT Kerberos as well.


[1] https://aur.archlinux.org/packages/freeipa-client/

--
Jan Cholasta

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


Re: [Freeipa-devel] client-only FreeIPA build

2016-11-22 Thread Jan Cholasta

Hi,

On 22.11.2016 16:04, Petr Spacek wrote:

Hello,

the recent changes with regard to
http://www.freeipa.org/page/V4/Integration_Improvements
beg a question whether we should invest into supporting client-only builds in
FreeIPA build system.

Right now, FreeIPA can be built on all architectures we care about so there is
no incentive to invest into client-only build - this applies to binary/RPM 
builds.


Client-only build lowers the barrier for porting IPA to new platforms 
(porting only client code is *much* easier than porting the whole 
thing), so I would very much prefer if we kept it.


Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] NTP in FreeIPA

2016-11-22 Thread Jan Cholasta

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


Re: [Freeipa-devel] Removing ipa.pot file from git tree

2016-11-22 Thread Jan Cholasta

On 22.11.2016 13:59, David Kupka wrote:

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.


+1



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




--
Jan Cholasta

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


Re: [Freeipa-devel] Design document: Integration Improvements

2016-11-21 Thread Jan Cholasta

On 21.11.2016 15:25, Jan Cholasta wrote:

On 21.11.2016 15:07, Christian Heimes wrote:

On 2016-11-21 14:44, Petr Spacek wrote:

3.3 ipaplatform auto-configuration

I'm not sure if guessing platform from ID_LIKE is really a good
idea. It
might work fine for centos -> rhel, but in general we can't really
assume it will always work, as the platforms listed in ID_LIKE
might not
be similar enough to the one in ID. I would rather add an ipaplatform
subpackage for every supported platform (including CentOS) than depend
on error-prone guesswork.


Can you show me a real-world example for your statement that ID_LIKE is
error-prone?

Your proposal doesn't scale. There are tons of Debian spins with their
own ID. For example my Raspberry Pi has ID=raspbian and ID_LIKE=debian.
Do you want to maintain an exhaustive list of all Debian and Ubuntu
variants?


Can we agree that it would be much better to get rid of platform
depedency in
client libraries and be done with this philosophical debate?


+1



Yes, that would be my preferable solution, too. But it's a lot of work
and I don't have any spare time to work on a redesign of ipaplatform /
ipalib. Who is going to do it?


I'm going to look into this.



Christian





--
Jan Cholasta

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


Re: [Freeipa-devel] Design document: Integration Improvements

2016-11-21 Thread Jan Cholasta

On 21.11.2016 14:15, Christian Heimes wrote:

On 2016-11-21 13:31, Jan Cholasta wrote:

Hi,

On 11.11.2016 15:25, Christian Heimes wrote:

Hello,

I have released the first version of a new design document. It describes
how I'm going to improve integration of FreeIPA's client libraries
(ipalib, ipapython, ipaclient, ipaplatform) for third party developers.

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


3.1 API for local configuration directory

"Both approaches have some disadvantages. A user must repeat the -e
option in every call to ipa or create a shell alias. It's both tedious
and error-prone."

This is pretty subjective. I don't think it's error-prone at all, since
it is explicit and you always know what confdir value will be used in
the ipa command just by looking at its arguments, as opposed to the
environment variable, which makes the configuration implicit and
depending on *sane* environment and is equivalent to preferring global
variables to function arguments in Python code.


It's not implicit. The env var has to be set explicitly just like you
have to use -e confdir explicitly in every call.


Yes, you need to set it explicitly, but then it is implicitly inherited 
by the command. And just like with global variables, you might have a 
hard time tracking down where it was set and why if you din't set it 
yourself, especially if you are a casual user and not a developer like us.





That being said, this whole section is filled with one-sided "facts" and
simply ignores everything else, which might lead the reader to believe
that the environment variable is something required, while it is in fact
just a nice-to-have convenience feature. A good design should include
both sides of an argument, even if you don't agree with one.

BTW, shell alias works perfectly fine in your virtualenv example above
in the design.


No, it does not work perfectly fine. By default shell aliases are
limited to interactive shells.


Last time I checked virtualenv provided an interactive shell.


My proposal also works with Python
subprocess module, a C program with execve(), Makefile, Ansible local
command, non-interactive shell script...


... which are all more or less write-once, so the env variable provides 
very little benefit over the command line option.





3.2.1 Build and runtime requirements

How are we going to detect and report missing runtime dependencies?
Currently if they are not installed, the code will fail at random places
during execution with an often cryptic error message. I think this is
unacceptable, and since there is no way specify external dependencies
using setuptools (right?), it needs to be done in our code during
package import (or other suitable place).


Instead of detecting missing dependencies, we document requirements and
treat users as adults.


We do all kinds of runtime checks in our commands - are you saying we 
should just remove them all, because the users are adults?



Runtime checks are a performance issue. Since
wheels don't execute code at installation time, we can't check for
missing dependencies during installation.


In other words, we will provide broken packages in PyPI, at least 
compared to our downstream packages. Is this really the normal thing to 
do for PyPI packages with external dependencies?





3.3 ipaplatform auto-configuration

I'm not sure if guessing platform from ID_LIKE is really a good idea. It
might work fine for centos -> rhel, but in general we can't really
assume it will always work, as the platforms listed in ID_LIKE might not
be similar enough to the one in ID. I would rather add an ipaplatform
subpackage for every supported platform (including CentOS) than depend
on error-prone guesswork.


Can you show me a real-world example for your statement that ID_LIKE is
error-prone?

Your proposal doesn't scale. There are tons of Debian spins with their
own ID. For example my Raspberry Pi has ID=raspbian and ID_LIKE=debian.
Do you want to maintain an exhaustive list of all Debian and Ubuntu
variants?


Yes, I'm aware of that, I was hoping we could find some sort of compromise.



Christian




--
Jan Cholasta

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


Re: [Freeipa-devel] Design document: Integration Improvements

2016-11-21 Thread Jan Cholasta

On 21.11.2016 15:07, Christian Heimes wrote:

On 2016-11-21 14:44, Petr Spacek wrote:

3.3 ipaplatform auto-configuration

I'm not sure if guessing platform from ID_LIKE is really a good idea. It
might work fine for centos -> rhel, but in general we can't really
assume it will always work, as the platforms listed in ID_LIKE might not
be similar enough to the one in ID. I would rather add an ipaplatform
subpackage for every supported platform (including CentOS) than depend
on error-prone guesswork.


Can you show me a real-world example for your statement that ID_LIKE is
error-prone?

Your proposal doesn't scale. There are tons of Debian spins with their
own ID. For example my Raspberry Pi has ID=raspbian and ID_LIKE=debian.
Do you want to maintain an exhaustive list of all Debian and Ubuntu
variants?


Can we agree that it would be much better to get rid of platform depedency in
client libraries and be done with this philosophical debate?


+1



Yes, that would be my preferable solution, too. But it's a lot of work
and I don't have any spare time to work on a redesign of ipaplatform /
ipalib. Who is going to do it?

Christian


--
Jan Cholasta

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


Re: [Freeipa-devel] Design document: Integration Improvements

2016-11-21 Thread Jan Cholasta

Hi,

On 11.11.2016 15:25, Christian Heimes wrote:

Hello,

I have released the first version of a new design document. It describes
how I'm going to improve integration of FreeIPA's client libraries
(ipalib, ipapython, ipaclient, ipaplatform) for third party developers.

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


3.1 API for local configuration directory

"Both approaches have some disadvantages. A user must repeat the -e 
option in every call to ipa or create a shell alias. It's both tedious 
and error-prone."


This is pretty subjective. I don't think it's error-prone at all, since 
it is explicit and you always know what confdir value will be used in 
the ipa command just by looking at its arguments, as opposed to the 
environment variable, which makes the configuration implicit and 
depending on *sane* environment and is equivalent to preferring global 
variables to function arguments in Python code.


That being said, this whole section is filled with one-sided "facts" and 
simply ignores everything else, which might lead the reader to believe 
that the environment variable is something required, while it is in fact 
just a nice-to-have convenience feature. A good design should include 
both sides of an argument, even if you don't agree with one.


BTW, shell alias works perfectly fine in your virtualenv example above 
in the design.



3.2.1 Build and runtime requirements

How are we going to detect and report missing runtime dependencies? 
Currently if they are not installed, the code will fail at random places 
during execution with an often cryptic error message. I think this is 
unacceptable, and since there is no way specify external dependencies 
using setuptools (right?), it needs to be done in our code during 
package import (or other suitable place).



3.3 ipaplatform auto-configuration

I'm not sure if guessing platform from ID_LIKE is really a good idea. It 
might work fine for centos -> rhel, but in general we can't really 
assume it will always work, as the platforms listed in ID_LIKE might not 
be similar enough to the one in ID. I would rather add an ipaplatform 
subpackage for every supported platform (including CentOS) than depend 
on error-prone guesswork.



Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] Design document: Integration Improvements

2016-11-21 Thread Jan Cholasta

On 21.11.2016 11:04, Christian Heimes wrote:

On 2016-11-21 10:46, Jan Cholasta wrote:

On 21.11.2016 10:32, Christian Heimes wrote:

On 2016-11-21 10:26, Jan Cholasta wrote:

On 11.11.2016 18:28, Christian Heimes wrote:

On 2016-11-11 17:46, Martin Basti wrote:



On 11.11.2016 15:25, Christian Heimes wrote:

Hello,

I have released the first version of a new design document. It
describes
how I'm going to improve integration of FreeIPA's client libraries
(ipalib, ipapython, ipaclient, ipaplatform) for third party
developers.

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

Regards,
Christian





Hello, I have a few questions:

1) dynamic platform files

Currently all RHEL/fedora-derived platforms work with the same
rhel/fedora packages. How do you want to achieve this with dynamic
platform files, do you want to keep mappings between platforms and
platform file? What about distributions that have in /etc/release
just mess?


I don't use /etc/releases but /etc/os-release. There is no mapping
involved. If a distribution has no /etc/os-release or a messed up
/etc/os-release, then it needs to be fixed by the distribution. It's a
common standard and all relevant distributions support this standard.

RHEL has ID=rhel and no ID_LIKE -> ipaplatform.rhel

Fedora has ID=fedora and no ID_LIKE -> ipaplatform.fedora

CentOS has ID=centos and ID_LIKE="rhel fedora"
-> ipaplatform.rhel

Even my Raspberry has an /etc/os-release with ID=raspbian and
ID_LIKE=debian -> error, soon ipaplatform.debian


There is more to ipaplatform than /etc/os-release offers. How do you
differentiate between e.g. "Debian with SysV init" and "Debian with
systemd"?


Timo,

do you support FreeIPA on Debian variants with SysV init?


This is not an issue of what is supported now, but rather what is
supportable in the future. Even if Debian with SysV init is not
supported ATM, someone might want to add support for it in the future,
and the design should not prevent them from doing so.


My proposal does not prevent sysv init support. In fact it makes it even
easier to support it. In case Debian SysV Init does not have a distinct
ID in /etc/os-release, I can easily add some additional check like

if platform == 'debian' and os.path.realpath('/sbin/init') !=
'/usr/lib/systemd/systemd':
platform = 'debian_sysvinit'


I didn't mean to say it does prevent it, just that it should be noted in 
the design page.


--
Jan Cholasta

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


Re: [Freeipa-devel] Design document: Integration Improvements

2016-11-21 Thread Jan Cholasta

On 21.11.2016 10:32, Christian Heimes wrote:

On 2016-11-21 10:26, Jan Cholasta wrote:

On 11.11.2016 18:28, Christian Heimes wrote:

On 2016-11-11 17:46, Martin Basti wrote:



On 11.11.2016 15:25, Christian Heimes wrote:

Hello,

I have released the first version of a new design document. It
describes
how I'm going to improve integration of FreeIPA's client libraries
(ipalib, ipapython, ipaclient, ipaplatform) for third party developers.

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

Regards,
Christian





Hello, I have a few questions:

1) dynamic platform files

Currently all RHEL/fedora-derived platforms work with the same
rhel/fedora packages. How do you want to achieve this with dynamic
platform files, do you want to keep mappings between platforms and
platform file? What about distributions that have in /etc/release
just mess?


I don't use /etc/releases but /etc/os-release. There is no mapping
involved. If a distribution has no /etc/os-release or a messed up
/etc/os-release, then it needs to be fixed by the distribution. It's a
common standard and all relevant distributions support this standard.

RHEL has ID=rhel and no ID_LIKE -> ipaplatform.rhel

Fedora has ID=fedora and no ID_LIKE -> ipaplatform.fedora

CentOS has ID=centos and ID_LIKE="rhel fedora"
-> ipaplatform.rhel

Even my Raspberry has an /etc/os-release with ID=raspbian and
ID_LIKE=debian -> error, soon ipaplatform.debian


There is more to ipaplatform than /etc/os-release offers. How do you
differentiate between e.g. "Debian with SysV init" and "Debian with
systemd"?


Timo,

do you support FreeIPA on Debian variants with SysV init?


This is not an issue of what is supported now, but rather what is 
supportable in the future. Even if Debian with SysV init is not 
supported ATM, someone might want to add support for it in the future, 
and the design should not prevent them from doing so.


--
Jan Cholasta

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


Re: [Freeipa-devel] Design document: Integration Improvements

2016-11-21 Thread Jan Cholasta

On 11.11.2016 18:28, Christian Heimes wrote:

On 2016-11-11 17:46, Martin Basti wrote:



On 11.11.2016 15:25, Christian Heimes wrote:

Hello,

I have released the first version of a new design document. It describes
how I'm going to improve integration of FreeIPA's client libraries
(ipalib, ipapython, ipaclient, ipaplatform) for third party developers.

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

Regards,
Christian





Hello, I have a few questions:

1) dynamic platform files

Currently all RHEL/fedora-derived platforms work with the same
rhel/fedora packages. How do you want to achieve this with dynamic
platform files, do you want to keep mappings between platforms and
platform file? What about distributions that have in /etc/release just mess?


I don't use /etc/releases but /etc/os-release. There is no mapping
involved. If a distribution has no /etc/os-release or a messed up
/etc/os-release, then it needs to be fixed by the distribution. It's a
common standard and all relevant distributions support this standard.

RHEL has ID=rhel and no ID_LIKE -> ipaplatform.rhel

Fedora has ID=fedora and no ID_LIKE -> ipaplatform.fedora

CentOS has ID=centos and ID_LIKE="rhel fedora"
-> ipaplatform.rhel

Even my Raspberry has an /etc/os-release with ID=raspbian and
ID_LIKE=debian -> error, soon ipaplatform.debian


There is more to ipaplatform than /etc/os-release offers. How do you 
differentiate between e.g. "Debian with SysV init" and "Debian with 
systemd"?





2) if I understand correctly, you want to separate client installer code
and client CLI code. In past we had freeipa-admintools but it was
removed because it was really tightly bounded to installed client. Do
you want to revive it and make it independent?


My proposal does not affect distribution packaging (rpm, deb) at all. It
is purely about Python packaging.

The client installer and client CLI code are already separated. The
Python wheels will only contain what 'python setup.py bdist_wheel' spits
out for ipaclient, ipalib, ipaplatform and ipapython. The 'ipa' CLI is
part of the ipaclient Python package.


3) why instead of environ variable we cannot have specified paths with
priority where IPA config can be located?
For example:
1) ./.ipa.conf
2) ~/.ipa.conf
3) /etc/ipa/default.conf  <-- as last resort


For Ansible, testing etc. I need an arbitrary amount of config
*directories* and full control. I don't like the idea that the current
working directory affects how commands work. It has too many security
implications, e.g. we have to verify that the file belongs to the
current user. The check must be TOCTOU safe, too. Env vars are easier to
control, more secure and less fragile.

Christian






--
Jan Cholasta

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


Re: [Freeipa-devel] [Design Review Request] V4/Automatic_Certificate_Request_Generation

2016-11-07 Thread Jan Cholasta

On 3.11.2016 00:18, Ben Lipton wrote:

On 10/20/2016 03:52 PM, Ben Lipton wrote:

On 10/17/2016 02:16 AM, Jan Cholasta wrote:

On 13.10.2016 17:23, Ben Lipton wrote:

Thank you, this was a really helpful clarification of your point.
Comments below. Once again, I'm sorry I missed the email for so long.

Ben

On 09/05/2016 06:52 AM, Jan Cholasta wrote:

On 27.8.2016 22:40, Ben Lipton wrote:

On 08/25/2016 04:11 PM, Rob Crittenden wrote:

Ben Lipton wrote:

On 08/23/2016 03:54 AM, Jan Cholasta wrote:

On 8.8.2016 22:23, Ben Lipton wrote:

On 07/25/2016 07:45 AM, Jan Cholasta wrote:

On 25.7.2016 13:11, Alexander Bokovoy wrote:

On Mon, 25 Jul 2016, Jan Cholasta wrote:

On 20.7.2016 16:05, Ben Lipton wrote:

Hi,

Thanks very much for the feedback! Some responses below; I
hope
you'll
let me know what you think of my reasoning.


On 07/20/2016 04:20 AM, Jan Cholasta wrote:

Hi,

On 17.6.2016 00:06, Ben Lipton wrote:

On 06/14/2016 08:27 AM, Ben Lipton wrote:

Hello all,

I have written up a design proposal for making certificate
requests
easier to generate when using alternate certificate
profiles:
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation.







The use case for this is described in
https://fedorahosted.org/freeipa/ticket/4899. I will be
working on
implementing this design over the next couple of months.
If you
have
the time and interest, please take a look and share any
comments or
concerns that you have.

Thanks!

Ben


Just a quick update to say that I've created a new document
that
covers
the proposed schema additions in a more descriptive way
(with
diagrams!)
I'm very new to developing with LDAP, so some more
experienced
eyes on
the proposal would be very helpful, even if you don't have
time to
absorb the full design. Please take a look at
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation/Schema







if you have a chance.


I finally had a chance to take a look at this, here are some
comments:

1) I don't like how transformation rules are tied to a
particular
helper and have to be duplicated for each of them. They
should be
generic and work with any helper, as helpers are just an
implementation detail and their resulting data is the same.

In fact, I think I would prefer if the CSR was generated
using
python-cryptography's CertificateSigningRequestBuilder [1]
rather
than
openssl or certutil or any other command line tool.


There are lots of tools that users might want to use to
manage
their
private keys, so I don't know if we can assume that whatever
library we
prefer will actually be able to access the private key to
sign a
CSR,
which is why I thought it would be useful to support more
than
one.


python-cryptography has the notion of backends, which allow
it to
support multiple crypto implementations. Upstream it currently
supports only OpenSSL [2], but some work has been done on
PKCS#11
backend [3], which provides support for HSMs and soft-tokens
(like
NSS
databases).

Alternatively, for NSS databases (and other "simple"
cases), you
can
generate the private key with python-cryptography using the
default
backend, export it to a file and import the file to the target
database, so you don't actually need the PKCS#11 backend for
them.

So, the only thing that's currently lacking is HSM support,
but
given
that we don't support HSMs in IPA nor in certmonger, I don't
think
it's an issue for now.


The
purpose of the mapping rule is to tie together the
transformation
rules
that produce the same data into an object that's
implementation-agnostic, so that profiles referencing those
rules
are
automatically compatible with all the helper options.


They are implementation-agnostic, as long as you consider
`openssl`
and `certutil` the only implementations :-) But I don't think
this
solution scales well to other possible implementations.

Anyway, my main grudge is that the transformation rules
shouldn't
really be stored on and processed by the server. The server
should
know the *what* (mapping rules), but not the *how*
(transformation
rules). The *how* is an implementation detail and does not
change in
time, so there's no benefit in handling it on the server. It
should be
handled exclusively on the client, which I believe would also
make
the
whole thing more robust (it would not be possible for a bug on
the
server to break all the clients).

This is a good point. However, for the scope of Ben's project
can we
limit it by openssl and certutil support? Otherwise Ben
wouldn't be
able
to complete the project in time.


I'm fine with that, but I don't think it's up to me :-)




This is turning out to be a common (and, I think, reasonable)
reaction
to the proposal. It is rather complex, and I worry that it
will be
difficult to configure. On the other hand, there is some
hidden
complexity to enabling a simpler config format, as well.
One of
the
goals of the project as it was presented to me was to
allow the
creation
of profiles that add certificate 

Re: [Freeipa-devel] [RFC] Matching and Mapping Certificates

2016-10-17 Thread Jan Cholasta

On 17.10.2016 16:50, Rob Crittenden wrote:

Jan Cholasta wrote:

Hi,

On 13.10.2016 18:52, Sumit Bose wrote:

= Issuer specific matching =
Although the MIT Kerberos rules allow to select the issuer of a
certificate there are use cases where a more specific selection is
needed. E.g. if there are some default matching rules for all issuers
and some other issuer specific rules where the default rules should
not apply. To make this possible with the above scheme the default
rules must have an  clause which matches all but the issuer
with the specific rules. Writing regular-expressions to not match a
specific string or a list of strings is at least error-prone if not
impossible.

To make it easier to define issuer specific rules and default rules at
the same time and optional issuer string can be added to the rule to
indicate that for the given issuer only those rules should be
considered. Given the use-case I think it is acceptable to require
that the full issuer must be specified here in LDAP order (see below)
and case-sensitive matching is used.


This could also be solved by adding priority to rules - if two rules
match, the one with higher priority (the issuer specific rule) is
preferred over the one with lower priority (the default rule). IMO this
is better than an optional issuer string as it offers greater
flexibility.


The use cases I've seen haven't had to do with priority, though that
would be a nice enhancement, but with only allowing certificates issued
by a specific CA to be allowed (this is pretty common in web servers).
Being able to say "only do the matching on certificates issued by foo"
is valuable.


Sure, I'm not suggesting that matching by issuer should be removed, only 
that rule precedence should not be determined by the issuer field setting.


--
Jan Cholasta

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


Re: [Freeipa-devel] [RFC] Matching and Mapping Certificates

2016-10-17 Thread Jan Cholasta
{{{
(&(altSecurityIdentities=*Red Hat*)(altSecurityIdentities=*Sumit Bose Sumit 
Bose*))
}}}
which should quite reliable find the right LDAP entry.

As an alternative it would be possible to add special mapping rules like 
 which would try in a best effort to produce 
the exact attribute value AD is using. This should work reliable with standard RDN 
types (see above). I think an optional 'ldapAttributeName' is useful here so that the 
same mapping rule can be used with different LDAP servers (e.g. IPA) where 
user-specific mapping attributes are used with the same content but a different 
attribute name.

According to the blob post describing altSecurityIdentities some other 
additional mapping rules might be useful too. This will give us
* 
* 
* 
* 
* 
* 

So far I didn't found a AD tool which creates to other mappings, if you know 
one, please let me know.
=== Configuration changes ===
Does your feature involve changes to configuration, like new options or options 
changing values? Summarize them here. There's no need to go into too many 
details, that's what man pages are for.

=== How To Test ===
This section should explain to a person with admin-level of SSSD understanding 
how this change affects run time behaviour of SSSD and how can an SSSD user 
test this change. If the feature is internal-only, please list what areas of 
SSSD are affected so that testers know where to focus.

=== How To Debug ===
Explain how to debug this feature if something goes wrong. This section might 
include examples of additional commands the user might run (such as keytab or 
certificate sanity checks) or explain what message to look for.

=== Authors ===
Give credit to authors of the design in this section.



Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] [Design Review Request] V4/Automatic_Certificate_Request_Generation

2016-10-17 Thread Jan Cholasta

On 13.10.2016 17:23, Ben Lipton wrote:

Thank you, this was a really helpful clarification of your point.
Comments below. Once again, I'm sorry I missed the email for so long.

Ben

On 09/05/2016 06:52 AM, Jan Cholasta wrote:

On 27.8.2016 22:40, Ben Lipton wrote:

On 08/25/2016 04:11 PM, Rob Crittenden wrote:

Ben Lipton wrote:

On 08/23/2016 03:54 AM, Jan Cholasta wrote:

On 8.8.2016 22:23, Ben Lipton wrote:

On 07/25/2016 07:45 AM, Jan Cholasta wrote:

On 25.7.2016 13:11, Alexander Bokovoy wrote:

On Mon, 25 Jul 2016, Jan Cholasta wrote:

On 20.7.2016 16:05, Ben Lipton wrote:

Hi,

Thanks very much for the feedback! Some responses below; I hope
you'll
let me know what you think of my reasoning.


On 07/20/2016 04:20 AM, Jan Cholasta wrote:

Hi,

On 17.6.2016 00:06, Ben Lipton wrote:

On 06/14/2016 08:27 AM, Ben Lipton wrote:

Hello all,

I have written up a design proposal for making certificate
requests
easier to generate when using alternate certificate profiles:
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation.






The use case for this is described in
https://fedorahosted.org/freeipa/ticket/4899. I will be
working on
implementing this design over the next couple of months.
If you
have
the time and interest, please take a look and share any
comments or
concerns that you have.

Thanks!

Ben


Just a quick update to say that I've created a new document
that
covers
the proposed schema additions in a more descriptive way (with
diagrams!)
I'm very new to developing with LDAP, so some more experienced
eyes on
the proposal would be very helpful, even if you don't have
time to
absorb the full design. Please take a look at
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation/Schema






if you have a chance.


I finally had a chance to take a look at this, here are some
comments:

1) I don't like how transformation rules are tied to a
particular
helper and have to be duplicated for each of them. They
should be
generic and work with any helper, as helpers are just an
implementation detail and their resulting data is the same.

In fact, I think I would prefer if the CSR was generated using
python-cryptography's CertificateSigningRequestBuilder [1]
rather
than
openssl or certutil or any other command line tool.


There are lots of tools that users might want to use to manage
their
private keys, so I don't know if we can assume that whatever
library we
prefer will actually be able to access the private key to sign a
CSR,
which is why I thought it would be useful to support more than
one.


python-cryptography has the notion of backends, which allow it to
support multiple crypto implementations. Upstream it currently
supports only OpenSSL [2], but some work has been done on PKCS#11
backend [3], which provides support for HSMs and soft-tokens
(like
NSS
databases).

Alternatively, for NSS databases (and other "simple" cases), you
can
generate the private key with python-cryptography using the
default
backend, export it to a file and import the file to the target
database, so you don't actually need the PKCS#11 backend for
them.

So, the only thing that's currently lacking is HSM support, but
given
that we don't support HSMs in IPA nor in certmonger, I don't
think
it's an issue for now.


The
purpose of the mapping rule is to tie together the
transformation
rules
that produce the same data into an object that's
implementation-agnostic, so that profiles referencing those
rules
are
automatically compatible with all the helper options.


They are implementation-agnostic, as long as you consider
`openssl`
and `certutil` the only implementations :-) But I don't think
this
solution scales well to other possible implementations.

Anyway, my main grudge is that the transformation rules shouldn't
really be stored on and processed by the server. The server
should
know the *what* (mapping rules), but not the *how*
(transformation
rules). The *how* is an implementation detail and does not
change in
time, so there's no benefit in handling it on the server. It
should be
handled exclusively on the client, which I believe would also
make
the
whole thing more robust (it would not be possible for a bug on
the
server to break all the clients).

This is a good point. However, for the scope of Ben's project
can we
limit it by openssl and certutil support? Otherwise Ben
wouldn't be
able
to complete the project in time.


I'm fine with that, but I don't think it's up to me :-)




This is turning out to be a common (and, I think, reasonable)
reaction
to the proposal. It is rather complex, and I worry that it
will be
difficult to configure. On the other hand, there is some hidden
complexity to enabling a simpler config format, as well. One of
the
goals of the project as it was presented to me was to allow the
creation
of profiles that add certificate extensions *that FreeIPA
doesn't
yet
know about*. With the current proposal, one only has to add a
rule
generating text that 

Re: [Freeipa-devel] Feature branches for sub-team efforts

2016-10-11 Thread Jan Cholasta

Hi,

On 11.10.2016 15:11, 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.


+1



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)


Personally I would just keep them there, but I don't really care.



2. Too big traffic on a list.
- IMO we actually want it - progress will be visible to others


+1




Naming:
I propose:
  refactoring-XXX
  feature-XXX


I would be perfectly fine with just XXX, but again I don't really care.



Thoughts? Anyone against?



Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] Build system refactoring - design document

2016-10-11 Thread Jan Cholasta

On 11.10.2016 09:36, Petr Spacek wrote:

On 11.10.2016 09:00, Jan Cholasta wrote:

Hi,

On 7.10.2016 11:56, Petr Spacek wrote:

Dear FreeIPA developers and packagers,

you can find first version of the Build system refactoring design document on:
http://www.freeipa.org/page/V4/Build_system_refactoring

If you do not care about implementation details, please be so kind and quickly
scan through chapter
http://www.freeipa.org/page/V4/Build_system_refactoring#Feature_Management

I'm not an FreeIPA packager so I might miss some important thing which needs
to be configurable.


1) There should be a --with-python switch to select the version of Python to
use in our command line tools and/or during build. The default would be
"python", i.e. the default Python interpreter found in the path.


Okay. Can we pick some descriptive name?

--with-default-python
or
--with--python?

I think that it would be confusing if we had just
--with-python
--with-python2
--with-python3


If the default values are "python", "python2", "python3" respectively, I 
don't think it would be confusing, since most of the time you only need 
to specify --with-python, if anything.


Do we even need --with-python2 and --with-python3? I think they would 
only make sense if you had multiple Python minor versions installed and 
wanted to make packages for all of them.





Besides that, I would make --with-default-python to accept either "2" or "3"
(and thus use path specified by --with-python? option).




2) There is --with-pylint, --with-jslint, but no --with-po-validate.


Let me clarify: I plan to use --with for things which have paths or other
parameters, --enable for booleans.


I see, that was not clear to me, I confused the two.



Where po-validate belongs? AFAIK target validate-po in install/po/Makefile is
calling script ../../ipatests/i18n.py which is in IPA source tree anyway.

Do you want to have a --enable/--disable switch for these PO checks?


Not really.





3) I would prefer that if pylint (or jslint or python-polib) is not installed
the build would fail instead of silently skipping the lint. Let it be a wilful
decision of the packager whether to run the lint or not.


Yes, that is my intent. It will not skip anything automatically.


Right.






4) It is explicitly stated that I can turn off features using
--without-feature. But how do I disable building server components?


I've added explicit mention of --disable-feature:
http://www.freeipa.org/index.php?title=V4%2FBuild_system_refactoring=revision=14311=14310


Thanks.





Also, I would appreciate ideas how to handle build versioning:
http://www.freeipa.org/page/V4/Build_system_refactoring#Versioning

My main questions are:
* What is triggering IPA upgrade?
* Would it be sufficient to bump release in RPM? (I mean - theoretically.
Could the code be modified to detect this?)

Here I'm trying to avoid unnecessary rebuilds caused by changes to
IPA_VENDOR_VERSION during each build.


How exactly is IPA_VENDOR_VERSION causing unnecessary rebuilds? I can see it
is written only to ipapython/version.py:

$ git grep -E '\bIPA_VENDOR_VERSION\b'
Makefile:IPA_VENDOR_VERSION=$(IPA_VERSION)$(IPA_VENDOR_VERSION_SUFFIX)
Makefile:   sed -i -e "s:__VENDOR_VERSION__:$(IPA_VENDOR_VERSION):"
ipapython/version.py


My bad, I should write 'IPA*VERSION*'.

Especially unconditional write to version.m4 is problematic but unconditional
writes to other files slows things as well, just not that much.


Could this be worked around by writing into a temporary file, comparing 
it with the real file and mv'ing the temporary file over the real file 
only if the differ?


--
Jan Cholasta

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


Re: [Freeipa-devel] Build system refactoring - design document

2016-10-11 Thread Jan Cholasta

Hi,

On 7.10.2016 11:56, Petr Spacek wrote:

Dear FreeIPA developers and packagers,

you can find first version of the Build system refactoring design document on:
http://www.freeipa.org/page/V4/Build_system_refactoring

If you do not care about implementation details, please be so kind and quickly
scan through chapter
http://www.freeipa.org/page/V4/Build_system_refactoring#Feature_Management

I'm not an FreeIPA packager so I might miss some important thing which needs
to be configurable.


1) There should be a --with-python switch to select the version of 
Python to use in our command line tools and/or during build. The default 
would be "python", i.e. the default Python interpreter found in the path.


2) There is --with-pylint, --with-jslint, but no --with-po-validate.

3) I would prefer that if pylint (or jslint or python-polib) is not 
installed the build would fail instead of silently skipping the lint. 
Let it be a wilful decision of the packager whether to run the lint or not.


4) It is explicitly stated that I can turn off features using 
--without-feature. But how do I disable building server components?





Also, I would appreciate ideas how to handle build versioning:
http://www.freeipa.org/page/V4/Build_system_refactoring#Versioning

My main questions are:
* What is triggering IPA upgrade?
* Would it be sufficient to bump release in RPM? (I mean - theoretically.
Could the code be modified to detect this?)

Here I'm trying to avoid unnecessary rebuilds caused by changes to
IPA_VENDOR_VERSION during each build.


How exactly is IPA_VENDOR_VERSION causing unnecessary rebuilds? I can 
see it is written only to ipapython/version.py:


$ git grep -E '\bIPA_VENDOR_VERSION\b'
Makefile:IPA_VENDOR_VERSION=$(IPA_VERSION)$(IPA_VENDOR_VERSION_SUFFIX)
Makefile:   sed -i -e "s:__VENDOR_VERSION__:$(IPA_VENDOR_VERSION):" 
ipapython/version.py


Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 0497] Py3: fix unicode/str error in LDAP*ReverseMember

2016-10-09 Thread Jan Cholasta

On 7.6.2016 10:35, Martin Basti wrote:



On 07.06.2016 10:35, Jan Cholasta wrote:

On 7.6.2016 10:29, Martin Basti wrote:



On 07.06.2016 09:08, Jan Cholasta wrote:

On 6.6.2016 14:33, Martin Basti wrote:

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

Patch attached.


Could we drop the error message parsing and do something sane instead?



Not now, we can do it later and push this patch just as workaround


What's the point of that?


Point is that it will work as before, and I don't have to time to fix it
nicely now.


Bump. Do you now have time to fix it nicely, or should we move the 
ticket to 4.5 or later?


--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument

2016-10-09 Thread Jan Cholasta

On 7.10.2016 12:23, Standa Laznicka wrote:

On 10/07/2016 08:31 AM, Jan Cholasta wrote:

On 17.8.2016 13:47, Stanislav Laznicka wrote:

On 08/11/2016 02:59 PM, Stanislav Laznicka wrote:

On 08/11/2016 07:49 AM, Jan Cholasta wrote:

On 2.8.2016 13:47, Stanislav Laznicka wrote:

On 07/19/2016 09:20 AM, Jan Cholasta wrote:

Hi,

On 14.7.2016 14:36, Stanislav Laznicka wrote:

Hello,

This patch fixes https://fedorahosted.org/freeipa/ticket/5640.

With not so much experience with the framework, it raises question
in my
head whether ipaldap.get_entries is used properly throughout the
system
- does it always assume that it gets ALL the requested entries or
just a
few of those as configured by the 'ipaSearchRecordsLimit'
attribute of
ipaConfig.etc which it actually gets?


That depends. If you call get_entries() on the ldap2 plugin
(which is
usually the case in the framework), then ipaSearchRecordsLimit is
used. If you call it on some arbitrary LDAPClient instance, the
hardcoded default (= unlimited) is used.



One spot that I know the get_entries method was definitely not used
properly before this patch is in the
baseldap.LDAPObject.get_memberindirect() method:

 692 result = self.backend.get_entries(
 693 self.api.env.basedn,
 694 filter=filter,
 695 attrs_list=['member'],
 696 size_limit=-1, # paged search will get
everything
anyway
 697 paged_search=True)

which to me seems kind of important if the environment size_limit
is not
set properly :) The patch does not fix the non-propagation of the
paged_search, though.


Why do you think size_limit is not used properly here?

AFAIU it is desired that the search is unlimited. However, due to the
fact that neither size_limit nor paged_search are passed from
ldap2.get_entries() to ldap2.find_entries() (methods inherited from
LDAPClient), only the number of records specified by
ipaSearchRecordsLimit is returned. That could eventually cause
problems
should ipaSearchRecordsLimit be set to a low value as in the ticket.


I see. This is *not* intentional, the **kwargs of get_entries()
should be passed to find_entries(). This definitely needs to be fixed.



Anyway, this ticket is not really easily fixable without more
profound
changes. Often, multiple LDAP searches are done during command
execution. What do you do with the size limit then? Do you pass the
same size limit to all the searches? Do you subtract the result size
from the size limit after each search? Do you do something else with
it? ... The answer is that it depends on the purpose of each
individual LDAP search (like in get_memberindirect() above, we
have to
do unlimited search, otherwise the resulting entry would be
incomplete), and fixing this accross the whole framework is a
non-trivial task.


I do realize that the proposed fix for the permission plugin is not
perfect, it would probably be better to subtract the number of
currently
loaded records from the sizelimit, although in the end the number of
returned values will not be higher than the given size_limit.
However,
it seems reasonable that if get_entries is passed a size limit, it
should apply it over current ipaSearchRecordsLimit rather than
ignoring
it. Then, any use of get_entries could be fixed accordingly if
someone
sees fit.



Right. Anyway, this is a different issue than above, so please put
this into a separate commit.


Please see the attached patches, then.


Self-NACK, with Honza's help I found there was a mistake in the code. I
also found an off-by-one bug which I hope I could stick to the other two
patches (attaching only the modified and new patches).


Works for me, but this bit in patch 0064 looks suspicious to me:

+if max_entries > 0 and len(entries) ==
max_entries:

Shouldn't it rather be:

+if max_entries > 0 and len(entries) >=
max_entries:

?


The length of entries list should not exceed max_entries if size_limit
is properly implemented. Therefore the list you get from execute()
should not have more then max_entries entries. You shouldn't also be
able to append a legacy entry to entries list if this check is the first
thing you do.


That's a lot of shoulds :-) I would expect at least an assert statement 
to make sure everything is right.




That being said, >= could be used but then some popping of entries from
entries list would be necessary. But it's perhaps safer to do, although
if there's a bug, it won't be that obvious :)


OK, nevermind then, just add the assert please, to make bugs *very* obvious.

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument

2016-10-07 Thread Jan Cholasta

On 17.8.2016 13:47, Stanislav Laznicka wrote:

On 08/11/2016 02:59 PM, Stanislav Laznicka wrote:

On 08/11/2016 07:49 AM, Jan Cholasta wrote:

On 2.8.2016 13:47, Stanislav Laznicka wrote:

On 07/19/2016 09:20 AM, Jan Cholasta wrote:

Hi,

On 14.7.2016 14:36, Stanislav Laznicka wrote:

Hello,

This patch fixes https://fedorahosted.org/freeipa/ticket/5640.

With not so much experience with the framework, it raises question
in my
head whether ipaldap.get_entries is used properly throughout the
system
- does it always assume that it gets ALL the requested entries or
just a
few of those as configured by the 'ipaSearchRecordsLimit'
attribute of
ipaConfig.etc which it actually gets?


That depends. If you call get_entries() on the ldap2 plugin (which is
usually the case in the framework), then ipaSearchRecordsLimit is
used. If you call it on some arbitrary LDAPClient instance, the
hardcoded default (= unlimited) is used.



One spot that I know the get_entries method was definitely not used
properly before this patch is in the
baseldap.LDAPObject.get_memberindirect() method:

 692 result = self.backend.get_entries(
 693 self.api.env.basedn,
 694 filter=filter,
 695 attrs_list=['member'],
 696 size_limit=-1, # paged search will get
everything
anyway
 697 paged_search=True)

which to me seems kind of important if the environment size_limit
is not
set properly :) The patch does not fix the non-propagation of the
paged_search, though.


Why do you think size_limit is not used properly here?

AFAIU it is desired that the search is unlimited. However, due to the
fact that neither size_limit nor paged_search are passed from
ldap2.get_entries() to ldap2.find_entries() (methods inherited from
LDAPClient), only the number of records specified by
ipaSearchRecordsLimit is returned. That could eventually cause problems
should ipaSearchRecordsLimit be set to a low value as in the ticket.


I see. This is *not* intentional, the **kwargs of get_entries()
should be passed to find_entries(). This definitely needs to be fixed.



Anyway, this ticket is not really easily fixable without more profound
changes. Often, multiple LDAP searches are done during command
execution. What do you do with the size limit then? Do you pass the
same size limit to all the searches? Do you subtract the result size
from the size limit after each search? Do you do something else with
it? ... The answer is that it depends on the purpose of each
individual LDAP search (like in get_memberindirect() above, we have to
do unlimited search, otherwise the resulting entry would be
incomplete), and fixing this accross the whole framework is a
non-trivial task.


I do realize that the proposed fix for the permission plugin is not
perfect, it would probably be better to subtract the number of
currently
loaded records from the sizelimit, although in the end the number of
returned values will not be higher than the given size_limit. However,
it seems reasonable that if get_entries is passed a size limit, it
should apply it over current ipaSearchRecordsLimit rather than ignoring
it. Then, any use of get_entries could be fixed accordingly if someone
sees fit.



Right. Anyway, this is a different issue than above, so please put
this into a separate commit.


Please see the attached patches, then.


Self-NACK, with Honza's help I found there was a mistake in the code. I
also found an off-by-one bug which I hope I could stick to the other two
patches (attaching only the modified and new patches).


Works for me, but this bit in patch 0064 looks suspicious to me:

+if max_entries > 0 and len(entries) == max_entries:

Shouldn't it rather be:

+if max_entries > 0 and len(entries) >= max_entries:

?

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0097 Add options to write lightweight CA cert or chain to file

2016-10-06 Thread Jan Cholasta

On 23.9.2016 05:29, Fraser Tweedale wrote:

Bump for review.

Rebased patches attached (there was a trivial conflict in imports).

Thanks,
Fraser

On Tue, Sep 06, 2016 at 02:05:06AM +1000, Fraser Tweedale wrote:

On Fri, Aug 26, 2016 at 10:28:58AM +0200, Jan Cholasta wrote:

On 19.8.2016 13:11, Fraser Tweedale wrote:

Bump for review.

On Wed, Aug 17, 2016 at 12:09:39AM +1000, Fraser Tweedale wrote:

On Tue, Aug 16, 2016 at 08:10:08AM +0200, Jan Cholasta wrote:

On 16.8.2016 07:24, Fraser Tweedale wrote:

On Mon, Aug 15, 2016 at 08:19:33AM +0200, Jan Cholasta wrote:

On 9.8.2016 16:47, Fraser Tweedale wrote:

On Mon, Aug 08, 2016 at 10:49:27AM +0200, Jan Cholasta wrote:

On 8.8.2016 09:06, Fraser Tweedale wrote:

On Mon, Aug 08, 2016 at 08:54:05AM +0200, Jan Cholasta wrote:

Hi,

On 8.8.2016 06:34, Fraser Tweedale wrote:

Please review the attached patch with adds --certificate-out and
--certificate-chain-out options to `ca-show' command.

Note that --certificate-chain-out currently writes a bogus file due
to a bug in Dogtag that will be fixed in this week's build.

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


1) The client-side *-out options should be defined on the client side, not
on the server side.


Will option defined on client side be propagated to, and observable
in the ipaserver plugin?  The ipaserver plugin needs to observe that
*-out has been requested and executes additional command(s) on that
basis.


Is there a reason not to *always* return the certs?


We hit Dogtag to retrieve them.


I don't think that's an issue in a -show command.


cert_show is invoked by other commands (cert_find*, cert_show,
cert_request, cert_status, ca_del) but these all hit Dogtag anyway
so I suppose that's fine.  I'll return the cert *and* the chain in
separate attributes, unconditionally.







2) I don't think there should be additional information included in summary
(and it definitely should not be multi-line). I would rather inform the user
via an error message when unable to write the files.


I was just following the pattern of other commands that write certs,
profile config, etc.  Apart from consistency with other commands I
agree that there is no need to have it.  So I will remove it.


If you think there is an actual value in informing the user about
successfully writing the files, please use ipalib.messages for the job.


3) IMO a better format for the certificate chain than PKCS#7 would be
concatenated PEM, as that's the most commonly used format in IPA (in
installers, there are no cert chains in API commands ATM).


Sure, but the main use case isn't IPA.  Other apps require PKCS #7
or concatenated PEMs, but sometimes they must be concatenated
forward, and othertimes backwards.  There is no one size fits all.


True, which is exactly why I think we should at least be self-consistent and
use concatenated PEM (and multi-value DER over the wire).


Dogtag returns a PKCS7 (either DER or PEM, according to HTTP Accept
header).

If we want list-of-PEMs between server and client we have to convert
on the server.  Do we have a good way of doing this without exec'ing
`openssl pkcs7' on the server?  Is it acceptable to exec 'openssl'
to do the conversion on the server?  python-nss does not have PKCS7
functions and I am not keen on adding a pyasn1 PKCS7 parser just for
the sake of pushing bits as list-of-PEMs.


I'm afraid we can't avoid conversion to/from PKCS#7 one way or the other.
For example, if we added a call to retrieve external CA chain using certs
from cn=certificates,cn=ipa,cn=etc, we would have to convert the result to
PKCS#7 if it was our cert chain format of choice.

What we can avoid though is executing "openssl pkcs7" to do the conversion -
we can use an approach similar to our DNSSEC code and use python-cffi to
call libcrypto's PKCS#7 conversion routines instead.


I had a look at the OpenSSL API for parsing PKCS #7; now I prefer to
exec `openssl' to do the job :)

I will transmit DER-encoded PKCS #7 object on the wire; we cannot
used multi-valued DER attribute because order is important.   Client
will convert to PEMs.


Well, my point was not to send PKCS#7 over the wire, so that clients
(including 3rd party clients) do not have to convert from PKCS#7 themselves.

In fact we can use multi-valued DER - whatever you send over the wire from
the server will be received in the exact same order by the client. Even if
it wasn't, you can easily restore the order by matching issuer and subject
names of the certificates.



Should have new patch on list this afternoon.

Thanks,
Fraser



FWIW, man pages and code suggest that PKCS #7 is accepted in
installer, etc.


True, but that's a relatively new feature (since 4.1) and the installer
internally executes "openssl pkcs7" to convert PKCS #7 to list of certs :-)




We can add an option to control the format later, but for now,
Dogtag returns a PKCS #7 (PEM or DER) so let's go with that.  Worst
case is an admin has to invoke `opens

Re: [Freeipa-devel] pylint: remove unused variables

2016-09-23 Thread Jan Cholasta

On 23.9.2016 13:23, Standa Laznicka wrote:

On 09/23/2016 07:28 AM, Jan Cholasta wrote:

On 22.9.2016 16:39, Martin Basti wrote:

Hello all,

In 4.5, I would like to remove all unused variables from code and enable
pylint check. Due to big amount of unused variables in the code this
will be longterm effort.

Why this?:

* better code readability

* removing dead code

* unused variable may uncover potential bug


It is clear what to do with unused assignments, but I need an agreement
what to do with unpacking or iteration with unused variables


For example:

for name, surname, gender in (('Martin', 'Basti', 'M'), ):

name, surname, gender = user['mbasti']

Where 'surname' is unused


Pylint will detect surname as unused variable and we have to agree on a
way how to tell pylint that this variable is unused on purpose:


1)

(

   name,

   surname,  # pylint: disable=unused-variable

   gender

) = user['mbasti']


I dont like this approach


+1




2)

Use defined keyword: 'dummy' is default in pylint, we can set our own,
like ignored, unused

name, dummy, gender = user['mbasti']


-1, not visible enough.




3)

use a prefix for unused variables: '_' or 'ignore_'

name, _surname, gender = user['mbasti']


This. We have already been using it in new code for quite some time,
and it's common in other Python projects too. Don't reinvent the wheel.




4)

we can combine all :)


For me the best is to have prefix '_' and 'dummy' keyword


Use '_dummy', it's both :-)


+1. I would rather use _meh as it's easier to type but perhaps not that
self-explanatory and not established at all, so _dummy is just fine :)


What I'm actually suggesting is that any local variable with "_" prefix 
should be considered unused, so _meh would be fine as well.


--
Jan Cholasta

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


Re: [Freeipa-devel] pylint: remove unused variables

2016-09-23 Thread Jan Cholasta

On 23.9.2016 10:40, Petr Spacek wrote:

On 23.9.2016 07:28, Jan Cholasta wrote:

On 22.9.2016 16:39, Martin Basti wrote:

Hello all,

In 4.5, I would like to remove all unused variables from code and enable
pylint check. Due to big amount of unused variables in the code this
will be longterm effort.

Why this?:

* better code readability

* removing dead code

* unused variable may uncover potential bug


It is clear what to do with unused assignments, but I need an agreement
what to do with unpacking or iteration with unused variables


For example:

for name, surname, gender in (('Martin', 'Basti', 'M'), ):

name, surname, gender = user['mbasti']

Where 'surname' is unused


Pylint will detect surname as unused variable and we have to agree on a
way how to tell pylint that this variable is unused on purpose:


1)

(

   name,

   surname,  # pylint: disable=unused-variable

   gender

) = user['mbasti']


I dont like this approach


+1




2)

Use defined keyword: 'dummy' is default in pylint, we can set our own,
like ignored, unused

name, dummy, gender = user['mbasti']


-1, not visible enough.




3)

use a prefix for unused variables: '_' or 'ignore_'

name, _surname, gender = user['mbasti']


This. We have already been using it in new code for quite some time, and it's
common in other Python projects too. Don't reinvent the wheel.




4)

we can combine all :)


For me the best is to have prefix '_' and 'dummy' keyword


Use '_dummy', it's both :-)


I like "__".  If it is not acceptable for rest of the team, I would be okay
with _dummy.


I'm not a fan of "__" (because it's as "brilliant" as "___" or ""), 
but if any local variable with "_" prefix is considered unused (i.e. 
what I'm proposing), it would be perfectly legal.



I would even use _dummy multiple times:

name, _dummy, _dummy = user['mbasti']

so namespace is not polluted and garbage collector can do the right thing.


This is a pretty misguided argument if I may say so. First, I don't see 
how locals namespace pollution could ever cause any issues, and even if 
it did, the proper way to avoid it is to not write long functions with 
many local variables. Second, removing a local variable does not 
guarantee that the garbage collector will do anything (because garbage 
collection is not always deterministic), and even if it did, you should 
be explicit about it and use the del statement.




Petr^2 Spacek



As first step I'll enable pylint check and disable it locally per module
where unused variables are, to avoid new regressions. Then I will fix it
module by module.


I'm open to suggestions

Martin^2





--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0091 Allow full customisability of CA subject name

2016-09-23 Thread Jan Cholasta

On 23.9.2016 09:15, Fraser Tweedale wrote:

On Fri, Sep 23, 2016 at 08:51:02AM +0200, Jan Cholasta wrote:

On 25.8.2016 12:08, Jan Cholasta wrote:

On 22.8.2016 07:00, Fraser Tweedale wrote:

On Fri, Aug 19, 2016 at 08:09:33PM +1000, Fraser Tweedale wrote:

On Mon, Aug 15, 2016 at 10:54:25PM +1000, Fraser Tweedale wrote:

On Mon, Aug 15, 2016 at 02:08:54PM +0200, Jan Cholasta wrote:

On 19.7.2016 12:05, Jan Cholasta wrote:

On 19.7.2016 11:54, Fraser Tweedale wrote:

On Tue, Jul 19, 2016 at 09:36:17AM +0200, Jan Cholasta wrote:

Hi,

On 15.7.2016 07:05, Fraser Tweedale wrote:

On Fri, Jul 15, 2016 at 03:04:48PM +1000, Fraser Tweedale wrote:

The attached patch is a work in progress for
https://fedorahosted.org/freeipa/ticket/2614 (BZ 828866).

I am sharing now to make the approach clear and solicit feedback.

It has been tested for server install, replica install (with and
without CA) and CA-replica install (all hosts running
master+patch).

Migration from earlier versions and server/replica/CA install
on a
CA-less deployment are not yet tested; these will be tested over
coming days and patch will be tweaked as necessary.

Commit message has a fair bit to say so I won't repeat here
but let
me know your questions and comments.

Thanks,
Fraser


It does help to attach the patch, of course ^_^


IMO explicit is better than implicit, so instead of introducing
additional
magic around --subject, I would rather add a new separate option
for
specifying the CA subject name (I think --ca-subject, for
consistency
with
--ca-signing-algorithm).


The current situation - the --subject argument which specifies the
not the subject but the subject base, is confusing enough (to say
nothing of the limitations that give rise to the RFE).

Retaining --subject for specifying the subject base and adding
--ca-subject for specifying the *actual* subject DN gets us over the
line in terms of the RFE, but does not make the installer less
confusing.  This is why I made --subject accept the full subject DN,
with provisions to retain existing behaviour.

IMO if we want to have separate arguments for subject DN and subject
base (I am not against it), let's bite the bullet and name arguments
accordingly.  --subject should be used to specify full Subject DN,
--subject-base (or similar) for specifying subject base.


IMHO --ca-subject is better than --subject, because it is more
explicit
whose subject name that is (the CA's). I agree that --subject
should be
deprecated and replaced with --subject-base.



(I intentionally defer discussion of specific behaviour if one, none
or both are specified; let's resolve the question or renaming /
changing meaning of arguments first).



By specifying the option you would override the default
"CN=Certificate
Authority,$SUBJECT_BASE" subject name. If --external-ca was not
used,
additional validation would be done to make sure the subject
name meets
Dogtag's expectations. Actually, it might make sense to always
do the
additional validation, to be able to print a warning that if a
Dogtag-incompatible subject name is used, it won't be possible to
change the
CA cert chaining from externally signed to self-signed later.

Honza


Bump, any update on this?


I have an updated patch that fixes some issues Sebastian encountered
in testing, but I've not yet tackled the main change requested by
Honza (in brief: adding --ca-subject for specifying that, adding
--subject-base for specifying that, and deprecating --subject;
Sebastian, see discussion above and feel free to give your
thoughts).  I expect I'll get back onto this work within the next
few days.


Update: I've got an updated version of patch almost ready for
review, but I'm still ironing out some wrinkles in replica
installation.

Expect to be able to send it Monday or Tuesday for review.


Updated patch attached.

I expect it will take a while to review, test and get the ACK, but
in any case: IMO it should not be merged until after ipa-4-4 branch
gets created.


1) Please fix these:

$ git show -U0 | pep8 --diff
./ipaserver/install/cainstance.py:508:13: E128 continuation line
under-indented for visual indent
./ipaserver/install/dsinstance.py:259:5: E303 too many blank lines (2)
./ipaserver/install/installutils.py:999:1: E302 expected 2 blank lines,
found 1
./ipaserver/install/server/common.py:161:80: E501 line too long (101 >
79 characters)
./ipaserver/install/server/replicainstall.py:93:80: E501 line too long
(82 > 79 characters)
./ipaserver/install/server/replicainstall.py:604:80: E501 line too long
(81 > 79 characters)


2) Please put 3rd party library (such as six) imports between standard
library imports and ipa imports.


3) ipa-ca-install should also have the --subject-base and --ca-subject
options.


4) Please use the original method of getting the configured subject base
from ipacertificatesubjectbase of the config object rather than
DSInstance.find_subject_base(), which is horrendous and should in fact
be obliterat

Re: [Freeipa-devel] [PATCH 0060] Add --force-join option to ipa-replica-install

2016-09-23 Thread Jan Cholasta

On 23.9.2016 09:01, Standa Laznicka wrote:

On 09/23/2016 08:50 AM, Jan Cholasta wrote:

On 25.8.2016 15:31, Martin Basti wrote:



On 10.08.2016 07:53, Stanislav Laznicka wrote:

On 08/10/2016 07:31 AM, Jan Cholasta wrote:

On 9.8.2016 18:52, Petr Vobornik wrote:

On 08/09/2016 04:18 PM, Martin Basti wrote:



On 09.08.2016 16:07, Stanislav Laznicka wrote:

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




Didn't we agreed that --force-join should be always used (without
extra
replica-install option)


+1


Did we?

IMO the default behavior should be the same as in domain level 0 when
trying to install replica on an already enrolled host.

That was my impression as well.


OK then, I don't like to add mostly useless option, but client install
is broken by design so whatever.


Bump, what is the status of this?

FTR this is what happens on domain level 0 if the host is already
enrolled:

# ipa-replica-install replica-info-test.example.com.gpg
WARNING: conflicting time synchronization service 'chronyd' will
be disabled in favor of ntpd

Directory Manager (existing master) password:

The host test.example.com already exists on the master server.
You should remove it before proceeding:
% ipa host-del test.example.com
ipa.ipapython.install.cli.install_tool(Replica): ERRORThe
ipa-replica-install command failed. See
/var/log/ipareplica-install.log for more information



There's been no status change.

I think the problem here is more about client-install advertising the
--force-join option which does not exist for ipa-replica-install. I do
not think we can detect that exactly this error occurred during
client-install being run from replica-install (can we?) but we can add
this option and pass it to client-install if required.


We could detect it before running ipa-client-install, but adding the 
option to ipa-replica-install is easier, so IMO that's what we should do.


--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0091 Allow full customisability of CA subject name

2016-09-23 Thread Jan Cholasta

On 25.8.2016 12:08, Jan Cholasta wrote:

On 22.8.2016 07:00, Fraser Tweedale wrote:

On Fri, Aug 19, 2016 at 08:09:33PM +1000, Fraser Tweedale wrote:

On Mon, Aug 15, 2016 at 10:54:25PM +1000, Fraser Tweedale wrote:

On Mon, Aug 15, 2016 at 02:08:54PM +0200, Jan Cholasta wrote:

On 19.7.2016 12:05, Jan Cholasta wrote:

On 19.7.2016 11:54, Fraser Tweedale wrote:

On Tue, Jul 19, 2016 at 09:36:17AM +0200, Jan Cholasta wrote:

Hi,

On 15.7.2016 07:05, Fraser Tweedale wrote:

On Fri, Jul 15, 2016 at 03:04:48PM +1000, Fraser Tweedale wrote:

The attached patch is a work in progress for
https://fedorahosted.org/freeipa/ticket/2614 (BZ 828866).

I am sharing now to make the approach clear and solicit feedback.

It has been tested for server install, replica install (with and
without CA) and CA-replica install (all hosts running
master+patch).

Migration from earlier versions and server/replica/CA install
on a
CA-less deployment are not yet tested; these will be tested over
coming days and patch will be tweaked as necessary.

Commit message has a fair bit to say so I won't repeat here
but let
me know your questions and comments.

Thanks,
Fraser


It does help to attach the patch, of course ^_^


IMO explicit is better than implicit, so instead of introducing
additional
magic around --subject, I would rather add a new separate option
for
specifying the CA subject name (I think --ca-subject, for
consistency
with
--ca-signing-algorithm).


The current situation - the --subject argument which specifies the
not the subject but the subject base, is confusing enough (to say
nothing of the limitations that give rise to the RFE).

Retaining --subject for specifying the subject base and adding
--ca-subject for specifying the *actual* subject DN gets us over the
line in terms of the RFE, but does not make the installer less
confusing.  This is why I made --subject accept the full subject DN,
with provisions to retain existing behaviour.

IMO if we want to have separate arguments for subject DN and subject
base (I am not against it), let's bite the bullet and name arguments
accordingly.  --subject should be used to specify full Subject DN,
--subject-base (or similar) for specifying subject base.


IMHO --ca-subject is better than --subject, because it is more
explicit
whose subject name that is (the CA's). I agree that --subject
should be
deprecated and replaced with --subject-base.



(I intentionally defer discussion of specific behaviour if one, none
or both are specified; let's resolve the question or renaming /
changing meaning of arguments first).



By specifying the option you would override the default
"CN=Certificate
Authority,$SUBJECT_BASE" subject name. If --external-ca was not
used,
additional validation would be done to make sure the subject
name meets
Dogtag's expectations. Actually, it might make sense to always
do the
additional validation, to be able to print a warning that if a
Dogtag-incompatible subject name is used, it won't be possible to
change the
CA cert chaining from externally signed to self-signed later.

Honza


Bump, any update on this?


I have an updated patch that fixes some issues Sebastian encountered
in testing, but I've not yet tackled the main change requested by
Honza (in brief: adding --ca-subject for specifying that, adding
--subject-base for specifying that, and deprecating --subject;
Sebastian, see discussion above and feel free to give your
thoughts).  I expect I'll get back onto this work within the next
few days.


Update: I've got an updated version of patch almost ready for
review, but I'm still ironing out some wrinkles in replica
installation.

Expect to be able to send it Monday or Tuesday for review.


Updated patch attached.

I expect it will take a while to review, test and get the ACK, but
in any case: IMO it should not be merged until after ipa-4-4 branch
gets created.


1) Please fix these:

$ git show -U0 | pep8 --diff
./ipaserver/install/cainstance.py:508:13: E128 continuation line
under-indented for visual indent
./ipaserver/install/dsinstance.py:259:5: E303 too many blank lines (2)
./ipaserver/install/installutils.py:999:1: E302 expected 2 blank lines,
found 1
./ipaserver/install/server/common.py:161:80: E501 line too long (101 >
79 characters)
./ipaserver/install/server/replicainstall.py:93:80: E501 line too long
(82 > 79 characters)
./ipaserver/install/server/replicainstall.py:604:80: E501 line too long
(81 > 79 characters)


2) Please put 3rd party library (such as six) imports between standard
library imports and ipa imports.


3) ipa-ca-install should also have the --subject-base and --ca-subject
options.


4) Please use the original method of getting the configured subject base
from ipacertificatesubjectbase of the config object rather than
DSInstance.find_subject_base(), which is horrendous and should in fact
be obliterated (not necessarily in this patch).


5) You can use "unicode(x509.get_subject(cert))" to get subject name 

Re: [Freeipa-devel] [PATCH 0060] Add --force-join option to ipa-replica-install

2016-09-23 Thread Jan Cholasta

On 25.8.2016 15:31, Martin Basti wrote:



On 10.08.2016 07:53, Stanislav Laznicka wrote:

On 08/10/2016 07:31 AM, Jan Cholasta wrote:

On 9.8.2016 18:52, Petr Vobornik wrote:

On 08/09/2016 04:18 PM, Martin Basti wrote:



On 09.08.2016 16:07, Stanislav Laznicka wrote:

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




Didn't we agreed that --force-join should be always used (without
extra
replica-install option)


+1


Did we?

IMO the default behavior should be the same as in domain level 0 when
trying to install replica on an already enrolled host.

That was my impression as well.


OK then, I don't like to add mostly useless option, but client install
is broken by design so whatever.


Bump, what is the status of this?

FTR this is what happens on domain level 0 if the host is already enrolled:

# ipa-replica-install replica-info-test.example.com.gpg
WARNING: conflicting time synchronization service 'chronyd' will
be disabled in favor of ntpd

Directory Manager (existing master) password:

The host test.example.com already exists on the master server.
You should remove it before proceeding:
% ipa host-del test.example.com
ipa.ipapython.install.cli.install_tool(Replica): ERRORThe 
ipa-replica-install command failed. See /var/log/ipareplica-install.log 
for more information



--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0107 Fix cert revocation when removing all certs via host/service-mod

2016-09-23 Thread Jan Cholasta

On 23.9.2016 05:30, Fraser Tweedale wrote:

Bump for review.


Works for me, ACK.

Pushed to master: 97d4ffc2dc5db00fd7ed10b0b290cc97a506d0ef

--
Jan Cholasta

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


Re: [Freeipa-devel] pylint: remove unused variables

2016-09-22 Thread Jan Cholasta

On 22.9.2016 16:39, Martin Basti wrote:

Hello all,

In 4.5, I would like to remove all unused variables from code and enable
pylint check. Due to big amount of unused variables in the code this
will be longterm effort.

Why this?:

* better code readability

* removing dead code

* unused variable may uncover potential bug


It is clear what to do with unused assignments, but I need an agreement
what to do with unpacking or iteration with unused variables


For example:

for name, surname, gender in (('Martin', 'Basti', 'M'), ):

name, surname, gender = user['mbasti']

Where 'surname' is unused


Pylint will detect surname as unused variable and we have to agree on a
way how to tell pylint that this variable is unused on purpose:


1)

(

   name,

   surname,  # pylint: disable=unused-variable

   gender

) = user['mbasti']


I dont like this approach


+1




2)

Use defined keyword: 'dummy' is default in pylint, we can set our own,
like ignored, unused

name, dummy, gender = user['mbasti']


-1, not visible enough.




3)

use a prefix for unused variables: '_' or 'ignore_'

name, _surname, gender = user['mbasti']


This. We have already been using it in new code for quite some time, and 
it's common in other Python projects too. Don't reinvent the wheel.





4)

we can combine all :)


For me the best is to have prefix '_' and 'dummy' keyword


Use '_dummy', it's both :-)




As first step I'll enable pylint check and disable it locally per module
where unused variables are, to avoid new regressions. Then I will fix it
module by module.


I'm open to suggestions

Martin^2




--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 190] expose `--secret` option in radiusproxy-* commands

2016-09-07 Thread Jan Cholasta

On 7.9.2016 16:13, Martin Babinsky wrote:

On 09/07/2016 03:55 PM, Jan Cholasta wrote:

On 21.7.2016 10:50, Jan Cholasta wrote:

On 21.7.2016 10:13, Martin Babinsky wrote:

On 07/20/2016 12:10 PM, Martin Babinsky wrote:

On 07/19/2016 12:32 PM, Jan Cholasta wrote:

Hi,

On 18.7.2016 13:51, Martin Babinsky wrote:

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


I don't think we want the secret searchable. Add a 'no_search'
flag to
the param to fix that.

Honza



'no_search' flag breaks the API backwards compatibility, so I am
sending
another two patches which fix handling of deprecated options in the
framework and deprecate `--secret` in radiusproxy-find command.

I hope this solution is the best.




After discussion with Jan we realized that it is enough to hide the
'--secret' option from CLI, not deprecate it.

Re-sending patch 190 and updated 193.1.


Thanks, ACK.

Pushed to master: 66da08445370f7024a6a529a6659714c33b7525e


Patch 192 will be send in
separate thread since the actual issue it fixes is orthogonal to this
one and requires a separate ticket.


Right. ATM this only affects --srchostcat in hbacrule-find.


Bump so that patch 192 is not forgotten.



Patch 192 was pushed as a fix to
https://fedorahosted.org/freeipa/ticket/6190.


Okay.

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 0183] ipa-advise: correct handling of plugin namespace iteration

2016-09-07 Thread Jan Cholasta

On 19.7.2016 09:15, Martin Babinsky wrote:

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

Hi,

On 11.7.2016 14:18, Martin Babinsky wrote:

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


Note that you should use .name rather than .__name__ to get plugin
names, otherwise the code won't work with plugins with non-default names.

There currently aren't any Advice plugins with non-default name, but I
would rather fix this now to avoid surprises later.

Honza



I didn't realize this when doing the patch, here's the fix for that.

I have attached the original closed ticket to the commit message, should
I create a new ticket for such a small change?


Bump. I'm fine with new ticket or no ticket, but don't use 6044.

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 190] expose `--secret` option in radiusproxy-* commands

2016-09-07 Thread Jan Cholasta

On 21.7.2016 10:50, Jan Cholasta wrote:

On 21.7.2016 10:13, Martin Babinsky wrote:

On 07/20/2016 12:10 PM, Martin Babinsky wrote:

On 07/19/2016 12:32 PM, Jan Cholasta wrote:

Hi,

On 18.7.2016 13:51, Martin Babinsky wrote:

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


I don't think we want the secret searchable. Add a 'no_search' flag to
the param to fix that.

Honza



'no_search' flag breaks the API backwards compatibility, so I am sending
another two patches which fix handling of deprecated options in the
framework and deprecate `--secret` in radiusproxy-find command.

I hope this solution is the best.




After discussion with Jan we realized that it is enough to hide the
'--secret' option from CLI, not deprecate it.

Re-sending patch 190 and updated 193.1.


Thanks, ACK.

Pushed to master: 66da08445370f7024a6a529a6659714c33b7525e


Patch 192 will be send in
separate thread since the actual issue it fixes is orthogonal to this
one and requires a separate ticket.


Right. ATM this only affects --srchostcat in hbacrule-find.


Bump so that patch 192 is not forgotten.

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-07 Thread Jan Cholasta

On 7.9.2016 10:28, Fraser Tweedale wrote:

On Wed, Sep 07, 2016 at 08:32:42AM +0200, Jan Cholasta wrote:

On 6.9.2016 19:36, Fraser Tweedale wrote:

On Tue, Sep 06, 2016 at 10:19:14AM +0200, Jan Cholasta wrote:

On 5.9.2016 17:30, Fraser Tweedale wrote:

On Mon, Sep 05, 2016 at 11:59:11PM +1000, Fraser Tweedale wrote:

On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:

Hi,

On 26.8.2016 07:42, Fraser Tweedale wrote:

On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:

Hi all,

Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
It depends on Honza's PR #20
https://github.com/freeipa/freeipa/pull/20.

Thanks,
Fraser


It does help to attach the patch :)


I think it would be better to call cert-find once per host-del/service-del
with the --host/--service option specified. That way you'll get all
certificates for the given host/service at once.

Honza


I agree that is a nicer approach.

'revoke_certs' is called from several other places besides just
host/service_del.  If we want to land this fix Real Soon I'd suggest
we either:

A) Define function 'revoke_certs_from_cert_find', call it from
host/service_del, and leave 'revoke_certs' alone; or

B) Land the patch as-is and do a bigger refactor at a later time.

What do you think?



Updated patch attached; comments inline.


C) Use cert-find-based revoke_certs() everywhere; use the --certificate
option of cert-find in the other places to get information about specific
certificates.


As discussed on IRC, I have implemented this option.  The caveat is
that for host/service-mod, we incur call to cert_find for each
removed certificate.


It's worth noting that A) and B) suffer from the same caveat.






Updated patch for option (A) is attached.


1) Instead of

if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:

use:

if result['revoked']:


Done.



2)

+if 'cacn' not in cert:
+# cert is known to Dogtag, but CA appears to have been
+# deleted.  We cannot revoke this cert via IPA anymore.
+# We could go directly to Dogtag to revoke it, but the
+# issuer's cert should have been revoked so never mind.
+continue

Or, it could be a cert issued by a 3rd party CA.


I updated to comment to include this.



3) host-mod/service-mod do not revoke certs:

$ ipa cert-request test.csr --principal host/test.example.com
  Serial number: 13

$ ipa cert-show 13
  Revoked: False
  Owner host: test.example.com

$ ipa host-mod test.example.com --certificate=

$ ipa cert-show 13
  Revoked: False


Nice find.  This was a pre-existing bug: nothing gets revoked when
all certs are removed.  Here is the fix:

-if certs and self.api.Command.ca_is_enabled()['result']:
+ca_is_enabled = self.api.Command.ca_is_enabled()['result']
+if 'usercertificate' in options and ca_is_enabled:
 ... revocation code


OK. Since it is a different bug, it should be fixed in a separate patch and
have a separate ticket.



Finally, host/service-remove-cert does not revoke the cert because
of (I think) a bug in cert-find.  If the cert does not exist on a
host/service the cert-find cannot find it with --certificate option.
Because host/service-remove-cert uses a post_callback to revoke the
cert, cert-find doesn't find it thus no revocation occurs.

Honza could you check whether this is indeed a bug/limitation of
cert-find or is it the smog in Saigon affecting me?


It's a bug - FTFY, <https://github.com/freeipa/freeipa/pull/64>.

Functional ACK. Full ACK once my fix is merged and the host/service-mod is
split off into a separate patch.


To clarify - you want only the fix discussed above in the separate
patch?


I want the fix for sub-CA revocation in one patch and the fix for 
host/service-mod with empty --certificate in other patch.


--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-07 Thread Jan Cholasta

On 6.9.2016 19:36, Fraser Tweedale wrote:

On Tue, Sep 06, 2016 at 10:19:14AM +0200, Jan Cholasta wrote:

On 5.9.2016 17:30, Fraser Tweedale wrote:

On Mon, Sep 05, 2016 at 11:59:11PM +1000, Fraser Tweedale wrote:

On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:

Hi,

On 26.8.2016 07:42, Fraser Tweedale wrote:

On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:

Hi all,

Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
It depends on Honza's PR #20
https://github.com/freeipa/freeipa/pull/20.

Thanks,
Fraser


It does help to attach the patch :)


I think it would be better to call cert-find once per host-del/service-del
with the --host/--service option specified. That way you'll get all
certificates for the given host/service at once.

Honza


I agree that is a nicer approach.

'revoke_certs' is called from several other places besides just
host/service_del.  If we want to land this fix Real Soon I'd suggest
we either:

A) Define function 'revoke_certs_from_cert_find', call it from
host/service_del, and leave 'revoke_certs' alone; or

B) Land the patch as-is and do a bigger refactor at a later time.

What do you think?



Updated patch attached; comments inline.


C) Use cert-find-based revoke_certs() everywhere; use the --certificate
option of cert-find in the other places to get information about specific
certificates.


As discussed on IRC, I have implemented this option.  The caveat is
that for host/service-mod, we incur call to cert_find for each
removed certificate.


It's worth noting that A) and B) suffer from the same caveat.






Updated patch for option (A) is attached.


1) Instead of

if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:

use:

if result['revoked']:


Done.



2)

+if 'cacn' not in cert:
+# cert is known to Dogtag, but CA appears to have been
+# deleted.  We cannot revoke this cert via IPA anymore.
+# We could go directly to Dogtag to revoke it, but the
+# issuer's cert should have been revoked so never mind.
+continue

Or, it could be a cert issued by a 3rd party CA.


I updated to comment to include this.



3) host-mod/service-mod do not revoke certs:

$ ipa cert-request test.csr --principal host/test.example.com
  Serial number: 13

$ ipa cert-show 13
  Revoked: False
  Owner host: test.example.com

$ ipa host-mod test.example.com --certificate=

$ ipa cert-show 13
  Revoked: False


Nice find.  This was a pre-existing bug: nothing gets revoked when
all certs are removed.  Here is the fix:

-if certs and self.api.Command.ca_is_enabled()['result']:
+ca_is_enabled = self.api.Command.ca_is_enabled()['result']
+if 'usercertificate' in options and ca_is_enabled:
 ... revocation code


OK. Since it is a different bug, it should be fixed in a separate patch 
and have a separate ticket.




Finally, host/service-remove-cert does not revoke the cert because
of (I think) a bug in cert-find.  If the cert does not exist on a
host/service the cert-find cannot find it with --certificate option.
Because host/service-remove-cert uses a post_callback to revoke the
cert, cert-find doesn't find it thus no revocation occurs.

Honza could you check whether this is indeed a bug/limitation of
cert-find or is it the smog in Saigon affecting me?


It's a bug - FTFY, <https://github.com/freeipa/freeipa/pull/64>.

Functional ACK. Full ACK once my fix is merged and the host/service-mod 
is split off into a separate patch.


--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-06 Thread Jan Cholasta

On 5.9.2016 17:30, Fraser Tweedale wrote:

On Mon, Sep 05, 2016 at 11:59:11PM +1000, Fraser Tweedale wrote:

On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:

Hi,

On 26.8.2016 07:42, Fraser Tweedale wrote:

On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:

Hi all,

Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
It depends on Honza's PR #20
https://github.com/freeipa/freeipa/pull/20.

Thanks,
Fraser


It does help to attach the patch :)


I think it would be better to call cert-find once per host-del/service-del
with the --host/--service option specified. That way you'll get all
certificates for the given host/service at once.

Honza


I agree that is a nicer approach.

'revoke_certs' is called from several other places besides just
host/service_del.  If we want to land this fix Real Soon I'd suggest
we either:

A) Define function 'revoke_certs_from_cert_find', call it from
host/service_del, and leave 'revoke_certs' alone; or

B) Land the patch as-is and do a bigger refactor at a later time.

What do you think?


C) Use cert-find-based revoke_certs() everywhere; use the --certificate 
option of cert-find in the other places to get information about 
specific certificates.





Updated patch for option (A) is attached.


1) Instead of

if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:

use:

if result['revoked']:


2)

+if 'cacn' not in cert:
+# cert is known to Dogtag, but CA appears to have been
+# deleted.  We cannot revoke this cert via IPA anymore.
+# We could go directly to Dogtag to revoke it, but the
+# issuer's cert should have been revoked so never mind.
+continue

Or, it could be a cert issued by a 3rd party CA.


3) host-mod/service-mod do not revoke certs:

$ ipa cert-request test.csr --principal host/test.example.com
  Serial number: 13

$ ipa cert-show 13
  Revoked: False
  Owner host: test.example.com

$ ipa host-mod test.example.com --certificate=

$ ipa cert-show 13
  Revoked: False


--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0014

2016-09-05 Thread Jan Cholasta

On 5.9.2016 10:42, Tomas Krizek wrote:


On 09/02/2016 09:05 AM, Florence Blanc-Renaud wrote:

On 09/02/2016 08:08 AM, Jan Cholasta wrote:

On 1.9.2016 19:37, Tomas Krizek wrote:

On 09/01/2016 03:58 PM, Florence Blanc-Renaud wrote:

Hi,

please find attached a patch for ipa-certupdate in CA-less deployment.
https://fedorahosted.org/freeipa/ticket/6288

Flo.




The patch is malformed, but you can simply delete the very first
character to fix it.

Other than that, patch works as expected -> ACK.


Nitpick: please avoid C-isms such as "if (ca_enabled):".


Hi all,

thanks for the review. Please find an updated patch version. Quite
difficult to get rid of typing habits...

Flo


ACK


Pushed to:
master: b36ee723b77a2721f4200d5df02268a9bd6a60b5
ipa-4-4: 1b8f6ec58600ad4bbfb538ddcff659ea1ba2c324

--
Jan Cholasta

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


Re: [Freeipa-devel] [Design Review Request] V4/Automatic_Certificate_Request_Generation

2016-09-05 Thread Jan Cholasta

On 27.8.2016 22:40, Ben Lipton wrote:

On 08/25/2016 04:11 PM, Rob Crittenden wrote:

Ben Lipton wrote:

On 08/23/2016 03:54 AM, Jan Cholasta wrote:

On 8.8.2016 22:23, Ben Lipton wrote:

On 07/25/2016 07:45 AM, Jan Cholasta wrote:

On 25.7.2016 13:11, Alexander Bokovoy wrote:

On Mon, 25 Jul 2016, Jan Cholasta wrote:

On 20.7.2016 16:05, Ben Lipton wrote:

Hi,

Thanks very much for the feedback! Some responses below; I hope
you'll
let me know what you think of my reasoning.


On 07/20/2016 04:20 AM, Jan Cholasta wrote:

Hi,

On 17.6.2016 00:06, Ben Lipton wrote:

On 06/14/2016 08:27 AM, Ben Lipton wrote:

Hello all,

I have written up a design proposal for making certificate
requests
easier to generate when using alternate certificate profiles:
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation.





The use case for this is described in
https://fedorahosted.org/freeipa/ticket/4899. I will be
working on
implementing this design over the next couple of months. If you
have
the time and interest, please take a look and share any
comments or
concerns that you have.

Thanks!

Ben


Just a quick update to say that I've created a new document that
covers
the proposed schema additions in a more descriptive way (with
diagrams!)
I'm very new to developing with LDAP, so some more experienced
eyes on
the proposal would be very helpful, even if you don't have
time to
absorb the full design. Please take a look at
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation/Schema





if you have a chance.


I finally had a chance to take a look at this, here are some
comments:

1) I don't like how transformation rules are tied to a particular
helper and have to be duplicated for each of them. They should be
generic and work with any helper, as helpers are just an
implementation detail and their resulting data is the same.

In fact, I think I would prefer if the CSR was generated using
python-cryptography's CertificateSigningRequestBuilder [1] rather
than
openssl or certutil or any other command line tool.


There are lots of tools that users might want to use to manage
their
private keys, so I don't know if we can assume that whatever
library we
prefer will actually be able to access the private key to sign a
CSR,
which is why I thought it would be useful to support more than
one.


python-cryptography has the notion of backends, which allow it to
support multiple crypto implementations. Upstream it currently
supports only OpenSSL [2], but some work has been done on PKCS#11
backend [3], which provides support for HSMs and soft-tokens (like
NSS
databases).

Alternatively, for NSS databases (and other "simple" cases), you
can
generate the private key with python-cryptography using the default
backend, export it to a file and import the file to the target
database, so you don't actually need the PKCS#11 backend for them.

So, the only thing that's currently lacking is HSM support, but
given
that we don't support HSMs in IPA nor in certmonger, I don't think
it's an issue for now.


The
purpose of the mapping rule is to tie together the transformation
rules
that produce the same data into an object that's
implementation-agnostic, so that profiles referencing those rules
are
automatically compatible with all the helper options.


They are implementation-agnostic, as long as you consider `openssl`
and `certutil` the only implementations :-) But I don't think this
solution scales well to other possible implementations.

Anyway, my main grudge is that the transformation rules shouldn't
really be stored on and processed by the server. The server should
know the *what* (mapping rules), but not the *how* (transformation
rules). The *how* is an implementation detail and does not
change in
time, so there's no benefit in handling it on the server. It
should be
handled exclusively on the client, which I believe would also make
the
whole thing more robust (it would not be possible for a bug on the
server to break all the clients).

This is a good point. However, for the scope of Ben's project can we
limit it by openssl and certutil support? Otherwise Ben wouldn't be
able
to complete the project in time.


I'm fine with that, but I don't think it's up to me :-)




This is turning out to be a common (and, I think, reasonable)
reaction
to the proposal. It is rather complex, and I worry that it will be
difficult to configure. On the other hand, there is some hidden
complexity to enabling a simpler config format, as well. One of
the
goals of the project as it was presented to me was to allow the
creation
of profiles that add certificate extensions *that FreeIPA doesn't
yet
know about*. With the current proposal, one only has to add a rule
generating text that the helper will understand.


... which will be possible only as long as the helper
understands the
extension. Which it might not, thus the current proposal works only
for *some* extensions that FreeIPA doesn't yet support.

We can go ad

Re: [Freeipa-devel] [PATCH] 0014

2016-09-02 Thread Jan Cholasta

On 1.9.2016 19:37, Tomas Krizek wrote:

On 09/01/2016 03:58 PM, Florence Blanc-Renaud wrote:

Hi,

please find attached a patch for ipa-certupdate in CA-less deployment.
https://fedorahosted.org/freeipa/ticket/6288

Flo.




The patch is malformed, but you can simply delete the very first
character to fix it.

Other than that, patch works as expected -> ACK.


Nitpick: please avoid C-isms such as "if (ca_enabled):".

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0220 move /bin/ipa to freeipa-client

2016-08-30 Thread Jan Cholasta

On 30.8.2016 14:09, Martin Basti wrote:



On 30.08.2016 09:27, Jan Cholasta wrote:

On 25.8.2016 13:09, Alexander Bokovoy wrote:

On Thu, 25 Aug 2016, Jan Cholasta wrote:

Hi,

On 25.8.2016 11:27, Alexander Bokovoy wrote:

Hi,

attached patch moves ipa CLI to freeipa-client and obsoletes
freeipa-admintools


The Obsoletes (both) should be on version < 4.4.1 rather than
%{version}, as per Fedora packaging guidelines [1].

Please move the Obsoletes and Provides on %{name}-admintools right
below Group (Obsoletes first) and put a newline between the
%{alt_name}-client and %{alt_name}-admintools blocks, for consistent
layout accross all subpackages in the spec file.



Solves https://fedorahosted.org/freeipa/ticket/5934

Here is how upgrade looks when running 'dnf':

Upgrading:
freeipa-client x86_64
4.4.0.201608250913GIT9c20682-0.fc24 @commandline
146 k
   replacing  freeipa-admintools.noarch
4.4.0.201608051228GIT590e30f-0.fc24


I'm going to test with yum as well, for RHEL and CentOS.

Updated patch attached.


Thanks, ACK.

Pushed to master: b91ba39d627d36d1a62ebf97a987a2579404395e


I'm experiencing this:

[root@vm-058-017 ~]# dnf reinstall /home/mbasti/freeipa-rpms/* -y
Last metadata expiration check: 1:32:14 ago on Tue Aug 30 12:32:32 2016.
Dependencies resolved.
===

 Package Arch Version Repository
Size
===

Reinstalling:
 freeipa-client x86_64 4.4.0-0.fc24
@commandline 146 k
 replacing  freeipa-admintools.noarch 4.4.0-0.fc24
 freeipa-client-common noarch 4.4.0-0.fc24
@commandline  28 k
 freeipa-common noarch 4.4.0-0.fc24
@commandline 386 k
 freeipa-debuginfo x86_64 4.4.0-0.fc24
@commandline 934 k
 freeipa-python-compat noarch 4.4.0-0.fc24
@commandline  22 k
 freeipa-server x86_64 4.4.0-0.fc24
@commandline 350 k
 freeipa-server-common noarch 4.4.0-0.fc24
@commandline 524 k
 freeipa-server-dns noarch 4.4.0-0.fc24
@commandline  26 k
 freeipa-server-trust-ad x86_64 4.4.0-0.fc24
@commandline 116 k
 python2-ipaclient noarch 4.4.0-0.fc24
@commandline 443 k
 python2-ipalib noarch 4.4.0-0.fc24
@commandline 560 k
 python2-ipaserver noarch 4.4.0-0.fc24
@commandline 1.2 M
 python2-ipatests noarch 4.4.0-0.fc24
@commandline 840 k
 python3-ipaclient noarch 4.4.0-0.fc24
@commandline 626 k
 python3-ipalib noarch 4.4.0-0.fc24
@commandline 568 k
 python3-ipatests noarch 4.4.0-0.fc24
@commandline 842 k

Transaction Summary
===


Total size: 7.4 M
Downloading Packages:
Running transaction check
Error: transaction check vs depsolve:
ipa-admintools conflicts with freeipa-client-4.4.0-0.fc24.x86_64
freeipa-admintools < 4.4.1 is obsoleted by
freeipa-client-4.4.0-0.fc24.x86_64
ipa-admintools conflicts with (installed)
freeipa-admintools-4.4.0-0.fc24.noarch
To diagnose the problem, try running: 'rpm -Va --nofiles --nodigest'.
You probably have corrupted RPMDB, running 'rpm --rebuilddb' might fix
the issue.


I think this will go away by itself once we release 4.4.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


Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-08-30 Thread Jan Cholasta

Hi,

On 26.8.2016 07:42, Fraser Tweedale wrote:

On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:

Hi all,

Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
It depends on Honza's PR #20
https://github.com/freeipa/freeipa/pull/20.

Thanks,
Fraser


It does help to attach the patch :)


I think it would be better to call cert-find once per 
host-del/service-del with the --host/--service option specified. That 
way you'll get all certificates for the given host/service at once.


Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0101 Add ca-disable and ca-enable commands

2016-08-30 Thread Jan Cholasta

Hi,

On 30.8.2016 09:56, Martin Babinsky wrote:

On 08/25/2016 10:25 AM, Fraser Tweedale wrote:

Hi team,

The attached patch fixes
https://fedorahosted.org/freeipa/ticket/6257.

The behaviour of cert-request when the CA is disabled is not very
nice (it reports a server error from Dogtag).  The Dogtag REST
interface gives much better errors so I plan to move to it in a
later change (which will also address
https://fedorahosted.org/freeipa/ticket/3473, in part).

Thanks,
Fraser





HI Fraser,

I have a couple of comments below:

1.)
@@ -25,6 +33,10 @@ EXAMPLES:
 ipa ca-add puppet --desc "Puppet" \\
 --subject "CN=Puppet CA,O=EXAMPLE.COM"

+  Disable a CA.
+
+ipa ca-disable puppet
+
 """)

You missed an example of `ca-enable` command in the doc string.

2.)

Regarding implementation of ca_enable/disable, I think you can reduce
the amount of code duplication by employing a base class which will look
up the required sub-CA and call the RA backend method required by the
subclass. See the attached untested diff (passes lint) for details.


NACK, please don't use getattr() this way. Since you are subclassing 
here, you should use polymorphism:


class CAQuery(LDAPQuery):
def execute(...):
...
self.perform_action(ca_api, ca_id)
...

def perform_action(self, ca_api, ca_id):
raise NotImplementedError()

class ca_enable(CAQuery):
def perform_action(self, ca_api, ca_id):
ca_api.enable_ca(ca_id)

Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0220 move /bin/ipa to freeipa-client

2016-08-30 Thread Jan Cholasta

On 25.8.2016 13:09, Alexander Bokovoy wrote:

On Thu, 25 Aug 2016, Jan Cholasta wrote:

Hi,

On 25.8.2016 11:27, Alexander Bokovoy wrote:

Hi,

attached patch moves ipa CLI to freeipa-client and obsoletes
freeipa-admintools


The Obsoletes (both) should be on version < 4.4.1 rather than
%{version}, as per Fedora packaging guidelines [1].

Please move the Obsoletes and Provides on %{name}-admintools right
below Group (Obsoletes first) and put a newline between the
%{alt_name}-client and %{alt_name}-admintools blocks, for consistent
layout accross all subpackages in the spec file.



Solves https://fedorahosted.org/freeipa/ticket/5934

Here is how upgrade looks when running 'dnf':

Upgrading:
freeipa-client x86_64
4.4.0.201608250913GIT9c20682-0.fc24@commandline
146 k
   replacing  freeipa-admintools.noarch
4.4.0.201608051228GIT590e30f-0.fc24


I'm going to test with yum as well, for RHEL and CentOS.

Updated patch attached.


Thanks, ACK.

Pushed to master: b91ba39d627d36d1a62ebf97a987a2579404395e

--
Jan Cholasta

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


Re: [Freeipa-devel] [DESIGN][UPDATE] Time-Based HBAC Policies

2016-08-30 Thread Jan Cholasta
o we may just do this again and profit from its original purpose.


The original purpose was to support deny rules, but they were deprecated.


To
top it off, this change should be really easy to implement to what I
currently have on SSSD side.

I was just wondering - would you propose for every newly created rule to
have the new accessRuleType set to "allow_with_time" or should the type
change with addition of time rules to the HBAC rule as it does
currently? Also, should the user be able to modify the type so that a
rule with the new type is also visible for older clients (=> he could
add "allow" to type anytime)?

Thanks for your ideas, I am very happy with what you suggested here :)


TBH I'm not - I don't find adding hacks on top of obsolete deprecated 
stuff to be a particularly appealing solution to anything.


--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0095 cert-request: allow directoryName in SAN extension

2016-08-30 Thread Jan Cholasta

On 29.8.2016 07:57, Fraser Tweedale wrote:

On Fri, Aug 26, 2016 at 10:41:37AM +0200, Jan Cholasta wrote:

Hi,

On 22.7.2016 07:18, Fraser Tweedale wrote:

While I was poking around SAN-processing code, I decided to
implement a small enhancement: allowing the subject principal's DN
to appear in SAN.

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

Patch depends on my other patches 0090, 0092, 0093, 0094.


I don't think this is how DN SANs are supposed to be handled. For example,
see this bit about DN name constraints in RFC 5280 section 4.2.1.10:

   Restrictions of the form directoryName MUST be applied to the subject
   field in the certificate (when the certificate includes a non-empty
   subject field) and to any names of type directoryName in the
   subjectAltName extension.

It would appear to me that DN SANs only provide additional values to the
subject name of the certificate and thus should be treated the same way as
the subject name.

We don't impose any restrictions on subject names with regard to DN of the
subject LDAP entry, so I think we should not do it for DN SANs as well. Or,
alternatively, we should do it for both.


I disagree.  Supporting an altname containing the LDAP DN is a valid
use case.  There is no need to apply the same rules to Subject DN
and Directory Name altname


Nowhere in the RFC is it stated that there is any semantic difference 
between the subject name and DN SANs, so I don't see why should we make 
DN SANs special.



(otherwise, why would the Directory Name
altname type even exist?).


To allow multiple subject DNs.


There are other possible values but this
one is trivial to validate so why not?


I have no issue with validation per se, I just find it very odd that the 
code would allow me to request a cert with any LDAP entry DN in subject 
name but only one specific LDAP entry DN in DN SAN.




As for the RFC excerpt, this is about the Name Constraints
extension.  In the unlikely case that a superior certificate has a
Name Constraints extension that applies to DNs, the way we construct
the Subject DN is probably the bigger problem ;)


Yes, this particular excerpt is about name constraints, but I doubt that 
if you looked anywhere else, it would say something different about the 
relationship of subject name and DN SANs.




Take the feature or leave it (after all, noone has asked for it yet)
but IMO the usage is valid.

Cheers,
Fraser




--
Jan Cholasta

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


Re: [Freeipa-devel] [DESIGN][UPDATE] Time-Based HBAC Policies

2016-08-29 Thread Jan Cholasta

On 26.8.2016 16:39, Simo Sorce wrote:

On Fri, 2016-08-26 at 12:39 +0200, Martin Basti wrote:

I miss "why" part of "To be able to handle backward compatibility

with

ease, a new object called ipaHBACRulev2 is introduced. " in the

design

page. If the reason is the above - old client's should ignore time

rules

then it has to be mentioned there. Otherwise I don't see a reason to
introduce a new object type instead of extending the current.


How do you want to enforce HBAC rule that have set time from 10 to 14
everyday? With the same objectclass old clients will allow this HBAC
for
all day. Isn't this CVE?


This is a discussion worth having.

In general it is a CVE only if an authorization mechanism fails to work
as advertised.

If you make it clear that old clients *DO NOT* respect time rules then
there is no CVE material, it is working as "described".

The admins already have a way to not set those rules for older clients
by simply grouping newer clients in a different host group and applying
time rules only there.

So the question really is: should we allow admins to apply an HBAC Rule
potentially to older clients that do not understand it and will
therefore allow access at any time of the day, or should we prevent it ?

This is a hard question to answer and can go both ways.

A time rule may be something that admins want to enforce at all cost or
deny access. In this case a client that fails to handle it would be a
problem.

But it may be something that is just used for defense in depth and not a
strictly hard requirement. In this case allowing older clients would
make it an easy transition as you just set up the rule and the client
will start enforcing the time when it is upgraded but work otherwise
with the same rules.


That does not make a lot of sense to me. If the admin does not really 
care about enforcing the access time, why would they bother setting it 
in the first place?




I am a bit conflicted on trying to decide what scenario we should
target, but the second one appeals to me because host groups do already
give admins a good way to apply rules to a specific set of hosts and
exclude old clients w/o us making it a hard rule.
OTOH if an admin does not understand this difference, they may be
surprised to find out there are clients that do not honor it.


The second one does not appeal to me, because it is inviting to the kind 
of mistakes which would allow access when it should not be allowed and 
IMHO it's better to be safe than sorry.




Perhaps we could find a way to set a flag on the rule such that when set
(and only when set) older clients get excluded by way of changing the
objectlass or something else to similar effect.

Open to discussion.

Simo.




--
Jan Cholasta

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


Re: [Freeipa-devel] [DESIGN][UPDATE] Time-Based HBAC Policies

2016-08-26 Thread Jan Cholasta

On 26.8.2016 12:21, Martin Basti wrote:



On 26.08.2016 12:13, Jan Cholasta wrote:

On 26.8.2016 11:55, Martin Basti wrote:



On 26.08.2016 11:43, Jan Cholasta wrote:

Hi,

On 11.8.2016 12:34, Stanislav Laznicka wrote:

Hello,

I updated the design of the Time-Based HBAC Policies according to the
discussion we led here earlier. Please check the design page
http://www.freeipa.org/page/V4/Time-Based_Account_Policies. The
biggest
changes are in the Implementation and Feature Management sections. I
also added a short How to Use section.


1) Please use the 'ipa' prefix for new attributes: memberTimeRule ->
ipaMemberTimeRule


2) Source hosts are deprecated and thus should be removed from
ipaHBACRuleV2.


3) Since time rules are defined by memberTimeRule, accessTime should
be removed from ipaHBACRuleV2.


ad 2) 3)

Because backward compatibility, ipaHBACRuleV2 must contain all
attributes from ipaHBACRule as MAY


Not true.



With current approach, when timerule is added to HBAC, we just change
objectclass from 'ipahbacrule' to 'ipahbacrulev2' so we keep all
attributes that was defined in older HBAC. Removing any attrs from
ipaHBACRuleV2 can cause schema violation.


Which is perfectly fine.




I'm not sure if want to handle this in code (removing deprecated
attributes from HBAC entry when timerule is added)


We don't have to do anything. If any of the deprecated attributes are
present when you change the object class (which they shouldn't
anyway), you'll get schema violation, otherwise it will work just fine.


I'm not sure if this is user friendly.


You can obviously catch the schema violation and provided a meaningful 
error instead.








I realized that AccessTime is MUST for 'ipahbacrule', so when timerule
('ipahbacrulev2') is removed and somebody deleted accesstime we have to
add it back.


It is MAY. The only MUST attribute is accessRuleType, but that is
deprecated as well and should be removed from ipaHBACRuleV2. We only
support allow rules, so when timerule is removed, that's the value you
set accessRuleType to.


Right, sorry.
Martin^2








4) The CLI sections needs more work, especially for non-standard
commands like timerule-test.



On the link below is a PROTOTYPE-patched FreeIPA that covers most
of the
CLI functionality (except for the creation of iCalendar strings from
options) for better illustration of the design.

https://github.com/stlaz/freeipa/tree/timerules_2

I will add FreeIPA people that recently had some say about this to
CC so
that we can get the discussion flowing.


Honza











--
Jan Cholasta

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


Re: [Freeipa-devel] [DESIGN][UPDATE] Time-Based HBAC Policies

2016-08-26 Thread Jan Cholasta

On 26.8.2016 11:55, Martin Basti wrote:



On 26.08.2016 11:43, Jan Cholasta wrote:

Hi,

On 11.8.2016 12:34, Stanislav Laznicka wrote:

Hello,

I updated the design of the Time-Based HBAC Policies according to the
discussion we led here earlier. Please check the design page
http://www.freeipa.org/page/V4/Time-Based_Account_Policies. The biggest
changes are in the Implementation and Feature Management sections. I
also added a short How to Use section.


1) Please use the 'ipa' prefix for new attributes: memberTimeRule ->
ipaMemberTimeRule


2) Source hosts are deprecated and thus should be removed from
ipaHBACRuleV2.


3) Since time rules are defined by memberTimeRule, accessTime should
be removed from ipaHBACRuleV2.


ad 2) 3)

Because backward compatibility, ipaHBACRuleV2 must contain all
attributes from ipaHBACRule as MAY


Not true.



With current approach, when timerule is added to HBAC, we just change
objectclass from 'ipahbacrule' to 'ipahbacrulev2' so we keep all
attributes that was defined in older HBAC. Removing any attrs from
ipaHBACRuleV2 can cause schema violation.


Which is perfectly fine.




I'm not sure if want to handle this in code (removing deprecated
attributes from HBAC entry when timerule is added)


We don't have to do anything. If any of the deprecated attributes are 
present when you change the object class (which they shouldn't anyway), 
you'll get schema violation, otherwise it will work just fine.




I realized that AccessTime is MUST for 'ipahbacrule', so when timerule
('ipahbacrulev2') is removed and somebody deleted accesstime we have to
add it back.


It is MAY. The only MUST attribute is accessRuleType, but that is 
deprecated as well and should be removed from ipaHBACRuleV2. We only 
support allow rules, so when timerule is removed, that's the value you 
set accessRuleType to.









4) The CLI sections needs more work, especially for non-standard
commands like timerule-test.



On the link below is a PROTOTYPE-patched FreeIPA that covers most of the
CLI functionality (except for the creation of iCalendar strings from
options) for better illustration of the design.

https://github.com/stlaz/freeipa/tree/timerules_2

I will add FreeIPA people that recently had some say about this to CC so
that we can get the discussion flowing.


Honza






--
Jan Cholasta

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


Re: [Freeipa-devel] [DESIGN][UPDATE] Time-Based HBAC Policies

2016-08-26 Thread Jan Cholasta

Hi,

On 11.8.2016 12:34, Stanislav Laznicka wrote:

Hello,

I updated the design of the Time-Based HBAC Policies according to the
discussion we led here earlier. Please check the design page
http://www.freeipa.org/page/V4/Time-Based_Account_Policies. The biggest
changes are in the Implementation and Feature Management sections. I
also added a short How to Use section.


1) Please use the 'ipa' prefix for new attributes: memberTimeRule -> 
ipaMemberTimeRule



2) Source hosts are deprecated and thus should be removed from 
ipaHBACRuleV2.



3) Since time rules are defined by memberTimeRule, accessTime should be 
removed from ipaHBACRuleV2.



4) The CLI sections needs more work, especially for non-standard 
commands like timerule-test.




On the link below is a PROTOTYPE-patched FreeIPA that covers most of the
CLI functionality (except for the creation of iCalendar strings from
options) for better illustration of the design.

https://github.com/stlaz/freeipa/tree/timerules_2

I will add FreeIPA people that recently had some say about this to CC so
that we can get the discussion flowing.


Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0095 cert-request: allow directoryName in SAN extension

2016-08-26 Thread Jan Cholasta

Hi,

On 22.7.2016 07:18, Fraser Tweedale wrote:

While I was poking around SAN-processing code, I decided to
implement a small enhancement: allowing the subject principal's DN
to appear in SAN.

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

Patch depends on my other patches 0090, 0092, 0093, 0094.


I don't think this is how DN SANs are supposed to be handled. For 
example, see this bit about DN name constraints in RFC 5280 section 
4.2.1.10:


   Restrictions of the form directoryName MUST be applied to the subject
   field in the certificate (when the certificate includes a non-empty
   subject field) and to any names of type directoryName in the
   subjectAltName extension.

It would appear to me that DN SANs only provide additional values to the 
subject name of the certificate and thus should be treated the same way 
as the subject name.


We don't impose any restrictions on subject names with regard to DN of 
the subject LDAP entry, so I think we should not do it for DN SANs as 
well. Or, alternatively, we should do it for both.


Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0097 Add options to write lightweight CA cert or chain to file

2016-08-26 Thread Jan Cholasta

On 19.8.2016 13:11, Fraser Tweedale wrote:

Bump for review.

On Wed, Aug 17, 2016 at 12:09:39AM +1000, Fraser Tweedale wrote:

On Tue, Aug 16, 2016 at 08:10:08AM +0200, Jan Cholasta wrote:

On 16.8.2016 07:24, Fraser Tweedale wrote:

On Mon, Aug 15, 2016 at 08:19:33AM +0200, Jan Cholasta wrote:

On 9.8.2016 16:47, Fraser Tweedale wrote:

On Mon, Aug 08, 2016 at 10:49:27AM +0200, Jan Cholasta wrote:

On 8.8.2016 09:06, Fraser Tweedale wrote:

On Mon, Aug 08, 2016 at 08:54:05AM +0200, Jan Cholasta wrote:

Hi,

On 8.8.2016 06:34, Fraser Tweedale wrote:

Please review the attached patch with adds --certificate-out and
--certificate-chain-out options to `ca-show' command.

Note that --certificate-chain-out currently writes a bogus file due
to a bug in Dogtag that will be fixed in this week's build.

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


1) The client-side *-out options should be defined on the client side, not
on the server side.


Will option defined on client side be propagated to, and observable
in the ipaserver plugin?  The ipaserver plugin needs to observe that
*-out has been requested and executes additional command(s) on that
basis.


Is there a reason not to *always* return the certs?


We hit Dogtag to retrieve them.


I don't think that's an issue in a -show command.


cert_show is invoked by other commands (cert_find*, cert_show,
cert_request, cert_status, ca_del) but these all hit Dogtag anyway
so I suppose that's fine.  I'll return the cert *and* the chain in
separate attributes, unconditionally.







2) I don't think there should be additional information included in summary
(and it definitely should not be multi-line). I would rather inform the user
via an error message when unable to write the files.


I was just following the pattern of other commands that write certs,
profile config, etc.  Apart from consistency with other commands I
agree that there is no need to have it.  So I will remove it.


If you think there is an actual value in informing the user about
successfully writing the files, please use ipalib.messages for the job.


3) IMO a better format for the certificate chain than PKCS#7 would be
concatenated PEM, as that's the most commonly used format in IPA (in
installers, there are no cert chains in API commands ATM).


Sure, but the main use case isn't IPA.  Other apps require PKCS #7
or concatenated PEMs, but sometimes they must be concatenated
forward, and othertimes backwards.  There is no one size fits all.


True, which is exactly why I think we should at least be self-consistent and
use concatenated PEM (and multi-value DER over the wire).


Dogtag returns a PKCS7 (either DER or PEM, according to HTTP Accept
header).

If we want list-of-PEMs between server and client we have to convert
on the server.  Do we have a good way of doing this without exec'ing
`openssl pkcs7' on the server?  Is it acceptable to exec 'openssl'
to do the conversion on the server?  python-nss does not have PKCS7
functions and I am not keen on adding a pyasn1 PKCS7 parser just for
the sake of pushing bits as list-of-PEMs.


I'm afraid we can't avoid conversion to/from PKCS#7 one way or the other.
For example, if we added a call to retrieve external CA chain using certs
from cn=certificates,cn=ipa,cn=etc, we would have to convert the result to
PKCS#7 if it was our cert chain format of choice.

What we can avoid though is executing "openssl pkcs7" to do the conversion -
we can use an approach similar to our DNSSEC code and use python-cffi to
call libcrypto's PKCS#7 conversion routines instead.


I had a look at the OpenSSL API for parsing PKCS #7; now I prefer to
exec `openssl' to do the job :)

I will transmit DER-encoded PKCS #7 object on the wire; we cannot
used multi-valued DER attribute because order is important.   Client
will convert to PEMs.


Well, my point was not to send PKCS#7 over the wire, so that clients
(including 3rd party clients) do not have to convert from PKCS#7 themselves.

In fact we can use multi-valued DER - whatever you send over the wire from
the server will be received in the exact same order by the client. Even if
it wasn't, you can easily restore the order by matching issuer and subject
names of the certificates.



Should have new patch on list this afternoon.

Thanks,
Fraser



FWIW, man pages and code suggest that PKCS #7 is accepted in
installer, etc.


True, but that's a relatively new feature (since 4.1) and the installer
internally executes "openssl pkcs7" to convert PKCS #7 to list of certs :-)




We can add an option to control the format later, but for now,
Dogtag returns a PKCS #7 (PEM or DER) so let's go with that.  Worst
case is an admin has to invoke `openssl pkcs7' and concat the certs
themselves.


AFAIK none of NSS, OpenSSL or p11-kit can use PKCS#7 cert chains directly,
so I'm afraid the worst case would happen virtually always.


If you're OK with invoking OpenSSL on the client to convert PKCS #7
to list-of-PEMs (sim

Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names

2016-08-26 Thread Jan Cholasta

On 23.8.2016 11:46, Fraser Tweedale wrote:

Thanks for review; rebased and updated patch attached.  Only 0090
has substantive changes.

Cheers,
Fraser

On Mon, Aug 22, 2016 at 09:22:08AM +0200, Jan Cholasta wrote:

On 19.8.2016 13:11, Fraser Tweedale wrote:

Bump for review.

On Mon, Aug 15, 2016 at 05:15:16PM +1000, Fraser Tweedale wrote:

Thanks for reviews.  Rebased and updated patches attached (and one
new patch).  No substantive changes to 92..94.  Patch order is:

92-2, 93-2, 94-2, 98, 90-3

Other comments inline.

Thanks,
Fraser

On Fri, Aug 12, 2016 at 11:33:28AM +0200, Jan Cholasta wrote:

Patch 0092: ACK

Patch 0093: ACK

Patch 0094: ACK


Please fix this PEP8 issue before pushing:

./ipaserver/plugins/cert.py:597:17: W503 line break before binary operator


Patch 0098: ACK



Patch 0090:

1) Generic otherNames (san_other) do not work correctly. The OID is not
included in the value and names with complex type other than
KerberosPrincipal are not parsed correctly. The value should include the OID
and DER blob of the name.


Updated to use "OID:b64(DER)" as the attribute value.


OK.




2) With --all, san_other should be included in the result for all
otherNames, even the known ones, to provide (limited) forward compatibility.


Done; when --all given, known otherName kinds are included in
'san_other' attribute in addition to their own attribute.


OK.




3) Do we have to support *all* the name types? I mean we could, for the sake
of completeness, but it might be easier to just keep the few ones we
actually care about (email, DNS name, principal name, UPN and directory name
in your patch 0095).


Yeah, why not support them all?  See also Petr's comments in other
branch of thread.


Works for me, but see Lukáš's reply, I think he has a point. Maybe we can
make a compromise and show only supported name types by default and
everything with --all?


Now only showing DNSName, RFC822Name, DirectoryName, UPN and
KRBPrincipalName unless --all is given.




4)

+obj.setdefault(attr_name, []).append(unicode(name))

The value should not (always) be unicode, but of the type declared by the
param (unicode or ipapython.kerberos.Principal or
ipapython.dnsutil.DNSName).


I now pass the value to the constructor of whatever type the
parameter uses:

attr_value = self.params[attr_name].type(name_formatted)
obj.setdefault(attr_name, []).append(attr_value)


OK.


5) san_directoryname should be a DNParam rather than Str.


Fixed, thanks.



6) Could we use "Subject " instead of "Subject Alternative Name
()" for labels? Or something else which is shorter and has the
name type more "visible" than the current form.


No worries.



7) The patch needs a rebase.


Thanks, ACK, but I have a couple additional nitpicks:

8) I think we should move the san_* param definitions right after 
subject param definition, so that they are visually close in CLI output.



9) san_directoryname should be added to default_attrs in patch 95, not here.

I took the liberty of fixing these myself.

Pushed to master: 48aaf2bbf5df6dcedaa466753c8fafb478cb31b2

Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0220 move /bin/ipa to freeipa-client

2016-08-25 Thread Jan Cholasta

Hi,

On 25.8.2016 11:27, Alexander Bokovoy wrote:

Hi,

attached patch moves ipa CLI to freeipa-client and obsoletes
freeipa-admintools


The Obsoletes (both) should be on version < 4.4.1 rather than 
%{version}, as per Fedora packaging guidelines [1].


Please move the Obsoletes and Provides on %{name}-admintools right below 
Group (Obsoletes first) and put a newline between the %{alt_name}-client 
and %{alt_name}-admintools blocks, for consistent layout accross all 
subpackages in the spec file.




Solves https://fedorahosted.org/freeipa/ticket/5934

Here is how upgrade looks when running 'dnf':

Upgrading:
freeipa-client x86_64
4.4.0.201608250913GIT9c20682-0.fc24@commandline
146 k
replacing  freeipa-admintools.noarch
4.4.0.201608051228GIT590e30f-0.fc24


I'm going to test with yum as well, for RHEL and CentOS.

Honza

[1] 
<https://fedoraproject.org/wiki/Upgrade_paths_%E2%80%94_renaming_or_splitting_packages>


--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0091 Allow full customisability of CA subject name

2016-08-25 Thread Jan Cholasta

On 22.8.2016 07:00, Fraser Tweedale wrote:

On Fri, Aug 19, 2016 at 08:09:33PM +1000, Fraser Tweedale wrote:

On Mon, Aug 15, 2016 at 10:54:25PM +1000, Fraser Tweedale wrote:

On Mon, Aug 15, 2016 at 02:08:54PM +0200, Jan Cholasta wrote:

On 19.7.2016 12:05, Jan Cholasta wrote:

On 19.7.2016 11:54, Fraser Tweedale wrote:

On Tue, Jul 19, 2016 at 09:36:17AM +0200, Jan Cholasta wrote:

Hi,

On 15.7.2016 07:05, Fraser Tweedale wrote:

On Fri, Jul 15, 2016 at 03:04:48PM +1000, Fraser Tweedale wrote:

The attached patch is a work in progress for
https://fedorahosted.org/freeipa/ticket/2614 (BZ 828866).

I am sharing now to make the approach clear and solicit feedback.

It has been tested for server install, replica install (with and
without CA) and CA-replica install (all hosts running master+patch).

Migration from earlier versions and server/replica/CA install on a
CA-less deployment are not yet tested; these will be tested over
coming days and patch will be tweaked as necessary.

Commit message has a fair bit to say so I won't repeat here but let
me know your questions and comments.

Thanks,
Fraser


It does help to attach the patch, of course ^_^


IMO explicit is better than implicit, so instead of introducing
additional
magic around --subject, I would rather add a new separate option for
specifying the CA subject name (I think --ca-subject, for consistency
with
--ca-signing-algorithm).


The current situation - the --subject argument which specifies the
not the subject but the subject base, is confusing enough (to say
nothing of the limitations that give rise to the RFE).

Retaining --subject for specifying the subject base and adding
--ca-subject for specifying the *actual* subject DN gets us over the
line in terms of the RFE, but does not make the installer less
confusing.  This is why I made --subject accept the full subject DN,
with provisions to retain existing behaviour.

IMO if we want to have separate arguments for subject DN and subject
base (I am not against it), let's bite the bullet and name arguments
accordingly.  --subject should be used to specify full Subject DN,
--subject-base (or similar) for specifying subject base.


IMHO --ca-subject is better than --subject, because it is more explicit
whose subject name that is (the CA's). I agree that --subject should be
deprecated and replaced with --subject-base.



(I intentionally defer discussion of specific behaviour if one, none
or both are specified; let's resolve the question or renaming /
changing meaning of arguments first).



By specifying the option you would override the default "CN=Certificate
Authority,$SUBJECT_BASE" subject name. If --external-ca was not used,
additional validation would be done to make sure the subject name meets
Dogtag's expectations. Actually, it might make sense to always do the
additional validation, to be able to print a warning that if a
Dogtag-incompatible subject name is used, it won't be possible to
change the
CA cert chaining from externally signed to self-signed later.

Honza


Bump, any update on this?


I have an updated patch that fixes some issues Sebastian encountered
in testing, but I've not yet tackled the main change requested by
Honza (in brief: adding --ca-subject for specifying that, adding
--subject-base for specifying that, and deprecating --subject;
Sebastian, see discussion above and feel free to give your
thoughts).  I expect I'll get back onto this work within the next
few days.


Update: I've got an updated version of patch almost ready for
review, but I'm still ironing out some wrinkles in replica
installation.

Expect to be able to send it Monday or Tuesday for review.


Updated patch attached.

I expect it will take a while to review, test and get the ACK, but
in any case: IMO it should not be merged until after ipa-4-4 branch
gets created.


1) Please fix these:

$ git show -U0 | pep8 --diff
./ipaserver/install/cainstance.py:508:13: E128 continuation line 
under-indented for visual indent

./ipaserver/install/dsinstance.py:259:5: E303 too many blank lines (2)
./ipaserver/install/installutils.py:999:1: E302 expected 2 blank lines, 
found 1
./ipaserver/install/server/common.py:161:80: E501 line too long (101 > 
79 characters)
./ipaserver/install/server/replicainstall.py:93:80: E501 line too long 
(82 > 79 characters)
./ipaserver/install/server/replicainstall.py:604:80: E501 line too long 
(81 > 79 characters)



2) Please put 3rd party library (such as six) imports between standard 
library imports and ipa imports.



3) ipa-ca-install should also have the --subject-base and --ca-subject 
options.



4) Please use the original method of getting the configured subject base 
from ipacertificatesubjectbase of the config object rather than 
DSInstance.find_subject_base(), which is horrendous and should in fact 
be obliterated (not necessarily in this patch).



5) You can use "unicode(x509.get_subject(cert))" to get subject name of 
a cert instead of &

Re: [Freeipa-devel] [PATCH] 0003 Validate key in otptoken-add

2016-08-23 Thread Jan Cholasta

On 22.8.2016 19:08, Tomas Krizek wrote:

I've attached the updated patch. Hopefully I didn't forget anything else
this time.


On 08/22/2016 05:48 PM, Martin Basti wrote:


On 22.08.2016 10:22, Tomas Krizek wrote:


Seems like a good idea, I'm attaching the updated patch. Autofill
does work when the param is required.


On 08/19/2016 04:19 PM, Martin Basti wrote:




On 16.08.2016 17:35, Tomas Krizek wrote:

Hi,

the attached patch fixes an error message when user provides an
empty key while adding otp token.

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





I'm curious why we don't fix it here:

OTPTokenKey('ipatokenotpkey?',
cli_name='key',
label=_('Key'),
doc=_('Token secret (Base32; default: random)'),
default_from=lambda: os.urandom(KEY_LENGTH),
autofill=True,
flags=('no_display', 'no_update', 'no_search'),
),


If OTPTokenKey is mandratory, it should be required param (autofill
should work in this case too)

Martin^2


--
Tomas Krizek


You changed API, you must regenerate API.txt (./makeapi) and increment
minor version in VERSION file

Option 'ipatokenotpkey?' in command 'otptoken_add/1' in API file not found
Options count in otptoken_add of 22 doesn't match expected: 23
Option ipatokenotpkey of command otptoken_add in ipalib, not in API file:
OTPTokenKey('ipatokenotpkey', autofill=True, cli_name='key')


NACK, this is a backward incompatible change.

AFAICT the option should remain optional, see the doc string:

Token secret (Base32; default: random)
  ^^^

--
Jan Cholasta

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


Re: [Freeipa-devel] [Design Review Request] V4/Automatic_Certificate_Request_Generation

2016-08-23 Thread Jan Cholasta

On 8.8.2016 22:23, Ben Lipton wrote:

On 07/25/2016 07:45 AM, Jan Cholasta wrote:

On 25.7.2016 13:11, Alexander Bokovoy wrote:

On Mon, 25 Jul 2016, Jan Cholasta wrote:

On 20.7.2016 16:05, Ben Lipton wrote:

Hi,

Thanks very much for the feedback! Some responses below; I hope you'll
let me know what you think of my reasoning.


On 07/20/2016 04:20 AM, Jan Cholasta wrote:

Hi,

On 17.6.2016 00:06, Ben Lipton wrote:

On 06/14/2016 08:27 AM, Ben Lipton wrote:

Hello all,

I have written up a design proposal for making certificate requests
easier to generate when using alternate certificate profiles:
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation.



The use case for this is described in
https://fedorahosted.org/freeipa/ticket/4899. I will be working on
implementing this design over the next couple of months. If you
have
the time and interest, please take a look and share any comments or
concerns that you have.

Thanks!

Ben


Just a quick update to say that I've created a new document that
covers
the proposed schema additions in a more descriptive way (with
diagrams!)
I'm very new to developing with LDAP, so some more experienced
eyes on
the proposal would be very helpful, even if you don't have time to
absorb the full design. Please take a look at
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation/Schema



if you have a chance.


I finally had a chance to take a look at this, here are some
comments:

1) I don't like how transformation rules are tied to a particular
helper and have to be duplicated for each of them. They should be
generic and work with any helper, as helpers are just an
implementation detail and their resulting data is the same.

In fact, I think I would prefer if the CSR was generated using
python-cryptography's CertificateSigningRequestBuilder [1] rather
than
openssl or certutil or any other command line tool.


There are lots of tools that users might want to use to manage their
private keys, so I don't know if we can assume that whatever
library we
prefer will actually be able to access the private key to sign a CSR,
which is why I thought it would be useful to support more than one.


python-cryptography has the notion of backends, which allow it to
support multiple crypto implementations. Upstream it currently
supports only OpenSSL [2], but some work has been done on PKCS#11
backend [3], which provides support for HSMs and soft-tokens (like NSS
databases).

Alternatively, for NSS databases (and other "simple" cases), you can
generate the private key with python-cryptography using the default
backend, export it to a file and import the file to the target
database, so you don't actually need the PKCS#11 backend for them.

So, the only thing that's currently lacking is HSM support, but given
that we don't support HSMs in IPA nor in certmonger, I don't think
it's an issue for now.


The
purpose of the mapping rule is to tie together the transformation
rules
that produce the same data into an object that's
implementation-agnostic, so that profiles referencing those rules are
automatically compatible with all the helper options.


They are implementation-agnostic, as long as you consider `openssl`
and `certutil` the only implementations :-) But I don't think this
solution scales well to other possible implementations.

Anyway, my main grudge is that the transformation rules shouldn't
really be stored on and processed by the server. The server should
know the *what* (mapping rules), but not the *how* (transformation
rules). The *how* is an implementation detail and does not change in
time, so there's no benefit in handling it on the server. It should be
handled exclusively on the client, which I believe would also make the
whole thing more robust (it would not be possible for a bug on the
server to break all the clients).

This is a good point. However, for the scope of Ben's project can we
limit it by openssl and certutil support? Otherwise Ben wouldn't be able
to complete the project in time.


I'm fine with that, but I don't think it's up to me :-)




This is turning out to be a common (and, I think, reasonable) reaction
to the proposal. It is rather complex, and I worry that it will be
difficult to configure. On the other hand, there is some hidden
complexity to enabling a simpler config format, as well. One of the
goals of the project as it was presented to me was to allow the
creation
of profiles that add certificate extensions *that FreeIPA doesn't yet
know about*. With the current proposal, one only has to add a rule
generating text that the helper will understand.


... which will be possible only as long as the helper understands the
extension. Which it might not, thus the current proposal works only
for *some* extensions that FreeIPA doesn't yet support.

We can go ad infinitum here but with any helper implementation, be it
python-cryptography or anything else, you will need to have a support
there as well.

Re: [Freeipa-devel] invoking ipa-certupdate from within installer

2016-08-22 Thread Jan Cholasta

Hi,

On 22.8.2016 09:37, Fraser Tweedale wrote:

#6019 requires adding tracking requests for existing lightweight CAs
as part of replica installation.  ipa-certupdate has logic to do
this.

Before I go ahead and implement, there are a few approaches I want
to mention and seek feedback from team members before I commit to
one.

1. invoke ipa-certupdate as a subprocess, from
CAInstance.configure_replica.  This is the simplest approach.  Not
much else to say about it, really :)

2. invoke ipa-certupdate's main() from the installer.  This is
slightly more work because currently it would fail due to API
already having been initialised.

3. extract all logic for adding tracking requests such that it can
be invoked separately; then refactor ipa-certupdate to call it as
well as calling it from CAInstance.configure_replica.  This is the
most work.

I lean towards (1) or (3).  If you wish it to be done a certain way
say your piece.


(4) Extract the relevant code from ipa-certupdate into a separate 
function and call it from CAInstance.configure_replica().


I would not go with (1) or (2) because it does more than track the 
certs. I would also not go with (3) because it requires extensive 
changes not suitable for 4.4.




Thanks,
Fraser



Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names

2016-08-22 Thread Jan Cholasta

On 19.8.2016 13:11, Fraser Tweedale wrote:

Bump for review.

On Mon, Aug 15, 2016 at 05:15:16PM +1000, Fraser Tweedale wrote:

Thanks for reviews.  Rebased and updated patches attached (and one
new patch).  No substantive changes to 92..94.  Patch order is:

92-2, 93-2, 94-2, 98, 90-3

Other comments inline.

Thanks,
Fraser

On Fri, Aug 12, 2016 at 11:33:28AM +0200, Jan Cholasta wrote:

Patch 0092: ACK

Patch 0093: ACK

Patch 0094: ACK


Please fix this PEP8 issue before pushing:

./ipaserver/plugins/cert.py:597:17: W503 line break before binary operator


Patch 0098: ACK



Patch 0090:

1) Generic otherNames (san_other) do not work correctly. The OID is not
included in the value and names with complex type other than
KerberosPrincipal are not parsed correctly. The value should include the OID
and DER blob of the name.


Updated to use "OID:b64(DER)" as the attribute value.


OK.




2) With --all, san_other should be included in the result for all
otherNames, even the known ones, to provide (limited) forward compatibility.


Done; when --all given, known otherName kinds are included in
'san_other' attribute in addition to their own attribute.


OK.




3) Do we have to support *all* the name types? I mean we could, for the sake
of completeness, but it might be easier to just keep the few ones we
actually care about (email, DNS name, principal name, UPN and directory name
in your patch 0095).


Yeah, why not support them all?  See also Petr's comments in other
branch of thread.


Works for me, but see Lukáš's reply, I think he has a point. Maybe we 
can make a compromise and show only supported name types by default and 
everything with --all?





4)

+obj.setdefault(attr_name, []).append(unicode(name))

The value should not (always) be unicode, but of the type declared by the
param (unicode or ipapython.kerberos.Principal or
ipapython.dnsutil.DNSName).


I now pass the value to the constructor of whatever type the
parameter uses:

attr_value = self.params[attr_name].type(name_formatted)
obj.setdefault(attr_name, []).append(attr_value)


OK.


5) san_directoryname should be a DNParam rather than Str.


6) Could we use "Subject " instead of "Subject Alternative 
Name ()" for labels? Or something else which is shorter and 
has the name type more "visible" than the current form.



7) The patch needs a rebase.


--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 689] tests: fix test_ipalib.test_frontend.test_Object

2016-08-21 Thread Jan Cholasta

On 18.8.2016 11:01, Martin Basti wrote:



On 18.08.2016 10:56, Petr Spacek wrote:

On 18.8.2016 10:08, Jan Cholasta wrote:

SSIA

Could you add one sentence or a link to a ticket which forced this
change?

When reading the patch, I have no way to say why the change is
necessary - so
it is impossible to verify correctness. (Sure, the test will pass, but
I have
no way to distinguish incorrect test passing on incorrect
implementation vs.
correct test passing on correct implementation.)


+1

and add there this link also https://fedorahosted.org/freeipa/ticket/6188


Updated patch attached.

--
Jan Cholasta
From aed35e08a8f61a022bcac6ef15d012566e081792 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Thu, 18 Aug 2016 10:04:59 +0200
Subject: [PATCH] tests: fix test_ipalib.test_frontend.test_Object

Update the fake API object to match the real API object interface including
the changes introduced with the API compatibility feature.

https://fedorahosted.org/freeipa/ticket/6188
---
 ipatests/test_ipalib/test_frontend.py | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/ipatests/test_ipalib/test_frontend.py b/ipatests/test_ipalib/test_frontend.py
index 892328a..6a0e488 100644
--- a/ipatests/test_ipalib/test_frontend.py
+++ b/ipatests/test_ipalib/test_frontend.py
@@ -922,6 +922,9 @@ class test_Object(ClassChecker):
 self.name = '%s_%s' % (obj_name, attr_name)
 else:
 self.name = name
+self.bases = (DummyAttribute,)
+self.version = '1'
+self.full_name = '{}/{}'.format(self.name, self.version)
 self.param = frontend.create_param(attr_name)
 
 def __clone__(self, attr_name):
@@ -940,15 +943,18 @@ class test_Object(ClassChecker):
 methods_format = 'method_%d'
 
 class FakeAPI(object):
-Method = NameSpace(
-get_attributes(cnt, methods_format)
-)
+def __init__(self):
+self._API__plugins = get_attributes(cnt, methods_format)
+self._API__default_map = {}
+self.Method = plugable.APINameSpace(self, DummyAttribute)
 def __contains__(self, key):
 return hasattr(self, key)
 def __getitem__(self, key):
 return getattr(self, key)
 def is_production_mode(self):
 return False
+def _get(self, plugin):
+return plugin
 api = FakeAPI()
 assert len(api.Method) == cnt * 3
 
-- 
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] 0084 cert-revoke: fix permission check bypass

2016-08-21 Thread Jan Cholasta

Hi,

On 19.8.2016 12:55, Fraser Tweedale wrote:

This patch fixes CVE-2016-5404.  Versions for master, ipa-4-3 and
ipa-4-2 branches are attached.


ACKed off-list.

Pushed to:
master: cf74584d0f772f3f5eccc1d30c001e4212a104fd
ipa-4-2: e26ec4c220b10a20fa7527ad7173c89c3032e480
ipa-4-3: 7eb1502863408d869dc2e706a5e194ad122997bf



Thanks,
Fraser


--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 0112-7] Speeding up cli help

2016-08-18 Thread Jan Cholasta

On 18.8.2016 11:06, David Kupka wrote:

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.


Works for me, ACK.

Pushed to master: b6d5ed139b261b5db078ab652d22ea1d3b8092d3

--
Jan Cholasta

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


[Freeipa-devel] [PATCH 689] tests: fix test_ipalib.test_frontend.test_Object

2016-08-18 Thread Jan Cholasta

SSIA

--
Jan Cholasta
From c3f3ffd235b39fbdc61d8ae0b3f55eca97613499 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Thu, 18 Aug 2016 10:04:59 +0200
Subject: [PATCH] tests: fix test_ipalib.test_frontend.test_Object

---
 ipatests/test_ipalib/test_frontend.py | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/ipatests/test_ipalib/test_frontend.py b/ipatests/test_ipalib/test_frontend.py
index c3dd910..ac299e2 100644
--- a/ipatests/test_ipalib/test_frontend.py
+++ b/ipatests/test_ipalib/test_frontend.py
@@ -922,6 +922,9 @@ class test_Object(ClassChecker):
 self.name = '%s_%s' % (obj_name, attr_name)
 else:
 self.name = name
+self.bases = (DummyAttribute,)
+self.version = '1'
+self.full_name = '{}/{}'.format(self.name, self.version)
 self.param = frontend.create_param(attr_name)
 
 def __clone__(self, attr_name):
@@ -940,15 +943,18 @@ class test_Object(ClassChecker):
 methods_format = 'method_%d'
 
 class FakeAPI(object):
-Method = NameSpace(
-get_attributes(cnt, methods_format)
-)
+def __init__(self):
+self._API__plugins = get_attributes(cnt, methods_format)
+self._API__default_map = {}
+self.Method = plugable.APINameSpace(self, DummyAttribute)
 def __contains__(self, key):
 return hasattr(self, key)
 def __getitem__(self, key):
 return getattr(self, key)
 def is_production_mode(self):
 return False
+def _get(self, plugin):
+return plugin
 api = FakeAPI()
 assert len(api.Method) == cnt * 3
 
-- 
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] 0001 Added new authentication method

2016-08-17 Thread Jan Cholasta

On 17.8.2016 16:33, Stanislav Laznicka wrote:

On 08/17/2016 04:11 PM, Tibor Dudlak wrote:


On Wed, Aug 17, 2016 at 3:36 PM, Stanislav Laznicka
<slazn...@redhat.com <mailto:slazn...@redhat.com>> wrote:

On 08/16/2016 03:16 PM, Tibor Dudlak wrote:

Hi,

I have edited this patch after review. It should be okay now.

Thank you.

On Thu, Aug 11, 2016 at 7:49 PM, Petr Vobornik
<pvobo...@redhat.com <mailto:pvobo...@redhat.com>> wrote:

On 08/11/2016 07:21 PM, Martin Basti wrote:
>
>
> On 11.08.2016 18:57, Pavel Vomacka wrote:
>>
>>
>> On 08/11/2016 02:00 PM, Petr Vobornik wrote:
>>> On 08/11/2016 10:54 AM, Alexander Bokovoy wrote:
>>>> On Thu, 11 Aug 2016, Jan Cholasta wrote:
>>>>> On 4.8.2016 17:27, Jan Pazdziora wrote:
>>>>>> On Wed, Aug 03, 2016 at 10:29:52AM +0300, Alexander
Bokovoy wrote:
>>>>>>> Got it. One thing I would correct, though, -- don't use
>>>>>>> kadmin.local, we
>>>>>>> do support setting ok_as_delegate on the service
principals via IPA
>>>>>>> CLI:
>>>>>>> $ ipa service-mod --help |grep -A1 ok-as-delegate
>>>>>>> --ok-as-delegate=BOOL
>>>>>>>Client credentials may be
delegated to the
>>>>>>> service
>>>>>> I've tried
>>>>>>
>>>>>>  ipa service-mod --ok-as-delegate=True
HTTP/$(hostname)
>>>>>>
>>>>>> but that does not seem to have the same effect as
>>>>>>
>>>>>>  modprinc +ok_to_auth_as_delegate
HTTP/ipa.example.test
>>>>>>
>>>>>> -- obtaining the delegated certificated fails.
>>>>> That's because ok_as_delegate and
ok_to_auth_as_delegate are different
>>>>> flags.
>>>> Right. The following patch adds ok_to_auth_as_delegate
to the service
>>>> principal.
>>>>
>>>> I haven't added any tickets to it yet.
>>>>
>>>>
>>> This might deserve also nice Web UI checkbox similar to
"Trusted for
>>> delegation". CCing Pavel.
>>>
>> Here is patch with new checkbox. It is without ticket in
commit message so
>> once we will have the ticket I will send another patch
witch updated commit
>> message.
>
> https://fedorahosted.org/freeipa/newticket
<https://fedorahosted.org/freeipa/newticket>
>
> ;-)

It's prerequisite for
https://fedorahosted.org/freeipa/ticket/5764
<https://fedorahosted.org/freeipa/ticket/5764> so we
might use that.



Please, add your answers at the end of the previous mail in the
future.

Also, your patch raises pep8 errors:
./ipaserver/plugins/xmlserver.py:31:80: E501 line too long (189 >
79 characters)
./ipaserver/rpcserver.py:885:5: E113 unexpected indentation

Could you please fix them?


Hi,

thanks for review Stanislav. I understand
./ipaserver/rpcserver.py:885:5: E113 unexpected indentation, that is
my fault but really do not understand first one. Is there policy that
you decided not to patch existing files, even if there was obviously
longer line before patch until it is not necessary?
Anyway I hope it should be ok now.

Thank you.


There's a policy to try to be pep8 compliant as much as we can with any
new patches. Your new patch would still raise some pep8 errors, please
see the attached patch that should be ok. If it's ok with you then ACK,
it seems to be working.


(16:54:22) pvoborni_: tdudlak: muzem pushnout tu standovu verzi tveho 
patche?

(16:54:36) tdudlak: jasne
(16:55:12) pvoborni_: jcholast: ^

Pushed to master: d25a0725c0e09891bd0df927641dac878dfe6a7d

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0001 Added new authentication method

2016-08-17 Thread Jan Cholasta

On 17.8.2016 16:36, Stanislav Laznicka wrote:

On 08/17/2016 03:50 PM, Pavel Vomacka wrote:




On 08/17/2016 02:42 PM, Pavel Vomacka wrote:



On 08/11/2016 07:49 PM, Petr Vobornik wrote:

On 08/11/2016 07:21 PM, Martin Basti wrote:


On 11.08.2016 18:57, Pavel Vomacka wrote:


On 08/11/2016 02:00 PM, Petr Vobornik wrote:

On 08/11/2016 10:54 AM, Alexander Bokovoy wrote:

On Thu, 11 Aug 2016, Jan Cholasta wrote:

On 4.8.2016 17:27, Jan Pazdziora wrote:

On Wed, Aug 03, 2016 at 10:29:52AM +0300, Alexander Bokovoy
wrote:

Got it. One thing I would correct, though, -- don't use
kadmin.local, we
do support setting ok_as_delegate on the service principals
via IPA
CLI:
$ ipa service-mod --help |grep -A1 ok-as-delegate
--ok-as-delegate=BOOL
Client credentials may be delegated
to the
service

I've tried

  ipa service-mod --ok-as-delegate=True HTTP/$(hostname)

but that does not seem to have the same effect as

  modprinc +ok_to_auth_as_delegate HTTP/ipa.example.test

-- obtaining the delegated certificated fails.

That's because ok_as_delegate and ok_to_auth_as_delegate are
different
flags.

Right. The following patch adds ok_to_auth_as_delegate to the
service
principal.

I haven't added any tickets to it yet.



This might deserve also nice Web UI checkbox similar to "Trusted for
delegation". CCing Pavel.


Here is patch with new checkbox. It is without ticket in commit
message so
once we will have the ticket I will send another patch witch
updated commit
message.

https://fedorahosted.org/freeipa/newticket

;-)

It's prerequisite for https://fedorahosted.org/freeipa/ticket/5764
so we
might use that.

Thank you, patch with updated commit message attached.




Attached patch adds checkbox also to host page.


Thank you, works as expected. ACK.


Pushed to master: c36d721a01106e24186bd6b2f0fc74d7af31d5ba

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0001 Added new authentication method

2016-08-17 Thread Jan Cholasta

On 17.8.2016 16:35, Stanislav Laznicka wrote:

On 08/17/2016 03:58 PM, Alexander Bokovoy wrote:

On Thu, 11 Aug 2016, Petr Vobornik wrote:

On 08/11/2016 07:21 PM, Martin Basti wrote:



On 11.08.2016 18:57, Pavel Vomacka wrote:



On 08/11/2016 02:00 PM, Petr Vobornik wrote:

On 08/11/2016 10:54 AM, Alexander Bokovoy wrote:

On Thu, 11 Aug 2016, Jan Cholasta wrote:

On 4.8.2016 17:27, Jan Pazdziora wrote:

On Wed, Aug 03, 2016 at 10:29:52AM +0300, Alexander Bokovoy wrote:

Got it. One thing I would correct, though, -- don't use
kadmin.local, we
do support setting ok_as_delegate on the service principals
via IPA
CLI:
$ ipa service-mod --help |grep -A1 ok-as-delegate
--ok-as-delegate=BOOL
   Client credentials may be delegated to the
service

I've tried

 ipa service-mod --ok-as-delegate=True HTTP/$(hostname)

but that does not seem to have the same effect as

 modprinc +ok_to_auth_as_delegate HTTP/ipa.example.test

-- obtaining the delegated certificated fails.

That's because ok_as_delegate and ok_to_auth_as_delegate are
different
flags.

Right. The following patch adds ok_to_auth_as_delegate to the
service
principal.

I haven't added any tickets to it yet.



This might deserve also nice Web UI checkbox similar to "Trusted for
delegation". CCing Pavel.


Here is patch with new checkbox. It is without ticket in commit
message so
once we will have the ticket I will send another patch witch
updated commit
message.


https://fedorahosted.org/freeipa/newticket

;-)


It's prerequisite for https://fedorahosted.org/freeipa/ticket/5764 so we
might use that.

Sounds good. Patch with updated commit message is attached.



Thank you for the updated patch, works as expected so ACK.


Pushed to master: 1c73ac91a4c76cbada91f2b30d8b731b91af5195

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 688] server install: do not prompt for cert file PIN repeatedly

2016-08-17 Thread Jan Cholasta

On 17.8.2016 15:07, Pavel Vomacka wrote:



On 08/17/2016 10:24 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes <https://fedorahosted.org/freeipa/ticket/6032>.

Honza




ACK.


Thanks.

Pushed to master: 4ee426a68ec60370eee6f5aec917ecce444840c7

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH 0112-7] Speeding up cli help

2016-08-17 Thread Jan Cholasta

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

--
Jan Cholasta

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


[Freeipa-devel] [PATCH 688] server install: do not prompt for cert file PIN repeatedly

2016-08-17 Thread Jan Cholasta

Hi,

the attached patch fixes <https://fedorahosted.org/freeipa/ticket/6032>.

Honza

--
Jan Cholasta
From 5dc9e3a60dcdec0d9cd00bfc8819c1c01e2c4e0f Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 16 Aug 2016 17:34:06 +0200
Subject: [PATCH] server install: do not prompt for cert file PIN repeatedly

Prompt for PIN only once in interactive mode.

This fixes ipa-server-install, ipa-server-certinstall and
ipa-replica-prepare prompting over and over when the PIN is empty.

https://fedorahosted.org/freeipa/ticket/6032
---
 ipaserver/install/ipa_replica_prepare.py| 6 +++---
 ipaserver/install/ipa_server_certinstall.py | 3 ++-
 ipaserver/install/server/install.py | 6 +++---
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index a6f0f1e..9467276 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -307,7 +307,7 @@ class ReplicaPrepare(admintool.AdminTool):
 if options.http_pin is None:
 options.http_pin = installutils.read_password(
 "Enter Apache Server private key unlock",
-confirm=False, validate=False)
+confirm=False, validate=False, retry=False)
 if options.http_pin is None:
 raise admintool.ScriptError(
 "Apache Server private key unlock password required")
@@ -321,7 +321,7 @@ class ReplicaPrepare(admintool.AdminTool):
 if options.dirsrv_pin is None:
 options.dirsrv_pin = installutils.read_password(
 "Enter Directory Server private key unlock",
-confirm=False, validate=False)
+confirm=False, validate=False, retry=False)
 if options.dirsrv_pin is None:
 raise admintool.ScriptError(
 "Directory Server private key unlock password required")
@@ -335,7 +335,7 @@ class ReplicaPrepare(admintool.AdminTool):
 if options.pkinit_pin is None:
 options.pkinit_pin = installutils.read_password(
 "Enter Kerberos KDC private key unlock",
-confirm=False, validate=False)
+confirm=False, validate=False, retry=False)
 if options.pkinit_pin is None:
 raise admintool.ScriptError(
 "Kerberos KDC private key unlock password required")
diff --git a/ipaserver/install/ipa_server_certinstall.py b/ipaserver/install/ipa_server_certinstall.py
index 5ab4730..0a8fb21 100644
--- a/ipaserver/install/ipa_server_certinstall.py
+++ b/ipaserver/install/ipa_server_certinstall.py
@@ -92,7 +92,8 @@ class ServerCertInstall(admintool.AdminTool):
 
 if self.options.pin is None:
 self.options.pin = installutils.read_password(
-"Enter private key unlock", confirm=False, validate=False)
+"Enter private key unlock",
+confirm=False, validate=False, retry=False)
 if self.options.pin is None:
 raise admintool.ScriptError(
 "Private key unlock password required")
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 8dc7a68..8d7fa9c 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -488,7 +488,7 @@ def install_check(installer):
 if options.http_pin is None:
 options.http_pin = installutils.read_password(
 "Enter Apache Server private key unlock",
-confirm=False, validate=False)
+confirm=False, validate=False, retry=False)
 if options.http_pin is None:
 raise ScriptError(
 "Apache Server private key unlock password required")
@@ -504,7 +504,7 @@ def install_check(installer):
 if options.dirsrv_pin is None:
 options.dirsrv_pin = read_password(
 "Enter Directory Server private key unlock",
-confirm=False, validate=False)
+confirm=False, validate=False, retry=False)
 if options.dirsrv_pin is None:
 raise ScriptError(
 "Directory Server private key unlock password required")
@@ -520,7 +520,7 @@ def install_check(installer):
 if options.pkinit_pin is None:
 options.pkinit_pin = read_password(
 "Enter Kerberos KDC private key unlock",
-confirm=False, validate=False)
+confirm=False, validate=False, retry=False)
 if options.pkinit_pin is None:
 raise ScriptError(
   

  1   2   3   4   5   6   7   8   9   10   >