Adapt cert-find performance workaround for users #2990

tiran · 2019-04-03T16:10:22Z

experimental patch

ipa cert-find --users=NAME was slow on system with lots of certificates.
User certificates have CN=$username, therefore the performance tweak
from ticket 7835 also works for user certificates.

Related: https://pagure.io/freeipa/issue/7835
Fixes: https://pagure.io/freeipa/issue/7901
Signed-off-by: Christian Heimes cheimes@redhat.com

rcritten · 2019-04-04T18:50:30Z

This LGTM.

frasertweedale · 2019-04-05T05:20:50Z

ipaserver/plugins/cert.py

+        # no ra_options are set and exactly one service, host, or user is
+        # supplied.
+        # IPA enforces that subject CN is either a hostname or a username.
+        # The complete flag is left to False to catch overrides.


This code (including the existing workaround) makes some assumptions that I'm not 100% sure about - in particular that the hostname/username is used as the CN in the issued cert (a custom profile may violate this assumption). I need to work through it in more detail, and have run out of week, so I'll resume early next week.

I'm confident that the restriction

freeipa/ipaserver/plugins/cert.py

Lines 747 to 753 in 3509545

elif principal_type == USER:

# check user name

if cn != principal.username:

raise errors.ValidationError(

name='csr',

error=_("DN commonName does not match user's login")

)

won't allow a custom profile to create certs that do have a different CN.

Agreed. I had the same concerns as @frasertweedale but after looking at the code I don't see a way for an IPA-issued cert to not have cn=uid in the subject.

@tiran @rcritten it is actually pretty easy - you just import a profile configuration with parameter like:

policyset.serverCertSet.1.default.params.name=UID=$request.req_subject_name.cn$, O=EXMAPLE.COM

...and update or relax the corresponding constraint component.

The CSR comes in with CN=alice, and comes out with UID=alice (but of course how you configure the subject DN to look is arbitrary).

@frasertweedale Oh no :/

We can either decide to ignore this edge case or somebody has to redesign and rewrite the entire cert_find API. The current implementation doesn't scale at all. Several common cases download all certificates from Dogtag, which puts a lot of stress on 389-DS, Dogtag, and the Python API. The BZ mentions > 100k certs.

I have an idea how to optimize the most critical cases for cert revocation. We can use the fact that host certs, service certs, and user certs are always stored on the host, service, or user LDAP entry. For simple host, service, and user cert search, perform _ldap_search before _ca_search. In _ca_search only retrieve certificates with subject CNs or serial numbers of certs as returned by _ldap_search.

There is one catch: It looks like Dogtag does not support list of serial numbers. :/

We can add support on the Dogtag side for that. I'm inclined to ACK this, given the problem only occurs on an edge case, and file tickets to rework the whole thing.

... once you fix the 'users' / 'user' issue that is :)

frasertweedale · 2019-04-08T08:15:55Z

ipaserver/plugins/cert.py

        if not ra_options:
            services = options.get('service', ())
            hosts = options.get('host', ())
-            if len(services) == 1 and not hosts:
-                principal = kerberos.Principal(options['service'][0])
+            users = options.get('users', ())


This should be options.get('user', ()) (the key in options is 'user').

But even with this correction, this PR regresses in the subject DN corner case I mentioned above.

So... I am considering NAKing this. We might need to go back to the drawing board on cert-find.

OTOH the scenario that breaks this code is a corner case. It's not the end of the world if we ship this patch as-is. cert-find has a lot of limitations as it is, and this patch may help in the common case.

Thanks for the view and good catch! I have updated the patch.

Do we already have a ticket to track the comments? GH has a tendency to eat comments on rebased PRs.

@tiran I filed https://pagure.io/freeipa/issue/7904 to capture the comments and discuss re-working cert-find and friends.

frasertweedale

One definite bug. And a bug in the Subject DN corner case (see inline discussion).

ipa cert-find --users=NAME was slow on system with lots of certificates. User certificates have CN=$username, therefore the performance tweak from ticket 7835 also works for user certificates. Related: https://pagure.io/freeipa/issue/7835 Fixes: https://pagure.io/freeipa/issue/7901 Signed-off-by: Christian Heimes <cheimes@redhat.com>

frasertweedale · 2019-04-09T02:06:10Z

I'm gonna ACK this and we can address the corner case later.
See https://pagure.io/freeipa/issue/7904

tiran · 2019-04-09T07:14:40Z

master:

8a5dc1b Adapt cert-find performance workaround for users

tiran added WIP Work in progress - not ready yet for review ipa-4-7 ipa-4-6 Mark for backport to ipa 4.6 labels Apr 3, 2019

tiran assigned frasertweedale Apr 3, 2019

frasertweedale reviewed Apr 5, 2019

View reviewed changes

frasertweedale reviewed Apr 8, 2019

View reviewed changes

frasertweedale requested changes Apr 8, 2019

View reviewed changes

tiran force-pushed the issue7901_slow_usercert branch from 8214f7f to 0ce63ee Compare April 8, 2019 14:46

tiran added needs review Pull Request is waiting for a review re-run Trigger a new run of PR-CI and removed WIP Work in progress - not ready yet for review labels Apr 8, 2019

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Apr 8, 2019

frasertweedale added ack Pull Request approved, can be merged and removed needs review Pull Request is waiting for a review labels Apr 9, 2019

frasertweedale approved these changes Apr 9, 2019

View reviewed changes

tiran added the pushed Pull Request has already been pushed label Apr 9, 2019

tiran closed this Apr 9, 2019

This was referenced Apr 9, 2019

[Backport][ipa-4-7] Adapt cert-find performance workaround for users #3011

Closed

[Backport][ipa-4-6] Adapt cert-find performance workaround for users #3012

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt cert-find performance workaround for users #2990

Adapt cert-find performance workaround for users #2990

tiran commented Apr 3, 2019

rcritten commented Apr 4, 2019

frasertweedale Apr 5, 2019

tiran Apr 5, 2019

rcritten Apr 5, 2019

frasertweedale Apr 8, 2019 •

edited

Loading

tiran Apr 8, 2019

frasertweedale Apr 8, 2019

frasertweedale Apr 8, 2019

frasertweedale Apr 8, 2019 •

edited

Loading

tiran Apr 8, 2019

frasertweedale Apr 9, 2019

frasertweedale left a comment

frasertweedale commented Apr 9, 2019

tiran commented Apr 9, 2019

	elif principal_type == USER:
	# check user name
	if cn != principal.username:
	raise errors.ValidationError(
	name='csr',
	error=_("DN commonName does not match user's login")
	)

Adapt cert-find performance workaround for users #2990

Adapt cert-find performance workaround for users #2990

Conversation

tiran commented Apr 3, 2019

rcritten commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frasertweedale Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frasertweedale Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frasertweedale left a comment

Choose a reason for hiding this comment

frasertweedale commented Apr 9, 2019

tiran commented Apr 9, 2019

frasertweedale Apr 8, 2019 •

edited

Loading

frasertweedale Apr 8, 2019 •

edited

Loading