-
Notifications
You must be signed in to change notification settings - Fork 43
RFC 1025 TOTP protected local device #11950
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
base: master
Are you sure you want to change the base?
Conversation
| this way no secret can be obtained from the stolen machine (unless, of course, | ||
| the attacker also has access to the user's TOTP secret). | ||
|
|
||
| This is not a one-size-fits-all solution since its removes the possibility to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| This is not a one-size-fits-all solution since its removes the possibility to | |
| This is not a one-size-fits-all solution since it removes the possibility to |
|
|
||
| ### 2.4 - How is used the secret obtained from the TOTP challenge ? | ||
|
|
||
| Once the TOTP challenge succeeded, the Parsec server send to the client a secret. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Once the TOTP challenge succeeded, the Parsec server send to the client a secret. | |
| Once the TOTP challenge succeeded, the Parsec server sends a secret to the client . |
| > - Also complex during claim operation. Need to add a new | ||
| > `claimer_XXX_finalize_totp_upload_opaque_key` to be called before `claimer_XXX_finalize_save_local_device` | ||
| > - Possible solution: don't both with TOTP during bootstrap/claim. Instead pass | ||
| > in the server configuration if TOTP protection is required, and display a popup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At which level is the TOTP configuration set ? By organization or by server ? Can it be allowed but optional ?
|
|
||
| Upload operation is exposed to the GUI through the libparsec API. | ||
|
|
||
| ```rust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible it would be nice to also include or replace this by the js version as that part should be reviewed by sharks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather stay with the Rust version since this code will be mainly implemented in Rust.
Beside, the change in naming are minimal (mainly switch from snake to camel case), so I don't think the shark team will struggle with this ;-p
| } | ||
| ``` | ||
|
|
||
| ### 5.3 - Changes in `Device(Save|Access)Strategy` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the TOTP config will be added to each access variants, wouldn't be it easier/clearer to pass it alongside the access instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would simplify the structure definition, but at the cost of additional complexity in it usage since all the functions taking a Device(Save|Access)Strategy would need to be modified to take an additional parameter (compared to the current approach where only the top caller and lower platform_device_loader function has to deal with the TOTP config)
On top of that, the TOTP config is related to the device acces/save strategy, so it makes sens to have it in this structure.
I guess we could modify the Device(Save|Access)Strategy to make it a structure that would have two fields primary (i.e. the enum) and totp (the totp config). But again I fear this would cause too much changes compared to just duplicating code in each variant type (however this approach can be tested during implementation to see if it is worth it 👍 )
| let key_from_totp_challenge: SecretKey = ... | ||
| let key_from_primary_protection: SecretKey = ... | ||
| let cleartext = key_from_totp_challenge.decrypt( | ||
| key_from_primary_protection.decrypt(ciphertext) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Not using a shamir system here?
Would allow to effectively use multiple auth system with threshold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An important point here is to be able to validate both protection apart from each other, this useful so that we can inform the end-user of the issue:
- his TOTP code was invalid
- his password was invalid
- an unexpected error occured: the secret obtain from the server (with the TOTP challenge) couldn't be used for decryption
On the other hand, if the ciphertext is encrypted by a single key that itself is derived from multiple secrets, we cannot know which secret was invalid if the decryption fails.
Would allow to effectively use multiple auth system with threshold
YAGNI, this add plenty of complexity and I don't see a real usecase for it now.
The need here is to prevent the client from decrypting the ciphertext without first achieving a challenge with the server.
Hence if we want to have multiple MFA systems that have to be used in conjunction (for instance the user has to provide a TOTP code provided by email, then another one from SMS), then it is something that is implemented has additional APIs exposed by the server, with the final API returning the secret to be used for decryption. So even in this case the secret is still a simple decryption key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a real usecase for it now.
I mean, we are talking about MFA with shamir we could extend the number of share when adding a new auth method and we can easily configure how many method to use (some org, ask for 3 methods 😰)
45748d9 to
1a11e9e
Compare
No description provided.