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

security; add new securityScheme type of mutualTLS #1764

Merged
merged 1 commit into from
Feb 7, 2019

Conversation

MikeRalphson
Copy link
Member

@MikeRalphson MikeRalphson commented Nov 28, 2018

Refs #1004 and #1366 & #1393

I'm still personally of the opinion a property which hints that the server may present a self-signed certificate would be useful to allow any or all of the following interactions:

  • denial of the request
  • prompt of a user or requiring of a special option
  • configuring client TLS libraries

But hey, we can always add it later.

@cmheazel
Copy link
Contributor

@MikeRalphson That was next on my list. Thanks for beating me to the punch.

@@ -3340,7 +3340,7 @@ When a list of Security Requirement Objects is defined on the [OpenAPI Object](#

Field Pattern | Type | Description
---|:---:|---
<a name="securityRequirementsName"></a>{name} | [`string`] | Each name MUST correspond to a security scheme which is declared in the [Security Schemes](#componentsSecuritySchemes) under the [Components Object](#componentsObject). If the security scheme is of type `"oauth2"` or `"openIdConnect"`, then the value is a list of scope names required for the execution. For other security scheme types, the array MUST be empty.
<a name="securityRequirementsName"></a>{name} | [`string`] | Each name MUST correspond to a security scheme which is declared in the [Security Schemes](#componentsSecuritySchemes) under the [Components Object](#componentsObject). If the security scheme is of type `"oauth2"` or `"openIdConnect"`, then the value is a list of scope names required for the execution. For other security scheme types, the array MAY contain a list of role names which are required for the execution, but are not otherwise defined or exchanged in-band.
Copy link
Member

Choose a reason for hiding this comment

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

I'm all for this proposal in general. I would like to discuss this last item a bit in today's call.

Copy link
Contributor

Choose a reason for hiding this comment

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

@darrelmiller @MikeRalphson

As we're adding support for custom securitySchemes #1812
I suggest to:

@ioggstream
Copy link
Contributor

ioggstream commented Dec 17, 2018

What did you agree for the other bits, eg. #1004 (comment)?

About the self-signed:

Copy link
Contributor

@ioggstream ioggstream left a comment

Choose a reason for hiding this comment

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

Agree on adding mutualTLS

@@ -3165,12 +3165,12 @@ animals:
#### <a name="securitySchemeObject"></a>Security Scheme Object

Defines a security scheme that can be used by the operations.
Supported schemes are HTTP authentication, an API key (either as a header, a cookie parameter or as a query parameter), OAuth2's common flows (implicit, password, application and access code) as defined in [RFC6749](https://tools.ietf.org/html/rfc6749), and [OpenID Connect Discovery](https://tools.ietf.org/html/draft-ietf-oauth-discovery-06).
Supported schemes are HTTP authentication, an API key (either as a header, a cookie parameter or as a query parameter), mutual TLS (use of a client certificate), OAuth2's common flows (implicit, password, application and access code) as defined in [RFC6749](https://tools.ietf.org/html/rfc6749), and [OpenID Connect Discovery](https://tools.ietf.org/html/draft-ietf-oauth-discovery-06).
Copy link
Contributor

Choose a reason for hiding this comment

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

+1


##### Fixed Fields
Field Name | Type | Applies To | Description
---|:---:|---|---
<a name="securitySchemeType"></a>type | `string` | Any | **REQUIRED**. The type of the security scheme. Valid values are `"apiKey"`, `"http"`, `"oauth2"`, `"openIdConnect"`.
<a name="securitySchemeType"></a>type | `string` | Any | **REQUIRED**. The type of the security scheme. Valid values are `"apiKey"`, `"http"`, `"oauth2"`, `"openIdConnect"`, `"mutualTLS"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@MikeRalphson
Copy link
Member Author

PR pared back to just adding mutualTLS as a new securityScheme type (omitting the widening of security scopes to non-oAuth types), as discussed.

@MikeRalphson MikeRalphson changed the title security; mutualTLS and non-oauth2 scopes security; add new securityScheme type of mutualTLS Feb 6, 2019
Copy link
Contributor

@ioggstream ioggstream left a comment

Choose a reason for hiding this comment

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

LGTM as discussed in the last TSC :)

Copy link
Member

@whitlockjc whitlockjc left a comment

Choose a reason for hiding this comment

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

LGTM

@earth2marsh
Copy link
Member

earth2marsh commented Feb 7, 2019

LGTM.

Another benefit of signaling mTLS is that from the docs perspective you can inform about the security model. That's great. I would expect that the client on-boarding process would provide info on acquiring a cert, and I'd wonder if we should consider an externalDocs link for security schemes in a future PR?

One more observation (which has no real bearing on this PR, I just find it interesting) is that most security schemes are observable in transit, such as from an API gateway. It would seem that mTLS wouldn't be observable as a requirement.

@webron
Copy link
Member

webron commented Feb 7, 2019

Looks like we have the votes on the call from the @OAI/tsc, merging!

Copy link
Member

@earth2marsh earth2marsh left a comment

Choose a reason for hiding this comment

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

approved per today's TSC call

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.

7 participants