fix: improve NanoTDF support #112

b-long · 2025-10-02T00:46:24Z

fix: improve NanoTDF support

gemini-code-assist · 2025-10-02T00:46:40Z

Summary of Changes

Hello @b-long, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the Python SDK by removing unused and placeholder service client interfaces and associated configuration options, leading to a leaner and more focused API. Crucially, it also advances NanoTDF support by enabling and updating its integration test, demonstrating basic encryption and decryption functionality with a directly provided key, paving the way for more robust KAS integration.

Highlights

SDK Interface Simplification: Several placeholder service client interfaces (e.g., Attributes, Namespace, Authorization) and related configuration parameters (trust manager, auth interceptor, platform services client) have been removed from the SDK and Services classes, streamlining the API.
NanoTDF Test Activation and Enhancement: The integration test for NanoTDF encryption and decryption has been unskipped. It now directly generates and uses a symmetric key for testing, demonstrating basic functionality and removing previous NotImplementedError handling.
Code Cleanup: Unused stubs for ProtocolClient, Interceptor, and TrustManager have been removed, along with their corresponding methods and attributes across the SDK and test files.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request enables an integration test for NanoTDF encryption and decryption. While the test itself is a good addition for verifying the round-trip functionality, it exposes a critical issue in the underlying NanoTDF implementation. The NanoTDF class in src/otdf_python/nanotdf.py defines methods with the same name multiple times, which is not supported in Python and leads to method overriding. This will cause the new test to fail with a TypeError. The underlying issue in nanotdf.py needs to be addressed before this test can pass.

gemini-code-assist · 2025-10-02T00:47:45Z

tests/test_nanotdf.py

+    config = NanoTDFConfig(kas_info_list=[kas_info], cipher=key.hex())
+
+    # Create and read NanoTDF
+    nanotdf_bytes = nanotdf.create_nanotdf(data, config)


This call to create_nanotdf will fail with a TypeError.

The NanoTDF class in src/otdf_python/nanotdf.py has two method definitions for create_nanotdf. In Python, the second definition overwrites the first. The method being called here (defined at line 442 in nanotdf.py) then makes a call to self.create_nano_tdf on line 455 with an incorrect number of arguments, which causes the TypeError.

A similar issue exists for the read_nanotdf method.

To fix this, the duplicate method names in src/otdf_python/nanotdf.py must be resolved, for instance by renaming the stream-based methods to something like create_nanotdf_to_stream. Since that file is not part of this pull request, this test will remain broken until the underlying code is fixed.

b-long · 2025-10-02T01:24:40Z

/gemini review

gemini-code-assist

Code Review

This pull request significantly improves NanoTDF support by implementing core encryption and decryption logic and adding a comprehensive suite of integration tests. The changes are a great step forward. My review focuses on two main areas for improvement. First, in ecc_mode.py, the new from_string method should be made safer by validating its input to prevent downstream errors. Second, in nanotdf.py, the logic for retrieving a symmetric key during decryption contains unreachable code and can be refactored for better robustness and readability. The new integration tests are well-structured and provide excellent coverage for the new functionality.

src/otdf_python/ecc_mode.py

src/otdf_python/nanotdf.py

b-long · 2025-10-05T13:39:49Z

/gemini review

gemini-code-assist

Code Review

This pull request provides a significant improvement to NanoTDF support by implementing the core encryption and decryption logic, and adding a suite of valuable integration tests. The new tests for interoperability with otdfctl and for the Python-only roundtrip are particularly well-done and increase confidence in the implementation. My review includes a couple of minor suggestions to improve code maintainability by replacing magic values and locally-scoped constants with more permanent constants.

src/otdf_python/ecc_mode.py

gemini-code-assist · 2025-10-05T13:41:15Z

tests/integration/otdfctl_to_python/test_nanotdf_cli_comparison.py

+        assert otdfctl_size < 10000, "otdfctl NanoTDF should be compact"
+        assert python_size < 10000, "Python NanoTDF should be compact"


The magic number 10000 is used here for asserting the file size. It's better to define this as a constant with a descriptive name at the module level (e.g., MAX_EXPECTED_NANO_TDF_SIZE = 10000) and use that constant in these assertions. This improves readability and makes the test easier to maintain.

b-long · 2025-10-05T15:03:03Z

/gemini review

gemini-code-assist

Code Review

This pull request significantly improves NanoTDF support by implementing KAS integration for key wrapping and unwrapping, adding the ability to derive KAS endpoints from the platform URL, and introducing a comprehensive suite of integration tests to ensure interoperability. The changes are well-structured and the addition of tests is a great step towards ensuring robustness. My review focuses on a few areas for improvement: moving local imports to the top level for better code style and performance, addressing a critical gap in policy handling during KAS unwrap, and simplifying some complex and redundant logic to enhance maintainability and correctness.

gemini-code-assist · 2025-10-05T15:04:49Z

src/otdf_python/nanotdf.py

+            policy_json = "{}"  # Default empty policy for now
+            # TODO: Extract actual policy from policy_info if needed


The policy is currently hardcoded to an empty JSON object ({}), and a TODO comment indicates that the actual policy extraction is not implemented. This is a significant gap, as it means the unwrap request sent to KAS will not contain the correct policy, potentially leading to incorrect access decisions or failures. The policy information should be extracted from the header_obj and included in the KAS request.

gemini-code-assist · 2025-10-05T15:04:49Z

src/otdf_python/nanotdf.py

+    def _local_unwrap(self, wrapped_key: bytes, config: NanoTDFConfig) -> bytes:
+        """Unwrap key locally using private key or mock unwrap (for testing/offline use)."""
+        kas_private_key = None
+        # Try to get from cipher field if it looks like a PEM key
+        if (
+            config.cipher
+            and isinstance(config.cipher, str)
+            and "-----BEGIN" in config.cipher
+        ):
+            kas_private_key = config.cipher
+
+        # Check if mock unwrap is enabled in config string
+        kas_mock_unwrap = False
+        if config.config and "mock_unwrap=true" in config.config.lower():
+            kas_mock_unwrap = True
+
+        if not kas_private_key and not kas_mock_unwrap:
+            raise InvalidNanoTDFConfig(
+                "Unable to unwrap NanoTDF key: KAS unwrap failed and no local private key available. "
+                "Ensure SDK has valid credentials or provide kas_private_key in config for offline use."
+            )
+
+        if kas_mock_unwrap:
+            # Use the KAS mock unwrap_nanotdf logic
+            from otdf_python.sdk import KAS
+
+            return KAS().unwrap_nanotdf(
+                curve=None,
+                header=None,
+                kas_url=None,
+                wrapped_key=wrapped_key,
+                kas_private_key=kas_private_key,
+                mock=True,
+            )
+        else:
+            asym = AsymDecryption(kas_private_key)
+            return asym.decrypt(wrapped_key)
+


The logic in _local_unwrap is overly complex and contains redundant paths. The if kas_mock_unwrap: branch is convoluted, inefficient due to KAS() instantiation, and ultimately performs the same action as the else branch: decrypting the key with AsymDecryption. This complexity increases the maintenance burden and can hide bugs.

The function can be greatly simplified by removing the kas_mock_unwrap logic and relying on a single, clear path for local private key decryption.

def _local_unwrap(self, wrapped_key: bytes, config: NanoTDFConfig) -> bytes: """Unwrap key locally using private key from config.""" kas_private_key = None # Try to get from cipher field if it looks like a PEM key if ( config.cipher and isinstance(config.cipher, str) and "-----BEGIN" in config.cipher ): kas_private_key = config.cipher if not kas_private_key: raise InvalidNanoTDFConfig( "Unable to unwrap NanoTDF key: KAS unwrap failed and no local private key available. " "Ensure SDK has valid credentials or provide a PEM private key in config.cipher for offline use." ) asym = AsymDecryption(kas_private_key) return asym.decrypt(wrapped_key)

gemini-code-assist · 2025-10-05T15:04:49Z

src/otdf_python/nanotdf.py

        Returns:
            tuple: (wrapped_key, kas_public_key)
        """
+        import logging


Imports should be at the top of the file, not inside functions. This file has several local imports (e.g., logging here, base64, Header, KeyAccess in _kas_unwrap).

Per PEP 8, imports should be at the top of the module. This practice improves readability, avoids circular import issues, and prevents the performance overhead of re-importing modules if a function is called multiple times in a loop. Please move all local imports to the top of the file.

gemini-code-assist · 2025-10-05T15:04:49Z

src/otdf_python/nanotdf.py

+        except Exception as e:
+            # If KAS unwrap fails, log and fall through to local unwrap methods
+            import logging
+
+            logging.warning(f"KAS unwrap failed for NanoTDF: {e}, trying local unwrap")
+            key = None


Catching a broad Exception is generally discouraged as it can mask unexpected programming errors (e.g., AttributeError, TypeError) by treating them as a KAS communication failure. This makes debugging more difficult. It would be more robust to catch more specific exceptions related to network I/O or specific SDKException subclasses that the KAS client might raise.

b-long changed the base branch from main to chore/remove-placeholders October 2, 2025 00:46

gemini-code-assist bot reviewed Oct 2, 2025

View reviewed changes

src/otdf_python/ecc_mode.py Outdated Show resolved Hide resolved

src/otdf_python/nanotdf.py Outdated Show resolved Hide resolved

Base automatically changed from chore/remove-placeholders to main October 4, 2025 14:03

fix: update test_nanotdf.py

c5c5142

b-long force-pushed the chore/improve-nanotdf-support branch from a5ae142 to c5c5142 Compare October 4, 2025 14:08

b-long added 2 commits October 5, 2025 09:21

fix: work-in-progress - add broken tests

f90eec0

fix: improve NanoTDF support

dd30965

gemini-code-assist bot reviewed Oct 5, 2025

View reviewed changes

fix: improve NanoTDF support

2d06e6a

gemini-code-assist bot reviewed Oct 5, 2025

View reviewed changes

b-long marked this pull request as draft October 5, 2025 19:45

		assert otdfctl_size < 10000, "otdfctl NanoTDF should be compact"
		assert python_size < 10000, "Python NanoTDF should be compact"

		policy_json = "{}" # Default empty policy for now
		# TODO: Extract actual policy from policy_info if needed

fix: improve NanoTDF support #112

Are you sure you want to change the base?

fix: improve NanoTDF support #112

Uh oh!

Conversation

b-long commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Oct 2, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

b-long commented Oct 2, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

b-long commented Oct 5, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

b-long commented Oct 5, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

b-long commented Oct 2, 2025 •

edited

Loading