search

CZ-NIC / django-fido

Django application for FIDO protocol U2F
GNU General Public License v3.0
28 stars 11 forks source link

Record user handle #146

Closed variable closed 2 years ago

variable commented 2 years ago

If we are going to comply with the spec

Since the user handle is not considered personally identifying information in § 14.4.2 Privacy of personally identifying information Stored in Authenticators, the Relying Party MUST NOT include personally identifying information, e.g., e-mail addresses or usernames, in the user handle. This includes hash values of personally identifying information, unless the hash function is salted with salt values private to the Relying Party, since hashing does not prevent probing for guessable input values. It is RECOMMENDED to let the user handle be 64 random bytes, and store this value in the user’s account.

When we set resident_key=True, we should not use username for user.id in user handle, hence this further change to comply with the spec so it returns a uuid4 string instead.

I have taken out the unique=True from user_handle field because:

  1. Allow people to migrate their existing database
  2. The package does not allow multiple key registration for the same user, so even using username as user.id won't get duplicates see update from comment https://github.com/CZ-NIC/django-fido/pull/146#issuecomment-979434854
  3. an uuid4 is unlikely to get duplicates.

Please note. this PR affects https://github.com/CZ-NIC/django-fido/pull/145, the backend was based on the impression that we don't need to store user_handle in Authenticator model, so it uses user__username to perform lookup, if this PR gets merged, I will need to update https://github.com/CZ-NIC/django-fido/pull/145 accordingly.

UPDATE: I incorporated this change into my own project and I feel this change is needed to comply with the spec, so I decided to change #145 to base on this PR.

Happy to discuss

tpazderka commented 2 years ago

I have taken out the unique=True from user_handle field because:

  1. Allow people to migrate their existing database
  2. The package does not allow multiple key registration for the same user, so even using username as user.id won't get duplicates This is not true. The package allows multiple keys per single user.
  3. an uuid4 is unlikely to get duplicates. Shouldn't this return the same uuid4 for different registration attempts for the same user?
variable commented 2 years ago
  1. The package does not allow multiple key registration for the same user, so even using username as user.id won't get duplicates This is not true. The package allows multiple keys per single user.

You are right, I forgot to update this detail after what I found out yesterday, it would allow the same user to register on different physical keys, I realised it was the excludeCredential that prevented this to happen on the same physical key, and I only received my second physical key recently to test this again.

Regarding on point 3, do we want to return the same uuid for the same user on different registration?

If same uuid for the user every time:

variable commented 2 years ago

An idea about user.id generation, I can add in a setting, which allows user to implement their own way to generate it eg.

if callable(SETTINGS.user_id_generation):
    return SETTINGS.user_id_generation(user)
...
tpazderka commented 2 years ago

It looks OK to me.

@ziima @stinovlas ?

variable commented 2 years ago

Hello guys any updates on this PR please?

tpazderka commented 2 years ago

Sorry, we plan/hope to get to that this week. Feel free to poke us if we don't get back to you on Friday.

variable commented 2 years ago

Sorry, we plan/hope to get to that this week. Feel free to poke us if we don't get back to you on Friday.

thank you @tpazderka appreciate it.

tpazderka commented 2 years ago

So we had a discussion with @ziima and decided on the following:

These changes should remove even the small chance of getting duplicates while still allowing multiple tokens per user for two factor login.

variable commented 2 years ago

So we had a discussion with @ziima and decided on the following:

  • Change the definition of Authenticator.user_handle to be unique=True, null=True, blank=True
  • Alter the existing migration to reflect these changes
  • Update the Fido2RegistrationForm.clean_user_handle to return None if resident_key=False
  • Also the typing on the clean_user_handle is incorrect

These changes should remove even the small chance of getting duplicates while still allowing multiple tokens per user for two factor login.

Thanks guys for the recommendation, I have refactored the code accordingly.

variable commented 2 years ago

Don't now why quality failed with py3.8, tox went fine locally

image

tpazderka commented 2 years ago

I believe it is broken on master as well. I will have a look and fix it.

tpazderka commented 2 years ago

Master is now passing, could you please rebase?

variable commented 2 years ago

Please rebase, but otherwise it looks good to me.

done, thanks for fixing the tests.

variable commented 2 years ago

Just a heads up, it seems there is a problem with the js when setting the user_handle in the form object when there is no user_handle in the form generated by django form. I will take a look at it tomorrow.

variable commented 2 years ago

@tpazderka @ziima please review

I have added a commit:

  1. In js added condition to test if user_handle is null before converting the value value
  2. Added user_handle field to Fido2AuthenticationForm, so js can set the form field value without error in point 1.
  3. Fixed admin add authenticator not setting the user_handle field
tpazderka commented 2 years ago

I am still getting an error on user_handle not being filled in.

tpazderka commented 2 years ago

I am still getting an error on user_handle not being filled in.

Nevermind, I had stale JS linked...

tpazderka commented 2 years ago

OK, my testing/review looks good. I have asked @stinovlas to test his workflow as he is using different setup.

tpazderka commented 2 years ago

It looks like the JS requires the user_handle be present in the form and fails if it is not present (with a rather cryptic message).

It should be possible to ignore the missing field and just fill whatever is present.

variable commented 2 years ago

It looks like the JS requires the user_handle be present in the form and fails if it is not present (with a rather cryptic message).

It should be possible to ignore the missing field and just fill whatever is present.

Yeah I overlooked it originally and the JS just stopped executing when assigning value to a non-existing field. The last commit added the user_handle field into the form so it is present in html and the JS can assign value to that form field.

stinovlas commented 2 years ago

It looks like the JS requires the user_handle be present in the form and fails if it is not present (with a rather cryptic message). It should be possible to ignore the missing field and just fill whatever is present.

Yeah I overlooked it originally and the JS just stopped executing when assigning value to a non-existing field. The last commit added the user_handle field into the form so it is present in html and the JS can assign value to that form field.

There are people that don't use the form provided in this library. I suggested that it would be better if JS doesn't crash in the case that field user_handle is not present (since it's only used with paswordless authentication). If the field is necessary, it may break some forms (including mine), but it's not a big change, so I'd also be OK with documenting it as a breaking change.

variable commented 2 years ago

It looks like the JS requires the user_handle be present in the form and fails if it is not present (with a rather cryptic message). It should be possible to ignore the missing field and just fill whatever is present.

Yeah I overlooked it originally and the JS just stopped executing when assigning value to a non-existing field. The last commit added the user_handle field into the form so it is present in html and the JS can assign value to that form field.

There are people that don't use the form provided in this library. I suggested that it would be better if JS doesn't crash in the case that field user_handle is not present (since it's only used with paswordless authentication). If the field is necessary, it may break some forms (including mine), but it's not a big change, so I'd also be OK with documenting it as a breaking change.

The user_record added to the authentication form is optional so I think it doesn't break existing code. I am happy to refactor again to remove the field from python side and make the JS to test if html form has user_handle. Let me know.

variable commented 2 years ago

Oh wait, sorry I misread your comment, thought you said people using the python form instead of the js form.

variable commented 2 years ago

It looks like the JS requires the user_handle be present in the form and fails if it is not present (with a rather cryptic message). It should be possible to ignore the missing field and just fill whatever is present.

Yeah I overlooked it originally and the JS just stopped executing when assigning value to a non-existing field. The last commit added the user_handle field into the form so it is present in html and the JS can assign value to that form field.

There are people that don't use the form provided in this library. I suggested that it would be better if JS doesn't crash in the case that field user_handle is not present (since it's only used with paswordless authentication). If the field is necessary, it may break some forms (including mine), but it's not a big change, so I'd also be OK with documenting it as a breaking change.

The user_record added to the authentication form is optional so I think it doesn't break existing code. I am happy to refactor again to remove the field from python side and make the JS to test if html form has user_handle. Let me know.

I will remove the user_handle from the Fido2AuthenticationForm, fix the unitests, then make JS test if form has the user_handle field before assigning the value.

variable commented 2 years ago

It looks like the JS requires the user_handle be present in the form and fails if it is not present (with a rather cryptic message). It should be possible to ignore the missing field and just fill whatever is present.

Yeah I overlooked it originally and the JS just stopped executing when assigning value to a non-existing field. The last commit added the user_handle field into the form so it is present in html and the JS can assign value to that form field.

There are people that don't use the form provided in this library. I suggested that it would be better if JS doesn't crash in the case that field user_handle is not present (since it's only used with paswordless authentication). If the field is necessary, it may break some forms (including mine), but it's not a big change, so I'd also be OK with documenting it as a breaking change.

The user_record added to the authentication form is optional so I think it doesn't break existing code. I am happy to refactor again to remove the field from python side and make the JS to test if html form has user_handle. Let me know.

I will remove the user_handle from the Fido2AuthenticationForm, fix the unitests, then make JS test if form has the user_handle field before assigning the value.

Please find last couple of commits to reflect the changes

tpazderka commented 2 years ago

I might be missing something, but isn't the user_handle in the AuthenticationForm actually needed?

variable commented 2 years ago

In the passwordless PR I have a separate auth form which has the user_handle field.

On Fri, 28 Jan 2022, 1:38 am tpazderka, @.***> wrote:

I might be missing something, but isn't the user_handle in the AuthenticationForm actually needed?

— Reply to this email directly, view it on GitHub https://github.com/CZ-NIC/django-fido/pull/146#issuecomment-1023165069, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEIIX2NBHTGIQQ3JA4RGU3UYE4EHANCNFSM5IVBTLJQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

tpazderka commented 2 years ago

Merged