Change in vdsm[master]: register: Add --ca-file option

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has uploaded a new change for review.

Change subject: register: Add --ca-file option
......................................................................

register: Add --ca-file option

Currently, we storage the Engine ca file in /etc/ovirt-engine/ca.pem
which is not required as it's used during the runtime.
This patch will add the --ca-file in case users want to keep the PEM file.

Change-Id: Ie24793674569107148c832f0395807586044b95e
Signed-off-by: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
---
M lib/vdsm/tool/register.py
1 file changed, 45 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/76/44576/1

diff --git a/lib/vdsm/tool/register.py b/lib/vdsm/tool/register.py
index ffb81c2..512829d 100644
--- a/lib/vdsm/tool/register.py
+++ b/lib/vdsm/tool/register.py
@@ -21,6 +21,7 @@
 import logging
 import os
 import pwd
+import shutil
 import ssl
 import sys
 import tempfile
@@ -40,12 +41,14 @@
                  fingerprint=None, ssh_port=None,
                  ssh_user=None, check_fqdn=True,
                  vdsm_port=None, node_address=None,
-                 vdsm_uuid=None, node_name=None):
+                 vdsm_uuid=None, node_name=None,
+                 ca_file=None):
         """
         Attributes:
 
         engine_fqdn       - Engine FQDN or IP address
         engine_https_port - Engine https port
+        ca_file           - Path to store the CA file from Engine
         fingeprint        - Fingerprint to be validated
         ssh_user          - SSH user that will establish the connection
                             from Engine
@@ -115,9 +118,13 @@
         self.logger.debug("VDSM UUID: {uuid_provided}".format(
                           uuid_provided=self.vdsm_uuid))
 
-        self.ca_dir = "/etc/pki/ovirt-engine/"
-        self.ca_engine = "{d}{f}".format(d=self.ca_dir, f="ca.pem")
-        self.logger.debug("Engine CA: {ca}".format(ca=self.ca_engine))
+        if ca_file is None:
+            self.temp_ca_file = True
+            self.ca_engine = None
+        else:
+            self.temp_ca_file = False
+            self.ca_engine = ca_file
+            self.logger.debug("Engine CA: {ca}".format(ca=self.ca_engine))
 
     def handshake(self):
         """
@@ -200,7 +207,7 @@
 
         Returns: Content of http request
         """
-        if self.check_fqdn:
+        if self.check_fqdn and cert_validation:
             cert_validation = self.ca_engine
         else:
             cert_validation = False
@@ -281,32 +288,44 @@
 
     def download_ca(self):
         """
-        Download CA from Engine and save self.ca_engine
+        Download CA from Engine and if CA file was specified in
+        the arguments, save it. Otherwise, use temp file.
         """
         self.logger.info("Collecting CA data from Engine...")
-        # If engine CA dir doesnt exist create it and download the ca.pem
-        temp_ca_file = None
-        if os.path.exists(self.ca_engine):
+
+        if self.ca_engine is None:
+            ca_dir = "/tmp"
+        else:
+            ca_dir = os.path.dirname(self.ca_engine)
+
+        if self.ca_engine and os.path.exists(self.ca_engine):
             calculated_fprint = self._calculate_fingerprint(self.ca_engine)
         else:
-            if not os.path.exists(self.ca_dir):
-                os.makedirs(self.ca_dir, 0o755)
-                self._silent_restorecon(self.ca_dir)
+            if not os.path.exists(ca_dir):
+                os.makedirs(ca_dir, 0o755)
+                self._silent_restorecon(ca_dir)
                 if utils.isOvirtNode():
                     from ovirt.node.utils.fs import Config
-                    Config().persist(self.ca_dir)
+                    Config().persist(ca_dir)
 
             res = self._execute_http_request(self.url_CA,
                                              cert_validation=False)
 
             with tempfile.NamedTemporaryFile(
-                dir=os.path.dirname(self.ca_dir),
+                dir=os.path.dirname(ca_dir),
                 delete=False
             ) as f:
                 f.write(res)
 
             calculated_fprint = self._calculate_fingerprint(f.name)
-            temp_ca_file = True
+
+            if self.temp_ca_file:
+                self.ca_engine = f.name
+            else:
+                shutil.move(f.name, self.ca_engine)
+                if utils.isOvirtNode():
+                    from ovirt.node.utils.fs import Config
+                    Config().persist(self.ca_engine)
 
         if self.fprint and self.fprint.lower() != calculated_fprint.lower():
             msg = "The fingeprints doesn't match:\n" \
@@ -315,20 +334,11 @@
                                                          a=self.fprint)
 
             self.logger.debug(msg)
-            if temp_ca_file:
-                os.unlink(f.name)
             raise RuntimeError(msg)
-
-        if temp_ca_file:
-            os.rename(f.name, self.ca_engine)
 
         self.fprint = calculated_fprint
         self.logger.info("Calculated fingerprint: {f}".format(
                          f=self.fprint))
-
-        if utils.isOvirtNode():
-            from ovirt.node.utils.fs import Config
-            Config().persist(self.ca_engine)
 
     def download_ssh(self):
         """
@@ -377,6 +387,9 @@
         self._execute_http_request(self.url_reg)
         self.logger.info("Registration completed, host is pending approval"
                          " on Engine: {e}".format(e=self.engine_fqdn))
+
+        if self.temp_ca_file:
+            os.unlink(self.ca_engine)
 
 
 @expose("register")
@@ -455,6 +468,12 @@
         help="Enforce VDSM UUID.",
     )
 
+    parser.add_argument(
+        '--ca-file',
+        help="Specify the full path for the CA file, it will be created"
+             " if doesn't exist."
+    )
+
     # Using [1:] to remove the 'register' option from arguments
     # and avoid vdsm-tool recognize it as an unknown option
     args = parser.parse_args(args=args[1:])
@@ -468,7 +487,8 @@
                    ssh_port=args.ssh_port,
                    fingerprint=args.fingerprint,
                    check_fqdn=args.check_fqdn,
-                   vdsm_uuid=args.vdsm_uuid)
+                   vdsm_uuid=args.vdsm_uuid,
+                   ca_file=args.ca_file)
 
     try:
         reg.handshake()


-- 
To view, visit https://gerrit.ovirt.org/44576
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie24793674569107148c832f0395807586044b95e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

automation(a)ovirt.org has posted comments on this change.

Change subject: register: Add --ca-file option
......................................................................


Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/44576
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie24793674569107148c832f0395807586044b95e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: register: Add --ca-file option
......................................................................


Patch Set 1: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/44576
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie24793674569107148c832f0395807586044b95e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Change in vdsm[master]: register: Add --ca-file

automation(a)ovirt.org has posted comments on this change.

Change subject: register: Add --ca-file
......................................................................


Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/44576
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie24793674569107148c832f0395807586044b95e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Change in vdsm[master]: register: Add --ca-file

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: register: Add --ca-file
......................................................................


Patch Set 2: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/44576
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie24793674569107148c832f0395807586044b95e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Change in vdsm[master]: register: Add --ca-file

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: register: Add --ca-file
......................................................................


Patch Set 2: Code-Review-1

(4 comments)

https://gerrit.ovirt.org/#/c/44576/2/lib/vdsm/tool/register.py
File lib/vdsm/tool/register.py:

Line 119:         self.logger.debug("VDSM UUID: {uuid_provided}".format(
Line 120:                           uuid_provided=self.vdsm_uuid))
Line 121: 
Line 122:         if ca_file is None:
Line 123:             self.temp_ca_file = True
why do you need this boolean? having ca_engine=None is enough to tell that we do not need
to store the ca cert.
Line 124:             self.ca_engine = None
Line 125:         else:
Line 126:             self.temp_ca_file = False
Line 127:             self.ca_engine = ca_file


Line 207:         cert_validation -- SSL cert will be verified
Line 208: 
Line 209:         Returns: Content of http request
Line 210:         """
Line 211:         if self.check_fqdn and cert_validation:
is this related to this patch? I think it deserves its own justification
Line 212:             cert_validation = self.ca_engine
Line 213:         else:
Line 214:             cert_validation = False
Line 215: 


Line 294:         """
Line 295:         self.logger.info("Collecting CA data from Engine...")
Line 296: 
Line 297:         if self.ca_engine is None:
Line 298:             ca_dir = "/tmp"
never use /tmp directly. tempdir knows which path should be used for temporary files.
Line 299:         else:
Line 300:             ca_dir = os.path.dirname(self.ca_engine)
Line 301: 
Line 302:         if self.ca_engine and os.path.exists(self.ca_engine):


Line 387:         """
Line 388:         self._execute_http_request(self.url_reg)
Line 389:         self.logger.info("Registration completed, host is pending
approval"
Line 390:                          " on Engine:
{e}".format(e=self.engine_fqdn))
Line 391: 
this cleanup does not seem related to execute_registration.

I find this logic very confusing. download_ca should download the ca cert to a temporary
directory.

If the user asks to persist it, a special function would copy the file and persist it.

Immediately after use, the temp dir should be removed.
Line 392:         if self.temp_ca_file:
Line 393:             os.unlink(self.ca_engine)
Line 394: 
Line 395: 


-- 
To view, visit https://gerrit.ovirt.org/44576
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie24793674569107148c832f0395807586044b95e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes