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

fix webauthn not working as second login #836

Merged
merged 1 commit into from Jun 15, 2023

Conversation

TylerNoblett
Copy link
Collaborator

When linking a webauthn account to an account created with the ashirt login, a user was unable to login using the webauthn account; this PR fixes that

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@jrozner
Copy link
Member

jrozner commented May 16, 2023

I wasn't able to reproduce the original bug this fixes. I logged into Albus using the username/password, added a webauthn credential, logged out, then logged in using webauthn. Is there another step to reproduce?

@TylerNoblett
Copy link
Collaborator Author

I wasn't able to reproduce the original bug this fixes. I logged into Albus using the username/password, added a webauthn credential, logged out, then logged in using webauthn. Is there another step to reproduce?

Interesting - that should be all you need to do 🤔 and this was in the main branch I assume?

I did some digging and found that this bug was introduced when I fixed the webauthn login bug that John found that I was also able to reproduce, which makes sense - I added an attribute when creating a webauthn user but didn't add this to the linking code

webauthnUser := makeWebAuthnUser(user.FirstName, user.LastName, username, user.Email, user.ID, authData.AuthnID, creds)

instead of

webauthnUser := makeWebAuthnUser(user.FirstName, user.LastName, username, user.Email, user.ID, creds)

What I've added here is the same fix that I added to register/finish to get it working (which is sessionData.UserData.AuthnID), which fixed John's (and my) issue, and that same fix worked here.

@JoelAtDeluxe
Copy link
Collaborator

I think I might have fixed this in this PR: #829

@TylerNoblett
Copy link
Collaborator Author

I think I might have fixed this in this PR: #829

Oh interesting - I'm getting the error on main right now (with 829 merged in), but this does raise an interesting thing I've wondered (though it doesn't make a lot of sense); I wonder if different keys work somewhat differently with the webauthn library/spec.

I say this because when I looked into the issue that John and I got, the webauthn code for ashirt hadn't changed, and we hadn't upgraded to a newer version of the go webauthn library, but both of us were getting the 403 forbidden error, AND when I went back to the commit in september where webauthn had been added, I couldn't get it to work (but obviously it had been working for at least Joel and I would assume either Joe or John).

Now that we have a second instance where one version of the code worked some some people, but not another, it makes me wonder if the hardware key itself has anything to do with it (I'm using the touch ID sensor on my macbook fwiw). but IDK why that would be the case; what do you guys think?

@JoelAtDeluxe
Copy link
Collaborator

I've been using a yubikey (I think a series 5). I've used it both with a fiingerprint biometric, and no biometric. I'm unclear why the different systems would behave differently, but I suppose it's something to look into.

@TylerNoblett
Copy link
Collaborator Author

I've been using a yubikey (I think a series 5). I've used it both with a fiingerprint biometric, and no biometric. I'm unclear why the different systems would behave differently, but I suppose it's something to look into.

yeah I'll take a look into it after I figure out some of this gorilla/mux stuff!

@jkennedyvz jkennedyvz merged commit 9798347 into main Jun 15, 2023
9 checks passed
@TylerNoblett TylerNoblett deleted the tn/fix-webauthn-ashirtauth-bug branch October 18, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants