You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@karaf.apache.org by "jbonofre (via GitHub)" <gi...@apache.org> on 2026/07/07 14:50:04 UTC
[PR] Fix flaky SSH security integration tests on Windows CI [karaf]
jbonofre opened a new pull request, #2764:
URL: https://github.com/apache/karaf/pull/2764
## Problem
The SSH command security integration tests (`ShellCommandSecurityTest`, `JaasSshCommandSecurityTest`, and the other `SshCommandTestBase` subclasses) intermittently fail on the Windows CI job with errors like:
```
java.lang.AssertionError: Should contain 'Command not found':
~
view3643776057000_1@root()> she
```
The captured output is truncated to a partial command echo (`she` instead of `shell:nano`), so the assertion fails. This is a well-known flake on the `test (windows-latest)` job (it also hits `main` independently of any given PR).
## Root cause
`SshCommandTestBase.assertCommand()` wrote the command to the SSH channel and then immediately sent `logout` in `closeSshChannel()` to tear down the session. On slower runners — in particular Windows — the session could be closed while the command output was still in flight, so only a partial, truncated output was captured before the channel was read.
## Fix
Introduce a `writeCommandAndWait()` helper that appends a sentinel `echo <marker>` command after the command under test and blocks until the unique marker appears in the captured output before closing the channel.
Because the remote shell reads and executes its input line by line, the marker cannot appear before the command under test has been fully executed and flushed back to the client. This provides a deterministic completion signal that also works for the `OK` case, which has no positive marker of its own. `echo` is a gogo built-in that is not restricted by any command ACL, so it is safe for every test user.
The helper is also used by `addUsers()`/`addViewer()` so the JAAS users are fully created before the test logs in as them.
## Testing
`ShellCommandSecurityTest` passes locally on Temurin JDK 17 (matching the CI runtime):
```
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 8.257 s -- in org.apache.karaf.itests.ssh.ShellCommandSecurityTest
BUILD SUCCESS
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky SSH security integration tests on Windows CI [karaf]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2764:
URL: https://github.com/apache/karaf/pull/2764#discussion_r3537781200
##########
itests/test/src/test/java/org/apache/karaf/itests/ssh/SshCommandTestBase.java:
##########
@@ -52,41 +54,67 @@ enum Result { OK, NOT_FOUND, NO_CREDENTIALS }
void addUsers(String manageruser, String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
Review Comment:
`writeCommandAndWait()` polls `out.toString()` while the SSHD client is concurrently writing to the same `ByteArrayOutputStream` (via `channel.setOut(out)`). `ByteArrayOutputStream` is not thread-safe, so concurrent reads/writes can intermittently throw (e.g., `StringIndexOutOfBoundsException`) or yield corrupted reads, reintroducing flakiness. Consider using a synchronized `ByteArrayOutputStream` implementation for all outputs passed to `openSshChannel()` when `writeCommandAndWait()` is used.
##########
itests/test/src/test/java/org/apache/karaf/itests/ssh/SshCommandTestBase.java:
##########
@@ -52,41 +54,67 @@ enum Result { OK, NOT_FOUND, NO_CREDENTIALS }
void addUsers(String manageruser, String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
+ ";jaas:user-add " + manageruser + " " + manageruser
+ ";jaas:role-add " + manageruser + " manager"
+ ";jaas:role-add " + manageruser + " viewer"
+ ";jaas:role-add " + manageruser + " ssh"
+ ";jaas:user-add " + vieweruser + " " + vieweruser
+ ";jaas:role-add " + vieweruser + " viewer"
+ ";jaas:role-add " + vieweruser + " ssh"
- + ";jaas:update;jaas:realm-manage --realm=karaf;jaas:user-list\n").getBytes());
- pipe.flush();
+ + ";jaas:update;jaas:realm-manage --realm=karaf;jaas:user-list");
closeSshChannel(pipe);
System.out.println(new String(out.toByteArray()));
}
void addViewer(String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
Review Comment:
`addViewer()` also passes a non-thread-safe `ByteArrayOutputStream` into `openSshChannel()`, but `writeCommandAndWait()` now reads it concurrently while the SSH channel is still writing. Use the same synchronized output stream here to avoid intermittent concurrency issues.
##########
itests/test/src/test/java/org/apache/karaf/itests/ssh/SshCommandTestBase.java:
##########
@@ -52,41 +54,67 @@ enum Result { OK, NOT_FOUND, NO_CREDENTIALS }
void addUsers(String manageruser, String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
+ ";jaas:user-add " + manageruser + " " + manageruser
+ ";jaas:role-add " + manageruser + " manager"
+ ";jaas:role-add " + manageruser + " viewer"
+ ";jaas:role-add " + manageruser + " ssh"
+ ";jaas:user-add " + vieweruser + " " + vieweruser
+ ";jaas:role-add " + vieweruser + " viewer"
+ ";jaas:role-add " + vieweruser + " ssh"
- + ";jaas:update;jaas:realm-manage --realm=karaf;jaas:user-list\n").getBytes());
- pipe.flush();
+ + ";jaas:update;jaas:realm-manage --realm=karaf;jaas:user-list");
closeSshChannel(pipe);
System.out.println(new String(out.toByteArray()));
}
void addViewer(String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
+ ";jaas:user-add " + vieweruser + " " + vieweruser
+ ";jaas:role-add " + vieweruser + " viewer"
+ ";jaas:role-add " + vieweruser + " ssh"
- + ";jaas:update;jaas:realm-manage --realm=karaf;jaas:user-list\n").getBytes());
- pipe.flush();
+ + ";jaas:update;jaas:realm-manage --realm=karaf;jaas:user-list");
closeSshChannel(pipe);
System.out.println(new String(out.toByteArray()));
}
- String assertCommand(String user, String command, Result result) throws Exception {
+ /**
+ * Writes the given command(s) to the SSH channel and blocks until they have been fully
+ * processed by the remote shell.
+ *
+ * <p>A sentinel {@code echo <marker>} command is appended after the supplied command and
+ * this method waits until the unique marker shows up in the captured output. Because the
+ * remote shell reads and executes its input line by line, the marker cannot appear before
+ * the supplied command has been fully executed and its output flushed back to the client.
+ * Without this synchronization the SSH session could be torn down (see
+ * {@link #closeSshChannel(OutputStream)}) while the command output is still in flight,
+ * producing truncated output and spurious assertion failures - regularly observed on slower
+ * Windows CI runners. {@code echo} is a gogo built-in that is not restricted by any command
+ * ACL, so it is safe to use for every test user.</p>
+ */
+ private void writeCommandAndWait(OutputStream pipe, ByteArrayOutputStream out, String command) throws IOException {
if (!command.endsWith("\n"))
command += "\n";
+ pipe.write(command.getBytes());
+ String marker = "KARAF_ITEST_MARKER_" + System.nanoTime();
+ pipe.write(("echo " + marker + "\n").getBytes());
+ pipe.flush();
+ try {
+ Awaitility.await().atMost(60, TimeUnit.SECONDS)
+ .pollInterval(200, TimeUnit.MILLISECONDS)
+ .until(() -> out.toString().contains(marker));
+ } catch (ConditionTimeoutException e) {
+ // Fall through: proceed with whatever output was captured so far. For assertions
+ // this yields a more useful failure message than the timeout itself.
+ }
+ }
+
+ String assertCommand(String user, String command, Result result) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel(user, user, out, out);
- pipe.write(command.getBytes());
- pipe.flush();
+ writeCommandAndWait(pipe, out, command);
Review Comment:
`assertCommand()` also uses `writeCommandAndWait()`, which reads the output stream while SSHD writes to it concurrently. Use the synchronized output stream here as well to avoid rare concurrency exceptions or partial reads during Awaitility polling.
##########
itests/test/src/test/java/org/apache/karaf/itests/ssh/SshCommandTestBase.java:
##########
@@ -52,41 +54,67 @@ enum Result { OK, NOT_FOUND, NO_CREDENTIALS }
void addUsers(String manageruser, String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
+ ";jaas:user-add " + manageruser + " " + manageruser
+ ";jaas:role-add " + manageruser + " manager"
+ ";jaas:role-add " + manageruser + " viewer"
+ ";jaas:role-add " + manageruser + " ssh"
+ ";jaas:user-add " + vieweruser + " " + vieweruser
+ ";jaas:role-add " + vieweruser + " viewer"
+ ";jaas:role-add " + vieweruser + " ssh"
- + ";jaas:update;jaas:realm-manage --realm=karaf;jaas:user-list\n").getBytes());
- pipe.flush();
+ + ";jaas:update;jaas:realm-manage --realm=karaf;jaas:user-list");
closeSshChannel(pipe);
System.out.println(new String(out.toByteArray()));
}
void addViewer(String vieweruser) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
OutputStream pipe = openSshChannel("karaf", "karaf", out);
- pipe.write(("jaas:realm-manage --realm=karaf"
+ writeCommandAndWait(pipe, out, "jaas:realm-manage --realm=karaf"
+ ";jaas:user-add " + vieweruser + " " + vieweruser
+ ";jaas:role-add " + vieweruser + " viewer"
+ ";jaas:role-add " + vieweruser + " ssh"
- + ";jaas:update;jaas:realm-manage --realm=karaf;jaas:user-list\n").getBytes());
- pipe.flush();
+ + ";jaas:update;jaas:realm-manage --realm=karaf;jaas:user-list");
closeSshChannel(pipe);
System.out.println(new String(out.toByteArray()));
}
- String assertCommand(String user, String command, Result result) throws Exception {
+ /**
+ * Writes the given command(s) to the SSH channel and blocks until they have been fully
+ * processed by the remote shell.
+ *
+ * <p>A sentinel {@code echo <marker>} command is appended after the supplied command and
+ * this method waits until the unique marker shows up in the captured output. Because the
+ * remote shell reads and executes its input line by line, the marker cannot appear before
+ * the supplied command has been fully executed and its output flushed back to the client.
+ * Without this synchronization the SSH session could be torn down (see
+ * {@link #closeSshChannel(OutputStream)}) while the command output is still in flight,
+ * producing truncated output and spurious assertion failures - regularly observed on slower
+ * Windows CI runners. {@code echo} is a gogo built-in that is not restricted by any command
+ * ACL, so it is safe to use for every test user.</p>
+ */
+ private void writeCommandAndWait(OutputStream pipe, ByteArrayOutputStream out, String command) throws IOException {
if (!command.endsWith("\n"))
command += "\n";
+ pipe.write(command.getBytes());
+ String marker = "KARAF_ITEST_MARKER_" + System.nanoTime();
+ pipe.write(("echo " + marker + "\n").getBytes());
+ pipe.flush();
+ try {
+ Awaitility.await().atMost(60, TimeUnit.SECONDS)
+ .pollInterval(200, TimeUnit.MILLISECONDS)
+ .until(() -> out.toString().contains(marker));
+ } catch (ConditionTimeoutException e) {
+ // Fall through: proceed with whatever output was captured so far. For assertions
+ // this yields a more useful failure message than the timeout itself.
+ }
Review Comment:
Swallowing `ConditionTimeoutException` can hide cases where the remote shell never processes the command(s) (or the marker never makes it back). In setup flows like `addUsers()`/`addViewer()`, continuing after the timeout risks follow-up failures that are harder to diagnose. Consider failing fast with an error that includes the partial output captured so far.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky SSH security integration tests on Windows CI [karaf]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre merged PR #2764:
URL: https://github.com/apache/karaf/pull/2764
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org