Resolve #6068 digestauth challenge redux

[blast-hardcheese]

Resolves #6068

Attempting to address PR feedback in #6077

Refactoring NonceKeeper

• Removing synchronized calls
• Switching var to Ref
• Making all exposed methods F[_]
• Splitting out AuthStore trait with a more secure scheme that doesn't require the user storing the user's passwords in plaintext

Some remaining tasks:

• Performance testing
• Push F[_] further: remove remaining var, while/tailrec/return

Opening this draft early on to communicate intent and direction, as well to get feedback early

[armanbilge]

Took a quick pass through—excellent work, this is looking very nice 👍

[armanbilge]

A couple other notes:

1. Annoying, but I think we have to keep the old NonceKeeper as well, because we need it to implement the deprecated methods in DigestAuth that are not using F[_] to preserve backwards-compatibility. Just like you did in your other PR.
2. Don't worry about performance testing, but it would be nice to clean up those remaining imperative stuff.

[blast-hardcheese]

Performance analysis, I ran this script against efadfc9 (origin/series/0.22) and c666a7f (this branch):

-BasicAuthentication should respond to a request with correct credentials: 15ms
+BasicAuthentication should respond to a request with correct credentials: 19ms
-BasicAuthentication should respond to a request with unknown username with 401: 4ms
+BasicAuthentication should respond to a request with unknown username with 401: 6ms
 BasicAuthentication should respond to a request with wrong password with 401: 1ms
-BasicAuthentication should respond to a request without authentication with 401: 20ms
+BasicAuthentication should respond to a request without authentication with 401: 24ms
-DigestAuthentication should avoid many concurrent replay attacks: 16ms
+DigestAuthentication should avoid many concurrent replay attacks: 31ms
-DigestAuthentication should respond to a request with correct credentials: 29ms
+DigestAuthentication should respond to a request with correct credentials: 40ms
-DigestAuthentication should respond to a request without authentication with 401: 8ms
+DigestAuthentication should respond to a request without authentication with 401: 49ms
-DigestAuthentication should respond to invalid requests with 401: 17ms
+DigestAuthentication should respond to invalid requests with 401: 7ms
-DigestAuthentication should respond to many concurrent requests while cleaning up nonces: 85ms
+DigestAuthentication should respond to many concurrent requests while cleaning up nonces: 140ms

(the script just runs server / testOnly org.http4s.server.middleware.authentication.AuthenticationSuite multiple times in a loop, then averages the resulting times)

I'm somewhat confused by the directive of "don't worry about performance", but I'm heartened by my largely amateurish attempt not having too negative of a performance impact. I'm available and interested in idioms or suggestions to improve performance, if necessary. mimaReportBinaryIssues reports success as well, which is also great.

[rossabaker]

You've been in good hands with others here, so I only skimmed it. Looking good, though. I imagine we'll do another round of releases this upcoming week, and I'd love to get this in.

On the subject of "don't worry about performance", I haven't followed the conversation closely enough to see where it came up. I'd say we do strive for performance, but correctness comes first, and we don't generally trade legibility or purity for performance without a profiled hotspot. Benchmarking and optimizing is fun, but when we're all mostly writing services that listen on a socket and then make a remote database query and then enrich that with a client call to another web service, an extra flatMap here or there is a drop in the ocean.

[armanbilge]

Ross beat my review by a few minutes, so apologies for the duplicated comments 😆

I made the comment about performance in #6138 (comment) (emphasis added):

Don't worry about performance testing

What I meant was, we wouldn't block your PR due to lack of benchmarks :) for sure, we don't want you to tank performance! Also, meaningful benchmarking is not easy: you have to account for JVM warm-up and JIT-optimizations, etc.

[blast-hardcheese]

Thanks for the reviews, I've got my work cut out for me!

In taking the next step in verifying my work here, I ran across Requires that the server can recover the password in clear text, which is _strongly_ discouraged., which would give the impression that this whole class is not intended for even limited production use in real systems, as it's impossible to use DigestAuth without supplying an instance of that function, but the function itself is documented as being strongly discouraged.

