Repository: ansible/ansible
Pull request: Give subversion module a validate_certs option
##### SUMMARY
Change:
- Add `validate_certs` option to subversion module. Defaults to off for
  backwards compatibility.

Tickets:
- Fixes #22599
- Fixes #40036

Signed-off-by: Rick Elrod <rick@elrod.me>

<!--- Describe the change below, including rationale and design decisions -->

<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Feature Pull Request

##### COMPONENT NAME

subversion module

Problem statement:
Subversion module disables certificate validation.
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and master branch are affected too.
-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Bug Report

##### COMPONENT NAME
<!--- Name of the module/plugin/task/feature -->
subversion

##### ANSIBLE VERSION
<!--- Paste verbatim output from “ansible --version” between quotes below -->
```
ansible 2.2.1.0
```

##### CONFIGURATION
<!---
Mention any settings you have changed/added/removed in ansible.cfg
(or using the ANSIBLE_* environment variables).
-->

##### OS / ENVIRONMENT
<!---
Mention the OS you are running Ansible from, and the OS you are
managing, or say “N/A” for anything that is not platform-specific.
-->

##### SUMMARY
<!--- Explain the problem briefly -->
The ansible subversion module disables X.509 certificate validation in subversion by unconditionally pasing the --trust-server-cert flag to svn. This allows MitM attacks on subversion.

##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->
Subversion uses the default trusted CA list to validate the server certificate when using HTTPS as transport.

##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->
The Subversion client trusts any X.509 certificate presented to it.

Subversion module does not validate TLS certificate
<!---
Verify first that your issue/request is not already reported on GitHub.
THIS FORM WILL BE READ BY A MACHINE, COMPLETE ALL SECTIONS AS DESCRIBED.
Also test if the latest release, and devel branch are affected too.
ALWAYS add information AFTER (OUTSIDE) these html comments.
Otherwise it may end up being automatically closed by our bot. -->

##### ISSUE TYPE
 - Bug Report

##### COMPONENT NAME
source_control/subversion.py

##### ANSIBLE VERSION
<!--- Paste, BELOW THIS COMMENT, verbatim output from "ansible --version" between quotes below -->
```
ansible 2.5.2
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/feld/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
```

##### CONFIGURATION
N/A; nothing changed

##### OS / ENVIRONMENT
CentOS Linux release 7.4.1708 (Core) 

##### SUMMARY
<!--- Explain the problem briefly -->
svn checkouts over HTTPS do not validate the certificate because the argument **--trust-server-cert** is hardcoded into the module

##### STEPS TO REPRODUCE
n/a

##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->
Validation that the certificate is from a trusted CA

##### ACTUAL RESULTS
Accepts the certificate of the server if the hostname matches and it is not expired

Hints from issue discussion:
cc @dsummersl
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
We should add a `validate_certs` parameter to the module to control this behavior.
Files identified in the description:
* [lib/ansible/modules/source_control/subversion.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/source_control/subversion.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @dsummersl
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Files identified in the description:
* [`lib/ansible/modules/subversion.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/subversion.py)

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->

Commit message: Give subversion module a validate_certs option

Change:
- Add `validate_certs` option to subversion module. Defaults to off for
  backwards compatibility.

Tickets:
- Fixes #22599

Signed-off-by: Rick Elrod <rick@elrod.me>

Patch to review:
diff --git a/changelogs/fragments/22599_svn_validate_certs.yml b/changelogs/fragments/22599_svn_validate_certs.yml
new file mode 100644
index 00000000000000..2fae8199d63933
--- /dev/null
+++ b/changelogs/fragments/22599_svn_validate_certs.yml
@@ -0,0 +1,2 @@
+minor_changes:
+  - subversion - Now has a ``validate_certs`` option, which, when true, will avoid passing ``--trust-server-cert`` to ``svn`` commands (https://github.com/ansible/ansible/issues/22599)
diff --git a/lib/ansible/modules/subversion.py b/lib/ansible/modules/subversion.py
index 730d26f0571b6c..34218705bd1d88 100644
--- a/lib/ansible/modules/subversion.py
+++ b/lib/ansible/modules/subversion.py
@@ -85,6 +85,13 @@
     default: "yes"
     version_added: "2.0"
     type: bool
+  validate_certs:
+    description:
+      - If C(no), passes the C(--trust-server-cert) flag to svn.
+      - If C(yes), does not pass the flag.
+    default: "no"
+    version_added: "2.11"
+    type: bool
 
 requirements:
     - subversion (the command line tool with C(svn) entrypoint)
@@ -119,7 +126,7 @@
 
 
 class Subversion(object):
-    def __init__(self, module, dest, repo, revision, username, password, svn_path):
+    def __init__(self, module, dest, repo, revision, username, password, svn_path, validate_certs):
         self.module = module
         self.dest = dest
         self.repo = repo
@@ -127,6 +134,7 @@ def __init__(self, module, dest, repo, revision, username, password, svn_path):
         self.username = username
         self.password = password
         self.svn_path = svn_path
+        self.validate_certs = validate_certs
 
     def has_option_password_from_stdin(self):
         rc, version, err = self.module.run_command([self.svn_path, '--version', '--quiet'], check_rc=True)
@@ -137,9 +145,10 @@ def _exec(self, args, check_rc=True):
         bits = [
             self.svn_path,
             '--non-interactive',
-            '--trust-server-cert',
             '--no-auth-cache',
         ]
+        if not self.validate_certs:
+            bits.append('--trust-server-cert')
         stdin_data = None
         if self.username:
             bits.extend(["--username", self.username])
@@ -257,6 +266,7 @@ def main():
             update=dict(type='bool', default=True),
             switch=dict(type='bool', default=True),
             in_place=dict(type='bool', default=False),
+            validate_certs=dict(type='bool', default=False),
         ),
         supports_check_mode=True,
     )
@@ -273,6 +283,7 @@ def main():
     checkout = module.params['checkout']
     update = module.params['update']
     in_place = module.params['in_place']
+    validate_certs = module.params['validate_certs']
 
     # We screenscrape a huge amount of svn commands so use C locale anytime we
     # call run_command()
@@ -281,7 +292,7 @@ def main():
     if not dest and (checkout or update or export):
         module.fail_json(msg="the destination directory must be specified unless checkout=no, update=no, and export=no")
 
-    svn = Subversion(module, dest, repo, revision, username, password, svn_path)
+    svn = Subversion(module, dest, repo, revision, username, password, svn_path, validate_certs)
 
     if not export and not update and not checkout:
         module.exit_json(changed=False, after=svn.get_remote_revision())


Code review comments:
diff_hunk: @@ -0,0 +1,2 @@
+minor_changes:
+  - subversion - Now has a ``validate_certs`` option, which, when true, will avoid passing ``--trust-server-cert`` to ``svn`` commands (https://github.com/ansible/ansible/issues/22599)
path: changelogs/fragments/22599_svn_validate_certs.yml
text: @user1:
```suggestion
  - subversion - ``validate_certs`` option, which, when true, will avoid passing ``--trust-server-cert`` to ``svn`` commands (https://github.com/ansible/ansible/issues/22599).
```