HTTPS storage furls #1189

2022-03-25T19:48:31Z

itamarst commented

2022-03-25 19:48:31 +00:00

(Migrated from github.com)

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3875

Probably ready for preliminary review, even if not quite to be merged.

Open issues, feedback from reviewer welcome:

How to test private key that doesn't match certificate (moved to anotehr ticket)
Licensing of Foolscap code I copied in (I asked @warner if he would relicense: https://github.com/warner/foolscap/issues/91)
Am I missing some other tests that need to be done?

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3875 Probably ready for preliminary review, even if not quite to be merged. Open issues, feedback from reviewer welcome: * [x] How to test private key that doesn't match certificate (moved to anotehr ticket) * [x] Licensing of Foolscap code I copied in (I asked @warner if he would relicense: https://github.com/warner/foolscap/issues/91) * [x] Am I missing some other tests that need to be done?

exarkun (Migrated from github.com) reviewed 2022-03-26 18:53:36 +00:00

exarkun (Migrated from github.com) left a comment

Thanks! I did a rough first pass and left some comments inline. Probably will spend some more time looking on Monday.

src/allmydata/storage/http_client.py

exarkun (Migrated from github.com) commented

2022-03-26 17:04:34 +00:00

Comment copied from Foolscap, right? But I don't think OpenSSL has a sense of humor and it just calls this function for every certificate presented by the peer (including those in the chain).

exarkun (Migrated from github.com) commented

2022-03-26 17:22:19 +00:00

Is this app data used by anything?

exarkun (Migrated from github.com) commented

2022-03-26 17:25:02 +00:00

Regarding naming, I suppose we could start to try to be consistent and refer to these as NURLs instead of FURLs. See https://tahoe-lafs.readthedocs.io/en/latest/specifications/url.html#nurls

exarkun (Migrated from github.com) commented

2022-03-26 17:56:27 +00:00

Playing a bit with hyperlink, it looks like the Twisted endpoint style URLs don't work. eg DecodedURL.from_text won't parse pb://1WUX44xKjKdpGLohmFcBNuIRN-8rlv1Iij_7rQ@tcp:127.1:34399/jhjbc3bjbhk#v=1. The extra : in the netloc makes it angry.

I'd almost say we can just drop this but this is how Tor and I2P are supported. A NURL for a Tor server has a netloc of tor:u33m4y7klhz3b.onion:1000 and for I2P it looks like i2p:c2ng2pbrmxmlwpijn.

I'm not sure what to do about this. Options that suggest themselves to me are ...

Don't use hyperlink to parse, represents, and serialize NURLs (😢 )
Change the grammar of NURLs so it works

I guess url-encoding the endpoint might fix it? except I don't know if that is a sensible thing to do to the netloc part of a URL.

Apart from this grammar/parsing problem I really like this function and how it fits in to the rest of StorageClient. I like how it is just an alternate constructor. I like how all of the certificate validation logic ends up bundled inside the underlying http client object. I like how it is all a self-contained transformation from a structured representation of a NURL (hopefully as a DecodedURL) to a different structured representation as a StorageClient.

Playing a bit with hyperlink, it looks like the Twisted endpoint style URLs don't work. eg `DecodedURL.from_text` won't parse `pb://1WUX44xKjKdpGLohmFcBNuIRN-8rlv1Iij_7rQ@tcp:127.1:34399/jhjbc3bjbhk#v=1`. The extra `:` in the netloc makes it angry. I'd almost say we can just drop this but this is how Tor and I2P are supported. A NURL for a Tor server has a netloc of `tor:u33m4y7klhz3b.onion:1000` and for I2P it looks like `i2p:c2ng2pbrmxmlwpijn`. I'm not sure what to do about this. Options that suggest themselves to me are ... 1. Don't use hyperlink to parse, represents, and serialize NURLs (:cry: ) 2. Change the grammar of NURLs so it works I _guess_ url-encoding the endpoint might fix it? except I don't know if that is a sensible thing to do to the netloc part of a URL. Apart from this grammar/parsing problem I really like this function and how it fits in to the rest of `StorageClient`. I like how it is just an alternate constructor. I like how all of the certificate validation logic ends up bundled inside the underlying http client object. I like how it is all a self-contained transformation from a structured representation of a NURL (hopefully as a DecodedURL) to a different structured representation as a StorageClient.

src/allmydata/storage/http_client.py

						
				@ -113,0 +168,4 @@

				            # the other end. OpenSSL calls it multiple times, for each errno

				            # for each certificate.

				            # We do not care about certificate authorities or revocation

exarkun (Migrated from github.com) commented

2022-03-26 17:12:43 +00:00

For your consideration:

>>> from cryptography.hazmat.bindings.openssl.binding import Binding
>>> b = Binding()
>>> [getattr(b.lib, "X509_V_ERR_" + s) for s in ["CERT_NOT_YET_VALID", "CERT_HAS_EXPIRED", "DEPTH_ZERO_SELF_SIGNED_CERT", "SELF_SIGNED_CERT_IN_CHAIN"]]
[9, 10, 18, 19]

I believe this is a public API, though cryptography.hazmat's documentation:

This is a "Hazardous Materials" module. You should ONLY use it if you're
100% absolutely sure that you know what you're doing because this module
is full of land mines, dragons, and dinosaurs with laser guns.

For your consideration: ``` >>> from cryptography.hazmat.bindings.openssl.binding import Binding >>> b = Binding() >>> [getattr(b.lib, "X509_V_ERR_" + s) for s in ["CERT_NOT_YET_VALID", "CERT_HAS_EXPIRED", "DEPTH_ZERO_SELF_SIGNED_CERT", "SELF_SIGNED_CERT_IN_CHAIN"]] [9, 10, 18, 19] ``` I believe this is a public API, though `cryptography.hazmat`'s documentation: > This is a "Hazardous Materials" module. You should ONLY use it if you're > 100% absolutely sure that you know what you're doing because this module > is full of land mines, dragons, and dinosaurs with laser guns.

src/allmydata/storage/http_common.py

						
				@ -9,1 +10,4 @@

				from cryptography.x509 import Certificate

				from cryptography.hazmat.primitives.serialization import Encoding, PublicFormat

				from werkzeug.http import parse_options_header

exarkun (Migrated from github.com) commented

2022-03-26 17:30:42 +00:00

Ahhh hello hazmat my old friend.

src/allmydata/storage/http_common.py

exarkun (Migrated from github.com) commented

2022-03-26 18:07:05 +00:00

The GBS spec calls for "urlsafe-base64 (without padding)" here. I think I believed that was necessary to be able to represent NURLs using standard URL-shaped tools. This hash ends up in the userinfo part of the URI and it looks like percent encoding is allowed there (https://datatracker.ietf.org/doc/html/rfc3986#section-3.2.1) so it seems like it is not necessary to use urlsafe-base64 to play nicely with other tools.

I wonder if it still might be preferable to avoid ever having a hash with percent encoding in. Maybe it would be slightly annoying to sometimes have to deal with such a hash "by hand" (ie, sometimes naive copy/paste will give you a bogus string and maybe you won't always immediately notice)? The flip side being that sometimes if you forget to handle the hash using the right base64 flavor you will have decoding problems.

So ... maybe it is a wash.

The GBS spec calls for "urlsafe-base64 (without padding)" here. I think I believed that was necessary to be able to represent NURLs using standard URL-shaped tools. This hash ends up in the userinfo part of the URI and it looks like percent encoding is allowed there (https://datatracker.ietf.org/doc/html/rfc3986#section-3.2.1) so it seems like it is _not_ *necessary* to use urlsafe-base64 to play nicely with other tools. I wonder if it still might be preferable to avoid ever having a hash with percent encoding in. Maybe it would be slightly annoying to sometimes have to deal with such a hash "by hand" (ie, _sometimes_ naive copy/paste will give you a bogus string and maybe you won't always immediately notice)? The flip side being that sometimes if you forget to handle the hash using the right base64 flavor you will have decoding problems. So ... maybe it is a wash.

src/allmydata/storage/http_server.py

exarkun (Migrated from github.com) commented

2022-03-26 18:20:01 +00:00

Dang. Too bad there still is no decent Python API for constructing a TLS server endpoint. :/

src/allmydata/test/test_storage_https.py

exarkun (Migrated from github.com) commented

2022-03-26 18:24:09 +00:00

There's also allmydata.crypto.rsa.create_signing_keypair which does the same thing.

There's also `allmydata.crypto.rsa.create_signing_keypair` which does the same thing.

exarkun (Migrated from github.com) commented

2022-03-26 18:26:05 +00:00

My guess is that it's necessary because even with non-persistent connections, Agent still fires application-code-facing Deferreds before it finishes closing its connections. It's probably necessary for IStorageServer HTTPS tests too but they get lucky more often (or they got persistently lucky once and some other aspect of cleanup lets the reactor tick just enough for those connections to end).

I have a similar work-around in a test fixture managing a Treq instance - https://github.com/PrivateStorageio/ZKAPAuthorizer/blob/main/src/_zkapauthorizer/tests/fixtures.py#L195-L205

:(

My guess is that it's necessary because even with non-persistent connections, `Agent` still fires application-code-facing Deferreds before it finishes closing its connections. It's probably necessary for IStorageServer HTTPS tests too but they get lucky more often (or they got persistently lucky once and some other aspect of cleanup lets the reactor tick just enough for those connections to end). I have a similar work-around in a test fixture managing a Treq instance - https://github.com/PrivateStorageio/ZKAPAuthorizer/blob/main/src/_zkapauthorizer/tests/fixtures.py#L195-L205 :(

exarkun (Migrated from github.com) commented

2022-03-26 18:52:44 +00:00

Hm. So, the way the TLS handshake goes is ... (a bunch of stuff) ... then the client sends a "ClientKeyExchange" message which has a "PreMasterSecret" in it, encrypted using the public key from the server's certificate. The server must decrypt this and do a cipher-specific operation to derive the "MasterSecret". Later the server sends its "Finished" message encrypted using "MasterSecret". The client decrypts it to extract a hash and a MAC of all of the handshake messages up until this point.

Without the private key corresponding to the certificate it presents, the server must compute the wrong "MasterSecret" and when the client decrypts the "Finished" message with the correct "MasterSecret" the hash and MAC will mismatch and the handshake will fail.

So ... a test for "the server is using the wrong private key" seems like it would mostly be a test for the underlying TLS implementation (which, at worst, would be able to deliver garbage data to the client instead of properly signaling a handshake failure).

However, I probably wouldn't mind seeing a test for this case if it were possible to write one since perhaps some TLS libraries offer some ways to choose ciphers so poorly that you somehow lose the above protection (I think this would have to be roughly a choice of the NULL cipher everywhere which would be a pretty bad mistake, but ...).

That said, I don't really know how we'd write this test, given OpenSSL's behavior. My only idea is ... find a different TLS implementation that lets you do something so obviously illegal?

It's probably fine to defer any such work to a follow-up ticket though.

Hm. So, the way the TLS handshake goes is ... (a bunch of stuff) ... then the client sends a "ClientKeyExchange" message which has a "PreMasterSecret" in it, encrypted using the public key from the server's certificate. The server must decrypt this and do a cipher-specific operation to derive the "MasterSecret". Later the server sends its "Finished" message encrypted using "MasterSecret". The client decrypts it to extract a hash and a MAC of all of the handshake messages up until this point. Without the private key corresponding to the certificate it presents, the server must compute the wrong "MasterSecret" and when the client decrypts the "Finished" message with the correct "MasterSecret" the hash and MAC will mismatch and the handshake will fail. So ... a test for "the server is using the wrong private key" seems like it would mostly be a test for the underlying TLS implementation (which, at worst, would be able to deliver garbage data to the client instead of properly signaling a handshake failure). However, I probably wouldn't mind seeing a test for this case if it were possible to write one since perhaps some TLS libraries offer some ways to choose ciphers so poorly that you somehow lose the above protection (I *think* this would have to be roughly a choice of the NULL cipher everywhere which would be a pretty bad mistake, but ...). That said, I don't really know how we'd write this test, given OpenSSL's behavior. My only idea is ... find a different TLS implementation that lets you do something so obviously illegal? It's probably fine to defer any such work to a follow-up ticket though.

meejah (Migrated from github.com) reviewed 2022-03-26 20:16:50 +00:00

src/allmydata/storage/http_server.py

meejah (Migrated from github.com) commented

2022-03-26 20:16:50 +00:00

besides SSL4ServerEndpoint(...)...?
(maybe this provides more inspiration? https://github.com/crossbario/crossbar/blob/master/crossbar/common/twisted/endpoint.py#L107 )

besides `SSL4ServerEndpoint(...)`...? (maybe this provides more inspiration? https://github.com/crossbario/crossbar/blob/master/crossbar/common/twisted/endpoint.py#L107 )

twm (Migrated from github.com) reviewed 2022-03-26 20:21:54 +00:00

src/allmydata/storage/http_server.py

twm (Migrated from github.com) commented

2022-03-26 20:21:54 +00:00

Sorry, I lost the plot here: https://twistedmatrix.com/trac/ticket/9885

exarkun (Migrated from github.com) reviewed 2022-03-27 15:03:43 +00:00

src/allmydata/storage/http_server.py

exarkun (Migrated from github.com) commented

2022-03-27 15:03:43 +00:00

besides SSL4ServerEndpoint(...)...?

Right, besides that. If that were the good Python API I imagine then I suppose it would be easier to use it than to build up a string and then parse it with serverFromString.

> besides SSL4ServerEndpoint(...)...? Right, besides that. If that were the good Python API I imagine then I suppose it would be easier to use it than to build up a string and then parse it with `serverFromString`.

exarkun (Migrated from github.com) reviewed 2022-03-27 15:13:09 +00:00

src/allmydata/storage/http_server.py

exarkun (Migrated from github.com) commented

2022-03-27 15:13:09 +00:00

Sorry, I lost the plot here: https://twistedmatrix.com/trac/ticket/9885

Me too. I couldn't find the mailing list post that ticket used to reference. Since wrapClientTLS is an API that makes TLS composable with other endpoints, maybe twisted:9885 is about a wrapServerTLS that works similarly?

That would be nice, certainly, and would let listen_tcp have a signature that accepts another endpoint instead of a hostname / port number (or would at least remove one barrier to doing this - the other being that the code still needs a way to describe whatever endpoint it is listening on, which is not possible for an arbitrary endpoint, at least if you actually adhere to the interface).

> Sorry, I lost the plot here: https://twistedmatrix.com/trac/ticket/9885 Me too. I couldn't find the mailing list post that ticket used to reference. Since `wrapClientTLS` is an API that makes TLS composable with other endpoints, maybe twisted:9885 is about a `wrapServerTLS` that works similarly? That would be nice, certainly, and would let `listen_tcp` have a signature that accepts another endpoint instead of a hostname / port number (or would at least remove one barrier to doing this - the other being that the code still needs a way to describe whatever endpoint it is listening on, which is not possible for an arbitrary endpoint, at least if you actually adhere to the interface).

twm (Migrated from github.com) reviewed 2022-03-28 03:26:06 +00:00

src/allmydata/storage/http_server.py

twm (Migrated from github.com) commented

2022-03-28 03:26:06 +00:00

Ah, good point. From memory the gist is https://github.com/glyph/txsni/blob/master/txsni/tlsendpoint.py + write a strports plugin.

twm (Migrated from github.com) reviewed 2022-03-28 03:34:52 +00:00

src/allmydata/storage/http_server.py

twm (Migrated from github.com) commented

2022-03-28 03:34:52 +00:00

I dug it up, linked and quoted on the ticket now.

itamarst (Migrated from github.com) reviewed 2022-03-28 15:23:15 +00:00

src/allmydata/storage/http_client.py

itamarst (Migrated from github.com) commented

2022-03-28 15:23:15 +00:00

It's called multiple times for the same certificate, though, once per errno?

exarkun (Migrated from github.com) reviewed 2022-03-28 15:27:22 +00:00

src/allmydata/storage/http_client.py

exarkun (Migrated from github.com) commented

2022-03-28 15:27:21 +00:00

Oh yea, that's probably true - keeping in mind that "no error" is one of the errnos (so - it doesn't skip any certificates).

Oh yea, that's probably true - keeping in mind that "no error" is one of the errnos (so - it doesn't *skip* any certificates).

itamarst (Migrated from github.com) reviewed 2022-03-28 15:27:50 +00:00

src/allmydata/storage/http_client.py

						
				@ -113,0 +168,4 @@

				            # the other end. OpenSSL calls it multiple times, for each errno

				            # for each certificate.

				            # We do not care about certificate authorities or revocation

itamarst (Migrated from github.com) commented

2022-03-28 15:27:50 +00:00

OK, switched to that.

itamarst (Migrated from github.com) reviewed 2022-03-28 15:28:31 +00:00

src/allmydata/storage/http_client.py

itamarst (Migrated from github.com) commented

2022-03-28 15:28:30 +00:00

Apparently not.

itamarst (Migrated from github.com) reviewed 2022-03-28 15:36:02 +00:00

src/allmydata/storage/http_client.py

itamarst (Migrated from github.com) commented

2022-03-28 15:36:02 +00:00

Done.

itamarst (Migrated from github.com) reviewed 2022-03-28 15:38:40 +00:00

src/allmydata/storage/http_client.py

itamarst (Migrated from github.com) commented

2022-03-28 15:38:40 +00:00

The way I've seen this handled elsewhere is by changing the protocol, so e.g.:

pb+https://1WUX44xKjKdpGLohmFcBNuIRN-8rlv1Iij_7rQ@127.1:34399/jhjbc3bjbhk#v=1
pb+http://1WUX44xKjKdpGLohmFcBNuIRN-8rlv1Iij_7rQ@127.1:34399/jhjbc3bjbhk#v=1
pb+tor+https://1WUX44xKjKdpGLohmFcBNuIRN-8rlv1Iij_7rQ@127.1:34399/jhjbc3bjbhk#v=1
pb+i2p://1WUX44xKjKdpGLohmFcBNuIRN-8rlv1Iij_7rQ@sdsdfsdfds/jhjbc3bjbhk#v=1

or something like that.

Seems like a new ticket?

The way I've seen this handled elsewhere is by changing the protocol, so e.g.: * `pb+https://1WUX44xKjKdpGLohmFcBNuIRN-8rlv1Iij_7rQ@127.1:34399/jhjbc3bjbhk#v=1` * `pb+http://1WUX44xKjKdpGLohmFcBNuIRN-8rlv1Iij_7rQ@127.1:34399/jhjbc3bjbhk#v=1` * `pb+tor+https://1WUX44xKjKdpGLohmFcBNuIRN-8rlv1Iij_7rQ@127.1:34399/jhjbc3bjbhk#v=1` * `pb+i2p://1WUX44xKjKdpGLohmFcBNuIRN-8rlv1Iij_7rQ@sdsdfsdfds/jhjbc3bjbhk#v=1` or something like that. Seems like a new ticket?

itamarst (Migrated from github.com) reviewed 2022-03-28 15:41:25 +00:00

src/allmydata/storage/http_common.py

itamarst (Migrated from github.com) commented

2022-03-28 15:41:25 +00:00

I guess I'm not using URL-safe base64 variant here, at minimum... should I do that, or do you want to switch to % encoding instead?

itamarst (Migrated from github.com) reviewed 2022-03-28 15:42:00 +00:00

src/allmydata/storage/http_server.py

itamarst (Migrated from github.com) commented

2022-03-28 15:42:00 +00:00

I'm just going to live with this for now, since it works.

itamarst (Migrated from github.com) reviewed 2022-03-28 15:43:45 +00:00

src/allmydata/test/test_storage_https.py

itamarst (Migrated from github.com) commented

2022-03-28 15:43:45 +00:00

OK.

itamarst (Migrated from github.com) reviewed 2022-03-28 15:44:22 +00:00

src/allmydata/test/test_storage_https.py

itamarst (Migrated from github.com) commented

2022-03-28 15:44:21 +00:00

Just going to leave this here, feels like test code is OK to be a bit duplicative because we might want to tweak it in various ways.

itamarst (Migrated from github.com) reviewed 2022-03-28 15:47:16 +00:00

src/allmydata/test/test_storage_https.py

itamarst (Migrated from github.com) commented

2022-03-28 15:47:16 +00:00

perhaps some TLS libraries offer some ways to choose ciphers so poorly that you somehow lose the above protection

This logic, as a reminder, is client side. And the test would be of the client's behavior. So I'm not sure this test adds very much over .. auditing we've configured the correct ciphers?

> perhaps some TLS libraries offer some ways to choose ciphers so poorly that you somehow lose the above protection This logic, as a reminder, is client side. And the test would be of the client's behavior. So I'm not sure this test adds very much over .. auditing we've configured the correct ciphers?

itamarst (Migrated from github.com) reviewed 2022-03-28 15:50:07 +00:00

src/allmydata/test/test_storage_https.py

itamarst (Migrated from github.com) commented

2022-03-28 15:50:07 +00:00

https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3884

itamarst (Migrated from github.com) reviewed 2022-03-28 16:04:30 +00:00

src/allmydata/test/test_istorageserver.py

						
				@ -1083,0 +1066,4 @@

				        # Listen on randomly assigned port, using self-signed cert:

				        private_key = generate_private_key()

				        certificate = generate_certificate(private_key)

				        _, endpoint_string = self._port_assigner.assign(reactor)

itamarst (Migrated from github.com) commented

2022-03-28 16:04:30 +00:00

This is causing tests to fail, since the two files don't end up installed with the package. Perhaps should make the cert generating utility from test_storage_https reusable, and just generate new cert each time.

This is causing tests to fail, since the two files don't end up installed with the package. Perhaps should make the cert generating utility from `test_storage_https` reusable, and just generate new cert each time.

itamarst (Migrated from github.com) reviewed 2022-03-28 16:05:16 +00:00

src/allmydata/storage/http_client.py

itamarst (Migrated from github.com) commented

2022-03-28 16:05:16 +00:00

Is there a guarantee it will be called at least once?

exarkun (Migrated from github.com) reviewed 2022-04-02 14:08:43 +00:00

exarkun (Migrated from github.com) left a comment

Thanks! Some more comments inline - nothing particularly major, though there's some stuff about determinism and reliability (esp in the test suite) that seems like it might need improvements in Twisted to fix in a really satisfying way. :(

setup.py

exarkun (Migrated from github.com) commented

2022-04-02 13:29:49 +00:00

There is also a MANIFEST.in ... does that need to be changed?

There is also a `MANIFEST.in` ... does that need to be changed?

src/allmydata/storage/http_client.py

						
				@ -125,0 +242,4 @@

				        Create a ``StorageClient`` for the given NURL.

				        ``persistent`` indicates whether to use persistent HTTP connections.

				        """

exarkun (Migrated from github.com) commented

2022-03-31 21:17:36 +00:00

reactor should be another parameter to from_nurl, probably?

`reactor` should be another parameter to `from_nurl`, probably?

src/allmydata/storage/http_common.py

exarkun (Migrated from github.com) commented

2022-03-31 21:20:20 +00:00

I think that if the rest of the code is using DecodedURL (or similar) correctly then % encoding will be applied there ... so I don't see anything that's wrong about the code now, apart from not following our spec. But I think I would prefer to see URL-safe base64 rather than letting the % encoding happen.

I think that if the rest of the code is using `DecodedURL` (or similar) correctly then % encoding will be applied there ... so I don't see anything that's wrong about the code now, apart from not following our spec. But I think I would prefer to see URL-safe base64 rather than letting the % encoding happen.

src/allmydata/storage/http_server.py

exarkun (Migrated from github.com) commented

2022-04-02 13:41:48 +00:00

For consistency, probably FilePath instead. If pathlib is better I'd get behind an effort to switch to that at some point but I think a mix of Path and FilePath is probably worse than either.

For consistency, probably `FilePath` instead. If `pathlib` is better I'd get behind an effort to switch to that at some point but I think a mix of `Path` and `FilePath` is probably worse than either.

src/allmydata/storage/http_server.py

exarkun (Migrated from github.com) commented

2022-03-31 21:26:24 +00:00

As implemented, it will fail sometimes, since Linux (at least) does not make binding to port 0 reliable.

I don't think I care much - and if anything, I wonder if we should get rid of the automatic port assignment "feature" from Tahoe altogether.

Maybe this layer needs to offer the feature so that GBS fits into the Foolscap-shaped hole we're filling, in which case this is fine. As we continue with the process, if we get to a point where we get to make a choice about whether we keep offering this or not, let's carefully consider whether we should.

As implemented, it will fail sometimes, since Linux (at least) does not make binding to port 0 reliable. I don't think I care much - and if anything, I wonder if we should get rid of the automatic port assignment "feature" from Tahoe altogether. Maybe this layer needs to offer the feature so that GBS fits into the Foolscap-shaped hole we're filling, in which case this is fine. As we continue with the process, if we get to a point where we get to make a choice about whether we keep offering this or not, let's carefully consider whether we should.

src/allmydata/test/test_istorageserver.py

exarkun (Migrated from github.com) commented

2022-03-31 21:27:52 +00:00

Ah, oops. This will fail intermittently, though. So either listen_tls needs retry logic for EADDRINUSE or this test code does.

If only we could wrap TLS around an arbitrary endpoint, we could keep using _port_assigner here...

Ah, oops. This will fail intermittently, though. So either `listen_tls` needs retry logic for `EADDRINUSE` or this test code does. If only we could wrap TLS around an arbitrary endpoint, we could keep using `_port_assigner` here...

src/allmydata/test/test_storage_https.py

exarkun (Migrated from github.com) commented

2022-04-02 13:54:04 +00:00

Separate cert_to_file and key_to_file returning FilePath would be slightly neater, I think.

Separate `cert_to_file` and `key_to_file` returning `FilePath` would be slightly neater, I think.

exarkun (Migrated from github.com) commented

2022-04-02 14:05:33 +00:00

I filed https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3887 which we could link to from here.

src/allmydata/test/test_storage_https.py

						
				@ -0,0 +84,4 @@

				    def not_async(*args, **kwargs):

				        return Deferred.fromCoroutine(f(*args, **kwargs))

				    return not_async

exarkun (Migrated from github.com) commented

2022-04-02 13:53:06 +00:00

I filed https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3886, might be nice to link to it from somewhere around here.

src/allmydata/test/test_storage_https.py

						
				@ -0,0 +117,4 @@

				        )

				        root = Data(b"YOYODYNE", "text/plain")

				        root.isLeaf = True

				        listening_port = await endpoint.listen(Site(root))

exarkun (Migrated from github.com) commented

2022-03-31 21:29:39 +00:00

This will intermittently fail with EADDRINUSE.

src/allmydata/test/test_storage_https.py

						
				@ -0,0 +145,4 @@

				        return treq_client.get(url)

				    @async_to_deferred

				    async def test_success(self):

exarkun (Migrated from github.com) commented

2022-04-02 13:33:04 +00:00

This failed on Windows CI with a dirty reactor error for <TLSMemoryBIOProtocol #0 on 54010>

This failed on Windows CI with a dirty reactor error for `<TLSMemoryBIOProtocol #0 on 54010>`

src/allmydata/test/test_storage_https.py

						
				@ -0,0 +178,4 @@

				                await self.request(url, certificate2)

				    @async_to_deferred

				    async def test_server_certificate_expired(self):

exarkun (Migrated from github.com) commented

2022-04-02 13:32:17 +00:00

This failed on Windows CI w/ a dirty reactor error for <TLSMemoryBIOProtocol #0 on 60408>

This failed on Windows CI w/ a dirty reactor error for `<TLSMemoryBIOProtocol #0 on 60408>`

src/allmydata/test/test_storage_https.py

						
				@ -0,0 +211,4 @@

				            cert_to_file(FilePath(self.mktemp()), certificate),

				        ) as url:

				            response = await self.request(url, certificate)

				            self.assertEqual(await response.content(), b"YOYODYNE")

exarkun (Migrated from github.com) commented

2022-04-02 13:27:12 +00:00

I guess one other test would be for the case where the certificate is not yet valid.

I guess one other test would be for the case where the certificate is not _yet_ valid.

itamarst (Migrated from github.com) reviewed 2022-04-06 15:36:40 +00:00

setup.py

itamarst (Migrated from github.com) commented

2022-04-06 15:36:40 +00:00

MANIFEST.in is post-processing after things like package_data have already been added, as per https://packaging.python.org/en/latest/guides/using-manifest-in/.

`MANIFEST.in` is post-processing after things like `package_data` have already been added, as per https://packaging.python.org/en/latest/guides/using-manifest-in/.

itamarst (Migrated from github.com) reviewed 2022-04-06 15:41:10 +00:00

src/allmydata/storage/http_client.py

itamarst (Migrated from github.com) commented

2022-04-06 15:41:10 +00:00

https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3888

itamarst commented

2022-04-06 15:54:52 +00:00

(Migrated from github.com)

Probably ready for another round of review.

coveralls commented

2022-04-06 16:14:42 +00:00

(Migrated from github.com)

Coverage increased (+0.03%) to 94.708% when pulling 4e0f912a10 on 3875-http-storage-furls into 0c18dca1ad on master.

[![Coverage Status](https://coveralls.io/builds/48291450/badge)](https://coveralls.io/builds/48291450) Coverage increased (+0.03%) to 94.708% when pulling **4e0f912a100bbee6e684fd830bb4d0510a2545e6 on 3875-http-storage-furls** into **0c18dca1adf742971dc62c14f38dd22dee273732 on master**.

exarkun (Migrated from github.com) approved these changes 2022-04-13 20:27:13 +00:00

exarkun (Migrated from github.com) left a comment

Thanks. This looks very good to me. Just one comment inline, and that not really actionable for this PR.

src/allmydata/test/test_storage_https.py

						
				@ -0,0 +216,4 @@

				    # A potential attack to test is a private key that doesn't match the

				    # certificate... but OpenSSL (quite rightly) won't let you listen with that

				    # so I don't know how to test that! See

				    # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3884

exarkun (Migrated from github.com) commented

2022-04-13 20:26:52 +00:00

One other error case that came to mind is the case of a server using a CA-issued certificate. I think this mostly overlaps with "has wrong hash" which is tested but I guess it might be nice to test separately just to be sure OpenSSL isn't somehow deciding to jump over all of our logic because it sees a signature from a well-known CA.

Of course, how to get a CA-issued certificate to use in the tests is an interesting question.

I don't consider this a blocker for this PR but it might be another case to track on 3884 or in another ticket.

One other error case that came to mind is the case of a server using a CA-issued certificate. I think this _mostly_ overlaps with "has wrong hash" which is tested but I guess it might be nice to test separately just to be sure OpenSSL isn't somehow deciding to jump over all of our logic because it sees a signature from a well-known CA. Of course, how to _get_ a CA-issued certificate to use in the tests is an interesting question. I don't consider this a blocker for this PR but it might be another case to track on 3884 or in another ticket.

Sign in to join this conversation.

No reviewers

exarkun