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

Support for embedded authenticators in WebAuthn. #37205

Closed
wants to merge 3 commits into from
Closed

Support for embedded authenticators in WebAuthn. #37205

wants to merge 3 commits into from

Conversation

nikosdion
Copy link
Contributor

@nikosdion nikosdion commented Mar 5, 2022

Summary of Changes

Adds support for embedded authenticators (Windows Hello, TouchID, FaceID, Android biometrics) to WebAuthn.

Also has Joomla require the phpseclib/bcmath_compat dependency which allows WebAuthn to work on servers which have neither the BCmath nor the GMP extensions. (WebAuthn depends on web-auth/webauthn-lib which depends on web-auth/cose-lib which depends on fgrosse/phpasn1 which requires one of: ext-gmp, ext-bcmatch or phpseclib/bcmath_compat; Joomla had not added the latter dependency).

Testing Instructions

Prerequisites

One of the following configurations:

Set 1:

  • Windows 10 or 11 on a computer with a TPM (all Win11 compatible computers have one).
  • Using Windows Hello with a PIN.
  • Make sure you do not have any hardware authenticators attached or Android devices linked to your computer.

Set 2:

  • Windows 10 or 11 on a computer with a fingerprint scanner or facial recognition camera compatible with Windows Hello.
  • Make sure you do not have any hardware authenticators attached or Android devices linked to your computer.

Set 3:

  • MacBook Air or MacBook Pro with a fingerprint reader (Intel or Apple Silicon model) -OR- any Apple Silicon based Apple computer (MacBook Air, MacBook Pro or iMac) with the TouchID-enabled Apple Aluminium keyboard.
  • You have set up TouchID for logging into your computer.
  • You are operating your computer with the lid open.

Set 4:

  • iPad or iPhone with a fingerprint sensor or a FaceID camera.
  • You have set up TouchID or FaceID for unlocking your device.
  • Safari on iOS 14.2 or later.

Set 5:

  • An Android device with a biometric unlock feature (typically a fingerprint scanner).
  • You have set up your device to unlock with biometrics.
  • Chrome browser, possibly other browsers (unsure; I don't have such an Android device)

Instructions

  1. Go to your site's backend
  2. From the User Menu in the top right corner of the page select Edit Account.
  3. Click on the "W3C Web Authentication (WebAuthn) Login" tab.
  4. Click on Add New Authenticator

Actual result BEFORE applying this Pull Request

You are asked to plug in an authenticator device, or you are otherwise told that there are no authenticator devices used. You can't use this feature without a hardware authenticator.

Expected result AFTER applying this Pull Request

You are asked to use your device's built-in authentication. Depending on your device:

  • You are asked to enter your Windows Hello PIN (no special hardware required!).
  • You are asked to unlock Windows Hello with a fingerprint or facial scan.
  • You are asked to use TouchID (macOS, iOS, iPadOS) or FaceID (iOS, iPadOS).
  • You are asked to use biometrics on your Android device.

Important: due to hardware availability I have only been able to verify this on:

  • Windows 11 with Windows Hello locked with a PIN (no biometric hardware or hardware authenticator).
  • iPhone SE with TouchID.
  • iPad Pro (2021 model) and iPhone 12 on iOS 15 with FaceID.
  • MacBook Pro (mid-2019 model) with TouchID on macOS Monterey.

I am using this code with my LoginGuard extension the past two years. Based on user feedback they have also used it with Windows Hello using a facial recognition camera or a fingerprint reader. I don't have direct proof that Android users have used it, but I have strong indications that at least three people had used it with a fingerprint scanner (based on the fact they asked me to help them recover access when they changed their device and the only second factor method registered on their account was WebAuthn). I'd appreciate tests on these platforms I cannot test myself.

Documentation Changes Required

None.

Backward compatibility

Fully backwards compatible.

Technical reasoning

It looks like the WebAuthn implementation we added in Joomla only supported the ECDSA signature method with NIST P-256

Unfortunately, Microsoft chose to only support RSASSA-PKCS1-v1_5 (RFC8017, article 8.1) signatures with SHA-256 as the hashing function. This is a less secure method and wasn't added on purpose. This PR adds support for this method as the last fallback the browser is allowed to use when signing the public key it sends us during the pairing ceremony.

Further to that, I have improved the supported algorithms to allow more secure algorithms to be used with higher priority. Here are in order, from top to the lowest priority:

  • Edwards-curve Digital Signature Algorithm (EdDSA), only if libsodium is enabled
  • ECDSA (keys based on Elliptic Curve Cryptography with NIST P-521, P-384 or P-256). The latter was the only option supported by Joomla until now and it continues to be supported still.
  • RSASSA-PSS with SHA-512, SHA-384 or SHA-256
  • Shared secret w/ HKDF and SHA-512 or SHA-256
  • Shared secret w/ AES-MAC 256-bit key
  • RSASSA-PKCS1-v1_5 (RFC8017 article 8.1) with SHA-512, SHA-384 or SHA-256. Windows Hello with PIN uses the latter.

These changes only affect newly registered authenticators and only if they support the relevant signing method. Already registered authenticators use ECDSA P-256, no change there. Essentially, we want authenticators to use the best signing method they support!

@brianteeman
Copy link
Contributor

TO TESTERS
Please remember that https is required before you can use webauthn

@brianteeman
Copy link
Contributor

Below is a recording of the setup procedure on my win11 laptop with biometric fingerprint and pixel5 phone. I could not set the physical key as the opensource solokeys that I bought has just made its way into the trash where it belongs.

Please pay attention in the video to the setup process. Not sure if its intentional or not.

setup.mp4

The problem comes when trying to use any of those to login

Warning
Call to undefined function Cose\Key\gmp_strval()

image

@brianteeman
Copy link
Contributor

If you want access to the site just let me know

@nikosdion
Copy link
Contributor Author

nikosdion commented Mar 5, 2022 via email

@brianteeman
Copy link
Contributor

It would appear not.

This means among other things that the code should be checking for gmp and notifying the user etc. Instead of letting you set it up without error and only fail when you try to use it.

@brianteeman
Copy link
Contributor

ah - maybe something else isnt working #29731

@brianteeman
Copy link
Contributor

The code submitted by @PhilETaylor correctly identified that gmp was not enabled

But you subsequently changed that code #30316

Changes the check in Joomla 4's implementation of WebAuthn to require either the GMP or the BCmatch PHP extension. You see, the library we use is compatible with both, despite what @PhilETaylor asserted 😉

But then you went on to say that all the checks could now be removed.
#30316 (comment)

/me confused now

@nikosdion
Copy link
Contributor Author

@brianteeman As you can see in libraries/vendor/fgrosse/phpasn1/composer.json:

"suggest": {
        "ext-gmp": "GMP is the preferred extension for big integer calculations",
        "ext-bcmath": "BCmath is the fallback extension for big integer calculations",
        "phpseclib/bcmath_compat": "BCmath polyfill for servers where neither GMP nor BCmath is available",
        "ext-curl": "For loading OID information from the web if they have not bee defined statically"
    },

You need either GMP or BCmath since Joomla does not include phpseclib/bcmath_compat. Your PHP version includes neither library which, as we had established two years ago, is extremely unlikely.

Sure, I can have Joomla depend on phpseclib/bcmath_compat in this PR to add a fallback (since this PR is all about extending compatibility) but as it will affect Composer dependencies you won't be able to use the patch tester, you'll have to wait for the Drone build to create an update package.

@brianteeman
Copy link
Contributor

it does include bcmath which is why I dont get the error message

image

@nikosdion
Copy link
Contributor Author

OK, now we're getting into "how does the third party library work" territory. I can tell you how it works, what I suspect happens but I cannot make a change in the third party library code — or even propose a change since we're using a very old version of the library on account of Joomla still stubbornly supporting the EOL versions of PHP 7.2 and 7.3.

The WebAuthn plugin uses the web-auth/webauthn-lib which uses web-auth/cose-lib which uses fgrosse/phpasn1.

This library needs to work with big integers. It has the FG\Utility\BigInteger class, file libraries/vendor/fgrosse/phpasn1/lib/Utility/BigInteger.php.

As you can see in lines 42 to 64 it needs to figure out if it's going to use its BCmatch or GMP adapter. For the context of what we're using in Joomla the self::$_prefer is always null which means we go into the feature detection code lines 55 to 63.

The only way it will use the GMP library is if the function gmp_add already exists on your server.

The error you got comes from libraries/vendor/fgrosse/phpasn1/lib/Utility/BigIntegerGmp.php line 42.

Here's the thing. For some reason on your server the method gmp_add exists but the method gmp_strval does not. The former causes the phpasn1 library to detect that GMP is available on your server. But it's not, at least not in full, which causes the error you get.

There is nothing in Joomla itself which defines gmp_add. So, you either have a third party extension doing something funky or something is seriously wrong with your server e.g. it has a broken GMP PHP extension.

Even if I wanted to force the use of BCmath (so at least we get to use the polyfill if all alse fails) I cannot do that since the call to phpasn1 is two levels deep into dependencies and I have no control over it. I'd have to modify third party libraries which We Do Not Do (that's the whole point of using Composer).

So... it's something on your end.

@brianteeman
Copy link
Contributor

brianteeman commented Mar 5, 2022

For some reason on your server the method gmp_add exists but the method gmp_strval does not.

Sorry that's not correct

https://domorewithcore.com/gmp.php

code is


<?php 
echo "<h1> Testing for gmp and bc math functions</h1>";
// random select of gmp functions
echo "gmp_add: ";
if (function_exists('gmp_add')) { echo "exists"; } else { echo "does not exist"; }
echo "<br>";
echo "gmp_abs: ";
if (function_exists('gmp_abs')) { echo "exists"; } else { echo "does not exist"; }
echo "<br>";
echo "gmp_intval: ";
if (function_exists('gmp_intval')) { echo "exists"; } else { echo "does not exist"; }
echo "<br>";
echo "gmp_cmp: ";
if (function_exists('gmp_cmp')) { echo "exists"; } else { echo "does not exist"; }
echo "<br>";
echo "gmp_gcd: ";
if (function_exists('gmp_gcd')) { echo "exists"; } else { echo "does not exist"; }
echo "<br>";
echo "gmp_strval: ";
if (function_exists('gmp_strval')) { echo "exists"; } else { echo "does not exist"; }
echo "<br>";
echo "<br>";
// random selection of BC Math Functions 
echo "bccomp: ";
if (function_exists('bccomp')) { echo "exists"; } else { echo "does not exist"; }
echo "<br>";
echo "bcadd: ";
if (function_exists('bcadd')) { echo "exists"; } else { echo "does not exist"; }
echo "<br>";
echo "bcscale: ";
if (function_exists('bcscale')) { echo "exists"; } else { echo "does not exist"; }


which means that the test is correct as it is testing for one of gmp and bc

so the problem is elsehwere and nothing to do with my server

@nikosdion
Copy link
Contributor Author

@brianteeman If you do not want me to read the code for you, you are welcome to run a debugger on your server, step through the code and tell us why this happens on your server and your server only.

@nikosdion
Copy link
Contributor Author

@brianteeman Further to that, I disabled GMP on my testserver (Ubuntu 21.10, PHP 8.0.15). BCmath is enabled. Checked on Joomla, System, System Information, PHP Information. I can register WebAuthn authenticators. No problem whatsoever.

Next up, I disabled BCmath as well. With this PR I can register an authenticator, no problem whatsoever.

Again, the problem may be that a third party extension registers a partial polyfill for the GMP library which declared gmp_add but not gmp_strval. This is something that your script running outside of Joomla would of course not catch.

Therefore the problem is not with the WebAuthn plugin, it is not with the PR (which DOES NOT update the library!), it is not with the third party library dependencies pulled in by the WebAuthn plugin. It is either a third party extension doing something wonky OR a problem with your server. You have ruled out the problem with your server therefore there must be a problem with an extension. If you do not have any extensions then the problem really is with the server and you need to debug your server.

Once again, for the very last time so people in the back can hear it.

If you do NOT have the GMP extension the library will fall back to BCMath. If you do not have BCMath the polyfill (third party library pulled in by this PR) will be used. Any problems with that means third party interference. Can't do jack about that, it ain't my problem. Don't use craphosts, as I have been saying the last 17 years. End of story.

I have better things to do with my weekends than playing whack-a-mole with rando hosting wacky issues half past midnight.

@brianteeman
Copy link
Contributor

The only extensions on this site are ones that you have written

@brianteeman
Copy link
Contributor

I can register WebAuthn authenticators. No problem whatsoever.

I have NO problem registering the authenticators. I showed that in the video. The problem is when you try to login using one.

@nikosdion
Copy link
Contributor Author

@brianteeman I of course did try to log in with the keys I registered and they are working fine. To make it clear, I test by registering an authenticator, log out, then try to log in again in the backend, log out, try to log into the frontend.

Yes, yesterday I did the test on a clean 4.1-dev installation because that's how we test PRs.

In any case, since you insist, today I used my dev site which has ALL OF MY EXTENSIONS (none of which is using GMP, mind you) and sure enough I can register and login authenticators just fine. So, no, I do not appreciate the accusation here.

Here's the bigger problem, though.

Adding support for platform authenticators (Windows Hello, TouchID, FaceID etc) is a few lines of code. I've had that IN MY OWN SOFTWARE for two years with zero problems. You are hijacking the discussion with an unrelated issue. Now nobody will test this and this feature is dead.

I am closing this PR and will submit a new one where all your tests will be marked invalid because you are on a broken environment. What a waste of time.

@nikosdion nikosdion closed this Mar 6, 2022
@brianteeman
Copy link
Contributor

So, no, I do not appreciate the accusation here.

No accusation at all. You said it was an extension causing it which I know was not the case as only your extensions were used.

@nikosdion nikosdion deleted the feature/webauthn-winhello-support branch March 6, 2022 09:19
@nikosdion
Copy link
Contributor Author

Y'all know better than me, who has this code in production for three years, how WebAuthn works.

Fine. I am deleting my branch. You get to figure out how to implement this feature.

Meanwhile, I will release my own extension which actually works. See how many flying fucks I give.

@brianteeman
Copy link
Contributor

plus ca change

@brianteeman
Copy link
Contributor

brianteeman commented Mar 6, 2022

Server 1 with bcmath only

Clean install with no extensions

Before PR
Setup phone ✔️
Login phone ✔️

After PR
Setup phone ✔️
Setup pin ✔️
Setup finger ✔️

Login phone ✔️
Login pin ❌
Login finger ❌

Message: Call to undefined function Cose\Key\gmp_strval()

server 2 with bcmath and gmp

clean install with no extensions

Before PR
Setup phone ✔️
Login phone ✔️

After PR
Setup phone ✔️
Setup pin ✔️
Setup finger ✔️

Login phone ✔️
Login pin ️✔️
Login finger ✔️

Server 3

built locally using the instructions from this great blog post using php 8.0 downloaded from php.net and then with gmp disabled in php.ini
Before PR
Setup phone ✔️
Login phone ✔️

After PR
Setup phone ✔️
Setup pin ✔️
Setup finger ✔️

Login phone ✔️
Login pin ❌
Login finger ❌

Message: Call to undefined function Cose\Key\gmp_strval()

Summary

On the server without gmp you can set up the embedded authenticators but they fail when being used.

@nikosdion
Copy link
Contributor Author

This is a bug in the old version (2.1) of the WebAuthn library we are using. It only happens with RSA signatures which is what your browser generated for Windows Hello.

The bug is fixed in version 3 of the WebAuthn library, however version 3 is completely different than 2 and backwards incompatible which means we cannot upgrade to it until Joomla 5 (I guess?). Which of course means that we cannot do anything useful with WebAuthn until then. By the time it's time for Joomla 5 it's very likely that this library will also be on the cusp of yet another new version, further compounding the upgrade situation.

Even worse, third party extensions cannot work around this problem either. The new version of the library uses the same namespace and class names as the old version. A 3PD extension would hit a brick wall unless they created private namespaces for all dependencies which considering the breadth of dependencies involved is impractical.

In so many words, we are totally fucked. We can never fix WebAuthn in Joomla.

For this reason I would ask @bembelimen to consider removing WebAuthn from Joomla 4.1 as broken and unfixable. In retrospect, this kind of feature should only exist as a third party extension, following its own development cycle which is not dragged down by Joomla's release cycle.

@brianteeman
Copy link
Contributor

🙈🙉🙊

@brianteeman
Copy link
Contributor

/me confused that it worked perfectly for you in your tests

@HLeithner
Copy link
Member

How would you think the composer dependency hell can be solved anyway? It doesn't matter if we fork a lib or not if it doesn't get any updates anymore. Actually it would mean we have to remove anything that's broken in a 3rd party lib if it breaks a subsystem of joomla.

Also we allow progressive enhancement as long as we have a b/c layer and my under standing is that GMP is not needed with the bcmath compat library right?

About SemVer, I bringing this RFC so we have a clarification on this, at the moment it's a pain to decide if something is a b/c break or not.

Any way if we don't support "Windows hello" by the lifetime of j4.x because of a missing function it's ok for me too but we have to live with this about 3,3 years.

@nikosdion
Copy link
Contributor Author

@brianteeman v3 is completely incompatible with v2. We need to reimplement the plugin, at least the parts generating and validating signatures.

I can't tell you for sure if it breaks existing registrations but I suspect it does. v2 only had a public key repository, v3 also has a metadata repository which did not previously exist. The impact of it is as yet unknown.

However, you are looking at it very myopically. Joomla includes this library which means that third party Joomla extensions also use this library, including my own LoginGuard. Upgrading the library to an incompatible version would break 3PD extensions because they are forced to use whatever Joomla includes (we can't load our own version).

The only way the library can be bumped to v3 is by also bumping Joomla's major version to 5 according to semver. This can't happen at the moment nor can we wait for another ~2 years until the time for Joomla 5 comes. Unless, of course, the Joomla projects says the hell with semver, we are going to randomly break everything in the middle of a point release which was the major and justified complaint against Joomla during the 1.5 / 1.6 / 1.7 / 2.5 lifecycle.

All the options are:

  • Leave WebAuthn as-is until Joomla 5, effectively killing this feature AND making it impossible for 3PDs to provide a better implementation without a massive amount of work.
  • Bump the library to v3, break 3PD extensions, Joomla no longer conforms to SemVer.
  • Patch the v2 library, puts using Composer into question (and requires writing a post-install and post-update Composer script to apply the monkey patch)
  • Increase the minimum server requirements to include GMP in the middle of the 4.x cycle, for a feature that's enabled by default, therefore Joomla no longer conforms to SemVer.

All solutions are bad.

@HLeithner

How would you think the composer dependency hell can be solved anyway?

In my opinion, third party dependencies other than PSR should be rebased into a private namespace (Joomla\Private).

It doesn't matter if we fork a lib or not if it doesn't get any updates anymore

It does. It prevents 3PDs from using a newer version of the library. I wanted to use WebAuthn v3 in LoginGuard but I couldn't because it shares the same namespace as the v2 version included in Joomla.

Also we allow progressive enhancement as long as we have a b/c layer and my under standing is that GMP is not needed with the bcmath compat library right?

Partially correct. The bug in the Key.php file means that it is always looking for GMP. There is no polyfill for GMP, only for BCmath.

About SemVer, I bringing this joomla/rfc#29 so we have a clarification on this, at the moment it's a pain to decide if something is a b/c break or not.

Yeah, I've read the RFC and I am still unclear what would be the correct thing to do here. My understanding is upgrade the library to v3 and bump the major Joomla version to 5. But that won't fly for marketing reasons so... dunno.

Any way if we don't support "Windows hello" by the lifetime of j4.x because of a missing function it's ok for me too but we have to live with this about 3,3 years.

The entire point of including WebAuthn was to make it easier and safer for people to log into their sites. Right now people think of it as "Joomla asks me to buy a $20 hardware key for every one of my users, I am not gonna do this". If we keep this mindset for 3 to 4 years this feature is as good as dead, we might just as well remove it and let 3PDs like me offer a better solution.

This is the same thing that happened with Two Factor Authentication. We added TOTP (which is supported on free of charge and/or Free software), that was OK. We added Yubikey support and this immediately KILLED all development on the feature. Trying to get U2F support for it hit a wall "we won't include any features in Joomla which require third party hardware or services". The planned upgrade to Two Step Verification with a captive login page was also killed because anything other than TOTP might require third party hardware or services.

From my perspective, Joomla has been very efficient at killing off the development of every login security enhancement feature as soon as it was merged in the core. Remember that you had personally asked me or rather borderline begged me in January 2020 in person to contribute WebAuthn to Joomla because it would be a major win for login security. Now you are "eh, it doesn't really matter, does it?". Well, that was the last time I have contributed a security feature to Joomla and it will remain the last time since Joomla doesn't take login security seriously. I'd much rather offer (free of charge!) extensions to do that. At least I do care about this subject. You feel me?

@brianteeman
Copy link
Contributor

I dont understand where these comments and attitude come from. I am obviously missing some important conversations so will withdraw from the conversation

@dgrammatiko
Copy link
Contributor

The only way the library can be bumped to v3

@nikosdion I will repeat that in your own plugin you can prefix the library and let Joomla do what it's doing best (keep things outdated). Just use the prefixer service or do it yourself.

FWIW prefixing libraries should be the norm for a system like Joomla where composer is not an option (for 3rd PD)...

@nikosdion
Copy link
Contributor Author

@brianteeman The discussions were in public. Anyway, if you think that trying to figure out if Joomla actually follows semver and whether it actually cares about login security (as opposed to have a meaningless checkmark item next to "we have a feature that's ostensibly for login security") is an attitude I am leaving you guys to it. You figure this out. Not my monkey, not my circus.

@HLeithner
Copy link
Member

All the options are:

  • Leave WebAuthn as-is until Joomla 5, effectively killing this feature AND making it impossible for 3PDs to provide a better implementation without a massive amount of work.
  • Bump the library to v3, break 3PD extensions, Joomla no longer conforms to SemVer.
  • Patch the v2 library, puts using Composer into question (and requires writing a post-install and post-update Composer script to apply the monkey patch)
  • Increase the minimum server requirements to include GMP in the middle of the 4.x cycle, for a feature that's enabled by default, therefore Joomla no longer conforms to SemVer.

All solutions are bad.

Actually you are wrong with point 3, we wouldn't patch it we would fork it and provide it as replacement for the upstream version.
But actually asking in the upstream repository for a new/fixed version would be the most straight forward one. Do you asked upstream?

@HLeithner

How would you think the composer dependency hell can be solved anyway?

In my opinion, third party dependencies other than PSR should be rebased into a private namespace (Joomla\Private).

That doesn't work because of you already mentioned it that this will break if you do it for 1000 dependencies sooner or later.

It doesn't matter if we fork a lib or not if it doesn't get any updates anymore

It does. It prevents 3PDs from using a newer version of the library. I wanted to use WebAuthn v3 in LoginGuard but I couldn't because it shares the same namespace as the v2 version included in Joomla.

Following this then Joomla is not allowed to use any composer libraries would run into this trouble.

About SemVer, I bringing this joomla/rfc#29 so we have a clarification on this, at the moment it's a pain to decide if something is a b/c break or not.

Yeah, I've read the RFC and I am still unclear what would be the correct thing to do here. My understanding is upgrade the library to v3 and bump the major Joomla version to 5. But that won't fly for marketing reasons so... dunno.

I think it's not only marketing, we would scare everyone if we release a 5 "only because of one library" in 4 month.

Any way if we don't support "Windows hello" by the lifetime of j4.x because of a missing function it's ok for me too but we have to live with this about 3,3 years.

The entire point of including WebAuthn was to make it easier and safer for people to log into their sites. Right now people think of it as "Joomla asks me to buy a $20 hardware key for every one of my users, I am not gonna do this". If we keep this mindset for 3 to 4 years this feature is as good as dead, we might just as well remove it and let 3PDs like me offer a better solution.

ok make sense but removing it would be more worse. at least for people using it already (like me on a daily basis)

From my perspective, Joomla has been very efficient at killing off the development of every login security enhancement feature as soon as it was merged in the core. Remember that you had personally asked me or rather borderline begged me in January 2020 in person to contribute WebAuthn to Joomla because it would be a major win for login security. Now you are "eh, it doesn't really matter, does it?". Well, that was the last time I have contributed a security feature to Joomla and it will remain the last time since Joomla doesn't take login security seriously. I'd much rather offer (free of charge!) extensions to do that. At least I do care about this subject. You feel me?

Yeah I feel you, I will never again ask anyone to do something for joomla the only thing that happens is that you get remembered for ever.

What I never said is that the feature "doesn't matter" if we only support a subset of devices is not nice but doesn't mean we should stop it. Btw. as I already sad we have progressive enhancement with allows us to use features only if the "server or client" site provides us with the needed environment . So having no "Windows hello" and other services which needs RSA only with GMP would be possible. But I think it could be hard to explain why it is this way...

@brianteeman
Copy link
Contributor

So having no "Windows hello" and other services which needs RSA only with GMP would be possible. But I think it could be hard to explain why it is this way...

that's what all this comes down to and my 2c is that it can be "explained" just like any other documented optional dependency. It just needs the code to be adjusted so that these features are only available when gmp is present - if that is possible with the library bug.

I just dont get what the drama is.

@kitepascal
Copy link
Contributor

kitepascal commented Mar 6, 2022

I just dont get what the drama is.

Me too.

I tested this PR on some, also known as "crappy" german hostings. And I'm happy that I can login using Windows PIN authentification until I get my (not too cheap) Hello face recognition camera tomorrow for further testing.

If the server requirement for that single feature is not met - too bad - but no drama, if possible, point that out in a little notice for the minority on exotic server environments.

Thanks for your effort, your discussions are really interesting apart from understanding train station in some ways.

@brianteeman
Copy link
Contributor

@kitepascal the "problem" is that with the bug on the library it lets you setup windows pin but doesnt let you use it. But I am sure we can work around that

@kitepascal
Copy link
Contributor

kitepascal commented Mar 6, 2022

The WP Plugin for that also requires gmp and mbstring with no fallback.

https://de.wordpress.org/plugins/wp-webauthn/

But does not matter here I guess.

@brianteeman
Copy link
Contributor

I see three options

  1. Do nothing - webauthn continues to work with just ble and keys
  2. nuclear option - kill the feature and remove the plugin
  3. add conditional loading for the allowed keys based on the availability of gmp

@HLeithner
Copy link
Member

  1. With some luck upstream fixes the bug for us
    Undocumented GMP extension usage in 2.1 web-auth/webauthn-framework#213

@kitepascal
Copy link
Contributor

3.) +1

@brianteeman
Copy link
Contributor

@HLeithner wouldnt another option being to go back to the original version of this code and just require gmp. As the upstream bug is with the fallback to bcmath

@HLeithner
Copy link
Member

thats the "documentation thing" right? or would you do a hard requirement for all webauthn stuff?

@brianteeman
Copy link
Contributor

No - its a change to this code to remove & function_exists('bccomp') === false

// Ensure the GMP or BCmath extension is loaded in PHP - as this is required by third party library
if ($allow_add && function_exists('gmp_intval') === false && function_exists('bccomp') === false)
{
$error = Text::_('PLG_SYSTEM_WEBAUTHN_REQUIRES_GMP');
$allow_add = false;
}

@HLeithner
Copy link
Member

that would mean installations running now bcmath would fail right? that would be a b/c

I still prefer to fork the library and fix it for us, we are already discussing this in the maintainer chat.

@brianteeman
Copy link
Contributor

ok as long as its being discussed I am happy.

@nikosdion
Copy link
Contributor Author

I spent last week working on it.

The best way to proceed is the following:

  • Upgrade the WebAuthn library to version 3
  • Use the badly named \Webauthn\Server object to do all the WebAuthn handling instead of doing it the hard way. This object, API-compatible, also existed in version 2 of the library.

If you want to play with this right now to see how it works, take a look at this. That's the WebAuthn plugin's parallel development — that's the repository where I developed the plugin before including it in Joomla and kept developing it since. I have included a namespaced copy of the WebAuthn library version 3 so you can install it on a Joomla 4.1 site without breaking anything.

You can see the difference between how we did things and how we can simplify it without losing any functionality and keeping compatibility with existing keys.

I have confirmed that library v3 works with Windows Hello with or without GMP, with or without BCmath. IN fact that's why it took me so long, my monitor broke down and I had no way of accessing my Windows desktop until I managed to fix the damn thing!

If you ARE interested in doing this the right way, using library v3, I can make a new PR. If you are going to fork the library v2 there's no point discussing this beyond knowing that there is, indeed, an easier way which doesn't require Joomla to maintain Yet Another Library Fork.

@brianteeman
Copy link
Contributor

If you ARE interested in doing this the right way, using library v3, I can make a new PR

I am !!

@HLeithner
Copy link
Member

so your suggestion is to ignore semver and update the library?

@brianteeman
Copy link
Contributor

Does the public api change?

@nikosdion
Copy link
Contributor Author

@HLeithner Yes and no. I have a better picture of the library now.

The root cause of the problem is that the WebAuthn library's documentation is really bad and was even even worse (read: completely missing...) when I built the original Passwordless Authentication plugin which I subsequently contributed to Joomla as plg_system_webauthn.

What they didn't say back then is that even the v2 library has the \Webauthn\Server object and developers are actually meant to use that, not go through the various internal objects which the library treated as internal implementation details (but didn't mark as such, except imply that in passing in their documentation, saying that doing anything else is the hard way and should be avoided!).

Therefore, the Webauthn plugin in Joomla and my third party code (Akeeba LoginGuard) are actually using the library wrong. We should be using the \Webauthn\Server object. This object is 100% API compatible in versions 2 and 3 of the library. Therefore upgrading to library v3 would not, in fact, be a violation of the SemVer per se since the only public APIs to the library are compatible between the two major versions!

The biggest problem is that the WebAuthn library doesn't actually follow SemVer any better than Joomla does which is to say that they feel they can drop or change features in minor versions. If you look at the Webauthn\Server implementation you will see that they intend to drop the last argument (the metadata repository) in a future 3.x version, changing the constructor signature. Meanwhile, they did bump the major version when they changed the method signatures in internal objects. It looks like they had some growing pains and we suffer second-hand pain because of that.

So, the best plan for Joomla would be:

  • Change the code to use the \Webauthn\Server object.
  • Upgrade the library to v3 in Joomla 4.2.
  • Communicate the change publicly NOW so any 3PDs using the library can use the \Webauthn\Server object which doesn't change its API signature between the two library versions. (I doubt anyone else besides me is using it but you never know, do you)

I also want to address two points you made earlier.

I did not ask for an upstream change because v2 is EOL. It's like someone coming to you with a PHP 8.1 compatibility patch for Joomla 2.5. You wouldn't release a new version, would you?

As for contributing features to be remembered by, I think you're not paying much attention.

I contributed Two Factor Authentication in Joomla 3.2 as a temporary solution with the promise that in one year (3.2 -> 3.5 -> 4.0) I would be allowed to implement it with a captive login page. I was also promised that in the meantime I could contribute more 2FA plugins, the first one which was pre-agreed upon was U2F. Imagine my surprise when the PR for the U2F plugin was rejected and I was told that no more work would be done on this feature. I am now remembered as the developer who implemented a half-assed 2FA for Joomla. I had to release my own, third party extension (Akeeba LoginGuard) to prove that no, I actually do know how to do it and that the entire problem was Joomla pulling a fast one on me. Do remember that 2FA was actually a feature of Admin Tools before I contributed it to Joomla.

I contributed FOF 2 in Joomla 3.2 on the promise that'd be developed as the Joomla RAD Framework. Needless to say, I got screwed over that one too. I am remembered as the developer who contributed a framework to Joomla and didn't bother updating it even though the problem was that Joomla itself didn't let me update it! Do remember that FOF was a separate framework I used in my company before contributing it to Joomla.

I contributed Joomla Update. There were things I was trying to contribute after that and kept being told no. The only improvement I did manage to contribute was install from file and that was only after there was a bad update in Joomla itself which broke Joomla Update. You guys have added bad features which erroneously report 3PD extensions as incompatible, exactly as I told you would happen three times in the 12 months before Joomla 4 was released. I even submitted a PR (#36531) to fix that on your request but it's still pending after 2 ½ months. Meanwhile, I am remembered as the developer who made a complicated and error-prone updater to Joomla even though the complicated UX and the errors ARE NOT MY CODE AND I ACTIVELY WARNED THE PROJECT AGAINST THEM! Do remember that Joomla Update was a feature in Admin Tools before contributing it to Joomla.

So, please, do tell me exactly how I benefit from contributing my code instead of keeping it as a 3PD extension. From where I am standing Joomla's mismanagement of my contributions is damaging my reputation. You are not doing me a favour accepting my contributions, it's me doing you (and all Joomla users) a favour. I contribute despite knowing that my reputation will suffer as a result, despite knowing that any small issue which would be solved in half an hour now takes a week of my time.

If you think that my contributions benefit me more than they do Joomla, remove them: Joomla Update, Post-installation messages, Quick Icons, Joomla and extension update notifications, Update sites management, paid extension updates. I can go back to publishing CMS Update or do core updates in Admin Tools, I don't care about post-installation messages, I can publish my own admin module for quick icons along with plugins for notifications for my own software and Joomla core updates, I can go back to using my own code to manage update sites and I can resurrect Akeeba Live Update for paid extension updates. That's what I was doing back in Joomla 1.5 / 1.6 and it was all working fine. I won't lose anything. But Joomla? Well, Joomla would lose big time. Do you understand now how my contributions benefit Joomla, not me?

@brianteeman
Copy link
Contributor

doesnt sound like a breaking change of the public api then to me. Interesting background reading if you are interested regarding how semver is effected when a dependency changes semver/semver#148 and https://thoughtspile.github.io/2021/11/08/semver-challenges/

@HLeithner
Copy link
Member

Ok you complain that Joomla doesn't follow semver good enough and like that we break semver, that's funny but ok ;-)

Based of the link provided by brian:

If your library exposes the underlying package B by letting the users access its objects or passing through user options, find out if the minor / breaking changes in B affects the exact part you expose. A safe & lazy option is to match your major / minor / patch update to the update in B.

since we expose this package as primary package in composer we are responsible for it's API.

Anyway if it's the best way to pump the library in j4.2 I will discuss this in maintainers chat. Beside that we need a solution for such problems, I know you don't know a solution and me ether so we should write something in the RFC or find a procedure how we (not directly you, more the maintainers) handle this situation transparently.

I will come back to you as soon as I have an answer.

@brianteeman
Copy link
Contributor

This may be me being ignorant - probably is - but I will say it anyway at the risk of being shouted down.

If the plugin is updated with a privately namespaced version of the v3 library (as shown in the linked tree from @nikosdion) doesnt that mean that we can still include the non-namespaced v2 library in the joomla package.

That way we get the new code that actually works and if someone was actually using the webauthn v2 api they still can do so withut any disruption.

@HLeithner
Copy link
Member

This may be me being ignorant - probably is - but I will say it anyway at the risk of being shouted down.

If the plugin is updated with a privately namespaced version of the v3 library (as shown in the linked tree from @nikosdion) doesnt that mean that we can still include the non-namespaced v2 library in the joomla package.

That way we get the new code that actually works and if someone was actually using the webauthn v2 api they still can do so withut any disruption.

of course that can be done but is even more odd then forking the v2 version and fix it^^, in this case I would say gmp is mandatory for webauthn starting with 4.2. that would solve the problem too iirc

@nikosdion
Copy link
Contributor Author

@HLeithner Yes, I understand the problem with SemVer. However, in this particular case the truth is that this library's intended public API (the \Webauthn\Server object, the interface for the repository and the various leaf node objects returned from the Server object) have a stable API between versions 2 and 3 of the library! Considering that the library's intended public API doesn't change it wouldn't be a SemVer violation upgrading the library.

The devil is indeed in the details, in that the WebAuthn library did not explicitly mark the internal objects as @internal. This would indeed allow a misguided developer like me to use the internal objects.

The biggest question is: who is using those internal objects beyond me (either on my 3PD software or the core plg_system_webauthn plugin)? Even my code had a bum-out if-block for version 3 of the library since last year. If you upgrade to v3 it will still work. Which leaves us with the question, do we break anyone's userland by upgrading to v3? If the answer is no then it makes more sense upgrading to v3 than forking the no longer updated v2!

Whichever method we choose for this particular library won't apply for any other dependency. This library is “special” in so many ways...

In any case, I think I gave you enough details to help you and the other maintainers make an informed decision, whichever it is.

Whichever decision you make we MUST change the WebAuthn plugin's code to use the Server object. It will ‘magically’ add support for Windows Hello and isolate the plugin's code from any future changes in the library, to the extent that the external library doesn't drop the Server object.

@brianteeman Yes, that is correct. That was the reason why I namsepaced the v3 library and ONLY those of its dependencies that differ from v2. I even used Rector with the configuration included in the tree so it's reproducible by anyone, even with another namespace prefix. I didn't use a third party service — even though it'd be much easier — to avoid depending on a black box solution someone else controls.

So, yes, moving the plugin to a private copy of the v3 library is most definitely an option and does not break b/c in any way. Even better, we can upgrade the library in the plugin as needed in the future without as much as batting an eyelid since it does not affect the Joomla distribution as a whole.

@HLeithner I disagree with this:

in this case I would say gmp is mandatory for webauthn starting with 4.2. that would solve the problem too iirc

This is an unnecessary dependency. There is a way to make WebAuthn work WITHOUT gmp or bcmath. Adding a dependency reduces the utility and applicability of this feature which is contradicting its stated goal of making authentication simpler, don't it?

@brianteeman
Copy link
Contributor

of course that can be done but is even more odd then forking the v2 version and fix it^^

Not really - as you now not only get the fix but any new features in v3 AND its a soft fork as in you can easily keep updating it with upstream and scripting th

Nik's reply came in as I was answering Harald. No need for me to comment further as he said the same as I was writing

@brianteeman
Copy link
Contributor

hit this hard again :( :(


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37205.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants