ldap_attr, ldap_entry: Add validate_certs option #24060

Akasurde · 2017-04-27T13:50:29Z

SUMMARY

This fix adds a module option `validate_certs' to check
self-signed certificate of LDAP server.

Fixes #24009

Signed-off-by: Abhijeet Kasurde akasurde@redhat.com

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

lib/ansible/modules/network/ldap_attr.py
lib/ansible/modules/network/ldap_entry.py

ANSIBLE VERSION

2.4 devel

ansibot · 2017-04-27T13:55:29Z

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

lib/ansible/modules/network/ldap_attr.py:0:0: E309 version_added for new option (validate_certs) should be 2.4. Currently 0.0
lib/ansible/modules/network/ldap_entry.py:0:0: E309 version_added for new option (validate_certs) should be 2.4. Currently 0.0

click here for bot help

ansibot · 2017-04-27T13:55:30Z

cc @jtyr
click here for bot help

jtyr

I have commented on the ldap_entry.py module only, but the same comments apply on the other one as well.

jtyr · 2017-04-27T14:19:13Z

lib/ansible/modules/network/ldap_entry.py

+    bind_dn: cn=Directory Manager
+    bind_pw: password
+    objectClass: organizationalUnit
+    validate_certs: True


This line doesn't make much sense if the default value is True. The whole example is redundant. Please remove it.

I would prefer to have a example with validate_certs: no.

You don't have to have examples with all possible options. Please remove the example entirely.

jtyr · 2017-04-27T14:21:32Z

lib/ansible/modules/network/ldap_entry.py

@@ -101,6 +101,12 @@
    default: present
    description:
      - The target state of the entry.
+  validate_certs:
+    required: false
+    choices: [true, false]


This should be:

['yes', 'no']

jtyr · 2017-04-27T14:24:03Z

lib/ansible/modules/network/ldap_entry.py

+    choices: [true, false]
+    default: true
+    description:
+      - If false, will ignore self-signed certificates of server and connect


It should be (decoration around the no and period at the end of the sentence):

- If set to C(no), it will ignore self-signed certificates of server and connect.

jtyr · 2017-04-27T14:32:51Z

lib/ansible/modules/network/ldap_entry.py

@@ -268,17 +287,18 @@ def main():
            'server_uri': dict(default='ldapi:///'),
            'start_tls': dict(default=False, type='bool'),
            'state': dict(default='present', choices=['present', 'absent']),
+            'validate_certs': dict(default=True, type='bool'),


I thing the option should be called verify_cert.

Actually, the option validate_certs is also used in the get_url module. I think we should keep it in sync with other modules.

gundalow

Please note that on rebase this files have moved to modules/net_tools/

gundalow · 2017-04-27T16:18:40Z

lib/ansible/modules/network/ldap_attr.py

+    choices: [true, false]
+    default: true
+    description:
+      - If false, will ignore self-signed certificates of server and connect


description: - If C(no), SSL certificates will not be validated. This should only be used on personally controlled sites using self-signed certificates. version_added: "2.4"

gundalow · 2017-04-27T16:19:09Z

lib/ansible/modules/network/ldap_entry.py

@@ -101,6 +101,12 @@
    default: present
    description:
      - The target state of the entry.
+  validate_certs:


Update as per other modules

jtyr · 2017-04-27T16:29:23Z

lib/ansible/modules/net_tools/ldap/ldap_attr.py

@@ -101,6 +101,13 @@
      - The value(s) to add or remove. This can be a string or a list of
        strings. The complex argument format is required in order to pass
        a list of strings (see examples).
+  validate_cert:


This should be validate_certs as per other modules (e.g. get_url). The same anywhere else in the code.

jtyr · 2017-04-27T16:33:48Z

lib/ansible/modules/net_tools/ldap/ldap_attr.py

+    server_uri: ldaps://localhost
+    bind_dn: cn=admin,dc=example,dc=com
+    bind_pw: password
+    validate_cert: no


As I said, it's not really necessary to demonstrate every option. Please remove this example.

Akasurde · 2017-04-28T05:08:45Z

ready_for_review

gundalow · 2017-04-28T11:46:08Z

Please change to

   description:
      - If C(no), SSL certificates will not be validated. This should only be used
        on personally controlled sites using self-signed certificates.
   version_added: "2.4"

jtyr · 2017-04-28T11:50:22Z

@gundalow I think that it's not necessary to mention that it should only be used on personally controlled sites. I would change it to this:

   description:
      - If C(no), SSL certificates will not be validated. This should only be used
        on sites using self-signed certificates.
   version_added: "2.4"

This fix adds a module option `validate_certs' to check self-signed certificate of LDAP server. Fixes ansible#24009 Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>

Akasurde · 2017-04-28T12:00:03Z

@jtyr @gundalow ready_for_review

jtyr · 2017-04-28T12:20:23Z

@Akasurde LGTM now. Thanks for the contribution.

@ansibot please shipit.

Akasurde · 2017-04-28T12:50:13Z

@jtyr @gundalow Thanks for comments.

gundalow · 2017-05-03T15:30:08Z

Merged, thanks for the PR and the reviews.

Akasurde · 2017-05-03T15:33:41Z

@gundalow Thanks a lot.

jtyr suggested changes Apr 27, 2017

View reviewed changes

ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_triage Needs a first human triage before being processed. labels Apr 27, 2017

gundalow added net_tools Net-tools category and removed networking Network category labels Apr 27, 2017

gundalow changed the title ~~Add validate_certs option to ldap_attr, ldap_entry~~ ldap_attr, ldap_entry: Add validate_certs option Apr 27, 2017

Akasurde force-pushed the i24009 branch from dc7afb0 to 7110ded Compare April 27, 2017 16:19

gundalow requested changes Apr 27, 2017

View reviewed changes

Akasurde force-pushed the i24009 branch from 7110ded to b4f8ebf Compare April 27, 2017 16:30

jtyr reviewed Apr 27, 2017

View reviewed changes

ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Apr 27, 2017

Akasurde force-pushed the i24009 branch from b4f8ebf to a2c159f Compare April 28, 2017 04:16

Add validate_certs option to ldap_attr, ldap_entry

6aa7f74

This fix adds a module option `validate_certs' to check self-signed certificate of LDAP server. Fixes ansible#24009 Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>

Akasurde force-pushed the i24009 branch from a2c159f to 6aa7f74 Compare April 28, 2017 11:59

gundalow approved these changes May 3, 2017

View reviewed changes

gundalow merged commit a3053d8 into ansible:devel May 3, 2017

Akasurde deleted the i24009 branch May 3, 2017 15:33

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ldap_attr, ldap_entry: Add validate_certs option #24060

ldap_attr, ldap_entry: Add validate_certs option #24060

Akasurde commented Apr 27, 2017

ansibot commented Apr 27, 2017

ansibot commented Apr 27, 2017

jtyr left a comment

jtyr Apr 27, 2017

Akasurde Apr 27, 2017

jtyr Apr 27, 2017

jtyr Apr 27, 2017

Akasurde Apr 27, 2017

jtyr Apr 27, 2017

Akasurde Apr 27, 2017

jtyr Apr 27, 2017

Akasurde Apr 27, 2017

jtyr Apr 27, 2017

gundalow left a comment

gundalow Apr 27, 2017

gundalow Apr 27, 2017

jtyr Apr 27, 2017

jtyr Apr 27, 2017

Akasurde commented Apr 28, 2017

gundalow commented Apr 28, 2017

jtyr commented Apr 28, 2017

Akasurde commented Apr 28, 2017

jtyr commented Apr 28, 2017

Akasurde commented Apr 28, 2017

gundalow commented May 3, 2017

Akasurde commented May 3, 2017

ldap_attr, ldap_entry: Add validate_certs option #24060

ldap_attr, ldap_entry: Add validate_certs option #24060

Conversation

Akasurde commented Apr 27, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ansibot commented Apr 27, 2017

ansibot commented Apr 27, 2017

jtyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gundalow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akasurde commented Apr 28, 2017

gundalow commented Apr 28, 2017

jtyr commented Apr 28, 2017

Akasurde commented Apr 28, 2017

jtyr commented Apr 28, 2017

Akasurde commented Apr 28, 2017

gundalow commented May 3, 2017

Akasurde commented May 3, 2017