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

feat: add security at the operation level #584

Merged
merged 34 commits into from
Apr 14, 2022

Conversation

sekharbans-ebay
Copy link

@sekharbans-ebay sekharbans-ebay commented Jul 15, 2021


feat: Add security at the channel/operation level

Description:
We are using AsyncAPI specifications for publishing the subscriber specific details of our topics and associated payloads for our HTTP push use cases.

Our subscribable channels(or topics) are protected by a set of oauth scopes (authorization code or client credentials) - that a potential subscriber needs to be aware of.

Here is a sample contract.

We are able to accommodate almost every detail of interest to an integrator/subscriber (THANK YOU) - with the exception of being able to call out the oauth scopes necessary for the subscription to the topic - i.e. the security aspect at the channel or rather channel/operation level.

The current specification is perhaps more aligned with a broker based topology - but ours is a pure push use case (to registered webhooks) and we need an ability to call out any security aspects at the channel operation level.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@magicmatatjahu
Copy link
Member

@sekharbans-ebay Hello! Please update the title with following Semantic release to something like:

feat: Add security at the channel/operation level

Also I think that this PR is related to the #418 issue.

@sekharbans-ebay
Copy link
Author

Hi @magicmatatjahu

I did change the title as asked - what would be the next steps please? Not sure if I am missing anything else.

Thanks
Sekhar

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jul 26, 2021

@sekharbans-ebay I meant to change the issue title 😅

what would be the next steps please

Discussion with core team and community, the best place will be here. You should also inform about your PR and point to the mentioned issue in our slack. By this more people will be interested/involved in this topic.

I am not the person who can single-handedly approve your changes, so you must wait for the other people :)

@magicmatatjahu magicmatatjahu changed the title Update asyncapi.md feat: Add security at the channel/operation level Jul 26, 2021
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@sekharbans-ebay could you provide some fixes here in the PR?

  • look at a failing check, there is an error with PR title
  • update PR title that should talk only about operation, not channel
  • please extend operation object examples with the new proposed security key
  • clarify how should it work if someone provides security on a server object too, what has priority
  • I think the operation traits object also needs to be extended

@sekharbans-ebay sekharbans-ebay changed the title feat: Add security at the channel/operation level feat: Add security at the operation level Jul 30, 2021
@sekharbans-ebay
Copy link
Author

@sekharbans-ebay could you provide some fixes here in the PR?

  • look at a failing check, there is an error with PR title
  • update PR title that should talk only about operation, not channel
  • please extend operation object examples with the new proposed security key
  • clarify how should it work if someone provides security on a server object too, what has priority
  • I think the operation traits object also needs to be extended

Apologies for the delay in responding - all of the review comments have been incorporated except for the 'operation object example' part. I have followed the approach that 'Server' has - where the examples on Security are referenced and provided in great details in an independent section on Security. The Security aspects are complex and require an independent treatment.

Trust that is acceptable.

@sekharbans-ebay sekharbans-ebay changed the title feat: Add security at the operation level feat: add security at the operation level Jul 30, 2021
@sekharbans-ebay sekharbans-ebay requested a review from derberg July 30, 2021 17:28
@derberg
Copy link
Member

derberg commented Aug 2, 2021

@sekharbans-ebay I like that you choose the path of being consistent with the spec. In this case though the problem is that we are actually missing an example of security on the server, not a patter that we should follow. To actually find a good example of usage of security you have to go through examples and find stuff like https://github.com/asyncapi/spec/blob/master/examples/streetlights-kafka.yml#L22

spec/asyncapi.md Outdated Show resolved Hide resolved
@sekharbans-ebay
Copy link
Author

sekharbans-ebay commented Aug 2, 2021

@sekharbans-ebay I like that you choose the path of being consistent with the spec. In this case though the problem is that we are actually missing an example of security on the server, not a patter that we should follow. To actually find a good example of usage of security you have to go through examples and find stuff like https://github.com/asyncapi/spec/blob/master/examples/streetlights-kafka.yml#L22

Question on security examples - Since there are different security mechanisms involved, the Security Requirements in the examples can either be repeated for each of oauth, password or api_key else it seems incomplete. As a reader of the specs , I like what you have for server - i.e. pointer to the relevant security part of the specs. If it is acceptable I can add - say only oauth as an example(both json/yaml). For example:

operationId: registerUser
summary: Action to sign a user up.
description: A longer description
security:
  - petstore_auth:
        - write:pets
        - read:pets
tags:
  - name: user
  - name: signup
  - name: register 
-----
----

@derberg
Copy link
Member

derberg commented Aug 3, 2021

@sekharbans-ebay it is ok to add example for just one security req, oauth is the best example I think

@sekharbans-ebay
Copy link
Author

@sekharbans-ebay it is ok to add example for just one security req, oauth is the best example I think

Thank you. Done.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 3, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sekharbans-ebay sekharbans-ebay requested a review from derberg August 3, 2021 18:01
@derberg
Copy link
Member

derberg commented Aug 9, 2021

@fmvilas have a look.
@sekharbans-ebay went through all comments and improved the proposal and therefore for me it is Stage 1: Proposal because I'm assuming @sekharbans-ebay is a champion for this proposal because he already went with PR creation.

I think we can label it properly and start pinging community for review

@fmvilas fmvilas added the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label Aug 9, 2021
@fmvilas
Copy link
Member

fmvilas commented Aug 9, 2021

Yes, let's start pinging the community for review. I like this idea a lot, @sekharbans-ebay 🙌

@derberg
Copy link
Member

derberg commented Aug 11, 2021

Proposal shared with the wider community:

spec/asyncapi.md Outdated
@@ -663,7 +663,8 @@ Field Name | Type | Description
---|:---:|---
<a name="operationObjectOperationId"></a>operationId | `string` | Unique string used to identify the operation. The id MUST be unique among all operations described in the API. The operationId value is **case-sensitive**. Tools and libraries MAY use the operationId to uniquely identify an operation, therefore, it is RECOMMENDED to follow common programming naming conventions.
<a name="operationObjectSummary"></a>summary | `string` | A short summary of what the operation is about.
<a name="operationObjectDescription"></a>description | `string` | A verbose explanation of the operation. [CommonMark syntax](https://spec.commonmark.org/) can be used for rich text representation.
<a name="operationObjectDescription"></a>description | `string` | A verbose explanation of the operation. [CommonMark syntax](http://spec.commonmark.org/) can be used for rich text representation.
<a name="operationObjectSecurity"></a>security | [[Security Requirement Object](#securityRequirementObject)]| A declaration of which security mechanisms are associated with this operation. Only one of the security requirement objects need to be satisfied to authorize an operation. In cases where Server Security also applies, it is expected the security mechanisms specified at the operation level would take priority and ideally should match or be a subset of that required by the server.
Copy link
Member

Choose a reason for hiding this comment

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

the security mechanisms specified at the operation level would take priority and ideally should match or be a subset of that required by the server

because this is a spec and we need to be super precise, I have to say ideally is not the best word to use here. I think this must be a requirement, that security mechanism on operation level must match one of the security mechanisms on the server level. Then we can have a proper validation in place. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

Suggest rewording to simply: " the security mechanisms specified at the operation level would take priority." and omitting the recommendation on the subset altogether. If the invoker does not have access to the operation any component that the operation relies on is automatically shielded.

In some topologies - the operations can use a completely different set of authentication scopes to talk to the broker. For example operation A is exposed with an auth scope a. However the operation could interact with the broker using a different auth scope (defined at the server level) - lets say from an internal set of allowed scopes say 'b' that is granted to the operational component(s) authorized to interact with the broker).

Copy link
Member

Choose a reason for hiding this comment

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

so we are basically dropping and ideally should match or be a subset of that required by the server? which is much clearer and better for me. Sounds good

Felix200521
Felix200521 previously approved these changes Dec 11, 2021
@derberg
Copy link
Member

derberg commented Dec 22, 2021

@fmvilas I'm in a position that we should go forward with this PR to Draft stage 2. We gave the community 6months to react and comment. We communicated it a few times on different AsyncAPI channels. I also offline pinged PayPal folks a few times to have a look (I knew that in the past they were asking in slack for something like this). This is coming directly from End User and I like it so much that I'm even happy to help @sekharbans-ebay to provide the initial implementation in AsyncAPI JS Parser + JSON Schema 😄 What is your opinion?

@sekharbans-ebay please enrich this PR with some official example that could land in the examples directory. Maybe an official ebay asyncapi file that already incorporated this new feature? Thoughts?

@Felix200521 I see you approved this PR. Can you share a comment on why you like this proposal, what is your use case? It will help us to get a better understanding of why this proposal makes sense for a wider audience.

@sekharbans-ebay
Copy link
Author

> @sekharbans-ebay please enrich this PR with some official example that could land in the examples directory. Maybe an official ebay asyncapi file that already incorporated this new feature? Thoughts?

Hi @derberg - I have made the alterations to reword the section. The asyncapi.md does include examples in this PR (relevant section(s) of the specification).
Will share the official asyncapi contract that has this feature incorporated - once (and if) this feature is approved. Trust the examples can wait a bit? Thanks a lot and wish you all a very happy and safe new year!

Copy link
Member

@quetzalliwrites quetzalliwrites left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@derberg derberg added 📄 Draft (RFC 2) RFC Stage 2 (See CONTRIBUTING.md) and removed 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) labels Apr 13, 2022
@smoya
Copy link
Member

smoya commented Apr 14, 2022

The rest of the PR's have been merged now:

Moving with the merge of this one as approvals from all Code Owners are present.

@smoya
Copy link
Member

smoya commented Apr 14, 2022

/rtm

@asyncapi-bot
Copy link
Contributor

Hello, @smoya! 👋🏼
This PR is not up to date with the base branch and can't be merged.
Please update your branch manually with the latest version of the base branch.
PRO-TIP: Add a comment to your PR with the text: /au or /autoupdate and our bot will take care of updating the branch in the future. The only requirement for this to work is to enable Allow edits from maintainers option in your PR.
Thanks 😄

@smoya
Copy link
Member

smoya commented Apr 14, 2022

/au

@smoya
Copy link
Member

smoya commented Apr 14, 2022

/rtm

@asyncapi-bot
Copy link
Contributor

Hello, @smoya! 👋🏼
This PR is not up to date with the base branch and can't be merged.
Please update your branch manually with the latest version of the base branch.
PRO-TIP: Add a comment to your PR with the text: /au or /autoupdate and our bot will take care of updating the branch in the future. The only requirement for this to work is to enable Allow edits from maintainers option in your PR.
Thanks 😄

@smoya
Copy link
Member

smoya commented Apr 14, 2022

I don't really understand the flow of the bots :(

@derberg
Copy link
Member

derberg commented Apr 14, 2022

the bot that talks to you now is an old bug in a workflow, that was fixed on master only. @KhudaDad414 already fixed it. So do not worry about it. We just need to make sure to update release branch with latest master

do-not-merge label can be taken off only by maintainers, so there is no way that someone else merge release PRs, as maintainers need to make sure related PRs in other repos were merged properly.

I see you merged both related PRs but problem is that the parser PR do not use @asyncapi/specs release candidate 2.14.0-2022-04-release.1. So it basically does not have the latest schema. In other words, someone using parser-js release candidate will not have validation included

@smoya
Copy link
Member

smoya commented Apr 14, 2022

Thanks for the answer @derberg!

the bot that talks to you now is an old bug in a workflow, that was fixed on master only. @KhudaDad414 already fixed it. So do not worry about it. We just need to make sure to update release branch with latest master

This wasn't working. I think since the latest changes on the bot, now autoupdate label is the only way to keep up with upstream.

I opened the following PR changing RELEASE_PROCESS.md step. Does it make sense? #756

do-not-merge label can be taken off only by maintainers, so there is no way that someone else merge release PRs, as maintainers need to make sure related PRs in other repos were merged properly.

I see you merged both related PRs but problem is that the parser PR do not use @asyncapi/specs release candidate 2.14.0-2022-04-release.1. So it basically does not have the latest schema. In other words, someone using parser-js release candidate will not have validation included

I created a PR in the parser for pointing specs package to the latest release candidate. However, wondering if I could use some wildcard to not having to do this change every time we wanna merge a change.

asyncapi/parser-js#521

@smoya
Copy link
Member

smoya commented Apr 14, 2022

@derberg @fmvilas @dalelane if you agree, please remove the do-not-merge label and proceed merging this PR. ( I can't do it, do-not-merge label is there)

@asyncapi-bot asyncapi-bot merged commit 4833e0d into asyncapi:2022-04-release Apr 14, 2022
@derberg derberg added 🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md) and removed 📄 Draft (RFC 2) RFC Stage 2 (See CONTRIBUTING.md) labels Apr 14, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.4.0-2022-04-release.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

security:
# This operation level security implies the ability to send a message to
# `smartylighting.streetlights.1.0.action.{streetlightId}.turn.on` with Authorization headers
# with `streetlights:read` scope. It is also possible for the same channel when using `test_auth`
Copy link
Member

@smoya smoya Apr 21, 2022

Choose a reason for hiding this comment

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

@sekharbans-ebay I just noticed there is a typo here. It should be: test_oauth

# This operation level security implies the ability to send a message to
# `smartylighting.streetlights.1.0.action.{streetlightId}.turn.on` with Authorization headers
# with `streetlights:read` scope. It is also possible for the same channel when using `test_auth`
# server to instead use credentials for security (scramSha256 in this example) specified on the server level
Copy link
Member

Choose a reason for hiding this comment

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

Now that I'm writing the release notes, this comment seems confusing to me.

The spec says:

In cases where Server Security also applies, it MUST also be satisfied.

However, by reading this comment, I understand that only one (the server or rather the operation) security requirement should be satisfied.

Would you mind clarifying this @sekharbans-ebay? Also pinging Code Owners @derberg @fmvilas @dalelane

Copy link
Author

@sekharbans-ebay sekharbans-ebay Apr 21, 2022

Choose a reason for hiding this comment

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

Now that I'm writing the release notes, this comment seems confusing to me.

The spec says:

In cases where Server Security also applies, it MUST also be satisfied.

However, by reading this comment, I understand that only one (the server or rather the operation) security requirement should be satisfied.

Would you mind clarifying this @sekharbans-ebay? Also pinging Code Owners @derberg @fmvilas @dalelane

The example text is poorly worded - will reword this. My intention was to state that you could still operate on the channel using security credentials supported at the server level perhaps thru a different operation. Is this okay:

  _# This operation level security implies the ability to send a message to 
  # `smartylighting.streetlights.1.0.action.{streetlightId}.turn.on` with Authorization headers 
  #  that have `streetlights:read` scope. Note that the operation level security  must still satisfy 
  # security options specified at the server level._

Copy link
Member

@smoya smoya Apr 22, 2022

Choose a reason for hiding this comment

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

Would you mind creating a PR against 2022-04-release branch with such a fix (and the others I mentioned in other comments) @sekharbans-ebay ?

Copy link
Author

Choose a reason for hiding this comment

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

Would you mind creating a PR against 2022-04-release branch with such a fix (and the others I mentioned in other comments) @sekharbans-ebay ?

Done. Please review:
#768

security:
streetlights_auth:
- streetlights:write
smartylighting.streetlights.1.0.action.{streetlightId}.turn.on:
Copy link
Member

@smoya smoya Apr 22, 2022

Choose a reason for hiding this comment

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

@sekharbans-ebay Would it make sense to link the channel to a server(s)?

You can do this by adding servers field. For example:

servers:
  - test_oauth

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.