Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement Username and Password authentication #2

Closed
wants to merge 89 commits into from
Closed

Implement Username and Password authentication #2

wants to merge 89 commits into from

Conversation

AnonymusNW
Copy link
Contributor

@AnonymusNW AnonymusNW commented Jan 30, 2024 •
edited
Loading

Implementing Username and Password authentication according to RFC1929

@AnonymusNW AnonymusNW force-pushed the username-and-password-authentication branch from 02be171 to 59b3bf7 Compare January 30, 2024 20:40
TSRBerry
TSRBerry requested changes Jan 30, 2024
View reviewed changes
Copy link
Owner

@TSRBerry TSRBerry left a comment

Choose a reason for hiding this comment

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

This repo is definitely not ready for contributions, since it is very early in development, but I do appreciate some help. So although I'm requesting changes, it's mostly due to me not updating the code properly, so without changes we will run into issues in the future.

The IProxyAuth interface and many other components of this library were initially copied from this PR: https://github.com/Ryujinx/Ryujinx/pull/5989

The PR only implements the client side of the protocol and I haven't updated everything to support both sides yet.

You can also look into the unit tests there, since I would like to have unit tests for all the authentication methods (and the rest of this codebase).

TSRBerry
TSRBerry requested changes Feb 5, 2024
View reviewed changes
Copy link
Owner

@TSRBerry TSRBerry left a comment

Choose a reason for hiding this comment

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

As requested, a review of your latest changes. Thank you for working on this!

@AnonymusNW AnonymusNW force-pushed the username-and-password-authentication branch from 51efe88 to 952d428 Compare February 5, 2024 14:14
@AnonymusNW AnonymusNW force-pushed the username-and-password-authentication branch from 952d428 to caafd11 Compare February 6, 2024 10:56
@AnonymusNW AnonymusNW marked this pull request as ready for review February 6, 2024 10:57
@AnonymusNW AnonymusNW requested a review from TSRBerry February 6, 2024 10:57
TSRBerry
TSRBerry requested changes Feb 7, 2024
View reviewed changes
Copy link
Owner

@TSRBerry TSRBerry left a comment

Choose a reason for hiding this comment

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

There are a few formatting issues and the client side implementation of Authenticate() is missing.

The packets still need some work, but we are slowly getting there.

@AnonymusNW AnonymusNW marked this pull request as draft February 7, 2024 06:03
@AnonymusNW AnonymusNW force-pushed the username-and-password-authentication branch from caafd11 to 70175fa Compare February 8, 2024 14:23
@AnonymusNW AnonymusNW marked this pull request as ready for review February 8, 2024 14:25
@AnonymusNW AnonymusNW force-pushed the username-and-password-authentication branch from 8877738 to 67ee2a3 Compare February 23, 2024 12:40
@AnonymusNW AnonymusNW requested a review from TSRBerry February 24, 2024 15:46
@TSRBerry TSRBerry force-pushed the username-and-password-authentication branch from c60b5de to 8110169 Compare February 25, 2024 18:26
TSRBerry
TSRBerry requested changes Feb 25, 2024
View reviewed changes
Copy link
Owner

@TSRBerry TSRBerry left a comment

Choose a reason for hiding this comment

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

I rebased your PR branch, after I just pushed a lot of changes, I hope that's okay for you.

There are a few things I noticed:

  • The response packet doesn't seem to have been adjusted to use Bytes directly.
  • Currently there is no client-side implementation for UsernameAndPassword.Authenticate() available. Could you add that?

Also could you move the packets to RyuSocks.Auth.Packets.UsernameAndPassword?

The last thing that would be missing is an entry in CHANGELOG.md. Could you add that as well?

@AnonymusNW AnonymusNW requested a review from TSRBerry March 5, 2024 13:51
TSRBerry
TSRBerry requested changes Mar 8, 2024
View reviewed changes
Copy link
Owner

@TSRBerry TSRBerry left a comment

Choose a reason for hiding this comment

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

Only need you to look at that changelog entry again, the rest is fine! Only other comments are some styling issues.

RyuSocks/Auth/UsernameAndPassword.cs Outdated
Comment on lines 32 to 39
public string Username;
public string Password;
public bool IsClient;
public Dictionary<string, string> Database
{
get;
set;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm somewhat unhappy with this, but I need to think about it a bit longer.
I don't want you to change anything here, since it looks fine, but design-wise I might want to change how this works eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally

@AnonymusNW AnonymusNW requested a review from TSRBerry March 9, 2024 17:56
TSRBerry
TSRBerry reviewed Mar 9, 2024
View reviewed changes
Copy link
Owner

@TSRBerry TSRBerry left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a few formatting issues

@AnonymusNW AnonymusNW requested a review from TSRBerry March 9, 2024 19:16
TSRBerry
TSRBerry reviewed Mar 11, 2024
View reviewed changes
Copy link
Owner

@TSRBerry TSRBerry left a comment

Choose a reason for hiding this comment

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

Lgtm! Just missing the tests now! I'll add some examples soon.

@TSRBerry
Copy link
Owner

Hope these tests serve as good examples for now: https://github.com/TSRBerry/RyuSOCKS/tree/dev/RyuSocks.Test

@AnonymusNW AnonymusNW force-pushed the username-and-password-authentication branch from 31a9791 to 015cbd8 Compare March 19, 2024 15:23
@AnonymusNW AnonymusNW requested a review from TSRBerry April 2, 2024 11:00
@AnonymusNW AnonymusNW force-pushed the username-and-password-authentication branch 2 times, most recently from bf28716 to 6db5394 Compare April 2, 2024 14:17
@AnonymusNW AnonymusNW force-pushed the username-and-password-authentication branch from fc7e3b3 to c3900d9 Compare April 15, 2024 16:35
AnonymusNW added 27 commits April 15, 2024 18:54
@AnonymusNW
Implement Username and Password check
62996a5
@AnonymusNW
Add UsernameAndPasswordResponse.cs Packet
931f3ee
@AnonymusNW
Apply PR suggestions
83adcbd
@AnonymusNW
Use Autoformat
f36d924
@AnonymusNW
Implement Properties and change constructor
07c5ffe
@AnonymusNW
run dotnet-format
5dec3ba
@AnonymusNW
run dotnet format with the right flags
129e0c4
@AnonymusNW
Implement client authentication for UsernameAndPassword
e83f1a6
@AnonymusNW
update CHANGELOG.md
06fb484
@AnonymusNW
update CHANGELOG.md
3c3a3ab
@AnonymusNW
fix formatting
afc2580
@AnonymusNW
fix formatting
6fb06f2
@AnonymusNW
fix formatting
cc9847f
@AnonymusNW
Add AuthenticationPacket.cs and UaPPacket.cs
57f0f03 
Adds UaPPacket, containing all necessary variables for UaP Authentication
Adds AuthenticationPacket containing version
@AnonymusNW
Implement FromArray() into UaPPacket.cs
c909564 
Added IAuthenticationPacket.cs as parent class for authentication packages
UsernameAndPassword.cs converts incomingPacket to UaPPacket.cs via FromArray
@AnonymusNW
Fix semantic code problems
83a7188 
Removed IAuthenticationPacket.cs
Renamed UaPPacket to UsernameAndPasswordRequest.cs and moved to ./Auth/UsernameAndPassword
Use Encoding.ASCII.GetString() instead of ToString()
Use byte instead of int for Username/Password Length
@AnonymusNW
Implement Username and Password check
a89f58a
@AnonymusNW
Add UsernameAndPasswordResponse.cs Packet
10b5852
@AnonymusNW
update CHANGELOG.md
fefeef8
@AnonymusNW
update CHANGELOG.md
223556b
@AnonymusNW
fix formatting
0c16218
@AnonymusNW
add 2 tests and a test data generator
c5034ec
@AnonymusNW
add random Username and Password pair generator
037ff12
@AnonymusNW
add testing with random Usernames and Passwords
30026ea
@AnonymusNW
run dotnet because someone hasn't automated it yet
71e64c6
@AnonymusNW
Disable warning
6054d98
@AnonymusNW
run dotnet after rebase
8618087
@AnonymusNW AnonymusNW closed this Apr 15, 2024
@AnonymusNW AnonymusNW force-pushed the username-and-password-authentication branch from c3900d9 to 8618087 Compare April 15, 2024 16:55
@TSRBerry TSRBerry mentioned this pull request Apr 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TSRBerry TSRBerry Awaiting requested review from TSRBerry

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AnonymusNW @TSRBerry