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

feat: use external kms key #131

Merged
merged 65 commits into from
Nov 8, 2024
Merged

feat: use external kms key #131

merged 65 commits into from
Nov 8, 2024

Conversation

kierramarie
Copy link
Contributor

Description

An external kms key can be used now. If an api key for the external account is passed, new iam policies will be created for COS to communicate with the external kms instance.

Git Issue: #107

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

/run pipeline

@ocofaigh
Copy link
Member

@kierramarie ensure changes (including variable descriptions) are consistent with terraform-ibm-modules/terraform-ibm-secrets-manager#147

@kierramarie
Copy link
Contributor Author

@ocofaigh this da uses kms for en and cos. should I update the cos part as well to use the external kms (I have already mostly implemented this)?

@ocofaigh
Copy link
Member

ocofaigh commented Jul 1, 2024

@kierramarie I don't get you - EN is not deployed in this DA? This is the SCC DA, so KMS key is only used to encrypt the COS bucket used by SCC

@kierramarie
Copy link
Contributor Author

sorry wrong PR 🤦‍♀️

@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

It appears that the upgrade test is failing because the iam policy is being changed. Is it okay to skip upgrade test?

@kierramarie
Copy link
Contributor Author

/run pipeline

@akocbek
Copy link
Contributor

akocbek commented Jul 3, 2024

@kierramarie why iam policy has been changed? do we know the reason? I guess, if we are not creating cross account policy, then settings for previous iam policy shouldn't change

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

see comments

@kierramarie
Copy link
Contributor Author

kierramarie commented Jul 3, 2024

@akocbek here is what I get when I run the upgrade test:

        	Test:       	TestRunUpgradeInstances
        	Messages:   	Resource(s) identified to be updated 
        	            	Name: policy
        	            	Address: module.cos[0].module.buckets.ibm_iam_authorization_policy.policy[0]
        	            	Actions: [update]
        	            	DIFF:
        	            	  Before: 
        	            		{}
        	            	  After: 
        	            		{}
        	            	
        	            	Change Detail:
        	            	{
        	            	  "actions": [
        	            	    "update"
        	            	  ],
        	            	  "after": {
        	            	    "description": "SECURE_VALUE_HIDDEN_HASH:-09fb55c072157e4ce445bf5fe16b664da39bd068027f4fc931f10a3f",
        	            	    "id": "55d07cd4-dd7a-4a5e-8eb7-d66d61c2b901",
        	            	    "resource_attributes": "SECURE_VALUE_HIDDEN_HASH:-3e1d813729df0a021667706febdcd7a7f98124a3b57504bcd816d767",
        	            	    "roles": "SECURE_VALUE_HIDDEN_HASH:-146edc4d125eee639e7b2fe54f8383e87581ec7ce1e96f015593f0e9",
        	            	    "source_resource_group_id": "",
        	            	    "source_resource_instance_id": "SECURE_VALUE_HIDDEN_HASH:-f017474dd062e5b907ad9905dd613aa0470cb62440fac5a98668e062",
        	            	    "source_resource_type": "",
        	            	    "source_service_account": "abac0df06b644a9cabc6e44f55b3880e",
        	            	    "source_service_name": "cloud-object-storage",
        	            	    "subject_attributes": "SECURE_VALUE_HIDDEN_HASH:-e8ee7542de3374f851ac974d42106aa6071443c888843351d421031a",
        	            	    "target_resource_group_id": "",
        	            	    "target_resource_instance_id": "SECURE_VALUE_HIDDEN_HASH:-1a38c97b5eeda8e6d0a9f1f9dea2eeadfbc13591f7253905aa260261",
        	            	    "target_resource_type": "",
        	            	    "target_service_name": "SECURE_VALUE_HIDDEN_HASH:-4124bfb20e5fe5a42f2fe15d06511d2983eb22432add9b34b9100455",
        	            	    "transaction_id": "30c7252dd2dd47fd95539f093c21b850",
        	            	    "version": null
        	            	  },
        	            	  "after_sensitive": {
        	            	    "description": "SECURE_VALUE_HIDDEN_HASH:-e81a5c43627617dcfaa30eb0ce9b7edef70ec15a89d8ef8e48e5169c",
        	            	    "resource_attributes": "SECURE_VALUE_HIDDEN_HASH:-80fba17a3c5b89dca881a72b334d5c21c13a35850708869c6525ef3b",
        	            	    "roles": "SECURE_VALUE_HIDDEN_HASH:-586b907cd5391dcac6cd3e60c57743f8b51eb548fe98e2b1223beeec",
        	            	    "source_resource_instance_id": "SECURE_VALUE_HIDDEN_HASH:-1139510326c6f8e2f492ea351ab99cda94e2c3c21fb1fe1a7bcdd930",
        	            	    "subject_attributes": "SECURE_VALUE_HIDDEN_HASH:-48eecc2d75e7d0d9919290d5b0c5ea3cd523e90c0f13b90a2584435d",
        	            	    "target_resource_instance_id": "SECURE_VALUE_HIDDEN_HASH:-d62341dcc03ffb1409bb39d039fb977c1a313356318319989abea4a6",
        	            	    "target_service_name": "SECURE_VALUE_HIDDEN_HASH:-3cfb03ac2394c4854fd9d235c58a61795cae882efd77790875cfb2bb"
        	            	  },
        	            	  "after_unknown": {},
        	            	  "before": {
        	            	    "description": "SECURE_VALUE_HIDDEN_HASH:-0eb76504eb23a58279c7abeb79a0ae3a5df813335249eac82bb6e7a0",
        	            	    "id": "55d07cd4-dd7a-4a5e-8eb7-d66d61c2b901",
        	            	    "resource_attributes": "SECURE_VALUE_HIDDEN_HASH:-e8ca899cbea54ccc842d681d0d4b141c1d24d20474498e1733f53d9b",
        	            	    "roles": "SECURE_VALUE_HIDDEN_HASH:-6c041bc4eb70851abf01f3f2c30e0782ad77d7fb43123e5f28b32a32",
        	            	    "source_resource_group_id": "",
        	            	    "source_resource_instance_id": "SECURE_VALUE_HIDDEN_HASH:-b819e7a15ecd6b64a1dfd855f0c7ff4254d01491e3ce187a8be842dc",
        	            	    "source_resource_type": "",
        	            	    "source_service_account": "abac0df06b644a9cabc6e44f55b3880e",
        	            	    "source_service_name": "cloud-object-storage",
        	            	    "subject_attributes": "SECURE_VALUE_HIDDEN_HASH:-22674679420bf72932cbba31231be558f4c250bec8a93934d97d0300",
        	            	    "target_resource_group_id": "",
        	            	    "target_resource_instance_id": "SECURE_VALUE_HIDDEN_HASH:-a21ea11290ee6383b51f50e4cb27903d631f67ae8953edec4c179425",
        	            	    "target_resource_type": "",
        	            	    "target_service_name": "SECURE_VALUE_HIDDEN_HASH:-9fa85af771ebe26be1e605a7404b13f837902dff9703679aeea6f04b",
        	            	    "transaction_id": "30c7252dd2dd47fd95539f093c21b850",
        	            	    "version": null
        	            	  },
        	            	  "before_sensitive": {
        	            	    "resource_attributes": "SECURE_VALUE_HIDDEN_HASH:-f9b28e669e518d0e0f3ea774d4ee0e2e257e8cae370e4fec82df827b",
        	            	    "roles": "SECURE_VALUE_HIDDEN_HASH:-ce6f5466f1504f402bf3ae201b7e9150ed3f91c176c3983a6e8ab252",
        	            	    "subject_attributes": "SECURE_VALUE_HIDDEN_HASH:-940ec00567a5a285ded6df85f16a697bd87f4eb4b1e10860a32625bc"
        	            	  }
}

I am unsure of what exactly is changing but it seems to be doing an update to the policy.

@ocofaigh
Copy link
Member

ocofaigh commented Jul 3, 2024

We should not be touching the auth policy being created in cos module, so this needs to be debugged to find out why its updating - suggest to recreate locally

@kierramarie
Copy link
Contributor Author

@ocofaigh From what I understand skip_iam_authorization in main is being set to false (default) but on my branch it is being changed to true to use the external policy. Would this cause a change/update in the upgrade test?

@ocofaigh
Copy link
Member

But @kierramarie The test is not setting any value for ibmcloud_kms_api_key so the value being passed for skip_iam_authorization_policy should not be changing as part of this PR. If it is, then you may have incorrect logic somewhere

@kierramarie
Copy link
Contributor Author

kierramarie commented Jul 16, 2024

@ocofaigh this is what I got when I ran locally. Its not actually changing but is triggering an update for some reason:

  ~ resource "ibm_iam_authorization_policy" "policy" {
      # Warning: this attribute value will be marked as sensitive and will not
      # display in UI output after applying this change. The value is unchanged.
      ~ description                 = (sensitive value)
        id                          = "a7d3640e-6ff1-4411-be7c-69a650afe464"
      # Warning: this attribute value will be marked as sensitive and will not
      # display in UI output after applying this change. The value is unchanged.
      ~ source_resource_instance_id = (sensitive value)
      # Warning: this attribute value will be marked as sensitive and will not
      # display in UI output after applying this change. The value is unchanged.
      ~ target_resource_instance_id = (sensitive value)
      # Warning: this attribute value will be marked as sensitive and will not
      # display in UI output after applying this change. The value is unchanged.
      ~ target_service_name         = (sensitive value)

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

@kierramarie
Copy link
Contributor Author

@ocofaigh is my assumption that if a user passes an existing scc instance, no kms variables are needed, correct?

@ocofaigh
Copy link
Member

ocofaigh commented Nov 1, 2024

@kierramarie I think what we agreed was if user is passing an existing SCC, the assumption is its already configured with a COS bucket, and hence no KMS details are required, since we only use KMS to encrypt the COS bucket.

@kierramarie
Copy link
Contributor Author

/run pipeline

@ocofaigh
Copy link
Member

ocofaigh commented Nov 6, 2024

@kierramarie can you address the failures here please?

@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

This is why the upgrade test is failing:

TestRunUpgradeInstances 2024-11-06T20:50:25Z command.go:185:   # module.cos[0].module.buckets.ibm_iam_authorization_policy.policy[0] will be updated in-place
TestRunUpgradeInstances 2024-11-06T20:50:25Z command.go:185:   ~ resource "ibm_iam_authorization_policy" "policy" {
TestRunUpgradeInstances 2024-11-06T20:50:25Z command.go:185:       ~ description                 = "Allow the COS instance 1eac2595-71fe-47d1-86f2-867bfb4ec650 to read the hs-crypto key 62953bc1-b5d1-4443-8ee6-17f3f0c353b1 from the instance e6dce284-e80f-46e1-a3c1-830f7adff7a9" -> "Allow the COS instance with GUID 1eac2595-71fe-47d1-86f2-867bfb4ec650 reader access to the hs-crypto instance GUID e6dce284-e80f-46e1-a3c1-830f7adff7a9"
TestRunUpgradeInstances 2024-11-06T20:50:25Z command.go:185:         id                          = "db2d2367-e099-4314-aa6a-5d317dfa8d1a"
TestRunUpgradeInstances 2024-11-06T20:50:25Z command.go:185:         # (11 unchanged attributes hidden)
TestRunUpgradeInstances 2024-11-06T20:50:25Z command.go:185: 
TestRunUpgradeInstances 2024-11-06T20:50:25Z command.go:185:         # (5 unchanged blocks hidden)
TestRunUpgradeInstances 2024-11-06T20:50:25Z command.go:185:     }

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

@kierramarie
Copy link
Contributor Author

/run pipeline

@ocofaigh ocofaigh merged commit 99414eb into main Nov 8, 2024
2 checks passed
@ocofaigh ocofaigh deleted the ksearle-ext-kms branch November 8, 2024 11:26
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 1.22.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants