packaging: Renew expiring vmconsole certificates #298
Conversation
| 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.
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.
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?).
| 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): |
There was a problem hiding this comment.
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.
Yes, we will have to clean up the mess: https://bugzilla.redhat.com/2066084
Yeah, if the code is correct then let's get it in and address possible improvements/refactoring separately |
In commit 35e8f51, a check was added to renew vmconsole certificates when the CA certificate is newer. But the certificates are still not renewed if they are expired or close to being expired. Let’s fix it by additionally checking for certificate expiration. In order to that, we must check the helper certificate file instead of key. Bug-Url: https://bugzilla.redhat.com/1988496
Thank you for review, I'll address your comments within the other bug. |
In commit 35e8f51, a check was added
to renew vmconsole certificates when the CA certificate is newer. But
the certificates are still not renewed if they are expired or close to
being expired. Let’s fix it by additionally checking for certificate
expiration.
In order to that, we must check the helper certificate file instead of
key.
Bug-Url: https://bugzilla.redhat.com/1988496