Include OIDC securityScheme, format scopes and add x-correlator header #53

fernandopradocabrillo · 2023-09-07T12:14:24Z

What type of PR is this?

Add one of the following kinds:

enhancement/feature

What this PR does / why we need it:

Align securitySchemes with the current commonality proposal to use OpenId to encapsulate grant types and authorization flows
Change the scopes format to the current proposal of commonalities and align with the rest of CAMARA APIs
Include X-Correlator headers according to Design Guidelines
Update API version to 0.5.0-wip

Sources for points 1 and 2:
* Commonalities Issue: camaraproject/Commonalities#53
* Identity and Consent Issue: camaraproject/IdentityAndConsentManagement#57
* Commonalities PR: camaraproject/Commonalities#57

Which issue(s) this PR fixes:

Fixes (#49) Add x-correlator as header attribute

Special notes for reviewers:

Notice that the documentation is still not update, once we merge the documentation PR (#52) I'll sync this one

Changelog input

N/A

Additional documentation

N/A

bigludo7

LGTM/

… into update-yaml-and-commonalities-sync

fernandopradocabrillo · 2023-09-08T11:33:01Z

@bigludo7 I updated the PR after the merge of the documentation PR. Now it is synchronized with the documentation.

Please approve again since it has been dismissed automatically 😄

bigludo7

LGTM/

code/API_definitions/sim_swap.yaml

hdamker

To have a base release within this repository, I propose to create an (alpha) release v0.4.1 of the current content in main before this PR will be merged and you continue with v0.5.0-wip. This also as some companies are want to start implemantation on a release version, knowing that they have to adapt the security schema and other points later.

hdamker · 2023-09-15T16:51:59Z

In addition the same comment as in camaraproject/NumberVerification#60 (review) ... in my view X-Correlator does not need to be defined within the OAS here.

hdamker · 2023-09-19T12:59:55Z

According to the discussion within #55 I agreee that this PR might need to be discussed and merged before a first release of this repo can be produced.

@fernandopradocabrillo

Unfortunately this PR is addressing two different issues which have unrelated discussions. I would recommend to reduce the scope of this PR to securityScheme and format scopes, but carve the X-Correlator discussion out into a separate PR later. That will allow that the discussion of in #49 will not block the PR (and in consequence a first release).

fernandopradocabrillo · 2023-09-20T12:01:22Z

@fernandopradocabrillo

Unfortunately this PR is addressing two different issues which have unrelated discussions. I would recommend to reduce the scope of this PR to securityScheme and format scopes, but carve the X-Correlator discussion out into a separate PR later. That will allow that the discussion of in #49 will not block the PR (and in consequence a first release).

@hdamker Sure, I will remove the x-correlator from this PR and open a new one so we only have the securitySchemes and scopes changes here

eric-murray · 2023-09-21T16:37:14Z

I might be missing something here, but why does this version of the API still require the MSISDN (phone number) to be passed in the /check and /retrieve-date calls when this will already be known from the authorisation token? I can see that it was required when client credentials were an option, but don't understand why it is still required for the three-legged flow as user consent is checked in a previous step. Maybe someone could enlighten me.

fernandopradocabrillo · 2023-09-21T17:00:29Z

I might be missing something here, but why does this version of the API still require the MSISDN (phone number) to be passed in the /check and /retrieve-date calls when this will already be known from the authorisation token? I can see that it was required when client credentials were an option, but don't understand why it is still required for the three-legged flow as user consent is checked in a previous step. Maybe someone could enlighten me.

@eric-murray I also think it should be removed. My idea was to first solve the authorization topic and the create an issue and a PR to remove it, since the new version is still going to be on wip until we go for a release. Step by step 😄

eric-murray · 2023-09-21T17:16:20Z

Thanks @fernandopradocabrillo for the prompt response. So I understand that removing this parameter will be discussed in a later step. So now I'm no longer confused! Many thanks.

gregory1g · 2023-09-25T05:05:56Z

@eric-murray I also think it should be removed. My idea was to first solve the authorization topic and the create an issue and a PR to remove it, since the new version is still going to be on wip until we go for a release. Step by step 😄

This was discussed earlier, when both client credentials and 3-legged where considered. Simply to keep payload auth independent. However, if 3-legged will be the only auth method, MSISDN in the input becomes redundant.

MSISDN is declared as a sensitive information, this leads to special handling (like putting it into header to reduce risk of occasional leakage). From this point of view, removing MSISDN from the payload completely is the best protection.

BUT, we should not postpone this step for a long time - removing MSISDN is a not backward compatible change.

fernandopradocabrillo · 2023-09-25T09:45:15Z

@gregory1g @eric-murray
Issue and PR created to solve this issue:
Issue: #58
PR: #59

fernandopradocabrillo · 2023-09-25T09:46:33Z

@fernandopradocabrillo
Unfortunately this PR is addressing two different issues which have unrelated discussions. I would recommend to reduce the scope of this PR to securityScheme and format scopes, but carve the X-Correlator discussion out into a separate PR later. That will allow that the discussion of in #49 will not block the PR (and in consequence a first release).

@hdamker Sure, I will remove the x-correlator from this PR and open a new one so we only have the securitySchemes and scopes changes here

@hdamker
Update the PR removing the x-correlator changes

eric-murray · 2023-10-09T14:37:42Z

@DT-DawidWroblewski
The decision is now to retain Client Credentials as a valid authorisation scheme for SIM Swap? As your meetings are not minuted, it can be difficult to follow.

fernandopradocabrillo · 2023-10-09T15:01:38Z

@DT-DawidWroblewski The decision is now to retain Client Credentials as a valid authorisation scheme for SIM Swap? As your meetings are not minuted, it can be difficult to follow.

@eric-murray
from Telefonica side, as stated in the meeting, we don't agree with that. We are okey maintaining the securitySchemes as OpenId which is what is being agreed in commonalities.
We should merge the PR with the agreed changes and discuss in commonalities if there should be an indication of the allowed auth flows inside the yaml.

This PR seems to be going in that direction: camaraproject/Commonalities#57

eric-murray · 2023-10-09T15:21:36Z

Thanks @fernandopradocabrillo. In the absence of meeting minutes, I shall wait to see if this PR is approved and merged in its current state.

DT-DawidWroblewski · 2023-10-10T06:54:14Z

Hi @eric-murray !

you can find MoMs in dedicated branch, here:

MoM

I was asked to make a clean-up and add MoMs to main branch, so I'll do it later this week.

hdamker · 2023-10-10T07:54:46Z

code/API_definitions/sim_swap.yaml

@@ -42,7 +49,7 @@ info:
  license:
    name: Apache 2.0
    url: https://www.apache.org/licenses/LICENSE-2.0.html
-  version: 0.4.1
+  version: 0.8.0


As long as you hadn't the interim versions like 0.5.0, please don't jump here for marketing reason. 0.8.0 is not nearer to v1.0 then any other 0.x version (and please remind that after v0.9.0 there is v0.10.0, not automatically v1.0).

got it - as long as we can converge towards release, version is not an issue

we stick to 0.5.0-wip as we're changing securityScheme based on consensus we reached on last meeting

@hdamker
This comment has been addressed, can you remove the change request to merge the PR? Thanks!

@fernandopradocabrillo yes, this is addressed.

DT-DawidWroblewski · 2023-10-10T11:09:22Z

@bigludo7 can you please review changes, so we can merge them and create a release version? thx!

fernandopradocabrillo · 2023-10-10T11:42:20Z

code/API_definitions/sim_swap.yaml

@@ -30,6 +30,13 @@ info:

    - POST check: Checks if SIM swap has been performed during a past period (defined in the request with 'maxAge' attribute) for a given phone number.

+    The API supports 2 security security schemes:
+
+    - clientCredentials


@bigludo7 @DT-DawidWroblewski @eric-murray
On this topic (at the risk of sounding repetitive) there is no agreement yet..
I understand the need to provide a release version, which I completely agree with, but it is a complicated issue that involves conversations in other WG at different levels.
In my opinion, we should release a version with the securitySchemes agreed (OpenId) and open a new PR to provide more information in the yaml description if that is decission on commonalities.

Hi @fernandopradocabrillo !

We agreed on our last sub-group meeting to have openIdConnect security scheme with well-known, where developer derives available security schemes. Additionaly, we provide information about available security schemes in description.

Alternatively, we can leave our agreement and simply create a release version based on existing API description that explicitly covers clientCredentials & three-legged-token, but that is against our consensus.

I suggest to continue with this PR and open a new one, that observes the outcome from other WGs.

Otherwise, we cannot converge towards release version...

Hi @DT-DawidWroblewski !
My comment is mainly focused on the main issue we are dealing with here. We cannot explicitly say that we support client credentials because that is not what has been agreed. And this is a discussion that unfortunately we cannot have in the subgroup and decide unilaterally, it has to be agreed in I&C and Commonalities.
In order to proceed with the release, including the OpenId securityScheme allows us to unlock this issue by delegating this decision to the I&C discussion and revisit it in the future if there is a new agreement.

We agreed that we want to give developer a hint what security schemes are available for SimSwap API, if a particular Auth Server has such capabilities. Can you suggest a place in yaml where we can add this? Thx!

@DT-DawidWroblewski @fernandopradocabrillo I'm bit lost now.

Can you suggest a place in yaml where we can add this?

If we use open id as described for example here https://swagger.io/docs/specification/authentication/openid-connect-discovery/ the objective is to not mix authorization and API yaml definition.

But @DT-DawidWroblewski I understood that it will not prevent you to use client credential as long as it is defined in the https://server.com/.well-known/openid-configuration file.

If we have in the yaml both openId & client credential listed that will be confusing no ?

I saw a comment inside Id&C working group. In order to be compliant with OAS, we should explicitly state both security schemes.

If we don't want to follow the consensus we made during a meeting, I suggest waiting until Issue #57 inside Id&C working group is closed.

DT-DawidWroblewski · 2023-10-27T06:14:15Z

Hi @fernandopradocabrillo @bigludo7 !

I updated API description according to recent OGW suggestion.

Please review/approve PR, so we can merge it and create release version.

Thx!
David

fernandopradocabrillo · 2023-10-27T07:42:19Z

Hi @fernandopradocabrillo @bigludo7 !

I updated API description according to recent OGW suggestion.

Please review/approve PR, so we can merge it and create release version.

Thx! David

@DT-DawidWroblewski
Great! Thanks for the update. It's fine from my side

I'm not sure about the version change, we are already in the 0.4.1, I don't think we can go now for a 0.4.1-wip. I think it is best to continue to a 0.5.0 and make the release in that version. We don't have anything else new on the table right now I believe.
what do you think @bigludo7 @gregory1g @eric-murray

bigludo7 · 2023-10-27T08:52:05Z

@fernandopradocabrillo @DT-DawidWroblewski
Same than @fernandopradocabrillo - As we have already the 0.4.1 should be 0.5.0-wip or 0.4.2-wip
thanks

DT-DawidWroblewski · 2023-10-27T09:56:37Z

Ok - I think that v0.5.0 makes sense :)

I'll update accordingly...

DT-DawidWroblewski · 2023-10-27T09:59:46Z

@fernandopradocabrillo @bigludo7 please review

eric-murray · 2023-10-27T10:18:29Z

code/API_definitions/sim_swap.yaml

-            retrieve-sim-swap-date: retrieveSimSwapDate operation
+    openId:
+      type: openIdConnect
+      openIdConnectUrl: https://example.com/.well-known/openid-configuration


I know this is currently a WIP but, given the existence of extension grants, the .well-known/openid-configuration could include any grant type that has a defined urn. Could we not give some guidance to those looking to implement this via a description documenting what grant types they might find in openid-configuration for this API and which they won't?

Or is that just too controversial at the moment?

For the moment it's been agreed to not indicate further information inside the yaml. I think there are further details regarding this in the I&C workgroup.

Can you point me to the agreement that says YAMLs must not say anything about what security schemes are applicable for that API?

I'm checking with my collegues and it seems I wasn't align with the final decisions on this topic and all the details. I'm sorry for the misunderstanding. From TEF we are align with what was decided in the TSC.
Would it be okey to merge this PR with the schemes and continue with the discussion of appropiate wording/way to put this information in a different issue?
It seems that this is something that affects all apis and we should document in the same way for all apis.

OK, let's return to this topic later. It needs to be resolved before any release.

fernandopradocabrillo · 2023-10-27T10:38:19Z

@fernandopradocabrillo @bigludo7 please review

LGTM

I can't approve because it's my PR but it's fine from my side :)

bigludo7

LGTM

DT-DawidWroblewski

LGTM

hdamker

Approval to remove my change request which was not related to topic at hand.

But there is the need to revist the documentation towards developers before a release of the API as stated multiple times above in comments, latest by @eric-murray in #53

fernandopradocabrillo added 2 commits September 7, 2023 12:12

include oidc securityScheme, format scopes and add x-correlator header

92651f3

change yaml file name

7a92b46

fernandopradocabrillo requested a review from DT-DawidWroblewski as a code owner September 7, 2023 12:14

fernandopradocabrillo self-assigned this Sep 7, 2023

fernandopradocabrillo requested a review from bigludo7 as a code owner September 7, 2023 12:14

bigludo7 previously approved these changes Sep 7, 2023

View reviewed changes

Merge branch 'main' of https://github.com/fernandopradocabrillo/SimSwap…

69c5981

… into update-yaml-and-commonalities-sync

fernandopradocabrillo dismissed bigludo7’s stale review via 69c5981 September 8, 2023 11:29

fernandopradocabrillo requested a review from bigludo7 September 8, 2023 11:31

bigludo7 previously approved these changes Sep 8, 2023

View reviewed changes

DT-DawidWroblewski reviewed Sep 11, 2023

View reviewed changes

code/API_definitions/sim_swap.yaml Show resolved Hide resolved

DT-DawidWroblewski reviewed Sep 11, 2023

View reviewed changes

code/API_definitions/sim_swap.yaml Show resolved Hide resolved

hdamker requested changes Sep 15, 2023

View reviewed changes

hdamker mentioned this pull request Sep 15, 2023

SimSwap API release version scope 0.4.0 #54

Closed

DT-DawidWroblewski mentioned this pull request Sep 20, 2023

Usage of OAUTH2.0 client credentials in SimSwap API camaraproject/IdentityAndConsentManagement#65

Closed

remove x-correlator from PR

9bda743

fernandopradocabrillo dismissed bigludo7’s stale review via 9bda743 September 25, 2023 08:25

fernandopradocabrillo requested review from bigludo7 and hdamker September 25, 2023 08:26

hdamker removed their request for review September 25, 2023 10:39

adding supported security schemes + version number

44e8b35

DT-DawidWroblewski requested a review from hdamker October 10, 2023 07:45

hdamker requested changes Oct 10, 2023

View reviewed changes

keeping v.0.5.0-wip

02c42ac

fernandopradocabrillo commented Oct 10, 2023

View reviewed changes

refactor: security scheme update

94cc561

refactor: version update

286f2cd

eric-murray reviewed Oct 27, 2023

View reviewed changes

bigludo7 approved these changes Oct 27, 2023

View reviewed changes

DT-DawidWroblewski approved these changes Oct 30, 2023

View reviewed changes

DT-DawidWroblewski requested a review from hdamker October 30, 2023 06:29

hdamker approved these changes Oct 30, 2023

View reviewed changes

fernandopradocabrillo merged commit 6eddd86 into camaraproject:main Oct 30, 2023

Include OIDC securityScheme, format scopes and add x-correlator header #53

Include OIDC securityScheme, format scopes and add x-correlator header #53

Conversation

fernandopradocabrillo commented Sep 7, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewers:

Changelog input

Additional documentation

bigludo7 left a comment

Choose a reason for hiding this comment

fernandopradocabrillo commented Sep 8, 2023 • edited Loading

bigludo7 left a comment

Choose a reason for hiding this comment

hdamker left a comment

Choose a reason for hiding this comment

hdamker commented Sep 15, 2023 • edited Loading

hdamker commented Sep 19, 2023

fernandopradocabrillo commented Sep 20, 2023

eric-murray commented Sep 21, 2023

fernandopradocabrillo commented Sep 21, 2023

eric-murray commented Sep 21, 2023

gregory1g commented Sep 25, 2023

fernandopradocabrillo commented Sep 25, 2023

fernandopradocabrillo commented Sep 25, 2023

eric-murray commented Oct 9, 2023 • edited Loading

fernandopradocabrillo commented Oct 9, 2023

eric-murray commented Oct 9, 2023

DT-DawidWroblewski commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DT-DawidWroblewski commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandopradocabrillo Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DT-DawidWroblewski commented Oct 27, 2023

fernandopradocabrillo commented Oct 27, 2023

bigludo7 commented Oct 27, 2023

DT-DawidWroblewski commented Oct 27, 2023

DT-DawidWroblewski commented Oct 27, 2023

Choose a reason for hiding this comment

fernandopradocabrillo Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandopradocabrillo commented Oct 27, 2023 • edited Loading

bigludo7 left a comment

Choose a reason for hiding this comment

DT-DawidWroblewski left a comment

Choose a reason for hiding this comment

hdamker left a comment

Choose a reason for hiding this comment

fernandopradocabrillo commented Sep 8, 2023 •

edited

Loading

hdamker commented Sep 15, 2023 •

edited

Loading

eric-murray commented Oct 9, 2023 •

edited

Loading

fernandopradocabrillo Oct 10, 2023 •

edited

Loading

fernandopradocabrillo Oct 27, 2023 •

edited

Loading

fernandopradocabrillo commented Oct 27, 2023 •

edited

Loading