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

Bug 1878648: oauth-apiserver: audit log oauthaccesstokens if fresh 4.6 install #324

Merged
merged 1 commit into from Sep 22, 2020
Merged

Bug 1878648: oauth-apiserver: audit log oauthaccesstokens if fresh 4.6 install #324

merged 1 commit into from Sep 22, 2020

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Aug 21, 2020 •
edited

The annotation oauth-apiserver.openshift.io/secure-token-storage: true set on the config/v1 apiserver object make oauth-apiserver deeply log oauth tokens, noth access and authorize.

This requires that there are no non-sha256 tokens in the system. For 4.6 installs this is guaranteed.

After downgrade to 4.5 there is a controller removing the annotation.

After upgrade from 4.5, the annotation is not set. The admin can set it manually after having made sure that no non-sha256 tokens exist.

For upgrade to 4.7, non-sha256 tokens must not exist. If they do, the upgrade will be blocked by the auth operator (to be implemented).

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2020
@sttts
Copy link
Contributor Author

sttts commented Aug 21, 2020

CI is down

/retest

sttts
sttts commented Aug 21, 2020
View reviewed changes
manifests/02_config.cr.yaml Outdated
@@ -4,5 +4,7 @@ metadata:
name: cluster
annotations:
release.openshift.io/create-only: "true"
# this flag is not set for a cluster coming from 4.5 via upgrade. Hence, 4.5 clusters will keep supporting non-sha256 tokens.
oauth-apiserver.openshift.io/secure-token-storage: "true"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if downgrade from fresh 4.6 cluster is possible. We have to add removal of this annotation to the 4.5 operator.

Copy link

Choose a reason for hiding this comment

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

Is 'fresh 4.6 install' a must requirement? Can we configure 'oauth-apiserver.openshift.io/secure-token-storage: "true"' manually after upgrading OCP4.5 to OCP4.6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you can guarantee that you don't have old oauth tokens (those without sha256- prefix) in the system, yes you can technically set this annotation.

Copy link

Choose a reason for hiding this comment

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

Thanks for your comment. I will try to test this patch and feed back later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how is it going?

Copy link

Choose a reason for hiding this comment

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

Thanks. The patch works well. I confirmed we can monitor events of creating/deleting oauthaccesstokens. So we think we can monitor login/logout events using this feature. But we are testing this patch now and it will take some time to complete. Please wait a moment for further feedback.

Copy link

Choose a reason for hiding this comment

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

We don't have additional comments. This patch works fine as we expected. Thanks.

@sttts
Copy link
Contributor Author

sttts commented Aug 25, 2020

/retest

@sttts sttts changed the title Proof-of-concept: oauth-apiserver: audit log oauthaccesstokens if fresh 4.6 install Bug 1878648: oauth-apiserver: audit log oauthaccesstokens if fresh 4.6 install Sep 15, 2020
@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Sep 15, 2020
@openshift-ci-robot
Copy link
Contributor

@sttts: This pull request references Bugzilla bug 1878648, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1878648: oauth-apiserver: audit log oauthaccesstokens if fresh 4.6 install

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.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 15, 2020
@openshift-ci-robot openshift-ci-robot mentioned this pull request Sep 15, 2020
Merged
@sttts sttts force-pushed the sttts-secure-access-token-audit-logs branch 3 times, most recently from 61ccfc0 to 8da31ae Compare September 15, 2020 12:22
@sttts sttts mentioned this pull request Sep 15, 2020
Merged
@sttts sttts force-pushed the sttts-secure-access-token-audit-logs branch from 8da31ae to c736aa0 Compare September 15, 2020 12:40
@sttts sttts mentioned this pull request Sep 15, 2020
Merged
p0lyn0mial
p0lyn0mial reviewed Sep 15, 2020
View reviewed changes
pkg/operator/starter.go
return err
}

// TODO(4.7): switch over to secure access-token logging by default and delete old non-sha256 tokens
Copy link
Contributor

Choose a reason for hiding this comment

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

who is going to delete old non-sha256 tokens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will block upgrade. To be done.

@sttts sttts mentioned this pull request Sep 15, 2020
Merged
@sttts sttts force-pushed the sttts-secure-access-token-audit-logs branch from c736aa0 to 969c134 Compare September 16, 2020 07:58
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2020
@sttts sttts force-pushed the sttts-secure-access-token-audit-logs branch from 969c134 to f2cbbcf Compare September 16, 2020 08:02
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2020
@sttts
Copy link
Contributor Author

sttts commented Sep 16, 2020

/retest

@openshift-ci-robot
Copy link
Contributor

@sttts: This pull request references Bugzilla bug 1878648, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1878648: oauth-apiserver: audit log oauthaccesstokens if fresh 4.6 install

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.

@sttts
Copy link
Contributor Author

sttts commented Sep 16, 2020

/retest

2 similar comments
@sttts
Copy link
Contributor Author

sttts commented Sep 17, 2020

/retest

@sttts
Copy link
Contributor Author

sttts commented Sep 17, 2020

/retest

@sttts
Copy link
Contributor Author

sttts commented Sep 18, 2020

/retest

@sttts
Copy link
Contributor Author

sttts commented Sep 18, 2020

/retest

GCP errors.

@sttts
Copy link
Contributor Author

sttts commented Sep 21, 2020

Seeing --audit-policy-file=/var/run/configmaps/audit/secure-oauth-storage-default.yaml in the oauth-apiserver deployment.

/hold cancel

@sttts
Copy link
Contributor Author

sttts commented Sep 21, 2020

/retest

1 similar comment
@sttts
Copy link
Contributor Author

sttts commented Sep 21, 2020

/retest

@tkashem
Copy link
Contributor

tkashem commented Sep 21, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts, tkashem

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

17 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 45200c8 into openshift:master Sep 22, 2020
@openshift-ci-robot
Copy link
Contributor

@sttts: All pull requests linked via external trackers have merged:

Bugzilla bug 1878648 has been moved to the MODIFIED state.

In response to this:

Bug 1878648: oauth-apiserver: audit log oauthaccesstokens if fresh 4.6 install

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.

@sttts sttts mentioned this pull request Mar 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p0lyn0mial p0lyn0mial p0lyn0mial left review comments

@k-keiichi-rh k-keiichi-rh k-keiichi-rh left review comments

@deads2k deads2k Awaiting requested review from deads2k

@stlaz stlaz Awaiting requested review from stlaz

Assignees

@tkashem tkashem

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@sttts @openshift-ci-robot @tkashem @openshift-bot @p0lyn0mial @k-keiichi-rh @openshift-merge-robot