Alon Bar-Lev has posted comments on this change. Change subject: Avoiding legacy health servlet usage ......................................................................
Patch Set 3: (7 comments) ok, this means that you do not store the certificate anywhere... but you do pass credentials. so I still suggest to prompt for the certificate fingerprint and subject and accept it after download, before proceeding, to avoid future CVEs http://gerrit.ovirt.org/#/c/26878/3/src/ovirt_hosted_engine_setup/check_liveliness.py File src/ovirt_hosted_engine_setup/check_liveliness.py: Line 43: TIMEOUT = 20 Line 44: Line 45: def __init__(self): Line 46: super(LivelinessChecker, self).__init__() Line 47: self.cacert = None _cacert? Line 48: Line 49: def _getPKICert(self, fqdn): Line 50: def check_ignore(*args, **kw): Line 51: return True Line 56: sock.set_post_connection_check_callback(check_ignore) Line 57: sock.connect(fqdn) Line 58: sock.shutdown(socket.SHUT_RDWR) Line 59: c_c = [c.as_pem() for c in sock.get_peer_cert_chain()] Line 60: if c_c[-1]: if you need only -1, then get it from the sock.get... directly, no? if you do not have any, raise exception. Line 61: self.logger.debug(content) Line 62: fd, self.cacert = tempfile.mkstemp( Line 63: prefix='engine-ca', Line 64: suffix='.crt', Line 58: sock.shutdown(socket.SHUT_RDWR) Line 59: c_c = [c.as_pem() for c in sock.get_peer_cert_chain()] Line 60: if c_c[-1]: Line 61: self.logger.debug(content) Line 62: fd, self.cacert = tempfile.mkstemp( where do you delete it? Line 63: prefix='engine-ca', Line 64: suffix='.crt', Line 65: ) Line 66: with os.fdopen(fd, 'w') as fileobj: Line 98: return isUp Line 99: Line 100: def __exit__(self, type, value, tb): Line 101: if self.cert is not None and os.path.exists(self.cert): Line 102: os.unlink(self.cert) oh... shouldn't this be self.cacert? Line 103: Line 104: Line 105: if __name__ == "__main__": Line 106: import sys http://gerrit.ovirt.org/#/c/26878/3/src/plugins/ovirt-hosted-engine-setup/engine/health.py File src/plugins/ovirt-hosted-engine-setup/engine/health.py: Line 89 Line 90 Line 91 Line 92 Line 93 why not raise as well? or keep asking? but how come we get response tha is out of valid values scope? Line 83: '(2) Power off and restart the VM\n' Line 84: '(3) Abort setup\n\n(@VALUES@)[@DEFAULT@]: ' Line 85: ), Line 86: prompt=True, Line 87: validValues=(_('1'), _('2'), _('3')), why don't you accept 'continue', 'poweroff', 'abort'? much more friendly and consistence with future set within answer file Line 88: default=_('1'), Line 89: caseSensitive=False) Line 90: if response == _('1').lower(): Line 91: if live_checker.isEngineUp(fqdn): Line 86: prompt=True, Line 87: validValues=(_('1'), _('2'), _('3')), Line 88: default=_('1'), Line 89: caseSensitive=False) Line 90: if response == _('1').lower(): this is strange... Line 91: if live_checker.isEngineUp(fqdn): Line 92: poll = False Line 93: else: Line 94: self.dialog.note( -- To view, visit http://gerrit.ovirt.org/26878 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3522ccb82eee4bf7f04ded012d9badc97c55b5a0 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: David Caro <dcaro...@redhat.com> Gerrit-Reviewer: Lev Veyde <lve...@gmail.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
