nsqd authorization #356

jehiah · 2014-05-24T22:14:28Z

This is an attempt at creating an authorization protocol as originaly requested in #224

Specification:

nsqd TCP protocol will add a AUTH command with a json body that MUST be called before SUB or PUB when nsqd has authorization enabled.

Nsqd will make authorization requests against any auth server configured until it gets a valid response. It will pass along the metadata from the client, and the clients remote address. The auth server will reply with a list of topic/channels that the connection is valid for. The topic and channel names returned will be interpreted as a regex which will allow for the auth server to give blanket auth response in the form of topic=.*, channels=[.*] response and will specify a TTL for how long the authorization is good for. The auth request will be cached and re-tried upon expiration of the authorization metadata. (this will ensure that when authorization is removed from a auth server, you can ensure that it no longer applies after a specified ttl).

Authorization information will be stored on the connection for the life of the connection or until the TTL expires, and will be checked at SUB or PUB time.

It is expected that when using an authorization server, the nsqd HTTP endpoints are not exposed to untrusted clients.

A simple auth server will be included that gives a UI to manage authorizations based on login, tls, remote_ip and optionally an oauth2 endpoint used to lookup login values. It is expected that other auth servers will be written for different authorization backends (like LDAP, ssl certificate checking, etc). Connections between nsqd and the authorization server are expected to be over a trusted network.

cc: @mreiferson

jehiah · 2014-05-25T03:35:06Z

mreiferson · 2014-05-25T14:50:21Z

Nice 👍

Focussing on the design rather than the code at this point, my only thought is that we may want to build in a way for the protocol to specify separate PUB/SUB rights. I don't think they should be treated equally (perhaps the default can be to allow both, though).

mreiferson · 2014-05-25T14:51:42Z

... and drop the wrapped HTTP responses, since we're moving away from that in the pending #330

mreiferson · 2014-05-25T14:53:01Z

And, for now, what is the assumption w/r/t access via HTTP?

jehiah · 2014-05-25T21:05:08Z

Added todo list for separate pub/sub permissions in the auth response

I'll adjust http format as the other PR get's merged in. Explicit assumption is that http (pub and stats) is not ever affected from AUTH. I expect i'll write a proxy to give filtering for those at some point in time, but that doesn't have to be in the core

jehiah · 2014-05-27T12:49:51Z

Some additional thoughts:

An AUTH message to a nsqd server that does not have addresses configured for nsqauthd should not affect the connection. This will enable switching to enable auth in large deployments with a slow rollout (new nsqd, send AUTH commands, rollout nsqd config for auth server).

It will also be useful if the auth server echo's back a user-agent of sorts w/ the identity of the authorized connection, and that info is displayed in nsqadmin (possibly a url as well?). This will, for example, enable authorization based on an oauth access token, and allow the auth server to echo back the identity which will then be exposed along w/ other connection stats, and displayed in nsqadmin.

mreiferson · 2014-05-27T14:08:17Z

I'm not sure I completely understand the second paragraph re: user-agenty string? Example maybe?

jehiah · 2014-05-27T14:46:08Z

Example being if I set my auth secret to an oauth access token, the auth server can echo "identity:jehiah" back so in the admin you can see details about the authentication.

mreiferson · 2014-05-27T14:51:43Z

oh, I see, yea that sounds good

jehiah · 2014-06-02T03:24:47Z

@mreiferson a status update. I have this working end to end with pynsq now for both SUB and PUB permissioning. PUB permissions are properly required from nsqd against nsqauthd based on the TTL

mreiferson · 2014-06-02T03:54:42Z

awesome, nice work.

just waiting for you to gimme the RFR 😁

jehiah · 2014-06-02T15:27:32Z

@mreiferson ok I know you are itching to comment on some naming, so i think this is at a spot to checkpoint and get in. I can tackle remaining items in separate PR's

The biggest question for cleanup in this PR is weather or not to add the python nsqauthd and remove later, or just to pull that out into it's own PR which morphs into Go code. I can also tackle exposing auth state in nsqadmin in a separate PR.

mreiferson · 2014-06-02T15:45:49Z

hah 🔥

re: nsqauthd there are lots of options, it could even be a separate repo... I dunno. It seems odd to toss some python in this repo considering the direction we've gone in (and the fact that you want to re-write it in Go anyway). Maybe we move the python nsqauthd to a separate repo (py-nsqauthd) and then leave room for a future Go PR here?

mreiferson · 2014-06-02T17:42:58Z

nsqd/client_v2.go

+	ID            int64
+	context       *context
+	UserAgent     string
+	Authorization *AuthorizationReq


the naming tension here probably means the client should hold just the authorizations and not the request... perhaps Expiry moves over to the Authorization struct

The way this landed, i could promote the Auth Secret, Authorizations Expiration, and Authorizations as top level. Felt more contained to be it's own thing. perhaps AuthState ?

it just feels odd for the variable to be named Authorization but it's actually an AuthorizationReq, so I suggested this might be because of which struct is responsible for certain metadata.

For example, I think it's an Authorization (struct) that technically has a TTL, not the AuthorizationReq. I think the AuthorizationReq is ephemeral, we can create and discard one in local scope whenever we make the request... it's the Authorization that are longer lived and should be hung off ClientV2 (but this implies some metadata gets moved around).

mreiferson · 2014-06-02T17:48:00Z

it occurred to me that we should probably require TLS for the AUTH command

mreiferson · 2014-06-02T17:48:31Z

nsqd/protocol_v2.go

+	}
+
+	if !client.HasAuthorizations() {
+		return nil, util.NewFatalClientErr(nil, "E_UNAUTHORIZED", "AUTH No authorizations found")


do we need to distinguish between E_AUTH_ERROR and E_UNAUTHORIZED (I prefer just the latter...)

jehiah · 2014-06-03T02:58:34Z

@mreiferson I'm trying to surface separate errors because there are separate auth states. The differences between unauthenticated (or unattempted), authenticated, but no permissions and unauthenticated seem important because they have different implications. I'm totally open to better naming, but want to expose as much info as possible so it's easy for people to troubleshoot these configuration/permissioning issues.

These are the diff states that i think are important:

If Authentication was required but you didn't AUTH (no permissions at time of sub/pub) you need to set your auth secret.

If Authentication was attempted but failed, you have a bad or incorrectly configured secret (or you have to use TLS). (Immediate failure on AUTH)

If Authentication succeeds but permission checks fail, you are SUB/PUB'ing for the wrong thing, or need to request permission.

jehiah · 2014-06-03T08:37:43Z

I don't want to require TLS for AUTH as sending AUTH is a way to trigger the strictly IP based authorization (ie: there is no need to encrypt secrets in that case). The TLS state (enabled or not) is passed on to the auth server so it can make the decision weather or not it's required.

mreiferson · 2014-06-03T12:10:39Z

nsqd/protocol_v2.go

+
+	// if nsqauth is enabled, the client must have authorized already
+	// compare topic/channel against cached authorization data
+	if client.context.nsqd.IsAuthenticationEnabled() {


hah, I meant this whole block can be DRYd up, not just the if statement :)

mreiferson · 2014-06-03T12:14:41Z

I guess that makes sense re: TLS

mreiferson · 2014-06-03T12:21:21Z

nsqd/protocol_v2.go

+	}
+
+	// to allow clients to start sending AUTH ahead of it being enabled on the server
+	// we response successfully here saying AUTH DISABLED.


Actually, related to this comment/change here and jehiah/pynsq@a197264, what do you think about clients sending or not sending an AUTH command by checking the version of the nsqd they're connected to (they get this back from the IDENTIFY command).

We could then restore the original response semantics.

True, I didn't think of feature negotiation as an option, but it's probably cleaner that way.

actually, it could (should) be even easier... nsqd should send "auth_required": true as part of the IDENTIFY response when auth is configured.

