-
Notifications
You must be signed in to change notification settings - Fork 108
Add config option to enable the encryption of AWS EKS secrets #2788
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
Changes from 6 commits
e76058b
319f947
dcfd836
60d2a96
36518be
567a05d
8374ada
a4d89cb
c3cc413
b204146
a2ff67c
3c309d5
b70adaa
8569dbe
422f59d
6f69cf9
243e764
095c6e0
f297a99
5f39265
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -121,6 +121,27 @@ def instances(region: str) -> Dict[str, str]: | |
| return {t: t for t in instance_types} | ||
|
|
||
|
|
||
| @functools.lru_cache() | ||
| def kms_key_arns(region: str) -> Dict[str, dict]: | ||
| """Return dict of available/enabled KMS key IDs and associated KeyMetadata for the AWS region.""" | ||
| session = aws_session(region=region) | ||
| client = session.client("kms") | ||
| # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/kms/client/list_keys.html | ||
| paginator = client.get_paginator("list_keys") | ||
|
||
| fields = [ | ||
| "Arn", | ||
| "KeyUsage", | ||
| "KeySpec", | ||
| # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/kms/client/describe_key.html#:~:text=Response%20Structure | ||
| ] | ||
| kms_keys = [ | ||
| client.describe_key(KeyId=j["KeyId"]).get("KeyMetadata") | ||
| for i in paginator.paginate() | ||
| for j in i["Keys"] | ||
| ] | ||
| return {i["KeyId"]: {k: i[k] for k in fields} for i in kms_keys if i["Enabled"]} | ||
|
||
|
|
||
|
|
||
| def aws_get_vpc_id(name: str, namespace: str, region: str) -> Optional[str]: | ||
| """Return VPC ID for the EKS cluster namedd `{name}-{namespace}`.""" | ||
| cluster_name = f"{name}-{namespace}" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,6 +182,7 @@ class AWSInputVars(schema.Base): | |
| eks_endpoint_access: Optional[ | ||
| Literal["private", "public", "public_and_private"] | ||
| ] = "public" | ||
| eks_kms_arn: Optional[str] = None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this field should be immutable to prevent clusters from being deleted. See src/_nebari/stages/kubernetes_services/init.py:66 for an example of an immutable field.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hello @dcmcand
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, so: What if someone tries to disable encryption, it fails, and then the reenable it in the config and try to deploy again? Does that succeed without error? If so then I think we are good to go. It would be nice to have one way gates as an option on immutable fields for situations like this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @joneszc can you test the above scenario and update with the latest main?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dcmcand
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dcmcand
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's good to hear, @joneszc. If I understood correctly if by any chance the user removes the encryption key from the config and redeploys, the user would first face an error, but if -- what I assume in a few minutes -- the user re-attempts the deployment, it will succeed? If so, please add this as an admonition to the docs under a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello @viniciusdc That is correct. |
||
| node_groups: List[AWSNodeGroupInputVars] | ||
| availability_zones: List[str] | ||
| vpc_cidr_block: str | ||
|
|
@@ -498,6 +499,7 @@ class AmazonWebServicesProvider(schema.Base): | |
| eks_endpoint_access: Optional[ | ||
| Literal["private", "public", "public_and_private"] | ||
| ] = "public" | ||
| eks_kms_arn: Optional[str] = None | ||
| existing_subnet_ids: Optional[List[str]] = None | ||
| existing_security_group_id: Optional[str] = None | ||
| vpc_cidr_block: str = "10.10.0.0/16" | ||
|
|
@@ -554,6 +556,37 @@ def _check_input(cls, data: Any) -> Any: | |
| f"Amazon Web Services instance {node_group.instance} not one of available instance types={available_instances}" | ||
| ) | ||
|
|
||
| # check if kms key is valid | ||
| available_kms_keys = amazon_web_services.kms_key_arns(data["region"]) | ||
| if "eks_kms_arn" in data and data["eks_kms_arn"] is not None: | ||
| key_id = [ | ||
| id for id in available_kms_keys.keys() if id in data["eks_kms_arn"] | ||
| ] | ||
| if ( | ||
| len(key_id) == 1 | ||
| and available_kms_keys[key_id[0]]["Arn"] == data["eks_kms_arn"] | ||
| ): | ||
| key_id = key_id[0] | ||
| # Symmetric KMS keys with Encrypt and decrypt key-usage have the SYMMETRIC_DEFAULT key-spec | ||
| # EKS cluster encryption requires a Symmetric key that is set to encrypt and decrypt data | ||
| if available_kms_keys[key_id]["KeySpec"] != "SYMMETRIC_DEFAULT": | ||
| if available_kms_keys[key_id]["KeyUsage"] == "GENERATE_VERIFY_MAC": | ||
| raise ValueError( | ||
| f"Amazon Web Services KMS Key with ID {key_id} does not have KeyUsage set to 'Encrypt and decrypt' data" | ||
| ) | ||
| elif available_kms_keys[key_id]["KeyUsage"] != "ENCRYPT_DECRYPT": | ||
| raise ValueError( | ||
| f"Amazon Web Services KMS Key with ID {key_id} is not of type Symmetric, and KeyUsage not set to 'Encrypt and decrypt' data" | ||
| ) | ||
| else: | ||
| raise ValueError( | ||
| f"Amazon Web Services KMS Key with ID {key_id} is not of type Symmetric" | ||
| ) | ||
| else: | ||
| raise ValueError( | ||
| f"Amazon Web Services KMS Key with ARN {data['eks_kms_arn']} not one of available/enabled keys={[v['Arn'] for v in available_kms_keys.values()]}" | ||
| ) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could avoid a lot of nesting here by flipping the logic on these if statements. Something like: available_kms_keys = amazon_web_services.kms_key_arns(data["region"])
# don't check if eks_kms_arn is not set
if "eks_kms_arn" not in data or data["eks_kms_arn"] is None:
return data
key_id = [
id for id in available_kms_keys.keys() if id in data["eks_kms_arn"]
]
# Raise error if key_id is not found in available_kms_keys
if (
len(key_id) != 1
or available_kms_keys[key_id[0]]["Arn"] != data["eks_kms_arn"]
):
raise ValueError(
f"Amazon Web Services KMS Key with ARN {data['eks_kms_arn']} not one of available/enabled keys={[v['Arn'] for v in available_kms_keys.values()]}"
)
key_id = key_id[0]
# EKS cluster encryption requires a Symmetric key that is set to encrypt and decrypt data
if available_kms_keys[key_id]["KeySpec"] != "SYMMETRIC_DEFAULT":
raise ValueError(
f"Amazon Web Services KMS Key with ID {key_id} is not of type Symmetric"
)
if available_kms_keys[key_id]["KeyUsage"] != "ENCRYPT_DECRYPT":
raise ValueError(
f"Amazon Web Services KMS Key with ID {key_id} KeyUsage not set to 'Encrypt and decrypt' data"
) available_kms_keys = amazon_web_services.kms_key_arns(data["region"])
# don't check if eks_kms_arn is not set
if "eks_kms_arn" not in data or data["eks_kms_arn"] is None:
return data
key_id = [
id for id in available_kms_keys.keys() if id in data["eks_kms_arn"]
]
# Raise error if key_id is not found in available_kms_keys
if (
len(key_id) != 1
or available_kms_keys[key_id[0]]["Arn"] != data["eks_kms_arn"]
):
raise ValueError(
f"Amazon Web Services KMS Key with ARN {data['eks_kms_arn']} not one of available/enabled keys={[v['Arn'] for v in available_kms_keys.values()]}"
)
key_id = key_id[0]
# EKS cluster encryption requires a Symmetric key that is set to encrypt and decrypt data
if available_kms_keys[key_id]["KeySpec"] != "SYMMETRIC_DEFAULT":
raise ValueError(
f"Amazon Web Services KMS Key with ID {key_id} is not of type Symmetric"
)
if available_kms_keys[key_id]["KeyUsage"] != "ENCRYPT_DECRYPT":
raise ValueError(
f"Amazon Web Services KMS Key with ID {key_id} KeyUsage not set to 'Encrypt and decrypt' data"
)Has less deeply nested logic and is therefore easier to read and test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dcmcand
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @joneszc I can see that. It might make sense to pull every validator into its own method then, and have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dcmcand
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you open an issue for breaking out the _check_input methods so that doesn't get lost?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @joneszc, once you open the issue, can you reference this one as well, to have a cross reference for contextualization?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @viniciusdc @dcmcand |
||
| return data | ||
|
|
||
|
|
||
|
|
@@ -843,6 +876,7 @@ def input_vars(self, stage_outputs: Dict[str, Dict[str, Any]]): | |
| name=self.config.escaped_project_name, | ||
| environment=self.config.namespace, | ||
| eks_endpoint_access=self.config.amazon_web_services.eks_endpoint_access, | ||
| eks_kms_arn=self.config.amazon_web_services.eks_kms_arn, | ||
| existing_subnet_ids=self.config.amazon_web_services.existing_subnet_ids, | ||
| existing_security_group_id=self.config.amazon_web_services.existing_security_group_id, | ||
| region=self.config.amazon_web_services.region, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is stylistic, so it is up to you whether to adopt this. I would probably add a
Kms_Key_Infoclass or something.The advantage here is more clarity in your type annotations. Rather than returning
Dict[str, dict]you can returnDict[str, Kms_Key_Info]which is clearer what is actually returned.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcmcand
Updated, per your suggestion