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

Hide body and headers by default in `HttpRequest#toString` and `HttpResponse#toString` #2560

Open
wants to merge 6 commits into
base: master
from

Conversation

@pfcoperez
Copy link

commented Jun 6, 2019

Purpose

Aimed to avoid PII and SPI leakage, closes #2409

References

References #2412
References #2409 (comment)
References #2412 (review)

Changes

  • Makes HttpResponse#toString and HttpRequest#toString omit bodies and headers from the default string representation thus avoiding potential PII and SPI in logs and outputs.
  • Adds documentation and examples explaining how to use type classes to expand the trimmed toString behaviour.

Background Context

It tackles the same problem as #2412 but with a different approach: Default to not showing and show via additional behaviour (e.g using type classes). Also, this PR includes the documentation to add this extra behaviour as well as some examples.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@pfcoperez pfcoperez force-pushed the pfcoperez:feature/2409/trim_httpmessage_tostring branch from f00c9c4 to 7ce919a Jun 6, 2019
@pfcoperez pfcoperez changed the title Hide body and headers by default in `HttpRequest#toString` and `HttpR… Hide body and headers by default in `HttpRequest#toString` and `HttpResponse#toString` Jun 6, 2019
@raboof

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

OK TO TEST

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

Test FAILed.

@pfcoperez pfcoperez force-pushed the pfcoperez:feature/2409/trim_httpmessage_tostring branch from 7ce919a to cfd2e22 Jun 6, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

Test PASSed.

Copy link
Member

left a comment

I like the change, but I'm not sure about what to show in the documentation.

docs/src/main/paradox/common/http-model.md Outdated Show resolved Hide resolved
@@ -125,7 +127,7 @@ object Dependencies {

lazy val httpJackson = l ++= Seq(jackson)

lazy val docs = l ++= Seq(Docs.sprayJson, Docs.gson, Docs.jacksonXml, Docs.reflections)
lazy val docs = l ++= Seq(Docs.sprayJson, Docs.gson, Docs.jacksonXml, Docs.reflections, Docs.cats, Docs.scalaz)

This comment has been minimized.

Copy link
@raboof

raboof Jun 17, 2019

Member

I'm not sure we want to show cats/scalaz in our documentation - people who are using that probably already know how to create such typeclasses?

This comment has been minimized.

Copy link
@pfcoperez

pfcoperez Jun 19, 2019

Author

Should I remove Scalaz and Cats examples from HttpRequestShow and HttpResponseShow. I kind of like the idea of having both options explained:

Some Cats and Scalaz users are unaware of the Show[T] typeclass.

Non-users might be completely unaware of the fact that these libraries exist or that they provide typeclasses and syntax to add to-string methods on top of existing classes and think: Oh, well now I have to write a trait, and object, implicit instances and implicit conversions to have what I used to have.

This comment has been minimized.

Copy link
@raboof

raboof Jun 19, 2019

Member

Not sure - I can see how it might give the reader an interesting pointer, on the other hand it increases the scope of the docs making them harder to understand and to maintain. Perhaps another reviewer can weigh in?

now I have to write a trait, and object, implicit instances and implicit conversions to have what I used to have

I'm not sure they'd have to go that full mile - they could just write a function?

This comment has been minimized.

Copy link
@pfcoperez

pfcoperez Jun 19, 2019

Author

I've no problem with removing the Cats and Scalaz examples and adding simpler ones (using vanilla Scala) + keeping the from-scratch type class.

This comment has been minimized.

Copy link
@pfcoperez

pfcoperez Jun 21, 2019

Author

This commit 409bbc9

Removes Cats and Scalaz dependencies and the examples using them. I initially included these because I liked @jypma suggestion in #2412 (comment) .

But I also understand that you don't want to introduce additional dependencies.

In any case, this is done in the last commit so if you finally decide that you want to include the examples, let me know and I can drop that commit.

Also there is a commit before that one adding examples using extension classes.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

Test PASSed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

Test PASSed.

@raboof
raboof approved these changes Jun 21, 2019
@pfcoperez

This comment has been minimized.

Copy link
Author

commented Jul 17, 2019

@raboof Thanks for reviewing this 🙇 Do you think it might hit next Akka-HTTP version?

@raboof

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@pfcoperez happy to, thanks for the contribution! the PR is waiting for its second reviewer, perhaps @jrudolph or @jlprat?

@pfcoperez

This comment has been minimized.

Copy link
Author

commented Oct 1, 2019

@jrudolph I've open a separate PR for hiding bodies of HttpEntity.Strict instances #2737

This way we can center this one into working of secrets redaction of headers.

@pfcoperez

This comment has been minimized.

Copy link
Author

commented Oct 1, 2019

@jrudolph Besides #2560 (comment) , and continuing with the minimal approach you suggested at #2560 (review)

I propose to redact (replace toString by an implementation showing the name of the header class) all headers not are not descendants of ModeledHeader

sealed trait ModeledHeader extends HttpHeader with Serializable {

That is blacklisting all headers which are not part of Akka-Http model.
For those which are, I've combed the list and I would blacklist these ones as they potentially can leak sensitive information:

  • Authorization
  • Cookie
  • Host
  • Origin
  • Referer
  • Remote-Address
  • X-Forwarded-For
  • X-Forwarded-Host
  • X-Real-Ip
  • Proxy-Authorization
@pfcoperez pfcoperez force-pushed the pfcoperez:feature/2409/trim_httpmessage_tostring branch from 02212f2 to dfb6474 Oct 2, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Oct 2, 2019

Test FAILed.

Pull request validation report

Failed Test Suites

Test result for 'docs / Pr-validation / ./ executeTests'

[info] ScalaTest
[info] Run completed in 3 minutes, 52 seconds.
[info] Total number of tests run: 288
[info] Suites: completed 57, aborted 0
[info] Tests: succeeded 288, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[error] Failed: Total 502, Failed 2, Errors 0, Passed 487, Skipped 13
[error] Failed tests:
[error] 	docs.http.javadsl.HttpRequestDetailedStringExampleTest
[error] 	docs.http.javadsl.HttpResponseDetailedStringExampleTest
@pfcoperez

This comment has been minimized.

Copy link
Author

commented Oct 2, 2019

@jrudolph

  • Removing body total o partial string representation is addressed in #2737
  • On the other hand, I've just committed a change to this branch (dfb6474) which adds a safeToString which can be invoked by potentially PII/Sensitive information leaking code, such HttpMessage#toString.
    • It defaults to not showing more than the header name. No modeled headers don't show their details in this method.
    • Except for modeled headers which are considered safe by default and, therefore, use toString implementation.
    • The ten modeled headers that have been identified as containers of sensitive information (#2560 (comment)) override modeled headers default behaviour, showing just their names.

I didn't want to change the renderer nor override toString to make these changes orthogonal to the rest of the codebase.

That's just a PoC so I haven't changed the docs until we discuss the approach.

@pfcoperez pfcoperez force-pushed the pfcoperez:feature/2409/trim_httpmessage_tostring branch from dfb6474 to 547547b Oct 2, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Oct 2, 2019

Test FAILed.

Pull request validation report

Failed Test Suites

Test result for 'docs / Pr-validation / ./ executeTests'

[info] ScalaTest
[info] Run completed in 3 minutes, 53 seconds.
[info] Total number of tests run: 288
[info] Suites: completed 57, aborted 0
[info] Tests: succeeded 288, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[error] Failed: Total 502, Failed 2, Errors 0, Passed 487, Skipped 13
[error] Failed tests:
[error] 	docs.http.javadsl.HttpRequestDetailedStringExampleTest
[error] 	docs.http.javadsl.HttpResponseDetailedStringExampleTest
@pfcoperez pfcoperez force-pushed the pfcoperez:feature/2409/trim_httpmessage_tostring branch from 547547b to 3115b00 Oct 2, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Oct 2, 2019

Test PASSed.

…ally PII/Sensitive information leaking code, such `HttpMessage#toString`.

It defaults to not showing more than the header name except for modeled headers which are considered safe by default and, therefore, use `toString` implementation.
10 modeled headers have been identified as containers of sensitive information and these override modeled headers default behaviour.
@pfcoperez pfcoperez force-pushed the pfcoperez:feature/2409/trim_httpmessage_tostring branch from 3115b00 to 1652a83 Oct 2, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Oct 2, 2019

Test PASSed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.