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

feat: add build command #19

Merged
merged 1 commit into from
Aug 1, 2024
Merged

feat: add build command #19

merged 1 commit into from
Aug 1, 2024

Conversation

alegrey91
Copy link
Owner

@alegrey91 alegrey91 commented Aug 1, 2024

Closes #9
This PR introduce the new build command that allows the user to create seccomp profiles from the collected syscalls.

Signed-off-by: Alessio Greggi <ale_grey_91@hotmail.it>
@alegrey91 alegrey91 marked this pull request as ready for review August 1, 2024 21:24
@alegrey91 alegrey91 merged commit bc4103d into main Aug 1, 2024
Copy link
Collaborator

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

Here are a few suggestions

Why did you merge so quickly?

var (
inputDirectory string
saveProfile bool
profileName = "seccomp.json"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one could/should be a const


syscalls := make([]string, 0)
for _, fileObj := range files {
//fmt.Println("[" + fileObj.Name() + "]")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a left behind debug

for _, fileObj := range files {
//fmt.Println("[" + fileObj.Name() + "]")

file, err := os.Open(inputDirectory + "/" + fileObj.Name())
Copy link
Collaborator

@ccoVeille ccoVeille Aug 1, 2024

Choose a reason for hiding this comment

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

Please use filepath.Join, do not convact things like that

}
defer profileFile.Close()

if err := profileFile.Chmod(0644); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if err := profileFile.Chmod(0644); err != nil {
if err := profileFile.Chmod(0o644); err != nil {

Use the octal notation

fmt.Printf("error setting permissions to %s: %v\n", profileFile.Name(), err)
}
// write to file
fmt.Fprintln(profileFile, profile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fmt.Fprintln(profileFile, profile)
err := fmt.Fprintln(profileFile, profile)
if err != nil {
// log something
}

defer profileFile.Close()

if err := profileFile.Chmod(0644); err != nil {
fmt.Printf("error setting permissions to %s: %v\n", profileFile.Name(), err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we return, here?


profile, err := seccomp.BuildProfile(syscalls)
if err != nil {
fmt.Printf("error building seccomp profile: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

No return here?

How could we continue if the profile cannot be built?

@alegrey91
Copy link
Owner Author

Hi @ccoVeille,
I'm happy this project got your interest 🙂
The reason why I merged the PR so quickly is that I need this functionality asap.
I really appreciate your review, so I'm going to open a new PR, in the upcoming days, to fix all the bad things you highlighted in the code (or if you want to contribute, feel free to open your own PR).
Thanks for your interest. I'm happy to see that someone care about the project ;)
Next time I could ask your review if you give your availability.

@ccoVeille
Copy link
Collaborator

I really appreciate your review, so I'm going to open a new PR, in the upcoming days, to fix all the bad things you highlighted in the code (or if you want to contribute, feel free to open your own PR).

I find bavk our conversation in my saved item.

Did you fix all the things I reported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement command to create seccomp profile
2 participants