8000 curl_certificate test by nasehim7 · Pull Request #5281 · osquery/osquery · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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

curl_certificate test #5281

Merged
merged 2 commits into from
Jul 26, 2020
Merged

curl_certificate test #5281

merged 2 commits into from
Jul 26, 2020

Conversation

nasehim7
Copy link
Contributor
@nasehim7 nasehim7 commented Nov 6, 2018

tests for the table curl_certificate - Fix #5061

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot facebook-github-bot added the cla signed Automated label: Pull Request author has signed the osquery CLA label Nov 6, 2018
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

obelisk
obelisk reviewed Nov 13, 2018
View reviewed changes
Copy link
Contributor
@obelisk obelisk left a comment

Choose a reason for hiding this comment

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

Do we have any other tests that rely on general internet access? Other than that I think this is fine, just want to be careful about setting precedent.

@obelisk
Copy link
Contributor
obelisk commented Nov 13, 2018

I'm going to accept, to remove it from my review queue, but I think @fmanco should weigh in on this (or @akindyakov)

obelisk
obelisk previously approved these changes Nov 13, 2018
View reviewed changes
@obelisk obelisk added the test label Nov 13, 2018
@guliashvili
Copy link
Contributor

Thanks for working on improving osquery tests @nasehim7 . If you could rebase it on the experimental branch, I will merge it soon.

@guliashvili guliashvili self-assigned this Jan 14, 2019
@guliashvili guliashvili removed their assignment Feb 27, 2019
@directionless
Copy link
Member

@nasehim7 Seems reasonable. https://dev.azure.com/trailofbits/osquery/_build/results?buildId=651 says formatting error

@nasehim7 nasehim7 force-pushed the test_curl branch 5 times, most recently from d24db54 to 63cb8c7 Compare August 28, 2019 17:06
@nasehim7
Copy link
Contributor Author
nasehim7 commented Aug 28, 2019 •
edited
Loading

@directionless Thanks! I am not able to understand where exactly I am messing it up. I am not using an IDE instead just a simple editor(Sublime Text)

@directionless
Copy link
Member

@nasehim7 The linter is running tools/formatting/format-check.py. AFAIK at the root of it, it's using clang-format.

I also don't use an IDE. I manually run my code through clang-format -i to fix the formatting.

@nasehim7 nasehim7 force-pushed the test_curl branch 2 times, most recently from 3adc764 to c0210ec Compare August 29, 2019 13:31
@Smjert
Copy link
Member
Smjert commented Aug 29, 2019

@nasehim7 The linter is running tools/formatting/format-check.py. AFAIK at the root of it, it's using clang-format.

I also don't use an IDE. I manually run my code through clang-format -i to fix the formatting.

It's not the same as calling clang-format on the whole file though, the script checks and eventually format only the modified lines, which is what we would like to maintain since it creates less noise and conflicts between different versions of clang-format used.

@nasehim7
Copy link
Contributor Author
nasehim7 commented Aug 29, 2019 •
edited
Loading

@Smjert Understandable, thanks for the explanation. It seems the formatting is fixed now but it is throwing:

/Users/vsts/agent/2.155.1/work/1/s/tests/integration/tables/curl_certificate.cpp:37:30: error: expected '}'
{ {"hostname", NormalType} {"common_name", NormalType} {
^
/Users/vsts/agent/2.155.1/work/1/s/tests/integration/tables/curl_certificate.cpp:37:3: note: to match this '{'
{ {"hostname", NormalType} {"common_name", NormalType} {
^
1 error generated.

Not sure why is it throwing that because if there was some parenthesis mismatch, it wouldn't have done the building with buck. Is it because of some vital differences in the build system?

@Smjert
Copy link
Member
Smjert commented Aug 29, 2019

@Smjert Understandable, thanks for the explanation. It seems the formatting is fixed now but it is throwing:

/Users/vsts/agent/2.155.1/work/1/s/tests/integration/tables/curl_certificate.cpp:37:30: error: expected '}'
{ {"hostname", NormalType} {"common_name", NormalType} {
^
/Users/vsts/agent/2.155.1/work/1/s/tests/integration/tables/curl_certificate.cpp:37:3: note: to match this '{'
{ {"hostname", NormalType} {"common_name", NormalType} {
^
1 error generated.

Not sure why is it throwing that because if there was some parenthesis mismatch, it wouldn't have done the building with buck. Is it because of some vital differences in the build system?

The error is not showing with Buck because it's not running those tests, good catch!
I'll land a PR to add them soon.

@nasehim7 nasehim7 force-pushed the test_curl branch 3 times, most recently from de0ec39 to d11143b Compare August 29, 2019 16:30
@Smjert
Copy link
Member
Smjert commented Jun 17, 2020

Hi @nasehim7, sorry for leaving this pending, are you still interested on working on this PR?

The curl_certificates table got some updates, especially there are many more columns now.
Also the openssl library has been updated, so the issues you were facing with the table not returning columns should've been fixed; at least, I just tried and it works.

So you would need to:

  1. Rebase this on latest master
  2. Fix the build, adding the rest of the columns, plus changing the name of the map type from ValidatatioMap to ValidationMap
  3. Format the code before pushing, from the build folder calling make format or cmake --build . --target format.

@Smjert Smjert added needs response virtual tables and removed cla signed Automated label: Pull Request author has signed the osquery CLA labels Jun 17, 2020
@theopolis theopolis force-pushed the test_curl branch 2 times, most recently from 10ba8b1 to 7b40230 Compare July 23, 2020 02:42
@osquery osquery deleted a comment from linux-foundation-easycla bot Jul 23, 2020
@theopolis
Copy link
Member

Heads up @nasehim7, I rebased this onto our master.

@theopolis
Copy link
Member

This PR has proved helpful! The certificate_has_expired column was never being set because in the row structure it was set to has_expired.

I think we should change the column API here to remove the certificate_ prefixes on columns.

@theopolis theopolis merged commit dcf7252 into osquery:master Jul 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@directionless directionless directionless left review comments

@Smjert Smjert Smjert left review comments

@theopolis theopolis theopolis approved these changes

@obelisk obelisk obelisk left review comments

Assignees
No one assigned
Labels
API change bug test virtual tables
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create tests for the table curl_certificate
7 participants
@nasehim7 @facebook-github-bot @obelisk @guliashvili @directionless @Smjert @theopolis
0