-
Notifications
You must be signed in to change notification settings - Fork 67
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
fix TLS proxy scenario (#802) #804
Conversation
{ | ||
} | ||
} | ||
internal sealed class SocketClosedException(Exception? innerException) : Exception("Socket has been closed.", innerException); |
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.
please rollback all the formatting changes. makes the review quite difficult. thanks.
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.
hope it is easier now
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.
thank you
#else | ||
await _socket.ConnectAsync(host, port, cts.Token).ConfigureAwait(false); | ||
await _socket.ConnectAsync(proxyOpts.Host, proxyOpts.Port, cts.Token).ConfigureAwait(false); |
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 this is giving the ability to swap out the host:port on top of the callback, can you also use OnConnectingAsync to take care of this swap instead?
public Func<(string Host, int Port), ValueTask<(string Host, int Port)>>? OnConnectingAsync { get; set; } |
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.
Isn't OnConnectingAsync intended for swaping the bus address? Why would it be needed for the proxy address as well?
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.
just trying to understand if we can use OnConnectingAsync + OnSocketThing instead of this change.
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.
No, because OnConnecting would require the socket for the proxy, which is created afterward. Furthermore, you would also have to consider the WebSocket case.
I even considered deleting OnSocketAvailable again. But I wasn't sure if there were other scenarios besides proxy where it would make sense.
{ | ||
// Create the CONNECT request with proxy authentication | ||
var auth = Convert.ToBase64String(Encoding.UTF8.GetBytes(proxyOpts.Auth)); | ||
connectAuth = $"Proxy-Authorization: Basic {auth}\r\n"; |
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.
other auth methods? Would there be other headers we need to think of?
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've thought about that, too. In my experience with our customers so far, I haven't encountered any other options, but there are certainly other proxy authentication options. Do you have a complete list of the methods to be supported here?
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. my concern is how we'd support this going forward. maybe we can have a callback instead?
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.
That would be possible. But I think every methodology has its own fixed format. For additional methods, I would extend properties in NatsProxyOpts. Do you want to leave that entirely up to the user?
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.
Not sure. what's your take? I think if we can support it with reasonable confidence just setting up the proxy host and port would be easy for the application. then the edge cases would require a mew release. (btw we must think of the security implications as well)
// Validate proxy response | ||
var receiveBuffer = new byte[4096]; | ||
var read = await ReceiveAsync(receiveBuffer).ConfigureAwait(false); | ||
var response = Encoding.UTF8.GetString(receiveBuffer, 0, read); |
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 needs to read the response until whatever. can't rely on a single read which might not return the whole response.
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 really understand what you're getting at? Isn't the code almost identical to your final example for OnSocketAvailableAsync from #647?
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.
that was just an example. you need to check the read count and read more if you need more etc. e.g. you might only get part of the message 200 Connec
first time, then the next.
What protocols are you trying to proxy:
And what kind of proxy are you trying to use?
|
} | ||
|
||
var serverWithPort = $"{host}:{port}"; | ||
var connectBuffer = Encoding.UTF8.GetBytes($"CONNECT {serverWithPort} HTTP/1.1\r\nHost: {serverWithPort}\r\n{connectAuth}Proxy-Connection: Keep-Alive\r\n\r\n"); |
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 am confused here, this appears to be an HTTP Proxy? Aren't HTTP Proxies only for proxying HTTP(S) connections? But this is in the TcpConnection
class which is used for the NATS protocol, which is not a HTTP-based protocol.
For WebSockets, there is NatsWebSocketOpts.ConfigureClientWebSocketOptions which could be used to set the ClientWebSocketOptions.Proxy property to a WebProxy
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.
Ah I see that HTTP CONNECT can work with protocols other than HTTP - MDN docs
Aside from enabling secure access to websites behind proxies, a HTTP tunnel provides a way to allow traffic that would otherwise be restricted (SSH or FTP) over the HTTP(S) protocol.
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.
Our current problem to solve for the customer environment is indeed NATS protocol Unencrypted or default Opportunistic TLS with HTTP proxy. The current code solves it
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 concerned we won't be able to support the proxy ourselves giving it the attention it deserves with all the different scenarios. Can you instead implement a IScocketConnection factory like suggested below? we can add that to NatsConnection class switching between the blow two blocks of code:
nats.net/src/NATS.Client.Core/NatsConnection.cs
Lines 338 to 357 in b6edd5f
if (uri.IsWebSocket) | |
{ | |
var conn = new WebSocketConnection(); | |
await conn.ConnectAsync(uri, Opts).ConfigureAwait(false); | |
_socket = conn; | |
} | |
else | |
{ | |
var conn = new TcpConnection(_logger); | |
await conn.ConnectAsync(target.Host, target.Port, Opts).ConfigureAwait(false); | |
_socket = conn; | |
if (Opts.TlsOpts.EffectiveMode(uri) == TlsMode.Implicit) | |
{ | |
// upgrade TcpConnection to SslConnection | |
var sslConnection = conn.UpgradeToSslStreamConnection(Opts.TlsOpts); | |
await sslConnection.AuthenticateAsClientAsync(uri, Opts.ConnectTimeout).ConfigureAwait(false); | |
_socket = sslConnection; | |
} | |
} |
nats.net/src/NATS.Client.Core/NatsConnection.cs
Lines 644 to 666 in b6edd5f
if (url.IsWebSocket) | |
{ | |
_logger.LogDebug(NatsLogEvents.Connection, "Trying to reconnect using WebSocket {Url} [{ReconnectCount}]", url, reconnectCount); | |
var conn = new WebSocketConnection(); | |
await conn.ConnectAsync(url, Opts).ConfigureAwait(false); | |
_socket = conn; | |
} | |
else | |
{ | |
_logger.LogDebug(NatsLogEvents.Connection, "Trying to reconnect using TCP {Url} [{ReconnectCount}]", url, reconnectCount); | |
var conn = new TcpConnection(_logger); | |
await conn.ConnectAsync(url.Host, url.Port, Opts).ConfigureAwait(false); | |
_socket = conn; | |
if (Opts.TlsOpts.EffectiveMode(url) == TlsMode.Implicit) | |
{ | |
// upgrade TcpConnection to SslConnection | |
_logger.LogDebug(NatsLogEvents.Connection, "Trying to reconnect and upgrading to TLS {Url} [{ReconnectCount}]", url, reconnectCount); | |
var sslConnection = conn.UpgradeToSslStreamConnection(Opts.TlsOpts); | |
await sslConnection.AuthenticateAsClientAsync(FixTlsHost(url), Opts.ConnectTimeout).ConfigureAwait(false); | |
_socket = sslConnection; | |
} | |
} |
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 very busy at the moment - but hope to do it by the end of the week.
I think a more flexible API may be to allow for specifying a
And that would be responsible for returning an opened ISocketConnection |
For our scenario i had to add
and
and in NatsOpts instance |
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.
Thanks @wolfkor appreciate the work but I think we can simplify this a little. I want to make sue the changes are minimal to avoid disturbing existing applications in case of unintended behavior changes we might introduce while not opening up internal APIs as much as possible. hope it makes sense.
|
||
namespace NATS.Client.Core; | ||
|
||
public class DefaultSocketConnectionFactory : ISocketConnectionFactory |
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.
Why do we need this all the functionality moved from connection class? Can't we just use the callback Func<NatsUri, NatsOpts, CancellationToken, ValueTask<ISocketConnection>>? SocketConnectionFactory
?
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.
no i don't see that. We need a change inside TcpConnection or WebSocketConnection for sending to the proxy. How to hide and give the customer the chance to change it.
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.
probably the only way i see is a callback which returns the pure Socket given by the proxy and put it in the constructor of TcpConnection. But the current OnSocketAvailable imlementations will break with that
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 mean we don't need this class. see below.
/// <summary> | ||
/// Hook when socket is available. | ||
/// </summary> | ||
Func<ISocketConnection, ValueTask<ISocketConnection>>? OnSocketAvailableAsync { get; set; } |
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.
we can't really change the public API
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.
OnSocketAvailableAsync was implemented for proxy scenario but is simply not working for Proxy+TLS. It must be replaced bei the new functionality
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.
we can leave it but down call it - not a smart way i think
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 prefer not to break existing applications
|
||
namespace NATS.Client.Core; | ||
|
||
public interface ISocketConnectionFactory |
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.
same. shouldn't need this.
@@ -258,6 +259,30 @@ public virtual async ValueTask DisposeAsync() | |||
} | |||
} | |||
|
|||
public NatsUri FixTlsHost(NatsUri uri) |
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 have to open this up I think we should documented as an internal api.
} | ||
|
||
_logger.LogInformation(NatsLogEvents.Connection, "Try to connect NATS {0}", uri); | ||
if (uri.IsWebSocket) |
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.
You can switch here. if we have the callback in the options use that to assign to _socket
otherwise let the rest of the code stay the same.
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.
here and in reconnect loop add these:
_logger.LogInformation(NatsLogEvents.Connection, "Tried to connect NATS {Url} [{ReconnectCount}]", url, reconnectCount);
+ if (Opts.SocketConnectionFactory != null)
+ {
+ _logger.LogDebug(NatsLogEvents.Connection, "Trying to reconnect using SocketFactory {Url} [{ReconnectCount}]", url, reconnectCount);
+ var conn = await Opts.SocketConnectionFactory(url, Opts, _disposedCancellationTokenSource.Token);
+ await conn.ConnectAsync(url, Opts).ConfigureAwait(false);
+ _socket = conn;
+ }
+ else if (url.IsWebSocket)
- if (url.IsWebSocket)
{
these and with the option addition that should be it.
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.
ISocketConnection has no ConnectAsync
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.
that's even better. we can just return a connected socket and avoid the extra connect call.
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.
and the caller has to implement for the return value of SocketConnectionFactory ISocketConnection on his own? That was one point for unsealing TcpConnection
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.
yes, we can start with that. there is very little code in TcpConnection with almost no logic. it's not worth trying to reuse it. creates more unnecessary coupling.
} | ||
} | ||
|
||
if (OnSocketAvailableAsync != null) |
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.
same. can't break api
} | ||
|
||
_logger.LogInformation(NatsLogEvents.Connection, "Tried to connect NATS {Url} [{ReconnectCount}]", url, reconnectCount); | ||
if (url.IsWebSocket) |
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.
same. switch if we have factory defined.
src/NATS.Client.Core/NatsOpts.cs
Outdated
@@ -97,6 +97,8 @@ public sealed record NatsOpts | |||
|
|||
public Encoding SubjectEncoding { get; init; } = Encoding.ASCII; | |||
|
|||
public ISocketConnectionFactory SocketConnectionFactory { get; init; } = DefaultSocketConnectionFactory.Instance; |
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.
Change to Func<NatsUri, NatsOpts, CancellationToken, ValueTask<ISocketConnection>>?
defaulting to null
{ | ||
public SocketClosedException(Exception? innerException) | ||
: base("Socket has been closed.", innerException) | ||
{ | ||
} | ||
} | ||
|
||
internal sealed class TcpConnection : ISocketConnection | ||
public class TcpConnection : ISocketConnection |
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 know if we should open these up. there isn't a lot of code here to be reused. implementations can copy paste the bits they need.
|
||
internal sealed class WebSocketConnection : ISocketConnection | ||
public class WebSocketConnection : ISocketConnection |
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.
same as tcp connection. no a lot of code to reuse. I'd rather not open too much. if we have to we must document as internal API.
…n>>? SocketConnectionFactory (nats-io#804)
thanks for the changes @wolfkor! btw you need to sign your commits. you will have to force push with a new commit. otherwise repo is set to block unsigned commits. all commits in a pr must be signed. |
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.
…n>>? SocketConnectionFactory (nats-io#804)
* Update docs and example links * Hide docs folder [nats:update-docs]
[nats:update-docs]
[nats:update-docs]
* Fix docs workflow * Disable conditional check for docfx workflow trigger Make sure this runs every time so that if it breaks, we know straightaway.
* Fix example code paths and reorg * Revert readme nats by example link
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 think something happened with the diff, it appears a folder was moved and now there are a bunch of changes in here due to that. Can you revert that?
Oh maybe it was caused by merging |
yes looks like we need a rebase @wolfkor. changes for this PR are only in NatsConnection, NatsOpts and NatsUri classes. |
I had to sign all my commits including the main merge by amend command. Give me a hint what to do. Making changes again in new branch? |
yes, opening a new pr and reference this one in comments. that might be easier. |
Fix for #802