Managing different kinds of secrets #11

liseli · 2025-11-06T21:52:48Z

This PR implements the requirements defined on the Jira ticket

Adjusting the logic to check the type of secrets before running the pull command.
Creating a list with the allowed or forbidden secrets in src/tkseal/configuration.py
Testing the new logic
Updating the README.MD file - Please read the session Forbidden Secrets Warning to understand how the logic works to filter out secrets.

Copilot

Pull Request Overview

This PR implements a forbidden secrets warning feature that prevents pulling sensitive system-managed Kubernetes secrets (like service account tokens, helm releases) while keeping users informed about what's being filtered.

Key changes include:

Added secret type filtering logic to automatically exclude forbidden secret types
Implemented warning messages in the CLI to inform users about filtered secrets
Enhanced test coverage for the new forbidden secrets functionality

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
src/tkseal/configuration.py	Defines allowed and forbidden secret type constants
src/tkseal/secret.py	Implements secret type filtering and tracking of forbidden secrets
src/tkseal/secret_state.py	Adds caching and method to expose forbidden secrets
src/tkseal/seal.py	Improves error handling for invalid JSON and preserves secret types
src/tkseal/cli.py	Adds warning display for forbidden secrets in pull command, comments out diff logic in seal command
tests/test_secret.py	Adds tests for secret type handling and filtering
tests/test_secret_state.py	Adds tests for forbidden secrets retrieval
tests/test_cli.py	Adds test for forbidden secrets warning display, comments out seal command tests
tests/test_seal.py	Updates error handling test expectations
tests/conftest.py	Adds kubectl_output fixture and updates mocks for forbidden secrets
README.md	Documents the forbidden secrets warning feature

Comments suppressed due to low confidence (3)

src/tkseal/cli.py:161

Commented-out code should be removed. The diff check logic is currently bypassed, which may allow sealing even when there are no differences. If this is intentional, add a TODO comment explaining why and when it will be restored.

        #if result.has_differences:
        #    click.echo(result.diff_output)

        # Confirm before sealing
        if click.confirm("Are you sure?"):
            seal_obj = Seal(secret_state)
            seal_obj.run()
            click.echo("Successfully sealed secrets to sealed_secrets.json")
        #else:
        #    click.echo("No differences")

src/tkseal/cli.py:153

This comment appears to contain commented-out code.

        #if result.has_differences:
        #    click.echo(result.diff_output)

src/tkseal/cli.py:161

This comment appears to contain commented-out code.

        #else:
        #    click.echo("No differences")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-06T21:55:54Z

tests/test_cli.py

+        self, cli_runner, mock_secret_state, mock_pull_cli, diff_result_no_changes,
+    temp_tanka_env):


The method parameters have inconsistent formatting. The closing parenthesis and temp_tanka_env parameter should be on the same line as the other parameters, or all parameters should follow consistent multi-line formatting.

Suggested change

self, cli_runner, mock_secret_state, mock_pull_cli, diff_result_no_changes,

temp_tanka_env):

self,

cli_runner,

mock_secret_state,

mock_pull_cli,

diff_result_no_changes,

temp_tanka_env,

):

Copilot · 2025-11-06T21:55:54Z

src/tkseal/configuration.py

+          "Opaque",  # Standard application secrets
+          "kubernetes.io/basic-auth",  # HTTP basic auth
+          "kubernetes.io/ssh-auth",  # SSH keys
+      }


Inconsistent indentation in set definition. The set elements have excessive leading whitespace (11 spaces) and the closing brace has only 7 spaces. Should use consistent indentation of 4 spaces.

Suggested change

"Opaque", # Standard application secrets

"kubernetes.io/basic-auth", # HTTP basic auth

"kubernetes.io/ssh-auth", # SSH keys

}

"Opaque", # Standard application secrets

"kubernetes.io/basic-auth", # HTTP basic auth

"kubernetes.io/ssh-auth", # SSH keys

}

Copilot · 2025-11-06T21:55:54Z

src/tkseal/configuration.py

