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

Nkey support #399

Merged
merged 3 commits into from
Oct 29, 2018
Merged

Nkey support #399

merged 3 commits into from
Oct 29, 2018

Conversation

derekcollison
Copy link
Member

Base nkey support.

Signed-off-by: Derek Collison [email protected]

Signed-off-by: Derek Collison <[email protected]>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

I think Option setter should be added and not sure of the place of the check of absence of signature callback if NKey is specified. But this could be addressed later if needed.

nats.go Outdated
@@ -94,6 +95,8 @@ var (
ErrInvalidContext = errors.New("nats: invalid context")
ErrNoEchoNotSupported = errors.New("nats: no echo option not supported by this server")
ErrClientIDNotSupported = errors.New("nats: client ID not supported by this server")
ErrNkeyButNoSigCB = errors.New("nats: Nkey defined without a signature handler")
ErrNkeysNoSupported = errors.New("nats: Nkeys not supported by the server.")
Copy link
Member

Choose a reason for hiding this comment

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

No punctuation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

nats.go Outdated
@@ -1208,6 +1227,16 @@ func (nc *Conn) checkForSecure() error {
o.Secure = true
}

if o.Nkey != "" && nc.info.Nonce == "" {
Copy link
Member

Choose a reason for hiding this comment

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

So if user defines NKey, then it can connect only to servers that support it, is that ok?

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 believe so, if a user defines them that is what they expect and if the server does not support it that should error IMO.

nats.go Outdated

// Check if we have an nkey but no signature callback
// defined.
if o.Nkey != "" && o.SignatureCB == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Why put this test in here? Since we have a test above that basically says that a client that has NKey cannot connect to a server that does not support it, why not do this test early in Connect()?

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 can move it..

// SignatureCB designates the function used to sign the nonce
// presented from the server.
SignatureCB SignatureHandler

Copy link
Member

Choose a reason for hiding this comment

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

There is no option function to set those, forcing users to use old methods of connecting from Options. I would recommend adding one function that sets the NKey and SignatureCB at the same time, which could ensure that signature is set or otherwise return error. We would still need to do the check outside, though, in case users connection from Options.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree will add it.

@coveralls
Copy link

coveralls commented Oct 26, 2018

Coverage Status

Coverage decreased (-0.5%) to 93.836% when pulling 4515025 on nkeys into 208e287 on master.

nats.go Show resolved Hide resolved
@sasbury
Copy link

sasbury commented Oct 26, 2018

I am not sure we should call the interface keypair, since that could be confused with the Nkey keypair. For java and TS we called it AuthHandler I think.

Signed-off-by: Derek Collison <[email protected]>
nats.go Outdated Show resolved Hide resolved
nats.go Outdated
@@ -1227,16 +1240,6 @@ func (nc *Conn) checkForSecure() error {
o.Secure = true
}

if o.Nkey != "" && nc.info.Nonce == "" {
Copy link
Member

Choose a reason for hiding this comment

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

That's not really the one I say we should be moving

nats.go Outdated Show resolved Hide resolved
Signed-off-by: Derek Collison <[email protected]>
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.

5 participants