cockpit-certificate-ensure: soften the certificate generation #21069
Conversation
Certificate generation now works in the following cases - Certificate and key files do not exist. - Only the certificate file does not exist. - Only the key file does not exist. - The certificate file is empty. - The key file is empty.
jmlemetayer
force-pushed
the
fix-cockpit-certificate-ensure
branch
from
4a8a6cc
to
e44a7b3
Compare
|
Can anyone take a look at this PR? Thanks |
| if (read_file (self->certificate_filename, &self->certificate) < 0) | ||
| return -1; | ||
|
|
||
| if (self->certificate.size == 0) |
There was a problem hiding this comment.
This feels arbitrary and too brittle. If the concern is short writes after a power loss, then "0 bytes" feels like a "lucky unlucky" case.
The normal way to avoid that is to write the file with a temp name, fsync() it, and then rename it to the final name. Your third commit aims at that, I'll comment on that separately.
On the reading side, the gnutls functions to parse the certificate should already fail (in certificate_and_key_parse_to_creds()) if the certificate is empty, or if the file is truncated, so this check here ought to be redundant?
|
|
||
| if (self->certificate.size == 0) | ||
| { | ||
| warnx ("empty: %s", self->certificate_filename); |
| @@ -314,7 +314,7 @@ static const TestCase case_bad_file = { | |||
| static const TestCase case_bad_file2 = { | |||
| .files = bad_files2, | |||
|
|
|||
| .check_stdout = "", | |||
| .check_stdout = "Unable to read*", | |||
There was a problem hiding this comment.
This is a symptom of the above "should go to stderr" comment. The stderr check already covers having a more precise error message.
| @@ -314,7 +314,7 @@ static const TestCase case_bad_file = { | |||
| static const TestCase case_bad_file2 = { | |||
There was a problem hiding this comment.
The tests need to be extended to cover the cases you mention (empty/truncated file and such). But let's sort out the above questions first.
|
Been looking into this as #21173 comes up often. (The next time I should ask if the user has valid certificates generated). So I have been trying to reproduce this issue but so far I get a usefulish error: The error is wrong but hmm can be fixed. So I wonder what code path you had which did not log an error. |
|
To aid this PR, I've split up the force sync to a separate PR #21204 and added you as Co-Authored-By. |
I use cockpit in an embedded device, and it can happen that the power is suddenly cut off. So the file system is not synchronized and the last file system actions are lost.
If generating the self-signed certificate was one of them, you end up with empty files. And the next time you reboot, cockpit won't be able to start because
cockpit-certificate-ensurewill fail.These commits prevent failure on empty files and force a new generation. In addition, we explicitly request file system synchronization after generation.