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.

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

Fix all TLS tests and framework #6170

Merged
merged 4 commits into from
Feb 3, 2020
Merged

Fix all TLS tests and framework #6170

merged 4 commits into from
Feb 3, 2020

Conversation

Smjert
Copy link
Member

@Smjert Smjert commented Jan 13, 2020

  • Return a Status when starting the test_http_server, so tests will fail
    if the server start fails.
  • Fixes to support HTTP 1.1
  • Changes to make the script and configuration files it depends on
    relocatable, avoiding hardcoded paths or paths relative
    to the script itself
  • Do not accept partial failures, they could hide bugs; in the future
    if there are specific cases where the test is expected to fail,
    lets check for those specific cases, one by one.
  • Move osquery/remote/enroll plugin and tests to plugin/remote/enroll

Closes #5768
Should address #6169

@Smjert Smjert added cmake pure cmake changes test buck labels Jan 13, 2020
@Smjert Smjert mentioned this pull request Jan 13, 2020
Closed
@Smjert Smjert force-pushed the stefano/test/tls_tests branch from 8009fd8 to 1431851 Compare January 13, 2020 23:40
@theopolis
Copy link
Member

Thanks for working on this @Smjert. It looks like the BUCK errors are easy to fix and caused by fixing the placement of the enroll plugins. The test failures may be because we need to seed the RNG for the HTTPS server port selection, that and implement a retry for when it select a random port already in use.

Is there anything I can do to help move this forward out of draft?

@Smjert
Copy link
Member Author

Smjert commented Jan 28, 2020

Thanks for working on this @Smjert. It looks like the BUCK errors are easy to fix and caused by fixing the placement of the enroll plugins. The test failures may be because we need to seed the RNG for the HTTPS server port selection, that and implement a retry for when it select a random port already in use.

Is there anything I can do to help move this forward out of draft?

Thanks for the offer and for the tips!
I think I can fix it this week(end); had to focus on another work (namely the container access from tables on Linux).

@muffins
Copy link
Contributor

muffins commented Jan 30, 2020

Ah wow, this will be super awesome to have! It looks like this should take us a very long way in building testing for #6197. @Smjert lemme know if there's anything I can do to help! I'll get a review on this today, but I think I'll hold off on doing too much heavy lifting to get tests for #6197 until this ships, as this should help a lot in testing our TLS verifications, thanks!

@muffins muffins mentioned this pull request Jan 30, 2020
Closed
@directionless directionless added this to the 4.2.0 milestone Jan 31, 2020
Smjert added 3 commits February 1, 2020 01:28
@Smjert
Fix all TLS tests and framework
2c7fc17 
- Return a Status when starting the test_http_server, so tests will fail
  if the server start fails.
- Fixes to support HTTP 1.1
- Changes to make the script and configuration files it depends on
  relocatable, avoiding hardcoded paths or paths relative
  to the script itself
- Do not accept partial failures, they could hide bugs; in the future
  if there are specific cases where the test is expected to fail,
  lets check for those specific cases, one by one.
- Move osquery/remote/enroll plugin and tests to plugin/remote/enroll
@Smjert
Implement and fix BUCK project files
308d4bf
@Smjert
Implement retry if python test server port is occupied
ee3aa1d 
Also change how the python script is launched,
avoiding having to go through a shell.
In this way we immediately have the pid of the python script,
and we can more easily understand if the listening port
is matched with the right process.
@Smjert Smjert force-pushed the stefano/test/tls_tests branch from 1431851 to ee3aa1d Compare February 1, 2020 01:04
@Smjert Smjert marked this pull request as ready for review February 1, 2020 01:09
@Smjert
Copy link
Member Author

Smjert commented Feb 1, 2020 •
edited
Loading

@muffins This should be ready; I also had to change the function that launches the python script process because it was using a shell in between and it was causing issues with matching pids, to understand if the process that was using a certain port was the one we expected or not.
Now it directly launches the python interpreter.

theopolis
theopolis previously approved these changes Feb 1, 2020
View reviewed changes
Copy link
Member

@theopolis theopolis left a comment

Choose a reason for hiding this comment

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

Looks great! Just one nitpick

osquery/remote/transports/tests/tls_transports_tests.cpp Outdated
}
ASSERT_FALSE(status.ok());

#if OSQUERY_WINDOWS
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, I think we should use isPlatform instead of the preprocessor here.

muffins
muffins approved these changes Feb 1, 2020
View reviewed changes
Copy link
Contributor

@muffins muffins left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks for the hard work here @Smjert !

muffins
muffins previously requested changes Feb 1, 2020
View reviewed changes
Copy link
Contributor

@muffins muffins left a comment

Choose a reason for hiding this comment

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

Ah looks like there's some trouble with that PlatformType, maybe missing the header include? Or the lib in BUCK for #include <osquery/utils/info/platform_type.h>? Otherwise lgtm!

@Smjert Smjert force-pushed the stefano/test/tls_tests branch from 38c4bb5 to 2ffd913 Compare February 1, 2020 22:57
@muffins muffins dismissed their stale review February 1, 2020 22:57

disregard me for a bit :)

@Smjert
Copy link
Member Author

Smjert commented Feb 1, 2020

Ah looks like there's some trouble with that PlatformType, maybe missing the header include? Or the lib in BUCK for #include <osquery/utils/info/platform_type.h>? Otherwise lgtm!

Yeah sorry, was too quick, just changed.

theopolis
theopolis approved these changes Feb 2, 2020
View reviewed changes
Copy link
Member

@theopolis theopolis left a comment

Choose a reason for hiding this comment

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

Outstanding!

muffins
muffins approved these changes Feb 3, 2020
View reviewed changes
Copy link
Contributor

@muffins muffins left a comment

Choose a reason for hiding this comment

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

lgtm!

@muffins muffins merged commit d0b42a9 into osquery:master Feb 3, 2020
@Smjert Smjert mentioned this pull request Feb 3, 2020
Merged
@Smjert Smjert deleted the stefano/test/tls_tests branch April 2, 2020 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muffins muffins muffins approved these changes

@theopolis theopolis theopolis approved these changes

Assignees
No one assigned
Labels
cmake pure cmake changes test
Projects
None yet
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.

Refactor and restore TLS tests
4 participants
@Smjert @theopolis @muffins @directionless