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

Adding remote config/logging capabilities to Windows build #2469

Merged
merged 12 commits into from
Sep 20, 2016
Merged

Adding remote config/logging capabilities to Windows build #2469

merged 12 commits into from
Sep 20, 2016

Conversation

yying
Copy link
Contributor

@yying yying commented Sep 15, 2016

Enabled remote config and logging capabilities using TLS.

For remote TLS logging and/or filesystem logging to work correctly, a glog with a bug fix (google/glog#123).

Closes #2425
Closes #2426

@yying yying added the Windows label Sep 15, 2016
@osqueryer
Copy link

Can one of the osquery admins reply with "ok to test" to kick off a build...

2 similar comments
@osqueryer
Copy link

Can one of the osquery admins reply with "ok to test" to kick off a build...

@osqueryer
Copy link

Can one of the osquery admins reply with "ok to test" to kick off a build...

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 15, 2016
@ghost
Copy link

ghost commented Sep 15, 2016

@yying updated the pull request - view changes

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 15, 2016
@ghost
Copy link

ghost commented Sep 15, 2016

@yying updated the pull request - view changes

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 15, 2016
@facebook-github-bot
Copy link

@yying updated the pull request - view changes

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 15, 2016
@mtmcgrew
Copy link
Contributor

ok to test

@osqueryer
Copy link

🙅 The commit aa121ed (Job results: 238) failed the code audit.

@osqueryer
Copy link

👎 The commit aa121ed (Job results: 265) failed one or more tests (Windows).

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 16, 2016
@ghost
Copy link

ghost commented Sep 16, 2016

@yying updated the pull request - view changes

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 16, 2016
@osqueryer
Copy link

👎 The commit bb99bff (Job results: 268) failed one or more tests (Windows).

@@ -27,7 +29,9 @@ std::string platformAsctime(const struct tm* timeptr) {
return "";
}

return std::string(buffer.data(), buffer.size());
std::string time(buffer.data());
Copy link
Member

Choose a reason for hiding this comment

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

nit, avoid time as a local variable name.

std::string expected;
if (isPlatform(PlatformType::TYPE_WINDOWS)) {
expected = std::string(
"\x1F\x8B\b\0\0\0\0\0\x2\v\xED\xC4\xB1\r\0\0\x4\0\xB0s\xC5"
Copy link
Member

Choose a reason for hiding this comment

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

nit there's only 1 character difference? Maybe we can use some concatenation?

std::string expected = "\x1F\x8B\b\0\0\0\0\0\x2";
expected += (isPlatform(PlatformType::TYPE_WINDOWS)) ? "\v" : "\x3";
expected += ...

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 16, 2016
@ghost
Copy link

ghost commented Sep 16, 2016

@yying updated the pull request - view changes

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 16, 2016
@osqueryer
Copy link

👎 The commit 8358e67 (Job results: 3263) failed one or more tests (OS X/Linux).

@osqueryer
Copy link

👎 The commit 8358e67 (Job results: 273) failed one or more tests (Windows).

@ghost
Copy link

ghost commented Sep 16, 2016

@yying updated the pull request - view changes

@osqueryer
Copy link

👎 The commit a150ba9 (Job results: 280) failed one or more tests (Windows).

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 16, 2016
@osqueryer
Copy link

👎 The commit a150ba9 (Job results: 3270) failed one or more tests (OS X/Linux).

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 19, 2016
@osqueryer
Copy link

👎 The commit b53eeeb (Job results: 291) failed one or more tests (Windows).

@ghost
Copy link

ghost commented Sep 20, 2016

@yying updated the pull request - view changes

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 20, 2016
@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 20, 2016
options.openssl_certificate(server_certificate_file_);

// On Windows, we cannot set openssl_certificate to a directory
if (status.type() == fs::regular_file) {
Copy link
Member

Choose a reason for hiding this comment

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

We should include an else and a LOG(WARNING).

@@ -10,6 +10,40 @@

#pragma once

// Our third-party version of cpp-netlib uses OpenSSL APIs.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe /*.

/// Suppressing warning C4005: 'ASIO_ERROR_CATEGORY_NOEXCEPT': macro redefinition
#pragma warning(disable: 4005)

// Suppressing warning C4244: 'argument': conversion from '__int64' to 'long', possible loss of data
Copy link
Member

Choose a reason for hiding this comment

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

nit, /// for consistency.

#ifdef WIN32
#pragma warning(pop)

// We need to reinclude this to re-enable boost's warning suppression
Copy link
Member

Choose a reason for hiding this comment

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

nit, same

@theopolis
Copy link
Member

Ok, one last round of changes and this looks good.

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 20, 2016
@ghost
Copy link

ghost commented Sep 20, 2016

@yying updated the pull request - view changes

@ghost ghost added the cla signed Automated label: Pull Request author has signed the osquery CLA label Sep 20, 2016
@theopolis theopolis merged commit a7af70d into osquery:master Sep 20, 2016
theopolis pushed a commit to thedrow/osquery that referenced this pull request Sep 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Automated label: Pull Request author has signed the osquery CLA remote Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants