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

[added] enforcement and usage of scoped signing keys #1805

Merged
merged 3 commits into from
Jan 14, 2021

Conversation

matthiashanel
Copy link
Contributor

@matthiashanel matthiashanel commented Jan 14, 2021

Signed-off-by: Matthias Hanel mh@synadia.com

I added a lookup of the scoped signing key to applyAccountLimits.
passing in the values into the function would have made the change a lot bigger.

Currently the update strategy of scoped keys is to disconnect the clients

Signed-off-by: Matthias Hanel <mh@synadia.com>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Maybe a change or two, otherwise, LGTM.

server/accounts.go Outdated Show resolved Hide resolved
server/accounts.go Outdated Show resolved Hide resolved
server/auth.go Outdated Show resolved Hide resolved
server/client.go Outdated Show resolved Hide resolved
server/jwt_test.go Outdated Show resolved Hide resolved
Signed-off-by: Matthias Hanel <mh@synadia.com>
Copy link
Member

@aricart aricart left a comment

Choose a reason for hiding this comment

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

A test in the signing keys is simply counting - when signing key could be added and removed before the server sees it. So the count is the same but contents are different.

server/accounts.go Show resolved Hide resolved
server/auth.go Outdated Show resolved Hide resolved
server/jwt_test.go Show resolved Hide resolved
server/jwt_test.go Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Would remote t.Log()..

server/jwt_test.go Outdated Show resolved Hide resolved
Signed-off-by: Matthias Hanel <mh@synadia.com>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

@kozlovic kozlovic merged commit 3439680 into master Jan 14, 2021
@kozlovic kozlovic deleted the scoped-signing-keys branch January 14, 2021 22:24
Copy link
Member

@aricart aricart left a comment

Choose a reason for hiding this comment

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

LGTM - we are simply going to d/c user's whose scope has changed since they connected. On reconnect, they will have the proper permissions.

continue
}
if _, ok := alteredScope[sk]; ok {
c.closeConnection(AuthenticationViolation)
Copy link
Member

Choose a reason for hiding this comment

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

so here we are simply d/c because their configuration changed, and a reconnect will make them have the correct permissions?

Copy link
Member

Choose a reason for hiding this comment

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

this seems reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants