camaraproject/IdentityAndConsentManagement

Review the APIs which are targeting "stable" maturity in the Fall24 meta-release

tanjadegroot opened this issue · 12 comments

tanjadegroot commented

Problem description
Review the stable APIs and their compliance to ICM, especially on topics beyond the basis checks, such as API abuse topic, security scheme, error responses, or other.
Request following TSC meeting of 2024-08-01.

Expected action
Review the 5 APIs targeting stable public release. The APIs are here: see https://wiki.camaraproject.org/x/cgB0AQ (click twice on the targeted maturity column to sort the table with stable APIs at the top of the list).

  • if you have a comment: create an issue in the Sub Project: "ICM review" and link it to the review issue in release management for that Sub Project repo
  • if you have no comments: put a comment "Reviewed by ICM" in the review issue in release management for that Sub Project repo.

Additional actions:
Please complete the RM release PR review guidelines for items that are not listed there (see link below).
Also create an ICM issue if you see that additional linting rules can be created to automate the checks (e.g. a check that security schemas are aligned with ICM guidelines, that AUth section is present in the descriptionfield, etc.

Additional context
Current checks done by the RM team are described here: https://wiki.camaraproject.org/x/UwESAg

jpengar commented

I would like to align the expectations of Release Management and TSC on the ICM validations that should be covered for the stable APIs.

Below I provide a list of validations that ICM could perform according to the existing definitions. Please add if I am missing anything and if agreed we can add the final list of items to the RM team's current checks document here: https://wiki.camaraproject.org/x/UwESAg

  • Check the ICM-defined info.description template (Authorization and Authentication section). Reference
  • Check the use of openIdConnect for `securitySchemes'. Reference
  • Check the use of the `security' property according to ICM definitions. Reference
  • Error codes are defined by Commonalities. However, the ICM could check the definition of a 403 INVALID_TOKEN_CONTEXT if it applies to a specific API (e.g. APIs using device object or phoneNumber in the API request). Reflects an inconsistency between information in some field of the API request and the access token.
  • Verify that there is no unexpected loss of users' personal information, such as API responses containing identifiers or information beyond the API functionality.
  • UPDATE (22/08): For APIs that use the device object in the API requests, ICM could check that the API spec includes the required section called "Identifying a device from the access token" in info.description that provides a detailed description of the expected handling of the device object in the API request as it relates to the access token. Reference
    • Some APIs, such as SIM Swap, define analogous sections like "Identifying a phone number from the access token" when using the phoneNumber field instead of the device object. However, this is not yet mandated by ICM/Commonalities as for the device object.

When is ICM expected to complete these checks for stable APIs? What is the deadline?.

There are also some open traversal issues that potentially affect these stable APIs as well, we should reach some kind of consensus on whether these issues are blockers or not and/or whether they need to be resolved for this meta release and/or considered in the ICM review...

Please complete the list if I have missed anything else. Note that while these issues are open in Commonalities, they are also tied to ICM definitions. And as far as I understand from the last Commonalities call, these two issues will not be included in the scope of this meta-release. But we should all be aware to adjust our expectations and consider them or not in the ICM review. CC @rartych.

In terms of new linting rules for ICM checks, I would say that the straight forward ones would be:

  • Check ICM-defined securitySchemes
  • Check ICM-defined security field

Anything else to add here?

There is also an existing open issue to add ICM security definitions to the CAMARA Common YAML file:

We may be able to align expectations and next action items to do during the next ICM bi-weekly call (14/08).

AxelNennker commented

@camaraproject/release-management_maintainers @tanjadegroot What is the timeline for the reviews, please?
We noticed that some releasmanagement review issues are already closed (without ICM review)

hdamker commented

@camaraproject/release-management_maintainers @tanjadegroot What is the timeline for the reviews, please? We noticed that some releasmanagement review issues are already closed (without ICM review)

@AxelNennker @jpengar The closed review issues were the ones for the M3 milestone with the release candidates. We will open new review issues in release management as soon as the release PRs for the M4 milestone are coming in. The Sub Project have to open their PRs for the public release until August 26th.

For the review by ICM I propose to review the r1.1 releases of the 5 APIs which have declared to create a stable release, which are:

A short version of the results can be documented here in the issue (or dedicated review issues here in ICM for each API), found issues should be opened within the repositories. It would be good to have done the reviews before the deadline mentioned above (August 26th), so there is time to resolve issues which can't wait for the next release.

jpengar commented

For the review by ICM I propose to review the r1.1 releases of the 5 APIs which have declared to create a stable release, which are:

@hdamker @AxelNennker

Ok, I will check Device Location, OTP Validation & SIM Swap from my side.

Just to align expectations, the validations to be performed are the ones listed above according to the existing ICM definitions. Please let me know if you are missing anything.

  • 👍1
jpengar commented

ICM Review - OTP Validation - one-time-password-sms v1.0.0

Ref: https://github.com/camaraproject/OTPValidation/releases/tag/r1.1

  • Check the ICM-defined info.description template (Authorization and Authentication section). Reference
    ✅ OK

  • Check the use of openIdConnect for `securitySchemes'. Reference
    ✅ OK

  • Check the use of the `security' property according to ICM definitions. Reference
    ✅ OK

  • Error codes are defined by Commonalities. However, the ICM could check the definition of a 403 INVALID_TOKEN_CONTEXT if it applies to a specific API (e.g. APIs using device object or phoneNumber in the API request). Reflects an inconsistency between information in some field of the API request and the access token.
    ⚠️ OK, with comments. The API spec does not define the 403 INVALID_TOKEN_CONTEXT error as per CAMARA Commonalities common YAML file. However, this error is not needed for the API use case.

  • Verify that there is no unexpected loss of users' personal information, such as API responses containing identifiers or information beyond the API functionality.
    ✅ OK

ICM Review Result: ✅ OK

  • 👍1
  • ❤️1
jpengar commented

ICM Review - Sim Swap - sim-swap v1.0.0

Ref: https://github.com/camaraproject/SimSwap/releases/tag/r1.1

  • Check the ICM-defined info.description template (Authorization and Authentication section). Reference
    ✅ OK

  • Check the use of openIdConnect for securitySchemes. Reference
    ✅ OK

  • Check the use of the security property according to ICM definitions. Reference
    ✅ OK. It defines two "openId" entries (as an OR operation) because this API spec defines the possibility to access the two existing endpoints with a common scope "sim-swap" or with an endpoint-specific scope, e.g. "sim-swap:retrieve-date"

  • Error codes are defined by Commonalities. However, the ICM could check the definition of a 403 INVALID_TOKEN_CONTEXT if it applies to a specific API (e.g. APIs using device object or phoneNumber in the API request). Reflects an inconsistency between information in some field of the API request and the access token.
    ✅ OK. The API specification also includes a special section called "Identifying a phone number from the access token" in info.description that provides a detailed description of the expected handling of the phoneNumber field as it relates to the access token. It was already reviewed in camaraproject/SimSwap#117

  • Verify that there is no unexpected loss of users' personal information, such as API responses containing identifiers or information beyond the API functionality.
    ⚠️ OK with comments. camaraproject/Commonalities#259 was not included in this review. It is a known traversal problem that is NOT in the scope of this meta-release.

ICM Review Result: ✅ OK

NOTE: r1.1 also includes "Sim Swap Subscriptions v0.1.0", but it is not intended to be a "stable" release and is outside the scope of this review.

  • 👍1
  • ❤️1
jpengar commented

ICM Review - Device Location - location-verification v1.0.0

Ref: https://github.com/camaraproject/DeviceLocation/releases/tag/r1.1

  • Check the ICM-defined info.description template (Authorization and Authentication section). Reference
    ✅ OK

  • Check the use of openIdConnect for securitySchemes. Reference
    ✅ OK

  • Check the use of the security property according to ICM definitions. Reference
    ✅ OK.

  • Error codes are defined by Commonalities. However, the ICM could check the definition of a 403 INVALID_TOKEN_CONTEXT if it applies to a specific API (e.g. APIs using device object or phoneNumber in the API request). Reflects an inconsistency between information in some field of the API request and the access token.
    ✅ OK. The API specification also includes the required section called "Identifying a device from the access token" in info.description that provides a detailed description of the expected handling of the device object in the API request as it relates to the access token. It is specified in Appendix A: info.description template for device identification from access token and it is required for APIs that use the device object in the API requests.

  • Verify that there is no unexpected loss of users' personal information, such as API responses containing identifiers or information beyond the API functionality.
    ⚠️ OK with comments. camaraproject/Commonalities#259 was not included in this review. It is a known traversal problem that is NOT in the scope of this meta-release.

ICM Review Result: ✅ OK

NOTE: r1.1 also includes "Device Location Retrieval v0.3.0" and "Device Geofencing Subscriptions v0.3.0", but these APIs are not intended to be a "stable" release and are outside the scope of this review.

  • 👍2
AxelNennker commented

Starting to review NumberVerification now.

  • 👍1
AxelNennker commented

ICM Review - NumberVerification - v1.0.0

Ref: https://github.com/camaraproject/NumberVerification/releases/tag/r1.1

ICM Review Result: ✅ OK

  • 👍1
AxelNennker commented

ICM Review - SimpleEdgeDiscovery - v1.0.0

Ref: https://github.com/camaraproject/SimpleEdgeDiscovery/blob/r1.2/code/API_definitions/simple-edge-discovery.yaml

ICM Review Result: ✅ OK

Kevsy commented

Many thanks for the SimpleEdgeDiscovery review @axel -

The API specification does not include the recommended section called "Identifying a device from the access token" in info.description that provides a detailed description of the expected handling of the device object in the API request as it relates to the access token

It is included - see lines 115-137 - as a sub-section of the "Identifying the device" section (because it is one of the options). Is that ok...?