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

[proposal] Add string type alias for sensitive fields #5653

Closed
mx-psi opened this issue Jul 8, 2022 · 4 comments
Closed

[proposal] Add string type alias for sensitive fields #5653

mx-psi opened this issue Jul 8, 2022 · 4 comments
Labels
area:config enhancement New feature or request

Comments

@mx-psi
Copy link
Member

mx-psi commented Jul 8, 2022

Is your feature request related to a problem? Please describe.

There have been several asks for some way of printing the full Collector configuration (see #5223, #1791 and #5615 (comment)). This will also be more important as the ability to merge multiple configuration files and use different configuration providers gains user adoption. To support this we need a way to censor sensitive information (tokens, passwords, API keys...) before printing the config.

In principle, we only need to censor strings (numbers, enums, booleans are generally not sensitive).

Describe the solution you'd like

Introduce a string type alias that must be used for sensitive fields:

type OpaqueString string

var _ encoding.TextMarshaler = (*OpaqueString)(nil)

func (s OpaqueString) MarshalText() ([]byte, error) {
	return bytes.Repeat([]byte("*"), len(s)), nil
}

This censors fields when marshaling and is transparent when unmarshaling (playground) while allowing to retrieve the value if casting to string.

Since it is an alias the casting is implicit in some cases (playground), therefore the breakage implied by changing a field from string to OpaqueString is reduced (it's still is a breaking change though).

Describe alternatives you've considered

Originally (see #5223 (comment)) I considered doing this via a struct tag (sensitive=true/false). There are some tradeoffs between the OpaqueString approach when compared with using struct tags:

  1. Struct tags can be applied to fields of any type instead of only strings (so you can censor integers, enums... without introducing new types).
  2. OpaqueString allows for more granular types: for a field like Headers, we can change it to map[string]OpaqueString, signaling that while the header names are not sensitive, its values are. With struct tags we would have an all-or-nothing situation: we either censor all the headers information or none of it.
  3. Adding a type alias implies some breakage: while you can set a.OpaqueField = <string>, you can't pass a.OpaqueField to a func f(string) function without explicit casting. Struct tags do not imply a breaking change of any kind.

(1) doesn't feel like a big deal to me; if the need arises for censoring other kinds of fields, we can always add more types; but I think (2) is a strong argument in favor of doing it via the OpaqueString approach. For (3), I think the breakage implied by this is minimal and we should just change fields without a previous deprecation.

Additional context

With this approach we are trusting component developers to correctly use the type on their fields. We could instead censor strings by default except for fields explicitly declared as 'safe to print'. This is an open question, and it implies a tradeoff between implementation hassle (there are much fewer sensitive than non-sensitive fields) and security.

@mx-psi mx-psi added enhancement New feature or request area:config labels Jul 8, 2022
@dyladan
Copy link
Member

dyladan commented Jul 8, 2022

With this approach we are trusting component developers to correctly use the type on their fields. We could instead censor strings by default except for fields explicitly declared as 'safe to print'. This is an open question, and it implies a tradeoff between implementation hassle (there are much fewer sensitive than non-sensitive fields) and security.

The safe by default method would be compatible with existing implementations which may already have secrets in unprotected string fields, but I think it would trash the value until everything is converted to be marked safe. Most fields should be safe so I would expect it to be not too difficult to go through contrib once and audit each module and mark unsafe strings. I'm happy to help with that process.

@mx-psi
Copy link
Member Author

mx-psi commented Jul 13, 2022

From the SIG meeting: the community prefers doing struct tags to avoid breaking changes. I will come up with a prototype to evaluate this solution.

@bogdandrutu
Copy link
Member

I think that it "breaking" is very minimal, and is part of an API that we don't expect users to use publicly component.Config object, which is public just because of the yaml unmarshalling limitations.

@mx-psi
Copy link
Member Author

mx-psi commented Dec 23, 2022

Closing this since configopaque is now available. #6851 tracks adoption of the package on core and contrib configuration structs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants