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

Add support for qsub permissions #68

Closed
wants to merge 1 commit into from
Closed

Add support for qsub permissions #68

wants to merge 1 commit into from

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Feb 7, 2020

This adds a different type for the subscribe permissions to be able to define queue permissions while still checking for whitespace for subjects from publish permissions.

nsc add user -a A \
  --name test \
  --allow-pub 'test.>' \
  --allow-sub 'test.> v1' 
  --allow-sub 'test v1' 

Fixes #67

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

Signed-off-by: Waldemar Quevedo <wally@synadia.com>
@wallyqs wallyqs requested a review from aricart February 7, 2020 00:32
@coveralls
Copy link

Pull Request Test Coverage Report for Build 331

  • 14 of 16 (87.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 88.861%

Changes Missing Coverage Covered Lines Changed/Added Lines %
types.go 14 16 87.5%
Totals Coverage Status
Change from base Build 317: -0.02%
Covered Lines: 1077
Relevant Lines: 1212

💛 - Coveralls

@wallyqs
Copy link
Member Author

wallyqs commented Feb 7, 2020

Would probably have to wait for this one to land before merging: #66

}

// Validate the allow, deny elements of a permission
func (p *SubscribePermission) Validate(vr *ValidationResults) {
Copy link
Member

Choose a reason for hiding this comment

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

the validation doesn't really work because if the subscribe permission is >.a queue the invalid subject is not validated. again this is the problem with this strategy, we cannot really validate, because if the user intended the subject a b, it would be interpreted as subject a queue b

Copy link
Member

Choose a reason for hiding this comment

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

The data model for expressing this is not correct - it cannot just add a space when one of the components specifies a space as illegal.

Copy link
Member Author

Choose a reason for hiding this comment

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

>.a is an invalid subject so we could add the IsValidLiteralSubject logic from the server here though would help the most in nsc to catch these earlier rather than during runtime which would only result in an authentication error rather than invalid creds for example. I think the model is fine I'd say that also comparatively easier to maintain than making the involved types more complex by moving from subject string to object type with subject + queue.

@aricart
Copy link
Member

aricart commented Feb 7, 2020

More over, not sure any of it matters, because the spec: https://docs.nats.io/nats-protocol/nats-protocol#protocol-conventions, says that some things are illegal, yet they don't seem to be.

% nats-pub .foo bar
Published [.foo] : 'bar'
% nats-pub "a>" bar
Published [a>] : 'bar'
% nats-pub "a.>" bar
Published [a.>] : 'bar'
% nats-pub "a b" hello   <---- only one to not come out right
Published [a b] : 'hello'
% nats-pub "..." hello
Published [...] : 'hello'
% nats-pub ">.a" hello
% nats-sub ">"
Listening on [>]
[#1] Received on [.foo]: 'bar'
[#2] Received on [a>]: 'bar'
[#3] Received on [a.>]: 'bar'
[#4] Received on [a]: 'hello' <---- this is the only one that didn't come through
[#5] Received on [...]: 'hello'
[#6] Received on [>.a]: 'hello'

I suspect setting a client on pedantic, would reject some of them.

@aricart
Copy link
Member

aricart commented Feb 7, 2020

@derekcollison @kozlovic
I am thinking that we shouldn't be performing any subject validation on JWT, as the server is pretty much happy to take anything. If we want to clarify what a valid subject is, we can then provide some guidance.

@derekcollison
Copy link
Member

The server will take anything but only when not in pedantic mode, which most clients set for performance reasons. But if you telnet into say the demo server it does the right thing since verbose and pedantic are true by default. I think nsc or JWT should check IMO.

@aricart
Copy link
Member

aricart commented Feb 11, 2020

@derekcollison @wallyqs Then if we have to check, given the current PR we cannot validate it, because effectively the separator used is an illegal character on the field.

@wallyqs
Copy link
Member Author

wallyqs commented Feb 11, 2020

@aricart we'll add the checks to nsc so that they get caught earlier, we can validate for other invalid characters there too but still allow space.

@aricart
Copy link
Member

aricart commented Feb 12, 2020

@aricart we'll add the checks to nsc so that they get caught earlier, we can validate for other invalid characters there too but still allow space.

That is the point, the data structure won't support this validation, and even if you did the check at entering, the data at rest would fail validation because it would be a field with a space on it, which a subject explicitly rejects.

@aricart
Copy link
Member

aricart commented Feb 12, 2020

@wallyqs I have set up a meeting so we can chat about this and get it resolved.

@wallyqs
Copy link
Member Author

wallyqs commented Feb 12, 2020

Generated data via nsc can potentially be invalid already since nsc didn't have the checks, it is ok for jwt lib to have them as well though for this case we also allow the space in the subject to be able to express the queue permission in a simple way.

@derekcollison
Copy link
Member

Short version is I am ok with spc for input in server config and nsc tooling, but can't be relied on for internal data structures (which it isn't IIRC) or jwt.

@wallyqs
Copy link
Member Author

wallyqs commented Feb 12, 2020

Closed in favor of #70

@wallyqs wallyqs closed this Feb 12, 2020
@aricart aricart deleted the qsubs branch February 21, 2020 13:15
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.

allow deny perms + queue permissions separated by a space
4 participants