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

enable service token role validation for all service tokens #266

Closed

Conversation

SeanMooney
Copy link
Contributor

As part of adressing CVE-2023-2088 cinder was modifed to
require the service role to be present in service token when
calling the attachemtn api to modify attachments related to nova
instance. One recomendation of that CVE mitigration discussions
was that all services shoudl enabel the service token role validation
by default. This change simplely enabled that by setting
[keystone_authtoken]/service_token_roles_required = true

Related: OSPRH191

As part of adressing CVE-2023-2088 cinder was modifed to
require the service role to be present in service token when
calling the attachemtn api to modify attachments related to nova
instance. One recomendation of that CVE mitigration discussions
was that all services shoudl enabel the service token role validation
by default. This change simplely enabled that by setting
[keystone_authtoken]/service_token_roles_required = true

Related: OSPRH191
@openshift-ci openshift-ci bot requested review from Akrog and olliewalsh September 15, 2023 13:26
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 15, 2023

@SeanMooney: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/precommit-check 6197bfa link true /test precommit-check
ci/prow/cinder-operator-build-deploy-tempest 6197bfa link false /test cinder-operator-build-deploy-tempest

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

# when enabled the service token user must have the service role to be considered valid.
# cinder already checks for this, explicitly in the case of the attchment API even when
# this is not enforced for all service token validation.
service_token_roles_required = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this needs a newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i guess i did not activate the venv when committing. i have precommet but it only runs if i have the venv activated
ill respin this shortly

Copy link
Contributor

Choose a reason for hiding this comment

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

We already added this in PR#257 and it can be seen here (without the CVE related comment):

service_token_roles_required = true

If I remember correctly the cinder CVE code doesn't really need that option, it has an additional internal check so it wouldn't have to depend on this config option. I enabled this option in my PR anyway because I like the keystone middleware being the one that gives the error..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh cool in that case ill abandon this pr

Copy link
Contributor

@Akrog Akrog left a comment

Choose a reason for hiding this comment

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

Thank you for keeping an eye on this, though we already have it. :-)

# when enabled the service token user must have the service role to be considered valid.
# cinder already checks for this, explicitly in the case of the attchment API even when
# this is not enforced for all service token validation.
service_token_roles_required = true
Copy link
Contributor

Choose a reason for hiding this comment

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

We already added this in PR#257 and it can be seen here (without the CVE related comment):

service_token_roles_required = true

If I remember correctly the cinder CVE code doesn't really need that option, it has an additional internal check so it wouldn't have to depend on this config option. I enabled this option in my PR anyway because I like the keystone middleware being the one that gives the error..

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 20, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SeanMooney
Once this PR has been reviewed and has the lgtm label, please ask for approval from asbishop. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@SeanMooney
Copy link
Contributor Author

fixed by #257

@SeanMooney SeanMooney closed this Sep 20, 2023
ASBishop pushed a commit to ASBishop/cinder-operator that referenced this pull request Mar 11, 2024
…rators/renovate/openstack-k8s-operators

Update openstack-k8s-operators
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants