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

[IMPROVED] Secure consumer create #3409

Merged
merged 1 commit into from
Aug 30, 2022
Merged

[IMPROVED] Secure consumer create #3409

merged 1 commit into from
Aug 30, 2022

Conversation

derekcollison
Copy link
Member

New consumer create that allows elevation of the stream and consumer names, and optional filter subject to the request subject.

Similar to changes in direct get allows proper security if needed for filter subject selection.

Signed-off-by: Derek Collison [email protected]

/cc @nats-io/core

server/consumer.go Show resolved Hide resolved
server/consumer.go Show resolved Hide resolved
server/jetstream_api.go Show resolved Hide resolved
Durable string `json:"durable_name,omitempty"`
Name string `json:"name,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding a new property to check, why not simply just say all consumers are durable. And that durables can be reaped by the server based on their inactive_threshold. Server can still autoname "ephemerals" when name is empty.

This would simplify the logic on clients, at the expense that durable_name would always be set, and name in ConsumerInfo would duplicate it.

Copy link
Member

@aricart aricart Aug 29, 2022

Choose a reason for hiding this comment

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

The only other concern then becomes how to reflect the new use of the API. If the consumer specifies a filter in the config, and the server matches the version with the new API, the client could simply use the new API. Otherwise, it can use the old API, allowing security compliance of the $JS.API.CONSUMER.CREATE... API without having to deal with model changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Durables could not be reaped, so I prefer to not change that behavior and leave as is for existing clients.

Going forward all consumers will have a name and optional inactive threshold.
Note also that consumer info already had Name, so it now simply matches that for new style.

@derekcollison
Copy link
Member Author

Let's make a call here.

I decided to use the same subject prefix to ease issues with existing permissions and leafnodes etc. However that does mean we have to make sure that if you want a durable to either add a filter subject or fill in the durable name, since otherwise the server thinks its a legacy ephemeral create.

I had the subject prefix different, and that is still and option for us, but I think I prefer using CONSUMER.CREATE.

@aricart
Copy link
Member

aricart commented Aug 30, 2022

So this means that if you want to create a durable, the client specifies durable_name, otherwise name, or empty name and durable_name for an ephemeral

@derekcollison
Copy link
Member Author

So this means that if you want to create a durable, the client specifies durable_name, otherwise name, or empty name and durable_name for an ephemeral

You can also create a named ephemeral by specifying name and inactive threshold.

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 (a comment in the test). I have verified that with this branch the nats.go and nats.c client libraries test suite pass.

server/jetstream_test.go Outdated Show resolved Hide resolved
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

@ripienaar
Copy link
Contributor

So feature wise we have still Durable and Ephemeral.

Both will survive reboots
Both have a idle timeout
Both have a name
Both supports the user giving a name

Is it then that the only difference is essentially just positioning - a durable without a timeout is ephemeral?

Then I guess implementation wise everything is a durable, the server only knows about durable consumers. If a client doesnt set durable_name or inactive_timeout we supply them in line with ephemeral behaviors of today. The result is a durable with a timeout and a generated name.

You can additionally have R1 durables, memory only durables and so forth. But all are durables.

Keeping the structures as is seem acceptible to me, no client changes needed to create an ephemeral or durable - the API simplification can use the features as it wish any old code continue to work?

So if above is not correct, what is the technical need for adding/deprecating around names, what can not be implemented without deprecating durable name?

These kind of API key changes, not on a major version boundary is really disruptive, so much so that I am thinking of just abandoning the terraform provider if the server simply cant keep stable interfaces since those systems require that.

@kozlovic kozlovic self-requested a review August 30, 2022 15:05
@@ -48,7 +48,9 @@ type ConsumerInfo struct {
}

type ConsumerConfig struct {
// Durable is deprecated. All consumers will have names. picked by clients.
Copy link
Member

Choose a reason for hiding this comment

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

this is still deprecated...

Copy link
Member Author

Choose a reason for hiding this comment

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

We could soften if you like..

server/consumer.go Show resolved Hide resolved
…s, and optional filter subject to the request subject.

Similar to changes in direct get allows proper security if needed for filter subject selection.

Signed-off-by: Derek Collison <[email protected]>
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

@derekcollison derekcollison merged commit a9790aa into main Aug 30, 2022
@derekcollison derekcollison deleted the cc-secure branch August 30, 2022 16:48
Durable string `json:"durable_name,omitempty"`
Name string `json:"name,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why omitempty if All consumers will have names?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's good hygiene for older consumers and clients since it does not introduce this on the wire in those situations.

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.

4 participants