Icinga DB: Config no_user_modify, Extract ValidateCertPath, and Support Redis username authentication

[oxzi]

Icinga DB: Config no_user_modify, Extract ValidateCertPath

Each configuration field of an IcingaDB Object was marked with no_user_modify as modifications via the API would not result in an actual change. While the Object would be updated, the internal Redis connection would not be restarted, resulting in an unexpected behavior.

The validation for cert_path and key_path was extracted from the generic Validate method to an own ValidateCertPath method.

The missing db_index was added to the documentation.

Icinga DB: Support Redis username authentication

The Redis ACL system was introduced with Redis 6.0. It introduced users with precisely granular permissions. This change allows Icinga 2 to use the Icinga DB feature against a Redis with an ACL user.

This was reflected in the documentation, next to the already implemented, but undocumented Redis database.

Closes #9536.

[oxzi]

Rebased after the last two CI fixes were merged.

[yhabteab]

Actually, I don't think this is the right place to perform such a constraint and even the existing certificate path validation. The IcingaDB::Validate() method should validate the object as a whole, initialising the environment ID for example, not such a specific attribute validity check. Instead, you can put this in its own method by overriding the IcingaDB::ValidateUsername() method.

[yhabteab]

Like this for example:

icinga2/lib/icingadb/icingadb.cpp

Lines 217 to 224 in 8beb0b7

	void IcingaDB::ValidateConnectTimeout(const Lazy<double>& lvalue, const ValidationUtils& utils)
	{
	ObjectImpl<IcingaDB>::ValidateConnectTimeout(lvalue, utils);
	if (lvalue() <= 0) {
	BOOST_THROW_EXCEPTION(ValidationError(this, { "connect_timeout" }, "Value must be greater than 0."));
	}
	}

[julianbrost]

Instead, you can put this in its own method by overriding the IcingaDB::ValidateUsername() method.

Aren't those for checking the values of individual fields in isolation?

The IcingaDB::Validate() method should validate the object as a whole, initialising the environment ID for example

Initializing something in a validate method sounds more like a hack to me.

[oxzi]

I thought this was the right place due to the fact that the TLS verification just follows. But I can change it to a more specific validation method.

[yhabteab]

Aren't those for checking the values of individual fields in isolation?

As far as I'm concerned, ValidateUsername() should fail as well if this constraint isn't met, but I see that a few other types also perform this kind of validation in the base method, so just as you like it!

[julianbrost]

I'm not sure if this would even work: Would GetPassword() within IcingaDB::ValidateUsername() even return a value yet? Would it depend on the order the attributes are set within the object definition?

[yhabteab]

I'm not sure if this would even work: Would GetPassword() within IcingaDB::ValidateUsername() even return a value yet?

lvalue is literally a wrapper around a plain GetFoo() method call 😅.

	if (2 & types)
		ValidateUsername(Lazy<String>([this]() { return GetUsername(); }), utils);

Would it depend on the order the attributes are set within the object definition?

No, the attributes are all initialised at the validation stage.

[yhabteab]

As it turns out, the individual validation methods are also called when modifying the attributes via the API:

icinga2/lib/base/configobject.cpp

Lines 193 to 194 in 3ffec77

	ModAttrValidationUtils utils;
	ValidateField(fid, Lazy<Value>{newValue}, utils);

Since the password field is set to no_user_modify, meaning that it isn't modifiable via the API but with the current state of this PR, that sanity check wouldn't actually be triggered when changing/setting the username via the API.

However, looking at the current Icinga DB code, it doesn't seem to make sense to me to make the config attributes runtime modifiable at all, as once Icinga DB is started, it never uses any of those, i.e. you can e.g. change the Redis port at runtime, but Icinga DB won't notice that change. Therefore, I would just mark the username to no_user_modify as well to disallow changes at runtime.

[oxzi]

I am currently at reworking this, thus I would include your comment as well. Thanks for taking another look, @yhabteab.

[oxzi]

I force pushed some changes according to @yhabteab's comments, which can be verified via the API as follows:

• Trying to create an IcingaDB object with a username but without a password:

$ curl -k -s -S -u root:icinga -H 'Accept: application/json' -X PUT 'https://localhost:5665/v1/objects/IcingaDBs/test' -d '{"attrs": {"username": "foo"}, "pretty": true}'
{
    "results": [
        {
            "code": 500,
            "errors": [
                "Error: Validation failed for object 'test' of type 'IcingaDB'; Attribute 'username': If a username is given, a password is also required. However, a password can exist on its own.\nLocation: in /var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf: 2:2-2:17\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(1): object IcingaDB \"test\" {\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(2):  username = \"foo\"\n                                                                                                        ^^^^^^^^^^^^^^^^\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(3):  version = 1726755773.422595\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(4):  zone = \"master\"\n"
            ],
            "status": "Object could not be created."
        }
    ]
}

• Trying to create an IcingaDB object with a faulty user certificate config (only cert_path being set):

$ curl -k -s -S -u root:icinga -H 'Accept: application/json' -X PUT 'https://localhost:5665/v1/objects/IcingaDBs/test' -d '{"attrs": {"enable_tls": true, "cert_path": "/foo"}, "pretty": true}'
{
    "results": [
        {
            "code": 500,
            "errors": [
                "Error: Validation failed for object 'test' of type 'IcingaDB'; Attribute 'cert_path': Either both a client certificate (cert_path) and its private key (key_path) or none of them must be given.\nLocation: in /var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf: 2:2-2:19\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(1): object IcingaDB \"test\" {\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(2):  cert_path = \"/foo\"\n                                                                                                        ^^^^^^^^^^^^^^^^^^\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(3):  enable_tls = true\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(4):  version = 1726755898.696290\n"
            ],
            "status": "Object could not be created."
        }
    ]
}

• Trying to create an IcingaDB object with a faulty user certificate config (only key_path being set):

$ curl -k -s -S -u root:icinga -H 'Accept: application/json' -X PUT 'https://localhost:5665/v1/objects/IcingaDBs/test' -d '{"attrs": {"enable_tls": true, "key_path": "/foo"}, "pretty": true}'
{
    "results": [
        {
            "code": 500,
            "errors": [
                "Error: Validation failed for object 'test' of type 'IcingaDB'; Attribute 'cert_path': Either both a client certificate (cert_path) and its private key (key_path) or none of them must be given.\nLocation: in /var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf: 1:0-1:21\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(1): object IcingaDB \"test\" {\n                                                                                                       ^^^^^^^^^^^^^^^^^^^^^^\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(2):  enable_tls = true\n/var/lib/icinga2/api/packages/_api/c9ef1947-e1ff-47f1-9afa-385ad39c88a2/conf.d/icingadbs/test.conf(3):  key_path = \"/foo\"\n"
            ],
            "status": "Object could not be created."
        }
    ]
}

• Successfully creating an IcingaDB object with username and password:

$ curl -k -s -S -u root:icinga -H 'Accept: application/json' -X PUT 'https://localhost:5665/v1/objects/IcingaDBs/test' -d '{"attrs": {"username": "foo", "password": "bar"}, "pretty": true}'
{
    "results": [
        {
            "code": 200,
            "status": "Object was created"
        }
    ]
}

• Trying to change the username of the just created IcingaDB object, which should fail now:

$ curl -k -s -S -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/objects/IcingaDBs/test' -d '{"attrs": {"username": "change"}, "pretty": true}'

{
    "results": [
        {
            "code": 500,
            "name": "test",
            "status": "Attribute 'username' could not be set: Error: Attribute cannot be modified.\n",
            "type": "IcingaDB"
        }
    ]
}

[Al2Klimov]

Instead, you can put this in its own method by overriding the IcingaDB::ValidateUsername() method.

Aren't those for checking the values of individual fields in isolation?

_{🎵That is true, hackers, that is true..🎵}

Especially here, three attrs are involved!

[oxzi]

You are referring to @julianbrost's question from above. @yhabteab wrote in a subsequent comment that this is not the case and one can reference other fields there as well.

I thought the consensus was to move the combined checks to their own methods.

But since I am just reading the Free Software Song out of your comment, please feel free to elaborate.

[Al2Klimov]

I have no doubt that it works to validate the combination of three attributes in a validator of a single attribute. I just think the referenced code belongs where it was – in the validator of the object as whole.

Same for the username-password thing below. Especially if you can't change attributes at runtime, why to validate the constraints this way?

[yhabteab]

Especially if you can't change attributes at runtime, why to validate the constraints this way?

Honestly, I don't quite get it! The ValidateAttributeFoo exists to exactly validate the attribute foo, and that's exactly what this PR does. Forget whether the attribute is mutable or not for now, why don't you want to perform such checks in the individual validators? But instead you want to decide where to perform the checks based on whether they are runtime modifiable or not?

[Al2Klimov]

Ok, forget about runtime modification.

Validation methods named after fields are "for checking the values of individual fields in isolation". What's done here is validation of a combination of them, which belongs in the central method.

What you're gonna do here, admittedly, works (lvalue is just a Get(), etc.) – but what for?

[oxzi]

But you are still against using field-specific validation methods to perform validations of multiple fields?

[Al2Klimov]

TL;DR: Exactly! They simply don't belong there. I, as a dev, would expect e.g the referenced code (TLS) where it was before. That this works at all is rather by accident IMAO.

<walloftext reading="optional">Sure, there's no_user_modify now. But it has nothing to do with the rest. And, one nice day, we could decide to just let users modify them (for the next reload) or even make them working on the next connection (#10102 (comment)). Then no_user_modify would disappear, but the validation of the combination of three attrs would be still pinned to one of them. So I could bypass the validator by choosing an attribute w/o the validator. Or, if that's too hypothetical for you, imagine instead that no_user_modify and username were two separate PRs – not depending on each other of course. Same validation weakness.
</walloftext>

[oxzi]

Either you or I is messing something up. In my understanding, there is no direct link between the validation methods and the no_user_modify attribute. The validation methods are being called to verify the state on creation and on further changes. The no_user_modify attribute, however, just forbids updating the field during runtime.

[Al2Klimov]

Yes, no_user_modify just prevents runtime-updating attrs which, if not prevented, would also validate individual new values. (ConfigObject#ModifyAttribute() calls ValidateField()!)

[julianbrost]

Let me try to summarize where all of this is coming from.

First of all, runtime object modification is quite naive: if you send a request modifying multiple attributes of an object, they are just set one after another in a loop:

icinga2/lib/remote/modifyobjecthandler.cpp

Lines 141 to 144 in b6b1506

	for (const Dictionary::Pair& kv : attrs) {
	key = kv.first;
	obj->ModifyAttribute(kv.first, kv.second);
	}

ConfigObject::ModifyAttribute() just calls ValidateField() for each field individually with the new value before actually modifying it:

icinga2/lib/base/configobject.cpp

Line 194 in b6b1506

ValidateField(fid, Lazy<Value>{newValue}, utils);

Note, that there is no call to the whole-object Validate() method, so the individual ValideteXYZ() methods that's all that's called for runtime objects. That's probably why this PR got so focused on no_user_modify: for the initial config loading, functionality-wise it makes no difference whether the check is performed in Validate() or ValidateXYZ(), as both are called at the same time anyways.

So I believe that's why @yhabteab suggested to put the validation code into ValidateUsername(): that way, the validation would still be run for runtime updates. However, if that's the intention, to do it properly, you'd also have to perform the very same check inside ValidatePassword() as updating that field could also introduce a violation of constraint over both username and password. And even when this would be done, the IcingaDB object type is a great example showing a new problem this would introduce: either both or none of cert_path and key_path can be set, so with the code updating attribute by attribute, you can't go between both or none set, you could only update already set paths to new paths (well, actually the way the check currently works, you could disable TLS first and do the changes).

Thus, no_user_modify is basically the workaround for "we can't properly validate multi-attribute constraints for runtime updates".

I have a slight tendency towards just keeping the multi-attribute checks in Validate() and prevent violating these constraints by forbidding runtime updates on the involved attributes. At least that was my intuition where these checks should be and while I think I understood the idea why they could be placed in ValidateXYZ(), that seems to either introduce some strange asymmetry (why check the combination of username and password only in ValidateUsername() but not ValidatePassword()?) or duplication of the check for questionable benefit, i.e. removing no_user_modify still sounds questionable, even when only taking the validation into account (ignoring that the Icinga DB feature would ignore the new values at runtime anyways).

[Al2Klimov]

Here there are only two. But still I could update (w/o no_user_modify) any of them causing this check to fail:

• Set username while there's no password
• Unset password while there's a username

If you insist, put this in ValidatePassword() at least IMAO.

[yhabteab]

But still I could update (w/o no_user_modify) any of them causing this check to fail:

I'm having a hard time understanding what you're trying to imply as to what the problem is, first of all, what does w/o no_user_modify mean? Did you remove the no_user_modify restriction? The password attribute is never supposed to be changed at runtime, nor is username. So, if you remove this restriction, what do you expect to happen then?