Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dogtag: search past the first 100 certificates #359

Closed
wants to merge 2 commits into from
Closed

dogtag: search past the first 100 certificates #359

wants to merge 2 commits into from

Conversation

HonzaCholasta
Copy link
Contributor

@HonzaCholasta HonzaCholasta commented Dec 21, 2016

Dogtag requires a size limit to be specified when searching for
certificates. When no limit is specified in the dogtag plugin, a limit of
100 entries is assumed. As a result, an unlimited certificate search
returns data only for a maximum of 100 certificates.

Raise the "unlimited" limit to the maximum value Dogtag accepts.

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

@tkrizek tkrizek self-assigned this Dec 21, 2016
@tkrizek
Copy link
Contributor

tkrizek commented Dec 21, 2016

With this fix, more than 100 certificates are displayed and click-able from WebUI overview. However, I'm still getting an error message pop up saying

Search result has been truncated: Configured size limit exceeded

And there is also this message at the bottom of the page:

Query returned more results than the configured size limit. Displaying the first 110 results.

@frasertweedale
Copy link
Contributor

This change is working for me, including having the expected behaviour for WebUI. @tomaskrizek please provide steps to reproduce your WebUI behaviour.

@tkrizek
Copy link
Contributor

tkrizek commented Jan 3, 2017

@frasertweedale
I ran into this issue when I created 100 users with different user certificates:

for i in {300..400}; do
ipa user-add "test$i" --first T --last T;
openssl req -new -newkey rsa:1024 -days 365 -nodes -keyout "private$i.key" -out "cert$i.csr" -subj "/CN=test$i";
ipa cert-request "cert$i.csr" --principal "test$i" ;
done

Please let me know if you are able to reproduce the issue in this way. It might be possible some unrelated issues may be the cause here.

@frasertweedale
Copy link
Contributor

@tomaskrizek yes, I can reproduce with your steps.

@frasertweedale
Copy link
Contributor

frasertweedale commented Jan 4, 2017

@tomaskrizek @HonzaCholasta it looks like the problem is:

  1. subsearches are conducted in order:

    1. _cert_search (if 'certificate' in options add key to result and "seal" it)
    2. _ca_search (actually perform the search against Dogtag, via ra.find)
    3. _ldap_search (look for local entries that have given cert in their userCertificate attr.
  2. if no explicit sizelimit is requested, and if there are > 100 entries with (userCertificate=*), _ldap_search will be truncated, and this result is carried across to the final result. The cert search from Dogtag is not truncated, but the search for entries to use to filter the result may have been truncated.

The simplest way to resolve this is (I think) to forcibly execute _ldap_search with sizelimit=0.

IMO _ldap_search should also be avoided or short-circuited if none of the owner-flitering options to cert-find are given. (edit to note: this will not find certs that are in IPA LDAP but not in Dogtag, which is guess is the wrong behaviour..? So I think we just have to have sizelimit=0. I am concerned about performance impact of cert-find with many principals with certs set... but that is a separate issue).

@stlaz
Copy link
Contributor

stlaz commented Jan 4, 2017

@frasertweedale if _ldap_search is performed with correct filters, sizelimit=0 is not the correct solution at least for CLI which should either follow the sizelimit argument if set or the records size limit in ipa config. It is only correct for WebUI which I believe should be setting sizelimit=0 and if it's not, I'd be looking for the bug there.

I tried to briefly go through the cert plugin code but it's a bit messy so my only hope is that the correct filter is indeed used there. On the way through it, though, I found something that seems like another size limit bug: https://github.com/freeipa/freeipa/blob/master/ipaserver/plugins/cert.py#L1306 -> which will not set our "unlimited" if sizelimit is set to 0. Also from there, if sizelimit is not set, we should go with ipa config sizelimit rather than having the magic do its trick somewhere else, right? Then the proposed value in options.get() could go away (be set in the cert.py module instead).

@frasertweedale
Copy link
Contributor

@stlaz as I see it, the _ldap_search can potentially search all objects of a particular type (user/service/host), which have (userCertificate=*). The result is then used to filter or add to the result, depending on whether the result is "key complete" or not (indicated by the variable complete).

Anyhow I leave to Honza to comment further; he probably understands the code better than me :)

Dogtag requires a size limit to be specified when searching for
certificates. When no limit is specified in the dogtag plugin, a limit of
100 entries is assumed. As a result, an unlimited certificate search
returns data only for a maximum of 100 certificates.

Raise the "unlimited" limit to the maximum value Dogtag accepts.

https://fedorahosted.org/freeipa/ticket/6564
@HonzaCholasta
Copy link
Contributor Author

I have identified some issues in search limit handling in cert-find and fixed them in an additional commit. See commit message for details.

If search limits are not specified in cert-find, use the configured limits.
This applies to the certificate search in the CA as well.

Detect and report if size limit was exceeded in the certificate search in
the CA.

Do not apply limits to the internal ca-find call.

https://fedorahosted.org/freeipa/ticket/6564
@frasertweedale
Copy link
Contributor

I think the behaviour of the command is correct now, but the Web UI is now limited
to 100 certs without (as far as I can see) a way to adjust the sizelimit or search past the first 100.

@tkrizek
Copy link
Contributor

tkrizek commented Jan 23, 2017

The behavior of the command seems to be correct now, but I'm also not sure about the WebUI. There seems to be a limit of 20 items when displayed in WebUI (with pagination). I'm not sure if it's possible to configure that.

@pvomacka Were there any recent changes in the WebUI pagination? Is it possible to configure how many items should be displayed?

@tkrizek
Copy link
Contributor

tkrizek commented Jan 24, 2017

We examined the WebUI side and it behaves as expected - the size limit is respected when viewing certificates.

@tkrizek tkrizek added the ack Pull Request approved, can be merged label Jan 24, 2017
@HonzaCholasta HonzaCholasta added the pushed Pull Request has already been pushed label Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants