duo-labs / py_webauthn

Pythonic WebAuthn 🐍
https://duo-labs.github.io/py_webauthn
BSD 3-Clause "New" or "Revised" License
856 stars 171 forks source link

Flask Demo: improve base64 handing #67

Closed gradyy closed 4 years ago

gradyy commented 4 years ago

Errata: webauthn.js line 50: catch clause should log err to console, not credentialCreateOptionsFromServer credentialCreateOptionsFromServer is undefined in the catch clause.

Base 64 handling: call replace() to translate websafe base64 encoding to standard base64 encoding. This commit enables webauthn.js to support websafe base64 challenge strings.

As demonstrated in pull request #8, resubmitting pull request #66 with commits on a separate branch.

jordan-wright commented 4 years ago

Thanks for sending this in @gradyy! Tagging in @nickmooney to give the 👍 on this.

nickmooney commented 4 years ago

Hey @gradyy -- thanks for sending this in! With respect to the Base64 changes, is this just to provide flexibility such that webauthn.js can handle challenges in standard or websafe encodings?

gradyy commented 4 years ago

Yes, the changes prevent websafe encoded base64 strings from causing an error when passed to atob() This translation was already done for user ids and this patch protects all four calls to atob()

With this change, generate_challenge does not need to be restricted to alphanumeric characters. This demo project should fully support base64 encoding to avoid giving the false impression that challenge strings must be or should be limited to alphanumeric characters.

Lifting the restriction allows secrets.token_urlsafe to be a drop in replacement, but the secrets module requires python >= 3.6 There is a backported library in pip but python2-secrets doesn't support python < 2.7 or (3.0, 3.1, 3.2, 3.3).

gradyy commented 4 years ago

The registration variables can be cleared by calling pop() on the session object. This is a more compact and pythonic method of achieving the same effect as calling del on the variables.

class flask.session

The session object works pretty much like an ordinary dict, with the difference that it keeps track of modifications.

dict.pop

If key is in the dictionary, remove it and return its value, else return default.

Does clearing these variables from the session serve a functional purpose? I don't see how previous registration values could affect webauthn_begin_activate because these session variables are immediately overwritten.

gradyy commented 4 years ago

rp_name appears as a magic constant within webauthn_begin_activate. This variable should be declared at the top with the other configuration constants.

In this example, RP_ID and RP_NAME have the same value. Distinguishing the two provides a slightly better example of the use case of these variables.

nickmooney commented 4 years ago

Thanks again for the work here, @gradyy. I am merging this as-is, but making a note to myself that we should likely be using a utility function to ensure that possibly-websafe-Base64 is converted to standard Base64 before calls to atob.

Additionally, you brought up two interesting points: