Skip to content

Key-Manager Standard - #599

Merged
josephineSei merged 35 commits into
mainfrom
key-manager-standard
Aug 26, 2024
Merged

Key-Manager Standard#599
josephineSei merged 35 commits into
mainfrom
key-manager-standard

Conversation

@josephineSei

Copy link
Copy Markdown
Contributor

closes #509

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
This test should be added to the mandatory service test as soon as a Key-Manager MUST be present in csc-conformant deployments.

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
@josephineSei

Copy link
Copy Markdown
Contributor Author

If we decide to move Barbican from Supported to Mandatory in the lists of mandatory and supported openstack services, we may not need an extra test script here, but should move it into the other test-file.

@artificial-intelligence artificial-intelligence left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This standard touches on a topic which comes up time and time again: Should we standardize something very implementation specific?

While the standard doesn't mandate anything in it's current form I could easily see this change in the future, because of the focus on Barbican here.

I'm conflicted if this is a problem or rather a feature though.

Still, even if currently no CSPs implement a Key-Manager I would like to hear opinions of other CSPs if they will implement this, if the standard doesn't really enforce anything.

Personally I don't see much value in a standard that doesn't enforce anything. For a draft and further discussion it is of course fine.
We need to start somewhere, after all.

Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
josephineSei and others added 3 commits June 6, 2024 11:35
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Co-authored-by: Sven <kieske@osism.tech>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

## Manual Tests

There need to be a manual test that searches the physical host of the Key-Manager host for the present of a Master-KEK in Plain-Text, e.g. in the Barbican config file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is unclear what would be the purpose of this test. When simple_crypto_plugin is used and deployed in a protected environment it is okay to have the secret in plain text. In the HSM case you will not find it. But what do you want to verify here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is possible, to run certain parts of Barbicna in an enclave that also holds the complete config file. In that case, the Master KEK will not be found.

So one option as a manual test would be to search for a plain text config file and check, whether simple_crypto is used as a plugin and the MasterKEK is written into the config.

Maybe in future there are other ways to secure the MasterKEK even within the simple_crypto plugin. We should not make assumptions about that part of configuration of this plugin here, other than that we want to encourage CSPs to protect the MasterKEK.

Comment thread Standards/scs-XXXX-wN-key-manager-implementation-testing.md Outdated
Comment thread Standards/scs-XXXX-wN-key-manager-implementation-testing.md Outdated
Comment thread Standards/scs-XXXX-wN-key-manager-implementation-testing.md Outdated
Comment thread Tests/iaas/key-manager/check-for-key-manager.py Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Tests/iaas/key-manager/check-for-key-manager.py Outdated
adjusted the phrasing, as gtema said.

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

@artificial-intelligence artificial-intelligence left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some spelling stuff.

Also there are different spellings of "key-manager", "Key Manager" and "Key-Manager" throughout the text. I don't know if this is a category name in this context, but I guess it should be spelled consistently for easier grep-ability 😉

Contentwise this looks mostly good to me, I still would like to have more opinions of different CSPs in here, but I guess I can't force people to comment.

Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-wN-key-manager-implementation-testing.md Outdated
Comment thread Standards/scs-XXXX-wN-key-manager-implementation-testing.md Outdated
josephineSei and others added 3 commits June 18, 2024 09:15
Co-authored-by: Sven <kieske@osism.tech>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

@artificial-intelligence artificial-intelligence left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@josephineSei

Copy link
Copy Markdown
Contributor Author

@gtema could you please review this standard again and check, whether all your points were addressed?

@gtema gtema left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@josephineSei, sadly I still have issues with the document "language". It is very hard to read the document and some sentences need to be re-read few times. This should not be the case with standards.

Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-vN-key-manager-standard.md Outdated
Comment thread Standards/scs-XXXX-wN-key-manager-implementation-testing.md Outdated
Comment thread Standards/scs-XXXX-wN-key-manager-implementation-testing.md Outdated

## Manual Tests

There needs to be a manual test that searches the physical host of the Key Manager host for the presence of a Master-KEK in Plain-Text, e.g. in the Barbican config file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Depending on the used plugin the config file may look very differently. One possibility is that the plugin invokes command that returns the key, but the configuration of the command itself contains the key in plain text (or base64).
I think it makes not much sense to state that there must be a test to check presense of the plain text master key. Instead I would explicitly state that it is out of scope and is in the hands of the CSP to ensure master kek is protected correspondingly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think an audit is needed somehow, but how about an audit of the architecture?

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
"name or pass it via --os-cloud"
)

return check_presence_of_key_manager(cloud)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now that we include the requirement of giving members access to the Key Manager API if present, we should move the corresponding test from the Roles Standard over to here:

def check_key_manager_permissions(conn: openstack.connection.Connection
) -> None:
"""
Limits the authentication to the "member" role using an application
credentials restricted to that role and verifies that the member role
has sufficient access to the Key Manager API functionality.
"""
secret_name = "scs-member-role-test-secret"
member_conn = reconnect_with_role(conn, "member")
def _find_secret(secret_name_or_id: str):
"""Replacement method for finding secrets.
Mimicks the behavior of Connection.key_manager.find_secret()
but fixes an issue with the internal implementation raising an
exception due to an unexpected microversion parameter.
"""
secrets = member_conn.key_manager.secrets()
for s in secrets:
if s.name == secret_name_or_id or s.id == secret_name_or_id:
return s
return None
try:
existing_secret = _find_secret(secret_name)
if existing_secret:
member_conn.key_manager.delete_secret(existing_secret)
member_conn.key_manager.create_secret(
name=secret_name,
payload_content_type="text/plain",
secret_type="opaque",
payload="foo"
)
new_secret = _find_secret(secret_name)
assert new_secret, (
f"Secret created with name '{secret_name}' was not discoverable by "
f"the user"
)
member_conn.key_manager.delete_secret(new_secret)
except openstack.exceptions.ForbiddenException as e:
print(
"Users of the 'member' role can use Key Manager API: FAIL"
)
print(
f"ERROR: {str(e)}"
)
exit(1)
finally:
delete_application_credential(conn, APP_CREDENTIAL_NAME)
print(
"Users of the 'member' role can use Key Manager API: PASS"
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I started integrating the code until I stumbled about this:

keystoneauth1.exceptions.http.Unauthorized: Error authenticating with application credential: Application credentials cannot request a scope. (HTTP 401) (Request-ID: req-afca1cf7-0c4f-49d9-b716-44c7bb4f6f72)

Now we are investigating, whether there are other ways to ensure that a logged in user only has the member role within the specified project.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I started integrating the code until I stumbled about this:

keystoneauth1.exceptions.http.Unauthorized: Error authenticating with application credential: Application credentials cannot request a scope. (HTTP 401) (Request-ID: req-afca1cf7-0c4f-49d9-b716-44c7bb4f6f72)

Now we are investigating, whether there are other ways to ensure that a logged in user only has the member role within the specified project.

@josephineSei mhm, weird, I didn't test it myself, but this should work?

see: https://opendev.org/openstack/keystone/commit/52da4d0e129048d1b808bdde07364cde698cf475

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have a devstack that already partly includes the new secure RBAC roles. They are enabled in Keystone (and Nova and Glance I think), but not in Barbican. Maybe that is the reason for this behavior. I can take another look into this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I now check the roles of the current user, when there is an admin or manager role present (or any custom role), the check for the creation of a secret will be skipped and a warning logged.

@josephineSei

Copy link
Copy Markdown
Contributor Author

Testing whether a user with the member role is allowed to interact with Barbican (That is, that either new SRBAC is used or the policy is changed to integrate the member role) is quite difficult:
In old deployments, or deployments where all services are using the old RBAC policies it is possible to reconnect only with the member role and check if the user is allowed to list secrets.
In nowadays and future deployments, this will not work anymore.
OpenStack does not offer any API to see what roles a user itself has, nor does it allow to only interact as having one specific role.

So we need to put the burden of a correct configuration into the CSPs hand: We need a user who only has the member role in the project, that is specified when executing the test.

If a CSP also gives out a reader role or a creator role, this will lead to a false positive: The test will succeed, but the standard may not be fulfilled.

We should document this behavior. But when the user-role-project triple is configured correctly, we can test the policy.

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
@artificial-intelligence

Copy link
Copy Markdown
Contributor

Testing whether a user with the member role is allowed to interact with Barbican (That is, that either new SRBAC is used or the policy is changed to integrate the member role) is quite difficult: In old deployments, or deployments where all services are using the old RBAC policies it is possible to reconnect only with the member role and check if the user is allowed to list secrets. In nowadays and future deployments, this will not work anymore. OpenStack does not offer any API to see what roles a user itself has, nor does it allow to only interact as having one specific role. [...]

can't we simply test this by e.g. calling a known admin only api endpoint and expecting it to fail? this isn't sufficient of course (this API could be disabled even for admins via a special crafted policy I guess). But for "normal" deployments, we should receive a HTTP 403 forbidden error code.

It's not bulletproof, but maybe better than nothing?

@gtema

gtema commented Jul 10, 2024

Copy link
Copy Markdown
Contributor

Testing whether a user with the member role is allowed to interact with Barbican (That is, that either new SRBAC is used or the policy is changed to integrate the member role) is quite difficult: In old deployments, or deployments where all services are using the old RBAC policies it is possible to reconnect only with the member role and check if the user is allowed to list secrets. In nowadays and future deployments, this will not work anymore. OpenStack does not offer any API to see what roles a user itself has, nor does it allow to only interact as having one specific role.

??? I do not get it. When you authorize you get back a token response. It contains all roles user have in the current scope (and for that you do not need to do any other call). In addition to that there is https://docs.openstack.org/api-ref/identity/v3/index.html#id627 but that requires you to have privileges for that (admin)

@josephineSei

Copy link
Copy Markdown
Contributor Author

In the sdk documentation I found this:

validate_user_has_project_role(project, user, role)

    Validates that a user has a role on a project

    Parameters:

            project – Either the ID of a project or a [Project](https://docs.openstack.org/openstacksdk/latest/user/resources/identity/v3/project.html#openstack.identity.v3.project.Project) instance.

            user – Either the ID of a user or a [User](https://docs.openstack.org/openstacksdk/latest/user/resources/identity/v3/user.html#openstack.identity.v3.user.User) instance.

            role – Either the ID of a role or a [Role](https://docs.openstack.org/openstacksdk/latest/user/resources/identity/v3/role.html#openstack.identity.v3.role.Role) instance.

Which would be really nice to use. But I need a role id (which does not seem to work - I have the member role but get a "False" from this method, when providing the id of the member role). Even if it would work, as a simple user without the admin role I am not able to list roles and their ids.

Additionally in the OpenStackSDK, the connection has the properties: current_project_id and current_user_id - but nothing about the roles, that are inherited.

@gtema

gtema commented Jul 15, 2024

Copy link
Copy Markdown
Contributor

Additionally in the OpenStackSDK, the connection has the properties: current_project_id and current_user_id - but nothing about the roles, that are inherited.

The token contains list of all roles (inherited roles are resolved and thus explicitly included). But there is a bug in Keystone (https://bugs.launchpad.net/keystone/+bug/2030061) for which fix was finally decided and merged on Friday where application credentials created with/without explicit specifying the AppCred roles are having this resolving not working properly (AppCreds created with i.e. requesting "member" role were not getting "reader" role included). This is not relevant here directly, but may be important to know when you use AppCreds for checks.

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

@bitkeks bitkeks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm.

very Barbican-centrific, but that's another topic.
"Simple Crypto Plugin" should be forbidden in case we make the Key Manager mandatory (and Barbican is the reference)

@artificial-intelligence artificial-intelligence left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one of the tests seems at least unclear or at worst incomplete to me. see the comment for details.

Comment thread Tests/iaas/key-manager/check-for-key-manager.py
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
@josephineSei

Copy link
Copy Markdown
Contributor Author

I looked of the standard today and adjusted some spelling mistakes left.
@gtema and @artificial-intelligence could you please check if there are still open points from your side? Thank you

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
@josephineSei

Copy link
Copy Markdown
Contributor Author

I asked today in the IaaS call for reviews from CSPs - because they are encouraged to integrate a key-manager sooner or later. I will leave this PR open until the end of this week, so feedback can still be written here.

…-standard.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
…o scs-0116-w1-key-manager-implementation-testing.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>
@josephineSei
josephineSei merged commit 4caa5f9 into main Aug 26, 2024
@josephineSei
josephineSei deleted the key-manager-standard branch August 26, 2024 07:49
OgarOgarovic pushed a commit that referenced this pull request Aug 27, 2024
* Create scs-XXXX-vN-key-manager-standard.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update scs-XXXX-vN-key-manager-standard.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Create check-for-key-manager.py

This test should be added to the mandatory service test as soon as a Key-Manager MUST be present in csc-conformant deployments.

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update scs-XXXX-vN-key-manager-standard.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update scs-XXXX-vN-key-manager-standard.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Sven <kieske@osism.tech>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Create scs-XXXX-wN-key-manager-implementation-testing.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update scs-XXXX-vN-key-manager-standard.md

adjusted the phrasing, as gtema said.

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update scs-XXXX-wN-key-manager-implementation-testing.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update check-for-key-manager.py

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Sven <kieske@osism.tech>
Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* unify all times "Key Manager" is written

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update scs-XXXX-wN-key-manager-implementation-testing.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Rephrasing many sentences for a better understanding.

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Rephrasing

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Add section for the Key Manager Policies

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update scs-XXXX-vN-key-manager-standard.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* add policies to the implementation notes

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update scs-XXXX-vN-key-manager-standard.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update scs-XXXX-wN-key-manager-implementation-testing.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update check-for-key-manager.py

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update check-for-key-manager.py

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update check-for-key-manager.py

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update check-for-key-manager.py

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update check-for-key-manager.py

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update check-for-key-manager.py

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Adjust Spelling and some phrases.

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update scs-XXXX-wN-key-manager-implementation-testing.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Rename scs-XXXX-vN-key-manager-standard.md to scs-0116-v1-key-manager-standard.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

* Update and rename scs-XXXX-wN-key-manager-implementation-testing.md to scs-0116-w1-key-manager-implementation-testing.md

Signed-off-by: josephineSei <128813814+josephineSei@users.noreply.github.com>

---------

Signed-off-by: Josephine Seifert <josephine.seifert@cloudandheat.com>
Co-authored-by: Sven <kieske@osism.tech>
Co-authored-by: Markus Hentsch <markus.hentsch@cloudandheat.com>
Signed-off-by: Ivan Vnučko <ivan@vnucko.com>
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.

Standardize List of allowed Barbican Plugins

6 participants