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

ipa-kra-install: fix check_host_keys #766

Closed
wants to merge 1 commit into from

Conversation

flo-renaud
Copy link
Contributor

@flo-renaud flo-renaud commented May 5, 2017

ipa-kra-install on a replica checks that the keys are available before
going further to avoid race condition due to replication. The issue is
that the check_host_keys method expects to find exactly one key for
cn=env/host but 2 may exist: one below cn=custodia and one below
cn=dogtag,cn=custodia.
The fix is to check that at least one key exist (not exactly one key).

https://pagure.io/freeipa/issue/6934

@@ -85,8 +85,8 @@ def check_host_keys(self, host):

ldap_filter = self.build_filter(IPA_CHECK_QUERY, {'host': host})
r = conn.search_s(self.keysbase, scope, ldap_filter)
if len(r) != 1:
raise ValueError("Incorrect number of results (%d) searching for"
if len(r) < 1:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

I think that if not r: is more pythonic and I'd also rephrase the error message, to "No public keys were found ...."

It is just nitpick, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicks for now, but AFAIK pylint 1.7.1 will report these checks as errors.

ipa-kra-install on a replica checks that the keys are available before
going further to avoid race condition due to replication. The issue is
that the check_host_keys method expects to find exactly one key for
cn=env/host but 2 may exist: one below cn=custodia and one below
cn=dogtag,cn=custodia.
The fix is to check that at least one key exist (not exactly one key).

https://pagure.io/freeipa/issue/6934
@flo-renaud
Copy link
Contributor Author

Hi @MartinBasti @martbab
thank you for the comment. PR updated with your suggestion.

@MartinBasti MartinBasti self-assigned this May 9, 2017
@MartinBasti MartinBasti added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels May 9, 2017
@MartinBasti
Copy link
Contributor

master:

  • 8983ce5 ipa-kra-install: fix check_host_keys

ipa-4-5:

  • b90dce8 ipa-kra-install: fix check_host_keys

@MartinBasti MartinBasti closed this May 9, 2017
@flo-renaud flo-renaud deleted the fixkrareplicainstall branch May 16, 2017 14:30
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
3 participants