Skip to content

Fix the wrong server response match for pg upstream tls#31762

Merged
ravenblackx merged 4 commits intoenvoyproxy:mainfrom
shiponcs:fix-pg-upstream-tls
Jan 16, 2024
Merged

Fix the wrong server response match for pg upstream tls#31762
ravenblackx merged 4 commits intoenvoyproxy:mainfrom
shiponcs:fix-pg-upstream-tls

Conversation

@shiponcs
Copy link
Contributor

In this doc of PostgreSQL, section 55.2.10. outlines what is sent from server upon requesting for SSL-

The server then responds with a single byte containing S or N, indicating that it is willing or unwilling to perform SSL, respectively.

Current postgres filter matches 'E' instead of 'N' what was fixed in this PR.

Commit Message:
Additional Description:
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Fixes commit #23990

Signed-off-by: Abdul Matin <admatinm@gmail.com>
@fabriziomello
Copy link
Contributor

@shiponcs @cpakulski shouldn't this change also need to fix and/or add unit tests?? I mean, seems this part should be revisited: https://github.com/envoyproxy/envoy/blob/main/contrib/postgres_proxy/filters/network/test/postgres_decoder_test.cc#L668 (or am I missing something?)

@shiponcs
Copy link
Contributor Author

@shiponcs
Copy link
Contributor Author

I am waiting to get approval from @cpakulski so as to make change in the test accordingly.

upstreamSSL = true;
} else {
if (c != 'E') {
if (c != 'N') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. I do not remember why we used 'E'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for recording E is for ErrorResponse

Signed-off-by: Abdul Matin <admatinm@gmail.com>
Signed-off-by: Abdul Matin <admatinm@gmail.com>
Signed-off-by: Abdul Matin <admatinm@gmail.com>
Copy link
Contributor

@cpakulski cpakulski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Copy link
Contributor

@fabriziomello fabriziomello left a comment

Choose a reason for hiding this comment

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

LGTM

@fabriziomello
Copy link
Contributor

LGTM. Thanks!

Wow... almost at the same time... hehehehe

@ravenblackx ravenblackx self-assigned this Jan 16, 2024
@ravenblackx ravenblackx merged commit ead437c into envoyproxy:main Jan 16, 2024
@shiponcs shiponcs deleted the fix-pg-upstream-tls branch January 19, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants