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

authz default behavior should not be vendor prerogative #99

Closed
haussli opened this issue Jul 19, 2023 · 7 comments
Closed

authz default behavior should not be vendor prerogative #99

haussli opened this issue Jul 19, 2023 · 7 comments

Comments

@haussli
Copy link
Contributor

haussli commented Jul 19, 2023

From authz/README.md, this is a bad choice, imo:

"A default gRPC-level Authorization Policy for every active gRPC service.

The initial default gRPC-level Authorization Policy can either allow access to all RPCs or deny access to all RPCs except for the gNSI.authz family of RPCs."

The default behavior should be prescribed and it probably should be 'permit' for the element of least surprise. Vendors choosing their default leads to operators needing to remember which vendor does what, like what is the default at the end of route-map vs a policy-statement, and be sure to have policy appropriate for each.

Also, this default policy makes no mention of the disposition of an authorization if a policy is defined and matches neither deny_rules or allow_rules.

And, there is no mention in the prot of the json schema for rules, and the README points to an MR rather than the actual proto.

And, why are rules in json rather than just a proto? Or, why isnt there a method to just use a normal proto w/o json serialization?

@morrowc
Copy link
Contributor

morrowc commented Jul 19, 2023

@tomek-US I think some of this came from authz convesions Tomek worked through.
It seems simplest here to propose the changes that seem sane and we can discuss that with tomek.

wanna take a shot at that and attach it to this issue?

@ssachinbharadwaj
Copy link

The default behavior should be prescribed and it probably should be 'permit' for the element of least surprise. Vendors choosing their default leads to operators needing to remember which vendor does what, like what is the default at the end of route-map vs a policy-statement, and be sure to have policy appropriate for each.

I agree with this.
IMO, the default should be to "ALLOW ALL RPCs" if authz policy is not yet loaded. This will avoid a lot of surprises when the device is upgraded with this feature and suddenly all the RPCs stop working. If we go with "ALLOW ONLY AUTHZ" approach, then the network operator is forced to first push authz policy.
Having backward compatibility for non-authz users is also something to be considered. And as you mentioned, it is good to be called out/prescribed so that all the vendors have consistent default behaviour.

And, there is no mention in the prot of the json schema for rules, and the README points to an MR rather than the actual proto.

And, why are rules in json rather than just a proto? Or, why isnt there a method to just use a normal proto w/o json serialization?

authz.proto helps in pushing the policy from the controller to the network element. But the policy must be consumed by gRPC server ultimately. gRPC stack's built-in authorization module is implemented here and it expects json format.
Unless gRPC stack consumes the authz policy in protobuf format, I don't see a use in changing the format from json string to protobuf in authz.proto
@tomek-US @morrowc please correct me if I am wrong on this.

@haussli
Copy link
Contributor Author

haussli commented Jul 26, 2023

I've tried to address the first two bits and fix the json example in the protobuf comment in #104

As for the use of JSON, it seems inconsistent and unnecessary. If the server wishes to use JSON internally, that is its prerogative, but should not be part of the API, or the user should have the option of using either. Of course, this is just my opinion.

@morrowc
Copy link
Contributor

morrowc commented Aug 1, 2023

ok, pending the conversation being sorted seems good to me.

@haussli
Copy link
Contributor Author

haussli commented Aug 1, 2023

Shall we split the json discussion off to separate issue?

@morrowc
Copy link
Contributor

morrowc commented Aug 1, 2023

seems reasonable to me! :)

@haussli
Copy link
Contributor Author

haussli commented Dec 1, 2023

Created #136 to splitt-off the json discussion.

@haussli haussli closed this as completed Dec 1, 2023
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

No branches or pull requests

3 participants