new Authorization workflow fix #1
Conversation
|
Thank you for your work. Let me know if an unprofessional like me can help with anything. |
@gherpai |
|
Wow, a lot of changes. I can try to make a review (security). For now (quick scan) a few comments/questions:
|
|
@UncleGoogle sorry, i've been editing this as i understand the issue you mentioned in point 1 more.
I thought it was added to address FriendsOfGalaxy#74. I didn't even see #2. The reason it was removed was that it was also broken in the latest steam update and the new auth flow fixed FriendsOfGalaxy#74. As far as security goes, the new flow is more secure than the previous flow. The password is RSA encrypted before we send it along the websocket. Steam gives us that public key, and only steam has the private key. They also seem to generate a new public/private key pair each time we provide a username. Even if it does not change from the last call. The new auth flow also gives us a refresh token, so we don't get any web cookies or credentials. The refresh token can be revoked at any point using the steam app or the desktop application. The new flow is the exact same as what the desktop Steam Application does. If you cannot trust our code with the username and password, you cannot trust The most secure thing to do would be to send it back to us as a post request with a protected JWT but that requires running a local http server and that's absolutely overkill. One security concern that is worth addressing is the use of RSA over pycryptodome. RSA is a pure python implementation, whereas pytcrpytodome is mostly pure but does use cpython. I do not know which is considered more secure. I used rsa for simplicity, but if that's the cost of business, so to speak, i'll switch to pycryptodome |
There's an open PR on my repo (ABaumher#16) that's in progress that is attempting to fix the test cases. as i last heard, we have about half passing, 1/4 failing, and 1/4 hanging. Unfortunately the devs helping me who are working on this have other priorities, so that may take some time. It also looks to update our dependencies to newer versions, so long as they don't introduce breaking changes. One current pinned dependency (
I'm basically the de facto maintainer by this point, so that doesn't really feel any different than it is now. That said, I'd like to be able to hand it off to a group of maintainers (including me) so if for whatever reason I'm unable to keep the plugin up to date we don't have another friends of Galaxy situation. |
|
I`ve got steam back to work with 1.0.5-beta release of steam plugin. |
|
I`ve got steam back to work with 1.0.5-beta release of steam plugin. |
|
Quick note: ~40 files are just dealing with protobuf changes. We now store the Also, i commit often so apologies for the high commit total |
|
As for #2, if they really do not trust gog's browser, it's possible to ask for their username, then provide them an RSA public key. This is straight from steam, we cannot decode anything encoded with this key. Have another program create the public key from the provided modulus and exponent, then provide it their password. Then paste in the generated cipher text. Normally we do this internally but retrieve the password through gog's browser. Adding this "feature" adds another layer of complexity, but by all means, let's be more 'secure' than the steam client itself |
|
Hi, thanks for the response. regarding FriendsOfGalaxy#2:
yes, that difference was exactly about that: gog browser is not a Steam, but more importantly we (plugin) are not Steam, so users who don't read the plugin code can't know if they are not a victim of a phishing. And this is really a thing -- it is possible to use Galaxy browser to pass bare creds to plugin code and send it to the malicious actor. From that perspective I understand why GOG is hesitant about changing the cfg source which also means that plugins will autoupdate based on "update_url" in manifest.json. Also because of that Steam may be suspicious for such projects as our -- just with their users protection in mind. That being said, in my perspective "public profiles" backend was a nice trade-off (I was using it privately too), so people don't have to choose to trust to anyone if they are scared. But if it does not work any longer so it is definitely not a thing to block by me.
for sure not for now, but sounds great. Probably not as a separate backend but as a variant of current login flow?
I see, it is always better (difficult to achieve open source world) to have more active maintainers. The idea of Nebula in current shape was to maintain a new Trying to compare both solutions: So from the one hand (Nebula auto-sync your repo)
From the other hand (Plugin maintained directly)
What do you think @ABaumher, @urwrstkn8mare, @Mixaill @AndrewDWhite ?
No worries, I just need to find a free hour or two, unfortunately, my free hour today were just eaten on writing this messages 😂 |
|
I guess the question is do they trust the public key we give them? Because in theory, if they don't view our source code, they could think we're just giving them a public key we generated, and we're still guilty of that phishing attack. I'll implement the "here's a rsa key, generate your own encoded text" flow as an option for the current flow during the overall rework. It's what i had when testing the new flow but it was not worth the headaches of making it user friendly. It's rather difficult to cleanly swap back to the username page if you realize you mistyped your username. With the rework, I'm trying to drastically simplify the code structure, which should make your review process (and by extension, GOG's) much easier. All the callback nonsense between protobuf client, protocol client, and websocket client will be removed. I'm not using asyncio queue to separate the websocket and gog plugin code, because that's caused no shortage of headaches (i think this was added because they misunderstood how async works. It's not multithreading, it's just delaying execution instead of blocking). The backend code will be 3 major classes: model, view, and controller, and a few helpers to parse data and keep the files clean. Type hints will be added everywhere i can (and haven't already) Currently i have a branch that updates the deps and switches to betterproto instead of protobuf. This gives us compile-time classes for the protos which are significantly easier to work with. If this all works as intended i can start simplifying the protobuf/protocol/websocket client code. A final option would be to truly implement parallel processing between the model (steam via websocket) and the controller (the code that calls them). Since python has GIL, multithreading is essentially useless, but we could do multi-process code instead. This in theory could speed up i/o on massive libraries since we don't need to wait for the gog-side code to process them |
|
As for the pros/cons of solo maintainer and upstream pulls vs maintained here, I'd greatly prefer having a team develop the plugins for the following reasons:
But i do recognize that maybe Nebula should not be that group. If we're trying to take over the cfg and make it so we're the security team that prevents malicious users from inserting bad faith code, then we should not be accepting PRs, etc. Would it be possible to compromise? Have a second group that includes everyone here that maintains the plugins and we use that as our upstream? That group could accept users a little more leniently, and ensure the code is kept clean. Then, Nebula could do security reviews before merging the upstream source, preferably with gog members as part of the team so gog can ensure they aren't letting malicious code slip through. |
sounds good, but a bit too complex for now. I hardly have a time to answer in those threads. I got hyped some time ago (we all did whoever made his own plugin), and get burned out with time i think. You joined the party late, but with a big inpact. This seems to be the last chance for GOG to regain community trust, otherwise nobody will waste time any longer. That being said, let just make working Steam plugin ready and cfg ready without complication and see what happen. UPDATE 10.06:
|
There was a problem hiding this comment.
I didn't finish but I'm publishing all comments I have because I got sick and don't want to block you. I'm sorry if some comment are more like drafts -- I still didn't understand all the flow and didn't test it very well.
Some comments are not only about security, because it wasn't decided if it goes only with the external org or inside Nebula. But you can create internal issues for some, this MR is too big to handle it at once.
|
From your current review, i definitely need to clean up the readmes and the comments. I'd like to have type hints everywhere and annotated classes and functions but I'm doing that as part of the larger refactor and a lot of it will be thrown out or changed so I'm not doing it right now |
|
Also, i tend to comment above a function instead of below, because that's what c/++, C#, Java all do. so a lot of these need to be fixed to be part of function, below the definition. |
|
The reviewed issues marked so far have been addressed. The readme is still a little rough; let me know if it still needs updating. bumped Release version to 1.0.6 to address a bug for long passwords and/or illegal characters |
|
as i look to refactor i'm learning there's a lot i can clean up, but the plugin is very stable at the moment, though extremely large libraries still hit bottlenecks. We can try band-aid fixing them in the meantime but the only real solution is to rewrite the code and optimize a little better. They tend to sync up, it just takes a lot longer than it should. |
|
alr exams done ill try to help out as much as possible
Thanks @ebonato! I think @UncleGoogle @AndrewDWhite and @Mixaill also deserve credit for the inception of this project.
I'm assuming 'plugin maintained directly' means maintained on this org? I think its a good idea in the case of this plugin.
wholeheartedly agree thanks @ABaumher for your work. also a second group for maintaining the plugins are bad idea imo. i think as long as we have pull request security the rest could be manually maintained until someone sets up the github action stuff. i remember working on that but it was so long ago it could be outdated or something so i'll be reviewing that. btw the pull request security would only be on the master/main branch so iterative work can be done on other branches.
i havent had a chance to go through the changes as @UncleGoogle has but from what you said it seems that the plugin is stable. if you wanna do the bandaids and push it out we could do that. if you'd rather just do it right the first time and do a rewrite i'd recommend merging this pr into a branch something like |
|
also @ABaumher ic that u and https://github.com/ValvePython/steam developed the new auth flow simulateneously. would it simplify the code if you used that package? i do understand that may negate alot of the hard work you put into implementing the auth flow yourself here. i could be very wrong btw i dont have the same experience as you |
auto-copy-mac.command
Outdated
There was a problem hiding this comment.
what is this and auto-copy-windows.bat for?
There was a problem hiding this comment.
Since i don't have currently have it set up where you can get it by searching through gog (not sure how that works) you need to download my zipped releases, and replace the old code with the stuff in the zip. The batch file does that for you.
There's a MacOS command equivalent as well, which pulls double duty. Since I've been building the plugins myself instead of using github actions, macos security flags all the cpython the included modules have. This script still procs security, but if you allow it, it clears all the security flags on the cpython scripts.
My latest releases have a separate zip with the installer.
|
At this point it's more stable than version 1.0.3 (the last release from FoG) and better at pulling achievements. we haven't had any regressions as far as i can tell. The full on refactor is ongoing, but i have it on a separate branch on my repo. I could close this and wait until that's done for a PR, but that could take quite a while. If you'd prefer to host my conversion branch i can do a pr here and let you create a branch for it, or you could contribute on my repo. I'm not sure which is easier or preferred. Not gonna lie, the place i'm doing active conversion is in an extremely rough, mostly broken state. I've been stripping down functions to nothing, adding type hints and documentation, then re-implementing them as i go. A good portion will remain the same or get slight optimizations, but i've found that a lot of code was poorly designed and going through each function like this lets me find them. I'm sure there's a better way but it's been working for me. I'm 95% positive i can clean up the bottlenecks with a rewrite so any costs we take from using betterproto will be irrelevant. |
|
Seeing as you are both better at python than me, i'm wondering if you can take a look at a parallelizer i wrote. Some of our code converts Steam protobuf messages to their GOG equivalents, and for large libraries (>1k licenses, though we have some testers above 10k licenses) this blocks the other tasks. This has been especially annoying when it takes so long gog assumes the plugin is not responding and sends us a shutdown. I think using edit: you can add a |
|
Setting up the repo on my M1 has proved annoying but I think I'm getting there. I agree that the original one is already broken so if this one is working and I'm pretty sure there's nothing malicious it's pretty safe to merge (consider this my unofficiak approval). @UncleGoogle probably will have more to say when he's free as he's done a more in-depth review During your rewrite if possible try breaking it down into smaller chunks unless the rewrite is completely changing the major structure of the plugin and logic. This makes it easier to review and also allows the prod code to get better incrementally. I'll have a look at the parallelise soon but I'm not means an expert. Let's leave the better python programmer title up to debate :) Also once merged I think we should just manually create releases for now as we can make sure releases are secured by protecting the master branch of the cfg repo. |
|
If it's not too much of a hassle, can you document how you set it up on Mac? I use windows and have borrowed a Mac for setup but it was not pleasant for me because i am not confortable on a Mac. I'm not sure if you prefer pycharm, xcode, or vs code, but any option would be preferred. The project has an existing |
|
As for the rewrite, it's unfortunately looking like a full change. I might be able to tag it incremenetally so you can review the changes a little more easily. I could also do it one file at a time and then worry about integration hell when i get there. |
…ers or are too long. Cleans up general code as per merge request fix suggestions. updated the readmes for clarity. fixed bug introduced into 1.0.6 beta during cleanup Update dev.txt This is fixed in other branches but required for new installs, and especially on MacOS.
…est to point there as well. cleaned up user info cache (again) so the is_initialized call is clean (tested it). and updated enum message to be less scary.
enums: Corrected type hint in `to_TwoFactorMethod` steam_auth_polling_data: Corrected 'any' check in 'has_valid_confirmation_method'. No 2FA's enum value is 0, which was being coerced to False. backend_steam_network: The value from no two-factor is now returned instead of set to result and unused. The _handle_steam_guard_none function now properly calls the finish auth process so we're actually logged in. previously it'd just short-circuit that process and then error later. Properly does this, expected wrong UserActionRequired value and also wasn't awaited. in other words, several small, dumb mistakes. bumped manifest and current version to 1.0.7 as a result of these changes. Hardened the User Info Cache code so it would not error if the data somehow was cleared to Null and written to the credentials storage. It will also write an empty dict to the credentials instead of a dict of nulls. Stripped the leading and trailing whitespace characters during 2FA code inputs so the whitespace wouldn't make your 2FA fail. Properly type-hint 2FA code so it would resolve nicely in any IDE or MyPy Fixed broken urls in current_version,json Updated version.py to have 1.0.7 with relevant changelog information Hardened the User Info Cache code so it would not error if the data somehow was cleared to Null and written to the credentials storage. It will also write an empty dict to the credentials instead of a dict of nulls. Stripped the leading and trailing whitespace characters during 2FA code inputs so the whitespace wouldn't make your 2FA fail. Properly type-hint 2FA code so it would resolve nicely in any IDE or MyPy Fixed broken urls in current_version,json update gitignore to avoid .gz files. The mac with installer is uses that compression. partially updated the batch file to deal with issues, still needs some tweaking
|
Squashed what i could, but since this is write protected i can't do that directly, and had to do it to my master. Once this is approved (again, sorry all), we should be able to just rebase and merge and be good. |
|
I will admit i got pretty annoyed at the whole interactive rebase so about halfway through i stopped cleaning up the messages. If you need me to clean up the commit messages i can go back and do it (but i don't want to XD) |
ABaumher
force-pushed
the
master
branch
2 times, most recently
from
4da4926
to
d856a02
Compare
Fixed license.txt - missed a merge commit message mistake Updated auto-copy-windows to use a full path. Was not done on nebula so i didn't break the reviews, but now that squashing did that anyway, i might as well.
|
ok, that should finally be all the squash merges handled. I actually missed removing a conflict message from my merge and visual studio does not normally enable the option to force push, so it was trying to pull than push and causing a whole big mess. I have that set properly on my other computer but not this one. It's now set here as well and should stop being a pain in the a** |
fix mobile confirm flow. not pretty but should work
(try to) fix the protobuf file conflict voodoo
There was a problem hiding this comment.
I'm getting a 😬:
method=initialize_cache, params={'data': '****'}
2023-06-24 19:36:55,620 - galaxy.api.plugin - ERROR - Unhandled exception during `handshake_complete` step
Traceback (most recent call last):
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/galaxy/api/plugin.py", line 325, in _initialize_cache
self.handshake_complete()
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/plugin.py", line 103, in handshake_complete
self.__backend = self._load_steam_network_backend()
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/plugin.py", line 115, in _load_steam_network_backend
return SteamNetworkBackend(http_client, ssl_context, persistent_storage_state, persistent_cache, update_user_presence, store_credentials, add_game)
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/backend_steam_network.py", line 122, in __init__
self._load_persistent_cache()
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/backend_steam_network.py", line 126, in _load_persistent_cache
self._games_cache.loads(self._persistent_cache["games"])
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/steam_network/games_cache.py", line 185, in loads
self._storing_map = LicensesCache.from_json(cache['licenses'])
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/dataclasses_json/api.py", line 78, in from_json
return cls.from_dict(kvs, infer_missing=infer_missing)
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/dataclasses_json/api.py", line 85, in from_dict
return _decode_dataclass(cls, kvs, infer_missing)
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/dataclasses_json/core.py", line 194, in _decode_dataclass
infer_missing)
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/dataclasses_json/core.py", line 254, in _decode_generic
res = _get_type_cons(type_)(xs)
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/dataclasses_json/core.py", line 298, in <genexpr>
for x in xs)
File "/Users/*/Library/Application Support/GOG.com/Galaxy/plugins/installed/steam_ca27391f-2675-49b1-92c0-896d43afa4f8/dataclasses_json/core.py", line 150, in _decode_dataclass
field_value = kvs[field.name]
KeyError: 'type'
i think unfortunately it changed something cause this error wasn't happening before im pretty sure
EDIT: it happens on master_not_nebula as well am i doing something wrong?
I will admit i got pretty annoyed at the whole interactive rebase so about halfway through i stopped cleaning up the messages. If you need me to clean up the commit messages i can go back and do it (but i don't want to XD)
yeah it is really annoying if you wanted i could've done the chore (least i could do). in the future lmk if u wan me to do any rebasing
| @@ -0,0 +1,12 @@ | |||
| { | |||
| "name": "Galaxy Steam plugin Version 2", | |||
There was a problem hiding this comment.
| "name": "Galaxy Steam plugin Version 2", | |
| "name": "Steam plugin for GOG Galaxy", |
not version 2 yet!
There was a problem hiding this comment.
Was trying to differentiate between ours and FoGs.
Am open to suggestions on a better name
There was a problem hiding this comment.
I'm not sure if this "name" makes any practical difference in Galaxy.
"version" is more important. 10.7 is ok for me, 2.0 is ok for me too as there is even a braking change (public profiles backend)
|
@urwrstkn8mare if you've been testing the better proto deps branch that key was renamed to I don't think our cache is forwards compatible. It's backwards compatible but you aren't expected to go back to an older version when the newer version is available. Unfortunately that branch is still in development so it's probably causing you issues. Idk if we bump the cache version (which is only stored in the games cache, idk why) so there's no way to recover in the legacy code. Deleting your db file (i recommend backing it up so you don't have to wait for it to fill back up when switching back to development) will fix it. |
now I regret about what I proposed 🙈 . I hope you won't have to resolve the same conflicts again when you start to rebase your branches onto newly rebased |
|
One step closer to getting this fixed for everyone! Good job, you've all put a lot of work into this, you deserve some praise! |
|
I've created a release: https://github.com/GOG-Nebula/galaxy-integration-steam/releases/tag/v1.0.7 Feel free to upload the zips and modify the description however you'd like |
|
@urwrstkn8mare Nebula responsibility We approved the plugin when installing from source as described in the Readme ;) Current plan to do it better is to have:
|
|
@UncleGoogle that sounds good. The only issue with automated release process is that unless we can figure out a way for maintainers to modify releases even if releases are automatically created they can be modified by members of this repo without changing the link to the file. |
I mean our automated release instead of creating actual github "release" object, can push commit to the repo on master branch and also commit a current_version_.json with a absolute link to commit sha (that would be 2 additional commits though).n |
As of March 2023, Steam updated their authentication flow for steam network connections. See FriendsOfGalaxy#159
This version implements the new auth flow. PyTest is largely broken; i do not expect this PR to go through initially. But i would like for it to be reviewed so we can make any changes you all deem necessary