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.
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.
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
Issue 3138 - Conformance Tests for BackendTLSPolicy - normative #3212
base: main
Are you sure you want to change the base?
Issue 3138 - Conformance Tests for BackendTLSPolicy - normative #3212
Changes from 5 commits
0084b12
0322d1c
ec1ca0e
19ff922
6e03d25
556c048
512866f
cade4a1
0a929ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get how this works. Are we supposed to send an HTTP request and then send a TLS request later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are just serving this as TLS already you can just do
r.TLS.ServerName
I thinkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that you send an HTTP or HTTPS request and the BackendTLSPolicy adds what is needed for the backend request to succeed (certificate and SNI). The echo service finds the SNI and echoes it back to the test to prove the BackendTLSPolicy was used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to dig into this a bit deeper, but I think #3212 (comment) is relevant to this question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic seems to be: on an HTTP request, on a new TCP listener, accept a new connection, get the TLS handshake data, then close the connection.
Should it not be: accept an HTTPS request and report the SNI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two connections - one terminates at the gateway and one authenticates with the backend. I was attempting to get the SNI from the second connection here. Maybe this isn't correct, but just accepting one connection is also not correct. Advice gladly accepted @shaneutt and @howardjohn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the backend though. We don't want to make a connection to ourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we want is
Test binary -- (1) TLS 1-->Gateway --(2) TLS 2 --> test app
Connection 1 is establihsed by the go test. Connection 2 is established by the gateway implementation under test.
The test app does not establish a connection, it accepts one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this TODO we should either:
a) resolve it
b) make sure it has an issue (or is part of some issue) somewhere that we can link here
Ideally, we resolve it but iterative PRs are OK too as long as we have good follow-ups between them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we potentially wanna use http.ListenAndServeTLS here instead of a TCP listener so that we can record (and then assert) the client payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. Right now all we want is the SNI in this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So take this as a question not a statement, but what I was getting at was: wouldn't we want to verify receipt of the payload we sent (which should be unique), in addition to the SNI such that we can be certain the request received was the one intended?