-
Notifications
You must be signed in to change notification settings - Fork 182
V2.2.0 #188
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
Conversation
Added NKey class Requires 2 dependencies
Cleaned up dependency on base32 encoder by adding one inline. Updated readme and changelog to 2.2.
Fixed a bunch of mismatches with go NKey implementation Cleaned up server info parser a bit Added options for authhandler to organize nkey authentication callbacks
Made nkey auth test optional based on version 2 of gnatsd.
1 similar comment
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.
Small comments, otherwise LGTM
- gnatsd_version=v1.3.0 | ||
- gnatsd_path=$TRAVIS_BUILD_DIR/gnatsd/gnatsd | ||
- GPG_KEYRING_FILE=.travis/nats.travis.gpg | ||
- secure: yvOfk7kJzzTQ38n444jTDets24FZmxewwb3lrhXwpHTwOnQyq/B8QaHeqvhneECMc0Bq5M4blTlJ/wOWJAvs61POv2QVkyw+u8cVNROzkb8GPaH4ybPo8HMl33EHFNqh1KRo2C9hAPMYbbTjKCVY2UdkdfJ2l4lN/Awk7uEDX8ckc/sENhDeQjY/xoGZUP28O568Eg4ZxN3fr3WEV/0T+R15YyL2X0ev8MiGJM5TojXnNFKdb5fkUodRWwiY8JDn5xzP7xUzzen7MqE/5YNTcIC6haU8LToJM2gXEQtdoWLZqMPWr7k4A+eTBO5vl9qWrPBaOodFJYKzEjrEDfHj5RR9uaufEsnwQzXKw1ODrIFVZiC2n73j/tatWDI+vjnJ5tO+VMwWj53qdBYrvYeyewIT3cz9rrDHH8fGINsKAsk6HgWM3SMgeNSuXjRN0ePxEph5FVQ3ZUjF1ZXp90O7kjD5kXg/jVs6GrhCviRT3fx6Z4hyat9ytshy66jqcttHEfJ5sSOBg8fVbWJjLbxmghWUFp1fuc0HGNiMJStEyOBai5AkG6uJccTlgjlNL/8mgEF+fxo8HGVyStQzRnr7LJuCmWW9hx/aBVmqXR4p6cRgsSO09PvHRmcsLQoktCxVxsvcfblQqMbiQKjsJ4tXLe0U88DMOHnEGOgtik/tt+4= |
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 have many of those here, and I wonder if this does not contribute to the many coveralls email that we receive when you submit a PR...
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 have GPG key? Are we building gnatsd everytime? Can't we just download a release build?
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.
maven
byte[] bytes; | ||
} | ||
|
||
/** |
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.
General note: I wonder if you should not have limited to what's really needed to sign the nonce. This is a complete API that allows to create users, accounts, etc.. which I am not sure was needed 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.
+1, can always add it in if needed.
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.
@sasbury I would try to remove what can be removed or at least make the extra ones private. Once it is public, it more difficult to remove.
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.
How do we define "extra"? Java shops may want to manage their NKeys with Java code, so I was thinking we give them that option.
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.
By extra I meant anything that is not needed to sign the nonce. But I get your point about people not wanted to use something else to manage nkeys.
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 the library should be complete, even on tests, you want to generate stuff. Sure there's a bunch of 'extra' from the perspective of the signing. In any case, the java library has the same functionality as the go library. So would this mean that the go library also needs to be paired down?
Another way to dice it is to have a nkey-signing library, that provides an API that is reduced, while still keeping all the functionality on the library as is.
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 see it that way. All the NKey features that we have in Go will be for tooling, and the good thing about being it in Go is that we could make simple executable for all platforms. Other lang that needs NKey support right now at least just need to sign.
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 see it as we have an nkey library that is used by a client but really could be used for other things 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.
I'd prefer to add that if we get lots of people asking for it..
|
||
appendOption(connectString, Options.OPTION_NKEY, nkey, true, true); | ||
appendOption(connectString, Options.OPTION_SIG, encodedSig, true, true); | ||
} else if (includeAuth) { |
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.
In Go client, when processing the info, if we get a nonce (that is, from a server that supports it), and the NKey option is not set (in your case, a auth handler) then we fail:
func (nc *Conn) processExpectedInfo() error {
(...)
if nc.Opts.Nkey != "" && nc.info.Nonce == "" {
return ErrNkeysNoSupported
}
(...)
I am not seeing that in this file. Did I miss it?
Updated some copyrights. Added a test for a bad auth handler. Updated failure path for auth handler to be more resilient.
Nkey support.
Updated travis to 1.3 gnatsd - need to update again when we get nonce-auth released.
Made the build a bit more friendly to 'build from source' folks