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

Clarify security; state what [] means #4007

Closed
wants to merge 1 commit into from

Conversation

rvedotrc
Copy link

@rvedotrc rvedotrc commented Aug 8, 2024

A partial fix for #3938 (partial because this PR only addresses 3.1.1-dev). Note that I have also created draft PRs to port this to the other two -dev branches. I have no opinion on in which order they should be merged.


Thoughts as far as I can tell so far, as we still try to find some consensus.

Mostly we're talking about two cases: (1) security is not specified, and (2) security is specified but empty ([]); and for each of those two cases, obvious options include (a) define it as "allow all" aka "no security"; (b) define it as "deny all" aka "there is no way to authorize"; (c) state that the behaviour is undefined; (d) state that the behaviour is implementation-defined.

Case 1: Leaving security unspecified

  • (a) "allow all" aka "no security"
  • (b) "deny all" aka "no way in". I don't think anyone is suggesting this option.
  • (c) undefined (which would be a sign that security should actually be a required field). I don't think anyone is suggesting this option either.
  • (d) implementation-defined

Case 2: security is []

  • (a) "allow all" aka "no security". A perfectly reasonable possibility; albeit one which makes the empty list a special case, not following the usual rules of list logic.
  • (b) "deny all" aka "no way in". Also a perfectly reasonable possibility, and one where the list logic follows the usual rules. Under this option, security: [] is how you express "deny all", and security: [{}] is one way of expressing "allow all".
  • (c) undefined
  • (d) implementation-defined

Whichever one we opt for, could we also state that security: [] is deprecated? "Implementers of services SHOULD NOT introduce new instances of security: [], and SHOULD seek an alternative formulation.", for example?

@rvedotrc rvedotrc force-pushed the warn-about-ambiguity branch 3 times, most recently from 914c581 to 581ec03 Compare August 8, 2024 19:06
@handrews handrews added this to the v3.1.1 milestone Aug 8, 2024
@handrews handrews added security security: config The mechanics of severs and structure of security-related objects labels Aug 8, 2024
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ralfhandl ralfhandl left a comment

Choose a reason for hiding this comment

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

+1, minor style suggestions

versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
@rvedotrc rvedotrc marked this pull request as ready for review August 9, 2024 15:04
ralfhandl
ralfhandl previously approved these changes Aug 12, 2024
Copy link
Contributor

@ralfhandl ralfhandl left a comment

Choose a reason for hiding this comment

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

Seems to cover the current state of discussion, see also comments on #3995 (which needs to be adjusted once this is approved & merged)

rvedotrc added a commit to blaahaj/OpenAPI-Specification that referenced this pull request Aug 12, 2024
rvedotrc added a commit to blaahaj/OpenAPI-Specification that referenced this pull request Aug 12, 2024
rvedotrc added a commit to blaahaj/OpenAPI-Specification that referenced this pull request Aug 12, 2024
rvedotrc added a commit to blaahaj/OpenAPI-Specification that referenced this pull request Aug 12, 2024
ralfhandl
ralfhandl previously approved these changes Aug 12, 2024
rvedotrc added a commit to blaahaj/OpenAPI-Specification that referenced this pull request Aug 12, 2024
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
@handrews
Copy link
Member

My thoughts after today's TDC call:

  • The security field is really a list of authentication mechanisms, so using "allow all" / "deny all" doesn't quite fit because these are authorization-related terms (credit for this to @mikekistler and @rvedotrc, I'm just summarizing)
  • A lack of security means that you don't know anything about authentication, which means that you assume none is needed (but might find out otherwise when you try something)
  • An empty array (security: []) means that you are being told there are no authentication mechanisms. It is ambiguous as to whether this is "there are no authentication mechanisms because you don't need any" (which we had been calling "allow all") or "there are no authentication mechanisms because you're not even authorized to know about them, which means you can't use this" (which we had been calling "deny all")

Note that we have a whole section on "filtering" an OAD due to security constraints, and also a note that allows an empty Paths Object because a filtered OAD might filter out all of the Path Item Objects if the target audience is not allowed to see them.

So I think the right thing to do here is to acknowledge the ambiguities:

  • lack of security is basically a "^%$ around and find out" invitation
  • logically, security: [] ought to mean "you need to authenticate but you aren't allowed to know how, so you're out of luck"
  • practically, security: [] has been assumed to mean the same as security not being present, so we can acknowledge that ambiguity; this is partially because setting security: [] at the Operation Object level is said to "remove" the OpenAPI Object-level security, explicitly equating security: [] with the security field being missing
  • security: [{}] is an unambiguous way to say "this API (or operation) does not require authentication", so it is RECOMMENDED that OAD authors use that construct if that is what they want to say

I no longer think there is an "undefined" or "implementation-defined" thing here. Lack of security is "we're not telling you, just try it", and the "removes" wording in the Operation Object explicitly equates security: [] with that state, so we can't make it mean anything else.

@rvedotrc
Copy link
Author

explicitly equating security: [] with the security field being missing

Explicit in the sense that it's hard to argue that there's a different, better interpretation, yes :-)

I'll have a go at creating a wording to match what you just wrote @handrews

rvedotrc added a commit to blaahaj/OpenAPI-Specification that referenced this pull request Aug 23, 2024
@rvedotrc rvedotrc changed the title Clarify security; state that [] is undefined behaviour Clarify security; state what [] means Aug 23, 2024
@ralfhandl ralfhandl requested review from mikekistler and a team August 23, 2024 17:56
rvedotrc added a commit to blaahaj/OpenAPI-Specification that referenced this pull request Aug 25, 2024
rvedotrc added a commit to blaahaj/OpenAPI-Specification that referenced this pull request Aug 25, 2024
rvedotrc added a commit to blaahaj/OpenAPI-Specification that referenced this pull request Aug 25, 2024
Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

After some thought and investigation, I do not think any changes are needed to clarify the behavior of the security field in the current specifications. I've given an explanation for this in a comment on Issue 3938.

My recommendation is that we close this PR without merge.

@miqui
Copy link
Contributor

miqui commented Aug 25, 2024

After some thought and investigation, I do not think any changes are needed to clarify the behavior of the security field in the current specifications. I've given an explanation for this in a comment on Issue 3938.

My recommendation is that we close this PR without merge.

I am 100% in agreement with @mikekistler

@lornajane
Copy link
Contributor

TDC agrees that this is too much additional content, we only need a small clarification on what the empty array can mean, we will try to work on wording to answer #3938

@lornajane lornajane closed this Aug 29, 2024
@darrelmiller
Copy link
Member

Security: [] in an operation is the same as not defining a security property at the root of the document. It is saying "I have not specified in this API Description what the security requirements are".

This goes back to the open/closed system debate we had years ago. The absence of some description does not say anything, positive or negative. e.g. Just because I don't declare an operation can return a 503 doesn't mean it will not return a 503.

Just because I didn't tell you what security I mechanisms I use doesn't mean I don't have security on my API.

jeremyfiel added a commit to jeremyfiel/OpenAPI-Specification that referenced this pull request Sep 4, 2024
jeremyfiel added a commit to jeremyfiel/OpenAPI-Specification that referenced this pull request Sep 4, 2024
jeremyfiel added a commit to jeremyfiel/OpenAPI-Specification that referenced this pull request Sep 4, 2024
jeremyfiel added a commit to jeremyfiel/OpenAPI-Specification that referenced this pull request Sep 4, 2024
jeremyfiel added a commit to jeremyfiel/OpenAPI-Specification that referenced this pull request Sep 4, 2024
jeremyfiel added a commit to jeremyfiel/OpenAPI-Specification that referenced this pull request Sep 4, 2024
jeremyfiel added a commit to jeremyfiel/OpenAPI-Specification that referenced this pull request Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security: config The mechanics of severs and structure of security-related objects security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants