Remove automatic redaction of header fields

[simonjbeaumont]

During an unrelated PR on the runtime repo, there was some discussion about HeaderField.redactedHeaderFields and whether it's something we should keep:

Original discussion here: https://github.com/apple/swift-openapi-runtime/pull/22/files#r1259948110

My 2¢ is that this is just a datatype and that overriding CustomStringConvertible to redact certain headers is a bit of a layering violation.

My concern is that by providing this functionality implicitly, we also imply that it's safe by default, but in reality it's best-effort at best, using a few "well-known" headers.

For folks running production servers I would imagine they'd have policies around logging the headers anyway, for this reason, and we can accomplish the same functionality with a middleware, with explicit API.

If we think this is important enough to do at the CustomStringConvertible layer, then I think I'd prefer adopters provide the keys they want redacting explicitly in the configuration.

//cc @czechboy0 and @FranzBusch who were on the linked discussion thread.

[FranzBusch]

I am leaning towards just implementing CustomStringConvertible where it includes all the information in the description. If a user is going to log a whole request then they are responsible for potentially logging PII. They can work around this by just logging specific parts of the request where they know it is safe to do so.

As long as we don't log anything inside one of the OpenAPI libraries then we should be good.

[czechboy0]

I'd prefer we keep this feature, but of course I'm more than happy to discuss the exact spelling.

Logging PII and logging an access token are still pretty distinct issues, and this feature is here to avoid accidentally logging tokens, either on the client, or the server. PII applies to practically all logging with dynamic values.

Why I think it's justified to have this feature is because every adopter, again both client or server, who uses authentication, will otherwise have to implement a whole graph of CustomStringCovertible on all the currency types. If we can provide a reasonable default, like we do today, but also allow customization, I think we have the right balance here - make simple things easy (not accidentally log access tokens), and difficult things possible (customize the header names).

Now, if the desire here is mainly about the public API, I think it's reasonable to discuss removing the customization option, and always use the 3 header names we have today. And if someone puts auth tokens into a non-standard header, then it's reasonable to also ask them to implement their own logging.

But I still feel that if an adopter goes through our tutorial, does a very common thing (puts the auth token into Authorization), and prints every request and response in a middleware, we should omit the sensitive headers.

Or, build a logging middleware that anyone can use, but that's create a dependency on swift-log from the runtime library, and we've been trying to keep the runtime library dependencies only to essential ones.

So, there are different avenues we can go if folks don't like the implementation today, but I don't think we should just remove the feature without a replacement, as it's been discussed on multiple occasions that the problem is solves is justified.

[FranzBusch]

But I still feel that if an adopter goes through our tutorial, does a very common thing (puts the auth token into Authorization), and prints every request and response in a middleware, we should omit the sensitive headers.

I disagree with this. In my opinion, this is not our responsibility and it would be wrong to just redact stuff from a description of a type. The end user of an application is responsible of what he logs and only he knows what is PII and what not. If he builds he middleware that just logs every request then he opts into logging PII. Just because we redact some headers doesn't mean no other PII is getting logged. They could still be part of the URL path or another header.

Moreover, we don't do any such redaction in our other Swift on Server libraries and always delegate this to the end users responsibility to know what they can and can't log. I also can't think of a server framework in a different language that offers this.

If we would be logging the request/response ourselves then making the whole log configurable would be something I understand but trying to offer a very limited filtering capability on headers seems unjustified to me.

So, there are different avenues we can go if folks don't like the implementation today, but I don't think we should just remove the feature without a replacement, as it's been discussed on multiple occasions that the problem is solves is justified.

IMO we are still in a phase where we can remove such features before we 1.0.0. This feature is something that users can implement themselves outside of the library and isn't tied to the core logic of this library.

[simonjbeaumont]

but I don't think we should just remove the feature without a replacement, as it's been discussed on multiple occasions that the problem is solves is justified.

I think we shouldn't assume that everything that's in the codebase today is justified purely by construction and FWIW I'm willing to entertain we remove things if they don't fit with the ecosystem or community requirements.

[czechboy0]

I'm willing to make any changes that we feel improve the adopter experience. But we also took some care coming up with the current state of things and I feel there should be more consideration about what this means for adopters, not just maintainers. I agree with all the benefits you point out here for maintainers, and as a maintainer, I see their appeal (less code to maintain is always better). But as an adopter, I know I'll now have to repeat the same work in every project, which is why I'm pushing back.

And I'm particularly sensitive to forcing almost every adopter to repeat the same work, especially when the cost to maintain this feature is low, and has a reasonable default. If we imagine 100 adopters needing to solve this problem, of logging requests and responses, do we feel that all of them will want to keep and redact pretty different pieces of data? I don't, I suspect most of the logging functions would do what we do today, just redact auth headers (again, PII is separate from auth tokens, and is governed by different rules, this feature is not about PII), and I've definitely seen header redaction with exactly these 3 headers in various projects, in different languages.

That's why I'm happy to entertain other solutions to this problem, but I continue to think this is a real, common problem that we should help adopters with. Otherwise we increase the bar of the work needed by adopters to have reasonable logging.

To be fair, I think we're also in a bit of a unique position here, since we don't vend just a library, but generate code, so the line between maintainer code and adopter code is a bit more blurry than with libraries. Not to say we shouldn't use existing work for inspiration, just that it's not a 1:1 mapping.

[FranzBusch]

I do look at this through the lens of both a maintainer and an adopter and in my opinion we are doing a disservice on both ends.
As a maintainer we are adding global state to our code base which needs to go through a lock and also forces every single call to description to go through a lock. Just because somebody might update the redacted headers during run time. This is already a pretty big code smell.

As an adopter, we are making an API visible to redact some headers but it only caters to the PII use-case where I have to redact headers; however, this is far from the truth when it comes to redacting PII in request/responses. Additionally, it gives me a false sense of safety since if I just make sure that I redact all my headers then I should be good with logging requests/responses which I should mostly never do unless I am debugging. Lastly, the API is pretty hard to discover due to its global nature.

In the end, we are doing a pretty unique thing here which is providing a global setting to influence the behaviour of a method on types. Moreover, this method is called by adopters in the end so this isn't event a toggle for changing behaviour in our internals and they could very well do it themselves.

[czechboy0]

Ok, let's remove the feature. The new swift-http-types have no affordance for doing this, and trying to maintain this feature would probably get very expensive once we move to those new types.

A PR would be appreciated here. Since this is API, we'll first need to deprecate it in this current version, and we'll remove the API in the next breaking version.

[czechboy0]

Step 1: Deprecation in 0.1.x: apple/swift-openapi-runtime#32

[czechboy0]

Step 2 will be removal in 0.2.0.