Skip to content

Fix Electron SSO handling to support multiple profiles#13028

Merged
t3chguy merged 7 commits intodevelopfrom
t3chguy/poc_riot_desktop_sso_multi_profile
Apr 14, 2020
Merged

Fix Electron SSO handling to support multiple profiles#13028
t3chguy merged 7 commits intodevelopfrom
t3chguy/poc_riot_desktop_sso_multi_profile

Conversation

@t3chguy
Copy link
Copy Markdown
Member

@t3chguy t3chguy commented Apr 3, 2020

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy requested a review from a team April 3, 2020 23:24
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy marked this pull request as ready for review April 7, 2020 13:48
Copy link
Copy Markdown
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm - so does this send the full path to the user data directory to the HS and back? This seems bad in terms of security although I can't think specifically how it would be exploitable, other than being a bit of a privacy leak, or maybe your HS making your Riot write files into weird places.

I guess the problem is that this can be any directory? If we could limit it to only being subdirectories of appData that would make it a bit simpler.

@t3chguy
Copy link
Copy Markdown
Member Author

t3chguy commented Apr 8, 2020

Hmm - so does this send the full path to the user data directory to the HS and back?

indeed

If we could limit it to only being subdirectories of appData that would make it a bit simpler.

we can't though because someone might use --profile-dir: https://github.com/vector-im/riot-desktop/blob/9ebf2171600a0388c7a34ee67d8abfdbbcc431a1/src/electron-main.js#L78

we could encrypt it and urlencode to prevent it being abused

t3chguy added 2 commits April 9, 2020 16:21
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy requested a review from dbkr April 9, 2020 15:24
@dbkr
Copy link
Copy Markdown
Member

dbkr commented Apr 9, 2020

I still have bit of a bad but nonspecific feeling: the key used isn't particularly secure given the HS knows what platform the user is on, and if it's Linux, "/opt/riot-desktop/Riot" is a pretty good guess. I also have a general feeling of nervousness around having bits of crypto code, using different crypto primitives, hanging around for less-oft-used features.

Thinking more about this, one option could be to store currently active SSO sessions with their profile directories in the main profile and then send an sso session ID as a param, looking it up in the map when it comes back?

@t3chguy
Copy link
Copy Markdown
Member Author

t3chguy commented Apr 9, 2020

I like that idea

t3chguy added 2 commits April 9, 2020 21:17
…esolve in a map to our profile data

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Copy link
Copy Markdown
Member Author

t3chguy commented Apr 9, 2020

"/opt/riot-desktop/Riot" is a pretty good guess.

fwiw, its not, the config is in /home/$USER/.config/Riot-$PROFILE/...

Copy link
Copy Markdown
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great - thanks for that!

@t3chguy
Copy link
Copy Markdown
Member Author

t3chguy commented Apr 14, 2020

Will create a matching PR for riot-desktop before landing as to not break nightlies

@t3chguy t3chguy merged commit 3cbc999 into develop Apr 14, 2020
@t3chguy t3chguy deleted the t3chguy/poc_riot_desktop_sso_multi_profile branch November 17, 2020 11:51
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.

New SSO flow doesn't work with non-default profile

2 participants