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

Should configopaque.String implement encoding.BinaryMarshaler? #9272

Closed
Tracked by #9167
mx-psi opened this issue Jan 12, 2024 · 4 comments · Fixed by #9279
Closed
Tracked by #9167

Should configopaque.String implement encoding.BinaryMarshaler? #9272

mx-psi opened this issue Jan 12, 2024 · 4 comments · Fixed by #9279

Comments

@mx-psi
Copy link
Member

mx-psi commented Jan 12, 2024

It would be easy to do (e.g. here is the implementation for net.URL, ours would just be returning []bytes(maskedString), nil).

Any reason not to do it?

@bogdandrutu
Copy link
Member

I don't have a reason to not to, but I do not have a reason on why to. So my rule of thumb when in doubt leave it out :)

@TylerHelmuth
Copy link
Member

If this statement is true, then I do think we'd want to prevent this situation. If the goal of configopaque.String is to hide the value unless the user explicitly decides they want to see it, then I think the roundabout situation of configopaque -> []byte -> string is a leak.

@TylerHelmuth
Copy link
Member

If we chose to do this: #9279

@mx-psi
Copy link
Member Author

mx-psi commented Jan 15, 2024

I don't have a reason to not to, but I do not have a reason on why to. So my rule of thumb when in doubt leave it out :)

Two arguments in favor:

mx-psi pushed a commit that referenced this issue Jan 19, 2024
#9279)

**Description:** <Describe what has changed.>
Implements the `encoding.BinaryMarshaler` interface for `String`. This
prevents the situation `configopaque` -> `[]byte` -> `string` from
leaking the `String` value.

**Link to tracking Issue:** <Issue number if applicable>
Closes
#9272

**Testing:** 
Added unit test

---------

Co-authored-by: Antoine Toulme <antoine@toulme.name>
TylerHelmuth added a commit to TylerHelmuth/opentelemetry-collector that referenced this issue Jan 23, 2024
open-telemetry#9279)

**Description:** <Describe what has changed.>
Implements the `encoding.BinaryMarshaler` interface for `String`. This
prevents the situation `configopaque` -> `[]byte` -> `string` from
leaking the `String` value.

**Link to tracking Issue:** <Issue number if applicable>
Closes
open-telemetry#9272

**Testing:** 
Added unit test

---------

Co-authored-by: Antoine Toulme <antoine@toulme.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants