PEP 748: tlslib - configuration

[Julien00859]

First time contributor 🎉

A few suggestions to make the configuration a bit more explicit. I decided to leave most of the attributes undocumented as they are pretty explicit to me, and to instead only document the few attributes that are different from the client and the server.

PEP 748: ConfigurationError is only for unsupported features

Discussed at trailofbits/tlslib.py#72 (comment)

ConfigurationError was intended for when specific implementations don't support certain behavior (e.g. adding a certificate by identifier). I think ValueError should be fine, probably raised when validating the configuration. ~Joop

PEP 748: certificate_chain is mandatory server-side

Discussed at https://discuss.python.org/t/pre-pep-discussion-revival-of-pep-543-a-unified-tls-api-for-python/51263/75

This makes sense to me. We can allow empty server certificates in the insecure module. ~Joop

PEP 748: disambiguate config trust_store=None

Discussed at https://discuss.python.org/t/pre-pep-discussion-revival-of-pep-543-a-unified-tls-api-for-python/51263/78

I agree, this is better. ~Joop

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[read-the-docs-community]

Documentation build overview

📚 pep-previews | 🛠️ Build #32662161 | 📁 Comparing 89e8de4 against latest (c093d5f)

  🔍 Preview build  

709 files changed · ± 709 modified

± Modified

• index.html
• numerical/index.html
• pep-0000/index.html
• pep-0001/index.html
• pep-0002/index.html
• pep-0003/index.html
• pep-0004/index.html
• pep-0005/index.html
• pep-0006/index.html
• pep-0007/index.html
• and 699 more...

[hugovk]

cc PEP authors @jvdprng and @woodruffw.

[jvdprng]

Thanks for the PR! I left two suggestions.

[jvdprng]

On second thought, there is one additional thing we should change.

[jvdprng]

I remember now why we had None here before: we wanted to avoid creating a shared default object in the API which should be immutable/comparable. So I think we need something else here:

• A sentinel object (we deliberately chose not to do this IIRC)
• Do not define the default, and specify that a missing TrustStore should default to the system trust store. This is my preference.

[Julien00859]

Do not define the default, and specify that a missing TrustStore should default to the system trust store. This is my preference.

I don't understand, can you add an example?

[Julien00859]

We wanted to avoid creating a shared default object in the API which should be immutable/comparable.

I think there is room for a TrustStore.is_system(truststore) method.

[jvdprng]

Yeah, what I was thinking of does not work. I think a larger problem is that TrustStore objects are currently not immutable. I would propose to make them immutable by freezing them (which means I need to fix some path handling in the stdlib, but that's probably fine) which would make it fine to assign the system trust store as a default parameter.

We could then consider making the configs frozen as well. What do you think?

[Julien00859]

Making the class immutable means that implementations that require one of _path or _buffer can't have the following:

# example for an implementation that doesn't support paths
def load(trust_store):
    if trust_store._buffer:
        return
    if trust_store._path:
        with open(trust_store._path, ...) as file:
            trust_store._buffer = file.read()
        return
    raise ValueError("TrustStore.system() is not supported")

which is a pattern that is implemented in both tlslib.stdlib (buffer->path) and siotls (path->buffer).

I think we need to discuss the three TrustStore, Certificate and PrivateKey APIs more¹². For the time being, I suggest we add a TrustStore.is_system() method to solve the problem at hand and have the discussion about immutability and path->buffer/buffer->path elsewhere.

    def is_system(self):
        return self._buffer is None and self._path is None and self._id is None

+1 to have the configuration immutable. It already is a frozen dataclass in siotls. I'll push a commit to specify it in the spec.

Footnotes

1. https://discuss.python.org/t/pre-pep-discussion-revival-of-pep-543-a-unified-tls-api-for-python/51263/71: § Certificate Chain, Certificate Revocation List ↩

2. https://discuss.python.org/t/pre-pep-discussion-revival-of-pep-543-a-unified-tls-api-for-python/51263/71: § Save DER in-place ↩

[Julien00859]

No need to change the spec, it already says that the configuration objects are immutable (emphasis mine):

For this reason, we split the responsibility of SSLContext into two separate objects, which are each split into server and client versions. The TLSServerConfiguration and TLSClientConfiguration objects act as containers for a TLS configuration: the ClientContext and ServerContext objects are instantiated with a TLSClientConfiguration and TLSServerConfiguration object, respectively, and are used to create buffers or sockets. All four objects would be immutable.