You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by Jan Bernhardt <jb...@talend.com.INVALID> on 2019/07/08 14:14:30 UTC
Potential bug in CXF XKMS LDAP implementation
Hi CXF developers,
I’m trying to understand if there is a bug or a feature that I don’t understand in the LDAP Repository implementation for CXF XKMS.
https://github.com/apache/cxf/blob/master/services/xkms/xkms-x509-repo-ldap/src/main/java/org/apache/cxf/xkms/x509/repo/ldap/LdapCertificateRepo.java
Line 206, 207
Here the service LDAP template filter gets applied first (looks fine to me), but then the result is send to the getCertificateForUIDAttr method. Here the UIDAttribute LDAP filter gets applied on top of the other filter, making the first filter useless (or even breaks it).
So from my perspective line 207 should look like line 241.
Can you confirm?
Jan
Re: Potential bug in CXF XKMS LDAP implementation
Posted by Colm O hEigeartaigh <co...@apache.org>.
Hi Jan,
I've merged a fix to make the private methods in LdapCertificateRepo
protected. I'm fine with adding a separate attribute for CA certs as you
propose - however, how do we know which filter to use in for example
findByServiceName / findByEndpoint?
Colm.
On Mon, Jul 22, 2019 at 2:46 PM Jan Bernhardt <jb...@talend.com> wrote:
> Hi Colm,
>
>
>
> thanks for your patch. Looks good to me.
>
>
>
> What do you think about changing the private methods to protected? This
> would make it a lot easier to subclass this implementation in case you need
> to customize something.
>
>
>
> For example according to the NATO Standard ACP 133, it can be possible to
> use different LDAP attributes for service certificates
> (userCertificate;binary) and CA certificates (cACertificate;binary)
>
> https://tools.ietf.org/html/draft-dally-acp133-and-ldap-01#section-2.21
>
>
>
> Currently I would have to copy all private methods into my subclass. It
> would be a lot easier if these methods would be protected.
>
>
>
> BTW. What do you thing about extending the implementation to support
> different LDAP attributes for “normal” certificates as well as for “CA”
> certificates as in my example above?
>
> We could add a key xkms.ldap.schema.attrCaCrtBinary in addition to
> xkms.ldap.schema.attrCrtBinary for this purpose. Default value could be
> “userCertificate” to ensure backward compatibility.
>
> WDYT?
>
>
>
> Viele Grüße
>
> Jan
>
>
>
> *From:* Colm O hEigeartaigh <co...@apache.org>
> *Sent:* Tuesday, 9 July 2019 12:28
> *To:* Jan Bernhardt <jb...@talend.com>
> *Cc:* dev@cxf.apache.org; Andrei Shakirin <as...@talend.com>
> *Subject:* Re: Potential bug in CXF XKMS LDAP implementation
>
>
>
>
>
> Hi Jan,
>
>
>
> Yes, you're correct - it's a bug. I filed a JIRA here:
> https://issues.apache.org/jira/browse/CXF-8071
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__issues.apache.org_jira_browse_CXF-2D8071&d=DwMFaQ&c=2w5q_42kFG40MI2alLPgJw&r=HZeq2aU1KcmwwCZOPf0sQ9nq4vkxyxqiWkZrOI1o4vI&m=lkMbhTZjOKa0SWqLHCuNYQGQAP1Uc72UysSC76k0LmI&s=zs0iyZtxdjt7I4lhx4fnvjErAU_tMR1S8aBtT_lPX2Y&e=>
>
>
>
> Here is a PR to fix the problem - https://github.com/apache/cxf/pull/565
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apache_cxf_pull_565&d=DwMFaQ&c=2w5q_42kFG40MI2alLPgJw&r=HZeq2aU1KcmwwCZOPf0sQ9nq4vkxyxqiWkZrOI1o4vI&m=lkMbhTZjOKa0SWqLHCuNYQGQAP1Uc72UysSC76k0LmI&s=rmFOyUMu-VVvrtrjAt2D7BkEuS0s5WV-iQUILVEI5Do&e=>
>
>
>
> I made a change to the default serviceCertUIDTemplate, as I don't think it
> makes sense to have it use "cn" by default.
>
>
>
> Let me know what you think,
>
>
>
> Colm.
>
>
>
> On Mon, Jul 8, 2019 at 3:14 PM Jan Bernhardt <jb...@talend.com>
> wrote:
>
> Hi CXF developers,
>
>
>
> I’m trying to understand if there is a bug or a feature that I don’t
> understand in the LDAP Repository implementation for CXF XKMS.
>
>
>
>
> https://github.com/apache/cxf/blob/master/services/xkms/xkms-x509-repo-ldap/src/main/java/org/apache/cxf/xkms/x509/repo/ldap/LdapCertificateRepo.java
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apache_cxf_blob_master_services_xkms_xkms-2Dx509-2Drepo-2Dldap_src_main_java_org_apache_cxf_xkms_x509_repo_ldap_LdapCertificateRepo.java&d=DwMFaQ&c=2w5q_42kFG40MI2alLPgJw&r=HZeq2aU1KcmwwCZOPf0sQ9nq4vkxyxqiWkZrOI1o4vI&m=lkMbhTZjOKa0SWqLHCuNYQGQAP1Uc72UysSC76k0LmI&s=QfujFyWzEak5z0Wq76ohlq_puKGVo-JbvHaqGJ1aQ5M&e=>
>
> Line 206, 207
>
>
>
> Here the service LDAP template filter gets applied first (looks fine to
> me), but then the result is send to the getCertificateForUIDAttr method.
> Here the UIDAttribute LDAP filter gets applied on top of the other filter,
> making the first filter useless (or even breaks it).
>
> So from my perspective line 207 should look like line 241.
>
>
>
> Can you confirm?
>
>
>
> Jan
>
>
>
>
>
>
>
> --
>
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>
--
Colm O hEigeartaigh
Talend Community Coder
http://coders.talend.com
RE: Potential bug in CXF XKMS LDAP implementation
Posted by Jan Bernhardt <jb...@talend.com.INVALID>.
Hi Colm,
thanks for your patch. Looks good to me.
What do you think about changing the private methods to protected? This would make it a lot easier to subclass this implementation in case you need to customize something.
For example according to the NATO Standard ACP 133, it can be possible to use different LDAP attributes for service certificates (userCertificate;binary) and CA certificates (cACertificate;binary)
https://tools.ietf.org/html/draft-dally-acp133-and-ldap-01#section-2.21
Currently I would have to copy all private methods into my subclass. It would be a lot easier if these methods would be protected.
BTW. What do you thing about extending the implementation to support different LDAP attributes for “normal” certificates as well as for “CA” certificates as in my example above?
We could add a key xkms.ldap.schema.attrCaCrtBinary in addition to xkms.ldap.schema.attrCrtBinary for this purpose. Default value could be “userCertificate” to ensure backward compatibility.
WDYT?
Viele Grüße
Jan
From: Colm O hEigeartaigh <co...@apache.org>
Sent: Tuesday, 9 July 2019 12:28
To: Jan Bernhardt <jb...@talend.com>
Cc: dev@cxf.apache.org; Andrei Shakirin <as...@talend.com>
Subject: Re: Potential bug in CXF XKMS LDAP implementation
Hi Jan,
Yes, you're correct - it's a bug. I filed a JIRA here: https://issues.apache.org/jira/browse/CXF-8071<https://urldefense.proofpoint.com/v2/url?u=https-3A__issues.apache.org_jira_browse_CXF-2D8071&d=DwMFaQ&c=2w5q_42kFG40MI2alLPgJw&r=HZeq2aU1KcmwwCZOPf0sQ9nq4vkxyxqiWkZrOI1o4vI&m=lkMbhTZjOKa0SWqLHCuNYQGQAP1Uc72UysSC76k0LmI&s=zs0iyZtxdjt7I4lhx4fnvjErAU_tMR1S8aBtT_lPX2Y&e=>
Here is a PR to fix the problem - https://github.com/apache/cxf/pull/565<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apache_cxf_pull_565&d=DwMFaQ&c=2w5q_42kFG40MI2alLPgJw&r=HZeq2aU1KcmwwCZOPf0sQ9nq4vkxyxqiWkZrOI1o4vI&m=lkMbhTZjOKa0SWqLHCuNYQGQAP1Uc72UysSC76k0LmI&s=rmFOyUMu-VVvrtrjAt2D7BkEuS0s5WV-iQUILVEI5Do&e=>
I made a change to the default serviceCertUIDTemplate, as I don't think it makes sense to have it use "cn" by default.
Let me know what you think,
Colm.
On Mon, Jul 8, 2019 at 3:14 PM Jan Bernhardt <jb...@talend.com>> wrote:
Hi CXF developers,
I’m trying to understand if there is a bug or a feature that I don’t understand in the LDAP Repository implementation for CXF XKMS.
https://github.com/apache/cxf/blob/master/services/xkms/xkms-x509-repo-ldap/src/main/java/org/apache/cxf/xkms/x509/repo/ldap/LdapCertificateRepo.java<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apache_cxf_blob_master_services_xkms_xkms-2Dx509-2Drepo-2Dldap_src_main_java_org_apache_cxf_xkms_x509_repo_ldap_LdapCertificateRepo.java&d=DwMFaQ&c=2w5q_42kFG40MI2alLPgJw&r=HZeq2aU1KcmwwCZOPf0sQ9nq4vkxyxqiWkZrOI1o4vI&m=lkMbhTZjOKa0SWqLHCuNYQGQAP1Uc72UysSC76k0LmI&s=QfujFyWzEak5z0Wq76ohlq_puKGVo-JbvHaqGJ1aQ5M&e=>
Line 206, 207
Here the service LDAP template filter gets applied first (looks fine to me), but then the result is send to the getCertificateForUIDAttr method. Here the UIDAttribute LDAP filter gets applied on top of the other filter, making the first filter useless (or even breaks it).
So from my perspective line 207 should look like line 241.
Can you confirm?
Jan
--
Colm O hEigeartaigh
Talend Community Coder
http://coders.talend.com
Re: Potential bug in CXF XKMS LDAP implementation
Posted by Colm O hEigeartaigh <co...@apache.org>.
Hi Jan,
Yes, you're correct - it's a bug. I filed a JIRA here:
https://issues.apache.org/jira/browse/CXF-8071
Here is a PR to fix the problem - https://github.com/apache/cxf/pull/565
I made a change to the default serviceCertUIDTemplate, as I don't think it
makes sense to have it use "cn" by default.
Let me know what you think,
Colm.
On Mon, Jul 8, 2019 at 3:14 PM Jan Bernhardt <jb...@talend.com> wrote:
> Hi CXF developers,
>
>
>
> I’m trying to understand if there is a bug or a feature that I don’t
> understand in the LDAP Repository implementation for CXF XKMS.
>
>
>
>
> https://github.com/apache/cxf/blob/master/services/xkms/xkms-x509-repo-ldap/src/main/java/org/apache/cxf/xkms/x509/repo/ldap/LdapCertificateRepo.java
>
> Line 206, 207
>
>
>
> Here the service LDAP template filter gets applied first (looks fine to
> me), but then the result is send to the getCertificateForUIDAttr method.
> Here the UIDAttribute LDAP filter gets applied on top of the other filter,
> making the first filter useless (or even breaks it).
>
> So from my perspective line 207 should look like line 241.
>
>
>
> Can you confirm?
>
>
>
> Jan
>
>
>
>
>
--
Colm O hEigeartaigh
Talend Community Coder
http://coders.talend.com