Don't consider an empty client.keys to be a failure condition

[chewi]

client.keys is already reloaded each time a given key is not found in memory so there's no harm in this file being empty. In fact, it's downright annoying if you're using authd because you have to wait for the first agent to register and then manually restart the server before they can start communicating. Removing this check would make the Chef cookbook less clunky.

Disclaimer: I haven't tested this at all because I've already sunk too much time into the cookbook. The change seems simple enough though.

[ddpbsd]

Can someone test this? I think it's a great change, but want to make sure it works first. (adding it to my list of things to do when I can make time)

[reyjrar]

I think this may break some stuff. This function is called by the agent and the server. I agree, on the server, this shouldn't be fatal, but I'm not so sure on the agent if this would fly.

[chewi]

I thought about sticking ifdef CLIENT around it but I see that agentd is built for TARGET=server as well. It would somehow need to determine whether it is an agent or server process at runtime.

[chewi]

It's not fantastic but since ARGV0 is hardcoded, how about this?

if (strcmp(__local_name, "ossec-remoted") != 0) {
  …
}

[chewi]

Rebased.

[chewi]

I finally gave this a try myself and found it segfaulted because keys->keyentries wasn't initialised. I've reworked it and now ossec-remoted at least doesn't crash any more.

[chewi]

I've now also got it to create client.keys on startup like authd does. This has to happen before dropping privileges because ossecr should not have write access. The file permissions could still be tightened up (currently root:root and 644) but authd doesn't make any effort to do this and the directory permissions at least prevent world read access. One for later perhaps.

[chewi]

Any chance of getting this merged now? I haven't seen any ill effects since and yet another person asked about this problem on the mailing list.

[dcid]

It looks good now, except for this function:

__realloc

Really confusing to use a standard C function name for this. Can you change to something more adequate to what it does? (allocate key mem)

[chewi]

Fair enough, renamed it to __realloc_keys.

[jrossi]

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

src/os_crypto/shared/keys.c, line 262 [r1] (raw file):
I dislike this for some reason. This does not mean it's wrong just something feels wrong that this is needed.

---

Comments from the review on Reviewable.io

[jrossi]

Sorry i am taking so long need to review some more things within the code base.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from the review on Reviewable.io

[chewi]

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

src/os_crypto/shared/keys.c, line 262 [r1] (raw file):
Believe me, I tried to think of a better way but couldn't find one. You're welcome to try.

---

Comments from the review on Reviewable.io

[chewi]

I'm concerned that 2.9 may get released without this and that would be very frustrating. The code smell mentioned above isn't a showstopper and could always be improved later.