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

Adding support for privacy level in string interpolation #204

Open
Frizlab opened this issue Aug 5, 2021 · 12 comments
Open

Adding support for privacy level in string interpolation #204

Frizlab opened this issue Aug 5, 2021 · 12 comments

Comments

@Frizlab
Copy link

Frizlab commented Aug 5, 2021

On macOS, the (Apple’s) Logger object uses a custom OSLogMessage object which allows it to have a custom string interpolation with support for format arguments privacy levels.

Would it be possible to add support for said string interpolation so we could finally have an efficient and fully feature-compatible OSLog Logger for swift-log?

Thanks!

@shaps80
Copy link

shaps80 commented Sep 22, 2021

@Frizlab – While not exactly what you're asking for, I have written something like this and mentioned it on another issue #208 to see if this is appropriate for this package. If not, it's still usable from my own package. Feel free to use.

@nashysolutions
Copy link

I noticed some advice regarding privacy here. Would be great to have a section in the docs about this.

@tomerd
Copy link
Member

tomerd commented Jan 4, 2023

That is a good idea @nashysolutions. would you or @Frizlab like to make a PR with proposed language? I suspect we may also want to consider a new metadata type for sensitive data that the log backend can reduct

@Mordil
Copy link

Mordil commented Jun 28, 2023

We've implemented a similar mechanism in our local code - is this something that might be generally useful?

extension Logger.MetadataValue {
  /// The different styles of privacy masks that can be applied to a metadata value.
  internal enum PrivacyMask {
    /// The value will be hashed and converted into an 8 character hex representation.
    case hash
    /// Replaces the value with `*****`.
    case redact
  }

  /// Replaces the value with `*****`.
  ///
  /// This is great for when the actual value is not of importance and should be masked when not debugging.
  ///
  /// For example:
  ///
  ///     let logger: Logger = ...
  ///     logger.info(
  ///       "replaced value with mask pattern",
  ///       metadata: [
  ///         "password": .mask(user.password)
  ///       ]
  ///     )
  public static func mask(_ value: Any) -> Logger.MetadataValue {
    return .stringConvertible(MaskedValue(value, mask: .redact))
  }

  /// The value will be hashed and converted into an 8 character hex representation.
  ///
  /// This is great for when identity of value between individual logs is important, but the actual value should still be masked.
  ///
  /// For example:
  ///
  ///     let logger: Logger = ...
  ///     logger.info(
  ///       "obfuscated value",
  ///       metadata: [
  ///         "ip_address": .hash(user.ipAddress)
  ///       ]
  ///     )
  public static func hash(_ value: Any) -> Logger.MetadataValue {
    return .stringConvertible(MaskedValue(value, mask: .hash))
  }
}

internal struct MaskedValue: CustomStringConvertible {
  internal let underlying: Any

  private let mask: Logger.MetadataValue.PrivacyMask

  internal var description: String {
    switch mask {
    case .hash: return self.hash("\(self.underlying)")
    case .redact: return "*****"
    }
  }

  fileprivate init(_ value: Any, mask: Logger.MetadataValue.PrivacyMask) {
    self.underlying = value
    self.mask = mask
  }

  private func hash(_ value: String) -> String {
    var hasher = Hasher()
    hasher.combine(value)
    return "\(String(format: "%x", hasher.finalize()))"
  }
}

@tomerd
Copy link
Member

tomerd commented Jun 28, 2023

this seems interesting to me. @ktoso? @weissi?

@Mordil
Copy link

Mordil commented Jun 29, 2023

Here's an example of the LogHandler transforming the metadata

switch metadataValue {
case let .stringConvertible(maskedValue as MaskedValue) where self.logLevel <= .debug:
      return "\(maskedValue.underlying)"

default: return "\(metadataValue)"
}

@shaps80
Copy link

shaps80 commented Jul 3, 2023

@Mordil @tomerd @Frizlab I actually open sourced exactly this around 2 years ago. It was discussed with the team here and on Swift Forums but was deemed inappropriate for this library.

So I pulled swift-log and then added the interpolations for similar convenience. It's been used in several projects since its release and I haven't had any reported issues.

It obviously doesn't have the same performance characteristics in production builds, since OSLog only executes its closures when logs are being pulled into Console.app or Xcode. It uses print which most other logging libraries do, so I don't see this as being an issue. Just full disclosure 👍

https://github.com/shaps80/Logging

@Mordil
Copy link

Mordil commented Jul 11, 2023

@shaps80 Do you have links to the discussions where it was determined this was inappropriate for the library to offer directly?

@shaps80
Copy link

shaps80 commented Jul 12, 2023

Yeah, https://forums.swift.org/t/swiftlog-stringinterpolation/52313/3
You actually were one of the last comments agreeing to this hahaha.

On this repo there was also: #208

@weissi
Copy link
Member

weissi commented Jul 12, 2023

@tomerd For apps I think adding privacy works just fine. For libraries it's IMHO not possible to sensibly annotate. Take a URL, some apps have sensitive URLs, other apps don't but the HTTP client library couldn't know how it's used.

My personal opinion is: If you need privacy, then

  • stick everything that's not constant into metadata (e.g. logger.info("user logged in", metadata: ["user-name": "\(userName)", "request-uuid": "\(request.uuid)"]))
  • in your LogHandler, redact every metadata value that isn't specifically allow listed

So the above message (logger.info("user logged in", metadata: ["user-name": "\(userName)", "request-uuid": "\(request.uuid)"])) should IMHO be logged as

user logged in [user-name: <redacted>, request-uuid: 1F4A3360-AEEC-48A4-92E4-60DA38823004]

because an IMHO sensible LogHandler configuration would (for a hypothetical SuperCoolPrivacyLogHandler) could look something like

SuperCoolPrivacyLogHandler(
    label: ...,
    configuration: SuperCoolPrivacyLogHandlerConfiguration(
        nonRedactedMetadataKeysList: ["request-uuid", "error", "timestamp", "count", "ahc-error", "ahc-connection-id", ...]
    )
)

The reason is that IMHO only the application can know what has PII and what doesn't. If the HTTP client is only used for requests that never contain PII, then it would be safe to add ahc-url or whatever into the nonRedactedMetadataKeysList but for many other apps that wouldn't be acceptable at all.


If it becomes too tedious to list each and every field, then I would suggest a convention for your application: For example if you say that every metadata key that starts with public: is never redacted. So logger.info("user logged in", metadata: ["user-name": "\(userName)", "public:user-id-hash": "\(sha256(userID))"] or something.


I'm guessing the tl;dr is:

  • swift-log is deliberately versatile and unopinionated. If you want to implement special privacy controls go nuts in your LogHandler
  • I don't think PII can be decided on the log message production side (i.e. the libraries that log). It has to be decided by a central point that is controlled by the application as only the application understands what is and what isn't PII in its domain (might even depend by country and what not)

@tomerd
Copy link
Member

tomerd commented Jul 13, 2023

one thing to keep in mind is that the application is also a "producer" log events. its makes perfect sense to me that libraries do not force PII decision on the app, but its nice for the apps to be able to emit ones taking into account PII considerations. that said, I can see how this can be handled in a sophisticated enough handler / backend with nice configuration for this kind of settings

@weissi
Copy link
Member

weissi commented Jul 13, 2023

one thing to keep in mind is that the application is also a "producer" log events. its makes perfect sense to me that libraries do not force PII decision on the app, but its nice for the apps to be able to emit ones taking into account PII considerations. that said, I can see how this can be handled in a sophisticated enough handler / backend with nice configuration for this kind of settings

Totally agree with everything. I guess I was trying to say doing something that adds privacy for the app only but ignores the libraries isn't any good. And yes, sophisticated LogHandlers are the way. I wish there was a properly configurable and featureful "logging middle-end" that would take care of these things once and for all.

fumoboy007 added a commit to fumoboy007/swift-retry that referenced this issue Jun 5, 2024
The retry implementation cannot know whether or not the error contains private information, so it should log the error and allow the user of the library to decide. As detailed [here](apple/swift-log#204 (comment)), the log handler can redact metadata values as needed.
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

6 participants