Validate SSL in panos_import #36972

kbreit · 2018-03-04T01:57:36Z

SUMMARY

Bug #36936 states panos_import should validate SSL connections to avoid man in the middle attacks.

Fixes #36936

ISSUE TYPE

Docs Pull Request

COMPONENT NAME

panos_import

ANSIBLE VERSION

ansible 2.6.0 (panos/bug-36936 67c27ef877) last updated 2018/03/03 19:53:31 (GMT -500)
  config file = None
  configured module search path = ['/Users/kbreit/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/kbreit/Documents/Programming/ansible/lib/ansible
  executable location = /Users/kbreit/Documents/Programming/ansible/bin/ansible
  python version = 3.5.4 (default, Feb 25 2018, 14:56:02) [GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)]

ADDITIONAL INFORMATION

This should likely be back ported to previous versions since the code seems to be the same.

No difference in output

ansibot · 2018-03-04T02:04:33Z

cc @ivanbojer @jtschichold
click here for bot help

kbreit · 2018-03-04T02:06:37Z

Note to reviewers. This changes default behavior from false to true. It is a much better default but may break existing plays.

ansibot · 2018-03-04T02:11:36Z

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

lib/ansible/modules/network/panos/panos_import.py:53:82: W291 trailing whitespace

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

lib/ansible/modules/network/panos/panos_import.py:0:0: E309 version_added for new option (validate_ssl) should be 2.6. Currently 0.0
lib/ansible/modules/network/panos/panos_import.py:0:0: E325 argument_spec for "validate_ssl" defines type="bool" but documentation does not

click here for bot help

gundalow · 2018-03-04T09:44:54Z

lib/ansible/modules/network/panos/panos_import.py

@@ -60,6 +63,12 @@
            - URL of the file that will be imported to device.
        required: false
        default: None
+    validate_ssl:
+        description:
+            - Whether or not certificates should be validated


Please change to
If C(no), SSL certificates will not be validated. This should only set to no used on personally controlled sites using self-signed certificates.

gundalow · 2018-03-04T09:45:03Z

lib/ansible/modules/network/panos/panos_import.py

+        description:
+            - Whether or not certificates should be validated
+        type: bool
+        default: True


kbreit

Changes applied

ansibot · 2018-03-04T14:04:15Z

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

lib/ansible/modules/network/panos/panos_import.py:0:0: E309 version_added for new option (validate_ssl) should be 2.6. Currently 0.0

click here for bot help

kbreit · 2018-03-05T18:05:52Z

All requested changes should be integrated. It's now just a question of whether we should change the default to validate SSL or if staying with 'false' as default makes sense.

kbreit · 2018-03-20T00:59:17Z

I am going to change to have the default to false to not break backwards compatibility.

If there is still a merge conflict, I’ll need someone to help fix that.

kbreit · 2018-04-02T02:43:20Z

Can this be merged?

ericwb · 2018-04-02T05:50:24Z

lib/ansible/modules/network/panos/panos_import.py

        default: software
    file:
        description:
            - Location of the file to import into device.
    url:
        description:
            - URL of the file that will be imported to device.
+    validate_cert:


To be consistent with other modules, this should be validate_certs, not validate_cert.

ericwb · 2018-04-02T05:52:27Z

lib/ansible/modules/network/panos/panos_import.py

        default: software
    file:
        description:
            - Location of the file to import into device.
    url:
        description:
            - URL of the file that will be imported to device.
+    validate_cert:
+        description:
+            - If C(no), SSL certificates will not be validated. This should only set to no used on personally controlled sites using self-signed certificates.


"This should only set to no used on" - sounds awkward. I also don't recommend reasons for when to turn this off. I'd just warn them it's not recommend. Even personally controlled sites using self-signed certs can fall victim to MITM attacks.

ericwb

Sorry, couple more changes please. Thanks for your changes so far.

- Renamed validate_cert to validate_certs - Changed documentation for disabling cert validation

kbreit

All changes applied and pushed.

ericwb

Looks good to me

gundalow · 2018-04-05T12:29:54Z

Thanks, merged into devel will be releases in 2.6.0
Not backporting as it introduces a new option.

* Fix bug 36936 * Added version_added to argument and fixed whitespace * Update panos_import documentation Update parameter documentation and add note. * Add type documentation * added version number for documentation For real * Integrated recommended changes - Added recommended changes from PR * Changed validate_ssl default back to True considering there is a note at the top of documentation explaining change * Format changes based on recommendations from gundalow * Rename validate_ssl to validate_cert * Change description to remove SSL reference * Change url default ih documentation * Integrated small changes from bug report - Renamed validate_cert to validate_certs - Changed documentation for disabling cert validation

gundalow self-assigned this Mar 4, 2018

gundalow added affects_1.3 and removed needs_triage Needs a first human triage before being processed. labels Mar 4, 2018

gundalow requested changes Mar 4, 2018

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 4, 2018

kbreit commented Mar 4, 2018

View reviewed changes

ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed ci_verified Changes made in this PR are causing tests to fail. labels Mar 4, 2018

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Mar 13, 2018

ansibot removed the stale_review Updates were made after the last review and the last review is more than 7 days old. label Mar 20, 2018

ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 31, 2018

Change url default ih documentation

915943f

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 1, 2018

ericwb reviewed Apr 2, 2018

View reviewed changes

ericwb suggested changes Apr 2, 2018

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 2, 2018

Integrated small changes from bug report

3856bc5

- Renamed validate_cert to validate_certs - Changed documentation for disabling cert validation

kbreit commented Apr 2, 2018

View reviewed changes

ericwb approved these changes Apr 2, 2018

View reviewed changes

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 2, 2018

acozine moved this from In progress to Community Modules Maintenance in Ansible-maintained Collections Documentation Apr 4, 2018

gundalow approved these changes Apr 5, 2018

View reviewed changes

gundalow merged commit d5cfc54 into ansible:devel Apr 5, 2018

Ansible-maintained Collections Documentation automation moved this from Community Modules Maintenance to Done Apr 5, 2018

kbreit deleted the panos/bug-36936 branch November 9, 2018 02:56

ansible locked and limited conversation to collaborators Apr 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate SSL in panos_import #36972

Validate SSL in panos_import #36972

kbreit commented Mar 4, 2018

ansibot commented Mar 4, 2018

kbreit commented Mar 4, 2018

ansibot commented Mar 4, 2018

gundalow Mar 4, 2018

gundalow Mar 4, 2018

kbreit left a comment

ansibot commented Mar 4, 2018

kbreit commented Mar 5, 2018

kbreit commented Mar 20, 2018

kbreit commented Apr 2, 2018

ericwb Apr 2, 2018

ericwb Apr 2, 2018

ericwb left a comment

kbreit left a comment

ericwb left a comment

gundalow commented Apr 5, 2018

Validate SSL in panos_import #36972

Validate SSL in panos_import #36972

Conversation

kbreit commented Mar 4, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Mar 4, 2018

kbreit commented Mar 4, 2018

ansibot commented Mar 4, 2018

gundalow Mar 4, 2018

Choose a reason for hiding this comment

gundalow Mar 4, 2018

Choose a reason for hiding this comment

kbreit left a comment

Choose a reason for hiding this comment

ansibot commented Mar 4, 2018

kbreit commented Mar 5, 2018

kbreit commented Mar 20, 2018

kbreit commented Apr 2, 2018

ericwb Apr 2, 2018

Choose a reason for hiding this comment

ericwb Apr 2, 2018

Choose a reason for hiding this comment

ericwb left a comment

Choose a reason for hiding this comment

kbreit left a comment

Choose a reason for hiding this comment

ericwb left a comment

Choose a reason for hiding this comment

gundalow commented Apr 5, 2018