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

Fix base64 flag #988

Closed
wants to merge 1 commit into from
Closed

Fix base64 flag #988

wants to merge 1 commit into from

Conversation

realgam3
Copy link

@realgam3 realgam3 commented Jan 5, 2020

For the people who uses the -b or --print-base64 command argument,
"s_inspector->set_buffer_format(sinsp_evt::PF_NORMAL);" will restore the state to normal dot escaped string instead of base64 (that they chose....).

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

If contributing rules or changes to rules, please make sure to also uncomment one of the following line:

/kind rule-update

/kind rule-create

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area examples

/area rules

/area integrations

/area tests

/area proposals

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #987

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@poiana
Copy link

poiana commented Jan 5, 2020

@realgam3: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@poiana
Copy link

poiana commented Jan 5, 2020

Welcome @realgam3! It looks like this is your first PR to falcosecurity/falco 🎉

Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

Hello @realgam3 thanks for submitting this.

Will look deeply into it soon.

In the meantime would you please fill the PR template, provide a good title for the PR, and sign-off your commit?

@leodido
Copy link
Member

leodido commented Jan 16, 2020

Hey @realgam3, a friendly ping :)

We need the commits to be signed-off :)

@realgam3
Copy link
Author

Hey @realgam3, a friendly ping :)

We need the commits to be signed-off :)
Should I create a new pull request?

@leodido
Copy link
Member

leodido commented Jan 17, 2020

Maybe yes! Otherwise I'll use some git magic here as soon I have some spare time :)

For the people who uses the -b or --print-base64 command argument,
"s_inspector->set_buffer_format(sinsp_evt::PF_NORMAL);"
will restore the state to normal dot escaped string instead of base64 (that they chose....).

Co-authored-by: Leonardo Di Donato <leodidonato@gmail.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido leodido self-requested a review January 24, 2020 13:19
@leodido leodido changed the title Fix issue #987 Fix base64 flag Jan 24, 2020
@poiana
Copy link

poiana commented Jan 28, 2020

LGTM label has been added.

Git tree hash: 55765d89151dac5c7986a05816f654fce68f3dc7

@fntlnz
Copy link
Contributor

fntlnz commented Jan 28, 2020

@realgam3 sorry CI didn't trigger (we had a problem with some settings earlier)
I'm closing and reopening to let the magic re-happen!

@fntlnz fntlnz closed this Jan 28, 2020
@fntlnz fntlnz reopened this Jan 28, 2020
@poiana
Copy link

poiana commented Jan 28, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leodido

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@leodido leodido self-requested a review January 28, 2020 11:33
@fntlnz fntlnz changed the base branch from dev to master February 3, 2020 15:15
@fntlnz
Copy link
Contributor

fntlnz commented Feb 4, 2020

I'm still waiting to merge this because I'm trying to understand how this is related to #581

@fntlnz
Copy link
Contributor

fntlnz commented Feb 4, 2020

That line was initially removed to support base64 but then was re-added because json was broken https://github.com/falcosecurity/falco/pull/410/files#diff-619b66cc1dd5ce6c64d2aa45a121ad63L127

@fntlnz
Copy link
Contributor

fntlnz commented Feb 4, 2020

Here's what I get after testing this patch:

Output with this change:

{
  "output": "{\"evt.buffer\":\"IBtbMG0bWzAxOzM0bURlc2t0b3AbWzBtICAgG1swMTszNG1Eb2N1bWVudHMbWzBtICAgG1swMTszNG1Eb3dubG9hZHMbWzBtICAgG1swMTszNG1nbxtbMG0gICAbWzAxOzM0bVBpY3R1cmVzG1swbSAgIBtbMDE7MzRtUHJvamVjdHMbWzBtICAgG1swMTszNG1WaWRlb3MbWzBtICAbWzAxOzM0bSdWaXJ0dWFsQm94IFZNcycbWzBtCg==\",\"evt.time\":1580811468970787674}",
  "priority": "Notice",
  "rule": "Test renzo",
  "time": "2020-02-04T10:17:48.970787674Z",
  "output_fields": {
    "evt.buffer": "IBtbMG0bWzAxOzM0bURlc2t0b3AbWzBtICAgG1swMTszNG1Eb2N1bWVudHMbWzBtICAgG1swMTszNG1Eb3dubG9hZHMbWzBtICAgG1swMTszNG1nbxtbMG0gICAbWzAxOzM0bVBpY3R1cmVzG1swbSAgIBtbMDE7MzRtUHJvamVjdHMbWzBtICAgG1swMTszNG1WaWRlb3MbWzBtICAbWzAxOzM0bSdWaXJ0dWFsQm94IFZNcycbWzBtCg==",
    "evt.time": 1580811468970787600
  }
}

This is wrong because:

  • Does base64 formatting but the "output" field becomes a JSON string and contains the content of "output_fields";

Output without this change:

{
  "output": "11:22:08.168296981: Notice Yo (error= .[0m.[01;34mDesktop.[0m   .[01;34mDocuments.[0m   .[01;34mDownloads.[0m   .[01;34mgo.[0m   .[01;34mPictures.[0m   .[01;34mProjects.[0m   .[01;34mVideos.[0m  .[01;34m'VirtualBox VMs'.[0m.)",
  "priority": "Notice",
  "rule": "Test renzo",
  "time": "2020-02-04T10:22:08.168296981Z",
  "output_fields": {
    "evt.buffer": " .[0m.[01;34mDesktop.[0m   .[01;34mDocuments.[0m   .[01;34mDownloads.[0m   .[01;34mgo.[0m   .[01;34mPictures.[0m   .[01;34mProjects.[0m   .[01;34mVideos.[0m  .[01;34m'VirtualBox VMs'.[0m.",
    "evt.time": 1580811728168297000
  }
}

This is wrong because

  • No base64

@fntlnz fntlnz mentioned this pull request Feb 4, 2020
@leodido
Copy link
Member

leodido commented Feb 4, 2020

/close in favor of #1033

@leodido leodido closed this Feb 4, 2020
@realgam3 realgam3 deleted the patch-1 branch February 7, 2020 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Falco Base64 doesn't work
4 participants