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

Pretty-printer with redacted/hidden passwords #135

Open
hcarty opened this issue Mar 22, 2019 · 5 comments
Open

Pretty-printer with redacted/hidden passwords #135

hcarty opened this issue Mar 22, 2019 · 5 comments

Comments

@hcarty
Copy link
Contributor

hcarty commented Mar 22, 2019

Hiding passwords in a pretty-printer is useful for logging. It may be worth having passwords hidden by default, with a way to opt-in to password printing.

A few possible options:

(* Fields we can redact when printing *)
type field =
  | Username
  | Password
  | Query of [`Full | `Values]
  | And_so_on

(* Opt-out of printing specific fields, replacing them with generic filler text.
   [redact] could default to [[Username; Password; Query `Values]] in a
   paranoid world. *)
val pp_redacted : ?redact:field list -> unit -> t Fmt.t

(* Opt-in to password printing - with_password would default to [false]. *)
val pp_passwordless : t Fmt.t

(* Just strip out everything *)
val pp_paranoid : t Fmt.t
@avsm
Copy link
Member

avsm commented Mar 24, 2019

I agree we should redact the passwords by default.

@hcarty
Copy link
Contributor Author

hcarty commented Mar 24, 2019

Should we change the behavior of pp and add a separate, more customizable pretty printer as well? I'd be happy to make a PR for this once an approach is decided on.

@avsm
Copy link
Member

avsm commented Jul 9, 2019

Given the existence of GDPR, the best approach is probably to default to not printing the username and password in pp. How about just a single function with the signature of pp_redacted above with a ?redact=[Username;Password] default?

@hcarty
Copy link
Contributor Author

hcarty commented Jul 11, 2019

I started looking at this a bit and have a few API design questions/thoughts.

I think `User and `Password constructors should be added to the component type rather than adding the redundant field type I suggested in the original issue text.

Rather than adding a pp_redacted function, I think we should make the components to redact a global ref and always use that ref's contents to determine the printing behavior in pp. So pp would act like pp_redacted as originally described but ?redact would be pulled from a (spooky!) global ref rather than a function argument.

As pure evil as global references may be, the ref-based approach does have the benefit of being accessible without having to grep/sed for all potential uses of Uri.pp.

The interface additions would be:

(** `User and `Password would be added to the component type definition *)
(** {!pp}'s behavior would silently change to pull from the set of redacted components when printing *)

(** Set of redacted components when printing with {!pp}.  Can be changed with {!add_redacted} and {!remove_redacted}.  Defaults to [`User] and [`Password]. *)
val redacted : unit -> component list

(** [add_redacted component] adds [component] to the set of redacted components when printing with {!pp}. *)
val add_redacted : component -> unit

(** [remove_redacted component] removes [component] from the set of redacted components when printing with {!pp}. *)
val remove_redacted : component -> unit

@talex5
Copy link

talex5 commented Jul 13, 2019

It would be better to mark Uri.pp as deprecated; then the compiler can find all the uses for you. Different callers will need different parts redacted, so having a global for this doesn't seem useful. For example, when a program prints out a message on first run telling you to visit an admin URL to configure the server, the URL printed must include all secret components.

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

No branches or pull requests

3 participants