authentication/login-logout: initial enhancement #894
Conversation
s-urbaniak
force-pushed
the
login-logout
branch
from
cc3a9c4
to
a09df87
Compare
|
@stlaz addressed comments, where feasible, ptal. |
s-urbaniak
force-pushed
the
login-logout
branch
from
a09df87
to
84085b5
Compare
s-urbaniak
force-pushed
the
login-logout
branch
from
84085b5
to
e15dfc2
Compare
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
I hope I addressed all comments. PTAL @s-urbaniak , @stlaz , @sttts , @EmilyM1 |
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
|
/lgtm |
|
|
||
| - unit tests, | ||
| - e2e tests in origin and | ||
| - tests at must-gather. |
There was a problem hiding this comment.
Note to future devs: https://github.com/openshift/origin/blob/master/test/extended/cli/mustgather.go
| Ideally we test our feature with: | ||
|
|
||
| - unit tests, | ||
| - e2e tests in origin and |
There was a problem hiding this comment.
Note to future devs: https://github.com/openshift/origin/blob/master/test/extended/cluster/audit.go
ibihim
force-pushed
the
login-logout
branch
2 times, most recently
from
8ba51bb
to
c20bfe5
Compare
ibihim
force-pushed
the
login-logout
branch
2 times, most recently
from
f43861b
to
19f95b7
Compare
|
|
||
| ### API Extensions | ||
|
|
||
| We introduce a new property called `audit` to the `OAuth` CRD. |
There was a problem hiding this comment.
OAuth CRD -> oauth.config.openshift.io type
|
|
||
| ### Operational Aspects of API Extensions | ||
|
|
||
| There is no explicit operational involvement expected. The property is set by default and is optional. |
There was a problem hiding this comment.
The property is set to do what by default?
There was a problem hiding this comment.
Isn't this the goal of the kubebuilder command kubebuilder:default?
|
|
||
| There is no explicit operational involvement expected. The property is set by default and is optional. | ||
|
|
||
| It could be turned off by operational interaction with the API extension. |
There was a problem hiding this comment.
what's an "operational interaction"?
| #### Support Procedures | ||
|
|
||
| If the `audit` can't be set in the CR, support would need to file a bugzilla. | ||
| If there are no audit events for login, login failures or logout events, verify that the `audit` values is set in the `oauth` CR. |
There was a problem hiding this comment.
The audit property does not influence logging of logout events
There was a problem hiding this comment.
It doesn't? What is then the whole purpose of setting audit?
Will the audit property not tell the cluster-authentication-operator to enable audit logging on oauth-server?
There was a problem hiding this comment.
As far as I know, it only influences login events, and also logs some login failures. Hence the WriteLoginEvents value that you can configure for the profile.
There was a problem hiding this comment.
But isn't the goal to turn the audit logging on and then add the logging of the other events too?
So there will be a certain moment in time, before we add more logging events to the oauth-server, where we will only log a very limited set of events. But once we adjusted oauth-server, it will log login, login failures and logout... oh noes... you are right. That is true. The logout events are logged by extending what is being logged on logout and not in particular by the profile. So we have different technical paths to solve the goal.
Thanks a lot for the insight and thorough review :D This enhancement helped me to gain so much more clarity on that topic.
There was a problem hiding this comment.
@ibihim yes, logout events cannot be captured in oauth-server
There was a problem hiding this comment.
@ibihim as described in this OEP, logout events are captured only via deletion of oauthaccesstokens, subject auf audit policy configuration in oauth-apiserver.
|
|
||
| If the `audit` can't be set in the CR, support would need to file a bugzilla. | ||
| If there are no audit events for login, login failures or logout events, verify that the `audit` values is set in the `oauth` CR. | ||
| If the `audit` is set to `profile: "WriteLoginEvents"` and there are no audit events for the oauth-server after the login, login failure or logout events, support would need to file a bugzilla. |
There was a problem hiding this comment.
I am mentioning logout events:
or logout events
There was a problem hiding this comment.
Or do you mean that I should remove the logout entry?
| profile: WriteLoginEvents | ||
| ``` | ||
|
|
||
| 4. Create Login failures, Login and Logout events. |
There was a problem hiding this comment.
Shouldn't I mention Logout events?
There was a problem hiding this comment.
Created a dedicated entry for the logout events.
|
|
||
| ### Operational Aspects of API Extensions | ||
|
|
||
| There is no explicit operational involvement expected. The property is optional and set by default to log audit events. |
|
|
||
| There is no explicit operational involvement expected. The property is optional and set by default to log audit events. | ||
|
|
||
| It could be turned off by a user by removing API extension. |
There was a problem hiding this comment.
That would actually default back to it being turned on.
There was a problem hiding this comment.
Do you think so? If there is no audit in OAuthSpec?
There was a problem hiding this comment.
Let me be more specific to set it to None, just in case we default, without a spec given.
| ``` | ||
|
|
||
| 4. Create Login failures and Login. | ||
| 5. Execute `oc adm must-gather`. |
There was a problem hiding this comment.
No, not like that (ditto for all the must-gathers below).
There was a problem hiding this comment.
Whats wrong with must gather?
There was a problem hiding this comment.
You're proposing to create a privileged container in the cluster to pull data from the cluster to get a tar that you need to process instead of doing a streamed API call
There was a problem hiding this comment.
Sure, for automated things 👍 👍 😍
|
|
||
| #### E2E testing steps | ||
|
|
||
| In order to test the audit logging as mentioned in the porposal: |
Co-authored-by: Krzysztof Ostrowski <kostrows@redhat.com>
|
/lgtm |
|
/lgtm |
|
/lgtm |
|
The design looks architecturally sound. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mfojtik 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
No description provided.