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

RFC: API for reporting PKINIT status #790

Closed
wants to merge 6 commits into from

Conversation

martbab
Copy link
Contributor

@martbab martbab commented May 16, 2017

This PR implements easily-consumable API that reports PKINIT status on masters
based on the presence of pkinitEnabled value in KDC entry's ipaConfigString
attribute.

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

@martbab martbab force-pushed the pkinit-status-reporting-api branch 2 times, most recently from bac2e2e to 0b96daf Compare May 19, 2017 15:50
Martin Babinsky added 6 commits May 24, 2017 17:07
In order to achieve the task, the following changes were required:

* vectorize the base class for server attributes
* add a child class that enforces single-value attributes. It still
  accepts/returns single-value lists in order to not break Liskov
  substitution principle
* Existing attributes inherit from the child class

https://pagure.io/freeipa/issue/6937
The `config` object now hosts a generic method for updating the config
entry for desired server role configuration (if not empty). The
duplicated code in dns/trust/vaultconfig commands was replaced by a call
to a common method.

https://pagure.io/freeipa/issue/6937
A new multi-valued server attribute `pkinit_server` was added which
reports IPA masters that have PKINIT configuration usable by clients.

The existing tests were modified to allow for testing the new attribute.

https://pagure.io/freeipa/issue/6937
This command is a more streamlined reporting tool for PKINIT feature
status in the FreeIPA topology. It prints out whether PKINIT is enabled
or disabled on individual masters in a topology. If a`--server` is
specified, it reports status for an individual server. If `--status` is
specified, it searches for all servers that have PKINIT enabled or
disabled.

https://pagure.io/freeipa/issue/6937
The test fixture haphazardly intermixed MockLDAP and ldap2 calls in
setup and teardown code, greatly hampering extension of the code and
also porting efforts to Python 3. Get rid of MockLDAP and use ldap2 for
all LDAP operations.

https://pagure.io/freeipa/issue/6937
@martbab martbab force-pushed the pkinit-status-reporting-api branch from 0b96daf to c8a14e9 Compare May 24, 2017 15:18
@pvoborni pvoborni added the prioritized Pull Request has higher priority for PR-CI label May 25, 2017
@HonzaCholasta
Copy link
Contributor

LGTM.

@martbab
Copy link
Contributor Author

martbab commented May 26, 2017

@HonzaCholasta thanks for looking on API, anyone for functional review?

@stlaz
Copy link
Contributor

stlaz commented May 26, 2017

I'll do it.

@stlaz
Copy link
Contributor

stlaz commented May 26, 2017

Run on replica:

[slaznick@machine ~]$ ipa help pkinit
ipa: ERROR: TypeError: 'NoneType' object has no attribute '__getitem__'
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/ipalib/cli.py", line 1351, in run
    sys.exit(api.Backend.cli.run(argv))
  File "/usr/lib/python2.7/site-packages/ipalib/cli.py", line 1105, in run
    kw = self.parse(cmd, argv[1:])
  File "/usr/lib/python2.7/site-packages/ipalib/cli.py", line 1125, in parse
    parser = self.build_parser(cmd)
  File "/usr/lib/python2.7/site-packages/ipalib/cli.py", line 1142, in build_parser
    usage=' '.join(self.usage_iter(cmd)),
  File "/usr/lib/python2.7/site-packages/ipalib/cli.py", line 1220, in usage_iter
    for arg in cmd.args():
  File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 255, in __get__
    obj.ensure_finalized()
  File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 230, in ensure_finalized
    self.finalize()
  File "/usr/lib/python2.7/site-packages/ipalib/plugable.py", line 210, in finalize
    self._on_finalize()
  File "/usr/lib/python2.7/site-packages/ipalib/cli.py", line 733, in _on_finalize
    if c.NO_CLI:
  File "/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/schema.py", line 247, in NO_CLI
    halp = self._schema[self.schema_key].get_help(self.full_name)
  File "/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/schema.py", line 329, in get_help
    return self._schema.get_help(self.name, key)
  File "/usr/lib/python2.7/site-packages/ipaclient/remote_plugins/schema.py", line 520, in get_help
    return self._help[namespace][member]
TypeError: 'NoneType' object has no attribute '__getitem__'
ipa: ERROR: an internal error has occurred

@stlaz
Copy link
Contributor

stlaz commented May 26, 2017

The above error is not related to this PR.

@stlaz
Copy link
Contributor

stlaz commented May 26, 2017

Tested like this:

[4.5R no] -- [4.4M no] -- [4.5R no]
            | UPGRADE |
[4.5R no] -- [4.5M yes] -- [4.5R no]
                | |
[4.5R yes] -- [4.5M yes] -- [4.5R no]
                | |
[4.5R yes] -- [4.5M yes] -- [4.5R yes]

and pkinit-status always reported correctly on all servers providing the command.
For a common Franta user, 0 records are shown which I suppose is OK, even though some permission message would probably be better. I guess that's more about the framework though, so I am ACKing this.

@stlaz stlaz added the ack Pull Request approved, can be merged label May 26, 2017
@martbab
Copy link
Contributor Author

martbab commented May 26, 2017

Well the command is intended to be used either by administrators or by hosts themselves so I have no problem with unprivileged users not seeing anything. We can fix it in a separate PR if the need arises anyway.

@martbab
Copy link
Contributor Author

martbab commented May 26, 2017

ipa-4-5:

  • c4aa3a1 Allow for multivalued server attributes
  • 753f8cf Refactor the role/attribute member reporting code
  • fbccb74 Add an attribute reporting client PKINIT-capable servers
  • 733cef9 Add the list of PKINIT servers as a virtual attribute to global config
  • 6b815aa Add pkinit-status command
  • 4fa29a3 test_serverroles: Get rid of MockLDAP and use ldap2 instead

master:

  • bddb90f Allow for multivalued server attributes
  • cac7e49 Refactor the role/attribute member reporting code
  • d8bb23a Add an attribute reporting client PKINIT-capable servers
  • f805532 Add the list of PKINIT servers as a virtual attribute to global config
  • 9935273 Add pkinit-status command
  • 58fd229 test_serverroles: Get rid of MockLDAP and use ldap2 instead

@martbab martbab added the pushed Pull Request has already been pushed label May 26, 2017
@martbab martbab closed this May 26, 2017
@martbab martbab deleted the pkinit-status-reporting-api branch May 26, 2017 14:13
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 prioritized Pull Request has higher priority for PR-CI pushed Pull Request has already been pushed
Projects
None yet
4 participants