[ADD] Support for https redirect URIs (TLS) #41
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ltratt
reviewed
Sep 6, 2024
|
Thanks for this -- this looks really good! |
|
Thanks for review, I will take care later today. |
|
This is excellent, thanks! I need to think a bit about if/where we store certificates, and so on, but that's a luxury add on: this PR does the hard bit. |
|
If you force push a rustfmt, I'll merge this. |
This commit adds support for TLS-encrypted redirect URIs. The commit adds a second server for https: 1. Binding a new TCP socket 2. Uses `rcgen` to generate a new TLS certificate (only in memory) 3. Uses `rustls` to do the TLS handshake and convert the TCPStream into a TLS stream 4. Generalizes the `request` function (and its dependent) to accept an object implementing `Read + Write` instead of just `TCPStream` 5. Redirects all `redirect_uri` that contain `https` to the https server Signed-off-by: Silvano Cortesi <[email protected]>
|
Sure, thanks a lot for the quick process! rustfmt is pushed. |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds support for TLS-encrypted redirect URIs. The commit adds a second server for https:
rcgento generate a new TLS certificate (only in memory)rustlsto do the TLS handshake and convert the TCPStream into a TLS streamrequestfunction (and its dependent) to accept an object implementingRead + Writeinstead of justTCPStreamredirect_urithat containhttpsto the https serverThis last point I think can be discussed: Should all redirect uri's containing
httpsbe redirected to thehttpsserver or only the redirect uri's containinglocalhost? IMO it should not make a difference, as all https URIs will support TLS.Also: the self-signed certificate is not trusted per default (at least on Firefox), so one has to go "Advanced" -> "Accept Risk ..." in Firefox when being redirected. For me this is no issue, if it is one, we could save the TLS certificate to the apropriate location(s) (requiring sudo).
I have not tested this commit across a http URI, just because I have no OAuth provider except of Microsoft (for hotmail). This commit solves #40.