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.

Sign up for GitHub

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

Jump to bottom

OSDOCS-2890: Updating for OAuth server audit logging #45799

Merged
merged 1 commit into from Jul 18, 2022
Merged

OSDOCS-2890: Updating for OAuth server audit logging #45799

merged 1 commit into from Jul 18, 2022

Conversation

bergerhoffer
Copy link
Contributor

@bergerhoffer bergerhoffer commented May 18, 2022 •
edited

@bergerhoffer bergerhoffer changed the title OSDOCS-2890: Updating for OAuth server audit logging [WIP] OSDOCS-2890: Updating for OAuth server audit logging May 18, 2022
@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 18, 2022
@netlify
Copy link

netlify bot commented May 18, 2022 •
edited

✅ Deploy Preview for osdocs ready!

Name Link
🔨 Latest commit 696b567af063be6732d3e33c1f3038beadb919cd
🔍 Latest deploy log https://app.netlify.com/sites/osdocs/deploys/62853fbe5feed600088dfe5f
😎 Deploy Preview https://deploy-preview-45799--osdocs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bergerhoffer bergerhoffer added peer-review-needed Signifies that the peer review team needs to review this PR branch/enterprise-4.11 labels May 18, 2022
@bergerhoffer bergerhoffer added this to the Future Release milestone May 18, 2022
@bergerhoffer bergerhoffer changed the title [WIP] OSDOCS-2890: Updating for OAuth server audit logging OSDOCS-2890: Updating for OAuth server audit logging May 18, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2022
@bergerhoffer
Copy link
Contributor Author

@ibihim Can you please review these doc updates and let me know if you have any feedback?

Previews:

CC @stlaz too

jeana-redhat
jeana-redhat approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@jeana-redhat jeana-redhat left a comment

Choose a reason for hiding this comment

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

LGTM

@jeana-redhat jeana-redhat added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels May 18, 2022
@bergerhoffer
Copy link
Contributor Author

@ibihim I made a few updates per the feedback. Can you please re-review?

Updated preview link is now here: http://file.rdu.redhat.com/~ahoffer/2022/OSDOCS-2890/security/audit-log-view.html#nodes-nodes-audit-log-basic-viewing_audit-log-view

@bergerhoffer
Copy link
Contributor Author

bergerhoffer commented Jul 11, 2022 •
edited

@xingxingxia Can you please QE review when you get a chance?

Preview:

@xingxingxia
Copy link
Contributor

xingxingxia commented Jul 12, 2022 •
edited

In "Gathering audit logs" section (not shown in this PR "Files changed"; happened to find it in your preview link)

Run the oc adm must-gather command with the -- /usr/bin/gather_audit_logs flag

/usr/bin/gather_audit_logs is not a "flag". It is a command, this can seen from oc adm must-gather -h:

$ oc adm must-gather -h
  # Gather information using a specific image, command, and pod-dir
  oc adm must-gather --image=my/image:tag --source-dir=/pod/directory -- myspecial-command.sh

@xingxingxia
Copy link
Contributor

Need we somewhere mention "authentication.openshift.io/decision" can be one of allow, deny, error?

@bergerhoffer
Copy link
Contributor Author

In "Gathering audit logs" section (not shown in this PR "Files changed"; happened to find it in your preview link)

Run the oc adm must-gather command with the -- /usr/bin/gather_audit_logs flag

/usr/bin/gather_audit_logs is not a "flag". It is a command, this can seen from oc adm must-gather -h:

$ oc adm must-gather -h
  # Gather information using a specific image, command, and pod-dir
  oc adm must-gather --image=my/image:tag --source-dir=/pod/directory -- myspecial-command.sh

Thanks @xingxingxia - I'm actually not familiar with this file so I'll track it down and will update to use the correct phrasing.

@bergerhoffer
Copy link
Contributor Author

Need we somewhere mention "authentication.openshift.io/decision" can be one of allow, deny, error?

@xingxingxia I don't really see anywhere else where we really get into the possible values for fields of an audit event, outside of generically mentioning the fields in this table: http://file.rdu.redhat.com/~ahoffer/2022/OSDOCS-2890/security/audit-log-view.html#nodes-pods-audit-log-basic_audit-log-view.

So I don't think so, but let me know if you feel strongly about it and we can look into it more.

@bergerhoffer
Copy link
Contributor Author

@xingxingxia Updated per your feedback, can you please take another look? Thanks!

@xingxingxia
Copy link
Contributor

xingxingxia commented Jul 13, 2022 •
edited by openshift-ci bot

AUTH-6 OEP https://github.com/openshift/enhancements/blob/master/enhancements/authentication/login-logout-events.md#proposal uses quite length to tell allow, deny or error. For AUTH-6, this is a feature point specific and non-trivial. So I prefer to mention allow, deny or error if we documen AUTH-6.

@bergerhoffer
Copy link
Contributor Author

AUTH-6 OEP https://github.com/openshift/enhancements/blob/master/enhancements/authentication/login-logout-events.md#proposal uses quite length to tell allow, deny or error. For AUTH-6, this is a feature point specific and non-trivial. So I prefer to mention allow, deny or error if we documen AUTH-6.

@xingxingxia Sure thing then! I mentioned this in the part about viewing the audit logs. Can you take a look and let me know if this works? Thanks!

http://file.rdu.redhat.com/~ahoffer/2022/OSDOCS-2890/security/audit-log-view.html#nodes-nodes-audit-log-basic-viewing_audit-log-view

@xingxingxia
Copy link
Contributor

LGTM, but the preview link https://deploy-preview-45799--osdocs.netlify.app/openshift-enterprise/latest/security/audit-log-view.html#nodes-nodes-audit-log-basic-viewing_audit-log-view page does not show "The possible values for the authentication.openshift.io/decision annotation are allow, deny, or error.", what is wrong?

@bergerhoffer
Copy link
Contributor Author

bergerhoffer commented Jul 17, 2022 •
edited

LGTM, but the preview link https://deploy-preview-45799--osdocs.netlify.app/openshift-enterprise/latest/security/audit-log-view.html#nodes-nodes-audit-log-basic-viewing_audit-log-view page does not show "The possible values for the authentication.openshift.io/decision annotation are allow, deny, or error.", what is wrong?

@xingxingxia Apologies - our automatic preview system has not been functioning for a month or two, so we've been having to upload manual previews instead. As long as you see the proper text on the latest preview link I shared (http://file.rdu.redhat.com/~ahoffer/2022/OSDOCS-2890/security/audit-log-view.html#nodes-nodes-audit-log-basic-viewing_audit-log-view), we should be good to go!

@xingxingxia
Copy link
Contributor

Ah, thanks for the explanation!
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2022
@bergerhoffer bergerhoffer merged commit 3a962f8 into openshift:main Jul 18, 2022
@bergerhoffer
Copy link
Contributor Author

/cherrypick enterprise-4.11

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jul 18, 2022
Merged
@openshift-cherrypick-robot
Copy link

@bergerhoffer: new pull request created: #47896

In response to this:

/cherrypick enterprise-4.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@bergerhoffer bergerhoffer deleted the OSDOCS-2890 branch August 3, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibihim ibihim ibihim left review comments

@xingxingxia xingxingxia xingxingxia left review comments

@jeana-redhat jeana-redhat jeana-redhat approved these changes

Assignees

@xingxingxia xingxingxia

Labels
branch/enterprise-4.11 lgtm Indicates that a PR is ready to be merged. peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
OCP 4.11 GA
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bergerhoffer @xingxingxia @openshift-cherrypick-robot @ibihim @jeana-redhat @kalexand-rh