[ELY-2491] Unable to add integrity support to existing filesystem realm

[cam-rod]

https://issues.redhat.com/browse/ELY-2491

Not as nice of a fix as I hoped for, particularly this variable needed to stop a loop. This solution was a mix of trying to minimize excessive file opening without modifying the actual integrity validation.

Applies cleanly to 2.x branch and tested on the server.

[fjuma]

@cam-rod Thanks a lot for working on this! To simplify the logic a bit and prevent a potential bypass of the integrity check, I'm wondering if we could look into a slightly different approach. I think one option might be to make sure that when writing an identity file, we are using the latest version of the schema in order to make use of the new functionality.

Currently, when creating an identity file, we're still writing the older namespace if the newer functionality hasn't been explicitly enabled. Thus, the new principal element isn't being written. If it was being written, then I think updating a newly created FileSystemSecurityRealm using the steps from ELY-2491 would work. Note that this approach wouldn't cover pre-existing identity files created with an older WildFly Elytron version since those would still have the older schema version. However, if that's something that we decide we do want to support, then I think it would be best to handle that via the Elytron Tool as we currently do for adding encryption to pre-existing identity files.

@OndrejKotek What do you think?

[cam-rod]

I think one option might be to make sure that when writing an identity file, we are using the latest version of the schema in order to make use of the new functionality.

Yeah, this would help with some of the backwards compatibility challenges.

Note that this approach wouldn't cover pre-existing identity files created with an older WildFly Elytron version since those would still have the older schema version. However, if that's something that we decide we do want to support, then I think it would be best to handle that via the Elytron Tool as we currently do for adding encryption to pre-existing identity files.

That sounds like a good plan. The main issue with the current implementation is that it has to check the version of the identity files, which requires it to bypass the integrity check. Moving it out of Elytron means we can replace that check with a message to use the tool, and should help to reduce the chance of downgrade attacks.

[OndrejKotek]

@fjuma @cam-rod Fully agree. We want the conversion functionality (in the tool) to enable migrations from older WF/EAP versions. What about using a :migrate like operation on the resource or subsystem instead of the tool? Do we need a new RFE or can we add this to an existing one?

[fjuma]

@OndrejKotek @cam-rod Because the integrity and encryption features are optional features that currently need to be explicitly enabled, I think Elytron Tool is a better fit. If we were to take existing elytron subsystem configuration and try to migrate it, we wouldn't know from the existing configuration if integrity/encryption should be enabled by default in the migrated configuration. WDYT?

Do you think adding the integrity aspect to Elytron Tool should be considered as an RFE? Or do you think this could be handled as a bug fix since it was missed in the original RFE?

[OndrejKotek]

Because the integrity and encryption features are optional features that currently need to be explicitly enabled, I think Elytron Tool is a better fit.

This could change in WF28, see https://issues.redhat.com/browse/WFCORE-5864 (just encryption planned now).

existing elytron subsystem configuration and try to migrate it

I'm suggesting to add something like filesystem-realm files migration operation. Just thinking aloud.

But it makes sense to separate such operation to the Elytron Tool to ensure it's performed carefully "offline",

Do you think adding the integrity aspect to Elytron Tool should be considered as an RFE?

It has the character of RFE but it's quite small. Yes, we can look at this as a bug fix for the original RFE. Let's handle it as a bug fix then.

[fjuma]

Because the integrity and encryption features are optional features that currently need to be explicitly enabled, I think Elytron Tool is a better fit.

This could change in WF28, see https://issues.redhat.com/browse/WFCORE-5864 (just encryption planned now).

Right. Just to clarify a bit more, the idea would be to enable encryption by default for filesystem-realms that are included in the WildFly out of the box configuration. Encryption wouldn't be enabled by default for all newly created filesystem-realms though so that's why I think it would be tricky to have a migrate op because for any user-created filesystem-realms, we wouldn't know whether or not to enable these features.

existing elytron subsystem configuration and try to migrate it

I'm suggesting to add something like filesystem-realm files migration operation. Just thinking aloud.

But it makes sense to separate such operation to the Elytron Tool to ensure it's performed carefully "offline",

Do you think adding the integrity aspect to Elytron Tool should be considered as an RFE?

It has the character of RFE but it's quite small. Yes, we can look at this as a bug fix for the original RFE. Let's handle it as a bug fix then.

That sounds good, thanks! @cam-rod Would you be able to create an ELY issue to track this?

[cam-rod]

@fjuma sure, opened as ELY-2496

[fjuma]

Thanks for creating the ELY issue @cam-rod! I'm going to close this PR now since the remaining work will be tracked there instead.