search

simplesamlphp / simplesamlphp-module-webauthn

A module implementing FIDO2 / WebAuthn as a second authentication factor
GNU Lesser General Public License v2.1
15 stars 8 forks source link

Bump dependencies & PHP8 testing #33

Closed tvdijen closed 3 years ago

tvdijen commented 3 years ago

With no changelog on the dependencies and limited unit tests on our end, I'm not too confident this won't break anything.. If anyone has an already working setup and could test-drive this branch for a bit, that'd be great!

restena-sw commented 3 years ago

I will deploy the branch together with SSP 1.19 to verify that it still does what it should.

restena-sw commented 3 years ago

I can't test this even on PHP 7.4 at the moment. The branch to be tested requires simplesamlphp master. Deploying that, our first-factor auth is RADIUS and there is a bug in that module which makes it incompatible with simplesamlphp master: https://github.com/simplesamlphp/simplesamlphp-module-radius/issues/3

I'll have to wait for a working radius module in master.

restena-sw commented 3 years ago

Okay, with the radius module working, I now get a WebauthN specific error with the tvdijen branch on ssp dev-master (which I don't get with the module's master branch on 1.19.0):

        SimpleSAML\Error\Error: UNHANDLEDEXCEPTION

Backtrace: 2 /export/hosting/restena/clueless_private/htdocs/simplesaml-master-80/_include.php:17 (SimpleSAML_exception_handler) 1 vendor/symfony/error-handler/ErrorHandler.php:594 (Symfony\Component\ErrorHandler\ErrorHandler::handleException) 0 [builtin] (N/A) Caused by: InvalidArgumentException: Expected the key "UserID" to exist. Backtrace: 12 vendor/webmozart/assert/src/Assert.php:2042 (Webmozart\Assert\Assert::reportInvalidArgument) 11 vendor/webmozart/assert/src/Assert.php:1665 (Webmozart\Assert\Assert::keyExists) 10 modules/webauthn/lib/Auth/Process/WebAuthn.php:159 (SimpleSAML\Module\webauthn\Auth\Process\WebAuthn::process) 9 lib/SimpleSAML/Auth/ProcessingChain.php:198 (SimpleSAML\Auth\ProcessingChain::processState) 8 lib/SimpleSAML/IdP.php:321 (SimpleSAML\IdP::postAuth) 7 [builtin] (call_user_func) 6 lib/SimpleSAML/Auth/Source.php:231 (SimpleSAML\Auth\Source::loginCompleted) 5 [builtin] (call_user_func) 4 lib/SimpleSAML/Auth/Source.php:153 (SimpleSAML\Auth\Source::completeAuth) 3 modules/core/lib/Auth/UserPassBase.php:317 (SimpleSAML\Module\core\Auth\UserPassBase::handleLogin) 2 modules/core/www/loginuserpass.php:83 (require) 1 lib/SimpleSAML/Module.php:272 (SimpleSAML\Module::process) 0 /export/hosting/restena/clueless_private/htdocs/simplesaml-1.19.0/module.php:10 (N/A)

Does that ring a bell?

tvdijen commented 3 years ago

It does ring a bell yes! On SSP 1.x we had a global userid.attribute setting pointing to the identifying attribute of the user. At some point we realized this is an incredibly stupid design choice and decided this should be a setting on authproc-level. Some authprocs already had this, but they all had different names, so we decided to rationalize.

So, what has to be done here is that we create a config-setting on authproc-level called identifyingAttribute that for example holds the value userPrincipalName. Then we pick the attribute with that name from the state-array and use it as the UserID, or fail with an exception if it doesn't exist.

To give you an idea, I've fixed this for another module like this; https://github.com/simplesamlphp/simplesamlphp/pull/1363/files

restena-sw commented 3 years ago

Okay, that should be doable (the module already has a config item asking the name of the attribute to use to authenticate the user).

Now that brings me to a related question:

I noticed that the module, being a post-auth authproc, only sees the attributes which are going to be released to the particular SP this session is for. So if the identifyingAttribute is generated IdP-side, but not intended to be released, it is redacted before the authproc, and the authproc doesn't have access to it and can't perform the 2FA.

In current code, this leads to the 2FA auth being skipped. But that of course reduces security. Is there a place in the state array which has all attributes so that querying for the identifyingAttribute is possible regardless of the particular SP's attribute release policy?

tvdijen commented 3 years ago

I guess that's only true if you run this authproc from the saml20-sp-remote metadata, am I right? In that case the attribute has to be released by the IDP (you can still prevent sending it by performing ARP in the saml20-sp-remote metadata after performing 2FA. I don't see any way around it and there's no entry in the state-array to deal with this (I will discuss with Jaime).

This module should just fail with an exception if there is no such attribute available.. It's a great security risk to just skip 2FA.

restena-sw commented 3 years ago

No, the authproc is the (only) listed authproc filter in idp-hosted metadata. When going to an SP which doesn't release the pertinent attribute, current code gives:

WARNING [1bde3b8e84] webauthn: cannot determine if user needs second factor, missing attribute "urn:oid:1.3.6.1.4.1.23735.100.0"

That attribute IS available at the IdP side. My only logical explanation is that it removed from the state array already on the idp side.

FWIW, it is on priority 100 which makes it take place after all the other authproc.idp entries defined in config.php . And there are also authporoc.sp settings in the config, below 100. Maybe that makes a difference?

restena-sw commented 3 years ago

Ah! There was a config-wide authproc calling "core:attributeLimit" which did the redact. Placing webauthn at higher priority than that makes things work.

Good.

What remains is that you are of course right that 2FA should not be skipped even in such situations.

I'll commit and push the change to fail hard with an Exception then. Afterwards, I'll start working on the code change for UserID.

restena-sw commented 3 years ago

So after fixing the failed Assertion, there are possibly some other artifacts. Now when trying to trigger 2FA, the module gets called but apparently in a wrong way: I get

Page not found. The URL wasn't found in the module. https://clueless-private.restena.lu/simplesaml/module.php/webauthn/webauthn.php?StateId=...

but of course templates/webauthn.twig exists. Any insights there?

tvdijen commented 3 years ago

Thanks for picking up where I left.. Regarding the authprocs, there's a nasty undocumented 'feature' where idp-authprocs, global authprocs and sp authprocs are merged and then processed based on priority (over the entire set).

So when having an idp authproc with prio 99 and an ap authproc with prio 50, the sp-authproc would be executed first.. I can take a look at the error, but I'd like to see the full error stack if you don't mind sharing..

I've had similar cases... I actually had one right now and the logs said something like "The controller for URI "/statistics/" is not callable: Controller "SimpleSAML\Module\statistics\StatisticsController" has required constructor arguments and does not exist in the container. Did you forget to define the controller as a service?" It appears to be some kind of caching issue, because first I tried restarting httpd/php-fpm, but that didn't help.. Then I'd browse to another module and go back to where I wanted to go and it worked

restena-sw commented 3 years ago

Sure. I have an error report for this here:

[26-Jan-2021 11:44:18 Europe/Luxembourg] simplesamlphp NOTICE STAT [57bdde4263] User 'swinter' successfully authenticated from 2001:a18:1:12::4 [26-Jan-2021 11:44:18 Europe/Luxembourg] simplesamlphp NOTICE STAT [57bdde4263] saml20-idp-SSO-first https://kb-ops.restena.lu/simplesaml/module.php/saml/sp/metadata.php/staff-SAML-private https://clueless-private.restena.lu/simplesaml/saml2/idp/metadata.php NA [26-Jan-2021 11:44:18 Europe/Luxembourg] simplesamlphp NOTICE STAT [57bdde4263] saml20-idp-SSO https://kb-ops.restena.lu/simplesaml/module.php/saml/sp/metadata.php/staff-SAML-private https://clueless-private.restena.lu/simplesaml/saml2/idp/metadata.php NA [26-Jan-2021 11:44:18 Europe/Luxembourg] simplesamlphp ERR [57bdde4263] SimpleSAML\Error\NotFound: The requested page 'https://clueless-private.restena.lu/simplesaml/module.php/webauthn/webauthn.php?StateId=_7c231977ce2dbbdbad9f1b2b5cb7ca6beaea13d352-private.restena.lu.php-ops.restena.lu.php.php-SAML-private-ops.restena.lu' could not be found. The URL wasn't found in the module. [26-Jan-2021 11:44:18 Europe/Luxembourg] simplesamlphp ERR [57bdde4263] Backtrace: [26-Jan-2021 11:44:18 Europe/Luxembourg] simplesamlphp ERR [57bdde4263] 1 /export/hosting/restena/clueless_private/private/simplesamlphp-master-80/lib/SimpleSAML/Module.php:256 (SimpleSAML\Module::process) [26-Jan-2021 11:44:18 Europe/Luxembourg] simplesamlphp ERR [57bdde4263] 0 /export/hosting/restena/clueless_private/htdocs/simplesaml-master-80/module.php:10 (N/A) [26-Jan-2021 11:44:18 Europe/Luxembourg] simplesamlphp ERR [57bdde4263] Error report with id 7db84a27 generated.

The one thing I notice is that there seems to be some confusion inside the StateId param: the IdP is called "clueless-private.restena.lu" (the cluleless- is dropped); and the SP is "kb-ops.restena.lu" (the kb- is dropped). The StateId is apparently using '-' as a separator. Maybe it simply has issues when the strings themselves have some, too?

But even then... this should only ruin the StateId, not the URL itself. So it is not a real explanation...

FWIW, I captured the $_SERVER variable inside module.php just before this error happens (I think). It looks correct, including having the CORRECT hostnames inside StateId. Some mangling later on seems to destroy them. Below an example (different session ID):

[DOCUMENT_URI] => /simplesaml/module.php [REQUEST_URI] => /simplesaml/module.php/webauthn/webauthn.php?StateId=_3de0cdbb41a4dcf613f71659f46ed5055105096cfb%3Ahttps%3A%2F%2Fclueless-private.restena.lu%2Fsimplesaml%2Fsaml2%2Fidp%2FSSOService.php%3Fspentityid%3Dhttps%253A%252F%252Fkb-ops.restena.lu%252Fsimplesaml%252Fmodule.php%252Fsaml%252Fsp%252Fmetadata.php%252Fstaff-SAML-private%26RelayState%3Dhttps%253A%252F%252Fkb-ops.restena.lu%252F%26cookieTime%3D1611659634 [SCRIPT_NAME] => /simplesaml/module.php [QUERY_STRING] => StateId=_3de0cdbb41a4dcf613f71659f46ed5055105096cfb%3Ahttps%3A%2F%2Fclueless-private.restena.lu%2Fsimplesaml%2Fsaml2%2Fidp%2FSSOService.php%3Fspentityid%3Dhttps%253A%252F%252Fkb-ops.restena.lu%252Fsimplesaml%252Fmodule.php%252Fsaml%252Fsp%252Fmetadata.php%252Fstaff-SAML-private%26RelayState%3Dhttps%253A%252F%252Fkb-ops.restena.lu%252F%26cookieTime%3D1611659634 [PATH_INFO] => /webauthn/webauthn.php

tvdijen commented 3 years ago

Wait, we're talking master here, right? Because we use routers and no .php scripts

restena-sw commented 3 years ago

Aye we do. That's master of SSP and the tvdijen branch of webauthn. You are saying "module.php" should not be called at all?

tvdijen commented 3 years ago

module.php, yes, but the part that comes after that is wrong. It should be module.php/webauthn/webauthn without the .php.. I should probably check all the controller-classes

tvdijen commented 3 years ago

I've fixed this in 98209c6 The stateID being mangled sounds like we forget to urlencode it somewhere.. Perhaps if you can run another test with fix and see how far we get?

restena-sw commented 3 years ago

That patch alone didn't change anything. But you may have missed: lib/WebAuthn/StaticProcessHelper.php

$url = Module::getModuleURL('webauthn/webauthn.php');

As soon as I removed that extra .php as well, things did change considerably (but can't call it success yet): StateId missing:

SimpleSAML\Error\BadRequest: BADREQUEST('%REASON%' => 'Missing required StateId query parameter.') Backtrace: 5 modules/webauthn/lib/Controller/WebAuthn.php:94 (SimpleSAML\Module\webauthn\Controller\WebAuthn::main) 4 vendor/symfony/http-kernel/HttpKernel.php:157 (Symfony\Component\HttpKernel\HttpKernel::handleRaw) 3 vendor/symfony/http-kernel/HttpKernel.php:79 (Symfony\Component\HttpKernel\HttpKernel::handle) 2 vendor/symfony/http-kernel/Kernel.php:195 (Symfony\Component\HttpKernel\Kernel::handle) 1 lib/SimpleSAML/Module.php:205 (SimpleSAML\Module::process) 0 /export/hosting/restena/clueless_private/htdocs/simplesaml-master-80/module.php:15 (N/A)

Maybe we are now zeroing in on the missing URLencode someplace...?

tvdijen commented 3 years ago

I know what the issue is.. I've mixed up Symfony's derivative for the $_GET and $_POST variables, so the code is checking for the StateId in the $_POST-var instead of the $_GET-var..

restena-sw commented 3 years ago

Grepping the source suggets it's all POST (oddly enough a single GET but in tests/ which I didn't write):

lib/WebAuthn/WebAuthnAbstractEvent.php: $this->fail("Unknown Key Type: " . $_POST['type']); lib/Controller/RegProcess.php: echo "<form id='regform' method='POST' action='" . templates/authentication.twig:

templates/webauthn.twig: templates/webauthn.twig: templates/webauthn.twig: templates/webauthn.twig: tests/lib/Controller/WebAuthnTest.php: 'POST', tests/lib/Controller/StateTest.php: ['POST', 'authprocess', Controller\AuthProcess::class, 'main'], tests/lib/Controller/StateTest.php: ['POST', 'managetoken', Controller\ManageToken::class, 'main'], tests/lib/Controller/StateTest.php: ['POST', 'regprocess', Controller\RegProcess::class, 'main'], tests/lib/Controller/StateTest.php: ['POST', 'webauthn', Controller\WebAuthn::class, 'main'], tests/lib/Controller/RegProcessTest.php: 'POST', tests/lib/Controller/AuthProcessTest.php: 'POST', tests/lib/Controller/ManageTokenTest.php: 'POST', tests/lib/Controller/ManageTokenTest.php: $_SERVER['REQUEST_METHOD'] = 'POST'; tests/lib/Controller/ManageTokenTest.php: 'POST', tests/lib/Controller/ManageTokenTest.php: $_SERVER['REQUEST_METHOD'] = 'POST'; tests/lib/Controller/ManageTokenTest.php: 'POST', tests/lib/Controller/ManageTokenTest.php: $_SERVER['REQUEST_METHOD'] = 'POST'; tests/lib/Controller/ManageTokenTest.php: 'POST', -bash-4.2$ grep -r GET * tests/lib/Controller/RegistrationTest.php: 'GET',

tvdijen commented 3 years ago

It should be fixed in 74c9296b23c3a99ff5598734d19068bfd69ac159 so I'm happy to hear what happens You didn't find anything because Symfony uses request for POST and query for GET.. You can find the POST/GET on the 0.9 branch before we used controllers/routing. Apparently only the StateId is transferred by query params and everything else in the POST body

restena-sw commented 3 years ago

Now this gets us - finally - to the core of your request in this PR: dependencies possibly breaking stuff! Because now there is a failure in the middle of the crypto libraries called by WebAuthn:

SimpleSAML\Error\Error: UNHANDLEDEXCEPTION Backtrace: 2 /export/hosting/restena/clueless_private/htdocs/simplesaml-master-80/_include.php:20 (SimpleSAML_exception_handler) 1 vendor/symfony/error-handler/ErrorHandler.php:594 (Symfony\Component\ErrorHandler\ErrorHandler::handleException) 0 [builtin] (N/A) Caused by: Symfony\Component\ErrorHandler\Error\UndefinedFunctionError: Attempted to call function "sprintf" from namespace "Safe". Did you mean to call "\sprintf"? Backtrace: 11 vendor/web-auth/cose-lib/src/Key/Key.php:88 (Cose\Key\Key::get) 10 vendor/web-auth/cose-lib/src/Key/Ec2Key.php:103 (Cose\Key\Ec2Key::curve) 9 vendor/web-auth/cose-lib/src/Key/Ec2Key.php:137 (Cose\Key\Ec2Key::getCurveOid) 8 vendor/web-auth/cose-lib/src/Key/Ec2Key.php:122 (Cose\Key\Ec2Key::asPEM) 7 modules/webauthn/lib/WebAuthn/WebAuthnAuthenticationEvent.php:62 (SimpleSAML\Module\webauthn\WebAuthn\WebAuthnAuthenticationEvent::validateSignature) 6 modules/webauthn/lib/WebAuthn/WebAuthnAuthenticationEvent.php:50 (SimpleSAML\Module\webauthn\WebAuthn\WebAuthnAuthenticationEvent::__construct) 5 modules/webauthn/lib/Controller/AuthProcess.php:138 (SimpleSAML\Module\webauthn\Controller\AuthProcess::main) 4 vendor/symfony/http-kernel/HttpKernel.php:157 (Symfony\Component\HttpKernel\HttpKernel::handleRaw) 3 vendor/symfony/http-kernel/HttpKernel.php:79 (Symfony\Component\HttpKernel\HttpKernel::handle) 2 vendor/symfony/http-kernel/Kernel.php:195 (Symfony\Component\HttpKernel\Kernel::handle) 1 lib/SimpleSAML/Module.php:205 (SimpleSAML\Module::process) 0 /export/hosting/restena/clueless_private/htdocs/simplesaml-master-80/module.php:15 (N/A)

Looks like a breakage in cose-lib...

tvdijen commented 3 years ago

Could you try and add the suggested backslash? I think it is indeed missing, so if we can verify that I can post a bugfix PR upstream

restena-sw commented 3 years ago

It's worse. They are really using a library called "Safe" and its sprintf function, NOT the native PHP one:

https://github.com/web-auth/cose-lib/commit/0ba9ba076238d22a1d8737e84c3caa3e16c7c876

I've added a comment to the commit that did it, and will make an issue out of it if there's no reaction in due time.

restena-sw commented 3 years ago

https://github.com/web-auth/webauthn-framework/issues/171

So we should bump this dependency to 3.3.2 when it is released.

tvdijen commented 3 years ago

Great! We don't have to do anything for that because we have no lock-file in place. I've subscribed to the issue to keep track.. Meanwhile I should try and set up my own working copy of this module so I don't have to bother you each time something needs testing.

restena-sw commented 3 years ago

All you need is a Mac with TouchID - it does FIDO2/WebAuthn (Google Chrome then). Or of course a USB stick for some 20 bucks, to be independent of OS and browser.

What I found here is not guaranteed to be the only instance of problems... merely the first :-)

tvdijen commented 3 years ago

I think I should be able to use my iPad Pro for that, but I also have a FIDO2 yubikey available. I had it working at some point, it's just that I reinstalled my playground and I didn't bother to reinstall & configure this module.. Last time I worked on it was at NTW2019.. It's just that I started working on getting all of our modules ready for SSP 2.0, hence this PR.

tvdijen commented 3 years ago

@restena-sw They have released a new version.. You still have your test-setup in place?

restena-sw commented 3 years ago

I'm resurrecting it right now. With fresh dev-master of SSP on PHP-8, I don't get very far even when calling the base URL:

SimpleSAML\Error\Error: UNHANDLEDEXCEPTION Backtrace: 2 /export/hosting/restena/clueless_private/htdocs/simplesaml-master-80-May-2021/_include.php:20 (SimpleSAML_exception_handler) 1 vendor/symfony/error-handler/ErrorHandler.php:607 (Symfony\Component\ErrorHandler\ErrorHandler::handleException) 0 [builtin] (N/A) Caused by: Error: Non-static method SimpleSAML\Utils\HTTP::getSelfHost() cannot be called statically Backtrace: 8 modules/radius/lib/Auth/Source/Radius.php:114 (SimpleSAML\Module\radius\Auth\Source\Radius::__construct) 7 lib/SimpleSAML/Auth/Source.php:316 (SimpleSAML\Auth\Source::parseAuthSource) 6 lib/SimpleSAML/Auth/Source.php:358 (SimpleSAML\Auth\Source::getById) 5 lib/SimpleSAML/Auth/Simple.php:68 (SimpleSAML\Auth\Simple::getAuthSource) 4 lib/SimpleSAML/Auth/Simple.php:167 (SimpleSAML\Auth\Simple::login) 3 [builtin] (call_user_func_array) 2 lib/SimpleSAML/HTTP/RunnableResponse.php:72 (SimpleSAML\HTTP\RunnableResponse::sendContent) 1 vendor/symfony/http-foundation/Response.php:394 (Symfony\Component\HttpFoundation\Response::send) 0 /export/hosting/restena/clueless_private/htdocs/simplesaml-master-80-May-2021/module.php:10 (N/A)

Looks like current master has an issue with a static/non-static call conflict?

I guess the RADIUS module needs an update there (and RADIUS is the first-factor, so that's important in the long run. But I can switch to static user files right now of course...)

tvdijen commented 3 years ago

Ah, yes, were are moving away from singleton utility classes because they are horrible in unit testing. The first batch has been migrated, but not every module has been updated just yet.. I'll fix the radius-module immediately!

tvdijen commented 3 years ago

This should fix it; https://github.com/simplesamlphp/simplesamlphp-module-radius/commit/fe46ec38adbf28c9e32cb3978adafb5a346060ba

restena-sw commented 3 years ago

Gets me a little bit further, but then some type-safety error it seems:

SimpleSAML\Error\Error: UNHANDLEDEXCEPTION Backtrace: 1 /export/hosting/restena/clueless_private/htdocs/simplesaml-master-80-May-2021/_include.php:20 (SimpleSAML_exception_handler) 0 [builtin] (N/A) Caused by: TypeError: Cannot assign int to property SimpleSAML\Module\radius\Auth\Source\Radius::$vendor of type ?string Backtrace: 5 modules/radius/lib/Auth/Source/Radius.php:119 (SimpleSAML\Module\radius\Auth\Source\Radius::construct) 4 lib/SimpleSAML/Auth/Source.php:316 (SimpleSAML\Auth\Source::parseAuthSource) 3 lib/SimpleSAML/Auth/Source.php:358 (SimpleSAML\Auth\Source::getById) 2 lib/SimpleSAML/IdP.php:106 (SimpleSAML\IdP::construct) 1 lib/SimpleSAML/IdP.php:138 (SimpleSAML\IdP::getById) 0 /export/hosting/restena/clueless_private/htdocs/simplesaml-master-80-May-2021/saml2/idp/SSOService.php:31 (N/A)

authsources.php certainly assigns an integer there:

    'attribute_vendor' => 23735,
    'attribute_vendor_type' => 4,
restena-sw commented 3 years ago

That looks good! I now authenticated with example-userpass as first and WebAuthN as second factor - the auth process itself worked fine. (RADIUS as first blocked by the attribute_vendor thing above)

I noticed one bug though: config has set "inflow_registration" => true, but the auth screen showed the trimmed-down version that belongs to the "standalone registration page" feature. I guess that config parameter is not picked up correctly.

tvdijen commented 3 years ago

Not sure how this is wrong... That feature is pretty new and I don't think anything has changed since it was merged in #30 I'll fix the radius type-bug, thanks for that one!!

restena-sw commented 3 years ago

Sure, this looks like an ordinary bug not related to dependencies and PHP8. So for the sake of this PR, I think we are done and consider it closed.

tvdijen commented 3 years ago

I've fixed radius again, so that should also work.. The original code had docblock typehints in place that identified them as strings and when I added actual typehints I didn't check and assumed the docblocks were correct.. Kinda surpised though that none of our QA-tests noticed this.