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

Agency: vehicle register: add provider_id field #469

Merged
merged 2 commits into from
Jun 11, 2020
Merged

Agency: vehicle register: add provider_id field #469

merged 2 commits into from
Jun 11, 2020

Conversation

Retzoh
Copy link
Contributor

@Retzoh Retzoh commented Mar 19, 2020

MDS Pull Request

Thank you for your contribution!

For most Pull Requests, the target branch should be dev. Please ensure you are targeting dev to avoid complications and help make the Review process as smooth as possible.

Explain pull request

In several cities, the Agency is not connected directly to mobility providers. There is an aggregator in-between centralising the data from the providers and re-exposing them to the Agency.

When registering a device with the Agency API, there is currently no way for the aggregator to indicate which provider the device belongs to. This PR proposes a way to fix this.

Is this a breaking change

A breaking change would require consumers or implementors of the API to modify their code for it to continue to function (ex: renaming of a required field or the change in data type of an existing field). A non-breaking change would allow existing code to continue to function (ex: addition of an optional field or the creation of a new optional endpoint).

  • No, not breaking: only adding an optional field

Impacted Spec

Which spec(s) will this pull request impact?

  • agency

Additional context

None

@Retzoh Retzoh requested a review from a team March 19, 2020 10:47
@Retzoh Retzoh requested a review from a team as a code owner March 19, 2020 10:47
@sarob sarob added Agency Specific to the Agency API enhancement New feature or request labels Mar 22, 2020
@sarob sarob added this to the Future milestone Mar 22, 2020
@marie-x
Copy link
Collaborator

marie-x commented Apr 2, 2020

This would require some sort of access controls, I would think. How does the instance of Agency know which intermediaries are allowed to represent which provider(s)?

This goes against the basic premise of Agency, which is that it is an interface for Providers, not intermediaries or aggregators. We should probably discuss this in more depth.

@Retzoh
Copy link
Contributor Author

Retzoh commented Apr 7, 2020

Hi @Karcass,

This would require some sort of access controls, I would think. How does the instance of Agency know which intermediaries are allowed to represent which provider(s)?

There is already an access control system in place since all MDS API requests are authenticated, and I hope that Agencies check that when providers push events on the Vehicle-Event endpoint, they check that the device being pushed onto belongs to the authenticated provider. If not we should explicit it in the specs since this would be an obvious security issue.
To handle an aggregator, agencies would just need to extend this check to a list of authorized providers.

Note that Agencies that are not connected to an aggregator would not need to change anything to their implementation. I can changed the PR to make the proposed provider_id field optional, in which case the vehicle is considered to belong to the authenticated provider. This even makes the changes non-breaking, what do you think ?

This goes against the basic premise of Agency, which is that it is an interface for Providers, not intermediaries or aggregators. We should probably discuss this in more depth.

I don't understand what difference it makes, happy to discuss it with you though.

@bhandzo
Copy link
Contributor

bhandzo commented Apr 8, 2020

Since Bird provides a platform for many operators this change would allow us to send a unified stream of events for all of those operators which would simplify things on our end, so we would support this.

@jfh01 jfh01 modified the milestones: Future, 1.0.0 Apr 9, 2020
@schnuerle schnuerle merged commit 6e07686 into openmobilityfoundation:dev Jun 11, 2020
@avatarneil
Copy link
Contributor

avatarneil commented Jun 30, 2020

I believe that without an addendum around authorization, this potentially opens a security hole: there is no mechanism described in the spec to dictate how you should determine if a given provider (an aggregator) is permitted to register vehicles for another provider. Consider this scenario:
Provider A tries to register a vehicle, and includes Provider B's provider_id in the payload. This could be scaled to be a full-on attack on Provider B, where a malicious provider (Provider A) could trigger SLA violations en-masse for the target provider (Provider B).

@Retzoh mentioned that this can be solved by an Agency storing a list of provider_ids for which a given provider (an aggregator) is permitted to register vehicles & push data on behalf of, and I think that's a solid solution; however, there is no mechanism for that described in MDS currently. I think changes in this PR pose a substantial security risk without being accompanied by explicit guidance on how implementers should handle this scenario.

@schnuerle
Copy link
Member

This may need to be removed for 1.0.0 so we can address it later with a larger security solution. Unless someone comes up with a solution that doesn't need a larger discussion. This would need to be addressed by tomorrow for the 1.0.0 release candidate.

@Retzoh
Copy link
Contributor Author

Retzoh commented Jul 1, 2020

@Retzoh mentioned that this can be solved by an Agency storing a list of provider_ids for which a given provider (an aggregator) is permitted to register vehicles & push data on behalf of, and I think that's a solid solution; however, there is no mechanism for that described in MDS currently. I think changes in this PR pose a substantial security risk without being accompanied by explicit guidance on how implementers should handle this scenario.

Hi @avatarneil , thanks for pointing that out. Note that the risk you are pointing out here exists for all the endpoints in both the Agency API and the provider API, not just the Agency register endpoint discussed here: anyone could try to send or expose events or telemetries for any device belonging to any provider, as long as they know the MDS uuid of the device.

Any mds implementation has to make sure that for any MDS request, the client authenticated with the JWT token is entitled to see / update / transmit the data in the payload.

To make this more explicit, we could add some language similar to provider/auth.md such as:

Any implementation of any MDS API SHALL verify that any service requesting those APIs is authorized to read and / or write the data it is trying to.

Or more generally:

MDS data contains business sensitive data regarding mobility provider, and privacy sensitive data regarding citizens. Any implementation of any MDS APIs should be authenticated, and verify that all clients are allowed to read / write the data they are trying to.

What would be the best place to add this language ?

@schnuerle @thekaveman @jfh01 , curious to hear your thoughts on this.

@schnuerle
Copy link
Member

Thanks @Retzoh. I can see adding language like this to the MDS docs.

Does this language solve the problem here? I think there is a difference between knowing the device UUID (which you have to get from authenticated MDS) and the provider UUID (which is public in this repo).

In this case of the provider UUID being used, I don't think language alone is enough to solve it.

@avatarneil
Copy link
Contributor

avatarneil commented Jul 1, 2020

I disagree that this exists for 'all the endpoints in both the Agency API and provider API', I think that the Authorization section of Agency covers the needs for 0.4.1; you are required to have a provider_id claim in the JWT, and you are only able to CRUD data that is associated with your provider_id (example language that also exists in Agency: 'Providers can only retrieve data for vehicles in their registered fleet.'). This notion of an aggregator, which can send + access data for multiple providers is new to the spec. I don't think the language proposed is sufficient, I think what we'd need to do is update the Authorization section to include a way to encode access for an aggregator. I can think of a couple different ways that it could be encoded, however think it's worth discussing in a WG rather than trying to settle on an approach in time for 1.0.0. We need to be very thoughtful about this.

Cc. @Retzoh @schnuerle

@Retzoh Retzoh deleted the patch-3 branch July 1, 2020 16:07
schnuerle added a commit that referenced this pull request Jul 9, 2020
This was discussed on working group calls and decided to be defined as part of a larger change in a forthcoming release due to security/abuse concerns noted in [this conversation](#469 (comment)).
@schnuerle
Copy link
Member

Per our working group discussion today and comments here, this one line has been removed from the 1.0.0 release candidate, so we can have a larger conversation and solution in the future.

See issue #551 for the start of this discussion.

leandroada added a commit to leandroada/data-specification that referenced this pull request Jan 29, 2024
This was discussed on working group calls and decided to be defined as part of a larger change in a forthcoming release due to security/abuse concerns noted in [this conversation](openmobilityfoundation/mobility-data-specification#469 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agency Specific to the Agency API enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants