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

If the security scheme is provided, Kiota should use it to create the right auth object #5070

Closed
Tracked by #5021
maisarissi opened this issue Aug 1, 2024 · 12 comments · Fixed by #5209
Closed
Tracked by #5021
Assignees
Labels
area:authentication Focused on the extension module of the product enhancement New feature or request generator Issues or improvements relater to generation capabilities.
Milestone

Comments

@maisarissi
Copy link
Contributor

maisarissi commented Aug 1, 2024

OpenAPI manifest can contain API security definitions using the security scheme and they can be referenced either globally or per operation.
If the security scheme is provided and the auth type is supported (vide #5071) Kiota should create the right auth object in the plugin manifest.

The object pattern is like the following:

"runtimes": 
    [
        {   
          "auth": { 
                    "type": "<string>",
                    "reference_id": "<string>"
                  }   
        }
    ]

Pattern A will be used for HTTP+Bearer Token, API Key and OpenId Connect:

"runtimes": 
    [
        {   
          "auth":
                 { 
                    "type": "ApiKeyPluginVault",
                    "reference_id": "{{<security_scheme_name>_REGISTRATION_ID}}"
                  }   
        }
    ]

Pattern B will be used for Oauth:

"runtimes": 
    [
        {   
          "auth":
                  { 
                    "type": "OAuthPluginVault",
                    "reference_id": "{{<security_scheme_name>_CONFIGURATION_ID}}"
                  }   
        }
    ]

For the reference_id, we are using the same pattern as TTK. Vide https://github.com/OfficeDev/teams-toolkit/blob/7422a1dffb7d54ca5e926e025fcdc72ae0380e17/packages/spec-parser/src/manifestUpdater.ts#L122

Acceptance Criteria:

  1. Only the following types are supported: http (with "scheme" = "bearer", https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml), oauth2 and openIdConnect. Any other type will return an error which will contain a list of supported auth types.
  2. Auth object will be generated only if the operation object has a security value and the root document has a security section.
  3. The generated auth object will match the auth type- Pattern A or B above.
  4. Only one entry in the operation security is allowed. More entries will return an error.
  5. Operation Security Requirement Object name needs to correspond to a security scheme defined in the document Security Schemes, error will be returned otherwise.
  6. Scopes of an OAuth type will be added to a new field - its name TBD.
@baywet
Copy link
Member

baywet commented Aug 2, 2024

Trying to add a couple of precisions here.
We want this to apply only when:

  1. The operation object has a security value
  2. The root document has a security section
  3. The operation security object has an entry of which key matches one of the document security entries
  4. The OAS security object is:
    • type oauth2 or http format bearer bearerFormat JWT (OAuthPluginVault)
    • type apiKey (ApiKeyPluginVault)

Questions for you @maisarissi :

  • what should we do when there are multiple entries in the operation security?
  • what should we do when none of the operation security entries match the document security entries?
  • do you expect anything to be done with the scopes in the context of an OAuth type?
  • am I right to assume "{{<security_scheme_name>_REGISTRATION_ID}}" and "{{OAUTH2_CONFIGURATION_ID}}" are just constants? or do you expect some kind of value replacement by kiota here?

@baywet baywet added enhancement New feature or request generator Issues or improvements relater to generation capabilities. status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Aug 2, 2024
@baywet baywet moved this from Needs Triage 🔍 to Waits for author 🔁 in Kiota Aug 2, 2024
@maisarissi
Copy link
Contributor Author

maisarissi commented Aug 2, 2024

Thanks for adding more details to the issue.

On top of the 4 items you mentioned, I would expect:

  1. If there is no security info in the OpenAPI and one haven't provided the auth type an reference ID, Kiota should still create the auth object using the default "None", as the auth object in the plugin manifest is required.

what should we do when there are multiple entries in the operation security?

The scenario where you need to do more than one authenticationq. Kiota should add a validation for multiple auth in one operation and thrown an error if that's the case.

what should we do when none of the operation security entries match the document security entries?

Just to make sure I understand what you mean by this is like, one created for example an oauth2 security scheme in components but in the operation they referencing a bearer one that was never added to the security scheme in the components? If that's the case, I believe OpenAPI spec says this is not valid and that the operation security entries should match the ones in the component security scheme. So we should validate and thrown an error.

do you expect anything to be done with the scopes in the context of an OAuth type?

For oauth, when one is creating the auth registration in the Teams Developer Portal, scopes needed to be included there. In the plugin manifest, there is no place for us to add that info, maybe we could thrown an warning or log to list all scopes? This might be helpful for one when creating the auth registration.

am I right to assume "{{<security_scheme_name>_REGISTRATION_ID}}" and "{{OAUTH2_CONFIGURATION_ID}}" are just constants? or do you expect some kind of value replacement by kiota here?

Yes. The intention here is for us to follow the same pattern as TTK. When using TTK to create the auth registration in ones behalf, TTK creates and env variable with that name that will then be replaced when provisioning. See https://github.com/OfficeDev/teams-toolkit/blob/7422a1dffb7d54ca5e926e025fcdc72ae0380e17/packages/spec-parser/src/manifestUpdater.ts#L122

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Aug 2, 2024
@maisarissi maisarissi moved this from Waits for author 🔁 to Todo 📃 in Kiota Aug 5, 2024
@baywet
Copy link
Member

baywet commented Aug 6, 2024

Thank you for the additional information.

For oauth, when one is creating the auth registration in the Teams Developer Portal, scopes needed to be included there. In the plugin manifest, there is no place for us to add that info

Can you start a conversation with them so we can define an additional field for us to drop the scope information in please? We'll also need to consider whether application only scenarios are a thing or not.

Yes. The intention here is for us to follow the same pattern as TTK

I'm going to ask the question again: do you want "{{<security_scheme_name>_REGISTRATION_ID}}" and "{{OAUTH2_CONFIGURATION_ID}}" as constants? or do you have a clear specification on how we should generate those strings?

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Aug 6, 2024
@maisarissi
Copy link
Contributor Author

Can you start a conversation with them so we can define an additional field for us to drop the scope information in please? We'll also need to consider whether application only scenarios are a thing or not.

Actually I was thinking on us logging in Kiota somehow the scopes, so one could easy get a list of the scopes and add in their auth registration in TDP manually.

I'm going to ask the question again: do you want "{{<security_scheme_name>_REGISTRATION_ID}}" and "{{OAUTH2_CONFIGURATION_ID}}" as constants? or do you have a clear specification on how we should generate those strings?

I think we should do programmatically; same way TTK does:

if (pluginAuthObj.type !== "None") {
  const safeRegistrationIdName = Utils.getSafeRegistrationIdEnvName(
    `${authInfo.name}_${ConstantString.RegistrationIdPostfix[authInfo.authScheme.type]}`
  );

  pluginAuthObj.reference_id = `\${{${safeRegistrationIdName}}}`;
}
static readonly RegistrationIdPostfix: { [key: string]: string } = {
    apiKey: "REGISTRATION_ID",
    oauth2: "CONFIGURATION_ID",
    http: "REGISTRATION_ID",
    openIdConnect: "REGISTRATION_ID",
  };

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Aug 9, 2024
@baywet baywet added this to the Kiota v1.18 milestone Aug 9, 2024
@petrhollayms petrhollayms added the area:authentication Focused on the extension module of the product label Aug 16, 2024
@calebkiage calebkiage self-assigned this Aug 22, 2024
@calebkiage calebkiage moved this from Todo 📃 to In Progress 🚧 in Kiota Aug 22, 2024
@calebkiage
Copy link
Contributor

calebkiage commented Aug 22, 2024

Should we ignore the value of the document root's security property? According to the spec, it should act as a default that applies to all endpoint operation. Individual operations can override it

@baywet
Copy link
Member

baywet commented Aug 22, 2024

This is how I understand the spec

security node operation result
{} N/A anonymous
single entry nothing use the single entry
multiple entries nothing use the first entry?
single entry maps to one entry use the entry that maps
multiple entry maps to one entry use the entry that maps

What do you think?

@calebkiage
Copy link
Contributor

If you're referring to the OpenAPI spec, I understood it differently. According to the spec, there are 2 locations for the security requirement object. The root of the document and under individual operations.

root (top-level security) operation result
N/A N/A anonymous (no security information)
single valid entry N/A require the security scheme defined by the single entry at the root level
multiple valid entries N/A require any of the security schemes defined at the root level
single valid entry single valid entry require the security scheme defined by the single entry at the operation level
multiple valid entries single valid entry require the security scheme defined by the single entry at the operation level
N/A single valid entry require the security scheme defined by the single entry at the operation level
multiple valid entries empty list ([]) anonymous (no security information)

Security being defined at the operation level means the top-level security information is ignored for the operation level security information.

@maisarissi
Copy link
Contributor Author

maisarissi commented Aug 22, 2024

@calebkiage As far as I could understand from the OpenAPI spec, one can only have security requirement object (operation level) if there is a security schema object (root - under components) with the same name.

@calebkiage
Copy link
Contributor

calebkiage commented Aug 23, 2024

That is true @maisarissi, but the security requirement object can be specified in both the top level and the operation level. For example, the following is a valid OpenAPI document:

openapi: 3.0.0
info:
  title: test
  version: "1.0"
security:
  - jwt0: [] # should apply to all operations that don't have security defined
paths:
  /test0:
    get:
      description: description for test0 path (uses jwt0 security)
      responses:
        '200':
          description: test
  /test1:
    get:
      description: description for test1 path (uses apiKey0 security
      responses:
        '200':
          description: test
      security:
        - apiKey0: [] # overrides the root security
components:
  securitySchemes:
    apiKey0:
      type: apiKey
      name: x-api-key
      in: header
    jwt0:
      type: http
      scheme: bearer
      bearerFormat: JWT

My question is, what do we do about this situation?

My suggestion is:
if the operation has a security requirement, we use that. If the operation does not have a security requirement, we check the top level and use that instead (with the same constraints as the operation level i.e. only allow a single security requirement).

@petrhollayms
Copy link
Contributor

petrhollayms commented Aug 23, 2024

I think @calebkiage got it right:

  1. Security requirements can be described both on the top level (OpenAPI object) and operation level.
    See https://spec.openapis.org/oas/v3.0.3#security-requirement-object

When a list of Security Requirement Objects is defined on the OpenAPI Object or Operation Object, only one of the Security Requirement Objects in the list needs to be satisfied to authorize the request.

  1. Security requirements defined at the operation level do overwrite the requirements defined at the top level.
    See https://spec.openapis.org/oas/v3.0.3#fixed-fields-7

This definition overrides any declared top-level security.

Special case here is even to remove any security requirement for the given operation (same link):

To remove a top-level security declaration, an empty array can be used.

Meaning, we really need to consider first the operation level security requirements and look on the requirements at the root OpenAPI Object only if there is none defined on the operation level - meaning, no security object defined there.
Note: an empty array is a valid definition which is removing the need for any authentication for the given method as shown above.

Additionally,

  1. Empty requirement object defined at either level does make the authentication optional.
    See https://spec.openapis.org/oas/v3.0.3#operation-object

To make security optional, an empty security requirement ({}) can be included in the array.

Note: included in the array, meaning in addition to other requirement objects (on the same level).

Implication for us is that we need to decide how to go about it- one entry to generate won't do anymore without being able to mark it as optional. @maisarissi
Also, we can't stop at processing the first requirement which can be {} just to make the authentication definition optional.
Also, our rule of "throw error if there is multiple entries" needs to be adjusted to allow for {}.

  1. There is a difference between (a) multiple security requirements and (b) multiple schemes in one object.
    See https://spec.openapis.org/oas/v3.0.3#security-requirement-object
    And examples: https://swagger.io/docs/specification/authentication/#multiple

(a) Several requirements

When a list of Security Requirement Objects is defined on the OpenAPI Object or Operation Object, only one of the Security Requirement Objects in the list needs to be satisfied to authorize the request.

More complex example of an optional authentication by using either one of the API keys or OAuth2 authentication.

security:
  - {}
  - apiKey0: []
  - apiKey1: []
  - petstore_auth:
    - write:pets
    - read:pets

If we keep our rule to throw error on multiple requirements defined, we won't be able to support a such API.
Not sure it is a big deal in our context now? @maisarissi , @darrelmiller

(b) Several schemes
Security Requirement Objects that contain multiple schemes require that all schemes MUST be satisfied for a request to be authorized. This enables support for scenarios where multiple query parameters or HTTP headers are required to convey security information.

Example of an optional authentication of an API which supports a pair of query parameters to be included in requests:

security:
  - {}
  - query_param_1: []
    query_param_2: []

We need to decide whether we want to support this option or not. @maisarissi , @darrelmiller ?

@darrelmiller
Copy link
Member

@petrhollayms Thank you Petr for one of the more comprehensive explanations of OpenAPI security that I have seen. It is somewhat terrifying to think that, as complex as it is, the OpenAPI security model is not nearly sophisticated enough to represent many real world scenarios. But I digress.

There are two questions that need to be answered:

  • How should security features of OpenAPI should be translated into the Plugin Manifest based on what the Plugin Manifest specification supports?
  • How do we prevent users from using features that the Plugin Manifest supports but our Copilot implementation currently does not support?

One approach is to say, we only create manifests with features that Copilot supports. That comes with the risk of us having to constantly update our generation process as new feature support is rolled out.

Another approach is to support all the features of the Plugin Manifest, and then use validation rules to indicate when someone is using a feature that is currently not supported by the implementation.

As our goal is for the Plugin Manifest to be used by more than just M365 Copilot, I would recommend the latter approach. It is also much easier to remove a validation rule than it is to update the generation process.

So, based on this, we need to look at the constraints of the Plugin Manifest first. The Plugin Manifest supports having multiple "Security Requirements" by using multiple runtimes that point to the same OpenAPI but using slightly different names for the functions.
e.g. GetIssuesAnoymously and GetIssuesWithApiKey

However, it does not currently support the notion of multiple Schemes per security Requirement. As multiple schemes per security Requirement are an "AND" scenario, it means we cannot support calling an API that has multiple schemes. (That sucks and should be fixed in our Copilot implementation because APIM solutions do this all the time).

@baywet baywet linked a pull request Aug 30, 2024 that will close this issue
@baywet
Copy link
Member

baywet commented Aug 30, 2024

@calebkiage any objection to closing this issue as done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:authentication Focused on the extension module of the product enhancement New feature or request generator Issues or improvements relater to generation capabilities.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants