Enable cert healthcheck #4590
Enable cert healthcheck #4590
Conversation
There was a problem hiding this comment.
IIUC this test will now be basically comparing the cert data in the NSS database against itself which is redundant. There's already some other tests (called selftests) that will check the cert validity, trust flags, etc. and they will run when the server is started. Since those properties do not change unless the cert is updated I don't think they need to be constantly checked. So I think this healthcheck test can be removed. No need to check the CSR either since it's not essential for the server to run.
|
|
||
| # get cert info from NSS database | ||
| cert_nssdb = subsystem.get_nssdb_cert_info(cert_tag) | ||
|
|
||
| if cert_nssdb: | ||
| cert.update(cert_nssdb) | ||
|
|
There was a problem hiding this comment.
I think the cert_nssdb variable name is confusing because here it's a dictionary object, then in line 54 it changes into binary cert data.
|
So did we determine that the cert value, stored in CS.cfg, or elsewhere is significant at all? Does it tell us anything about the deployment? If not then I think this check can be dropped altogether. |
Since certs will not be stored in CS.cfg in the future this test is useless so it is removed.
fmarco76
force-pushed
the
ModifySysCertHealthCheck
branch
from
ef796a5 to
6a9ada9
Compare
|
OK, since all agree I have fully removed the test. |
|
Kudos, SonarCloud Quality Gate passed!
|
|
@edewata Thanks! |
Cert healtcheck was disabled while the system certificate storage was modified to avoid double copy of the certificate in
CS.cfgand NSS DB (PRs: #4588 and #4584).The check is modified to work again but actually it verify that the certs used inside the subsystem are the one in NSSDB. This is not a valuable check IMHO so it could be totally removed or modified to add extra checks. As an example it could integrate checks for certificate validity, presence of CSR (this is not mandatory, a subsystem works correctly without CSR so not sure if it make sense to check).
In my opinion we can leave this check as it is now (or remove if it is considered useless). @edewata @rcritten what do you think?