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

Added event for default rule "Read SSH Information" #112

Merged
merged 9 commits into from
Apr 2, 2024

Conversation

GLVSKiriti
Copy link
Contributor

What type of PR is this?

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

/kind bug

/kind cleanup

/kind documentation

/kind tests

/kind feature

Any specific area of the project related to this PR?

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

/area commands

/area pkg

/area events

What this PR does / why we need it:
Added event for default rule "Read SSH Information"

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@GLVSKiriti
Copy link
Contributor Author

It is also working as expected! Error log is produced on left terminal as expected

Screenshot from 2024-03-23 12-55-52

@GLVSKiriti GLVSKiriti requested a review from FedeDP March 27, 2024 09:04
events/syscall/utils_linux.go Outdated Show resolved Hide resolved
events/syscall/read_ssh_information.go Outdated Show resolved Hide resolved
@GLVSKiriti GLVSKiriti requested a review from leogr March 27, 2024 13:45
@GLVSKiriti
Copy link
Contributor Author

GLVSKiriti commented Mar 28, 2024

@FedeDP Kindly tell me if there are any changes!

if err != nil {
return err
}
sshDir := filepath.Join(tempDirectoryName, ".ssh")
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this line right? Since createSshDirectoryUnderHome already creates .ssh folder inside the temp folder it can just return sshDir!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FedeDP Yes we can do that! But before the function return we should also clean the temp folder created by createSshDirectoryUnderHome .
defer os.RemoveAll(tempDirectoryName)
So if we return sshDir instead of tempDir . How can we clean tempDir ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if we return sshdir then one valid approach is trimming the last 5 characters in sshDir gives tempdir path. Is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

I got it, your are right! Nope, i'd leave another comment on the helper function!

return "", err
}

return tempDirectoryName, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return tempDirectoryName, nil
return sshDir, nil

Copy link
Contributor

Choose a reason for hiding this comment

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

I got #112 (comment), what if this function also returns a cleanup function?, like:

return sshDir, func() { _ = os.RemoveAll(tempDirectoryName) }, nil

then the caller will just defer cleanup() after it captures the result, of course if cleanup != nil.
This is often used in go :)

Copy link
Contributor Author

@GLVSKiriti GLVSKiriti Apr 2, 2024

Choose a reason for hiding this comment

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

Yepp!! Awesome
Just did that and tested it its working absolutely fine 💯

@GLVSKiriti GLVSKiriti requested a review from FedeDP April 2, 2024 09:03
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

I think in the future we might want to make createSshDirectoryUnderHome more reusable (by eg: taking a new parameter fileName string in order to allow it to be used to create any file in a temporary user folder directory).
For now, it LGTM!

@poiana
Copy link

poiana commented Apr 2, 2024

LGTM label has been added.

Git tree hash: 9dbddf5a266c60ade1af9fcf089d373a940f9ed7

@poiana
Copy link

poiana commented Apr 2, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, GLVSKiriti

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

@poiana poiana added the approved label Apr 2, 2024
@poiana poiana merged commit cb4f3c9 into falcosecurity:main Apr 2, 2024
4 checks passed
@GLVSKiriti
Copy link
Contributor Author

/approve

I think in the future we might want to make createSshDirectoryUnderHome more reusable (by eg: taking a new parameter fileName string in order to allow it to be used to create any file in a temporary user folder directory).
For now, it LGTM!

Yess certainly It would be very useful

@GLVSKiriti GLVSKiriti deleted the ReadSSHInformation branch April 2, 2024 12:53
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.

None yet

4 participants