SSH CA cert feature SAT-28038#20453
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
register_host,lceandcvare treated as objects with.idattributes but are initialized fromcli_factoryhelpers (which usually return dict-like data); consider consistently using either dict keys or attributes (e.g.lce['id']/cv['id']) to avoid type/attribute mismatches. - The three
test_positive_ssh_ca_*tests share substantial setup and assertion logic (CA creation, host registration, invocation, and log checks); consider extracting common helpers/fixtures to reduce duplication and make intent clearer. - When constructing
InstallerCommandforforeman_proxy_plugin_remote_execution_script_ssh_host_ca_public_key, you wrap the certificate value in extra quotes (f'"{host_ca_cert}"'); consider passing the raw value and lettingInstallerCommandhandle quoting to avoid subtle shell-escaping issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `register_host`, `lce` and `cv` are treated as objects with `.id` attributes but are initialized from `cli_factory` helpers (which usually return dict-like data); consider consistently using either dict keys or attributes (e.g. `lce['id']` / `cv['id']`) to avoid type/attribute mismatches.
- The three `test_positive_ssh_ca_*` tests share substantial setup and assertion logic (CA creation, host registration, invocation, and log checks); consider extracting common helpers/fixtures to reduce duplication and make intent clearer.
- When constructing `InstallerCommand` for `foreman_proxy_plugin_remote_execution_script_ssh_host_ca_public_key`, you wrap the certificate value in extra quotes (`f'"{host_ca_cert}"'`); consider passing the raw value and letting `InstallerCommand` handle quoting to avoid subtle shell-escaping issues.
## Individual Comments
### Comment 1
<location> `tests/foreman/destructive/test_remoteexecution.py:249-148` </location>
<code_context>
+ )
+
+
+def log_compare(satellite, host):
+ return host.execute(
+ f'[ $(( $(cat /root/saved_sshd_log) + 1 )) -eq $(journalctl -u sshd | grep {satellite.ip} | grep " CA " | wc -l) ]'
+ ).status
+
+
</code_context>
<issue_to_address>
**suggestion:** Clarify and/or assert more explicitly what `log_compare` is checking to avoid fragile log-count based assertions
This helper assumes `journalctl | grep {satellite.ip} | grep " CA " | wc -l` increases by exactly 1 between `log_save` and the job run, which is brittle: concurrent SSH activity in shared environments can match the same pattern, and log rotation/truncation can change counts independently of this test.
Please consider either matching a more specific pattern (e.g., including a unique test-run identifier) or parsing `journalctl` output and asserting that at least one new relevant line appears, rather than enforcing an exact count change. If you keep this approach, add a brief comment showing the expected `sshd` log line so the dependency is explicit.
Suggested implementation:
```python
def log_save(satellite, host):
# We track sshd log entries for CA-based publickey auth from the satellite IP.
# Expected log line (simplified example):
# "Accepted publickey for <user> from <satellite_ip> ... ssh2: RSA-CERT ID ... CA ..."
host.execute(
f'journalctl -u sshd | grep "{satellite.ip}" | grep "Accepted publickey" | grep " CA " | wc -l > /root/saved_sshd_log'
)
```
```python
def log_compare(satellite, host):
"""Check that at least one new CA-based sshd log entry appears for the satellite since log_save().
Compares the current count of sshd log lines matching:
satellite.ip + "Accepted publickey" + " CA "
with the count stored by log_save(). Returns the exit status of the check command
(0 for success, non-zero for failure).
"""
return host.execute(
f'[ $(journalctl -u sshd | grep "{satellite.ip}" | grep "Accepted publickey" | grep " CA " | wc -l) -gt $(cat /root/saved_sshd_log) ]'
).status
from robottelo.config import get_credentials
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lhellebr
force-pushed
the
ssh_ca_cert
branch
from
c15d328 to
7d6aa40
Compare
|
Ready but:
Feel free to review but do not merge yet. |
|
TODO: provisioning coverage (possibly in a separate PR) once I get access to provisioning infra |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
test_executionhelper returns the result ofsatellite.cli.JobInvocation.info, which in other tests is treated as a dict (result['success']), but intest_positive_ssh_ca_sat_onlyyou assertresult.status == 0; this inconsistency is likely a bug and should be aligned to a single, correct return type/usage. - There is a lot of duplicated shell command and path logic (e.g., repeated
ssh-keygeninvocations,restorecon/chown/chgrpsequences, and path string munging withsplit('/')) across fixtures and tests; consider extracting small reusable helpers to reduce duplication and make the setup steps easier to read and maintain. - Functions like
log_comparereturn a bare.statusinteger and the tests then compare that to 0/!=0, which makes the intent a bit opaque; wrapping this in a boolean-returning helper (e.g.,used_ca_authentication(host, sat) -> bool) would make the assertions more self-explanatory.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `test_execution` helper returns the result of `satellite.cli.JobInvocation.info`, which in other tests is treated as a dict (`result['success']`), but in `test_positive_ssh_ca_sat_only` you assert `result.status == 0`; this inconsistency is likely a bug and should be aligned to a single, correct return type/usage.
- There is a lot of duplicated shell command and path logic (e.g., repeated `ssh-keygen` invocations, `restorecon/chown/chgrp` sequences, and path string munging with `split('/')`) across fixtures and tests; consider extracting small reusable helpers to reduce duplication and make the setup steps easier to read and maintain.
- Functions like `log_compare` return a bare `.status` integer and the tests then compare that to 0/!=0, which makes the intent a bit opaque; wrapping this in a boolean-returning helper (e.g., `used_ca_authentication(host, sat) -> bool`) would make the assertions more self-explanatory.
## Individual Comments
### Comment 1
<location> `tests/foreman/destructive/test_remoteexecution.py:281` </location>
<code_context>
+ register_host(sat, host)
+ result = test_execution(sat, host)
+ # assert the run actually happened and it was authenticated using cert
+ assert result.status == 0
+ logger.debug(result)
+ assert log_compare(sat, host) == 0
</code_context>
<issue_to_address>
**issue (bug_risk):** JobInvocation result is treated as an object with `.status` but other tests use it as a dict, which is likely to break this test
Here `test_execution` returns `satellite.cli.JobInvocation.info(...)`, which is treated elsewhere in this file as a dict (`result['success'] == '1'`). Using `result.status` is inconsistent and may raise an `AttributeError` or behave incorrectly. Please align with the other tests and assert on the appropriate dict fields (e.g. `result['success']` and/or `result['overall']`) instead of `.status`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| register_host(sat, host) | ||
| result = test_execution(sat, host) | ||
| # assert the run actually happened and it was authenticated using cert | ||
| assert result.status == 0 |
There was a problem hiding this comment.
issue (bug_risk): JobInvocation result is treated as an object with .status but other tests use it as a dict, which is likely to break this test
Here test_execution returns satellite.cli.JobInvocation.info(...), which is treated elsewhere in this file as a dict (result['success'] == '1'). Using result.status is inconsistent and may raise an AttributeError or behave incorrectly. Please align with the other tests and assert on the appropriate dict fields (e.g. result['success'] and/or result['overall']) instead of .status.
|
trigger: test-robottelo |
|
PRT Result |
Satellite-QE
added
the
PRT-Passed
rmynar
left a comment
rmynar
left a comment
There was a problem hiding this comment.
I had to stare at the fixtures for a while to understand them so a brief description would be nice.
Overall ACK, just some non-blocking comment in code
| cert_name = f'{key_name}-cert.pub' | ||
| assert ( | ||
| rhel_contenthost.execute( | ||
| f'cd {host_ssh_path} && if ! [ -f ssh_host_ed25519_key ]; then ssh-keygen -t ed25519 -f {key_name} -N ""; fi' |
There was a problem hiding this comment.
f'cd {host_ssh_path} && if ! [ -f {key_name} ]; then ssh-keygen -t ed25519 -f {key_name} -N ""; fi'
Satellite-QE
removed
the
PRT-Passed
adamlazik1
left a comment
adamlazik1
left a comment
There was a problem hiding this comment.
I apologize for not taking a look earlier, it somehow slipped my mind. Left some thoughts.
| @pytest.mark.no_containers | ||
| @pytest.mark.rhel_ver_match([settings.content_host.default_rhel_version]) | ||
| def test_positive_ssh_ca_sat_only(ca_sat, rhel_contenthost): | ||
| """Setup Satellite's SSH key's cert, register host and run REX on that host |
There was a problem hiding this comment.
| """Setup Satellite's SSH key's cert, register host and run REX on that host | |
| """Setup Satellite's SSH cert, register host and run REX on that host |
Applies to other tests as well.
|
|
||
| :id: 353a21bf-f379-440a-9dc6-e17bf6414713 | ||
|
|
||
| :expectedresults: Verify the job has been run successfully against the host, Sat's cert hasn't been added to host's authorized_keys and CA verification has been used instead |
There was a problem hiding this comment.
| :expectedresults: Verify the job has been run successfully against the host, Sat's cert hasn't been added to host's authorized_keys and CA verification has been used instead | |
| :expectedresults: Verify the job has been run successfully against the host, Sat's public key hasn't been added to host's authorized_keys and CA verification has been used instead |
| def log_save(satellite, host): | ||
| """Save a number of lines mentioning CA was used in sshd log, | ||
| for later use | ||
| """ | ||
| host.execute( | ||
| f'journalctl -u sshd | grep {satellite.ip_addr} | grep CA | wc -l > /root/saved_sshd_log' | ||
| ) |
There was a problem hiding this comment.
I suggest more straightforward approach as opposed to saving a value to a file:
| def log_save(satellite, host): | |
| """Save a number of lines mentioning CA was used in sshd log, | |
| for later use | |
| """ | |
| host.execute( | |
| f'journalctl -u sshd | grep {satellite.ip_addr} | grep CA | wc -l > /root/saved_sshd_log' | |
| ) | |
| def log_count(satellite, host): | |
| """Return number of lines mentioning CA was used in sshd log.""" | |
| result = host.execute( | |
| f'journalctl -u sshd | grep {satellite.ip_addr} | grep CA | wc -l' | |
| ) | |
| return int(result.stdout.strip()) |
Then instead of log_compare I would simply do:
saved_count = log_count(sat, host)
assert log_count(sat, host) == saved_count + 1
In my eyes it makes the tests a bit more readable. Also probably a little more resource efficient since we aren't creating, writing to, and reading a file.
There was a problem hiding this comment.
also, isn't there a risk that tests will overwrite this file for each other if run concurrently? I suggest trying out pytest session with -n4 or something. If you decide to use the files, I suggest to introduce some test-specific naming to it. Adam's suggestion is also good but I'm afraid both suggestions don't fully cover the concurrency concern, because still there is a chance the log count will be for example +2 if other test did something meanwhile. Isn't there another way to verify success apart from checking logs? Checking task details maybe?
There was a problem hiding this comment.
Sadly I think the sshd logs should be verified. Otherwise there is a risk that the job passes using plain public key authentication. That shouldn't happen in the current implementation but if we don't test it then regressions can arise in the future. But wouldn't concurrent tests be run on different hosts?
There was a problem hiding this comment.
Of course I'll reimplement this to not use files, that was a leftover from testing I missed.
I agree with Adam, it's necessary to look at the logs.
| host_ca_file_local = f'/tmp/{gen_string("alpha")}' | ||
| host.get(host_path, host_ca_file_local) | ||
| satellite.put(host_ca_file_local, satellite_path) | ||
|
|
There was a problem hiding this comment.
I'd move the fixtures above the first test in the module
| return f'/var/lib/foreman-proxy/ssh/{ca_contenthost[1].split("/")[-1]}' | ||
|
|
||
|
|
||
| def register_host(satellite, host, cockpit=False): |
There was a problem hiding this comment.
bit puzzled about the purpose of this fixture. Is it required by your scenarios to create a separate organization for each registration? If not, then you could use module_org fixture. Also not sure about the cockpit condition, if we run this against the same ca_contenthost multiple times, it is possible that the cockpit repos will be enabled on the machine from the previous runs so cockpit=False is not a 100% guarantee. If separate orgs and strict separation of cockpit/non-cockpit hosts aren't a strong requirement I guess we could just do satellite.register_host_custom_repo with module_org in every tests and setup cockpit repos in the one test that actually requires that.
There was a problem hiding this comment.
Note this is not a fixture, it needs to run as part of test.
Note these are destructive tests so each of them should get their own Satellite, so no interference.
It's probably possible to setup cockpit everywhere, but that would do the setup steps (including repo sync) everywhere, even if it won't be used. The code will be a line shorter but it will be expensive, I would keep the condition.
And ok, I will save the org creation step and use function_org instead.
lhellebr
force-pushed
the
ssh_ca_cert
branch
from
ba80bb5 to
e75d9ae
Compare
|
trigger: test-robottelo |
|
PRT Result |
Satellite-QE
added
the
PRT-Passed
lhellebr
force-pushed
the
ssh_ca_cert
branch
from
29ae662 to
2921b8e
Compare
Satellite-QE
removed
the
PRT-Passed
| """Return number of lines mentioning CA was used in sshd log, | ||
| for later use | ||
| """ |
There was a problem hiding this comment.
Nitpick, non-blocking
| """Return number of lines mentioning CA was used in sshd log, | |
| for later use | |
| """ | |
| """Return number of lines mentioning CA was used in sshd log""" |
5 participants
No description provided.