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

Use AuthSecret as Bearer token for lookupd queries #313

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

jehiah
Copy link
Member

@jehiah jehiah commented Nov 23, 2020

The AUTH protocol spec and nsqd AUTH details don't give much guidance for how to use AUTH with a lookupd endpoint (except implying that nsqauthfilter is one approach).

Currently the only exposed capability to use a lookup endpoint that requires authorization is to bake authorization credentials into the URL used as a lookup endpoint. go-nsq then makes a GET request to that endpoint during discovery.

Security best practice is to move authorization details to a header and out of the URL i.e. a Authorization: Bearer {Auth Secret} header.

This issue is to introduce new behavior where an AuthSecret when set is used as a Bearer token on lookupd calls. That seems like a sane default

LookupdAuthorization=false can be used to opt out of the new behavior (AuthSecret used on lookupd requests)

cc: @mreiferson @ploxiln in case you have any thoughts on this approach. I think our AUTH docs could use a little help, i'll try to build some better documentation of that.

@jehiah jehiah self-assigned this Nov 23, 2020
@jehiah jehiah requested review from mreiferson and ploxiln November 23, 2020 21:34
Copy link
Member

@ploxiln ploxiln left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

(I did take a moment to remind myself of the elements here: the AuthSecret is currently just used for tcp-protocol to nsqd, and with this change it still is, but with the addition of being sent to nsqlookupd in the auth header, unless explicitly disabled. Should be fully backward compatible, except it can "expose" your AuthSecret to your nsqlookupd by default, which is probably not the end of the world.)

config.go Outdated Show resolved Hide resolved
@mreiferson
Copy link
Member

LGTM otherwise

@jehiah jehiah force-pushed the auth_bearer_token_313 branch from 34d2366 to 6b8799f Compare December 2, 2020 18:45
@jehiah jehiah merged commit b933d01 into nsqio:master Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants