-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
nsqd authorization #356
nsqd authorization #356
Conversation
TODO (at a minimum):
|
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 |
... and drop the wrapped HTTP responses, since we're moving away from that in the pending #330 |
And, for now, what is the assumption w/r/t access via HTTP? |
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 |
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. |
I'm not sure I completely understand the second paragraph re: user-agenty string? Example maybe? |
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. |
oh, I see, yea that sounds good |
@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 |
awesome, nice work. just waiting for you to gimme the RFR 😁 |
@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. |
hah 🔥 re: |
ID int64 | ||
context *context | ||
UserAgent string | ||
Authorization *AuthorizationReq |
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.
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
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.
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
?
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.
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).
it occurred to me that we should probably require TLS for the |
} | ||
|
||
if !client.HasAuthorizations() { | ||
return nil, util.NewFatalClientErr(nil, "E_UNAUTHORIZED", "AUTH No authorizations found") |
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.
do we need to distinguish between E_AUTH_ERROR
and E_UNAUTHORIZED
(I prefer just the latter...)
@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. |
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. |
|
||
// if nsqauth is enabled, the client must have authorized already | ||
// compare topic/channel against cached authorization data | ||
if client.context.nsqd.IsAuthenticationEnabled() { |
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.
hah, I meant this whole block can be DRYd up, not just the if statement :)
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.
DRY'd up
I guess that makes sense re: TLS |
} | ||
|
||
// to allow clients to start sending AUTH ahead of it being enabled on the server | ||
// we response successfully here saying AUTH DISABLED. |
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.
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.
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.
True, I didn't think of feature negotiation as an option, but it's probably cleaner that way.
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.
actually, it could (should) be even easier... nsqd
should send "auth_required": true
as part of the IDENTIFY
response when auth is configured.
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.
oops read this now after writing code. I sent it as authentication_enabled
but that wording is much better.
re: naming - I now realize we've been a bit inconsistent with |
err = p.Send(client, frameTypeResponse, []byte("AUTH Success")) | ||
if err != nil { | ||
return nil, util.NewFatalClientErr(err, "E_AUTH_FAILED", "AUTH failed "+err.Error()) | ||
} |
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 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
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.
also, the comment here (above) might have gotten lost in the shuffle but I think it still stands
I'll take a pass at consolidating authorize/authenticate to just auth. I agree that's a good path. |
"github.com/bitly/nsq/util" | ||
) | ||
|
||
type Authorization struct { |
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.
seems like the last vestigial name... how about Grant
?
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 paused at also carrying through w/ that name in the json response for the auth protocol. It didn't feel right there.
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'm not sure I understand, are you saying you want to keep this one use of Authorization
?
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'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).
@mreiferson here is the output I get from pynsq which i'm happy with for end user context. (some small formatting details aside)
|
err = p.Send(client, frameTypeResponse, []byte(response)) | ||
if err != nil { | ||
return nil, util.NewFatalClientErr(err, "E_AUTH_FAILED", "AUTH failed "+err.Error()) | ||
} |
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'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...
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 drive a hard bargain.
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.
🔨 time? I assume you'll open a separate PR against the docs branch? |
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 |
good idea. let me squash these down and run one last end to end test first |
merge me |
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") |
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.
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.
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.
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.
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
updated and squashed |
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. |
👍 Have a need for this, great to see it go in, thanks! |
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