ocaml-uri icon indicating copy to clipboard operation
ocaml-uri copied to clipboard

Pretty-printer with redacted/hidden passwords

Open hcarty opened this issue 5 years ago • 5 comments

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

hcarty avatar Mar 22 '19 23:03 hcarty

I agree we should redact the passwords by default.

avsm avatar Mar 24 '19 21:03 avsm

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.

hcarty avatar Mar 24 '19 23:03 hcarty

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?

avsm avatar Jul 09 '19 03:07 avsm

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

hcarty avatar Jul 11 '19 00:07 hcarty

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.

talex5 avatar Jul 13 '19 08:07 talex5