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

WebAuthn extension #24316

Merged
merged 10 commits into from
Mar 24, 2022
Merged

WebAuthn extension #24316

merged 10 commits into from
Mar 24, 2022

Conversation

FroMage
Copy link
Member

@FroMage FroMage commented Mar 15, 2022

This is the infamous Quarkus WebAuthn extension, finally.

I'm making this a draft PR because since my rebase on the latest Quarkus, it fails due to a call to Context.get in the Vert.x WebAuthn extension, which hopefully @cescoffier and @pmlopes are going to help me with.

Also, I'll need @pmlopes's help with the documentation for the WebAuthnOptions which I'm exposing as Quarkus config: they're undocumented so I can't document them.

Once this is resolved I can turn this out of draft, but otherwise the tests are there, passing (well, minus the context issue), and the docs are written, and the quickstart PR is on its way.

Also, I have a question for @stuartwdouglas and @geoand: here I'm shipping a copy of RenardeCookieFilter which is a fix/fork of the RestAssured CookieFilter which has an invalid cookie expiry date parser that actually doesn't work. I'd like to add this class to an existing Quarkus test module so that everyone can use it, but I'm not sure which would be suitable, do you know?

Fixes #9316

@pmlopes
Copy link
Contributor

pmlopes commented Mar 15, 2022

@FroMage I don't have write permissions on quarkus, so I'll make a fork from your repo and do PR's against your branch so you can cherry pick and push to quarkus. Is this OK?

@FroMage
Copy link
Member Author

FroMage commented Mar 15, 2022

Sure, thanks :)

@cescoffier
Copy link
Member

@pmlopes One of the problems @FroMage is seeing is that VertxContextPRNG is using Context.put, which is forbidden in Quarkus. We had many issues with bad usage of Context.get / Context.put methods, leaking sensible data between unrelated processing.

2022-03-15 11:06:45,608 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (vert.x-eventloop-thread-0) HTTP Request to /webauthn/register failed, error id: 38925640-db93-4129-aa7a-05207437e5e2-1: java.lang.UnsupportedOperationException: Access to Context.put(), Context.get() and Context.remove() are forbidden as it can leak data between unrelated processing. Use Context.putLocal(), Context.getLocal() and Context.removeLocal() instead. Note that these methods can only be used from a 'duplicated' Context, and so may not be available everywhere.
    at io.quarkus.vertx.core.runtime.VertxLocalsHelper.throwOnRootContextAccess(VertxLocalsHelper.java:18)
    at io.vertx.core.impl.AbstractContext.get(AbstractContext.java)
    at io.vertx.ext.auth.VertxContextPRNG.current(VertxContextPRNG.java:66)
    at io.vertx.ext.auth.VertxContextPRNG.current(VertxContextPRNG.java:98)
    at io.vertx.ext.auth.webauthn.impl.WebAuthnImpl.<init>(WebAuthnImpl.java:65)
    at io.vertx.ext.auth.webauthn.WebAuthn.create(WebAuthn.java:55)
    at io.quarkus.security.webauthn.WebAuthnSecurity.<init>(WebAuthnSecurity.java:71)
    at io.quarkus.security.webauthn.WebAuthnSecurity_Bean.create(Unknown Source)
    at io.quarkus.security.webauthn.WebAuthnSecurity_Bean.create(Unknown Source)

@geoand
Copy link
Contributor

geoand commented Mar 15, 2022

I'd like to add this class to an existing Quarkus test module so that everyone can use it, but I'm not sure which would be suitable, do you know?

I don't think we have a test module for HTTP related stuff

@pmlopes
Copy link
Contributor

pmlopes commented Mar 15, 2022

@FroMage see: FroMage#1

@pmlopes
Copy link
Contributor

pmlopes commented Mar 15, 2022

@FroMage @cescoffier I could create a specific constructor on WebauthnImpl that create a unique VertxContextPRNG instance, which doesn't invoke the Context.get()/Context.put().

Or I can update the current constructor to do a try catch and if it fails, fallback to a local instance of PRNG. But I'm not sure if this try catch affects the native builds?

  /**
   * Get or create a secure non blocking random number generator using the provided vert.x context. This method will not
   * throw an exception.
   *
   * @param context a Vert.x context.
   * @return A secure non blocking random number generator, or {@code null} if context access fails.
   */
  @GenIgnore
  static VertxContextPRNG current(final Context context) {
    try {
      final String contextKey = "__vertx.VertxContextPRNG";
      // attempt to load a PRNG from the current context
      PRNG random = context.get(contextKey);

      if (random == null) {
        synchronized (context) {
          // attempt to reload to avoid double creation when we were
          // waiting for the lock
          random = context.get(contextKey);
          if (random == null) {
            // there was no PRNG in the context, create one
            random = new PRNG(context.owner());
            // need to make the random final
            final PRNG rand = random;
            // save to the context
            context.put(contextKey, rand);
          }
        }
      }

      return random;
    } catch (RuntimeException e) {
      // Access to the current context is probably blocked
      return null;
    }
  }

This would solve the problem for all places where we use VertxContextPRNG

Copy link
Contributor

@pmlopes pmlopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some comments at the high-level config and JS script. Please have a look at the downstream PR that adds documentation.

//private JsonObject extensions;

// FIXME
//private Map<String, X509Certificate> rootCertificates;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could enable this like in vert.x, you can use the "standard" base64 certificate encoder string:

https://github.com/vert-x3/vertx-auth/blob/master/vertx-auth-webauthn/src/main/java/io/vertx/ext/auth/webauthn/WebAuthnOptions.java#L47-L184

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this open until someone wants this. I don't feel good telling people to add base64 certificates in the config file, that's just bad UX.
If anything, I'd prefer the config to point at files:

quarkus.webauthn.root-certificates.something=something.pem

Or even a convention where these files get automatically configured src/main/resources/META-INF/webauthn/root/*.pem

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, what's the point of the Map? Why not just a list?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for the map was because older versions of vertx didn't supported all attestation modes and the root certificates are to be bound to a given attestation type. Internally vert.x uses a service loader to load the attestation implementations, so this was dynamic.

Perhaps we can improve the upstream code to not use a service loader as it now supports all attestations so we can define speicifc config properties, e.g.:

  • android-safetynet
  • android-key
  • apple

And mds (metadata service).

We could have a dual option too: either inline buffer (which you don't like) and a filesystem path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this open until someone wants this.

Please note, that without this functionality you cannot claim that your implementation is FIDO CONFORMANT as you cannot guarantee that Android and IPhone's are trustworthy if attestation is required.

Same for any kind of token that has been identified as faulty as MDS cannot be trusted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can handle files downstream, that's no problem. But I still don't understand why this is a Map, do you do anything special with the keys?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, they need to match the attestation types:

  • android-safetynet
  • android-key
  • apple

This way during the attestation we know which certificate to use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//private Map<String, X509Certificate> rootCertificates;

// FIXME
//private List<X509CRL> rootCrls;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is like the previous. Without this, MDS doesn't work. MDS is important for PSD2.0 apps (banking/finance) where authenticators must be verified, for example, a bank should not trust a rooted android, as a bad actor could attempt to extract the private key from the hardware in case of security bugs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this open until someone wants this. I don't feel good telling people to add base64 certificates in the config file, that's just bad UX.
If anything, I'd prefer the config to point at files:

quarkus.webauthn.root-crls=something.crl

Or even a convention where these files get automatically configured src/main/resources/META-INF/webauthn/root/*.crl

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before, we can propose an upstream update that takes a list of files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downstream can handle the files when we support this, no problem.

@@ -0,0 +1,241 @@
"use strict";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you should just include this from maven?

https://repo1.maven.org/maven2/io/vertx/vertx-auth-webauthn/4.2.5/vertx-auth-webauthn-4.2.5-client.js

This will be updated once the new Level 3 spec is released as we hope that the API will handle JSON directly instead of this dance of encode/decode buffers.

Plus we need to support https://github.com/vert-x3/vertx-auth/issues/539 and you will start drifting from the original script and have double maintenance

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated it with more methods, to be able to only do the first half and get the login/registration challenges without calling the callback.

@cescoffier
Copy link
Member

@pmlopes You need to be absolutely sure PRNG is thread-safe, as with that you can have concurrent access.

Anyway, yes, having another constructor that does use Context.put / Context.get would be great.

@pmlopes
Copy link
Contributor

pmlopes commented Mar 15, 2022

PRNG should be thread safe as it relies on SecureRandom which is thread safe. Feel free to review:

https://github.com/vert-x3/vertx-auth/blob/master/vertx-auth-common/src/main/java/io/vertx/ext/auth/PRNG.java

The only extra thing PRNG is doing, is that it sets up a timer that reseeds the secure random, as described by the OWASP security best practices.

@pmlopes
Copy link
Contributor

pmlopes commented Mar 16, 2022

@FroMage @cescoffier please review https://github.com/vert-x3/vertx-auth/pull/546 it should address the context issue and work not only for webauthn but for all vert.x code that uses it.

@FroMage
Copy link
Member Author

FroMage commented Mar 16, 2022

The Vert.x fix is at https://github.com/vert-x3/vertx-auth/pull/546 and will be in 4.2.6

@FroMage
Copy link
Member Author

FroMage commented Mar 17, 2022

… Which… has been released !

@FroMage FroMage marked this pull request as ready for review March 17, 2022 13:53
@FroMage
Copy link
Member Author

FroMage commented Mar 22, 2022

Awesome! That'd be really great!

@sberyozkin
Copy link
Member

Hi @FroMage Sorry for a delay, IMHO it is quite close but I also think we should have 2 approvals here, @stuartwdouglas hi Stuart, can you also have a look please ?

@gsmet
Copy link
Member

gsmet commented Mar 22, 2022

Ok, well, I have no problem merging this post CR1 as it's preview so we can take a little more time.

But let's get it merged before the end of the week, please.

@sberyozkin
Copy link
Member

@gsmet Thanks, it feels like we need another day or so

@sberyozkin
Copy link
Member

@FroMage Can you please start squashing the commits ?

@FroMage
Copy link
Member Author

FroMage commented Mar 23, 2022

@FroMage Can you please start squashing the commits ?

Well, there are commits from @gsmet and @pmlopes so I'd lose them. I don't really mind, but I'm not sure it's proper.

@sberyozkin
Copy link
Member

@gsmet Can you please review the list of commits and agree with @FroMage which of them should not be squashed, thanks

@FroMage
Copy link
Member Author

FroMage commented Mar 24, 2022

Look, if you insist, I'll sqash them all, and never mind for authors. I really don't care if you prefer that.

@sberyozkin
Copy link
Member

@FroMage I don't mind either way, I've just asked Guillaume for his preference, my understanding it is OK to have more than one commit as long as they represent some specific additions. etc. It is nearly there but lets wait for Guillaume to decide on this one

@sberyozkin sberyozkin self-requested a review March 24, 2022 16:50
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @FroMage and @pmlopes for working on this exciting new feature; as we've discussed with Steph, some further cleanup/minor improvements may be necessary before its preview status gets upgraded, but indeed, Quarkus users should get a chance to start experimenting with this feature asap

@sberyozkin
Copy link
Member

sberyozkin commented Mar 24, 2022

@FroMage I think we should indeed position WebAuthn not as an alternative to BasicAuth but as a preferred successor to Form Based Authentication

@FroMage
Copy link
Member Author

FroMage commented Mar 24, 2022

Yes, but the only difference between the two is that the credentials get passed via headers all the time, or via a form that gets you a login header (cookie, but that's the same thing as a bearer token)

@gsmet
Copy link
Member

gsmet commented Mar 24, 2022

The commits are semantic, fine by me.

@gsmet gsmet merged commit 3888f18 into quarkusio:main Mar 24, 2022
@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Mar 24, 2022
@cescoffier
Copy link
Member

Hum, this PR update to Vert.x 4.2.6, however, we do not have a constructor run for it yet, and the constructor reported issues with Vert.x main.

Let's see how it goes, I would need to find slots on between my travels, as I believe we will have a bunch of issues.

@FroMage
Copy link
Member Author

FroMage commented Mar 25, 2022

If you want to revert this change, I can delay the webauthn support.

@gsmet
Copy link
Member

gsmet commented Mar 28, 2022

Given the fact that Vert.x 4.2.6 seems to be causing problems, I won't backport this to 2.8 for now.

@cescoffier
Copy link
Member

After discussing with your @Sanne and @DavideD, we are going to revert this PR until we found the problem. Fortunately we will have a vertx 4.2.7 with a fix soon.

@FroMage
Copy link
Member Author

FroMage commented Apr 25, 2022

Apparently we did a 2.8.1.Final release with Vert.x 4.2.7, so perhaps we can backport this to 2.8 now @gsmet ?

@gsmet
Copy link
Member

gsmet commented May 12, 2022

@FroMage FWIW, I preferred waiting for 2.9 as this feature needs proper exposure and micros don't really get that much exposure given people are not expecting new features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/testing area/vertx release/noteworthy-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web Authn Extension
6 participants