Use non-boolean enums for TLS mode. #816

nihohit · 2024-01-17T17:29:06Z

This allows us forward compatibility, so that once we introduce more complex TLS usage schemes, such as loading specific credentials, we won't break the API.

Issue #, if available:
#786

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This allows us forward compatibility, so that once we introduce more complex TLS usage schemes, such as loading specific credentials, we won't break the API.

barshaul · 2024-01-21T09:43:17Z

python/python/glide/config.py

@@ -51,6 +51,14 @@ class ProtocolVersion(Enum):
    """


+class TlsMode(Enum):


I think the name is confusing, it sounds like TlsMode should hold 'Disabled'/'Enabled' options, so the Auto option sounds like it choose automatically if to disable or to enable TLS. Maybe we can add 'Disabled' entry that will be set by default.

How would this enum be used if we would want to allow the user to pass his own certificates?

we'd need to have an object to handle that. The core of this change is to change the value to not be a boolean. As long as we plan on supporting auto & enabled, in TS & Python we could add more object values on top.

added "disabled" option to enum.

barshaul · 2024-01-21T09:57:20Z

python/python/glide/config.py

@@ -51,6 +51,14 @@ class ProtocolVersion(Enum):
    """


+class TlsMode(Enum):
+    """
+    Enabled TLS mode, with automatic loading of certificates.


with automatic loading of certificates. = > with built-in set of CA certificates

built-in into what?

To what rustls is using

barshaul

We need to think how to create a TLS mode that can be used with passing client auth certificates / CA certificate, so we won't need to break the API again

shachlanAmazon · 2024-01-22T07:05:50Z

We need to think how to create a TLS mode that can be used with passing client auth certificates / CA certificate, so we won't need to break the API again

In Python & TS adding more values / objects on top of existing values isn't an API break, so this isn't an issue for us ATM.

barshaul · 2024-01-22T07:33:57Z

We need to think how to create a TLS mode that can be used with passing client auth certificates / CA certificate, so we won't need to break the API again

In Python & TS adding more values / objects on top of existing values isn't an API break, so this isn't an issue for us ATM.

So what is your suggestion? that we will have an TlsMode enum, and another object that will handle TLS certs? then we will need to handle scenarios where a user passed TlsMode=Disabled together with TLS certs.

shachlanAmazon · 2024-01-22T08:13:26Z

So what is your suggestion? that we will have an TlsMode enum, and another object that will handle TLS certs? then we will need to handle scenarios where a user passed TlsMode=Disabled together with TLS certs.

No, just keep using a union of values:

tlsMode: "enabled" | "disabled" | {
 certPaths: string[]
}

class TlsCerts: 
  cert_paths: List[string]


[...]
tls_mode: Union[TlsMode, TlsCerts]

barshaul

We talked about it yesterday - we need to think if the enum & objet union is the best way to represent TLS modes (or, for example, classes), and if we do go with the enum - we need to think of a better name for the Auto mode (as it sounds like the client chooses automatically to enable/disable tls based on the server)

barshaul · 2024-01-22T13:30:45Z

python/python/glide/config.py

@@ -115,8 +128,8 @@ def __init__(
                        {address:sample-address-0001.use1.cache.amazonaws.com, port:6379},
                        {address: sample-address-0002.use2.cache.amazonaws.com, port:6379}
                    ].
-            use_tls (bool): True if communication with the cluster should use Transport Level Security.
-                Should match the TLS configuration of the server/cluster, otherwise the connection attempt will fail
+            tls_mode (TlsMode): `auto` if communication with the cluster should use Transport Level Security, with automatic loading of certificates.


'auto' => 'TlsMode.Auto'

barshaul · 2024-01-22T13:33:02Z

python/python/glide/config.py

@@ -178,7 +195,8 @@ class RedisClientConfiguration(BaseClientConfiguration):
                    {address:sample-address-0001.use1.cache.amazonaws.com, port:6379},
                    {address: sample-address-0002.use2.cache.amazonaws.com, port:6379}
                ].
-        use_tls (bool): True if communication with the cluster should use Transport Level Security.
+        tls_mode (TlsMode): `auto` if communication with the cluster should use Transport Level Security, with automatic loading of certificates.


Same -'TlsMode.Auto', or 'Auto'

barshaul · 2024-01-22T13:34:33Z

python/python/glide/config.py

@@ -197,7 +215,7 @@ class RedisClientConfiguration(BaseClientConfiguration):
    def __init__(
        self,
        addresses: List[NodeAddress],
-        use_tls: bool = False,
+        tls_mode: Optional[TlsMode] = None,


Shouldn't be optional, but configured to TlsMode.Disabled by default

barshaul · 2024-01-22T13:34:57Z

python/python/glide/config.py

@@ -248,7 +266,8 @@ class ClusterClientConfiguration(BaseClientConfiguration):
                [
                    {address:configuration-endpoint.use1.cache.amazonaws.com, port:6379}
                ].
-        use_tls (bool): True if communication with the cluster should use Transport Level Security.
+        tls_mode (TlsMode): `auto` if communication with the cluster should use Transport Level Security, with automatic loading of certificates.


Same 'auto'

barshaul · 2024-01-22T13:35:10Z

python/python/glide/config.py

@@ -266,7 +285,7 @@ class ClusterClientConfiguration(BaseClientConfiguration):
    def __init__(
        self,
        addresses: List[NodeAddress],
-        use_tls: bool = False,
+        tls_mode: Optional[TlsMode] = None,


barshaul · 2024-01-22T13:35:47Z

python/python/tests/conftest.py

@@ -111,14 +112,14 @@ async def create_client(
    protocol: ProtocolVersion = ProtocolVersion.RESP3,
 ) -> Union[RedisClient, RedisClusterClient]:
    # Create async socket client
-    use_tls = request.config.getoption("--tls")
+    tls_mode = TlsMode.Auto if request.config.getoption("--tls") else None


else TlsMode.Disabled

barshaul · 2024-01-23T14:31:18Z

python/python/tests/test_config.py



 def test_default_client_config():
    config = BaseClientConfiguration([])
    assert len(config.addresses) == 0
    assert config.read_from.value == ProtobufReadFrom.Primary
-    assert config.use_tls is False
+    assert config.tls_mode is None


should be TlsMode.Disabled

Use non-boolean enums for TLS mode.

657e71c

This allows us forward compatibility, so that once we introduce more complex TLS usage schemes, such as loading specific credentials, we won't break the API.

nihohit requested a review from a team as a code owner January 17, 2024 17:29

shachlanAmazon requested review from barshaul and ikolomi January 17, 2024 17:35

barshaul reviewed Jan 21, 2024

View reviewed changes

barshaul requested changes Jan 21, 2024

View reviewed changes

Added Disabled setting to enum.

1cfb15e

barshaul requested changes Jan 23, 2024

View reviewed changes

Yury-Fridlyand added python node labels Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use non-boolean enums for TLS mode. #816

Use non-boolean enums for TLS mode. #816

nihohit commented Jan 17, 2024

barshaul Jan 21, 2024

barshaul Jan 21, 2024 •

edited

shachlanAmazon Jan 22, 2024

shachlanAmazon Jan 22, 2024

barshaul Jan 21, 2024

shachlanAmazon Jan 22, 2024

barshaul Jan 22, 2024

barshaul left a comment

shachlanAmazon commented Jan 22, 2024

barshaul commented Jan 22, 2024

shachlanAmazon commented Jan 22, 2024

barshaul left a comment

barshaul Jan 22, 2024

barshaul Jan 22, 2024

barshaul Jan 22, 2024

barshaul Jan 22, 2024

barshaul Jan 22, 2024

barshaul Jan 22, 2024

barshaul Jan 22, 2024

barshaul Jan 23, 2024

		@@ -51,6 +51,14 @@ class ProtocolVersion(Enum):
		"""


		class TlsMode(Enum):

Use non-boolean enums for TLS mode. #816

Are you sure you want to change the base?

Use non-boolean enums for TLS mode. #816

Conversation

nihohit commented Jan 17, 2024

Choose a reason for hiding this comment

barshaul Jan 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul left a comment

Choose a reason for hiding this comment

shachlanAmazon commented Jan 22, 2024

barshaul commented Jan 22, 2024

shachlanAmazon commented Jan 22, 2024

barshaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Jan 21, 2024 •

edited