Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

utils: added password_with_output function to capture ssh output #72

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

danlavu
Copy link

@danlavu danlavu commented Jan 21, 2024

some test cases will need to check the ssh output

@danlavu danlavu added the enhancement New feature or request label Jan 21, 2024
@danlavu danlavu self-assigned this Jan 21, 2024
@danlavu danlavu marked this pull request as draft January 21, 2024 03:52
@danlavu danlavu changed the title authentication: Added function parameters log and opts to ssh.auth.password authentication: auth.ssh.gssapi and auth.ssh.password_log functions Jan 22, 2024
@danlavu danlavu force-pushed the ssh branch 2 times, most recently from 2c99d8a to fb49ab4 Compare January 22, 2024 06:12
@danlavu danlavu marked this pull request as ready for review January 22, 2024 06:14
@danlavu danlavu requested a review from pbrezina January 22, 2024 06:15
@danlavu
Copy link
Author

danlavu commented Jan 22, 2024

I don't know how I feel about this, yet. Using expect to issue a double SSH connection. This is what the test is looking like.

@pytest.mark.importance("critical")
@pytest.mark.ticket(bz=2077893)
@pytest.mark.topology(KnownTopology.AD)
def test_sssd__enable_and_disable_with_gssapi(client: Client, provider: GenericProvider):
    """
    :title: Authselect sssd profile with-gssapi
    :setup:
        1. Create provider user "user-1"
        2. Select SSSD profile with-gssapi and with-mkhomedir
        3. Start SSSD
        4. Configure sshd and allow gssapi authentication
        5. Reload sshd
    :steps:
        1. SSH as "user-1" using gssapi
        2. Disable with-gssapi feature and SSH as "user-1" using gssapi
    :expectedresults:
        2. Authentication is successful for "user-1"
        2. Feature is disabled and authentication is unsuccessful for "user-1"
    :customerscenario: False
    :requirement: IDM-SSSD-TC: sssd: bz2077893 add with-gssapi (pam_sss_gss.so)
    """
    provider.user("user-1").add()
    client.authselect.select("sssd", ["with-gssapi", "with-mkhomedir"])
    client.sssd.start()

    client.sshd.config_set([
            {"GSSAPIAuthentication": "yes"},
            {"PasswordAuthentication": "yes"},
            {"UsePAM": "yes"}
    ])
    client.sshd.reload()

    assert client.auth.ssh.gssapi("user-1", password="Secret123")

@danlavu danlavu requested a review from ikerexxe January 22, 2024 13:40
@danlavu danlavu marked this pull request as draft January 22, 2024 21:50
@danlavu
Copy link
Author

danlavu commented Jan 22, 2024

I don't like it, taking it out of "ready for review"

@danlavu danlavu force-pushed the ssh branch 2 times, most recently from eeab4f6 to 0115da9 Compare January 31, 2024 21:11
@danlavu
Copy link
Author

danlavu commented Jan 31, 2024

I removed the gssapi stuff for now and just made this PR about capturing the ssh output for some test cases.

@danlavu danlavu marked this pull request as ready for review January 31, 2024 21:12
@danlavu danlavu force-pushed the ssh branch 2 times, most recently from 0291205 to ffd73ea Compare January 31, 2024 21:14
@danlavu danlavu changed the title authentication: auth.ssh.gssapi and auth.ssh.password_log functions authentication.py: expanded ssh.password to optional capture ssh output Jan 31, 2024
@danlavu
Copy link
Author

danlavu commented Jan 31, 2024

print(client.auth.ssh.password("user-1", password="GOODPASS", output=True))

spawn ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o PreferredAuthentications=password -o NumberOfPasswordPrompts=1 -l user-1 localhost Warning: Permanently added 'localhost' (ECDSA) to the list of known hosts. user-1@localhost's password: Last failed login: Wed Jan 31 04:08:06 UTC 2024 from 172.16.100.1 on ssh:notty There were 4 failed login attempts since the last successful login. Could not chdir to home directory /home/test/user-1: Permission denied -sh: /home/test/user-1/.profile: Permission denied -sh-5.2$ expect result: Password authentication successful

print(client.auth.ssh.password("user-1", password="GOODPASS", output=False)) : TRUE

print(client.auth.ssh.password("user-1", password="GOODPASS")) : TRUE

print(client.auth.ssh.password("user-1", password="BADPASS")) : FALSE

@pbrezina
Copy link
Member

pbrezina commented Feb 6, 2024

Is this ready for review?

@danlavu
Copy link
Author

danlavu commented Feb 6, 2024

Is this ready for review?

Yup.

@danlavu danlavu changed the title authentication.py: expanded ssh.password to optional capture ssh output authentication.py: expanded ssh.password to optionally capture ssh output Feb 6, 2024
@danlavu danlavu changed the title authentication.py: expanded ssh.password to optionally capture ssh output utils: added password_with_output function to capture ssh output Feb 15, 2024
@danlavu danlavu force-pushed the ssh branch 3 times, most recently from f77db47 to 3d149c0 Compare February 15, 2024 15:36
ikerexxe
ikerexxe previously approved these changes Feb 15, 2024
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better! Thanks for taking into account the feedback.

@danlavu
Copy link
Author

danlavu commented Feb 15, 2024

I had to fix the return type on the password function, @ikerexxe that has been updated.

ikerexxe
ikerexxe previously approved these changes Feb 16, 2024
@pbrezina
Copy link
Member

Ack. But since we are doing this, I would like to add one more thing to the return value, to avoid changing it later again.

It should return (expect return code, spawned command return code, stdout, stderr).

I also added next-actions/pytest-mh@ee357a9 so we can capture only the ssh output. Please, consider the following change:

diff --git a/sssd_test_framework/utils/authentication.py b/sssd_test_framework/utils/authentication.py
index 8727791..aaa0b68 100644
--- a/sssd_test_framework/utils/authentication.py
+++ b/sssd_test_framework/utils/authentication.py
@@ -413,7 +413,7 @@ class SSHAuthenticationUtils(MultihostUtility[MultihostHost]):
         self.opts = "-o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no"
         """SSH CLI options."""
 
-    def password_with_output(self, username: str, password: str) -> tuple[int, str, str]:
+    def password_with_output(self, username: str, password: str) -> tuple[int, int, str, str]:
         """
         SSH to the remote host and authenticate the user with password and captures standard output and error.
 
@@ -421,16 +421,34 @@ class SSHAuthenticationUtils(MultihostUtility[MultihostHost]):
         :type username: str
         :param password: User password.
         :type password: str
-        :return: Tuple containing [return code, standard out, standard error].
-        :rtype: Tuple[int, str, str]
+        :return: Tuple containing [except return code, command exit code, stdout, stderr].
+        :rtype: Tuple[int, int, str, str]
         """
 
         result = self.host.ssh.expect_nobody(
             rf"""
+            # Disable debug output
+            exp_internal 0
+
+            proc exitmsg {{ msg code }} {{
+                # Send EOF, if we are in the prompt
+                send \x04;
+
+                # Wait for the exit code
+                lassign [wait] pid spawnid os_error_flag rc
+
+                puts ""
+                puts "expect result: $msg"
+                puts "expect exit code: $code"
+                puts "expect spawn exit code: $rc"
+                exit $code
+            }}
+
             # It takes some time to get authentication failure
             set timeout {DEFAULT_AUTHENTICATION_TIMEOUT}
             set prompt "\n.*\[#\$>\] $"
             log_user 1
+            log_file /tmp/expect.log
 
             spawn ssh {self.opts} \
                 -o PreferredAuthentications=password \
@@ -439,27 +457,35 @@ class SSHAuthenticationUtils(MultihostUtility[MultihostHost]):
 
             expect {{
                 "password:" {{send "{password}\n"}}
-                timeout {{puts "expect result: Unexpected output"; exit 201}}
-                eof {{puts "expect result: Unexpected end of file"; exit 202}}
+                timeout {{exitmsg "Unexpected output" 201}}
+                eof {{exitmsg "Unexpected end of file" 202}}
             }}
 
             expect {{
-                -re $prompt {{puts "expect result: Password authentication successful"; exit 0}}
-                "{username}@localhost: Permission denied" {{puts "expect result: Authentication failure"; exit 1}}
-                "Connection closed by UNKNOWN port 65535" {{puts "expect result: Connection closed"; exit 2}}
-                timeout {{puts "expect result: Unexpected output"; exit 201}}
-                eof {{puts "expect result: Unexpected end of file"; exit 202}}
+                -re $prompt {{exitmsg "Password authentication successful" 0}}
+                "{username}@localhost: Permission denied" {{exitmsg "Authentication failure" 1}}
+                "Connection closed by UNKNOWN port 65535" {{exitmsg "Connection closed" 2}}
+                timeout {{exitmsg "Unexpected output" 201}}
+                eof {{exitmsg "Unexpected end of file" 202}}
             }}
 
-            puts "expect result: Unexpected code path"
-            exit 203
-        """
+            exitmsg "Unexpected code path" 203
+            """,
+            verbose=False,
         )
 
         if result.rc > 200:
             raise ExpectScriptError(result.rc)
 
-        return result.rc, result.stdout, result.stderr
+        expect_data = result.stdout_lines[-3:]
+
+        # Get command exit code.
+        cmdrc = int(expect_data[2].split(":")[1].strip())
+
+        # Alter stdout, first line is spawned command, the last three are our expect output.
+        stdout = "\n".join(result.stdout_lines[1:-3])
+
+        return result.rc, cmdrc, stdout, result.stderr
 
     def password(self, username: str, password: str) -> bool:
         """
@@ -472,7 +498,7 @@ class SSHAuthenticationUtils(MultihostUtility[MultihostHost]):
         :return: True if authentication was successful, False otherwise.
         :rtype: bool
         """
-        rc, _, _ = self.password_with_output(username, password)
+        rc, _, _, _ = self.password_with_output(username, password)
         return rc == 0
 
     def password_expired(self, username: str, password: str, new_password: str) -> bool:

@danlavu
Copy link
Author

danlavu commented Feb 16, 2024

@pbrezina That's fine with me, I applied your patch.

@danlavu
Copy link
Author

danlavu commented Feb 16, 2024

@pbrezina #85 Same changes to SU

@pbrezina
Copy link
Member

@ikerexxe Do you want to look at latest changes?

@ikerexxe
Copy link
Contributor

No need. I'm sure they've improved the code.

@pbrezina pbrezina merged commit 2e031d6 into SSSD:master Feb 19, 2024
2 of 5 checks passed
@danlavu danlavu deleted the ssh branch April 17, 2024 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants