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 Secret Filtering component #1593

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

romain-gaillard
Copy link

@romain-gaillard romain-gaillard commented Aug 30, 2024

PR Description

This PR adds a secret filtering component for Loki into Alloy. It is based on the work we have done in the August 2024 hackathon.

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@CLAassistant
Copy link

CLAassistant commented Aug 30, 2024

CLA assistant check
All committers have signed the CLA.

@romain-gaillard romain-gaillard self-assigned this Sep 3, 2024
@kelnage kelnage self-requested a review September 5, 2024 07:40
@romain-gaillard romain-gaillard changed the title Secret Filtering component for Alloy feat: Add Secret Filtering component Sep 5, 2024
@romain-gaillard romain-gaillard marked this pull request as ready for review September 6, 2024 16:39
//
// For the first case, we can replace the entire match with the redaction string.
// For the second case, we can replace the first submatch with the redaction string (to avoid redacting something else than the secret such as delimiters).
for _, occ := range r.regex.FindAllStringSubmatch(entry.Line, -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think we can split this block into a separate func, lots of deep nesting going on.

Copy link
Author

Choose a reason for hiding this comment

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

Changed in d8fa6e5.

}

// Update implements component.Component.
func (c *Component) Update(args component.Arguments) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update should likely include the work your doing in new, ie update should handle them changing the toml file or any other field. So maybe a shared func for recreating the internal representation.

Copy link
Author

Choose a reason for hiding this comment

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

Changed in d8fa6e5.

Actually, because New(...) calls Update(...) anyway, I moved the code there so that it's run both when the component is created and updated 😄

Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

Some initial (minor) doc suggestions

Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

Some initial (minor) doc suggestions

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Sep 6, 2024
@romain-gaillard
Copy link
Author

Some initial (minor) doc suggestions

Thanks a lot! I addressed your suggestions in 500891f 😄

}
// If allowed, skip redaction
if allowRule != nil {
level.Info(c.opts.Logger).Log("msg", "secret in allowlist", "rule", r.name, "source", allowRule.Source)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be on "Debug" level


func init() {
component.Register(component.Registration{
Name: "loki.secretfilter",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add live debugging support? See this PR on how to add it: https://github.com/grafana/alloy/pull/1304/files
It would be nice for users to see the affected log lines before and after the process to make sure that the redaction is working as expected for them. Of course, if they expose the endpoint in prod they should not let the live debugging enabled but there are warnings in the doc about this


{{< docs/shared lookup="stability/experimental.md" source="alloy" version="<ALLOY_VERSION>" >}}

`loki.secretfilter` receives log entries and redacts sensitive information, such as secrets, from them.
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to have a little bit more information on how the redaction is performed. It "only" redacts sensitive information that matches the provided regex (custom or the one in the embedded file). It's possible that sensitive information still passes through the net; users should be warned not to rely blindly on the component.

for _, t := range c.args.Types {
if strings.HasPrefix(strings.ToLower(rule.ID), strings.ToLower(t)) {
found = true
continue
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
continue
break

secret = occ[r.secretGroup]
} else {
// If not and there are two submatches, the first one is the secret
if len(occ) == 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this just be an "else if" ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants