Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFD 151: Database Permission Management #33734
RFD 151: Database Permission Management #33734
Changes from 2 commits
5d3c285
6fc078f
d92f345
bcd5d00
59b3ebf
1a36626
3ba058d
5d15577
360eb39
b34ac60
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the "protocol" needed here? I'm thinking if you need to create an import rule for, say, Postgres databases, you mark them with appropriate label and filter here through "db_labels" above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't absolutely needed, just something which is likely to be helpful. Otherwise you may end up adding labels per protocol type on your own, which is not very user friendly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I still think that having the protocol here is a little confusing and that labels are flexible enough for users to be able to construct rules they want. I would keep it out of initial scope, we can always add it later if needed.
Also I agree that we should support table names, something similar to Okta import rules like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the meaning for
table_names
for other object kinds, e.g. stored procedures? Should we addstored_procedure_names
,view_names
,trigger_names
etc.?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and simplified the permissions matching part; now it is only done via labels, and not object attributes (name, schema, ...). You can still get the same effective behaviour if you add an import rule which propagates the attributes that you need (which is some work), but the positive part is that the implementation gets easier to write and understand; the latter part is a win in my book, as it makes the UX better. The roles are shorter, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is an error in the schema definition, how will the administrator know? For allow functions the failure mode is obvious, but a typo in a deny action could result in an expected control not functioning.
Can we validate this schema at the point of configuration? There is still a concern around database changes happening after configuration, but it would at least prevent typo like failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what can we do except to provide some facilities to debug the algorithm, visualising the intermediates etc. This is not a new problem: the existing deny rules that depend on matching have no guardrails either. There is also no existing functionality to help debug these or warn in case of mismatches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear that this is matching label
product: sales
. It's also on the same level as matchingobject_kind
. What if there is a labelobject_kind
that conflicts?We probably need an extra level:
or something similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In current design attributes and labels are matched together; if the same key is present for both, then the attributes are taking precedence. But I can see this is causing confusion, so its probably best that I change that. This will also remove some ambiguity, while the verbosity increase should be minimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the attributes from the
db_permissions
in roles; now this is purely based on the object labels.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you think about applying change on insert? Otherwise people will be still able to access the DB using the old permissions for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schema change is mentioned as a potential trigger, but this needs to be detected somehow. No general way to do it, so a cron job is a sensible baseline to have. But yeah, this is definitely an extension we want to support at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Postgres schema change, probably can use PostgreSQL trigger+notify:
https://medium.com/launchpad-lab/postgres-triggers-with-listen-notify-565b44ccd782
But agreed there is no general way to do it for all protocols and object kinds.
Assuming the periodic scan happens on Database Service (as opposed to discovery service or another service), how to coordinate the scan in a HA setup (multiple db service serving the same database)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, something like this should work for Postgres assuming permissions align etc. In case of errors periodic scan would provide a viable fallback. The actual notify implementation may be unreliable, too.
As a baseline, a random jitter would be added to the timer, and objects would be discovered multiple times. This should look just like regular discovery, except one running with increased frequency.
This hints at one possible solution to increased backend load: provide a tuneable for discovery frequency, and decrease it automatically as a function of the number of servers performing the discovery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure these details are clearly described in the docs. I believe that our customers will be shouldering a lot with understanding their specific database implementation, the permission model within it, and the schema design and update process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be helpful to include exact statements Teleport will use to assign/revoke these permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These depend on the object kinds, but here is the current procedure, lifted from the linked PR. As noted elsewhere, it handles only tables, but it is easy to see how it can be expanded for further object kinds.