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

Add support for Web Authentication API #17972

Closed
wants to merge 1 commit into from

Conversation

MauricioFauth
Copy link
Member

@MauricioFauth MauricioFauth commented Dec 17, 2022

Adds a two factor authentication plugin that supports FIDO2/WebAuthn security keys.

The automatic migration from FIDO U2F to FIDO2 WebAuthn is not implemented yet.

Some of the functions in webauthn.js file are copied from the @web-auth/webauthn-helper Yarn package. I'll probably rewrite them to avoid installing this package or I'll install another package. It's basically for encoding and decoding base64url strings.

@williamdes, Do you have more than one security key? If you do, could you please test using a different key?

Adds a two factor authentication plugin that supports FIDO2/WebAuthn
security keys.

- Fixes phpmyadmin#17229

Signed-off-by: Maurício Meneghini Fauth <mauricio@fauth.dev>
@codecov
Copy link

codecov bot commented Dec 17, 2022

Codecov Report

Base: 48.58% // Head: 46.45% // Decreases project coverage by -2.13% ⚠️

Coverage data is based on head (fb62602) compared to base (0314730).
Patch coverage: 7.10% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             QA_5_2   #17972      +/-   ##
============================================
- Coverage     48.58%   46.45%   -2.14%     
- Complexity    16747    16808      +61     
============================================
  Files           602      603       +1     
  Lines         62308    71634    +9326     
============================================
+ Hits          30275    33278    +3003     
- Misses        32033    38356    +6323     
Flag Coverage Δ
dbase-extension 47.19% <13.33%> (-0.02%) ⬇️
recode-extension 47.18% <13.33%> (-1.17%) ⬇️
unit-7.2-ubuntu-latest 47.20% <13.33%> (-0.03%) ⬇️
unit-7.3-ubuntu-latest 47.59% <7.40%> (-3.42%) ⬇️
unit-7.4-ubuntu-latest 47.60% <7.40%> (-3.41%) ⬇️
unit-8.0-ubuntu-latest 47.71% <6.87%> (-2.17%) ⬇️
unit-8.1-ubuntu-latest 47.68% <6.87%> (-2.20%) ⬇️
unit-8.2-ubuntu-latest 47.66% <6.87%> (-2.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...es/classes/Controllers/Export/ExportController.php 57.54% <0.00%> (+15.51%) ⬆️
libraries/classes/Export.php 6.36% <0.00%> (-0.19%) ⬇️
libraries/classes/Plugins/Export/ExportCsv.php 93.16% <0.00%> (-0.82%) ⬇️
libraries/classes/Plugins/Export/ExportLatex.php 95.15% <0.00%> (+0.06%) ⬆️
...braries/classes/Plugins/Export/ExportMediawiki.php 93.52% <0.00%> (-1.60%) ⬇️
libraries/classes/Plugins/Export/ExportOds.php 94.85% <0.00%> (+3.03%) ⬆️
libraries/classes/Plugins/Export/ExportOdt.php 95.44% <0.00%> (+6.24%) ⬆️
libraries/classes/Plugins/Export/ExportPdf.php 56.55% <0.00%> (-0.08%) ⬇️
...ibraries/classes/Plugins/Export/ExportPhparray.php 88.50% <0.00%> (-2.96%) ⬇️
libraries/classes/Plugins/Export/ExportSql.php 80.24% <0.00%> (+0.09%) ⬆️
... and 326 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@williamdes
Copy link
Member

@williamdes, Do you have more than one security key? If you do, could you please test using a different key?

Yes, it's a recommendation. I will test using my two keys :)

@@ -93,7 +94,11 @@
"tecnickcom/tcpdf": "For PDF support",
"pragmarx/google2fa-qrcode": "^2.1 - For 2FA authentication",
"bacon/bacon-qr-code": "^2.0 - For 2FA authentication",
"code-lts/u2f-php-server": "For FIDO U2F authentication"
"code-lts/u2f-php-server": "For FIDO U2F authentication",
"web-auth/webauthn-lib": "^3.3 - For FIDO2/WebAuthn authentication",
Copy link
Member

Choose a reason for hiding this comment

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

As it adds one more library and it costs a lot of maintenance for me, maybe we can pick the code or add the support on the actual library

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by pick the code?

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 could try to write a simple implementation myself, to simplify the packaging, but we should add that package for master branch.

Copy link
Member

Choose a reason for hiding this comment

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

This should also be for QA
We can maybe cherry pick or adapt the current library to handle webauthn

The cost of a new library to package in Debian is so high

If we can have only one library it would be awesome

Copy link
Member Author

Choose a reason for hiding this comment

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

This should also be for QA

I meant writing a simple implementation for QA to avoid needing to package, but requiring the third party package in master.

We can maybe cherry pick or adapt the current library to handle webauthn

The cost of a new library to package in Debian is so high

If we can have only one library it would be awesome

I'm not sure if I'm following you here. What do you mean by having only one library?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, let me know
Reducing it to only one library is a good idea, whatever the lib is

That's okay if I packaged a lib for nothing
It's better if I know it now before debian freezes

Copy link
Member Author

Choose a reason for hiding this comment

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

When it will freeze?

Copy link
Member

Choose a reason for hiding this comment

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

Start of 2023, I found this article https://www.linuxadictos.com/en/they-already-released-the-debian-12-bookworm-freeze-date.html
It was twitter or mailing list announced if you want to see the official dates

I am currently pending acceptation for slim psr7 to be able to package 5.2
Yeah here we are ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there another PSR-7 debian package? Use can use those instead.

Copy link
Member

Choose a reason for hiding this comment

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

yes, niholm's but we depend on some cool things from slim

dont worry it should be accepted soon

@williamdes
Copy link
Member

williamdes commented Dec 22, 2022

I configured my key using Firefox https and tried to use it on Brave http

TypeError: Cannot read properties of undefined (reading 'get')

This is because you need to do like the other implementation, prevent if HTTPS is not active. It does not work without as far as i know

@williamdes
Copy link
Member

williamdes commented Dec 22, 2022

@williamdes, Do you have more than one security key? If you do, could you please test using a different key?

There is no way to add a second one in the preferences page :/ So I can not test it for now ;)

@MauricioFauth
Copy link
Member Author

@williamdes, Do you have more than one security key? If you do, could you please test using a different key?

There is no way to add a second one in the preferences page :/ So I can not test it for now ;)

This is a phpMyAdmin's limitation. I meant test if using a different key to authenticate shows an error. Could you also test the custom server in #17989?

@MauricioFauth
Copy link
Member Author

Closing this in favor of #17989.

@MauricioFauth MauricioFauth self-assigned this Dec 22, 2022
@MauricioFauth MauricioFauth deleted the webauthn branch January 16, 2023 21:35
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.

None yet

2 participants