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 Bearer security scheme & apply globally #179

Closed
wants to merge 1 commit into from
Closed

feat: add Bearer security scheme & apply globally #179

wants to merge 1 commit into from

Conversation

uniqueg
Copy link
Contributor

@uniqueg uniqueg commented Jul 26, 2022

Fixes #151

This PR is an alternative to #162, raised in response to an evaluation of the security definitions across a range of GA4GH OpenAPI specs. It precisely follows the definition and application of security schemes in the GA4GH Service Registry and Service Info specifications.

The reasoning to define only the Bearer authentication security scheme (as opposed to specifying Bearer, Basic and Passport-based schemes as in #162) is as follows:

  • In my opinion, only those security schemes should be defined that all implementations are expected to implement. Implementations can easily add additional security schemes that they choose to support, but I think that we should rather remain conservative with regard to the security schemes that we mandate implementers to adopt.
  • Across the GA4GH Cloud API, as well as the GA4GH Discovery API specs for Service Registry and Service Info, the common theme appears to be to require the adoption of Bearer authentication. Being compatible with OAuth2, this is in line with GA4GH's Data Security vision statement and thus seems a reasonable common denominator for OpenAPI security scheme definitions. The exception here is the security scheme definition in the TRS specification, which technically is of type apiKey, but in spirit resembles Bearer authentication, and its definition as type apiKey is likely an artifact of a previous OpenAPI/Swagger 2.0 version of the specification (OpenAPI/Swagger 2.0 does not explicitly support Bearer authentication), as mentioned in this comment.
  • GA4GH Passports, at least in their current form, are designed to provide a means of access control to data objects, not compute resources, the type of resource that TES implementations typically/generally provide. A security scheme specifically designed for GA4GH Passports therefore makes sense as part of the DRS specification (which provides access to data objects), but is probably not a good fit for the TES specification. If the scope of GA4GH Passports should expand in the future to also provide a means of access control for compute resources, a corresponding security scheme can still be added.
  • Basic authentication, i.e., a base64-encoded usernam:password string passed in the Authorization header, is one of the security schemes defined in the DRS specification. And while this type of Authorization/Authentication is certainly useful for what are basically data object repositories, which, presumably, would frequently have frontends that users could log into to browse available objects, TES implementations are not very likely to be used in this way, and so, in my opinion, the case for mandating its adoption across TES implementations is weak. Moreover, Basic authentication may pose a security risk if not combined with additional measures of security.

The reasoning to apply the Bearer authentication security scheme globally, i.e., to all operations, is as follows:

  • Compute resources should never be provisioned without authentication/authorization. Neither should anonymous users be allowed to modify tasks or even receive information about individual tasks.
  • The global application is consistent with that in the GA4GH Service Registry and Service Info specifications.

Note that definitions for 401/Unauthorized responses were deliberately not added to the operations as part of this PR, as the TES specification (unlike other GA4GH OpenAPI specifications) does currently not specify any specific error responses to operations, such as 400/Bad Request or 404/Not Found. If desired for clarity and/or consistency, those could be added in a different PR.

@uniqueg uniqueg closed this by deleting the head repository Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant