Skip to content

Introduce SSH cert support #867

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.

Sign up for GitHub

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

Jump to bottom
Open
adamlazik1 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Introduce SSH cert support #867

adamlazik1 wants to merge 1 commit into from

Conversation

@adamlazik1
Copy link
Contributor

@adamlazik1 adamlazik1 commented Jun 26, 2025

No description provided.

@adamlazik1
Copy link
Contributor Author

adamlazik1 commented Jun 26, 2025

Depends on: theforeman/smart_proxy_remote_execution_ssh#126

@adamlazik1 adamlazik1 force-pushed the ssh-cert-support branch 5 times, most recently from 702e256 to 18db7e9 Compare July 11, 2025 07:12
@adamlazik1 adamlazik1 marked this pull request as ready for review July 11, 2025 07:14
@adamlazik1
Copy link
Contributor Author

adamlazik1 commented Jul 11, 2025

This is now ready for review.

adamlazik1
adamlazik1 commented Jul 11, 2025
View reviewed changes
manifests/plugin/ansible.pp Outdated
}
$known_hosts_file_option = $foreman_proxy::plugin::remote_execution::script::ssh_host_ca_public_key ? {
undef => '',
default => "-o UserKnownHostsFile=${foreman_proxy::plugin::remote_execution::script::ssh_identity_dir}/known_hosts -o UserKnownHostsFile=${foreman_proxy::plugin::remote_execution::script::ssh_ca_known_hosts_file}",
Copy link
Contributor Author

@adamlazik1 adamlazik1 Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Same issue as in remote execution. If we have host CA cert available, should we enforce StrictHostKeyChecking?

@evgeni
Copy link
Member

evgeni commented Jul 14, 2025

/packit build

@evgeni
Copy link
Member

evgeni commented Jul 14, 2025

@adamlazik1 could you rebase this please on latest master, so that Packit starts working?

@packit-as-a-service
Copy link

packit-as-a-service bot commented Jul 14, 2025

No config file for packit (e.g. .packit.yaml) found in theforeman/puppet-foreman_proxy on commit 18db7e9

For more info, please check out the documentation or contact the Packit team. You can also use our CLI command config validate or our pre-commit hooks for validation of the configuration.

@evgeni
Copy link
Member

evgeni commented Jul 14, 2025

/packit build

@adamlazik1 adamlazik1 force-pushed the ssh-cert-support branch 2 times, most recently from f6c78c1 to 5e673ed Compare July 14, 2025 15:05
@adamlazik1
Copy link
Contributor Author

adamlazik1 commented Jul 14, 2025

Updated ansible ssh args to enforce strict host key checking if host CA is provided.

@lhellebr
Copy link
Contributor

lhellebr commented Jul 15, 2025

/packit build

@packit-as-a-service
Copy link

packit-as-a-service bot commented Jul 15, 2025

Account lhellebr has no write access nor is author of PR!

@evgeni
Copy link
Member

evgeni commented Jul 15, 2025

/packit build

@adamlazik1
Copy link
Contributor Author

adamlazik1 commented Jul 21, 2025

Switching back to draft since the feature got postponed to 3.16

@adamlazik1 adamlazik1 marked this pull request as draft July 21, 2025 09:06
adamlazik1
adamlazik1 commented Jul 21, 2025
View reviewed changes
manifests/plugin/remote_execution/script.pp
# $ssh_identity_file:: Provide an alternative name for the SSH keys
#
# $ssh_keygen:: Location of the ssh-keygen binary
# $ssh_user_ca_public_key_file:: Public key file for the SSH CA certificate
Copy link
Contributor Author

@adamlazik1 adamlazik1 Jul 21, 2025

Choose a reason for hiding this comment

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

Should we unify with the host CA key behavior and pass the public key contents instead of a path? I would solve issues with permissions.

Copy link
Contributor

@lhellebr lhellebr Jul 22, 2025

Choose a reason for hiding this comment

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

From security POV, it should be fine since the CA key is public. But how long is it? How long will it eventually be? Maybe the shell won't like a line that long. I think this kind of data shouldn't be passed in the command line. OTOH, if it's consistent with something we already have, it may be the least confusing.
The permission issues themselves can be solved by documentation + help text.

Copy link
Contributor Author

@adamlazik1 adamlazik1 Jul 22, 2025

Choose a reason for hiding this comment

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

I see, thanks for your input. You have a good point with possible future length, so I will probably leave it as it is.

Copy link
Contributor Author

@adamlazik1 adamlazik1 Dec 18, 2025

Choose a reason for hiding this comment

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

Updated: parameter now accepts file path with the key instead of string.

@lhellebr
Copy link
Contributor

lhellebr commented Oct 30, 2025

/packit build

@packit-as-a-service
Copy link

packit-as-a-service bot commented Oct 30, 2025

Account lhellebr has no write access nor is author of PR!

@adamruzicka
Copy link
Contributor

adamruzicka commented Oct 30, 2025

/packit build

@ekohl
Copy link
Member

ekohl commented Oct 30, 2025

/packit build

@packit-as-a-service
Copy link

packit-as-a-service bot commented Oct 30, 2025

Account adamruzicka has no write access nor is author of PR!

@adamlazik1
Copy link
Contributor Author

adamlazik1 commented Oct 30, 2025

/packit build

@adamlazik1
Copy link
Contributor Author

adamlazik1 commented Nov 10, 2025

/packit build

@adamlazik1
Copy link
Contributor Author

adamlazik1 commented Nov 11, 2025

/packit build

1 similar comment
@lhellebr
Copy link
Contributor

lhellebr commented Nov 19, 2025

/packit build

@packit-as-a-service
Copy link

packit-as-a-service bot commented Nov 19, 2025

Account lhellebr has no write access nor is author of PR!

@adamlazik1
Copy link
Contributor Author

adamlazik1 commented Nov 19, 2025

/packit build

lhellebr
lhellebr reviewed Nov 21, 2025
View reviewed changes
lhellebr
lhellebr reviewed Nov 21, 2025
View reviewed changes
This was referenced Dec 3, 2025
Merged
Open
Open
lhellebr
lhellebr approved these changes Dec 3, 2025
View reviewed changes
Copy link
Contributor

@lhellebr lhellebr left a comment

Choose a reason for hiding this comment

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

I have been able to verify the SSH CA cert feature as a complex of the following 4 PRs:
theforeman/smart_proxy_remote_execution_ssh#126
theforeman/foreman#10571
theforeman/foreman_remote_execution#977
#867

Including the following use cases:
SSH REX
Ansible REX
Pull mode REX
Cockpit

Including hosts created by:
Global registration
Provisioning

Including scenarios:
SSH CA on Satellite side
SSH CA on host side
SSH CA on both sides

Both positive scenarios and negative scenarios, that is incorrect CA, incorrect cert, incorrect principal.

=> ACK to this PR

@adamruzicka
Copy link
Contributor

adamruzicka commented Dec 17, 2025

@adamlazik1 could you please rebase and then undraft?

@adamlazik1 adamlazik1 force-pushed the ssh-cert-support branch 2 times, most recently from b95fb9a to d614cca Compare December 18, 2025 09:42
@adamlazik1 adamlazik1 marked this pull request as ready for review December 18, 2025 09:42
@adamlazik1 adamlazik1 force-pushed the ssh-cert-support branch 2 times, most recently from 8164f9a to 844a7ae Compare December 18, 2025 12:04
@adamlazik1
Copy link
Contributor Author

adamlazik1 commented Dec 18, 2025

/packit build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lhellebr lhellebr lhellebr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@adamlazik1 @evgeni @lhellebr @adamruzicka @ekohl