-
Notifications
You must be signed in to change notification settings - Fork 270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
packaging: Renew expiring vmconsole certificates #298
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,17 +22,20 @@ | |
|
||
from ovirt_engine_setup import constants as osetupcons | ||
from ovirt_engine_setup.engine import constants as oenginecons | ||
from ovirt_engine_setup.engine_common import pki_utils | ||
from ovirt_engine_setup.vmconsole_proxy_helper import constants as ovmpcons | ||
|
||
|
||
def _(m): | ||
return gettext.dgettext(message=m, domain='ovirt-engine-setup') | ||
|
||
|
||
def _refresh_needed(cert_path): | ||
def _refresh_needed(cert_path, check_cert=True): | ||
ca_cert_path = oenginecons.FileLocations.OVIRT_ENGINE_PKI_ENGINE_CA_CERT | ||
return (not os.path.exists(cert_path) or | ||
os.stat(ca_cert_path).st_mtime > os.stat(cert_path).st_mtime) | ||
os.stat(ca_cert_path).st_mtime > os.stat(cert_path).st_mtime or | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure we want also this check (os.stat)? It means that a mere 'touch file.cer' will trigger a renew, which while in theory is harmless, is not meaningless. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC this check might be recommended by you. The idea is that if the CA certificate is touched then it may be changed and we must renew in such a case to get the vmconsole certificate signed by the new CA certificate. If a user touches the CA certificate just for fun or so then we renew unnecessarily but yes, this is harmless. The time check is in theory not absolutely reliable but it's simple and good enough I think (or do we already have a better way?). |
||
(check_cert and | ||
pki_utils.cert_expires(pki_utils.x509_load_cert(cert_path)))) | ||
|
||
|
||
@util.export | ||
|
@@ -186,7 +189,7 @@ def _setup(self): | |
ovmpcons.ConfigEnv.VMCONSOLE_PROXY_CONFIG | ||
] and _refresh_needed( | ||
ovmpcons.FileLocations. | ||
OVIRT_ENGINE_PKI_VMCONSOLE_PROXY_HELPER_KEY | ||
OVIRT_ENGINE_PKI_VMCONSOLE_PROXY_HELPER_CERT | ||
) | ||
), | ||
) | ||
|
@@ -283,7 +286,8 @@ def _miscPKIEngine(self): | |
os.path.join( | ||
ovmpcons.FileLocations.VMCONSOLE_PKI_DIR, | ||
'proxy-ssh_host_rsa', | ||
) | ||
), | ||
check_cert=False | ||
) | ||
), | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can live with current code, but on the next patch to it I'd expect some refactoring. Currently, it's called from two places - one of them on helper cert (was on the helper .key.nopass file, current patch changes this), other on proxy-ssh_host_rsa (key file as used by the proxy (sshd), unlike the .key.nopass which is in the engine pki dir (and actually not used other than here, IIUC. I do not really know very well all this code)). Anyway, "cert_path" was a complete lie (it's a key) and now is a lie half of the time. 'check_cert' is in theory correct, only that if I got it right, it can only check a specific kind of certs (the one it's called on right now, and not /etc/pki/ovirt-vmconsole/proxy-ssh_host_rsa-cert.pub which is what is actually used). Since (IIUC) users are not expected to touch all of these files, it all should be ok as-is anyway. Perhaps add a TODO comment to clean up some mess on the next patch someone might have to do...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we will have to clean up the mess: https://bugzilla.redhat.com/2066084