Re: [Freeipa-devel] [PATCH 0413] fix permission: Read Replication Agreements

[Jan Cholasta]

Hi,

On 17.2.2016 14:49, Martin Basti wrote:

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

Patch attached (for master, 4.3, 4.2)


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



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



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



Honza

--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCH 0011] Move freeipa certmonger helpers to libexecdir.

[Jan Cholasta]

On 18.2.2016 10:10, David Kupka wrote:

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

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

On 19.1.2016 14:26, Martin Kosek wrote:

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

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

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

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


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


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



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






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

Thanks for the catch.



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


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


--
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 0022-23] Coverity patches

[Jan Cholasta]

On 2.2.2016 13:36, Stanislav Laznicka wrote:

On 02/01/2016 02:24 PM, Jan Cholasta wrote:

On 1.2.2016 12:11, Petr Spacek wrote:

On 1.2.2016 09:03, Jan Cholasta wrote:

Hi,

On 29.1.2016 15:49, Martin Basti wrote:



On 29.01.2016 15:49, Stanislav Laznicka wrote:

Reworded the commits so that they better reflect what's going on
in those.

On 01/29/2016 02:49 PM, Stanislav Laznicka wrote:

Hello,

I made some patches based on the Coverity report from 18.1.2016.

Cheers,
Standa




NACK, see my previous email


I don't think this deserves 9 patches, 1 would be sufficient enough.


I would rather have it is separate patches as these fixes are largely
not
related. It will make bisecting easier.


Most of the fixes are cosmetic, which makes them related, and the rest
are small isolated changes, so in reality it would hardly make
bisecting easier and only increase the overhead. In the past we
usually had put such fixes into a single patch and AFAIK nobody
complained so far.


Squeezed the changes into two new patches, then. One for the very
cosmetic changes, one for possible bugs.


OK.





Patch 0013:

1) I think this unreachable return is intentional, as indicated by
the comment:

-#we shouldn't get here
-return [UNKNOWN_ERROR]


I would use
assert False, "we shouldn't get here"
neither we nor Coverity are confused when we hit the code path one day.

UNKNOWN_ERROR would pop up somewhere else and it will be harder to
find out
why the hell the code behaves as mad. Traceback will clearly indicate
that
there is a problem with the 'switch'.


Sure, my point is that returning None is no better than returning
UNKNOWN_ERROR.


Added assert as suggested. There should still be no way of getting to
it, though.


Petr^2 Spacek


2) How is this dead code?

-if options.mode == 'validate_pot' or options.mode ==
'validate_po':
+if options.mode in ('validate_pot', 'validate_po'):

-elif options.mode == 'create_test' or 'test_gettext':
+elif options.mode in ('create_test', 'test_gettext'):



Patch 0014-0015: LGTM


Actually scratch that, patch 0015 breaks correct code.


The dead code appears in the 'else' branch as the latter of these two
conditions always evaluates to True. The first condition change is just
a cosmetic one so that both of the conditions look the same.


OK.



Also removed the changes made in patch 0015.



Patch 0016: The original code is in fact correct.


Point taken, removed the change.


Patch 0017: This will break Python 3. The two branches are
performing the same
action, but with different data types.


This might undergo further investigation in the future as there is no
way how "bytes" instance could become an argument of this function (as
suggested). Not even the newest Python 3 patches from pviktori mention
this code.


OK. (This is not what Coverity was complaining about, though.)



Patch 0018: LGTM


Patch 0019: IMO the original code is better (None has a __class__
too, you know).


Made it more "Coverity friendly" yet nice enough modification.


Patch 0020: LGTM


It seems that there actually is a check that checks whether the input is
correct. It is called ad-hoc but that might be the test feature.
Therefore just added an assert so that Coverity does not complain.


OK.



Patch 0021: Please use the original error messages (there are no
requests
being added to D-Bus, but to certmonger).


Honza





Added error messages that reflect the situation better, then.


Could you please mention Coverity in the commit messages, so that it's 
clear why are we doing these changes? Otherwise LGTM.


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

[Simo Sorce]

On Sun, 2016-02-21 at 20:20 -0500, Nathaniel McCallum wrote:
> https://github.com/npmccallum/freeipa/pull/1
> 
> The above (pseudo) pull request contains four patches against FreeIPA
> to enable the insertion of Authentication Indicators into Kerberos
> tickets. The basic flow looks like this.
> 
> First, we patch ipa-pwd-extop to return a control indicating what
> authentication method succeeded resulting in a successful bind.
> 
> Second, we patch ipa-otpd to check the returned control to ensure that
> the bind resulted from an otp validation.
> 
> Third, we patch ipa-kdb to enable the KDC to return either the
> encrypted timestamp or encrypted challenge preauth mechanism when the
> user is configured for optional 2FA logins. Clients can then decide
> whether to do 1FA or 2FA login (for kinit, sane behavior already
> exists).
> 
> Forth, we patch ipa-kdb again to insert hard-coded authentication
> indicators for either OTP or RADIUS.
> 
> Some explanation is required for the first two patches. Currently, it
> is possible to do a 1FA through the otp preauthentication mechanism if
> the user is configured for doing optional 2FA. However, because we want
> to insert an authentication indicator in this code path, we need to
> guarantee that a request going through the otp preauth mechanism
> actually validates an OTP. This is the purpose of the control.
> 
> Items still on the TODO list:
> 
>   * Authentication Indicator enforcement
> - Upstream libkrb5 needs to grow funcs for reading indicators
> - Schema change to add indicators multi-value attr to services
> - ipa-kdb needs to implement check_policy_tgs()
> 
> 
>   * SSSD needs to learn to handle optional 2FA
> 
> I will write up a project page for all of this tomorrow. But this small
> code basically amounts to my brainstorming. It is not ready for merge,
> just basic review.
> 

It looks mostly ok, however the LDAP control part needs to be done as a
request/response pair.
A client that wishes to know what kind of authentication happened should
send a request control, and only in that case , the server will send the
associated reply control with the requested information.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

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

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

[Nathaniel McCallum]

https://github.com/npmccallum/freeipa/pull/1

The above (pseudo) pull request contains four patches against FreeIPA
to enable the insertion of Authentication Indicators into Kerberos
tickets. The basic flow looks like this.

First, we patch ipa-pwd-extop to return a control indicating what
authentication method succeeded resulting in a successful bind.

Second, we patch ipa-otpd to check the returned control to ensure that
the bind resulted from an otp validation.

Third, we patch ipa-kdb to enable the KDC to return either the
encrypted timestamp or encrypted challenge preauth mechanism when the
user is configured for optional 2FA logins. Clients can then decide
whether to do 1FA or 2FA login (for kinit, sane behavior already
exists).

Forth, we patch ipa-kdb again to insert hard-coded authentication
indicators for either OTP or RADIUS.

Some explanation is required for the first two patches. Currently, it
is possible to do a 1FA through the otp preauthentication mechanism if
the user is configured for doing optional 2FA. However, because we want
to insert an authentication indicator in this code path, we need to
guarantee that a request going through the otp preauth mechanism
actually validates an OTP. This is the purpose of the control.

Items still on the TODO list:

  * Authentication Indicator enforcement
    - Upstream libkrb5 needs to grow funcs for reading indicators
    - Schema change to add indicators multi-value attr to services
    - ipa-kdb needs to implement check_policy_tgs()


  * SSSD needs to learn to handle optional 2FA

I will write up a project page for all of this tomorrow. But this small
code basically amounts to my brainstorming. It is not ready for merge,
just basic review.

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