Skip to content
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

Connect Enhancements #833

Merged
merged 9 commits into from
Oct 30, 2023
Merged

Connect Enhancements #833

merged 9 commits into from
Oct 30, 2023

Conversation

scottf
Copy link
Collaborator

@scottf scottf commented Oct 26, 2023

  • Reconnect On Connect
  • Authentication Expired support
  • Ensuring not failing on Authentication Or Authorization Error
  • Support 2.10.3 TLS (Handshake) First

@scottf scottf requested a review from mtmk October 27, 2023 15:36
" NKEY Seed printed below can be used to sign and prove identity.\n" +
" NKEYs are sensitive and should be treated as secrets.\n" +
"NKEY Seed printed below can be used to sign and prove identity.\n" +
"NKEYs are sensitive and should be treated as secrets.\n" +
Copy link
Collaborator Author

@scottf scottf Oct 27, 2023

Choose a reason for hiding this comment

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

Obsessive formatting done here, nothing functional, although I had to change unit tests.

@@ -240,6 +240,46 @@ var expiredUserJwt
Assert.Equal("'Authorization Violation'", ex.Message, StringComparer.OrdinalIgnoreCase);
}
}

[Fact]
public void TestRealUserAuthenticationExpired()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test uses a JWT that will expire quickly. The user connects to the server while the jwt is valid, and then the server disconnects the connection when the jwt expires.

Copy link
Contributor

@mtmk mtmk left a comment

Choose a reason for hiding this comment

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

LGTM

@scottf scottf changed the title Reconnect On Connect and Auth Error Handling Connect Enhancements Oct 27, 2023
if (opts.TlsFirst)
{
processExpectedInfo();
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tls first, don't check/process the server info. checkForSecure used to be the last thing in processExpectedInfo. I moved it out so I could do it on either side. I supposed I could have done the change inside processExpectedInfo but this matches the java code

throw new NATSSecureConnWantedException();
}
else if (info.TlsRequired && !Opts.Secure)
if (!Opts.TlsFirst)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if tlsFirst, it will always be secure

Opts.Secure = true;
// Check to see if we need to engage TLS
// Check for mismatch in setups
if (Opts.Secure && !info.TlsRequired)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opts.Secure now returns true if TlsFirst is set. This explains the 2 tests later on

@@ -1272,10 +1296,9 @@ private void checkForSecure(Srv s)
// processExpectedInfo will look for the expected first INFO message
// sent when a connection is established. The lock should be held entering.
// Caller must lock.
private void processExpectedInfo(Srv s)
private void processExpectedInfo()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't need the Srv parameter since that work is now done outside this function

@scottf scottf merged commit e199ed6 into main Oct 30, 2023
1 check passed
@scottf scottf deleted the reconnect-on-connect-auth-reconnect branch October 30, 2023 14:16
@scottf scottf mentioned this pull request Oct 30, 2023
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.

2 participants