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

New module: Add kinit (identity/kinit) #26861

Open
wants to merge 8 commits into
base: devel
from

Conversation

@bincyber
Contributor

bincyber commented Jul 15, 2017

SUMMARY

Added a new module for obtaining a short-lived Kerberos ticket-granting ticket (TGT).

Some cli tools such as ipa use Kerberos for authentication thus they require obtaining a valid Kerberos ticket before they are executed.

Currently, this can be accomplished using the expect module:

- name: expect | acquire a short-lived Kerberos ticket
  expect:
    command: "/usr/bin/kinit {{ kerberos_principal }} -l 60"
    responses:
      "Password for.*": "{{ password }}"
  no_log: true

- name: shell | register an HTTP service in FreeIPA for this node
  shell: "/usr/bin/ipa service-add HTTP/{{ ansible_fqdn }} > /etc/ipa/service-add-http.log"
  args:
    creates: "/etc/ipa/service-add-http.log"

or the shell module:

- name: shell | acquire a short-lived Kerberos ticket
  shell: "echo {{ password }} | /usr/bin/kinit {{ kerberos_principal }} -l 60"
  no_log: true

However, the expect module requires a newer version of pexpect installed on the host (which is only available via pip on RHEL/CentOS machines and not yum). Also, since no_log is used, it hides error messages if the task fails and does not log. It is possible to use environment variables with the shell module to avoid the need to use no_log:

- name: shell | acquire a short-lived Kerberos ticket
  shell: "echo $KERBEROS_PASSWORD | /usr/bin/kinit {{ kerberos_principal }} -l 60"
  environment:
    KERBEROS_PASSWORD: "{{ password }}"

But I'd prefer a native module.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

kinit

ANSIBLE VERSION
ansible 2.5.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'modules/']
  python version = 2.7.13 (default, May 10 2017, 20:04:28) [GCC 6.3.1 20161221 (Red Hat 6.3.1-1)]
ADDITIONAL INFORMATION

@bincyber bincyber force-pushed the bincyber:kinit_module branch Jul 15, 2017

@bincyber bincyber changed the title from Added new module: kinit (identity/kinit) to New module: kinit (identity/kinit) Jul 15, 2017

@ansibot ansibot removed the new_module label Jul 17, 2017

@alikins alikins requested a review from nitzmahone Jul 17, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Jul 23, 2017

The test ansible-test sanity --test ansible-doc --python 2.7 failed with the following error:

Command "ansible-doc kinit" returned exit status 1.
>>> Standard Error
[ERROR]: unable to parse
/root/src/github.com/ansible/ansible/lib/ansible/modules/identity/kinit.py
ERROR! module kinit missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test ansible-doc --python 3.5 failed with the following error:

Command "ansible-doc kinit" returned exit status 1.
>>> Standard Error
[ERROR]: unable to parse
/root/src/github.com/ansible/ansible/lib/ansible/modules/identity/kinit.py
ERROR! module kinit missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test ansible-doc --python 2.6 failed with the following error:

Command "ansible-doc kinit" returned exit status 1.
>>> Standard Error
[ERROR]: unable to parse
/root/src/github.com/ansible/ansible/lib/ansible/modules/identity/kinit.py
ERROR! module kinit missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test ansible-doc --python 3.6 failed with the following error:

Command "ansible-doc kinit" returned exit status 1.
>>> Standard Error
[ERROR]: unable to parse
/root/src/github.com/ansible/ansible/lib/ansible/modules/identity/kinit.py
ERROR! module kinit missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/identity/kinit.py:36:92: E302 DOCUMENTATION is not valid YAML

click here for bot help

@bincyber bincyber force-pushed the bincyber:kinit_module branch 2 times, most recently Jul 23, 2017

@bincyber bincyber changed the title from New module: kinit (identity/kinit) to New module: Add kinit (identity/kinit) Jul 30, 2017

@bincyber

This comment has been minimized.

Contributor

bincyber commented Jul 30, 2017

ready_for_review

@bincyber

This comment has been minimized.

Contributor

bincyber commented Oct 2, 2017

What do I need to do to get this module merged?

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 27, 2017

@Nosmoht @Akasurde @cyberark-bizdev @dj-wasabi @enunez-cyberark @erasmix @fxfitz

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

lib/ansible/modules/identity/kinit.py Outdated
DOCUMENTATION = '''
---
module: kinit
version_added: "2.4"

This comment has been minimized.

@alxgu

alxgu Nov 27, 2017

Contributor

2.4 is alreay released. Use 2.5

lib/ansible/modules/identity/kinit.py Outdated
password = module.params['password']
lifetime = module.params.get('lifetime', 60)
kinit = find_executable('kinit')

This comment has been minimized.

@alxgu

alxgu Nov 27, 2017

Contributor

the AnsibleModule class has a method to find executables. Use that method:
kinit_cmd = module.get_bin_path("kinit", required=True)

This comment has been minimized.

@bincyber

bincyber Dec 2, 2017

Contributor

Done. Thanks

Added new module: kinit
- added a new module which is a wrapper around kinit to obtain
  a short-lived Kerberos ticket-granting ticket (TGT)
@bincyber

This comment has been minimized.

Contributor

bincyber commented Feb 19, 2018

@puiterwijk Thanks for providing feedback on this PR. I will modify the module to check all collection credential caches using kinit -A and check that any existing TGT is for the right principal.

Updated kinit module
- module now checks all collection credential caches for a TGT
  for the specified principal
@ansibot

This comment has been minimized.

Contributor

ansibot commented Feb 23, 2018

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

lib/ansible/modules/identity/kinit.py:144:0: anomalous-backslash-in-string Anomalous backslash in string: '\s'. String constant might be missing an r prefix.
lib/ansible/modules/identity/kinit.py:178:26: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/identity/kinit.py:181:35: ansible-format-automatic-specification Format string contains automatic field numbering specification

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/identity/kinit.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/identity/kinit.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/identity/kinit.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/identity/kinit.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/identity/kinit.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test no-underscore-variable [explain] failed with 2 errors:

lib/ansible/modules/identity/kinit.py:174:13: use `dummy` instead of `_` for a variable name
lib/ansible/modules/identity/kinit.py:183:13: use `dummy` instead of `_` for a variable name

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/identity/kinit.py:147:24: E201 whitespace after '['
lib/ansible/modules/identity/kinit.py:147:86: E202 whitespace before ']'

The test ansible-test sanity --test validate-modules [explain] failed with 4 errors:

lib/ansible/modules/identity/kinit.py:0:0: E324 Value for "default" from the argument_spec ('present') for "state" does not match the documentation (None)
lib/ansible/modules/identity/kinit.py:0:0: E324 Value for "default" from the argument_spec (60) for "lifetime" does not match the documentation (None)
lib/ansible/modules/identity/kinit.py:0:0: E326 Value for "choices" from the argument_spec (['present', 'absent']) for "state" does not match the documentation ([])
lib/ansible/modules/identity/kinit.py:47:15: E302 DOCUMENTATION is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/identity/kinit.py:47:15: error DOCUMENTATION: syntax error: expected <block end>, but found '<scalar>'

click here for bot help

@ansibot ansibot added ci_verified and removed ci_verified labels Feb 23, 2018

@bincyber bincyber force-pushed the bincyber:kinit_module branch to 05311c3 Feb 23, 2018

@puiterwijk

Looks fine to me, just a few suggestions.

module.exit_json(changed=True, msg='Obtained a Kerberos ticket', cache_name=cache_name)
elif kerberos_ticket.state == 'absent':
if module.check_mode:

This comment has been minimized.

@puiterwijk

puiterwijk Feb 23, 2018

Member

Why do you have a different code path here?
The non-check line does the same but provides a bit more info, so just using that in the check case should be fine I'd say.

else:
module.exit_json(changed=False)
rc, err = kerberos_ticket.renew_ticket()

This comment has been minimized.

@puiterwijk

puiterwijk Feb 23, 2018

Member

I'd say that if there is still a ticket that has a validity of more than was requested, we don't need to try to renew and can just return a changed=False, msg='We already had a sufficient ticket'?

This comment has been minimized.

@bincyber

bincyber Feb 24, 2018

Contributor

That's true. I will update the module.

@bincyber

This comment has been minimized.

Contributor

bincyber commented Mar 6, 2018

ready_for_review

@bincyber

This comment has been minimized.

Contributor

bincyber commented Sep 16, 2018

It's been over a year since I've opened this PR. Can we get this merged please?

@puiterwijk

puiterwijk approved these changes Sep 20, 2018 edited

While the nested if's in the main function seems slightly clunky, I think this code looks reasonable from a krb5 point of view.

@maxamillion

This comment has been minimized.

Contributor

maxamillion commented Sep 24, 2018

@puiterwijk thank you

@maxamillion

This comment has been minimized.

Contributor

maxamillion commented Sep 25, 2018

@alikins I'm good with it if you are, but if there's more to discuss here then let's add it to the community meeting agenda.

@bincyber

This comment has been minimized.

Contributor

bincyber commented Nov 10, 2018

Thanks for reviewing @puiterwijk. @gundalow can this be merged please?

@gundalow

This comment has been minimized.

Contributor

gundalow commented Nov 13, 2018

@bincyber Hi, I've pushed a change to update a few bits of docs.

I've asked @alikins to do a final review

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 13, 2018

The test ansible-test sanity --test shebang [explain] failed with 1 error:

lib/ansible/modules/identity/kinit.py:0:0: module should not be executable

click here for bot help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment