Feature #6308: Write tests for SSH

Feature #6308

Write tests for SSH

Added by bertagaz 2013-09-26 05:48:52 . Updated 2015-03-31 19:13:21 .

Status:

Resolved

Priority:

Normal

Assignee:

Category:

Test suite

Target version:

Tails_1.3.2

Start date:

2013-09-26

Due date:

% Done:

100%

Feature Branch:

kytv:test/6308-ssh

Type of work:

Code

Blueprint:

Starter:

Affected tool:

Deliverable for:

Description

Subtasks

Related issues

Related to Tails - ~~Bug #9141~~: Requirements for running SSH tests are not fully documented	Resolved	2015-03-31
Related to Tails - ~~Bug #9142~~: SSH tests are missing the corresponding infrastructure	Resolved	2015-03-31
Blocked by Tails - ~~Feature #6301~~: Securely store secrets needed by the automated test suite	Resolved	2013-09-26
Copied to Tails - ~~Feature #9087~~: Write tests for connecting to hosts on a LAN via SSH	Resolved	2015-03-21

History

#1 Updated by intrigeri 2013-09-26 06:10:18

Target version set to Sustainability_M1

#2 Updated by intrigeri 2013-10-03 06:11:10

Subject changed from Write tests for ssh to Write tests for SSH

#3 Updated by intrigeri 2015-01-06 12:59:16

blocks #8538 added

#4 Updated by anonym 2015-01-09 13:24:35

Assignee set to kytv
Target version changed from Sustainability_M1 to Tails_1.5

#5 Updated by anonym 2015-01-09 15:53:20

blocks deleted (~~~~Feature #6300~~: Framework to set up throw-away services for the test suite~~)

#6 Updated by anonym 2015-01-09 15:54:29

blocked by ~~Feature #6301~~: Securely store secrets needed by the automated test suite added

#7 Updated by anonym 2015-01-09 16:28:18

See ~~Feature #6301#note-8~~ for a pointer on how this should be implemented.

#8 Updated by intrigeri 2015-02-19 15:32:00

blocked by ~~Feature #8920~~: Have and use a repo shared between core developers for storing test suite secrets added

#9 Updated by kytv 2015-02-22 23:21:13

Status changed from Confirmed to In Progress
Target version changed from Tails_1.5 to Tails_1.3.2
% Done changed from 0 to 20

I’ve almost got this done. Now working on the Gnome Places > Connect to a Server bits.

#10 Updated by kytv 2015-02-25 19:02:01

Assignee changed from kytv to anonym
QA Check set to Ready for QA
Feature Branch set to kytv:test/6308-ssh

#11 Updated by intrigeri 2015-03-02 16:44:27

Assignee changed from anonym to kytv
QA Check changed from Ready for QA to Dev Needed

The proposed branch is based on the devel one, so is unsuitable for 1.3.1. Please rebase your branch on top of the stable branch. Thanks!

#12 Updated by kytv 2015-03-02 18:28:32

Assignee changed from kytv to anonym
QA Check changed from Dev Needed to Ready for QA

Oops. Rebased on stable and force pushed.

(I’ll do the same for my other 1.3.1 changes.)

#13 Updated by anonym 2015-03-19 20:52:29

Assignee changed from anonym to kytv
QA Check changed from Ready for QA to Dev Needed

General comments:

As for the format in local.yml, perhaps we should structure it a bit more, i.e.:

SSH:
  username: ...
  hostname: ...
  private_key: ...
  public_key: ...

As for the LAN tests, I think we should, when possible, try to reduce the amount of setup needed. In this case we can drop the LAN_SSH_* stuff and set up the LAN ssh server ourselves, i.e.:

  Scenario: Connecting to an SSH server on the LAN
    Given an SSH server is running on the LAN
    When I connect to an SSH server on the LAN
    And I verify the SSH fingerprint for the SSH server
    Then I have sucessfully logged into the SSH server

If it’s of any help for the an SSH server is running on the LAN step, I have a little “recipe” for setting up an unprivileged SSH server, even though it obviously has to be adapted:

mkdir /tmp/sshd
cd /tmp/sshd
ssh-keygen -t rsa -N "" -f ssh_host_rsa_key
cat > sshd_config <<EOF
Port 10022
UsePrivilegeSeparation no
HostKey /tmp/sshd/ssh_host_rsa_key
PidFile /tmp/sshd/sshd.pid
EOF
/usr/sbin/sshd -f sshd_config -D

Possibly a process helper for sshd (like features/support/helpers/display_helper.rb and friends) should be made as well.

#14 Updated by anonym 2015-03-20 09:47:00

I didn’t run the LAN test since I think we should do what I proposed in my previous comment. I like what you did with the prefixes and read_and_validate_ssh_config(), although it’ll have to be slightly redone with my proposal. Unfortunately then the LAN credentials stuff can’t be put in features/config/defaults.yml but has to be determined programatically, i.e. the username from Etc.getlogin or something, and the host from features/domains/default_new.xml (i.e. 10.2.1.1).

As for the non-LAN tests, I ran them and they do work very well, but here’s a code review and some suggestions for improvements:

There’s a code vs doc discrepancy:

> @ssh_host = $config[“#{prefix}SSH_host”]

but in usage.mdwn it’s documented as SSH_hostname (and LAN_SSH_hostname).

> assert(!secret_ssh_key.nil? && …@

I constantly do the !...nil? thing myself for some reason, but imho an easier to read equivalent is to drop that so we get e.g. assert(secret_ssh_key && …@. There are several instances of this.

> @ unless prefix.include?(‘Unsafe’)@

Is this for debugging? If so, please add comment, cause it threw me off a bit. If you don’t think it’s needed any more, please remove it.

> if ssh_port@@
> @ assert(ssh_port.is_a?(Integer), "#{prefix}SSH_port must be an integer.")
> end

I suggest being a bit more forgiving and instead convert to integer via .to_i when assigning ssh_port.

> Then /^I connect to an SFTP server(?: on the| on a)?\s?(|LAN|Internet)$/ do |loc|
> @ next if skip_steps_while_restoring_background
> @ step “I have the SSH key pair for an SSH server on the #{loc}”@

Each scenario running this step already runs I have the SSH key... first, which is the right way to do this. Now I believe we create the SSH keys twice.

> @ I should be able to access SSH server on my LAN via their IP addresses@

I don’t get the emphasis on “should”. We miss “an” between “access SSH”.

#15 Updated by kytv 2015-03-21 00:12:28

anonym wrote:
> I didn’t run the LAN test since I think we should do what I proposed in my previous comment.

For the time being I removed all of the LAN bits, to be reimplemented as suggested (once I figure out how) ;)

> As for the non-LAN tests, I ran them and they do work very well, but here’s a code review and some suggestions for improvements:

Everything that was pointed out, other than the LAN bits, has been fixed.

#16 Updated by intrigeri 2015-03-21 01:03:25

> For the time being I removed all of the LAN bits, to be reimplemented as suggested.

=> please create a new ticket, with mostly the same metadata, about testing the LAN bits.

#17 Updated by kytv 2015-03-21 17:50:24

copied to ~~Feature #9087~~: Write tests for connecting to hosts on a LAN via SSH added

#18 Updated by kytv 2015-03-21 17:52:10

Assignee changed from kytv to anonym
QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> > For the time being I removed all of the LAN bits, to be reimplemented as suggested.
>
> => please create a new ticket, with mostly the same metadata, about testing the LAN bits.

Done and passing back to anonym.

#19 Updated by anonym 2015-03-25 08:57:11

Assignee changed from anonym to kytv
% Done changed from 20 to 40
QA Check changed from Ready for QA to Dev Needed

Two last things before we merge this:

First, this is what you get when your local.yml isn’t correct:

  Scenario: Connecting to an SSH server on the Internet                  # features/ssh.feature:19                                                                
    Given I have the SSH key pair for an SSH server                      # features/step_definitions/ssh.rb:17                                                    
      undefined method `[]' for nil:NilClass (NoMethodError)
      ./features/step_definitions/ssh.rb:2:in `read_and_validate_ssh_config'
      ./features/step_definitions/ssh.rb:22:in `/^I have the SSH key pair for an? (Git repository|SSH server)$/'                                                  
      features/ssh.feature:20:in `Given I have the SSH key pair for an SSH server'

Look how it’s done in pidgin.rb’s xmpp_account(), e.g. by rescuing NoMethodError. Yeah, I’ve realized that it’s a bit awkward to have these “deeper” YAML structures since it’ll require more work to deal with errors. When I do Feature #9040 I intend to improve the situation, so that a generic but appropriate error is thrown when trying to access a variable that isn’t in local.yml but that should be there. Until then, I think we have to do something like what’s done in xmpp_account().

Second, and this is pretty minor:

> Given /^I verify the SSH fingerprint for the (Git repository|SSH server)$/ do |server_type|
> next if @skip_steps_while_restoring_background
> assert(server_type = ‘Git repository’ || server_type = ‘SSH server’,
> “Unknown server type #{server_type} specified.”)

Please switch to (?:Git repository|SSH server) and drop |server_type| and the assertion. The assertion cannot possibly fail since this step won’t be run if for e.g. I verify the SSH fingerprint for the LAN SSH server since it won’t match the step regex.

#20 Updated by BitingBird 2015-03-25 23:15:22

% Done changed from 40 to 80

#21 Updated by kytv 2015-03-26 14:10:39

Note: I haven’t pushed the fixes yet.

anonym wrote:
> Two last things before we merge this:
>
> First, this is what you get when your local.yml isn’t correct:
> […]
> Look how it’s done in pidgin.rb’s xmpp_account(), e.g. by rescuing NoMethodError.

GAH, of course! I can’t explain why I didn’t already do that. Done now.

> Second, and this is pretty minor:
>
> > Given /^I verify the SSH fingerprint for the (Git repository|SSH server)$/ do |server_type|
> > next if skip_steps_while_restoring_background > > assert(server_type = 'Git repository' || server_type = 'SSH server', > > "Unknown server type #{server_type} specified.") > > Please switch to(?:Git repository|SSH server)@ and drop |server_type| and the assertion.

Perfect! This is actually what I wanted to do to begin with but didn’t realize that (?:blahblah) would work here since I obviously couldn’t care less about capturing the data in the regex—I just want it to match. What you suggested (thanks!) allows that to happen.

Done and testing this as well.

Your recent test addition will prevent this from being seen in the future:

  @check_tor_leaks
  Scenario: Connecting to an SSH server on the Internet                  # features/ssh.feature:19
    Given I have the SSH key pair for an SSH server                      # features/step_definitions/ssh.rb:27
    When I connect to an SSH server on the Internet                      # features/step_definitions/ssh.rb:51
    And I verify the SSH fingerprint for the SSH server                  # features/step_definitions/ssh.rb:45
    Then I have sucessfully logged into the SSH server                   # features/step_definitions/ssh.rb:65
Full network capture available at: /testsuite-tmp/ssh_sniffer.pcap-2015-03-26T13:35:36+00:00
      The following IPv4 TCP non-Tor Internet hosts were contacted:
      91.97.163.139 (RuntimeError)
      /home/kytv/tails/features/support/helpers/firewall_helper.rb:115:in `assert_no_leaks'
      /home/kytv/tails/features/support/hooks.rb:149:in `After'
Scenario failed at time 00:08:25
Took screenshot "/testsuite-tmp/ssh-2015-03-26T13:35:37+00:00.png"

AutomatedTestSuite :)

Now re-running/re-testing with stable merged in.

#22 Updated by kytv 2015-03-26 16:46:33

Assignee changed from kytv to anonym
QA Check changed from Dev Needed to Ready for QA

All noted changes have been made (thanks as always!), stable has been merged in, and the result has been pushed.

#23 Updated by Tails 2015-03-27 09:44:38

Applied in changeset commit:e63573b121dd77685c54c26fe6a765e9567c4734.

#24 Updated by anonym 2015-03-27 09:44:39

Status changed from In Progress to Fix committed
% Done changed from 80 to 100

Applied in changeset commit:402d53cb3c08bc0107ea3b985f53a75c46c00b83.

#25 Updated by anonym 2015-03-27 09:46:19

Assignee deleted (~~anonym~~)
QA Check changed from Ready for QA to Pass

#26 Updated by intrigeri 2015-03-31 09:57:44

related to ~~Bug #9141~~: Requirements for running SSH tests are not fully documented added

#27 Updated by intrigeri 2015-03-31 09:58:05

related to ~~Bug #9142~~: SSH tests are missing the corresponding infrastructure added

#28 Updated by intrigeri 2015-03-31 10:01:37

I think this can’t be marked as resolved until the missing bits (~~Bug #9142~~, ~~Feature #8920~~) are up.

#29 Updated by anonym 2015-03-31 19:05:46

Parent task deleted (~~~~Feature #6298~~~~)

#30 Updated by anonym 2015-03-31 19:09:49

blocks deleted (~~~~Feature #8920~~: Have and use a repo shared between core developers for storing test suite secrets~~)

#31 Updated by anonym 2015-03-31 19:13:21

Status changed from Fix committed to Resolved