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

Support multiple credentials for shell and ssh #1291

Closed
wants to merge 2 commits into from

Conversation

nlabriet
Copy link
Contributor

Description

Add support for having a list of usernames and passwords to try to connect with.
This touches shelldriver, networkservice and sshdriver.

Depending on the software provisioned on a place, credentials may be different. It's my case where 3 different credentials are needed (user1/pass1 user2/pass2 user3/pass3) when soft1 or soft2 or soft3 is on the target.

In order not to have to change the LabGrid configuration (and the exporter as networkservice holds the IP and credentials for ssh) I can up with this PR.

The credentials are tried in turn and I don't get a timeout with 3 credentials. More credentials will probably need a timeout tweak or a way to configure it.

I have manually tested login in with console, ssh, and -s shell ssh, I have also tested sending a file with scp.

This also enable a smoother test experience. My tests can now use the same configuration for all software.

Checklist

  • Documentation for the feature
  • Tests for the feature
  • The arguments and description in doc/configuration.rst have been updated
  • PR has been tested

Add support for trying different credentials to authenticate.
username and password can be lists of strings that are tried in turn to
log in.

Signed-off-by: Nicolas Labriet <[email protected]>
Add support for trying different credentials to authenticate.
username and password can be lists of strings that are tried in turn to
log in.

Signed-off-by: Nicolas Labriet <[email protected]>
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (d08687c) 63.0% compared to head (7f4a8bd) 62.9%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1291     +/-   ##
========================================
- Coverage    63.0%   62.9%   -0.1%     
========================================
  Files         160     160             
  Lines       11901   11920     +19     
========================================
+ Hits         7502    7507      +5     
- Misses       4399    4413     +14     
Files Coverage Δ
labgrid/resource/networkservice.py 100.0% <100.0%> (ø)
labgrid/driver/sshdriver.py 55.6% <53.3%> (-0.1%) ⬇️
labgrid/driver/shelldriver.py 26.4% <16.6%> (-0.7%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Bastian-Krause
Copy link
Member

Thanks for thinking about contributing.

We had several discussions about "generic environment configurations" recently. My perspective is: The environment configuration is intended to be DUT-specific (including software). If username/password are different, you need a different configuration. The same applies to cases like "different bootloader", "different prompt" etc.

The situation is different when you connect multiple identical DUTs, but your infrastructure is different, e.g. another power switch is used requiring a different driver in the environment config. For these cases, we think that an environment config managed by the coordinator might be a valid approach. Changing this place-specific, central config could be done via labgrid-client. A local config could still be used, both would be merged as long as they do not conflict.

So I don't think that this change is something that we want to merge into labgrid. What do you think, @Emantor, @jluebbe?

@nlabriet
Copy link
Contributor Author

nlabriet commented Nov 8, 2023

Thank you for your feedback.
I agree with the DUT-specific configuration needing to be in the environment configuration.
This should of course include credentials and prompts.

But I think the address given in NetworkService should come from the infrastructure and as of now, SSH credentials are mandatory in this class.

By the way, why keep SSH and console credentials separate?

My PR is meant to highlight and easy my use case, but I could probably manage another way (yaml anchors are great in the environment file).

@Bastian-Krause
Copy link
Member

Thank you for your feedback. I agree with the DUT-specific configuration needing to be in the environment configuration. This should of course include credentials and prompts.

But I think the address given in NetworkService should come from the infrastructure and as of now, SSH credentials are mandatory in this class.

If the address should come from the infrastructure (i.e. via a SNMPEthernetPort), you can use a NetworkService with a placeholder address and update it during runtime with this information.

By the way, why keep SSH and console credentials separate?

I think @jluebbe or @Emantor have to comment on that.

@nlabriet
Copy link
Contributor Author

If the address should come from the infrastructure (i.e. via a SNMPEthernetPort), you can use a NetworkService with a placeholder address and update it during runtime with this information.

The address is provided by the exporter and will always be the same for a given place.

What about adding username and password to SSHDriver and using thoses if they are defined? Falling back on the ones provided by NetworkService to maintain compatibility.

Since SSHDriver is in the environment file, credentials could then be completely managed by the environment file.

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.

2 participants