Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Fix http/s proxy authentication with long username/passwords#16504

Merged
clokep merged 6 commits intomatrix-org:developfrom
MagicRB:fix-http-proxy-auth
Oct 24, 2023
Merged

Fix http/s proxy authentication with long username/passwords#16504
clokep merged 6 commits intomatrix-org:developfrom
MagicRB:fix-http-proxy-auth

Conversation

@MagicRB
Copy link
Contributor

@MagicRB MagicRB commented Oct 16, 2023

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@MagicRB MagicRB requested a review from a team as a code owner October 16, 2023 15:32
@MagicRB
Copy link
Contributor Author

MagicRB commented Oct 16, 2023

I'm not sure if the changelog change should be squashed with the actual change or not, please advise.

@clokep
Copy link
Member

clokep commented Oct 16, 2023

I'm not sure if the changelog change should be squashed with the actual change or not, please advise.

It doesn't matter, we squash merge anyway. So don't bother.

clokep
clokep reviewed Oct 16, 2023
View reviewed changes
@clokep clokep requested a review from a team October 16, 2023 15:47
@MagicRB
Copy link
Contributor Author

MagicRB commented Oct 19, 2023

i will apply what the lint said and force push

@MagicRB MagicRB force-pushed the fix-http-proxy-auth branch 2 times, most recently from 446f012 to 4f69a64 Compare October 20, 2023 13:58
@MagicRB
Copy link
Contributor Author

MagicRB commented Oct 20, 2023

Took me a while, sorry about that :) it's faster than trying to get the infra working on NixOS

@DMRobertson
Copy link
Contributor

DMRobertson commented Oct 23, 2023

Introduced in #10475

DMRobertson
DMRobertson reviewed Oct 23, 2023
View reviewed changes
@clokep clokep changed the title Fix http/s proxy authentication Fix http/s proxy authentication with long username/passwords Oct 23, 2023
@DMRobertson
Copy link
Contributor

DMRobertson commented Oct 23, 2023

The following patch adds a test case which reproduces the issue. Could you apply it and include it as part of this PR?

From 5fe76b9434e22bb752c252dd9c66c3c2bfb90dfc Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Mon, 23 Oct 2023 19:21:23 +0100
Subject: [PATCH] Add test case to detect dodgy b64 encoding

---
 tests/http/test_proxyagent.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/http/test_proxyagent.py b/tests/http/test_proxyagent.py
index 8164b0b78e..b48c2c293a 100644
--- a/tests/http/test_proxyagent.py
+++ b/tests/http/test_proxyagent.py
@@ -217,6 +217,20 @@ def test_parse_proxy(
         )
 
 
+class TestBasicProxyCredentials(TestCase):
+    def test_long_user_pass_string_encoded_without_newlines(self) -> None:
+        """Reproduces https://github.com/matrix-org/synapse/pull/16504."""
+        creds = BasicProxyCredentials(
+            b"looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooonguser:pass@proxy.local:9988"
+        )
+        auth_value = creds.as_proxy_authorization_value()
+        self.assertNotIn(b"\n", auth_value)
+        self.assertEqual(
+            creds.as_proxy_authorization_value(),
+            b"Basic: bG9vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vb29vbmd1c2VyOnBhc3M=",
+        )
+
+
 class MatrixFederationAgentTests(TestCase):
     def setUp(self) -> None:
         self.reactor = ThreadedMemoryReactorClock()
-- 
2.41.0

MagicRB and others added 2 commits October 24, 2023 12:45
@MagicRB MagicRB force-pushed the fix-http-proxy-auth branch from 4f69a64 to 47861e9 Compare October 24, 2023 10:46
clokep
clokep reviewed Oct 24, 2023
View reviewed changes
clokep
clokep approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

@DMRobertson
Copy link
Contributor

DMRobertson commented Oct 24, 2023

Hmm. That test is failing and I don't know why.

@DMRobertson
Copy link
Contributor

DMRobertson commented Oct 24, 2023

Oh, it's because I passed the entire auth string into the credentials constructor, rather than just the username:password. I'll fix.

clokep
clokep reviewed Oct 24, 2023
View reviewed changes
DMRobertson pushed a commit that referenced this pull request Oct 24, 2023
Revert "Add test case to detect dodgy b64 encoding"
2f1065f 
This reverts commit 5fe76b9.

I think I had this accidentally commited on my local develop branch, and
so it accidentally got merged into upstream develop.

This should re-land with corrections in #16504.
@clokep clokep enabled auto-merge (squash) October 24, 2023 13:37
@clokep clokep merged commit 95076f7 into matrix-org:develop Oct 24, 2023
@clokep
Copy link
Member

clokep commented Oct 24, 2023

Thank you! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@clokep clokep clokep approved these changes

@DMRobertson DMRobertson DMRobertson left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MagicRB @clokep @DMRobertson