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

unmarshal exact - ensuring the config does not contain invalid values #815

Closed
wants to merge 8 commits into from

Conversation

svyotov
Copy link

@svyotov svyotov commented Mar 27, 2022

Description:

Fixes #811. This PR tries to ensure if there are extra invalid values in the git config to fail and not continue silently. Uses viper.UnmarshalExact which uses mapstructure.

UnmarshalExact unmarshals the config into a Struct, erroring if a field is nonexistent in the destination struct.

Checklist:

  • Does your PR pass tests?
  • Have you written new tests for your changes?
  • Have you lint your code locally prior to submission?

@svyotov svyotov changed the title unmarshal strict unmarshal exact - ensuring the config does not contain invalid values Mar 27, 2022
@svyotov svyotov marked this pull request as draft March 27, 2022 10:24
@svyotov svyotov marked this pull request as ready for review March 27, 2022 10:57
@svyotov
Copy link
Author

svyotov commented Mar 30, 2022

@zricethezav Please take a look at this PR, merging it should be straight forwards, and makes configuration much easier, because end users will know when they put invalid/unsupported keys in the config file. Thank you!

@svyotov
Copy link
Author

svyotov commented Apr 20, 2022

@zricethezav any thoughts for this PR, thank you?

@zricethezav
Copy link
Collaborator

@svyotov going to take a look at this soon. Sorry for the delay

@svyotov
Copy link
Author

svyotov commented Sep 7, 2022

Hello @zricethezav! Do you have a verdict for this PR, will it be useful or should we close it?

@zricethezav
Copy link
Collaborator

@svyotov I think I agree with these changes. UnmarshalExact will only throw an error if an extraneous or incorrect entry is discovered, not if one is missing right? For example, most of the rules do not contain path entries

@svyotov
Copy link
Author

svyotov commented Nov 11, 2022

@svyotov I think I agree with these changes. UnmarshalExact will only throw an error if an extraneous or incorrect entry is discovered, not if one is missing right? For example, most of the rules do not contain path entries

I believe so, and also the tests indicate so (error when there was an extra key) and none of the old unit tests failed (some of them must be missing keys).

unmarshalWantError: &mapstructure.Error{Errors: []string{"'Rules[0]' has invalid keys: fake_key"}},

Copy link

@jit-ci jit-ci bot left a comment

Choose a reason for hiding this comment

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

❌ Jit has detected 2 important findings in this PR that you should review.
The findings are detailed below as separate comments.
It’s highly recommended that you fix these security issues before merge.

regex = '''(A3T[A-Z0-9]|AKIA|AGPA|AIDA|AROA|AIPA|ANPA|ANVA|ASIA)[A-Z0-9]{16}'''
tags = ["key", "AWS"]
[rules.allowlist]
regexes = ['''AKIALALEMEL33243OLIA''']
Copy link

Choose a reason for hiding this comment

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

Security control: Secret Detection

Type: Aws-Access-Token

Description: AWS

Severity: HIGH


Jit Bot commands and options (e.g., ignore issue)

You can trigger Jit actions by commenting on this PR review:

  • #jit_ignore_fp Ignore and mark this specific single instance of finding as “False Positive”
  • #jit_ignore_accept Ignore and mark this specific single instance of finding as “Accept Risk”
  • #jit_undo_ignore Undo ignore command

Copy link
Author

Choose a reason for hiding this comment

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

#jit_ignore_fp

RuleID: "aws-access-key",
Allowlist: Allowlist{
Regexes: []*regexp.Regexp{
regexp.MustCompile("AKIALALEMEL33243OLIA"),
Copy link

Choose a reason for hiding this comment

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

Security control: Secret Detection

Type: Aws-Access-Token

Description: AWS

Severity: HIGH


Jit Bot commands and options (e.g., ignore issue)

You can trigger Jit actions by commenting on this PR review:

  • #jit_ignore_fp Ignore and mark this specific single instance of finding as “False Positive”
  • #jit_ignore_accept Ignore and mark this specific single instance of finding as “Accept Risk”
  • #jit_undo_ignore Undo ignore command

Copy link
Author

Choose a reason for hiding this comment

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

#jit_ignore_fp

@svyotov
Copy link
Author

svyotov commented Nov 11, 2022

Hello, I have re-based, added a test for missing keys, and made a change to the previous test to work with the new schema.
I did it fast, so please be thorough when reviewing. Thank you.

@svyotov
Copy link
Author

svyotov commented Nov 11, 2022

@zricethezav please let me know if I need to do anything about the current failures in the CI. I do want to help get this in - if it is useful.

@svyotov
Copy link
Author

svyotov commented Nov 11, 2022

To be clear though, this will be a breaking change, as anyone having invalid values in their config - values which up to now where ignored - will get errors from the tool, so the version change for this release should be minor /major and not patch.

@svyotov
Copy link
Author

svyotov commented May 4, 2023

closing this due to lack of interest/response.

@svyotov svyotov closed this May 4, 2023
@svyotov svyotov deleted the unmarshal-strict branch May 4, 2023 10:15
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.

gitleaks ignores bad fields in the configuration silently
2 participants