Re: [Freeipa-devel] [PATCH] 0001 cert-show: Remove check if hostname != CN
On 13.10.2015 19:26, Rob Crittenden wrote:
> Jan Orel wrote:
>>> The restriction was there so that hosts had limited visibility. This
>>> applies that limitation to all users. I think the host check needs to be
>>> re-added.
>>
>> I am confused, correct me if I am wrong, but the "if hostname:" check
>> seems always redundat because it would raise exception before
>> either here:
>>
>> 615 if not bind_principal.startswith('host/'):
>> 616 raise acierr
>>
>> or in validate_principal()
>
> Anything bound to IPA can potentially retrieve a certificate. This code
> adds special handling for hosts and probably should cover services as
> well now that I think about it. I don't think services could be included
> in ACIs when this was originally written.
>
> The idea was that hosts have no need to be able to query random serial
> numbers so it should be limited to viewing its own. Removing the if
> hostname: applies this logic to ALL retrieval which is by far overkill
> and limits all non-admin entries to only be able to view certs they own
> (or can write) which sort of kills the reason for the 'retrieve
> certificate' permission.
>
>>
>>> Also, every host is not guaranteed to have a krbPrincipalAux (it can be
>>> unenrolled). I assume you used this to cover managed services as well,
>>> that's why the broad search base?
>>
>> Checking it, even host which is not enrolled have objectClass:
>> krbprincipalaux,
>> but advise me if different search should be used.
>
> If a host is added with a password (random or otherwise) it won't have
> this objectclass. I'd make the search filter something like
> (|(objectclass=ipahost)(objectclass=ipaservice)).
>
> rob
>
Rob, could you or Honza (or somebody else) hand-hold Jan Orel a little bit?
I was talking with boss of the guy and they are still interested in getting
the patch in IPA but need more guidance and patience from us :-)
--
Petr^2 Spacek
--
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 cert-show: Remove check if hostname != CN
On 15.10.2015 17:28, Jan Orel wrote:
> diff --git a/ipalib/plugins/cert.py b/ipalib/plugins/cert.py
> index e459320..55f9484 100644
> --- a/ipalib/plugins/cert.py
> +++ b/ipalib/plugins/cert.py
> @@ -625,9 +625,12 @@ class cert_show(VirtualCommand):
> result['md5_fingerprint'] =
> unicode(nss.data_to_hex(nss.md5_digest(cert.der_data), 64)[0])
> result['sha1_fingerprint'] =
> unicode(nss.data_to_hex(nss.sha1_digest(cert.der_data), 64)[0])
> if hostname:
> -# If we have a hostname we want to verify that the subject
> -# of the certificate matches it, otherwise raise an error
> -if hostname != cert.subject.common_name:#pylint:
> disable=E1101
> +# If we have a hostname we want to verify that we can
> +# write to the usercertificate attr of the target
> +ldap = self.api.Backend.ldap2
> +entry = ldap.find_entry_by_attr("cn", cert.subject.common_name,
> +"ipahost", base_dn=api.env.basedn)
> +if not ldap.can_write(entry.dn, 'usercertificate'):
> raise acierr
>
> return dict(result=result)
I can't say anything about correctness of the change itself but it would be
good to add explanatory error message to acierr, when you are at it. Something
like 'Insufficient permissions for write to userCertificate attribute of $DN
entry' or so.
Thanks!
--
Petr^2 Spacek
--
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 cert-show: Remove check if hostname != CN
2015-10-13 19:26 GMT+02:00 Rob Crittenden :
> Jan Orel wrote:
>>> The restriction was there so that hosts had limited visibility. This
>>> applies that limitation to all users. I think the host check needs to be
>>> re-added.
>>
>> I am confused, correct me if I am wrong, but the "if hostname:" check
>> seems always redundat because it would raise exception before
>> either here:
>>
>> 615 if not bind_principal.startswith('host/'):
>> 616 raise acierr
>>
>> or in validate_principal()
>
> Anything bound to IPA can potentially retrieve a certificate. This code
> adds special handling for hosts and probably should cover services as
> well now that I think about it. I don't think services could be included
> in ACIs when this was originally written.
>
> The idea was that hosts have no need to be able to query random serial
> numbers so it should be limited to viewing its own. Removing the if
> hostname: applies this logic to ALL retrieval which is by far overkill
> and limits all non-admin entries to only be able to view certs they own
> (or can write) which sort of kills the reason for the 'retrieve
> certificate' permission.
>
>>
>>> Also, every host is not guaranteed to have a krbPrincipalAux (it can be
>>> unenrolled). I assume you used this to cover managed services as well,
>>> that's why the broad search base?
>>
>> Checking it, even host which is not enrolled have objectClass:
>> krbprincipalaux,
>> but advise me if different search should be used.
>
> If a host is added with a password (random or otherwise) it won't have
> this objectclass. I'd make the search filter something like
> (|(objectclass=ipahost)(objectclass=ipaservice)).
>
> rob
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0001 cert-show: Remove check if hostname != CN
> Anything bound to IPA can potentially retrieve a certificate. This code
> adds special handling for hosts and probably should cover services as
> well now that I think about it. I don't think services could be included
> in ACIs when this was originally written.
>
> The idea was that hosts have no need to be able to query random serial
> numbers so it should be limited to viewing its own. Removing the if
> hostname: applies this logic to ALL retrieval which is by far overkill
> and limits all non-admin entries to only be able to view certs they own
> (or can write) which sort of kills the reason for the 'retrieve
> certificate' permission.
OK, anyway I don't think I am able to refactor right now to include
also the services.
I am attaching new simple patch.
From de2f71384e72cacf3f8b1deb864518246835942b Mon Sep 17 00:00:00 2001
From: Jan Orel
Date: Thu, 15 Oct 2015 16:59:14 +0200
Subject: [PATCH] cert-show: verify write access to usercertificate
---
ipalib/plugins/cert.py | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/ipalib/plugins/cert.py b/ipalib/plugins/cert.py
index e459320..55f9484 100644
--- a/ipalib/plugins/cert.py
+++ b/ipalib/plugins/cert.py
@@ -625,9 +625,12 @@ class cert_show(VirtualCommand):
result['md5_fingerprint'] = unicode(nss.data_to_hex(nss.md5_digest(cert.der_data), 64)[0])
result['sha1_fingerprint'] = unicode(nss.data_to_hex(nss.sha1_digest(cert.der_data), 64)[0])
if hostname:
-# If we have a hostname we want to verify that the subject
-# of the certificate matches it, otherwise raise an error
-if hostname != cert.subject.common_name:#pylint: disable=E1101
+# If we have a hostname we want to verify that we can
+# write to the usercertificate attr of the target
+ldap = self.api.Backend.ldap2
+entry = ldap.find_entry_by_attr("cn", cert.subject.common_name,
+"ipahost", base_dn=api.env.basedn)
+if not ldap.can_write(entry.dn, 'usercertificate'):
raise acierr
return dict(result=result)
--
2.4.3
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0001 cert-show: Remove check if hostname != CN
Jan Orel wrote:
>> The restriction was there so that hosts had limited visibility. This
>> applies that limitation to all users. I think the host check needs to be
>> re-added.
>
> I am confused, correct me if I am wrong, but the "if hostname:" check
> seems always redundat because it would raise exception before
> either here:
>
> 615 if not bind_principal.startswith('host/'):
> 616 raise acierr
>
> or in validate_principal()
Anything bound to IPA can potentially retrieve a certificate. This code
adds special handling for hosts and probably should cover services as
well now that I think about it. I don't think services could be included
in ACIs when this was originally written.
The idea was that hosts have no need to be able to query random serial
numbers so it should be limited to viewing its own. Removing the if
hostname: applies this logic to ALL retrieval which is by far overkill
and limits all non-admin entries to only be able to view certs they own
(or can write) which sort of kills the reason for the 'retrieve
certificate' permission.
>
>> Also, every host is not guaranteed to have a krbPrincipalAux (it can be
>> unenrolled). I assume you used this to cover managed services as well,
>> that's why the broad search base?
>
> Checking it, even host which is not enrolled have objectClass:
> krbprincipalaux,
> but advise me if different search should be used.
If a host is added with a password (random or otherwise) it won't have
this objectclass. I'd make the search filter something like
(|(objectclass=ipahost)(objectclass=ipaservice)).
rob
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0001 cert-show: Remove check if hostname != CN
> The restriction was there so that hosts had limited visibility. This
> applies that limitation to all users. I think the host check needs to be
> re-added.
I am confused, correct me if I am wrong, but the "if hostname:" check
seems always redundat because it would raise exception before
either here:
615 if not bind_principal.startswith('host/'):
616 raise acierr
or in validate_principal()
> Also, every host is not guaranteed to have a krbPrincipalAux (it can be
> unenrolled). I assume you used this to cover managed services as well,
> that's why the broad search base?
Checking it, even host which is not enrolled have objectClass: krbprincipalaux,
but advise me if different search should be used.
thanks, jan
--
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 cert-show: Remove check if hostname != CN
Jan Orel wrote: >> Agreed. The corresponding checks for certificate issuance via >> cert-request, where the bind principal is a host, check that the >> subject host (and SAN dNSNames) is "managed by" the bind host. >> This is checked via `ldap.can_write(dn_of_subject_principal)'. >> >> 1. retrieve cert >> 2. read CN >> 3. ensure CN refers to a known host principal >>and call ldap.can_write(...) to ensure bind principal >>manages it. >> > > Thanks for the feedback. Attaching new patch. > The restriction was there so that hosts had limited visibility. This applies that limitation to all users. I think the host check needs to be re-added. Also, every host is not guaranteed to have a krbPrincipalAux (it can be unenrolled). I assume you used this to cover managed services as well, that's why the broad search base? rob -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0001 cert-show: Remove check if hostname != CN
> Agreed. The corresponding checks for certificate issuance via
> cert-request, where the bind principal is a host, check that the
> subject host (and SAN dNSNames) is "managed by" the bind host.
> This is checked via `ldap.can_write(dn_of_subject_principal)'.
>
> 1. retrieve cert
> 2. read CN
> 3. ensure CN refers to a known host principal
>and call ldap.can_write(...) to ensure bind principal
>manages it.
>
Thanks for the feedback. Attaching new patch.
From f75c5a3c297c4639645f53c973e5ba91eff46315 Mon Sep 17 00:00:00 2001
From: Jan Orel
Date: Mon, 12 Oct 2015 17:07:24 +0200
Subject: [PATCH] cert-show: verify write access to userCertificate
---
ipalib/plugins/cert.py | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/ipalib/plugins/cert.py b/ipalib/plugins/cert.py
index e459320..286e05c 100644
--- a/ipalib/plugins/cert.py
+++ b/ipalib/plugins/cert.py
@@ -606,7 +606,6 @@ class cert_show(VirtualCommand):
def execute(self, serial_number, **options):
ca_enabled_check()
-hostname = None
try:
self.check_access()
except errors.ACIError as acierr:
@@ -614,7 +613,6 @@ class cert_show(VirtualCommand):
bind_principal = getattr(context, 'principal')
if not bind_principal.startswith('host/'):
raise acierr
-hostname = get_host_from_principal(bind_principal)
result=self.Backend.ra.get_certificate(serial_number)
cert = x509.load_certificate(result['certificate'])
@@ -624,11 +622,13 @@ class cert_show(VirtualCommand):
result['valid_not_after'] = unicode(cert.valid_not_after_str)
result['md5_fingerprint'] = unicode(nss.data_to_hex(nss.md5_digest(cert.der_data), 64)[0])
result['sha1_fingerprint'] = unicode(nss.data_to_hex(nss.sha1_digest(cert.der_data), 64)[0])
-if hostname:
-# If we have a hostname we want to verify that the subject
-# of the certificate matches it, otherwise raise an error
-if hostname != cert.subject.common_name:#pylint: disable=E1101
-raise acierr
+
+# verify we can write to userCertificate attribute of the target
+ldap = self.api.Backend.ldap2
+entry = ldap.find_entry_by_attr("cn", cert.subject.common_name,
+"krbPrincipalAux", base_dn=api.env.basedn)
+if not ldap.can_write(entry.dn, 'usercertificate'):
+raise acierr
return dict(result=result)
--
2.4.3
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0001 cert-show: Remove check if hostname != CN
On Fri, Oct 09, 2015 at 08:39:10AM -0400, Rob Crittenden wrote: > Jan Orel wrote: > > Hello, > > > > this patch removes (IMHO) redundat check in cert_show, which fails when > > host tries to re-submit certificate of different host/service which he > > can manage. > > > > I also reported the bug here: > > https://bugzilla.redhat.com/show_bug.cgi?id=1269089 > > > > I tired to run the tests as well and it doesn't seem to break anything. > > Any feedpack appriciated. > > This works around the "Retrieve Certificates from the CA" ACL when done > as a host. > > I guess if the hostname isn't the subject then the host for the subject > needs to be read and then look to see if hostname is in the managed_by list. > > rob > Agreed. The corresponding checks for certificate issuance via cert-request, where the bind principal is a host, check that the subject host (and SAN dNSNames) is "managed by" the bind host. This is checked via `ldap.can_write(dn_of_subject_principal)'. 1. retrieve cert 2. read CN 3. ensure CN refers to a known host principal and call ldap.can_write(...) to ensure bind principal manages it. Cheers, Fraser > -- > Manage your subscription for the Freeipa-devel mailing list: > https://www.redhat.com/mailman/listinfo/freeipa-devel > Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code -- 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 cert-show: Remove check if hostname != CN
On 2015-10-09 15:11, Jan Cholasta wrote: > On 9.10.2015 15:00, Christian Heimes wrote: >> On 2015-10-09 13:21, Jan Orel wrote: >>> Hello, >>> >>> this patch removes (IMHO) redundat check in cert_show, which fails when >>> host tries to re-submit certificate of different host/service which he >>> can manage. >>> >>> I also reported the bug here: >>> https://bugzilla.redhat.com/show_bug.cgi?id=1269089 >>> >>> I tired to run the tests as well and it doesn't seem to break anything. >>> Any feedpack appriciated. >> >> Jan Cholasta, you implemented the check in 2011. What purpose does it >> have? > > I did not, it was added in commit 2e8bae59 by Rob. Sorry, I didn't check the context, just the output of $ git annotate ipalib/plugins/cert.py | grep common_name signature.asc Description: OpenPGP digital signature -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0001 cert-show: Remove check if hostname != CN
On 9.10.2015 15:00, Christian Heimes wrote: On 2015-10-09 13:21, Jan Orel wrote: Hello, this patch removes (IMHO) redundat check in cert_show, which fails when host tries to re-submit certificate of different host/service which he can manage. I also reported the bug here: https://bugzilla.redhat.com/show_bug.cgi?id=1269089 I tired to run the tests as well and it doesn't seem to break anything. Any feedpack appriciated. Jan Cholasta, you implemented the check in 2011. What purpose does it have? I did not, it was added in commit 2e8bae59 by Rob. hostname == CN has been deprecated by RFC 2818 for some time, see https://tools.ietf.org/html/rfc2818#section-3.1 The current check is also not sufficient to prevent forgery. Browsers and modern TLS libraries completely ignore CN when a dNSName SAN extension is present. 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] [PATCH] 0001 cert-show: Remove check if hostname != CN
Christian Heimes wrote: > On 2015-10-09 13:21, Jan Orel wrote: >> Hello, >> >> this patch removes (IMHO) redundat check in cert_show, which fails when >> host tries to re-submit certificate of different host/service which he >> can manage. >> >> I also reported the bug here: >> https://bugzilla.redhat.com/show_bug.cgi?id=1269089 >> >> I tired to run the tests as well and it doesn't seem to break anything. >> Any feedpack appriciated. > > Jan Cholasta, you implemented the check in 2011. What purpose does it have? > > hostname == CN has been deprecated by RFC 2818 for some time, see > https://tools.ietf.org/html/rfc2818#section-3.1 The current check is > also not sufficient to prevent forgery. Browsers and modern TLS > libraries completely ignore CN when a dNSName SAN extension is present. He just tweaked a pylint error, I did this code. The check isn't perfect (by far) but I don't think forgery is an issue. We're talking about retrieving a public cert, not doing a handshake. I think checking just the common name is ok because of the way IPA issues server certs. I'm not sure if SAN would even come into play in the case of checking this ACL. The only way I see this as being a problem is if a new profile is created for issuing server certs where the CN of the target host isn't in the subject. rob -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0001 cert-show: Remove check if hostname != CN
On 2015-10-09 13:21, Jan Orel wrote: > Hello, > > this patch removes (IMHO) redundat check in cert_show, which fails when > host tries to re-submit certificate of different host/service which he > can manage. > > I also reported the bug here: > https://bugzilla.redhat.com/show_bug.cgi?id=1269089 > > I tired to run the tests as well and it doesn't seem to break anything. > Any feedpack appriciated. Jan Cholasta, you implemented the check in 2011. What purpose does it have? hostname == CN has been deprecated by RFC 2818 for some time, see https://tools.ietf.org/html/rfc2818#section-3.1 The current check is also not sufficient to prevent forgery. Browsers and modern TLS libraries completely ignore CN when a dNSName SAN extension is present. Christian signature.asc Description: OpenPGP digital signature -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0001 cert-show: Remove check if hostname != CN
Jan Orel wrote: > Hello, > > this patch removes (IMHO) redundat check in cert_show, which fails when > host tries to re-submit certificate of different host/service which he > can manage. > > I also reported the bug here: > https://bugzilla.redhat.com/show_bug.cgi?id=1269089 > > I tired to run the tests as well and it doesn't seem to break anything. > Any feedpack appriciated. This works around the "Retrieve Certificates from the CA" ACL when done as a host. I guess if the hostname isn't the subject then the host for the subject needs to be read and then look to see if hostname is in the managed_by list. rob -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
