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

[WIP] WebAuthn implementation #9451

Closed
wants to merge 1 commit into from
Closed

[WIP] WebAuthn implementation #9451

wants to merge 1 commit into from

Conversation

e3b0c442
Copy link
Contributor

@e3b0c442 e3b0c442 commented Dec 21, 2019

Fixes #6892.

This PR is tracking my progress on updating the legacy U2F security key implementation to use standard WebAuthn, as discussed in #6892. I'm opening this as a draft PR so that my (slow) progress can be tracked and discussed in real time. If you have any feedback at all as I progress on this, I welcome it! I will convert the PR to a standard PR once all of the pieces are in place.

This change will replace the https://github.com/tstranex/u2f module with https://github.com/duo-labs/webauthn https://github.com/e3b0c442/warp on the backend with the necessary model adjustments and remove the Google u2fApi javascript library in favor of the WebAuthn standard API that is now universally available across current browsers. There will be no changes to the registration or login flows at this time -- that is to say, I'm only implementing the familiar 2nd-factor auth flow, and not the single-factor passwordless flow. I'm choosing to make this a clean break due to the fact that 1) All browsers that supported U2F now support WebAuthn and have for some time, and 2) while existing U2F credentials can be used for WebAuthn, credentials created with WebAuthn APIs cannot be utilized by U2F.

Here is the list of subtasks as I see them now, in the general order I expect to complete them (to be updated as additional things inevitably pop up):

  • New WebAuthn credential model
    • Implement credential model
    • Database migration for new credential model
    • Tests for new credential model
    • Database migration for legacy U2F registrations to new credential model
  • WebAuthn instance/handler
    • Add WebAuthn handler initiation to web app startup
    • Determine best way to get handler to routes (middleware?)
  • WebAuthn login flow
    • New routes and handlers using duo-labs/webauthn e3b0c442/warp
    • New JavaScript using standard WebAuthn
  • WebAuthn registration flow
    • New routes and handlers using duo-labs/webauthn e3b0c442/warp
    • New JavaScript using standard WebAuthn
  • Update strings for WebAuthn
  • Clean up legacy U2F
    • Remove legacy U2F models
    • Remove legacy U2F settings
    • Database migration to remove legacy U2F registration table
    • Remove legacy U2F API routes
    • Remove tstranex/u2f vendoring
    • Remove u2fApi Javascript library
    • Remove legacy U2F strings

Planned changes vs U2F implementation

  • No user-configurable options. Will not use the optional icon, and relying party ID and origin will be inferred from ROOT_URL, instead of being defined explicitly as options. My opinion is that these are irrelevant, as the RPID and origin must match, meaning that there is no way to register a credential for a domain different than the domain you are visiting.

@codecov-io
Copy link

Codecov Report

Merging #9451 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9451      +/-   ##
==========================================
- Coverage   41.52%   41.46%   -0.07%     
==========================================
  Files         570      571       +1     
  Lines       74639    74705      +66     
==========================================
- Hits        30995    30977      -18     
- Misses      39806    39890      +84     
  Partials     3838     3838
Impacted Files Coverage Δ
models/error.go 31.79% <0%> (-0.3%) ⬇️
models/webauthn.go 0% <0%> (ø)
modules/indexer/code/indexer.go 44.73% <0%> (-10.53%) ⬇️
services/pull/check.go 48.59% <0%> (-4.93%) ⬇️
modules/process/manager.go 74.69% <0%> (-3.62%) ⬇️
services/pull/temp_repo.go 31.62% <0%> (-2.57%) ⬇️
models/gpg_key.go 55.03% <0%> (-0.56%) ⬇️
models/repo_list.go 74.07% <0%> (+0.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f42e03...48a3793. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 21, 2019
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 21, 2019
@e3b0c442
Copy link
Contributor Author

I have a request for anyone looking at this -- if you have a fairly large user base with a large number of U2F registrations and would be willing to provide me a dump of the u2f_registration table, I would sincerely appreciate it. There shouldn't be anything sensitive in that table, just public keys. I just want to make sure the database migration works at a scale larger than me and the four YubiKeys I own.

Feel free to encrypt it with my GPG public key if you are concerned about public exposure.

@e3b0c442
Copy link
Contributor Author

The data model portion should be complete at this point.

There is a failing sqlite migration test that I haven't been able to nail down yet. If anybody has any clues here, I would sure appreciate a hand.

var WebAuthn *webauthn.WebAuthn

//Init initializes the WebAuthn instance from the config.
func Init() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% happy with this. I would prefer to use webauthn.New() in order to validate the settings, but this routers.GlobalInit() seems to be the best place to put this and there is no error handling there. Open to suggestions.

@e3b0c442
Copy link
Contributor Author

Quick status update here... while working to integrate duo-labs/webauthn, I ended up encountering a show-stopper bug. I have submitted an issue to have it addressed, but it may not be due to the possibility that making the change could affect others' implementations who are using the library.

That issue along with some other concerns I have about the design of that library (the large number of dependencies and its attachment directly to http requests in particular) is leading me to explore alternatives. I will do that while I wait for resolution on the duo-labs/webauthn showstopper.

@e3b0c442
Copy link
Contributor Author

Another quick status update.

I ran into more issues trying to adapt the existing flows to duo-labs/webauthn which would have required backwards-incompatible changes to duo-labs/webauthn to accomplish within the macaron context. This led me to write a new WebAuthn implementation https://github.com/e3b0c442/warp that is standalone and implementation-agnostic (extensible, not tied to the http.Request object). I've made that transition on my local branch and will push the changes once I've done one or two more iterations on the warp library and gain some confidence in the stability of the API.

Ultimately I believe this will be the cleaner way to go, but willing to discuss any concerns around this direction.

@stale
Copy link

stale bot commented Mar 18, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Mar 18, 2020
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Mar 19, 2020
@stale stale bot removed the issue/stale label Mar 19, 2020
@e3b0c442
Copy link
Contributor Author

This hasn't fallen off of my radar, it's just not my day job. As mentioned previously, anybody else can feel free to pick this up and run with it, e3b0c442/warp should be stable enough at this point.

@6543
Copy link
Member

6543 commented Apr 1, 2020

@e3b0c442 no problem just feed the stale bot some time ;)

@haraldrudell
Copy link

What would be really nice is Webauthn using Touch ID in ungoogled-chromium using challenge-response inside TLS which should be possible on the macOS Big Sur public beta available since 9/30/2020

https://webauthn.io/ has golang source code links. The site works, the golang code seems to need some help

Basically: everyone uses MacBook Pro, nobody uses Safari, fingerprint bio-metrics is safe

challenge response inside TLS is completely decentralized

The @google implementation is a privacy nightmare

@e3b0c442
Copy link
Contributor Author

e3b0c442 commented Jan 1, 2021

2020 unfortunately was not kind to me time wise. I'm going to close this PR so that it doesn't deter anybody else that wants to from taking a crack. If I get to the point where I have something fully functional I'll open a new one.

@e3b0c442 e3b0c442 closed this Jan 1, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Feb 11, 2021
@6543
Copy link
Member

6543 commented Jan 14, 2022

-> #17957

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement WebAuthn support
6 participants