code-review
  • puffycid

    puffycid

    10/20/2021, 11:55 PM
    could someone review/provide feedback for 1 or more of the PRs i opened at:https://github.com/osquery/osquery/pulls/puffyCid https://github.com/osquery/osquery/pull/7261 - amcache/registry parser https://github.com/osquery/osquery/pull/7260 - jumplist parser https://github.com/osquery/osquery/pull/7259 - UAL support for macos https://github.com/osquery/osquery/pull/7258 - FSEvents parser https://github.com/osquery/osquery/pull/7160 - PE, MACHO, ELF parser support thanks!
  • a

    alessandrogario

    12/13/2021, 6:26 PM
    About the Apple Silicon support: I am available for doing pair code reviews on the CMake, and talk about the READMEs we added for each library (cc @seph && @zwass and anyone else with review access and enough time 🙂)
  • m

    Mayur Shingote

    12/25/2021, 1:14 AM
    Hello, Need 👀 on my first osquery PR Added code to timeout connection for broken servers within 2 mins. https://github.com/osquery/osquery/pull/7429 Thanks.
  • p

    Peijun Zhu

    04/21/2022, 6:29 AM
    Hello Team, just a reminder to review my first osquery PR. Thanks! Make TLS status logging configurable #7550https://github.com/osquery/osquery/pull/7550
  • Daniel Bretón Suárez

    Daniel Bretón Suárez

    04/21/2022, 1:45 PM
    Hi! I want to contribute with this feature but I'm not in the osquery team. Do I have to create another blueprint and link it in some way to the original issue?https://github.com/osquery/osquery/issues/6790
  • j

    Joe Sweeney

    04/28/2022, 5:32 PM
    Hey everyone, I have a PR #7530 that we have reviewed inside of Trail of Bits that allows non-blocking reading of special files to avoid potential denial of service attacks. It is approved, but I wanted to advertise here so that anyone that wants to take a look or comment before merging can.
  • Mike Myers

    Mike Myers

    05/02/2022, 6:31 PM
    If any osquery TSC member can approve these external contributor PRs, I have recently reviewed and tested: • https://github.com/osquery/osquery/pull/7156 https://github.com/osquery/osquery/pull/7391 https://github.com/osquery/osquery/pull/7407
  • s

    seph

    05/02/2022, 9:52 PM
    👀
  • s

    seph

    05/02/2022, 11:42 PM
    Y’all have a bunch of approved PRs hanging out. I think you could sweep through and merge them. I probably will eventually
  • Mike Myers

    Mike Myers

    06/01/2022, 4:19 PM
    My PR to bring back
    kernel_panics
    is not that big, but generating kernel panic logs is annoying so I've attached sample files so the reviewer can test or just check how they look. https://github.com/osquery/osquery/pull/7585
  • Daniel Bretón Suárez

    Daniel Bretón Suárez

    06/02/2022, 7:56 AM
    This PR is ready to be reviewed, thank you on advancehttps://github.com/osquery/osquery/pull/7598
  • n

    np5

    06/28/2022, 3:44 PM
    Well, I thought I would try fix the issue that I posted about in #general:
  • n

    np5

    06/28/2022, 4:24 PM
    tested on an apple silicon mac and an intel mac. it works.
  • Lucas Rodriguez

    Lucas Rodriguez

    08/02/2022, 10:00 PM
    Hi @seph! I've pushed the suggested changes to #7675.
  • a

    Andre Pinter

    08/29/2022, 6:22 PM
    I've got a small issue that I'd like to put a PR up for... I've got a fix working locally but would like to chat in here first to see if I'm on the right track (also hi, I'm new to osquery and not a cpp programmer, so be aware 🙂 ). The issue: file carve table gets broken when running in debug because a type assertion fails in the json tree construction A solution: change the loading code in carves.cpp to switch on type
    diff --git a/osquery/tables/forensic/carves.cpp b/osquery/tables/forensic/carves.cpp
    index 1fcdb25af..80929ad13 100644
    --- a/osquery/tables/forensic/carves.cpp
    +++ b/osquery/tables/forensic/carves.cpp
    @@ -53,8 +53,10 @@ void enumerateCarves(QueryData& results, const std::string& new_guid) {
           r["time"] = INTEGER(tree.doc()["time"].GetUint64());
         }
    
    -    if (tree.doc().HasMember("size")) {
    +    if (tree.doc().HasMember("size") && tree.doc()["size"].IsInt()) {
           r["size"] = INTEGER(tree.doc()["size"].GetInt());
    +    } else if (tree.doc().HasMember("size") && tree.doc()["size"].IsString()) {
    +      r["size"] = INTEGER(tree.doc()["size"].GetString());
         }
    
         stringToRow("sha256", r, tree);
    From reading more of the codebase and the database code it seems like there's some friction where the update functions only take strings and then it seems its up to casts elsewhere in the codebase to turn them into the right types
  • Mike Myers

    Mike Myers

    08/30/2022, 5:50 AM
    Hi @Andre Pinter I think opening an issue for this is a good idea. It's great that you've already got a possible fix. If you open a PR, point it to the issue, it should be reviewed after you click to agree to the contributor license agreement (CLA)
  • Mike Myers

    Mike Myers

    08/30/2022, 5:52 AM
    In cases where a greater amount of code is being considered, it's always a good plan to get explicit agreement on the issue and the approach before starting. Just so that you don't implement a big chunk of work that cannot be accepted for design or practical reasons
  • Lucas Rodriguez

    Lucas Rodriguez

    09/06/2022, 10:37 PM
    Hi folks! This is ready for review (CI now passes, was failing due to flaky tests): https://github.com/osquery/osquery/pull/7712
  • Mike Myers

    Mike Myers

    09/16/2022, 4:44 PM
    Would anyone be able to review this PR https://github.com/osquery/osquery/pull/7714
  • Mike Myers

    Mike Myers

    09/26/2022, 4:08 PM
    Does anyone know enough to review that PR? https://github.com/osquery/osquery/pull/7714 If the answer is no, we will have to self-approve it
  • a

    Artemis Tosini

    09/29/2022, 4:10 PM
    I've ported forward some code for containerd events. Most of the original code is from @Stefano Bonicatti but I can fix any issues: https://github.com/osquery/osquery/pull/7751
  • Lucas Rodriguez

    Lucas Rodriguez

    10/19/2022, 6:54 PM
    Hi folks! The following two PRs are ready for re-review:1. #7655: Fixes the current odd behavior + adds missing documentation for "discovery" queries in distributed queries. It also fixes the following two issues: #5260 and #7793. 2. #7712: This is for future-proof support of osquery downgrades. So that osquery doesn't break whenever a new version adds a column family to RocksDB and users need to downgrade osquery.
  • b

    Brad Girardeau

    10/28/2022, 2:52 AM
    Hi osquery reviewers! I opened a new blueprint for making schedule epoch and counter behavior more consistent, to support periodic snapshots and stream alerting from the same differential rule: https://github.com/osquery/osquery/issues/7799 It has a couple PRs and draft commits for proposed implementation - curious what people think when you have time 🙂
  • Stefano Bonicatti

    Stefano Bonicatti

    11/10/2022, 12:45 PM
    Hello there, can anyone give a quick review to https://github.com/osquery/osquery/pull/7813, this issue is blocking/keeps failing our CI
  • zwass

    zwass

    12/02/2022, 1:44 AM
    There are a few lingering PRs from Fleet team members that could use some attention:
  • zwass

    zwass

    12/02/2022, 1:45 AM
    https://github.com/osquery/osquery/pull/7655 - @seph Do Lucas's answers work for you?
  • zwass

    zwass

    12/02/2022, 1:45 AM
    https://github.com/osquery/osquery/pull/7751 - @Stefano Bonicatti do you think you could take a look at this one?
  • zwass

    zwass

    12/02/2022, 1:45 AM
    https://github.com/osquery/osquery/pull/7712 - @Stefano Bonicatti do you think you could take a look at this as well? It's adapted from your idea.
  • zwass

    zwass

    12/02/2022, 1:46 AM
    https://github.com/osquery/osquery/pull/7821 - @thor this one is actually pretty new, but could you please lend a hand with your Windows expertise?