[WFCORE-6384] When shuting down to perform an installation CLI is dis… #5537

yersan · 2023-06-06T17:12:31Z

…connected from DC

Jira issue: https://issues.redhat.com/browse/WFCORE-6384

Instead of comparing Paths to understand whether we are using a local CLI client instance when performing an installation, now we use a CLI client marker file. The content of this client marker file is returned by the shutdown --perform-installation management operation. If the content doesn't match, we are not using a local CLI instance. Otherwise, we use the most restrictive approach assuming assuming the CLI is local.

jfdenise · 2023-06-07T07:23:26Z

host-controller/src/main/java/org/jboss/as/host/controller/operations/HostShutdownHandler.java

+
+            // check the presence of a client marker in our server installation and if so, returns its value so the client
+            // can detect whether he was launched from the same installation dir.
+            final Path cliMarker = environment.getHomeDir().toPath().resolve("bin").resolve("cli-marker");


We should test that the Files.exist

I think we can do it without checking the existence since we can build Path objects for unknown files since Path doesn't access to the filesystem, do you agree?

yes, I agree, the exception will get thrown when creating the BufferedReader, but this exception is then catch. Can be cleaner to check the existence of the file prior to create the BufferedReader.

jfdenise · 2023-06-07T07:23:36Z

server/src/main/java/org/jboss/as/server/operations/ServerShutdownHandler.java

+            // check the presence of a client marker in our server installation and if so, returns its value so the client
+            // can detect whether he was launched from the same installation dir.
+            final Path cliMarker = environment.getHomeDir().toPath().resolve("bin").resolve("cli-marker");
+            try (BufferedReader reader = new BufferedReader(new FileReader(cliMarker.toFile()))) {


We should test that the Files.exist

If the file doesn't exist, it is captured on the catch block which is explicitly ignored

jfdenise · 2023-06-07T07:24:31Z

cli/src/main/java/org/jboss/as/cli/handlers/ShutdownHandler.java

+            if (clientMarker != null) {
+                final String jbossHome = WildFlySecurityManager.getEnvPropertyPrivileged("JBOSS_HOME", null);
+                final String clientMarkerData = WildFlySecurityManager.getPropertyPrivileged(Util.CLI_MARKER_VALUE, null);
+                if (jbossHome != null && clientMarkerData != null) {


Not sure why we need to check for the presence of JBOSS_HOME env variable?

Right, it was left over from a previous approach, we can remove the JBOSS_HOME check here

jfdenise · 2023-06-07T07:26:48Z

cli/src/main/java/org/jboss/as/cli/CommandLineMain.java

+                WildFlySecurityManager.setPropertyPrivileged(Util.CLI_MARKER_VALUE, data);
+            }
+        } catch (Throwable e) {
+            // explicitly ignore this.


Could we have a warning displayed. I imagine the case where part of the JBOSS_HOME sub directories would be readonly.

Yes, this is explicitly ignored because of the read-only use case (or any other exception).

We could log a WARN (we should move the method invocation after the one that setups the logging) but I considered it as a corner case that could go silently; if that occurs the logic will assume we are using a local CLI instance and it will get closed when the update is in place just because we won't be able to detect whether it is local.

This won't affect the update/rollback process, it is just that if your user is running on a read-only mode over the JBOSS_HOME/bin directory and you are using a non-local CLI instance your CLI instance will be closed with the aim to ensure you will be using the latest jboss-modules.jar file after the update, although for this case, it wouldn't be necessary to close it.

We can add a WARN, but, shouldn't it be just noise since it is a very particular case only related to the shutdown when performing an installation? WDYT?

Or at the very least is any logging possible? Catch blocks that drop the Throwable somehow end up being the place you need more information from when remote debugging an issue.

I agree, yes, a debug log could be useful for the future, I'm going to add it

darranl · 2023-06-07T09:16:34Z

cli/src/main/java/org/jboss/as/cli/CommandLineMain.java

+     */
+    private static void createClientMarker() {
+        try {
+            final String jbossHome = WildFlySecurityManager.getEnvPropertyPrivileged("JBOSS_HOME", null);


I am not sure we need the SecurityManager wrappers here, in this case as this is the entry point there should not be other protection domains on the call stack.

I probably incorrectly tend to use WildFlySecurityManager to ensure we can load a system property or env variables regardless of the current user permissions if, in this case, the CLI is running with the Security Manager enabled.

Is this incorrect in this case? Notice the JBOSS_HOME is exported by us in the jboss-cli scripts

Are you maybe suggesting just using a privileged block instead of the WildFlySecurityManager?

I am suggesting neither, just access the property directly.

Cannot the CLI be executed with the security manager enabled?

The security manager being possibly enabled is never a reason to add a doPrivileged.

When you add a doPrivileged call what you are effectively doing is dropping the protection domains of any callers currently on the call stack and setting up the permission check to be in the context of the protection domain of the class that is calling the method with the permissions check.

As this is a main method / entry point - what callers are being dropped from the call stack and what measures have been put in place to verify they can safely call this method.

The way these properties are being used the safety is probably fine but what callers are being dropped from the call stack?

As this is a main method / entry point - what callers are being dropped from the call stack and what measures have been put in place to verify they can safely call this method.

I just wanted to avoid the need to require specific permission to load an environment variable that is indeed configured by the launch script. We know that this is safe to do because it is the launch script the one that invokes this Main class.

The way these properties are being used the safety is probably fine but what callers are being dropped from the call stack?

Ok, so the point is that since it is a main class, it can be invoked by anyone, and in that case, if it is invoked by something that is not controlled by us (e.x. something different than our launch script) we want to ensure the caller has been granted with the required permission.

In such a case, then it is better to ensure that any caller has been granted the expected permission, including if it is invoked from our script.

Ok, I'm going to remove the privileged block and force any caller to have the expected permission since we can't ensure what is the caller of this code.

@darranl I've just noticed that in this class we are already reading/setting system properties under a privileged block to configure the logs:

https://github.com/wildfly/wildfly-core/blob/main/cli/src/main/java/org/jboss/as/cli/CommandLineMain.java#L101-L106, if we apply the same reasoning to them, those privileged blocks should be also removed.

I just wanted to avoid the need to require specific permission to load an environment variable that is indeed configured by the launch script. We know that this is safe to do because it is the launch script the one that invokes this Main class.

This is not the purpose of a doPrivileged call, it is not possible to prevent the permission check. The ProtectionDomain for the CommandLineMain class will still need the relevant permissions to be granted.

When a permission check is performed it loops through the PermissionDomains of all classes on the call stack, and each and every one of those must be granted that permission or the check will fail. What doPrivileged does is it says ignore the classes that called me but it does not get away from the "me" class being required the permission.

If this class is always intended to be called as the main entry point to the program it does not need any doPrivileged calls as it will always be the starting point of the call stack so there will be no protection domains to drop.

On the other hand if this is expected that other classes from other protection domains will call this class then this class should only be using doPrivileged if those are trusted called / it has been designed so the doPrivileged calls can not be abused.

TBH this pattern that creeps in where if there is going to be a permission check a doPrivileged is added is one of the reasons that adds to the justification of the removal of the security manager in Java.

I'm starting to understand the tricky details of the security manager when, for good reasons, it was going to be removed :)...never is late, even if it going to be an obsolete knowledge

darranl · 2023-06-07T09:16:59Z

cli/src/main/java/org/jboss/as/cli/CommandLineMain.java

+            final String jbossHome = WildFlySecurityManager.getEnvPropertyPrivileged("JBOSS_HOME", null);
+            if (jbossHome != null) {
+                final Path cliMarkerPath = Paths.get(jbossHome).resolve("bin").resolve(Util.CLI_MARKER);
+                Files.deleteIfExists(cliMarkerPath);


What happens if multiple instances of the CLI are running?

The RFE contemplates this situation where we have stated that is the user the one who has to ensure no other processes are launched when they are updating the server.

In this case, the marker value returned by the mgnt Operation won't match the current CLI instance, so it will assume it is not a local instance and won't be automatically closed.

The user will end up in the same situation as the current Patch tool, where an update/patch can update the underlying jboss-modules.jar. Since the client CLI session is not automatically closed, the user will be using an obsolete CLI process launched from a patched jboss-modules.jar. Probably he will get errors using the CLI and he will be forced to close it and open it again.

There are more sophisticated approaches but I guess out of the scope of the first release, for example, a kind of watcher service over the boss-modules.jar resource used by the CLI instance and if it is modified, close the client. Or use a kind of file lock that allows us to wait on the mgmt operation side if the client has been closed before shutdown.

yersan · 2023-06-14T08:33:23Z

@jfdenise / @darranl Could you take another look and approve if you think we can proceed as it is? Thanks!

marekkopecky · 2023-06-14T09:28:57Z

I confirm that the original issue described in https://issues.redhat.com/browse/WFCORE-6384 is fixed by this MR! Thank you!

jfdenise · 2023-06-14T09:29:52Z

cli/src/main/java/org/jboss/as/cli/CommandLineMain.java

+     */
+    private static void createClientMarker() {
+        try {
+            final String jbossHome = System.getenv("JBOSS_HOME");


@yersan when CLI is started as a fat jar (bin/client/jboss-cli-client.jar), we don't have this env set. I am wandering if we should attempt to find the jar location (class.getProtectionDomain().getCodeSource().getLocation()) and determinate if that is inside an installation?

@jfdenise We should be fine there since Installation Manager high level commands are not available for the CLI if the CLI is not running on a modular environment.

Indeed, none of the extension commands are available when running on a non-modular environment.

jfdenise · 2023-06-14T10:29:51Z

cli/src/main/java/org/jboss/as/cli/handlers/ShutdownHandler.java

                ctx.printLine("The JBoss CLI session will be closed automatically to allow the server be updated. Once the server has been restarted, you can relaunch the JBoss CLI session.", false);
                try {
                    TimeUnit.SECONDS.sleep(3);
                } catch (InterruptedException e) {
                    // Ignored
                }
-
                // We are using a CLI which was launched from the server installation we have requested to be updated.
                // In order to prevent keeping using a jboss-modules.jar that could have been updated, we finish the CLI process
                // Once the server has been restarted the user will launch again the CLI that will use the most recent updates
                ctx.terminateSession();


Shouldn't you return after this call?

I've never found an issue, but yes, looks correct returning here.

…connected from DC Jira issue: https://issues.redhat.com/browse/WFCORE-6384

darranl

Probably still needs review from @jfdenise but no objections from me.

yersan · 2023-06-14T15:24:21Z

Thanks @jfdenise / @darranl / @marekkopecky

cli/src/main/java/org/jboss/as/cli/CommandLineMain.java

yersan requested a review from jfdenise June 6, 2023 17:12

github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jun 6, 2023

jfdenise reviewed Jun 7, 2023

View reviewed changes

yersan requested a review from jfdenise June 7, 2023 08:38

darranl reviewed Jun 7, 2023

View reviewed changes

yersan force-pushed the WFCORE-6384 branch from 644e5ac to 191acc6 Compare June 8, 2023 10:39

yersan requested a review from darranl June 8, 2023 10:39

jfdenise reviewed Jun 14, 2023

View reviewed changes

[WFCORE-6384] When shuting down to perform an installation CLI is dis…

ff4cfc9

…connected from DC Jira issue: https://issues.redhat.com/browse/WFCORE-6384

yersan force-pushed the WFCORE-6384 branch from 191acc6 to ff4cfc9 Compare June 14, 2023 11:26

darranl approved these changes Jun 14, 2023

View reviewed changes

jfdenise approved these changes Jun 14, 2023

View reviewed changes

yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Jun 14, 2023

yersan merged commit 0168a28 into wildfly:main Jun 14, 2023
10 of 11 checks passed

yersan deleted the WFCORE-6384 branch June 14, 2023 15:24

jamezp reviewed Jun 14, 2023

View reviewed changes

cli/src/main/java/org/jboss/as/cli/CommandLineMain.java Show resolved Hide resolved

jamezp mentioned this pull request Jun 14, 2023

[WFCORE-6384] Do not initialize the logger before the log manager is … #5550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFCORE-6384] When shuting down to perform an installation CLI is dis… #5537

[WFCORE-6384] When shuting down to perform an installation CLI is dis… #5537

yersan commented Jun 6, 2023

jfdenise Jun 7, 2023

yersan Jun 7, 2023

jfdenise Jun 14, 2023

jfdenise Jun 7, 2023

yersan Jun 7, 2023

jfdenise Jun 7, 2023

yersan Jun 7, 2023

jfdenise Jun 7, 2023

yersan Jun 7, 2023 •

edited

darranl Jun 7, 2023

yersan Jun 7, 2023

darranl Jun 7, 2023

yersan Jun 7, 2023

yersan Jun 7, 2023

darranl Jun 7, 2023

yersan Jun 7, 2023

darranl Jun 7, 2023

yersan Jun 8, 2023

yersan Jun 8, 2023

darranl Jun 8, 2023

yersan Jun 8, 2023

darranl Jun 7, 2023

yersan Jun 7, 2023 •

edited

yersan Jun 7, 2023

yersan commented Jun 14, 2023

marekkopecky commented Jun 14, 2023

jfdenise Jun 14, 2023

yersan Jun 14, 2023 •

edited

jfdenise Jun 14, 2023

jfdenise Jun 14, 2023

yersan Jun 14, 2023

darranl left a comment

yersan commented Jun 14, 2023

[WFCORE-6384] When shuting down to perform an installation CLI is dis… #5537

[WFCORE-6384] When shuting down to perform an installation CLI is dis… #5537

Conversation

yersan commented Jun 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yersan Jun 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yersan Jun 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yersan commented Jun 14, 2023

marekkopecky commented Jun 14, 2023

Choose a reason for hiding this comment

yersan Jun 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darranl left a comment

Choose a reason for hiding this comment

yersan commented Jun 14, 2023

yersan Jun 7, 2023 •

edited

yersan Jun 7, 2023 •

edited

yersan Jun 14, 2023 •

edited