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.

Sign up for GitHub

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

Jump to bottom

Fix logging public nkey on auth violation #894

Merged
merged 1 commit into from
Feb 2, 2019
Merged

Fix logging public nkey on auth violation #894

merged 1 commit into from
Feb 2, 2019

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Feb 1, 2019 •
edited
Loading

Fixes a few logging entries where the public nkey would be missing in the log:

[92146] 2019/02/01 14:03:34.151755 [ERR] 127.0.0.1:60653 - cid:2 - Subscription Violation - Nkey "UD6AYQSOIN2IN5OGC6VQZCR4H3UFMIOXSW6NNS6N53CLJA4PB56CEJJI", Subject "foo.bar", SID 1
[92146] 2019/02/01 14:03:34.151882 [ERR] 127.0.0.1:60653 - cid:2 - Publish Violation - Nkey "UD6AYQSOIN2IN5OGC6VQZCR4H3UFMIOXSW6NNS6N53CLJA4PB56CEJJI", Subject "foo.bar"
[92146] 2019/02/01 14:13:57.147115 [INF] Removed sub "foo" (sid "1") for Nkey "UD6AYQSOIN2IN5OGC6VQZCR4H3UFMIOXSW6NNS6N53CLJA4PB56CEJJI" - not authorized
  • Documentation added (if applicable)
  • Tests added
  • Branch rebased on top of current master (git pull --rebase origin master)
  • Changes squashed to a single commit (described here)
  • Build is green in Travis CI
  • You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

Fixes #862

/cc @nats-io/core

Signed-off-by: Waldemar Quevedo wally@synadia.com

@coveralls
Copy link

coveralls commented Feb 1, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.04%) to 91.663% when pulling e4a4c98 on wallyqs:nkeys-log-user into a4741c5 on nats-io:master.

derekcollison
derekcollison requested changes Feb 1, 2019
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

Let's make this a small func on client that returns a string and just pass that to Errorf, Noticef etc.

server/client.go Outdated
c.Errorf("Subscription Violation - User %q, Subject %q, SID %s",
c.opts.Username, sub.subject, sub.sid)

var userInfo string
Copy link
Member

Choose a reason for hiding this comment

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

I think better to make this a small function on client.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, just updated to use a small func on the client.

server/client.go Outdated
@@ -2455,12 +2462,26 @@ sendToRoutes:

func (c *client) pubPermissionViolation(subject []byte) {
c.sendErr(fmt.Sprintf("Permissions Violation for Publish to %q", subject))
c.Errorf("Publish Violation - User %q, Subject %q", c.opts.Username, subject)

var userInfo string
Copy link
Member

Choose a reason for hiding this comment

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

Can call same small func here.

derekcollison
derekcollison reviewed Feb 2, 2019
View reviewed changes
server/client.go Outdated
}

func (c *client) replySubjectViolation(reply []byte) {
c.sendErr(fmt.Sprintf("Permissions Violation for Publish with Reply of %q", reply))
c.Errorf("Publish Violation - User %q, Reply %q", c.opts.Username, reply)
c.Errorf("Publish Violation - %s, Reply %q", c.getUserInfo(), reply)
Copy link
Member

Choose a reason for hiding this comment

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

Might still want to %q since it encloses in quotes..

Copy link
Member Author

Choose a reason for hiding this comment

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

If I use %q here then the quotes from the returned string would be escaped, for example:

Publish Violation - "Nkey \"UD6AYQSOIN2IN5OGC6VQZCR4H3UFMIOXSW6NNS6N53CLJA4PB56CEJJI\"", Subject "ngs.echo"```

derekcollison
derekcollison reviewed Feb 2, 2019
View reviewed changes
server/client.go
return fmt.Sprintf("Nkey %q", c.opts.Nkey)
case c.opts.Username != "":
return fmt.Sprintf("User %q", c.opts.Username)
default:
Copy link
Member

Choose a reason for hiding this comment

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

Since we are looking for an auth user here (maybe rename it that), I don't think default should be cid, we get that in client logging. Maybe N/A or unauthorized?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed the function to getAuthUser instead here

Copy link
Member Author

Choose a reason for hiding this comment

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

Also replaced logging cid to instead logging User "N/A"

@wallyqs
Fix logging public nkey on auth violation
e4a4c98 
Signed-off-by: Waldemar Quevedo <wally@synadia.com>
@derekcollison
Copy link
Member

LGTM

@derekcollison derekcollison merged commit ae80c4e into nats-io:master Feb 2, 2019
@wallyqs wallyqs deleted the nkeys-log-user branch February 2, 2019 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekcollison derekcollison derekcollison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@wallyqs @coveralls @derekcollison