opentelemetry-collector
opentelemetry-collector copied to clipboard
[proposal] Add string type alias for sensitive fields
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 https://github.com/open-telemetry/opentelemetry-collector/issues/5615#issuecomment-1172609483). 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 https://github.com/open-telemetry/opentelemetry-collector/issues/5223#issuecomment-1099957554) 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:
- Struct tags can be applied to fields of any type instead of only strings (so you can censor integers, enums... without introducing new types).
-
OpaqueString
allows for more granular types: for a field likeHeaders
, we can change it tomap[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. - Adding a type alias implies some breakage: while you can set
a.OpaqueField = <string>
, you can't passa.OpaqueField
to afunc 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.
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.
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.
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.
Closing this since configopaque
is now available. #6851 tracks adoption of the package on core and contrib configuration structs.