Add a CI and a basic integration test #194

Res260 · 2020-03-19T02:47:57Z

I had a burst of motivation and did this, I don't really care if you want it or not but offer it nonetheless, I mostly did this as a personnal challenge :)

Basically, I added a Github Actions workflow that does two things on push and on Pull Request on the repo:
1. Try to build the Docker Image.
2. Try to install PyRDP and run a new integration test I made on ubuntu and windows.
3. Show a coverage report and fail it it falls under 50%

This test is based on an RDP session I just recorded which try to use as most RDP features that PyRDP supports as possible.

Right now, it does assertions on these:

Asserts that PyRDP correctly build the requested virtual channels;
Asserts that PyRDP sees a clipboard PDU with the pasted data;
Asserts that the number of mouse clicks parsed matches the number of total clicks;
Asserts that PyRDP clearly sees some text that was typed on the keyboard at the end of the session;

Beyond those assertions, it executes a large portion of the PyRDP codebase:

It does NOT test these:

TLS, RC4 and anything else crypto
Logging
Replays
Device Redirection Channel
Executables in the "bin" folders
The first two packets of the RDP connection

This test could be better (and can be improved in the future), but I believe it will catch most low-hanging fruits/bugs!

Because of (what I believe is) a synchronization problem, I disabled testing for the device redirection channel.

It is heavily based on the pyrdp-replay branch (requires scapy as well).

…ssion that went through PyRDP to assert that various features of PyRDP are respected

hope these dont show up in the gosecure slack

alxbl · 2020-03-19T12:07:40Z

This is very cool! I think it's a great first step towards having proper unit testing and integration testing in PyRDP. I have a few comments but the general approach looks good to me.

Good use of dependency injection for the configs, too.

.github/workflows/ci.yml

test/test.py

alxbl · 2020-03-20T16:40:07Z

If you can rebase this and --force-with-lease to clean up the commit history, I'd be willing to merge this now.

Res260 · 2020-03-20T16:42:40Z

@alxbl wouldn't squash and merge do the job?

Edit: wording as the first message seemed passive-agressive

obilodeau · 2020-03-20T16:47:44Z

@alxbl I would like to review before you merge, please. I'm very interested in GitHub Actions beyond just this repo.

alxbl · 2020-03-20T17:16:56Z

@alxbl wouldn't squash and merge do the job?

Edit: wording as the first message seemed passive-agressive

It will, but your commits will disappear. I was offering you to keep your commit history in master.

Res260 · 2020-03-20T17:40:52Z

Ahhh I don't really care about having multiple commits for this PR, and the branch is still kept in GitHub if we really need the information

obilodeau

This is amazing! Thanks a lot for taking the time to build it.

Some general notes:

I'm not fond of the 16-megabyte pcap addition. It's large. I suspect it can be compressed efficiently (especially if it's the text PDUs). If the compression ratio is good, I'm probably going to compress it and decompress it from the test suite when testing.
How did you do the coverage analysis? Can you provide instructions in docs/devel.adoc, please.

.github/workflows/ci.yml

pyrdp/mitm/BasePathMITM.py

obilodeau · 2020-03-24T18:15:21Z

pyrdp/mitm/RDPMITM.py



 class RDPMITM:
    """
    Main MITM class. The job of this class is to orchestrate the components for all the protocols.
    """

-    def __init__(self, mainLogger: SessionLogger, crawlerLogger: SessionLogger, config: MITMConfig):
+    def __init__(self, mainLogger: SessionLogger, crawlerLogger: SessionLogger, config: MITMConfig, state: RDPMITMState=None, recorder: Recorder=None):


Note to self: this is duplicated from another active PR

obilodeau · 2020-03-24T18:17:32Z

test/test.py

+    return ".".join(str(b) for b in data)
+
+
+def parseExportedPdu(packet: packet.Raw):


Code duplicated from an active PR. One of us will rebase and refactor to avoid this useless duplication.

Co-Authored-By: Olivier Bilodeau <obilodeau@gosecure.net>

obilodeau · 2020-03-24T18:40:13Z

A simple zip of the pcap gave a 5.9M binary (from 16M). It's not as good as I have expected. Since git stores it's pack metadata already compressed (so also on the network), I'm going to let go on this.

I might revisit if we add more pcap-based tests to reduce checkout size.

.github/workflows/ci.yml

Res260 · 2020-03-24T18:42:13Z

A simple zip of the pcap gave a 5.9M binary (from 16M). It's not as good as I have expected. Since git stores it's pack metadata already compressed (so also on the network), I'm going to let go on this.

I might revisit if we add more pcap-based tests to reduce checkout size.

Should I revert the commit then?

obilodeau · 2020-03-24T18:44:56Z

Should I revert the commit then?

Since it's already done I'm tempted to say no.

That said, are you sure this directory will always be writable to the user running the tests? It looks like the GitHub action worked so we can keep it.

Res260 · 2020-03-24T18:46:33Z

That said, are you sure this directory will always be writable to the user running the tests? It looks like the GitHub action worked so we can keep it.

There is no garantee indeed, i'm not sure what would be the best approach. It will always work for the CI though

Res260 · 2020-03-24T18:47:10Z

For the code coverage, I could add it to the CI if you want using https://www.codecov.io/, thoughts?

Res260 · 2020-03-24T18:53:14Z

Actually I can do it inside the CI without a third party

obilodeau · 2020-03-24T18:59:42Z

For the code coverage, I could add it to the CI if you want using https://www.codecov.io/, thoughts?

then

Actually I can do it inside the CI without a third party

Do what you prefer.

Res260 · 2020-03-24T19:07:29Z

Added windows installation and test, as a treat :)

It does the same thing as the ubuntu one but on windows. My reasonning behind this is that even though installation does not fail per se, execution can fail because dependencies sometime have a hard time testing for both windows and linux, so I'd rather run the test twice.

Res260 · 2020-03-24T23:04:51Z

I added the coverage report to the CI, and it will fail if it falls under 50% (currently 53% coverage).

@obilodeau Should I still add documentation for the coverage in docs or it's not needed?

obilodeau · 2020-03-25T02:28:45Z

@obilodeau Should I still add documentation for the coverage in docs or it's not needed?

Not needed. This is self-explanatory now. Thanks!

obilodeau

This is ready to go in. Squash merging. Thanks @Res260 for the work!

Res260 added 9 commits March 18, 2020 22:02

Add a basic integration test which takes a pcap of a decrypted RDP se…

932394d

…ssion that went through PyRDP to assert that various features of PyRDP are respected

Add github actions (probably wont work at first try)

ac4bdb6

fix yml

7778361

third of many commits

3893bb5

install pyrdp dependencies before installing pyrdp

f58aa2c

add scapy dependency

c9e1ec8

fix wrong working dir

241dbdb

-_-

658c9c0

imported changes from pyrdp-replay to allow this to work

6be53df

hope these dont show up in the gosecure slack

Res260 added the enhancement New feature or request label Mar 19, 2020

Res260 requested review from obilodeau and alxbl March 19, 2020 02:47

alxbl reviewed Mar 19, 2020

View reviewed changes

pr comments

e4502a6

alxbl added this to the vNext milestone Mar 24, 2020

obilodeau requested changes Mar 24, 2020

View reviewed changes

Update .github/workflows/ci.yml

9962532

Co-Authored-By: Olivier Bilodeau <obilodeau@gosecure.net>

Zip the test_session pcap instead

39846fa

obilodeau reviewed Mar 24, 2020

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

Res260 added 2 commits March 24, 2020 14:44

remove \n

9344bc3

remove \n

2204146

Res260 added 2 commits March 24, 2020 14:57

try to test using windows as well

aad1efc

fix python3 call instead of python

84b01ad

remove setuptools installation from windows ci

1e294c7

Res260 added 2 commits March 24, 2020 18:58

Add coverage report to the CI, fail under 50% coverage

88a5f91

fix typo

7dc667c

obilodeau approved these changes Mar 25, 2020

View reviewed changes

obilodeau merged commit ff96ed2 into master Mar 25, 2020

obilodeau deleted the integration_test branch March 25, 2020 02:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a CI and a basic integration test #194

Add a CI and a basic integration test #194

Res260 commented Mar 19, 2020 •

edited

Loading

alxbl commented Mar 19, 2020

alxbl commented Mar 20, 2020

Res260 commented Mar 20, 2020 •

edited

Loading

obilodeau commented Mar 20, 2020

alxbl commented Mar 20, 2020

Res260 commented Mar 20, 2020

obilodeau left a comment

obilodeau Mar 24, 2020

obilodeau Mar 24, 2020

obilodeau commented Mar 24, 2020

Res260 commented Mar 24, 2020

obilodeau commented Mar 24, 2020

Res260 commented Mar 24, 2020

Res260 commented Mar 24, 2020

Res260 commented Mar 24, 2020

obilodeau commented Mar 24, 2020

Res260 commented Mar 24, 2020

Res260 commented Mar 24, 2020

obilodeau commented Mar 25, 2020

obilodeau left a comment

		return ".".join(str(b) for b in data)


		def parseExportedPdu(packet: packet.Raw):

Add a CI and a basic integration test #194

Add a CI and a basic integration test #194

Conversation

Res260 commented Mar 19, 2020 • edited Loading

alxbl commented Mar 19, 2020

alxbl commented Mar 20, 2020

Res260 commented Mar 20, 2020 • edited Loading

obilodeau commented Mar 20, 2020

alxbl commented Mar 20, 2020

Res260 commented Mar 20, 2020

obilodeau left a comment

Choose a reason for hiding this comment

obilodeau Mar 24, 2020

Choose a reason for hiding this comment

obilodeau Mar 24, 2020

Choose a reason for hiding this comment

obilodeau commented Mar 24, 2020

Res260 commented Mar 24, 2020

obilodeau commented Mar 24, 2020

Res260 commented Mar 24, 2020

Res260 commented Mar 24, 2020

Res260 commented Mar 24, 2020

obilodeau commented Mar 24, 2020

Res260 commented Mar 24, 2020

Res260 commented Mar 24, 2020

obilodeau commented Mar 25, 2020

obilodeau left a comment

Choose a reason for hiding this comment

Res260 commented Mar 19, 2020 •

edited

Loading

Res260 commented Mar 20, 2020 •

edited

Loading