oops read this now after writing code. I sent it as authentication_enabled but that wording is much better.

mreiferson · 2014-06-03T14:58:09Z

re: naming - I now realize we've been a bit inconsistent with authorization vs authentication - we should pick one or just use auth everywhere to be a bit ambiguous (there's a few places)

mreiferson · 2014-06-03T14:59:08Z

nsqd/protocol_v2.go

+	err = p.Send(client, frameTypeResponse, []byte("AUTH Success"))
+	if err != nil {
+		return nil, util.NewFatalClientErr(err, "E_AUTH_FAILED", "AUTH failed "+err.Error())
+	}


I say drop this whole custom send/failure case and just return okBytes - I don't see a need (anymore) to have a special success response

also, the comment here (above) might have gotten lost in the shuffle but I think it still stands

jehiah · 2014-06-03T15:47:39Z

I'll take a pass at consolidating authorize/authenticate to just auth. I agree that's a good path.

mreiferson · 2014-06-04T02:56:41Z

nsqd/authorizations.go

+	"github.com/bitly/nsq/util"
+)
+
+type Authorization struct {


seems like the last vestigial name... how about Grant?

I paused at also carrying through w/ that name in the json response for the auth protocol. It didn't feel right there.

I'm not sure I understand, are you saying you want to keep this one use of Authorization?

I'm open to suggestions, but absent anything better i think this use is good, I don't like Grant, and PermissionSet was the other idea i had (which i also didn't really like).

jehiah · 2014-06-04T03:36:40Z

@mreiferson here is the output I get from pynsq which i'm happy with for end user context. (some small formatting details aside)

--- Auth Success, SUB fine
[I 140603 23:31:20 client:30] [127.0.0.1:4150:auth_test:asdf] AUTH sent
[I 140603 23:31:20 client:33] [127.0.0.1:4150:auth_test:asdf] AUTH response 'AUTH Success'
--- Auth failed (bad secret, or unable to reach auth server(s))
[I 140603 23:31:43 client:30] [127.0.0.1:4150:auth_test:asdf] AUTH sent
[E 140603 23:31:44 client:38] [127.0.0.1:4150:auth_test:asdf] ERROR: Error('E_AUTH_ERROR AUTH failed',)
--- Auth Success, invalid permissions
[I 140603 23:32:40 client:30] [127.0.0.1:4150:auth_test:asdf] AUTH sent
[I 140603 23:32:40 client:33] [127.0.0.1:4150:auth_test:asdf] AUTH response 'AUTH Success'
[E 140603 23:32:40 client:38] [127.0.0.1:4150:auth_test:asdf] ERROR: Error('E_UNAUTHORIZED AUTH failed for SUB on "auth_test" "asdf"',)

mreiferson · 2014-06-04T13:55:15Z

nsqd/protocol_v2.go

+	err = p.Send(client, frameTypeResponse, []byte(response))
+	if err != nil {
+		return nil, util.NewFatalClientErr(err, "E_AUTH_FAILED", "AUTH failed "+err.Error())
+	}


I'm not sure I understand the motivation for not making it computer parseable... It sets it up to be more flexible, and mirror the response format from other commands (like IDENTIFY).

It's a protocol for computers, right? 😄

The clients can always format it as human readable...

you drive a hard bargain.

are @ploxiln and @jphines getting soft over there?

mreiferson · 2014-06-07T05:31:04Z

🔨 time?

I assume you'll open a separate PR against the docs branch?

mreiferson · 2014-06-07T05:32:17Z

also, we can merge nsqio/go-nsq#35 first and bump the dependency here, if you want, so that the apps get support as of this PR

jehiah · 2014-06-07T05:32:53Z

good idea. let me squash these down and run one last end to end test first

jehiah · 2014-06-07T06:20:38Z

merge me

mreiferson · 2014-06-07T15:13:59Z

nsqd/protocol_v2.go

+		if err != nil {
+			// we don't want to leak errors contacting the auth server to untrusted clients
+			log.Printf("PROTOCOL(V2): [%s] Auth Failed %s", client, err)
+			return util.NewFatalClientErr(nil, "E_AUTH_ERROR", "AUTH failed")


fine tooth comb!

I know we discussed this, but not this specifically, there are only two spots where we use E_AUTH_ERROR and the rest use E_AUTH_FAILED - heck, this even says AUTH failed in the description. Let's just be consistent and use E_AUTH_FAILED, I don't think we need this specific granularity.

part of your comment i agree with; there was an inconsistency between the error key and the txt. I've cleaned that up.

I'd like to keep the two unexpected error cases (json encoding, and unable to send) as error states, and the other (failure to connect to the auth server, or validate a secret against the auth server) as failures.

mreiferson · 2014-06-07T17:17:06Z

alright, alright, alright

LGTM after that last tweak (feel free to squash)

mreiferson · 2014-06-07T17:17:15Z

nice work

* support AUTH command, and configurable auth-endpoints to enable * apply auth state against SUB/PUB/MPUB * echo identity back to client, and in stats

jehiah · 2014-06-08T01:40:56Z

updated and squashed

nsqd authorization

jehiah · 2014-06-08T02:18:11Z

For reference of anyone else following this, an auth daemon implementing this spec can be found at https://github.com/jehiah/nsqauth-contrib as well as a tool that filters nsq stats responses based on auth state.

pwaller · 2014-06-08T09:20:56Z

👍 Have a need for this, great to see it go in, thanks!

jehiah added enhancement labels May 24, 2014

This was referenced May 27, 2014

nsqd Authentication Support nsqio/go-nsq#35

Merged

nsqd Authentication Support nsqio/pynsq#72

Merged

mreiferson reviewed Jun 2, 2014
View reviewed changes

mreiferson reviewed Jun 3, 2014
View reviewed changes

mreiferson reviewed Jun 4, 2014
View reviewed changes

mreiferson reviewed Jun 7, 2014
View reviewed changes

jehiah added 2 commits June 7, 2014 21:29

nsqd auth protocol support

a8776c3

* support AUTH command, and configurable auth-endpoints to enable * apply auth state against SUB/PUB/MPUB * echo identity back to client, and in stats

bump go-nsq to pickup auth support

dc298af

mreiferson added a commit that referenced this pull request Jun 8, 2014

Merge pull request #356 from jehiah/nsqauthd_356

269cdb3

nsqd authorization

mreiferson merged commit 269cdb3 into nsqio:master Jun 8, 2014

jehiah deleted the nsqauthd_356 branch June 27, 2014 17:31

jehiah mentioned this pull request Jul 2, 2014

AUTH Support dlecocq/nsq-py#21

Closed

nsqd authorization #356

nsqd authorization #356

Conversation

jehiah commented May 24, 2014

jehiah commented May 25, 2014

mreiferson commented May 25, 2014

mreiferson commented May 25, 2014

mreiferson commented May 25, 2014

jehiah commented May 25, 2014

jehiah commented May 27, 2014

mreiferson commented May 27, 2014

jehiah commented May 27, 2014

mreiferson commented May 27, 2014

jehiah commented Jun 2, 2014

mreiferson commented Jun 2, 2014

jehiah commented Jun 2, 2014

mreiferson commented Jun 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreiferson commented Jun 2, 2014

Choose a reason for hiding this comment

jehiah commented Jun 3, 2014

jehiah commented Jun 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreiferson commented Jun 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreiferson commented Jun 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jehiah commented Jun 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jehiah commented Jun 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreiferson commented Jun 7, 2014

mreiferson commented Jun 7, 2014

jehiah commented Jun 7, 2014

jehiah commented Jun 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreiferson commented Jun 7, 2014

mreiferson commented Jun 7, 2014

jehiah commented Jun 8, 2014

jehiah commented Jun 8, 2014

pwaller commented Jun 8, 2014