Re: [Freeipa-devel] [PATCH 0049] Add support for protected tokens
On 13.6.2014 21:59, Nathaniel McCallum wrote: On Wed, 2014-06-11 at 12:43 -0400, Nathaniel McCallum wrote: On Wed, 2014-06-11 at 12:12 +0200, Ludwig Krispenz wrote: On 05/13/2014 04:33 PM, Jan Cholasta wrote: On 12.5.2014 21:02, Nathaniel McCallum wrote: On Thu, 2014-05-08 at 13:51 -0400, Simo Sorce wrote: On Thu, 2014-05-08 at 12:26 -0400, Nathaniel McCallum wrote: On Wed, 2014-05-07 at 11:17 -0400, Simo Sorce wrote: On Wed, 2014-05-07 at 09:54 -0400, Dmitri Pal wrote: On 05/07/2014 09:05 AM, Nathaniel McCallum wrote: On Wed, 2014-05-07 at 11:42 +0200, Jan Cholasta wrote: Hi, On 6.5.2014 17:08, Nathaniel McCallum wrote: On Tue, 2014-05-06 at 09:49 -0400, Nathaniel McCallum wrote: On Mon, 2014-05-05 at 12:42 -0400, Nathaniel McCallum wrote: This also constitutes a rethinking of the token ACIs after the introduction of SELFDN support. Admins, as before, have full access to all token permissions. Normal users have read/search/compare access to all of the non-secret data for tokens assigned to them, whether protected or non-protected. Users can add or delete non-protected tokens and modify most of their metadata. However they cannot create, delete or modify protected tokens. Regardless of whether the token is protected or not, users cannot change a token's ownership or unique identity. In contrast, admins can create protected tokens. This protects the token from deletion or modification when assigned to users. Additionally, when a user account is deleted, the assigned non-protected tokens are deleted but the protected tokens are merely orphaned. This permits the token to be reassigned without having to recreate it. This last point is particularly useful in the case of hardware tokens. https://fedorahosted.org/freeipa/ticket/4228 NOTE: This patch depends on my patch 0048. This new version makes ipatokenDisabled visible for token owners. It is also writable if the token is non-protected. This additionally fixes: https://fedorahosted.org/freeipa/ticket/4259 This new version changes the way the default value of protected is setup in accordance with the changes made for the review of my patch 0048.2. Nathaniel Is using the ipatokenprotected attribute the final design? No. Alternate designs are welcome. The code is easy enough to modify. I did not dig too deep into this, but I think it might be better to instead use the managedby attribute on a token to limit who can delete (or administer in other way) the token. On otptoken-add, managedby would be set to the whoami user DN, unless run with --protected, in which case managedby would be left empty. Then, when deleting a user, the token would be deleted only if the user manages the token. It seems to me that the mechanics of this are roughly the same as protected, just with a different syntax. The cost of this is more complex ACIs. In particular, we'd have to use two userdn clauses (is this possible?) instead of a simple filter. If there is a clear benefit, we can justify the more obtuse syntax. We usually try not to create new attributes until it is fully justified. I would like Simo to chime in on this. I would also prefer to reuse existing attributes and mechanism if possible and if it will not preclude future work. In this case, it sounds like managed-by has the appropriate meaning: who manages the token ? - myself, admin, other, none ? Nathaniel can you send 2 lines showing the difference in ACIs between using managed-by vs a new attribute ? These are the ACIs using the protected mechanism: aci: (targetfilter = (objectClass=ipaToken))(targetattrs = objectclass || description || ipatokenUniqueID || ipatokenDisabled || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial || ipatokenOwner || ipatokenProtected)(version 3.0; acl Users can read basic token info; allow (read, search, compare) userattr = ipatokenOwner#USERDN;) aci: (targetfilter = (objectClass=ipatokenTOTP))(targetattrs = ipatokenOTPalgorithm || ipatokenOTPdigits || ipatokenTOTPtimeStep)(version 3.0; acl Users can see TOTP details; allow (read, search, compare) userattr = ipatokenOwner#USERDN;) aci: (targetfilter = (objectClass=ipatokenHOTP))(targetattrs = ipatokenOTPalgorithm || ipatokenOTPdigits)(version 3.0; acl Users can see HOTP details; allow (read, search, compare) userattr = ipatokenOwner#USERDN;) aci: (targetfilter = ((objectClass=ipaToken)(!(ipatokenProtected=TRUE(targetattrs = description || ipatokenDisabled || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial)(version 3.0; acl Users can write basic token info; allow (write) userattr = ipatokenOwner#USERDN;) aci: (target = ldap:///ipatokenuniqueid=*,cn=otp,$SUFFIX;)(targetfilter = ((objectClass=ipaToken)(!(ipatokenProtected=TRUE)(version 3.0; acl Users can create and delete tokens; allow (add, delete) userattr = ipatokenOwner#SELFDN;) This is what they look like using managedBy (I have not tested this): aci:
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On 06/13/2014 10:20 PM, Simo Sorce wrote: [...] 2) and I think this is a MUCH bigger issue, the Admin users are unbounded and pass any Access Control Check and this means they can now retrieve any key for users or machines. It is already bad enough that admins can unconditionally set any key, but this at least leaves back a pretty big trail (the original client password/key fails to work), and is a necessary evil (password resets, hosts creation/recovery). But I am not very comfortable with the idea an admin can retrieve any key without actually ending up changing it. Petr do we have any short term plan to address the Admin's super ACI ? No, nothing in the short term. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0049] Add support for protected tokens
On 06/16/2014 09:17 AM, Jan Cholasta wrote: On 13.6.2014 21:59, Nathaniel McCallum wrote: On Wed, 2014-06-11 at 12:43 -0400, Nathaniel McCallum wrote: On Wed, 2014-06-11 at 12:12 +0200, Ludwig Krispenz wrote: On 05/13/2014 04:33 PM, Jan Cholasta wrote: On 12.5.2014 21:02, Nathaniel McCallum wrote: On Thu, 2014-05-08 at 13:51 -0400, Simo Sorce wrote: On Thu, 2014-05-08 at 12:26 -0400, Nathaniel McCallum wrote: On Wed, 2014-05-07 at 11:17 -0400, Simo Sorce wrote: On Wed, 2014-05-07 at 09:54 -0400, Dmitri Pal wrote: On 05/07/2014 09:05 AM, Nathaniel McCallum wrote: On Wed, 2014-05-07 at 11:42 +0200, Jan Cholasta wrote: Hi, On 6.5.2014 17:08, Nathaniel McCallum wrote: On Tue, 2014-05-06 at 09:49 -0400, Nathaniel McCallum wrote: On Mon, 2014-05-05 at 12:42 -0400, Nathaniel McCallum wrote: This also constitutes a rethinking of the token ACIs after the introduction of SELFDN support. Admins, as before, have full access to all token permissions. Normal users have read/search/compare access to all of the non-secret data for tokens assigned to them, whether protected or non-protected. Users can add or delete non-protected tokens and modify most of their metadata. However they cannot create, delete or modify protected tokens. Regardless of whether the token is protected or not, users cannot change a token's ownership or unique identity. In contrast, admins can create protected tokens. This protects the token from deletion or modification when assigned to users. Additionally, when a user account is deleted, the assigned non-protected tokens are deleted but the protected tokens are merely orphaned. This permits the token to be reassigned without having to recreate it. This last point is particularly useful in the case of hardware tokens. https://fedorahosted.org/freeipa/ticket/4228 NOTE: This patch depends on my patch 0048. This new version makes ipatokenDisabled visible for token owners. It is also writable if the token is non-protected. This additionally fixes: https://fedorahosted.org/freeipa/ticket/4259 This new version changes the way the default value of protected is setup in accordance with the changes made for the review of my patch 0048.2. Nathaniel Is using the ipatokenprotected attribute the final design? No. Alternate designs are welcome. The code is easy enough to modify. I did not dig too deep into this, but I think it might be better to instead use the managedby attribute on a token to limit who can delete (or administer in other way) the token. On otptoken-add, managedby would be set to the whoami user DN, unless run with --protected, in which case managedby would be left empty. Then, when deleting a user, the token would be deleted only if the user manages the token. It seems to me that the mechanics of this are roughly the same as protected, just with a different syntax. The cost of this is more complex ACIs. In particular, we'd have to use two userdn clauses (is this possible?) instead of a simple filter. If there is a clear benefit, we can justify the more obtuse syntax. We usually try not to create new attributes until it is fully justified. I would like Simo to chime in on this. I would also prefer to reuse existing attributes and mechanism if possible and if it will not preclude future work. In this case, it sounds like managed-by has the appropriate meaning: who manages the token ? - myself, admin, other, none ? Nathaniel can you send 2 lines showing the difference in ACIs between using managed-by vs a new attribute ? These are the ACIs using the protected mechanism: aci: (targetfilter = (objectClass=ipaToken))(targetattrs = objectclass || description || ipatokenUniqueID || ipatokenDisabled || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial || ipatokenOwner || ipatokenProtected)(version 3.0; acl Users can read basic token info; allow (read, search, compare) userattr = ipatokenOwner#USERDN;) aci: (targetfilter = (objectClass=ipatokenTOTP))(targetattrs = ipatokenOTPalgorithm || ipatokenOTPdigits || ipatokenTOTPtimeStep)(version 3.0; acl Users can see TOTP details; allow (read, search, compare) userattr = ipatokenOwner#USERDN;) aci: (targetfilter = (objectClass=ipatokenHOTP))(targetattrs = ipatokenOTPalgorithm || ipatokenOTPdigits)(version 3.0; acl Users can see HOTP details; allow (read, search, compare) userattr = ipatokenOwner#USERDN;) aci: (targetfilter = ((objectClass=ipaToken)(!(ipatokenProtected=TRUE(targetattrs = description || ipatokenDisabled || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial)(version 3.0; acl Users can write basic token info; allow (write) userattr = ipatokenOwner#USERDN;) aci: (target = ldap:///ipatokenuniqueid=*,cn=otp,$SUFFIX;)(targetfilter = ((objectClass=ipaToken)(!(ipatokenProtected=TRUE)(version 3.0; acl Users can create and delete tokens;
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On Fri, 13 Jun 2014, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
Other than the first point though, anything else looks good to me.
I read through the patch and apart from the points Simo made, I don't
have much complaints. One additional thing to do is how to test it --
how to create the payload? Since this code doesn't require anything
specific, it could be well tested through a CI infrastructure.
Additionally, man page is missing.
--
/ Alexander Bokovoy
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On 06/14/2014 12:05 AM, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
If you want to use idiomatic Python,
starmap(lambda a, b: a ^ b, izip(u, [ord(c) for c in l]))
can be rewritten as
[a ^ ord(b) for a, b in zip(u, l)]
(izip is overkill if the list isn't too long)
then you can use
dk.extend(u)
to keep a single list of ints in dk, and at the end,
return ''.join(chr(c) for c in dk[:self.klen])
to turn part you need into a string.
It would be nice to have some tests for this.
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
Other than the first point though, anything else looks good to me.
Simo.
I see the patch contains lots of classes with no state and a single
method. I believe it would simplify the code greatly if functions were
used instead.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 210 Allow SAN in IPA certificate profile
On 06/11/2014 02:59 PM, Jan Cholasta wrote:
On 11.6.2014 13:29, Martin Kosek wrote:
On 06/11/2014 10:58 AM, Jan Cholasta wrote:
On 10.6.2014 09:55, Martin Kosek wrote:
On 06/06/2014 12:50 PM, Jan Cholasta wrote:
On 23.1.2014 14:34, Jan Cholasta wrote:
On 22.1.2014 16:43, Simo Sorce wrote:
On Wed, 2014-01-22 at 16:05 +0100, Jan Cholasta wrote:
On 22.1.2014 15:34, Simo Sorce wrote:
On Wed, 2014-01-22 at 10:40 +0100, Jan Cholasta wrote:
On 21.1.2014 17:12, Simo Sorce wrote:
Later in the patch you seem to be changing from needing
managedby_host
to needing write access to an entry, I am not sure I understand
why that
was changed. not saying it is necessarily wrong, but why the
original
check is not right anymore ?
The original check is wrong, see
https://fedorahosted.org/freeipa/ticket/3977#comment:23.
The check in my patch allows SAN only if the requesting host has
write
access to all of the SAN services. I'm not entirely sure if this is
right, but even if it is not, I think we should still check for write
access to the SAN services, so that access control can be (partially)
handled by ACIs.
Right, I remembered that comment, but it just says to check the right
object's managed-by, here instead you changed it to check if you can
write the usercertificate.
I guess it is the same *if* there is an ACI that gives write
permission
when the host is in the managed-by attribute, is that the reasoning ?
Exactly. The ACIs that allow this by default are named Hosts can
manage
service Certificates and kerberos keys and Hosts can manage other
host
Certificates and kerberos keys.
I think the check can be extended to users as well, so that requesting
certificate with SAN is allowed only to users which have write access
to
the SAN services.
I have done the modification, see attached patches.
Sounds good to me then, thanks for explaining.
The patches also look good, but I would like someone to give them a try
for a formal ack.
OK, thanks.
Bump.
I have added stricter validation of subject alt names as well as
certificate
extensions in general to the second patch.
Thanks!
Updated patches attached.
Note that you will need python-nss 0.15 in order to test, you can get a
RPM for
Fedora here:
http://koji.fedoraproject.org/koji/buildinfo?buildID=514739.
John, do you think we could build python-nss 0.15 also for Fedora 20? The
fix
incorporated in it is pretty important to warrant bugfix update in bodhi.
It
would also allow FreeIPA 4.0 to be installed on Fedora 20.
Also, resubmitting HTTP and LDAP Server-Cert certmonger requests does not
work,
because https://fedorahosted.org/freeipa/ticket/4370.
This worked for me, I suspect this is a DS bug. More info in the ticket.
Now back to review of the patch:
210.4: looks ok
234.4:
1) Virtual operation request certificate with subjectaltname should be a
member of Certificate Administrators privilege
OK.
2) I would write Request Certificate With SubjectAltName with with
instead
of With. I.e.:
Request Certificate with SubjectAltName
OK.
3) Why is the extension check only enforced for non-hosts?
+if not bind_principal.startswith('host/'):
+for ext in extensions:
+operation = self._allowed_extensions.get(ext)
+if operation:
+self.check_access(operation)
My tricky certmonger request passed:
# ipa-getcert request -d /etc/pki/nssdb/ -n test-san-1 -I test-san-1 -g
2048 -r
-N cn=`hostname` -K host/`hostname` -D test1.example.com -D
test2.example.com
-E mko...@redhat.com
while when I posted the same CSR as privileged user, it was rejected:
$ klist
Ticket cache: KEYRING:persistent:96203:96203
Default principal: f...@idm.lab.eng.brq.redhat.com
$ ipa cert-request --principal test/`hostname` certmonger.csr
ipa: ERROR: invalid 'csr': subject alt name type RFC822 Name is forbidden
Right, I meant to NACK myself, but you were faster. Fixed.
4) Regular users cannot read Virtual Operations, so even if I assign them a
permission, command is refused:
$ ipa cert-request --principal test/`hostname` openssl.csr
ipa: ERROR: Insufficient access: No such virtual command
I think we will need to create new managed permission Read Virtual
Operations
and assign it to Certificate Administrators privilege by default as this
privilege has the virtual operation permissions assigned. Petr3, what is
your
take on this?
Otherwise the patch worked well for me, the authorization part looked OK.
My
biggest concern was just the host/user differentiation wrt unknown
subjectaltname types.
Martin
Updated patches attached.
1) I could not compile the patch set:
$ make rpms
...
if [ != yes ]; then \
./makeapi --validate; \
./makeaci --validate; \
fi
Traceback (most recent call last):
File ./makeapi, line 457, in module
sys.exit(main())
File ./makeapi, line 428, in
Re: [Freeipa-devel] [PATCHES] 0581-0582 ipalib.config: Only convert numeric values to float
On 06/16/2014 07:04 AM, Fraser Tweedale wrote: On Fri, Jun 13, 2014 at 02:12:41PM +0200, Petr Viktorin wrote: First patch: minor fix in env loading Second patch: When api.env is loaded, strings that look like floats get auto-converted to floats. This is wrong, as the conversion can lose precision, which matters for the new api_version. Here's a patch that disables this conversion, and fixes places that the disabling might break. -- Petr³ Changes look fine, apply fine, and ipalib tests pass. ACK to both. Thanks for the review! Pushed to master: 521df77744233f424ec68caa68548bede6e575fb -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] User Life Cycle: enforce ipaUniqueID generation by the server
Hello, When a stage user is activate (ipa stageuse-activate), UUID plugin (DS) checks that the ipaUniqueID value of the new active user is 'autogenerate'. This is useful to prevent a provisioning systems to create Active user with invalid ipaUniqueID. Now one of the workflow step is to move a Delete user into the Stage container. In that case the Stage entry contains a ipaUniqueID and can not activate. A possibility is to 'reset' the ipaUniqueID value to 'autogenerate' during that step but I wonder it it is valid to reset it. Also, is it valid to reset it and keep others values like uidNumber/gidNumber ? thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 210 Allow SAN in IPA certificate profile
On 16.6.2014 13:31, Martin Kosek wrote:
On 06/11/2014 02:59 PM, Jan Cholasta wrote:
On 11.6.2014 13:29, Martin Kosek wrote:
On 06/11/2014 10:58 AM, Jan Cholasta wrote:
On 10.6.2014 09:55, Martin Kosek wrote:
On 06/06/2014 12:50 PM, Jan Cholasta wrote:
On 23.1.2014 14:34, Jan Cholasta wrote:
On 22.1.2014 16:43, Simo Sorce wrote:
On Wed, 2014-01-22 at 16:05 +0100, Jan Cholasta wrote:
On 22.1.2014 15:34, Simo Sorce wrote:
On Wed, 2014-01-22 at 10:40 +0100, Jan Cholasta wrote:
On 21.1.2014 17:12, Simo Sorce wrote:
Later in the patch you seem to be changing from needing
managedby_host
to needing write access to an entry, I am not sure I understand
why that
was changed. not saying it is necessarily wrong, but why the
original
check is not right anymore ?
The original check is wrong, see
https://fedorahosted.org/freeipa/ticket/3977#comment:23.
The check in my patch allows SAN only if the requesting host has write
access to all of the SAN services. I'm not entirely sure if this is
right, but even if it is not, I think we should still check for write
access to the SAN services, so that access control can be (partially)
handled by ACIs.
Right, I remembered that comment, but it just says to check the right
object's managed-by, here instead you changed it to check if you can
write the usercertificate.
I guess it is the same *if* there is an ACI that gives write permission
when the host is in the managed-by attribute, is that the reasoning ?
Exactly. The ACIs that allow this by default are named Hosts can manage
service Certificates and kerberos keys and Hosts can manage other host
Certificates and kerberos keys.
I think the check can be extended to users as well, so that requesting
certificate with SAN is allowed only to users which have write access to
the SAN services.
I have done the modification, see attached patches.
Sounds good to me then, thanks for explaining.
The patches also look good, but I would like someone to give them a try
for a formal ack.
OK, thanks.
Bump.
I have added stricter validation of subject alt names as well as certificate
extensions in general to the second patch.
Thanks!
Updated patches attached.
Note that you will need python-nss 0.15 in order to test, you can get a
RPM for
Fedora here: http://koji.fedoraproject.org/koji/buildinfo?buildID=514739.
John, do you think we could build python-nss 0.15 also for Fedora 20? The fix
incorporated in it is pretty important to warrant bugfix update in bodhi. It
would also allow FreeIPA 4.0 to be installed on Fedora 20.
Also, resubmitting HTTP and LDAP Server-Cert certmonger requests does not
work,
because https://fedorahosted.org/freeipa/ticket/4370.
This worked for me, I suspect this is a DS bug. More info in the ticket.
Now back to review of the patch:
210.4: looks ok
234.4:
1) Virtual operation request certificate with subjectaltname should be a
member of Certificate Administrators privilege
OK.
2) I would write Request Certificate With SubjectAltName with with instead
of With. I.e.:
Request Certificate with SubjectAltName
OK.
3) Why is the extension check only enforced for non-hosts?
+if not bind_principal.startswith('host/'):
+for ext in extensions:
+operation = self._allowed_extensions.get(ext)
+if operation:
+self.check_access(operation)
My tricky certmonger request passed:
# ipa-getcert request -d /etc/pki/nssdb/ -n test-san-1 -I test-san-1 -g
2048 -r
-N cn=`hostname` -K host/`hostname` -D test1.example.com -D
test2.example.com
-E mko...@redhat.com
while when I posted the same CSR as privileged user, it was rejected:
$ klist
Ticket cache: KEYRING:persistent:96203:96203
Default principal: f...@idm.lab.eng.brq.redhat.com
$ ipa cert-request --principal test/`hostname` certmonger.csr
ipa: ERROR: invalid 'csr': subject alt name type RFC822 Name is forbidden
Right, I meant to NACK myself, but you were faster. Fixed.
4) Regular users cannot read Virtual Operations, so even if I assign them a
permission, command is refused:
$ ipa cert-request --principal test/`hostname` openssl.csr
ipa: ERROR: Insufficient access: No such virtual command
I think we will need to create new managed permission Read Virtual
Operations
and assign it to Certificate Administrators privilege by default as this
privilege has the virtual operation permissions assigned. Petr3, what is your
take on this?
Otherwise the patch worked well for me, the authorization part looked OK. My
biggest concern was just the host/user differentiation wrt unknown
subjectaltname types.
Martin
Updated patches attached.
1) I could not compile the patch set:
$ make rpms
...
if [ != yes ]; then \
./makeapi --validate; \
./makeaci --validate; \
fi
Traceback (most recent call last):
File ./makeapi, line 457, in module
sys.exit(main())
File ./makeapi, line 428, in main
api.finalize()
File
[Freeipa-devel] [PATCHES] 295-299 Allow changing chaining of the IPA CA certificate
Hi,
the attached patches implement
https://fedorahosted.org/freeipa/ticket/3737.
My patches 241-253 and 262-294 are required for this
(http://www.redhat.com/archives/freeipa-devel/2014-June/msg00276.html,
http://www.redhat.com/archives/freeipa-devel/2014-June/msg00307.html).
The installation/testing guidelines from
http://www.redhat.com/archives/freeipa-devel/2014-March/msg00385.html
apply here as well.
Honza
--
Jan Cholasta
From 73b54fdd44a7f59f40b0e34dd565020deea74f00 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Fri, 13 Jun 2014 14:44:03 +0200
Subject: [PATCH 1/5] Add new NSSDatabase method get_cert for getting certs
from NSS databases.
Part of https://fedorahosted.org/freeipa/ticket/3737
---
ipaserver/install/certs.py | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/ipaserver/install/certs.py b/ipaserver/install/certs.py
index 7f3c246..127d0dd 100644
--- a/ipaserver/install/certs.py
+++ b/ipaserver/install/certs.py
@@ -210,9 +210,21 @@ class NSSDatabase(object):
raise RuntimeError(
Setting trust on %s failed % root_nickname)
+def get_cert(self, nickname, pem=False):
+args = ['-L', '-n', nickname]
+if pem:
+args.append('-a')
+else:
+args.append('-r')
+try:
+cert, err, returncode = self.run_certutil(args)
+except ipautil.CalledProcessError:
+raise RuntimeError(Failed to get %s % nickname)
+return cert
+
def export_pem_cert(self, nickname, location):
Export the given cert to PEM file in the given location
-cert, err, returncode = self.run_certutil([-L, -n, nickname, -a])
+cert = self.get_cert(nickname)
with open(location, w+) as fd:
fd.write(cert)
os.chmod(location, 0444)
--
1.9.0
From b9d0562bb8a71599de699428a509692a1cc90145 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Fri, 13 Jun 2014 14:45:29 +0200
Subject: [PATCH 2/5] Allow changing chaining of the IPA CA certificate in
ipa-cacert-manage.
Part of https://fedorahosted.org/freeipa/ticket/3737
---
install/tools/man/ipa-cacert-manage.1 | 6 +
ipaserver/install/ipa_cacert_manage.py | 47 +-
2 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/install/tools/man/ipa-cacert-manage.1 b/install/tools/man/ipa-cacert-manage.1
index 92fe717..cf42b24 100644
--- a/install/tools/man/ipa-cacert-manage.1
+++ b/install/tools/man/ipa-cacert-manage.1
@@ -42,6 +42,12 @@ When the IPA CA is not configured, this command is not available.
\fB\-p\fR \fIDM_PASSWORD\fR, \fB\-\-password\fR=\fIDM_PASSWORD\fR
The Directory Manager password to use for authentication.
.TP
+\fB\-\-self\-signed\fR
+Sign the renewed certificate by itself.
+.TP
+\fB\-\-external\-ca\fR
+Sign the renewed certificate by external CA.
+.TP
\fB\-\-external\-cert\-file\fR=\fIFILE\fR
PEM file containing a certificate signed by the external CA. Must be given with \-\-external\-ca\-file.
.TP
diff --git a/ipaserver/install/ipa_cacert_manage.py b/ipaserver/install/ipa_cacert_manage.py
index 2b799c1..7c041de 100644
--- a/ipaserver/install/ipa_cacert_manage.py
+++ b/ipaserver/install/ipa_cacert_manage.py
@@ -27,7 +27,7 @@ import krbV
from ipapython import admintool, certmonger, ipautil
from ipapython.dn import DN
-from ipalib import api, errors, x509, util
+from ipalib import api, errors, x509, certstore
from ipaserver.install import certs, cainstance, installutils
from ipaserver.plugins.ldap2 import ldap2
@@ -51,6 +51,14 @@ class CACertManage(admintool.AdminTool):
renew_group = OptionGroup(parser, Renew options)
renew_group.add_option(
+--self-signed, dest='self_signed',
+action='store_true',
+help=Sign the renewed certificate by itself)
+renew_group.add_option(
+--external-ca, dest='self_signed',
+action='store_false',
+help=Sign the renewed certificate by external CA)
+renew_group.add_option(
--external-cert-file, dest='external_cert_file',
help=PEM file containing a certificate signed by the external CA)
renew_group.add_option(
@@ -145,7 +153,12 @@ class CACertManage(admintool.AdminTool):
if options.external_cert_file:
return self.renew_external_step_2(ca, cert)
-if x509.is_self_signed(cert, x509.DER):
+if options.self_signed is not None:
+self_signed = options.self_signed
+else:
+self_signed = x509.is_self_signed(cert, x509.DER)
+
+if self_signed:
return self.renew_self_signed(ca)
else:
return self.renew_external_step_1(ca)
@@ -178,20 +191,19 @@ class CACertManage(admintool.AdminTool):
options = self.options
cert_filename = options.external_cert_file
+ca_filename =
Re: [Freeipa-devel] [PATCH] 655 webui: move RPC result extraction logic to Adapter
On 12.6.2014 14:40, Endi Sukma Dewata wrote: On 6/11/2014 8:05 AM, Petr Vobornik wrote: It enables declarative extraction of values from partial results of a batch commands and also further extensibility in custom adapters. The default adapter has detection logic for this extraction so it can use bare record or extract data from normal or batch RPC command. Minor change of user plugin fixed: https://fedorahosted.org/freeipa/ticket/4355 ACK. Pushed to master: 5a428608beb3752b111b558ba2355ef112a057f1 -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 546 webui: expose krbprincipalexpiration
On 12.6.2014 14:40, Endi Sukma Dewata wrote: On 2/25/2014 11:07 AM, Petr Vobornik wrote: Depends on tbabej's patches # 137, 138 and my 546. https://fedorahosted.org/freeipa/ticket/3306 ACK on #547. Pushed to master: 4de9c5fc51c1e9a07a23b430ba531eb096960732 -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 210 Allow SAN in IPA certificate profile
On 06/16/2014 02:57 PM, Jan Cholasta wrote:
On 16.6.2014 13:31, Martin Kosek wrote:
On 06/11/2014 02:59 PM, Jan Cholasta wrote:
On 11.6.2014 13:29, Martin Kosek wrote:
On 06/11/2014 10:58 AM, Jan Cholasta wrote:
On 10.6.2014 09:55, Martin Kosek wrote:
On 06/06/2014 12:50 PM, Jan Cholasta wrote:
On 23.1.2014 14:34, Jan Cholasta wrote:
On 22.1.2014 16:43, Simo Sorce wrote:
On Wed, 2014-01-22 at 16:05 +0100, Jan Cholasta wrote:
On 22.1.2014 15:34, Simo Sorce wrote:
On Wed, 2014-01-22 at 10:40 +0100, Jan Cholasta wrote:
On 21.1.2014 17:12, Simo Sorce wrote:
Later in the patch you seem to be changing from needing
managedby_host
to needing write access to an entry, I am not sure I understand
why that
was changed. not saying it is necessarily wrong, but why the
original
check is not right anymore ?
The original check is wrong, see
https://fedorahosted.org/freeipa/ticket/3977#comment:23.
The check in my patch allows SAN only if the requesting host has
write
access to all of the SAN services. I'm not entirely sure if this is
right, but even if it is not, I think we should still check for
write
access to the SAN services, so that access control can be
(partially)
handled by ACIs.
Right, I remembered that comment, but it just says to check the
right
object's managed-by, here instead you changed it to check if you can
write the usercertificate.
I guess it is the same *if* there is an ACI that gives write
permission
when the host is in the managed-by attribute, is that the reasoning
?
Exactly. The ACIs that allow this by default are named Hosts can
manage
service Certificates and kerberos keys and Hosts can manage other
host
Certificates and kerberos keys.
I think the check can be extended to users as well, so that
requesting
certificate with SAN is allowed only to users which have write
access to
the SAN services.
I have done the modification, see attached patches.
Sounds good to me then, thanks for explaining.
The patches also look good, but I would like someone to give them a
try
for a formal ack.
OK, thanks.
Bump.
I have added stricter validation of subject alt names as well as
certificate
extensions in general to the second patch.
Thanks!
Updated patches attached.
Note that you will need python-nss 0.15 in order to test, you can get a
RPM for
Fedora here:
http://koji.fedoraproject.org/koji/buildinfo?buildID=514739.
John, do you think we could build python-nss 0.15 also for Fedora 20? The
fix
incorporated in it is pretty important to warrant bugfix update in
bodhi. It
would also allow FreeIPA 4.0 to be installed on Fedora 20.
Also, resubmitting HTTP and LDAP Server-Cert certmonger requests does
not
work,
because https://fedorahosted.org/freeipa/ticket/4370.
This worked for me, I suspect this is a DS bug. More info in the ticket.
Now back to review of the patch:
210.4: looks ok
234.4:
1) Virtual operation request certificate with subjectaltname should be
a
member of Certificate Administrators privilege
OK.
2) I would write Request Certificate With SubjectAltName with with
instead
of With. I.e.:
Request Certificate with SubjectAltName
OK.
3) Why is the extension check only enforced for non-hosts?
+if not bind_principal.startswith('host/'):
+for ext in extensions:
+operation = self._allowed_extensions.get(ext)
+if operation:
+self.check_access(operation)
My tricky certmonger request passed:
# ipa-getcert request -d /etc/pki/nssdb/ -n test-san-1 -I test-san-1 -g
2048 -r
-N cn=`hostname` -K host/`hostname` -D test1.example.com -D
test2.example.com
-E mko...@redhat.com
while when I posted the same CSR as privileged user, it was rejected:
$ klist
Ticket cache: KEYRING:persistent:96203:96203
Default principal: f...@idm.lab.eng.brq.redhat.com
$ ipa cert-request --principal test/`hostname` certmonger.csr
ipa: ERROR: invalid 'csr': subject alt name type RFC822 Name is forbidden
Right, I meant to NACK myself, but you were faster. Fixed.
4) Regular users cannot read Virtual Operations, so even if I assign
them a
permission, command is refused:
$ ipa cert-request --principal test/`hostname` openssl.csr
ipa: ERROR: Insufficient access: No such virtual command
I think we will need to create new managed permission Read Virtual
Operations
and assign it to Certificate Administrators privilege by default as
this
privilege has the virtual operation permissions assigned. Petr3, what is
your
take on this?
Otherwise the patch worked well for me, the authorization part looked
OK. My
biggest concern was just the host/user differentiation wrt unknown
subjectaltname types.
Martin
Updated patches attached.
1) I could not compile the patch set:
$ make rpms
...
if [ != yes ]; then \
./makeapi --validate; \
./makeaci --validate; \
fi
Traceback (most recent call
Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP
Jan Cholasta wrote: Hi, the attached patches implement https://fedorahosted.org/freeipa/ticket/3259 and https://fedorahosted.org/freeipa/ticket/3520. This work depends on my patches 241-253 and 262-266 (http://www.redhat.com/archives/freeipa-devel/2014-June/msg00276.html). Note that automatic distribution of CA certificates to IPA systems is not implemented yet (it's planned for IPA 4.2, see https://fedorahosted.org/freeipa/ticket/4322), so /etc/ipa/ca.crt, /etc/pki/nssdb, /etc/dirsrv/slapd-REALM and /etc/httpd/alias are updated *only* during client/server install. 267 What is the purpose of this change? Won't this cause no certificates to be exported in the default case? diff --git a/ipaserver/install/certs.py b/ipaserver/install/certs.py index 61a954f..3cd7a53 100644 --- a/ipaserver/install/certs.py +++ b/ipaserver/install/certs.py @@ -463,7 +463,7 @@ class CertDB(object): do that step. # export the CA cert for use with other apps ipautil.backup_file(self.cacert_fname) -root_nicknames = self.find_root_cert(nickname) +root_nicknames = self.find_root_cert(nickname)[:-1] fd = open(self.cacert_fname, w) for root in root_nicknames: (cert, stderr, returncode) = self.run_certutil([-L, -n, root, -a]) Or are you separating out issuing CA from the rest of the chain? 268 ACK 269 ACK 270 If a user has their own CA installed, such as the case where they used ipa-server-certinstall, this will break it. What can be in the other set? Docs are needed. You potentially add a bunch of certs with no trust, what is the purpose? 271 ipaSecretKey isn't mentioned on http://www.freeipa.org/page/V4/CA_certificate_renewal . What is it and does it need to be added now? 272 ACK 273 ACK 274 ACK 275 ACK 276 There isn't any error handling around the ASN.1 decoder. Is that wise? There should be unit tests 277 There should be unit tests 278 When the certificate must be passed in as DER can you use dercert as the argument name so it's clear what is needed? In update_ca_cert() and get_ca_certs() should the values for trust be case insensitive? This breaks the convention where attribute names are always lower-case. I can't really grok add_ca_cert(). You are adding certs but removing values? Is this un-setting the list of trusted CA's maybe? There isn't a single comment in this file beyond the header. 279 Looks ok 280 Why add the chain with no trust? 281 / 282 What is the difference between add_cert and import_cert other than passing in trust and not having the db password? Do we even need add_single_pem_cert anymore? 283 / 284 In __import_ca_certs() I assume the pass is there for NotFound for the case of creating a replica with an older master that doesn't have these? How is SSL trust setup then? This code looks awfully similar. 285 ACK 286 It looks ok, just one wild thought. If writing the certificate fails and you go to clean up by removing ca_file, what if that removal fails, for the same reason the write failed, SELinux for example? 287 If this weren't addressed in a later patch I've have dinged you on the: +return [cert] There are some places where you pluralize the variables (certs) and others where you do not (ca_cert, existing_ca_cert). 288 Is a rawcert a dercert? I'd use that name instead for consistency. normalize_certificate() can raise exceptions. Those should be handled in write_certificate_list() 289 ACK 290 This can be dangerous because if another database is opened in the process things may fail. Additional warnings and red flags should be provided, or the context should be compared to known places this will blow up (e.g server). 291 You use a temporary database to make cleanup easier I suppose? Did you test this in enforcing? 292 Need unit tests 293 Why the fixme for the x509 import? Isn't this changing already published API for [insert|remove]_ca_cert_into_systemwide_ca_store? 294 Can you change the old occurances of cafile = config.dir + /ca.crt to use CACERT instead? Bad case in exception, Ca cert file is not available. Is there any additional information we can provide, like what to do about it and where we looked? You actually remove one occurrence of CACERT and replace it with a hardcoded path, is that on purpose? --- I still need to do functional testing and will probably take another pass the changes through but this patchset generally looks ok. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0583-0584 Convert DNS default permissions to managed
On 06/13/2014 05:25 PM, Petr Viktorin wrote:
With the first patch, old SYSTEM permissions can be replaced. The Read
DNS Entries did not have an associated ACI, but was rather rolled into
a single ACI with the managedBy rule used for per-zone access.
(and before that it was part of a deny rule.)
We can't remove this permission in an update file, because we need to
check that it is indeed an old SYSTEM perm and not a new one with the
same name.
The second patch converts DNS permissions to managed.
The ACIs are put directly in $SUFFIX, because the cn=dns subtree does
not exist in all installations.
I hope to change this for https://fedorahosted.org/freeipa/ticket/4058,
when I've thought more about relationships between plugins, packages,
install options, and the updater.
Testing more, I found a benign bug: the updater complained if the cn=dns
container was missing. Fixed here.
Also, the update_dns_permissions plugin is now now obsolete, the third
patch removes it.
--
Petr³
From 2d213434a065c18943b8b33e921bb5b9995a581d Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 13 Jun 2014 15:58:24 +0200
Subject: [PATCH] managed permission updater: Add mechanism to replace SYSTEM
permissions
The Read DNS Entries permission, which was marked SYSTEM (no associated
ACI), can now be converted to a regular managed permission.
Add a mechanism for the updater to replace old SYSTEM permissions.
This cannot be done in an update file because we do not want to replace
V2 permissions with the same name.
Part of the work for: https://fedorahosted.org/freeipa/ticket/4346
---
.../install/plugins/update_managed_permissions.py | 18 ++
1 file changed, 18 insertions(+)
diff --git a/ipaserver/install/plugins/update_managed_permissions.py b/ipaserver/install/plugins/update_managed_permissions.py
index 7b1405a1974826fd90acd0d5082f51d8b25034cd..2ca054d50d11eec9527e0ef1e5d53d2f8e479ed0 100644
--- a/ipaserver/install/plugins/update_managed_permissions.py
+++ b/ipaserver/install/plugins/update_managed_permissions.py
@@ -67,6 +67,8 @@
* replaces
- A list of ACIs corresponding to legacy default permissions replaced
by this permission.
+* replaces_system
+ - A list of names of old SYSTEM permissions this replaces.
* fixup_function
- A callable that may modify the template in-place before it is applied.
- Called with the permission name, template dict, and keyword arguments:
@@ -410,6 +412,21 @@ def update_permission(self, ldap, obj, name, template, anonymous_read_aci):
self.log.info(Removing legacy permission '%s', legacy_name)
self.api.Command[permission_del](unicode(legacy_name))
+for name in template.get('replaces_system', ()):
+name = unicode(name)
+try:
+entry = ldap.get_entry(permission_plugin.get_dn(name),
+ ['ipapermissiontype'])
+except errors.NotFound:
+self.log.info(Legacy permission '%s' not found, name)
+else:
+flags = entry.get('ipapermissiontype', [])
+if list(flags) == ['SYSTEM']:
+self.log.info(Removing legacy permission '%s', name)
+self.api.Command[permission_del](name, force=True)
+else:
+self.log.info(Ignoring V2 permission '%s', name)
+
def get_upgrade_attr_lists(self, current_acistring, default_acistrings):
Compute included and excluded attributes for a new permission
@@ -497,6 +514,7 @@ def update_entry(self, obj, entry, template,
template = dict(template)
template.pop('replaces', None)
+template.pop('replaces_system', None)
fixup_function = template.pop('fixup_function', None)
if fixup_function:
--
1.9.0
From 2878c7dbebc2352b91fc091aae3c4010c5243fb4 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Mon, 9 Jun 2014 15:06:35 +0200
Subject: [PATCH] Convert DNS default permissions to managed
Convewrt the existing default permissions.
The Read permission is split between Read DNS Entries and Read
DNS Configuration.
Part of the work for: https://fedorahosted.org/freeipa/ticket/4346
---
ACI.txt | 12 +
install/share/dns.ldif | 59
install/updates/40-delegation.update | 6 +--
install/updates/40-dns.update| 28 +-
ipalib/plugins/dns.py| 101 +++
5 files changed, 118 insertions(+), 88 deletions(-)
diff --git a/ACI.txt b/ACI.txt
index 2ceaacc077467b6ef54e09d0aa7d3d5695c8fd40..6b75e79c3d771d33558750958f61ada82fd1e5eb 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -10,6 +10,18 @@ dn: cn=System: Read Global Configuration,cn=permissions,cn=pbac,dc=ipa,dc=exampl
aci: (targetattr = cn || ipacertificatesubjectbase || ipaconfigstring || ipacustomfields || ipadefaultemaildomain ||
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On Fri, 2014-06-13 at 18:05 -0400, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Argh. Yes. Thanks for this.
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
There is only one PBKDF2 with differing PRFs. This is precisely what is
implemented. However, I'm still working with jdennis to see if I can
coax python-nss to do this for me. I'm happy to delete my own
implementation.
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
My understanding is that comprehensions are now preferred. I know
reduce() has been removed in Python 3.x. However, map() still appears to
be there. Does anyone have the official word on the preferred style?
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
Sounds good. Let's see if nss can do this. If not, I'll clean this up.
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
In general, this kind of complexity is reduced somewhat in Python 3.x's
new bytes/str division. I have a bytes subclass for Py3k that I like to
use which implements the bitwise operators. I've even had ideas about
submitting this as an upstream Python patch for the bytes class.
Other than the first point though, anything else looks good to me.
Thanks for looking at this!
Nathaniel
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On 06/16/2014 06:23 PM, Nathaniel McCallum wrote:
On Fri, 2014-06-13 at 18:05 -0400, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Argh. Yes. Thanks for this.
lxml.etree.XMLParser has a separate option for this, no_network, which
is True by default.
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
There is only one PBKDF2 with differing PRFs. This is precisely what is
implemented. However, I'm still working with jdennis to see if I can
coax python-nss to do this for me. I'm happy to delete my own
implementation.
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
My understanding is that comprehensions are now preferred. I know
reduce() has been removed in Python 3.x. However, map() still appears to
be there. Does anyone have the official word on the preferred style?
Here's my view, strictly unofficial:
Comprehensions are better in almost all cases.
This (converting each value) is just about the only one where map is
more concise, and roughly equally readable.
So, for consistency, it's better to use comprehensions everywhere.
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
Sounds good. Let's see if nss can do this. If not, I'll clean this up.
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
In general, this kind of complexity is reduced somewhat in Python 3.x's
new bytes/str division. I have a bytes subclass for Py3k that I like to
use which implements the bitwise operators. I've even had ideas about
submitting this as an upstream Python patch for the bytes class.
Try the python-ideas list first. There's a recent thread there from Nick
Coghlan on a similar topic, you might want to read it.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing
On Mon, 2014-06-16 at 11:53 +0300, Alexander Bokovoy wrote:
On Fri, 13 Jun 2014, Simo Sorce wrote:
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
I am CC'ing Simo because he wants to review my PBKDF2 implementation.
Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.
I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])
We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?
Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.
It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2
Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?
Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.
Or given you already use lambdas in there perhaps simplify even to:
dk = []
for i ...
...
dk.append(u)
return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]
In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.
Other than the first point though, anything else looks good to me.
I read through the patch and apart from the points Simo made, I don't
have much complaints. One additional thing to do is how to test it --
how to create the payload? Since this code doesn't require anything
specific, it could be well tested through a CI infrastructure.
I don't see why we can't use the samples that jcholast dug up as test
data since they come from the RFC. The noted fix for Figure 7 should be
included so that the test works.
Additionally, man page is missing.
The man page should be in 0053.2.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP
Rob Crittenden wrote: Jan Cholasta wrote: Hi, the attached patches implement https://fedorahosted.org/freeipa/ticket/3259 and https://fedorahosted.org/freeipa/ticket/3520. This work depends on my patches 241-253 and 262-266 (http://www.redhat.com/archives/freeipa-devel/2014-June/msg00276.html). Note that automatic distribution of CA certificates to IPA systems is not implemented yet (it's planned for IPA 4.2, see https://fedorahosted.org/freeipa/ticket/4322), so /etc/ipa/ca.crt, /etc/pki/nssdb, /etc/dirsrv/slapd-REALM and /etc/httpd/alias are updated *only* during client/server install. 267 What is the purpose of this change? Won't this cause no certificates to be exported in the default case? diff --git a/ipaserver/install/certs.py b/ipaserver/install/certs.py index 61a954f..3cd7a53 100644 --- a/ipaserver/install/certs.py +++ b/ipaserver/install/certs.py @@ -463,7 +463,7 @@ class CertDB(object): do that step. # export the CA cert for use with other apps ipautil.backup_file(self.cacert_fname) -root_nicknames = self.find_root_cert(nickname) +root_nicknames = self.find_root_cert(nickname)[:-1] fd = open(self.cacert_fname, w) for root in root_nicknames: (cert, stderr, returncode) = self.run_certutil([-L, -n, root, -a]) Or are you separating out issuing CA from the rest of the chain? 268 ACK 269 ACK 270 If a user has their own CA installed, such as the case where they used ipa-server-certinstall, this will break it. What can be in the other set? Docs are needed. You potentially add a bunch of certs with no trust, what is the purpose? 271 ipaSecretKey isn't mentioned on http://www.freeipa.org/page/V4/CA_certificate_renewal . What is it and does it need to be added now? 272 ACK 273 ACK 274 ACK 275 ACK 276 There isn't any error handling around the ASN.1 decoder. Is that wise? There should be unit tests 277 There should be unit tests 278 When the certificate must be passed in as DER can you use dercert as the argument name so it's clear what is needed? In update_ca_cert() and get_ca_certs() should the values for trust be case insensitive? This breaks the convention where attribute names are always lower-case. I can't really grok add_ca_cert(). You are adding certs but removing values? Is this un-setting the list of trusted CA's maybe? There isn't a single comment in this file beyond the header. 279 Looks ok 280 Why add the chain with no trust? 281 / 282 What is the difference between add_cert and import_cert other than passing in trust and not having the db password? Do we even need add_single_pem_cert anymore? 283 / 284 In __import_ca_certs() I assume the pass is there for NotFound for the case of creating a replica with an older master that doesn't have these? How is SSL trust setup then? This code looks awfully similar. 285 ACK 286 It looks ok, just one wild thought. If writing the certificate fails and you go to clean up by removing ca_file, what if that removal fails, for the same reason the write failed, SELinux for example? 287 If this weren't addressed in a later patch I've have dinged you on the: +return [cert] There are some places where you pluralize the variables (certs) and others where you do not (ca_cert, existing_ca_cert). 288 Is a rawcert a dercert? I'd use that name instead for consistency. normalize_certificate() can raise exceptions. Those should be handled in write_certificate_list() 289 ACK 290 This can be dangerous because if another database is opened in the process things may fail. Additional warnings and red flags should be provided, or the context should be compared to known places this will blow up (e.g server). 291 You use a temporary database to make cleanup easier I suppose? Did you test this in enforcing? 292 Need unit tests 293 Why the fixme for the x509 import? Isn't this changing already published API for [insert|remove]_ca_cert_into_systemwide_ca_store? 294 Can you change the old occurances of cafile = config.dir + /ca.crt to use CACERT instead? Bad case in exception, Ca cert file is not available. Is there any additional information we can provide, like what to do about it and where we looked? You actually remove one occurrence of CACERT and replace it with a hardcoded path, is that on purpose? --- I still need to do functional testing and will probably take another pass the changes through but this patchset generally looks ok. Several issues found during testing: 1. Enrollment from RHEL-5 fails because the entire cert chain is not retrieved, only the issuing CA cert. Joining realm failed: libcurl failed to execute the HTTP POST transaction. SSL certificate problem, verify that the CA cert is OK. Details: error:14090086:SSL
