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

nsqd: support POST auth #1487

Merged
merged 1 commit into from
May 12, 2024
Merged

Conversation

danrjohnson
Copy link
Contributor

This adds support for POST based authentication in addition to GET based authentication.

As described in #1486, the default behavior of nsqd to to propagate upward any errors from net/http clients which leads to Secret's leakage if the authentication server is unavailble.

@danrjohnson
Copy link
Contributor Author

ready for review

@danrjohnson
Copy link
Contributor Author

I am curious for folks feedback on if we should make application/json POST requests or typical POST form behavior. Would love thoughts and happy to make a change here.

Copy link
Member

@mreiferson mreiferson left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this, just a few changes...

internal/auth/authorizations.go Outdated Show resolved Hide resolved
internal/http_api/api_request.go Outdated Show resolved Hide resolved
apps/nsqd/options.go Show resolved Hide resolved
@mreiferson mreiferson changed the title Support post auth nsqd: support post auth Apr 28, 2024
@mreiferson mreiferson changed the title nsqd: support post auth nsqd: support POST auth Apr 28, 2024
@danrjohnson danrjohnson requested a review from mreiferson April 29, 2024 15:30
@danrjohnson
Copy link
Contributor Author

@mreiferson Updated per your feedback! Thanks!

Copy link
Member

@mreiferson mreiferson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mreiferson mreiferson force-pushed the support_post_auth branch from c3556fa to f162880 Compare May 12, 2024 10:25
@mreiferson
Copy link
Member

(I did some minor function renaming and squashed/renamed the commit for merge...)

Adds simple config option and flag to allow for auth to occur via POST
request in addition to GET. Rationale: Errors from net/http requests are
bubbled to nsqd when there is an error during authentication, such as if
the nsq authentication server is unavailable. These errors include the
full path, including any GET parameter, thus causing the authentication
secret to be logged. This does not occur by default for the POST body
thus helping protect secrets in transit between nsqd and the
authentication server.
@mreiferson mreiferson force-pushed the support_post_auth branch from f162880 to 0db445c Compare May 12, 2024 10:37
@mreiferson mreiferson merged commit 4de1606 into nsqio:master May 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants