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

Add Nkey and User JWT Support #260

Merged
merged 11 commits into from
Jun 7, 2019
Merged

Add Nkey and User JWT Support #260

merged 11 commits into from
Jun 7, 2019

Conversation

ColinSullivan1
Copy link
Member

Add NATS 2.0 security features and support the new NATS 2.0 authentication protocol.

Resolves #253.
Resolves #254.

Signed-off-by: Colin Sullivan [email protected]

Signed-off-by: Colin Sullivan <[email protected]>
Signed-off-by: Colin Sullivan <[email protected]>
Signed-off-by: Colin Sullivan <[email protected]>
Signed-off-by: Colin Sullivan <[email protected]>
Signed-off-by: Colin Sullivan <[email protected]>
/// <para>-or-</para>
/// <para>An exception was encountered while connecting to a NATS Server. See <see cref="Exception.InnerException"/> for more
/// details.</para></exception>
public IConnection CreateConnection(string url, string credentials)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would likely be better as credentialsPath.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree...

/// <summary>
/// Gets the JWT file.
/// </summary>
public string JwtFile { get => jwtFile; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or:

Suggested change
public string JwtFile { get => jwtFile; }
public string JwtFile => jwtFile;

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Will fix.

/// </summary>
/// <param name="filePath">Full path to the JWT or cred file.</param>
/// <returns>The encoded JWT</returns>
public string UserFromFile(string filePath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

LoadUserFromFile and just path to be idiomatic with .NET. And should this be static?

/// </summary>
/// <param name="filePath">The credentials file, could be a "*.nk" or "*.creds" file.</param>
/// <returns>A NATS Ed25519 KeyPair</returns>
public static NkeyPair NkeyPairFromSeedFile(string filePath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

LoadNkeyPairFromSeedFile and just path to be idiomatic with .NET.


/*! \mainpage %NATS .NET client.
*
* \section intro_sec Introduction
*
* The %NATS .NET Client is part of %NATS an open-source, cloud-native
* messaging system.
* messaging
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Accidental... thanks for noticing!

}
finally
{
Nkeys.Wipe(text);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think putting these string wipe calls into finally blocks actually makes this situation worse, as the strings would hang around much longer than they would during reading in the loop.

If this is sensitive enough to keep out of memory any longer than necessary, why not require this to return byte[] instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the wipe w/ strings needs to be revisited, but really don't want to hold this up right now. I'll take another pass after the other 2.0 features are implemented.

/// </summary>
/// <param name="credentials">A user JWT, e.g user.jwt</param>
/// <param name="keyfile">Private Key file</param>
public void SetUserCredentials(string credentials, string keyfile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

credentialsPath and privateKeyPath

/// Sets user credentials using the NATS 2.0 security scheme.
/// </summary>
/// <param name="credentials">A chained credentials file, e.g user.cred</param>
public void SetUserCredentials(string credentials)
Copy link
Collaborator

Choose a reason for hiding this comment

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

credentialsPath

public void SetUserCredentials(string credentials, string keyfile)
{
if (string.IsNullOrWhiteSpace(credentials))
throw new ArgumentException("Invalid credentials path", "credentials");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
throw new ArgumentException("Invalid credentials path", "credentials");
throw new ArgumentException("Invalid credentials path", nameof(credentials));

/// <param name="SignatureEventHandler"></param>
public void SetJWTEventHandlers(EventHandler<UserJWTEventArgs> JWTEventHandler, EventHandler<UserSignatureEventArgs> SignatureEventHandler)
{
UserJWTEventHandler = JWTEventHandler ?? throw new ArgumentNullException("JWTEventHandler");
Copy link
Collaborator

Choose a reason for hiding this comment

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

nameof

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.

@ColinSullivan1 ColinSullivan1 changed the title Add Nkey and User JWT support (WIP) Add Nkey and User JWT Support Jun 7, 2019
@ColinSullivan1
Copy link
Member Author

@watfordgnf , Thank you for the review and comments. I do think wipe needs to be improved. I'll review it further over the weekend, but don't want to hold up the 2.0 features to improve it right now. Does that make sense?

Signed-off-by: Colin Sullivan <[email protected]>
@ColinSullivan1 ColinSullivan1 merged commit 171c27d into master Jun 7, 2019
@ColinSullivan1 ColinSullivan1 deleted the nkeys-creds branch June 9, 2019 22:15
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.

NATS 2.0 Connect API/Protocol NKEY Support
2 participants