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

[wip] Use public key authentication #1131

Closed
wants to merge 2 commits into from

Conversation

foursixnine
Copy link
Member

@foursixnine foursixnine commented Mar 22, 2019

Attempt to allow key based authentication for ssh based backends.

There's still the VNC part of this that I haven't quite found the way how to make it work, as it's still using password auth.

Attempt to allow key based authentication for ssh based backends
@foursixnine foursixnine changed the title [PoC] Use public key authentication [wip] Use public key authentication Mar 22, 2019
@@ -1170,24 +1170,40 @@ sub new_ssh_connection {
$args{username} ||= 'root';

my $ssh = Net::SSH2->new;

my $privatekey_path = '/home/foursixnine/.openqa';
Copy link
Member Author

Choose a reason for hiding this comment

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

This would be a worker setting

my ($self, $username, $host, $gui, $privatekey) = @_;
my $sshopts = "-o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no";
$sshopts .= " -o PubkeyAuthentication=no" unless defined $privatekey;
if (defined $privatekey && -e $privatekey){
Copy link
Member Author

@foursixnine foursixnine Mar 22, 2019

Choose a reason for hiding this comment

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

@coolo @mnowaksuse I'm tempted to use a job variable (say VIRSH_SSH_PRIVATEKEY and use that if it's set instead of expecting the private key to be passed somehow (Something I haven't figured out yet how to)... wdyt?

@mnowaksuse
Copy link

There's still the VNC part of this that I haven't quite found the way how to make it work, as it's still using password auth.

Perhaps make it password-less? I don't see much security in this.

@codecov
Copy link

codecov bot commented Mar 22, 2019

Codecov Report

Merging #1131 into master will decrease coverage by 11.25%.
The diff coverage is 5%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1131       +/-   ##
===========================================
- Coverage   38.71%   27.45%   -11.26%     
===========================================
  Files          40       40               
  Lines        4812     4822       +10     
  Branches      811      816        +5     
===========================================
- Hits         1863     1324      -539     
- Misses       2623     3285      +662     
+ Partials      326      213      -113
Impacted Files Coverage Δ
backend/baseclass.pm 22.94% <0%> (-27.72%) ⬇️
consoles/console.pm 26.66% <12.5%> (-25.84%) ⬇️
OpenQA/Benchmark/Stopwatch.pm 5% <0%> (-63.34%) ⬇️
consoles/network_console.pm 36.36% <0%> (-45.46%) ⬇️
ppmclibs/blib/lib/tinycv.pm 7.52% <0%> (-39.79%) ⬇️
consoles/VNC.pm 3.88% <0%> (-38.67%) ⬇️
consoles/vnc_base.pm 8.33% <0%> (-31.25%) ⬇️
needle.pm 37.8% <0%> (-15.25%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f59b6c0...d00617f. Read the comment docs.

@pevik
Copy link
Contributor

pevik commented Mar 26, 2019

@foursixnine What is a benefit of pub key authentication for our testing? Is it problematic to pass the password (I guess it's working everywhere, isn't it?) or security (why bother for testing)?
It'd be nice to briefly explain it in commit message.

@foursixnine
Copy link
Member Author

@foursixnine What is a benefit of pub key authentication for our testing? Is it problematic to pass the password (I guess it's working everywhere, isn't it?) or security (why bother for testing)?
It'd be nice to briefly explain it in commit message.

Actually it's a byproduct of another issue I was working with, regarding timeouts on svirt backends, main benefit here is just to not need to input the password, and avoid the risk of having keys missing when there's high network load (which correlates with connection timeout messages) which was an issue in the past. You can give a look to poo#41504

Security wise, it's not my concern atm but rather stability and safety that the connection can be stablished...

@pevik
Copy link
Contributor

pevik commented Mar 27, 2019

@foursixnine make sense, thanks for an explanation. I'd still explain the motivation in commit message or pointed out poo#41504. (if you search in git, you usually don't check PR on the web)

@foursixnine
Copy link
Member Author

@pevik makes sense tbh :) I'll do that at a later point

@okurz
Copy link
Member

okurz commented Nov 13, 2019

please reopen when you continue the work. Thanks. Closing to clean up old open PRs.

@okurz okurz closed this Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants