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

Test script for ipa-custodia #948

Closed
wants to merge 1 commit into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Aug 1, 2017

You may find my test script for ipa-custodia useful for testing and debugging issues like https://bugzilla.redhat.com/show_bug.cgi?id=1476150

Signed-off-by: Christian Heimes cheimes@redhat.com

@tiran tiran requested review from stlaz and simo5 August 1, 2017 09:36
Copy link
Contributor

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

LGTM

@tiran
Copy link
Member Author

tiran commented Aug 1, 2017

@stlaz suggested to add the script to freeipa tools rather than freeipa's contrib directory. I'm happy with either.

@tiran tiran force-pushed the ipa-custodia-tester branch 3 times, most recently from c476f46 to 1c7e978 Compare August 18, 2017 16:48
@stlaz
Copy link
Contributor

stlaz commented Oct 3, 2017

Looks good to me. Perhaps we may keep it here. If you please rebase the PR on current master, @tiran, so that the tests pass and we can push it, that's be nice :)

@stlaz stlaz added the ack Pull Request approved, can be merged label Oct 3, 2017
@Tiboris
Copy link
Member

Tiboris commented Oct 17, 2017

Hi @tiran,

rebase to master could help pr-ci tests to pass. Could you please try it?

@Tiboris Tiboris removed the ack Pull Request approved, can be merged label Oct 17, 2017
@tiran tiran force-pushed the ipa-custodia-tester branch 3 times, most recently from 4da77ab to 11a8b30 Compare October 18, 2017 10:59
@flo-renaud
Copy link
Contributor

Hi @tiran,

Thank you for this tool!

When ipa-custodia-check is run with target server=replica which does not have the CA installed, it reports errors trying to retrieve ca/* keys. To me this should not be considered as an error because the replica does not store the keys. The tool could add a check on the server role "CA" and try to retrieve ca/* keys only if CA role is present, or turn the error into a warning.

An error also happens if run with --store from a replica without CA with target server = CA, because the tool needs /etc/pki/pki-tomcat/password.conf in order to store the certs, and the directory /etc/pki/pki-tomcat does not exist if the replica doesn't have the CA role.

@tiran
Copy link
Member Author

tiran commented Nov 2, 2017

By default the test script attempts to retrieve all possible secrets. IMO it's the correct answer, too. After all the remote server is not able to offer these secrets and therefore cannot be used to create a CA replica. You can limit the keys with command line options, e.g. ipa-custodia-check dm/DMHash.

The store option is rather dangerous. I've changed the script to suppress the option. It's still there but --help no longer lists the argument, --store should only be used if you really know what you are doing.

@rcritten
Copy link
Contributor

rcritten commented Nov 9, 2017

It doesn't handle running on an unconfigured server:

# ipa-custodia-check `hostname`
Traceback (most recent call last):
  File "/home/rcrit/ipa-custodia-check", line 282, in <module>
    main()
  File "/home/rcrit/ipa-custodia-check", line 276, in main
    tester = IPACustodiaTester(parser, args)
  File "/home/rcrit/ipa-custodia-check", line 105, in __init__
    self.realm = api.env.realm
AttributeError: 'Env' object has no attribute 'realm'

@tiran
Copy link
Member Author

tiran commented Nov 9, 2017

I've added a check for is_ipa_configured.

@tiran tiran force-pushed the ipa-custodia-tester branch 2 times, most recently from 6832511 to 1aa9757 Compare November 15, 2017 12:38
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@rcritten
Copy link
Contributor

This looks ok. Before I git it the ack is there a reason you are installing into libexec rather than bin/sbin?

I'm assuming it is because it is a diagnostic and not meant for regular use but just want to confirm first.

@tiran
Copy link
Member Author

tiran commented Nov 15, 2017

Yes, you are correct. I don't want to have a diagnostic tool in sbin. It's not useful for a general audience. The script is designed for rare occasion of a failing replica installation.

@rcritten rcritten added the ack Pull Request approved, can be merged label Nov 15, 2017
@tiran
Copy link
Member Author

tiran commented Nov 16, 2017

master:

  • 38b17e1 Test script for ipa-custodia

@tiran tiran added the pushed Pull Request has already been pushed label Nov 16, 2017
@tiran tiran closed this Nov 16, 2017
@tiran tiran deleted the ipa-custodia-tester branch November 16, 2017 11:18
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
6 participants