Allow plain connection after ssl deny #526
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
This will come in handy lately.
This will reset a state of the client connection, so it will be ready of new startup packets.
Lagovas
reviewed
Apr 14, 2022
| self.tearDown() | ||
| raise | ||
|
|
||
| def testPlainConnectionAfterDeny(self): |
There was a problem hiding this comment.
I suggest to rewrite this function:
def testPlainConnectionAfterDeny():
# declare fully async function to call run_until_complete once. better to separate async and sync code
async def _testPlainConnectionAfterDeny(self):
conn = await async.connect(...)
conn.fetch(...)
loop = asyncio.new_event_loop() # create new to avoid concurrent usage of the loop in the current thread and allow parallel execution in the future
loop.run_until_complete(_testPlainConnectionAfterDeny())
decryptor/postgresql/pg_decryptor.go
Outdated
| return errors.New("can't stop background goroutine") | ||
| } | ||
|
|
||
| // TODO: why twice? |
There was a problem hiding this comment.
actually I don't remember) maybe to be sure)
or you can remove it and run several tests to check is it break something or not.
There was a problem hiding this comment.
Ohh, I guess this resets the timeout, because it uses time.Time{} instead of time.Now()
decryptor/postgresql/pg_decryptor.go
Outdated
| @@ -408,6 +408,41 @@ func (proxy *PgProxy) sendClientError(msg string, logger *log.Entry) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // stopClientThread sends a signal to a client thread to stop. Returns error in | |||
| // case of an error or timeout. Is used to stop and reload client with TLS | |||
| func (proxy *PgProxy) stopClientThread(logger *log.Entry) error { | |||
There was a problem hiding this comment.
maybe lets name stopClientGoroutine or stopProxyClientConnection as opposite to ProxyClientConnection?
To separate async and sync code.
To be more consistent with `ProxyClientConnection`
Lagovas
approved these changes
Apr 15, 2022
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.
Right now, if some clients receive
SSL denyfrom the server, they can try to start plain connection by sending plainStartup message. The last one would then be served not as a startup, but as a general one, which will hang a connection.This PR fixes this by reloading client thread after
ssl denyto expect startup message again.There are a couple of
TODOs, which I hope you will help me to resolve.Checklist
Public API has proper documentation in the Acra documentation site or has PR on documentation repositorywith new changes
CHANGELOG.md is updated (in case of notable or breaking changes)Benchmark results are attached (if applicable)Example projects and code samples are up-to-date (in case of API changes)