duosecurity/duo_universal_nodejs

Wrong sub in second exchange token

lukashroch opened this issue · 3 comments

I think this one (c130fc8) breaks the login flow, although the issue is not with node.js implementation. From this point it starts to verify the username in incoming token payload.

But if you have two or more usernames / aliases defined in admin section, different username is returned.

E.g. user logs in with username1, which is stored for verification, user goes through prompt successfully. Second exchange then returns payload with token signed with subject having username2 -> which then mismatch and rejected.

@jeffreyparker We had this issue a few times during initial development. I'd bet that the 'does the username match?' code is looking at the username claim in the JWT? If so, it needs to look at the preferred_username.

username will contain the primary username of the Duo user record.
preferred_username will have the username that was originally sent to Duo in the initial request.

See https://github.com/duosecurity/duo_universal_python/blob/main/duo_universal/client.py#L309

This PR should fix this. Partially reverts c130fc8, where the verification using subject claim was introduced.

@AaronAtDuo , @jeffreyparker ... though shouldn't the token subject be signed with initiator username in first place? I mean why would 3rd need to know the primary Duo record information? Looks like a bit of information leak... I guess 3rd party implementing MFA against provided "application credentials" are trusted to some level. But I think trust should not be implicit in authentication flow ... if they do not need such information. This can disclose additional information Duo Admins might not want to / or are not aware of. E.g. allowing 3rd party MFA implementation against different set of usernames linked with primary duo records using this aliases features.

PR was merged and new version released.

As to the username question, the cat is well out of the bag at this point and we can't change it.

Thanks again lukashroch!