Prevent chains in authorization_realms

[tvernum]

If realm "A" delegates authoriaation to realm "B" then it is not
permissible for realm "B" to also be using delegated authorization.
A realm which is in the value for "authorization_realms" must handle
its own authorization.

[elasticmachine]

Pinging @elastic/es-security

[bizybot]

nit: rename realms to say resolvedLookupRealms

[bizybot]

nit: rename this method name to describe what it checks, would also rename realms as that confused me while reading the code.

[bizybot]

LGTM, except few nits. Thank you.

[albertzaharovits]

This might come out as a "drive by comment", but it's 💯 legit:

I am thinking to have two realm chains, grouping realms in two chains, one chain handling the user name verification (authc realm chain) and the other doing the user attributes lookup (authz realm chain).
In this way we acknowledge the segregation existing in the realm chain, which became apparent in this PR.
Specifically, realms in the authc realm chain can have the delegate flag setting enabled and realms in the authz realm chain cannot enable this setting. Moreover, we could disallow SAML, PKI and Kerberos realm types in the authz realm chain.

WDYT Tim and team ?

[tvernum]

In this way we acknowledge the segregation existing in the realm chain, which became apparent in this PR.

If I understand your proposal correctly, then the problems are

1. that it explicitly forces the realm to be exclusively lookup, or not at all.
2. that it assumes that all delegating realms will want to use the same realm for lookups (maybe this isn't what you mean)

On 1, that's an issue for two reasons:
a) We're explicitly trying to make this feature work as much like run-as as we can so it has a low maintenance cost & cognitive overhead. And run-as doesn't so that. I think many of us would love to change our authc/realm impl as part of a major release to remove some of the weird idiosyncrasies (like the reserved realm forcing basic auth to have top priority regardless of the chain order), but I dont see us getting time for that any time soon. So we cant change run-as, and we'd really prefer to have a single "this is how we do lookups" process.
b) That's actually not a helpful behaviour for the end-user. Think Kerberos, since that was the primary driver for implementing this now. Many Kerberos realms will exist simply to allow browser SSO, and will delegate to AD (or other LDAP). But in a non-browser environment, they will probably want to do Basic auth with user/pass against that AD realm. It is both authc + authz.

Moreover, we could disallow SAML, PKI and Kerberos realm types in the authz realm chain.

I'd like to do this (in the general case, regardless of whether we have separate chains) but we removed the userLookupSupported method in 6.x so we'd need to bring it back again.

[tvernum]

@albertzaharovits If you have a strong view that we should take a step back and redesign our approach to authorization_realms, then let's discuss that.

We expect this to be a relative niche feature (perhaps more common in Kerberos, but not very common elsewhere), so we're trying to balance:

• making it relatively flexible/featured
• building on top of existing features so it has a low maintenance cost (etc).
• having it make sense to customers who need it
• not confusing customers who don't use it
• while also releasing it in a minor (BWC).

If we're going down the wrong path, let's talk - we've got time to revisit this before we merge to master, but I don't think this PR is going to be the best place for that discussion.

[albertzaharovits]

Alright @tvernum, when you lay it like this, I agree another realm chain in unwarranted.
The ideea with another realm chain sprouted up when I realized: 1) we assume all realms will use the same realm for lookup when checking for the cycle, and 2) that realms with the authorization_realms setting can work as authenticating realms only . So there you have it, realms that work in a single function and that don't allow for a certain setting. However, it is true that, in the common case, when realms don't have the authorization_realms setting, they can be present in both the proposed realm chains.

If I understand it correctly, a cycle in the configuration will still function correctly? I have checked the code for LdapRealm and I think it does? Do we expect it may not work for other realms? Is it is only a precaution to guard against unintended misconfiguration? If yes, then I am not convinced we should add this check.

Thanks for digging through my vagueness.

[tvernum]

we assume all realms will use the same realm for lookup when checking for the cycle

I don't understand. The check simply validates that any realm that is configured as an authorization_realm cannot also have authorization_realm set on it. There's no assumption that these lists are common across realms.

If I understand it correctly, a cycle in the configuration will still function correctly?
Define correctly. It will behave very strangely, but it will "work", maybe.

Without this check you could have (for example) ldap1 delegating to ldap2 and ldap2 delegating to ldap3.
Should the User objects for ldap1 be resolved from ldap2 or ldap3? Neither answer is sensible. The only sensible answer is don't do that.

[albertzaharovits]

Okay, thank you for the clarification and example!

It would all have been simpler if I would have started with an example myself. Using yours, I think User object from ldap1 is expected to be resolved by ldap2 which is the present behavior. But this is probably my narrow view from the inside.

[albertzaharovits]

LGTM

[tvernum]

I think User object from ldap1 is expected to be resolved by ldap2 which is the present behavior

That is the current behaviour, and it's probably the more logical behaviour too, but it's still confusing, hence this PR.