+      }
+
+# Allowed secret types that tkseal can manage but with extra caution - showing warnings in the CLI
+MANAGED_SECRETS_CAREFULY_TYPES = {


Corrected spelling of 'CAREFULY' to 'CAREFULLY'.

Suggested change

MANAGED_SECRETS_CAREFULY_TYPES = {

MANAGED_SECRETS_CAREFULLY_TYPES = {

Copilot · 2025-11-06T21:55:54Z

src/tkseal/configuration.py

+  # "kubernetes.io/tls",  # TLS certificates
+  "kubernetes.io/dockerconfigjson",  # Docker registry credentials. Users manage all these secrets manually,


Inconsistent indentation in set definition. The comment at line 26 uses 2 spaces, the element at line 27 uses 2 spaces, but the continuation comment at line 28 uses 4 spaces. Should use consistent indentation of 4 spaces for all elements.

Suggested change

# "kubernetes.io/tls", # TLS certificates

"kubernetes.io/dockerconfigjson", # Docker registry credentials. Users manage all these secrets manually,

# "kubernetes.io/tls", # TLS certificates

"kubernetes.io/dockerconfigjson", # Docker registry credentials. Users manage all these secrets manually,

Copilot · 2025-11-06T21:55:55Z

src/tkseal/configuration.py

+          "Opaque",  # Standard application secrets
+          "kubernetes.io/basic-auth",  # HTTP basic auth
+          "kubernetes.io/ssh-auth",  # SSH keys
+      }
+
+# Allowed secret types that tkseal can manage but with extra caution - showing warnings in the CLI
+MANAGED_SECRETS_CAREFULY_TYPES = {
+  # "kubernetes.io/tls",  # TLS certificates
+  "kubernetes.io/dockerconfigjson",  # Docker registry credentials. Users manage all these secrets manually,
+    # so is safe to handle by tkseal.
+}
+
+# Never allow these (system-managed, high risk)
+FORBIDDEN_SECRET_TYPES = {
+  "kubernetes.io/service-account-token",  # Cluster API tokens
+  "bootstrap.kubernetes.io/token",  # Bootstrap tokens
+  "helm.sh/release.v1",  # Helm release data


Inconsistent indentation in set definition. The set elements use 2 spaces while Python convention is 4 spaces for indentation.

Suggested change

"Opaque", # Standard application secrets

"kubernetes.io/basic-auth", # HTTP basic auth

"kubernetes.io/ssh-auth", # SSH keys

}

# Allowed secret types that tkseal can manage but with extra caution - showing warnings in the CLI

MANAGED_SECRETS_CAREFULY_TYPES = {

# "kubernetes.io/tls", # TLS certificates

"kubernetes.io/dockerconfigjson", # Docker registry credentials. Users manage all these secrets manually,

# so is safe to handle by tkseal.

}

# Never allow these (system-managed, high risk)

FORBIDDEN_SECRET_TYPES = {

"kubernetes.io/service-account-token", # Cluster API tokens

"bootstrap.kubernetes.io/token", # Bootstrap tokens

"helm.sh/release.v1", # Helm release data

"Opaque", # Standard application secrets

"kubernetes.io/basic-auth", # HTTP basic auth

"kubernetes.io/ssh-auth", # SSH keys

}

# Allowed secret types that tkseal can manage but with extra caution - showing warnings in the CLI

MANAGED_SECRETS_CAREFULY_TYPES = {

# "kubernetes.io/tls", # TLS certificates

"kubernetes.io/dockerconfigjson", # Docker registry credentials. Users manage all these secrets manually,

# so is safe to handle by tkseal.

}

# Never allow these (system-managed, high risk)

FORBIDDEN_SECRET_TYPES = {

"kubernetes.io/service-account-token", # Cluster API tokens

"bootstrap.kubernetes.io/token", # Bootstrap tokens

"helm.sh/release.v1", # Helm release data

Copilot · 2025-11-06T21:55:55Z

src/tkseal/seal.py

                            "namespace": self.secret_state.namespace,
-                        }
+                        },
+                        # Preseve secret type if specified in plain_secrets.json


Corrected spelling of 'Preseve' to 'Preserve'.

Suggested change

# Preseve secret type if specified in plain_secrets.json

# Preserve secret type if specified in plain_secrets.json

Copilot · 2025-11-06T21:55:55Z

README.md

+  - `kubernetes.io/service-account-token`
+  - `helm.sh/release.v1`
+  - Any other secret types deemed sensitive or system-managed
+  - The forbidden and allowed secret types are defined in `/src/tkseal/confingutration.py` 


Corrected spelling of 'confingutration' to 'configuration'.

Suggested change

- The forbidden and allowed secret types are defined in `/src/tkseal/confingutration.py`

- The forbidden and allowed secret types are defined in `/src/tkseal/configuration.py`

Copilot · 2025-11-06T21:55:56Z

tests/test_cli.py

        mock_seal.run.assert_called_once()
        # Verify Diff.plain() was called
-        mock_diff.plain.assert_called_once()
+        # mock_diff.plain.assert_called_once()


Commented-out assertion should be removed or the test should be updated to reflect the current expected behavior. Leaving commented code in tests reduces clarity.

Suggested change

# mock_diff.plain.assert_called_once()

mock_diff.plain.assert_called_once()

Copilot · 2025-11-06T21:55:56Z

src/tkseal/cli.py

+        #click.secho(
+        #    'This shows what would change in the cluster based on "plain_secrets.json"',
+        #    fg="yellow",
+        #)


Commented-out code should be removed. If the functionality is temporarily disabled, consider using a feature flag or documenting why it's disabled with a TODO comment.

Copilot · 2025-11-06T21:55:56Z

tests/test_secret_state.py

 from pathlib import Path
 from unittest.mock import Mock

+from tkseal.secret import Secrets


Import of 'Secrets' is not used.

Suggested change

from tkseal.secret import Secrets

liseli · 2025-11-10T14:53:00Z

src/tkseal/configuration.py

+
+# Allowed secret types that tkseal can manage but with extra caution - showing warnings in the CLI
+MANAGED_SECRET_CAREFULLY_TYPES = {
+    # "kubernetes.io/tls",  # TLS certificates


It seems we should add kubernetes.io/tls to the list of FORBIDDEN_SECRET_TYPES. Could you confirm it @aelkiss and @Ronster2018?

I think that's correct. It's possible we might need to manually create such a secret and have it be managed by tkseal, but having it in the forbidden secret types makes sense for now.

I agree. Sometimes the kubernetes.io/tls secrets are used in the cluster so that apps can talk to one another as well and are generated by the cluster itself so tampering with these can break a lot of things. Keeping these in FORBIDDEN_SECRETS_TYPES is a good call.

aelkiss

Overall this looks like it's doing what's intended. I would like to see filter_out_forbidden_secrets renamed / clarified, and there are a few other things we might want to consider cleaning up.

Also noting that I thought I saw something about having tkseal seal refuse to seal any secrets with forbidden types, but now I don't see it. I don't think that's implemented currently but I do think it's a good idea.

aelkiss · 2025-11-10T17:14:07Z

src/tkseal/cli.py

+        forbidden_secrets = secret_state.get_forbidden_secrets()
+        if forbidden_secrets:
+            click.secho(
+                "\n⚠ Warning: Found forbidden secrets in namespace that cannot be pulled:",


I think this warning might be harsher than needed especially because all namespaces will have at least the default service account. I think we could put the "These secrets are system-managed and will not be included in plain_secrets.json" at the beginning of the output as an informational thing, and omit the "Warning: Found forbidden secrets..."

aelkiss · 2025-11-10T17:15:05Z

src/tkseal/secret.py

+        # Store forbidden secrets for reporting - [{"secret1":"kubernetes.io/service-account-token"}]
+        self.forbidden_secrets = Secrets.filter_out_forbidden_secrets(raw_secrets)
+
+        # TODO: If the secret does not have type, assume "Opaque" (Kubernetes default)???


I think this is correct

aelkiss · 2025-11-10T17:23:31Z

src/tkseal/secret.py

        return cls(raw_secrets)

+    @staticmethod
+    def filter_out_forbidden_secrets(


The name and documentation isn't consistent with how it's used. This method returns a list of the secrets with disallowed types; https://github.com/hathitrust/tkseal/pull/11/files#diff-f1c4d61122289605e79f55501de6d8e8151d29da5882a66a39cb6f35fc5951b4R80-R84 does the actual filtering. I'd suggest changing the name and documentation here.

aelkiss · 2025-11-10T17:25:16Z

src/tkseal/secret_state.py


    @property
-    def namespace(self) -> str:
+    def namespace(self) -> Any:


Why did the type hint change to "Any" here & elsewhere in this file? Are there cases where we aren't returning a string?

Those changes were a quick attempt to silence the mypy type checker because get_val() could return str or None. I'll check if there is a way to apply some casting to be more specific on these functions. The easiest change is to return an empty string instead of None.

aelkiss · 2025-11-10T17:29:35Z

src/tkseal/secret_state.py

+        """Get list of forbidden secrets that exist in the namespace but cannot be pulled.
+
+        Returns:
+            list[dict[str, str]]: List of dictionaries with secret names and types.


May want to consider using a subclass of secret (ForbiddenSecret?) instead of dict[str, str] maybe where ForbiddenSecret returns None for data and raises a TKSealError if you try to seal it..? Not sure.

It could be a good abstraction here. Let me compare it with the current logic to avoid/reduce overcomplicating.

aelkiss · 2025-11-10T17:31:23Z

tests/test_secret.py

    assert secret.name == "test-secret"


+def test_secret_type():


Might be worth testing a non-Opaque secret type instead / in addition?

aelkiss · 2025-11-10T17:33:51Z

tests/conftest.py



+@pytest.fixture()
+def kubectl_output():


This looks good for reducing duplication in some of the tests however it's only used in test_secret.py currently and maybe should just be there? If we want to use it elsewhere in the future then I think we could keep it here.

I moved it to test_secret.py

aelkiss · 2025-11-10T17:36:08Z

tests/test_kubectl.py

        assert KubeCtl.exists() is False

    def test_get_secrets_success(self, mocker, load_secret_file):
+        """Test successful retrieval and parsing of Kubernetes secrets."""


I see some inconsistencies between using comments and doc strings in the tests - e.g. we just changed from doc strings to comments in test_configuration.py. This seems like something the code formatter could or should be doing. I don't have a preference and it isn't a major issue, just happened to notice the change immediately above followed by a new doc string here.

I saw the inconsistencies and fixed them.

aelkiss · 2025-11-10T17:40:21Z

tests/test_secret.py

    assert secrets.items[1].name == "secret2"

+
+def test_secret_allowed_type(kubectl_output):


The tests here look good.

Ronster2018

So far these changes look good to me. I agree with Aarons comments about the warning output and how that can just be more informative instead. I also think that kubernetes.io/tls should be in the forbidden type since messing with that without intricate know how can break things on the cluster.

liseli · 2025-11-12T22:19:27Z

@aelkiss and @Ronster2018, I've reviewed this PR, adding most of your provided feedback

Managing forbidden secrets with the class ForbiddenSecret
Updating/Adding tests
Improving doc strings and CLI messages.

Ronster2018

I reviewed these changes on a zoom call with @liseli and talked about moving the tls secrets to the forbidden type. Also reviewed the rest of the code changes.

…warning when forbidden secrets are pulled Refactoring to remove duplicate tests Appling all the feedbacks and created the class ForbiddenSecret to manage the not allowed secret types

liseli requested review from Ronster2018, aelkiss and Copilot November 6, 2025 21:52

Copilot AI reviewed Nov 6, 2025

View reviewed changes

liseli commented Nov 10, 2025

View reviewed changes

aelkiss reviewed Nov 10, 2025

View reviewed changes

aelkiss mentioned this pull request Nov 10, 2025

Ett 241 json2yaml format #12

Merged

Ronster2018 reviewed Nov 11, 2025

View reviewed changes

Ronster2018 self-requested a review November 13, 2025 16:40

Ronster2018 approved these changes Nov 13, 2025

View reviewed changes

Managing different kinds of secrets; Testing the logic and showing a …

179cac7

…warning when forbidden secrets are pulled Refactoring to remove duplicate tests Appling all the feedbacks and created the class ForbiddenSecret to manage the not allowed secret types

liseli force-pushed the ETT-242_handle_secret_type branch from 35bc52d to 179cac7 Compare November 13, 2025 17:03

liseli merged commit b37b5f2 into main Nov 13, 2025
1 check passed

liseli deleted the ETT-242_handle_secret_type branch November 13, 2025 17:20

		self, cli_runner, mock_secret_state, mock_pull_cli, diff_result_no_changes,
		temp_tanka_env):

	MANAGED_SECRETS_CAREFULY_TYPES = {
	MANAGED_SECRETS_CAREFULLY_TYPES = {

		# "kubernetes.io/tls", # TLS certificates
		"kubernetes.io/dockerconfigjson", # Docker registry credentials. Users manage all these secrets manually,

	# Preseve secret type if specified in plain_secrets.json
	# Preserve secret type if specified in plain_secrets.json

	- The forbidden and allowed secret types are defined in `/src/tkseal/confingutration.py`
	- The forbidden and allowed secret types are defined in `/src/tkseal/configuration.py`

	# mock_diff.plain.assert_called_once()
	mock_diff.plain.assert_called_once()

		assert secrets.items[1].name == "secret2"


		def test_secret_allowed_type(kubectl_output):

Managing different kinds of secrets #11

Managing different kinds of secrets #11

Uh oh!

Conversation

liseli commented Nov 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelkiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liseli Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liseli Nov 12, 2025 •

edited

Loading