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

[databroker] Fix permissions bug. #655

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

argerus
Copy link
Contributor

@argerus argerus commented Sep 13, 2023

Fix permissions bug inadvertently introduced when adding iter_entries().

Description

Access control in databroker is implemented by requiring a DatabaseReadAccess in order to read anything from the db. It's only possible to get a DatabaseReadAccess if you supply a set of permissions, that will then be kept as a reference in this struct.

pub struct DatabaseReadAccess<'a, 'b> {
    db: &'a Database,
    permissions: &'b Permissions,
}

Once you have that, you have access to the database only available through the following interface.

impl DatabaseReadAccess {
    pub fn get_entry_by_id(&self, id: i32) -> Result<&Entry, ReadError>;
    pub fn get_entry_by_path(&self, path: impl AsRef<str>) -> Result<&Entry, ReadError>;
    pub fn get_entries_by_regex(&self, regex: regex::Regex) -> Result<Vec<Entry>, ReadError>;
    pub fn get_metadata_by_id(&self, id: i32) -> Option<&Metadata>;
    pub fn get_metadata_by_path(&self, path: &str) -> Option<&Metadata>;

    // Newly added
    pub fn iter_entries(&self) -> EntryIterator;
    pub fn get_entries_by_regex(&self, regex: regex::Regex) -> Result<Vec<Entry>, ReadError>;
}

Obviously, this only works if these functions (which internally have full access to the db) enforce the permissions.

This was not the case for the two newly added functions.
This PR fixes iter_entries() in this regard, and get_entries_by_regex() will be removed (in #654) as it is just a specialized case of iter_entries().

Implications

The get function of the kuksa.val.v1 API is not enforcing permissions correctly when a wildcard pattern (which includes branches without any actual wildcard) is used.

Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

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

LGTM, just one genral thought/comment


pub fn metadata(&self) -> &Metadata {
&self.entry.metadata
}
Copy link
Contributor

Choose a reason for hiding this comment

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

General I saw it in DatabrokerReadAccess neither, but don't we state in https://github.com/eclipse/kuksa.val/blob/master/doc/KUKSA.val_data_broker/authorization.md that

read:Vehicle.Speed | Allow client to read Vehicle.Speed (including metadata)
-- | --

metadata needs read access too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true.
I have a feeling metadata should be readable by all. But I could be wrong.
It's not enforced in the other accessors either, so if we decide that you need read access in order to read metadata, those would need to change as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I see that it's more useful if everyone can access it. Just had a look at documentation and I think we should clarify it there at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the long run I think we might get requests to restrict the possibility to read metadata. Not useful if the server/broker only has a standard VSS file as config, but if it contains a lot of private (OEM-specific) signals then just showing their names (and descriptions) could reveal sensitive information on how certain problems have been solved in that vehicle, One can compare with DBC or ARXML files which typically are considered confidential by OEMs.

But for now I think it would be sufficient if the limitation is mentioned in documentation. Maybe we should also have a concept on how the token format needs to be changed/extended to allow managing metadata access.

Copy link
Contributor Author

@argerus argerus Sep 14, 2023

Choose a reason for hiding this comment

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

I'm actually starting to think it might be better to change databroker according to what the documentation says (as pointed out by @lukasmittag).

You get access to the metadata (implicitly) as soon as you have some form of access to that entry. Otherwise you don't.

And an extension of that could be that based on what form of access you have (read, actuate) vs provide, the metadata you would have access to would be different?

Fix permissions bug inadvertently introduced when adding
`iter_entries()`.
@argerus argerus merged commit ab6ac64 into eclipse:master Sep 14, 2023
10 checks passed
@erikbosch erikbosch deleted the feature/fix_permissions branch October 31, 2024 13:13
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