Make order setting mandatory for Realm config

[ywangd]

The order parameter must be explicitly specified for each realm.
This is a breaking change and will start take effect in 8.0

Resolves: #37614

[ywangd]

@tvernum This PR is still WIP. But I'd like to get a PR in early its easier for you to review and set me on the right track.

Current changes are focused on making the order parameter mandatory and fixing relevant tests. Others things remaining are:

• No two realm should have the same order
• Warning in 7.x
• Docs update
• Breaking change update

[ywangd]

@elasticmachine update branch

[ywangd]

@elasticmachine run elasticsearch-ci/2

[tvernum]

I think it would be better for this constructor to be protected, and update the tests to pass through real Settings object with an order config.

I know that's going to be a pain - so please feel free to convince me if you feel otherwise.

[ywangd]

Are you suggesting the caller should create a Settings object that actually includes an order? In this case, this constructor is no longer necessary? Am I getting this right?

[ywangd]

I initially actual prefer to not adding another constructor. But the number of call site changes made me change my mind. But I am happy to drop the constructor and make more call site changes if you favour this approach. Please let me know. Thanks

[ywangd]

@tvernum I plan to go ahead with following changes. Please let me know if this works for you:

• Drop the new constructor. The only constructor will mandate that the Settings object has an order setting.
• Change relevant call sites in tests to always add order into Settings.
  Personally I think above approach works better since it does not change production code to cater tests.

To ensure no two realms having the same order, it seems that some logic needs to be added in Security.java. But shall we enforce this requirement to builtin realms (file, index) that are added automatically? Currently they are both added with default order of Integer.MAX_VALUE. It does not seem to be very meaningful to have different order values for them.

[ywangd]

Just realised that the Settings object is also referenced from the Environment object. The following code snippet shows how we add default file realm:

realms.add(fileRealm.create(new RealmConfig(
      new RealmConfig.RealmIdentifier(FileRealmSettings.TYPE, "default_" + FileRealmSettings.TYPE),
      settings, env, threadContext)));

To add the order setting, above code would change to something like the followings:

var realmIdentifier = new RealmConfig.RealmIdentifier(FileRealmSettings.TYPE, "default_" + FileRealmSettings.TYPE);
realms.add(fileRealm.create(new RealmConfig(
      realmIdentifier,
     Settings.builder().put(settings)
           .put(RealmSettings.realmSettingPrefix(realmIdentifier) + "order", Integer.MIN_VALUE).build(),
     env, threadContext)));

Note that a new Settings object is created with the addition of order setting. But the env object still references old Settings object. Will this create a problem somewhere else in the code path?

[ywangd]

@tvernum I would like to change my view on this again ... Sorry for the back and forth. I kept the new constructor but changed the order parameter to be of a primitive int type. The Constructor remains public as it is used by Realms and ReservedRealm.

A few reasons to go with this approach:

• The mandatory order setting is not 100% strict, because native realms can be added without any settings. So currently it feels we have an escape hatch for this rule, which seems to make sense for another constructor.
• When default native realms are added, they don't alter the existing Settings object. I'd like to follow this pattern.
• The Settings object is referenced in multiple places, Realms, Environment, every realm and likely more. I am unsure what would be the consequence if they each reference a different Settings object (mostly the same content, but distinct object).
• This approach is likely more extensible if we ever wanna make another setting mandatory or some other requirement, e.g. it could refactor to replace the int parameter with a value provider.

Other changes I have pushed are:

• Test file for RealmConfig - Please let me know whether they are idomatic in elasticsearch's term.
• Enforce unique order for each realm in Realms - Note this is only applied to user configure realms. I feel this does its job without worring about orders of default native realms (Integer.MIN_VALUE).
• Updated tests for Realms

[ywangd]

Tried to identify all places in docs that are relevant to the order setting. To my surprise, not many places need update. Here is a list of updated files for review convenience:

• security.asciidoc
• security-settings.asciidoc
• configuring-ldap-realm.asciidoc
• configuring-pki-realm.asciidoc

[ywangd]

@elasticmachine update branch

[elasticmachine]

Pinging @elastic/es-security (:Security/Authentication)

[albertzaharovits]

I've done a first pass.
I appreciate the effort involved, but it's one of those cases where I would've made a different choice.

But I think this works and I'll LGTM when I get to look a second time.

[albertzaharovits]

I would've probably made a different choice of where to place this check because we pass RealmConfigs around in too many places. For example, this:

• will throw an exception in InternalRealms.getBootstrapChecks so the stacktrace for it is funky.
• will throw an exception in the SamlMetadataCommand which is not nice, although it might not happen in practice too often.

I would've probably removed the order and enabled local vars and do the checks in Realms#initRealms. But there might be subtle problems in that case as well.

Nevertheless, this does the job, and looks OK, to me.

Nit: we use the reverse form for negative condition, i.e. false == <function> (unless we are in some few older files that use a different convention in which case we blend with the background).

[ywangd]

I am also tempted to move the logic into Reamls#initRealms (see below) comments for details. But your examples show that by itself is not sufficient. Both of the two examples are not covered if the logic is just in Realms#initRealms, while they do get covered if the logic is in RealmConfig. I am now again unsure about what would be the better approach.

[albertzaharovits]

Maybe I've not made myself clear

will throw an exception in InternalRealms.getBootstrapChecks so the stacktrace for it is funky.

I mean that there is a minor inconvenience that the validation that the order parameter is defined is performed inside a different code than the code that deals with realm construction.

[albertzaharovits]

Not to dwell on it too much, but just because we need this new constructor is a smell. In this case, the order local var is sometimes the value from the settings, and sometimes a different one passed by this constructor. I know this is only used in tests, but it's still not nice to have to deal with it (Integer.MAX_VALUE).

[ywangd]

The new constructor is mostly used by test code, but it is also used by production code, see Realms and ReservedRealm.

I also think adding a new constructor is not ideal due to some of the concerns you mention. It however seems to be a better choice comparing to the other choice, i.e. Add order into the settings parameter on caller side so that it can just keep using the existing constructor.

This approach should work fine for tests. But in production code, the settings object is referenced in many other classes, e.g. Environment, Realms, etc. So when adding FileRealm (in Realms.java), if we modify the settings object to add order, we will need to do something like the follows:

var newSettings = Settings.Builder().put(settings).build();
... new RealmConfig(..., newSetings, ...), ...;

So the newSettings is now passed into RealmConfig which will store a reference to it. Now we have two Settings object across the application. They are mostly identical, only differs in the order parameter. And this is just FileRealm, the same applies to native realm, for which we will have a third settings. I personally think this is worse than adding a new constructor. That is why the new constructor is added so we can avoid manipulating the settings object.

With above being said, I realise it is making an assumption that the logic for mandating the order setting resides in the RealmConfig class. If we can move away from this assumption, like you suggested, enforce the logic in Realms.initRealms, we can probably make the change set much smaller.

Initially I thought about adding the logic into Security but that was too high level. So we decided to add it in RealmConfig itself, where it is low level the logic is close to its natural owner. But maybe we need something in between. Realms could be the right candidate especially it is where the order uniqueness is enforced.

[albertzaharovits]

My choice would also be to only inspect the order setting inRealms#initRealms (where the order value duplication is checked). That's because the order is only really used in that place, specifying the initial order in the realm chain (which would then change as we reorder the chain during authentications), so it's not suitable as a final local variable.
This way we would also avoid having to specify the order parameter for the "default" realms (file and native) which is simply ignored anyways.

Your approach works as well and I can't argue strongly against it, so this LGTM to me even though I would've approached it differently.

[ywangd]

Following discussion with Tim, I have made the following changes:

• Removed the new constructor of RealmConfig
• Still kept the logic of checking mandatory order setting in RealmConfig. I think this class is still overall the better place for enforcing the check because it is the choke point for many different upstream use cases, including some command line tools. So we will have a consistent report on the missing order setting no matter what the entry point is.
• Since RealmConfig no longer has a special constructor for handling missing order setting, it is now the caller's responsibility to ensure this setting exists for reserved, file and native realms (when they are added automatically). I am aware that this approach creates new settings object along the way. This is however an existing issue in other places. A separate issue (Move configuration of special realms to additionalSettings #51387) has been created to tackle it.
• The approach to ensure uniqueness of realm orders remains the same (i.e. in Realms#initRealms)
• Again most of the changes are for test classes (making sure each of them has an order setting configured).

[albertzaharovits]

LGTM, thank you Yang!

[albertzaharovits]

This last phrase doesn't make sense, I would avoid describing again what order does.

[ywangd]

Will do thanks

[albertzaharovits]

I would remove

If you are configuring multiple realms, you must ...

altogether.

[ywangd]

Somehow missed this occurrence ... Will remove. Thanks

[albertzaharovits]

@ywangd I just remembered, we need to follow-up this PR with a another one that adds a deprecation check in https://github.com/elastic/elasticsearch/blob/7.x/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java . This should inform users that realm definition sections without the order parameter are invalid. The PR should be based against the 7.x branch (these checks do not exist on master).

[ywangd]

@albertzaharovits Warning/Deprecation message for 7.x is on my to-do list. Thanks for pointing me to the right file to start with.

[tvernum]

LGTM