In an attempt to resolve this, I've pushed up Flesh out AuthenticationStore, a bincompat change that promotes a type alias to a sealed trait, preserving the previous functionality but offering a more secure variant.

I'm offering this here as it's convenient to only break bincompat once, since we're already changing everything up, but this definitely falls under scope creep.

[blast-hardcheese]

I made the comment about performance in #6138 (comment) (emphasis added):

Don't worry about performance testing

What I meant was, we wouldn't block your PR due to lack of benchmarks :) for sure, we don't want you to tank performance! Also, meaningful benchmarking is not easy: you have to account for JVM warm-up and JIT-optimizations, etc.

That makes more sense 😅

I figured enough runs of the auth suite would be sufficient ballpark, which is why I included them, since this is (from my vantage point) a significant enough change to warrant them, though Ross' point about an extra flatMap here or there was useful for keeping from spiraling off into the weeds.

I've updated the PR with feedback, and will update again shortly once I've finished this test for the Md5HashedAuthenticationStore.

[blast-hardcheese]

Alright, this is good to go, barring one final thought:

Without knowing how to precompute the ha1, I perceive this sort of like an open loop; we're expecting the users to just know how to do that, reimplementing a part of the spec in their own service.

I added a precomputeHash(username: String, realm: String, password: String): F[String] convenience function attached to the Md5HashedAuthStore companion object to resolve this usability gap, though I'm open to removing it if it's deemed unnecessary.

[rossabaker]

This is shaping up well. I gave it a more thorough review. Lots of comments, but hopefully none of them stir the pot too much.

[rossabaker]

Does this need to be sealed? I see we match on them in checkAuthParams. Would it be better if those cases were a method on this, to permit more pluggable implementations? Or are plaintext and MD5 the two that are specified?

[blast-hardcheese]

Based on how I understand the spec, there are only two sensible implementations:

Plain-text passwords:

md5($un:$realm:$pw):$nonce:$nc:$cnonce:$qop:md5($method:$uri)

Pre-hashed passwords:

md5($un:$realm:$pw) -> ha1 in the db
                |
         retrieve from db
                v
               $ha1:$nonce:$nc:$cnonce:$qop:md5($method:$uri)

The way I see it, by pushing more into subclasses of AuthStore, we'd need to expose all these parameters for dubious benefit. ha1 is the only secret that needs to be exposed to user code

[blast-hardcheese]

Reviewing the spec again for another comment, this is actually called out explicitly by the spec:

Note that the HTTP server does not actually need to know the user's
cleartext password. As long as H(A1) is available to the server, the
validity of an Authorization header may be verified.

[rossabaker]

An optional header allows the server to specify the algorithm used to
create the checksum or digest. By default the MD5 algorithm is used
and that is the only algorithm described in this document.

It looks like it can be md5, md5-sess, or extensions that probably nobody understands. I think it would be unpleasant to register and understand more for little gain.

[blast-hardcheese]

The need to add support for this stuff may motivate new contributors 😉

[blast-hardcheese]

(Additionally, adding the required helper methods, similar to Md5HashedAuthStore.precomputeHash)

[rossabaker]

This is going to need a Blocker and a ContextShift in the long run. This can merge before #6165, but it would be best to get that done before we have a release and need to do a MiMa exemption.

The nice thing is, in Cats-Effect 3, this becomes more transparent in the API.

[blast-hardcheese]

Sort of interesting -- NonceKeeperF[F].newNonce() already requires a permit for the singleton semaphore, so I think acquiring an instance of Blocker at that time makes sense. I can keep moving it further up if desired, I really don't have a good intuition around where that instance needs to come from.

[rossabaker]

Calling new BigInteger(bits, random) will block on some platforms as it gathers entropy. We want to shift that call specifically to another thread, so we're not blocking the compute pool on the IO to /dev/random. There's also a mutex to worry about on SecureRandom, but that should be fine since you're putting this instance behind a semaphore.

[blast-hardcheese]

Does Blocking[F].use actually acquire a new thread/threadpool? Should that parameter be bubbled all the way out to the caller's code?

[rossabaker]

Cats-Effect 2's Blocker does require a thread pool. One Blocker should suffice for most apps, but any app that does blocking needs one. It's part of the runtime in Cats-Effect 3, and available from the Sync constraint, so this will melt away in 0.23.

Engineering tradeoff: if we don't thread it through, this could suck real bad for a small number of users. If we do thread it through, we're going to all that work for, like, Windows users, and other people who do Weird Shit. http4s-0.22 is basically EOL. Personally, I'm only fixing bugs on it. And this blocking is not new, and nobody has complained. So if you'd prefer, we could put a fat disclaimer on it and know it will be better in CE3. But this is "safer".

[rossabaker]

Oh, I overlooked the Blocker.use. Yeah, that creates a new cached thread pool on every invocation. What we want is one cached blocker for the entire app that gets threaded through. This is where the API is going to get uglier in CE2.

[blast-hardcheese]

Swapped it out to a single allocated Blocker on init, backed by the global threadpool, as this is going away anyway.

[rossabaker]

We don't want to run blocking operations on the global threadpool. That pool has special magic if you additionally use a special standard library blocking incantation, which we aren't. Blocking should typically be done on a dedicated, cached thread pool. I think it would be better to remove it entirely if we're not going to thread it through.

[blast-hardcheese]

Alright -- threaded it through, as this is a new integration, and the exposure of it will better match the functionality in CE3 for users through the upgrade process.

[blast-hardcheese]

Seems like an unrelated failure, I spent a few minutes trying to understand what's going on but I'm completely out of my depth.

While the failure message is

==> X org.http4s.DecodeSpec.decode should either succeed or raise a CharacterCodingException  0.083s munit.FailException: /Users/dstewart/Projects/deps/http4s/tests/src/test/scala/org/http4s/DecodeSpec.scala:134
133:
134:    test("decode should either succeed or raise a CharacterCodingException") {
135:      forAll { (bs: Array[Byte], cs: Charset) =>

Failing seed: ul4X_OcjscjNgHDzfcislTYeeRBho4GD7b-pOuxHoPJ=
You can reproduce this failure by adding the following override to your suite:

  override val scalaCheckInitialSeed = "ul4X_OcjscjNgHDzfcislTYeeRBho4GD7b-pOuxHoPJ="

Falsified after 91 passed tests.
> ARG_0: [B@7f12f19
> ARG_1: x-COMPOUND_TEXT
> ARG_0_ORIGINAL: [B@55d47e2
[error] Failed: Total 10, Failed 1, Errors 0, Passed 9
[error] Failed tests:
[error]         org.http4s.DecodeSpec
[error] (tests / Test / testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 8 s, completed Mar 21, 2022 2:31:32 PM

I actually found that val caused a NPE, but override def reproduces the error:

-  override val scalaCheckInitialSeed = "ul4X_OcjscjNgHDzfcislTYeeRBho4GD7b-pOuxHoPJ=";
+  override def scalaCheckInitialSeed = "ul4X_OcjscjNgHDzfcislTYeeRBho4GD7b-pOuxHoPJ=";

[rossabaker]

Yeah, that test flake looks like another #4262. Don't worry about that here.

[rossabaker]

Thanks for your perseverance on this. I think it's in a good place.

[rossabaker]

@armanbilge, are you good with this as is? You had opinions earlier.

[armanbilge]

Yeah, the scope of this PR got away from me 😆 really fantastic work by @blast-hardcheese. Both CE2 and the RFC are a bit out of my comfort zone.

I glanced through and have one question, but not a blocker.

[armanbilge]

When this becomes an effect, it can no longer be a global. So where will it be initialized?

[rossabaker]

The usage is deprecated. I don't think we need to make it an effect. Though we could maybe add a deprecation notice to this type, to assure it's only called through deprecated code.

[armanbilge]

Oh, is this not going to use the stuff from #6165?

[rossabaker]

I was planning on welding it together after this. @blast-hardcheese has suffered enough. But I could merge it into this just to see what it looks like.

[armanbilge]

Cool, sounds good to me 🚀