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 support for optional config file #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

celliott
Copy link

@celliott celliott commented Oct 4, 2021

This changes adds the ability to pass a config file for the tfsec scans. See tfsec config-file docs.

We are using tfsec with cdktf generated tf.json files. tfsec scan does work well but we don't have the ability to add tfsec:ignore:<rule> to the cdktf def or the generated tf.json. We also use a common github actions and don't want to specify overrides using tfsec_exclude. Adding support for a config file gives us the option to pass a config file that lives in the cdktf repo and does allow us to exclude checks. This is an ideal integration for cdktf and tfsec b/c we would prefer not to globally ignore checks and instead use tfsec ignore on specific resources blocks.

@@ -16,6 +16,8 @@ fi

if [[ -n "$INPUT_TFSEC_EXCLUDE" ]]; then
TFSEC_OUTPUT=$(/go/bin/tfsec ${TFSEC_WORKING_DIR} --no-colour -e "${INPUT_TFSEC_EXCLUDE}" ${INPUT_TFSEC_OUTPUT_FORMAT:+ -f "$INPUT_TFSEC_OUTPUT_FORMAT"} ${INPUT_TFSEC_OUTPUT_FILE:+ --out "$INPUT_TFSEC_OUTPUT_FILE"})
elif [[ -n "$INPUT_TFSEC_CONFIG_FILE" ]]; then
Copy link
Owner

Choose a reason for hiding this comment

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

I'm wondering if this should be an elif or a separate option. For me, this is not exclusive to use a parameter or another, therefore I would rather have this option under its own if. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

@triat thx for getting back. I apologize for the slow response. I'm finally getting a chance to circle back to this. The main reason I used the elif was that I followed the same pattern that already existing for INPUT_TFSEC_EXCLUDE. Is this how you'd imagine using config file?

if [[ -n "$INPUT_TFSEC_CONFIG_FILE" ]]; then
  TFSEC_CONFIG_FILE="--config-file ${INPUT_TFSEC_CONFIG_FILE}"

if [[ -n "$INPUT_TFSEC_EXCLUDE" ]]; then
  TFSEC_OUTPUT=$(/go/bin/tfsec ${TFSEC_WORKING_DIR} --no-colour ${TFSEC_CONFIG_FILE} -e "${INPUT_TFSEC_EXCLUDE}" ${INPUT_TFSEC_OUTPUT_FORMAT:+ -f "$INPUT_TFSEC_OUTPUT_FORMAT"} ${INPUT_TFSEC_OUTPUT_FILE:+ --out "$INPUT_TFSEC_OUTPUT_FILE"})
else
  TFSEC_OUTPUT=$(/go/bin/tfsec ${TFSEC_WORKING_DIR} --no-colour ${TFSEC_CONFIG_FILE} ${INPUT_TFSEC_OUTPUT_FORMAT:+ -f "$INPUT_TFSEC_OUTPUT_FORMAT"} ${INPUT_TFSEC_OUTPUT_FILE:+ --out "$INPUT_TFSEC_OUTPUT_FILE"})
fi

Copy link
Owner

Choose a reason for hiding this comment

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

At best, I'd like to stay as close as possible to what TFSEC does. And I believe that you should be able to give any parameters without getting another one magically removed (when this makes sense, of course).

This is why I'd prefer to have ifs that add parameters to the command rather than if ... elses that take whatever first parameter you give to your action. Does this make sense?

This might be wrongly done right now, and would require some refactoring if this is wrong. But to be honest, I'm not using it right now and I'm not spending that much time on open source projects recently. This does not mean I won't do it at some point tho.

@triat
Copy link
Owner

triat commented Oct 18, 2021

Hi @celliott,
Thanks for the PR, it is always appreciated.

I've left you a comment that might require some rework. I can also see that the CI has shown a few things that could be improved if you don't mind changing them.

@celliott
Copy link
Author

celliott commented Dec 7, 2021

Hi @celliott, Thanks for the PR, it is always appreciated.

I've left you a comment that might require some rework. I can also see that the CI has shown a few things that could be improved if you don't mind changing them.

Thx for looking. I made a change that should resolve the CI issue. I also added a note about your other comment. Let me know what you think. thx!

@triat
Copy link
Owner

triat commented Dec 16, 2021

Hi @celliott, Thanks for the PR, it is always appreciated.
I've left you a comment that might require some rework. I can also see that the CI has shown a few things that could be improved if you don't mind changing them.

Thx for looking. I made a change that should resolve the CI issue. I also added a note about your other comment. Let me know what you think. thx!

Replied to you in the comments. Don't worry about the late answer, we're all living our lives and this take time :)

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.

2 participants