add audit spec for password authenticator flow #968
Conversation
| In case that the authentication happens through the `oauth-server`, we suggest to add: | ||
|
|
||
| - `authentication.openshift.io/username`, which is the username for the authentication attempt. | ||
| - `authentication.openshift.io/authn-method`, which mentions the OAuth2 authentication method in use. |
There was a problem hiding this comment.
This can be implied based on the request URL, no?
Why is this important?
Should it be oauth-method instead of authn-method, then?
There was a problem hiding this comment.
Yeah, this is really a thing that makes me wonder:
- UX for "just reading the annotations" during a "wake up call in the night"
vs - redundant information that could (Murphy's Law) contradict itself.
The method and the decision can be deduced from the other values.
There was a problem hiding this comment.
I didn't want say "oauth-method" as I would rather think in terms of implicit grant or code grant when thinking of oauth-methods. So as we pick "just" the authentication method, I went for authn.
There was a problem hiding this comment.
I think that for now we can just go with username and decision to keep things minimal. We can add more later if there's such a request
| - `authentication.openshift.io/decision`, which is an enum that can be `allow` or `deny`. | ||
| - `authentication.openshift.io/reason`, which defines a reason in case that the `authentication.openshift.io/decision` is set to `deny`. In case that it is `allow`, the value is an empty `string`. |
There was a problem hiding this comment.
Aren't these two going to be redundant to the response status and message?
There was a problem hiding this comment.
More or less... the reason might have more context.
| In case that the authentication happens through the `oauth-server`, we suggest to add: | ||
|
|
||
| - `authentication.openshift.io/username`, which is the username for the authentication attempt. | ||
| - `authentication.openshift.io/authn-method`, which mentions the OAuth2 authentication method in use. |
There was a problem hiding this comment.
I think that for now we can just go with username and decision to keep things minimal. We can add more later if there's such a request
Signed-off-by: Krzysztof Ostrowski <kostrows@redhat.com>
ibihim
force-pushed
the
AUTH-6-Add_Audit_Def
branch
from
af214a2
to
744c92e
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: s-urbaniak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
What
Adding specification for
auditevents in the case that the authentication happens on theoauth-server.Why
We didn't specify how the
auditevent should look like and I would like to take the opportunity to discuss this with the team.Note
It doesn't yet contain the specification for IdentityProvider cases.
@s-urbaniak, @stlaz, @slaskawi, @EmilyM1