Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

interface: disable ssl.VERIFY_X509_STRICT for self-signed certs #9258

Merged
merged 1 commit into from
Oct 20, 2024

Conversation

SomberNight
Copy link
Member

The ssl.VERIFY_X509_STRICT flag for openssl verification has been enabled by default in python 3.13+ (it was disabled before that). see python/cpython#107361 and https://discuss.python.org/t/ssl-changing-the-default-sslcontext-verify-flags/30230/16

We explicitly disable it for self-signed certs, thereby restoring the pre-3.13 defaults, as it seems to break lots of servers.

For example, using python 3.13 (or setting sslc.verify_flags |= ssl.VERIFY_X509_STRICT),

  • I can connect to btc.electroncash.dk:60002:s
  • but not to electrum.emzy.de:50002:s

despite both using self-signed certs.

We should investigate more why exactly "strict" verification fails for some self-signed certs and not for others, and make sure that at least newly generated certs adhere to the stricter requirements (e.g. update guide in e-x?).

The "ssl.VERIFY_X509_STRICT" flag for openssl verification has been enabled by default in python 3.13+ (it was disabled before that).
see python/cpython#107361
and https://discuss.python.org/t/ssl-changing-the-default-sslcontext-verify-flags/30230/16

We explicitly disable it for self-signed certs, thereby restoring the pre-3.13 defaults,
as it seems to break lots of servers.

For example, using python 3.13 (or setting `sslc.verify_flags |= ssl.VERIFY_X509_STRICT`),
- I can connect to `btc.electroncash.dk:60002:s`
- but not to `electrum.emzy.de:50002:s`
despite both using self-signed certs.

We should investigate more why exactly "strict" verification fails for some self-signed certs and not for others,
and make sure that at least newly generated certs adhere to the stricter requirements (e.g. update guide in e-x?).
@SomberNight SomberNight added the topic-network 🕸 related to logic in network.py (etc) label Oct 18, 2024
@SomberNight
Copy link
Member Author

This cert successfully verifies with ssl.VERIFY_X509_STRICT:

cert for btc.electroncash.dk:60002:s
-----BEGIN CERTIFICATE-----
MIIFHTCCAwWgAwIBAgIUPqsaiqD5Oco7gIaJ5c63dtExBmAwDQYJKoZIhvcNAQEL
BQAwHjEcMBoGA1UEAwwTYnRjLmVsZWN0cm9uY2FzaC5kazAeFw0xOTA5MTMxMjU3
MjVaFw0yOTA5MTAxMjU3MjVaMB4xHDAaBgNVBAMME2J0Yy5lbGVjdHJvbmNhc2gu
ZGswggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoICAQC/vqkOX6DoGjqQRezp
r3jUpMXiNOpdJN3Lxdtnzcp4Wwf3I53qaP/HQmM6Oa+9zKAud/eRrk8kgpmdsC/q
27SiLZYnwiBiYrHCS+geNcXkVy6jDfm3qk2pHGtzoriSYj+3QJ4kJv+g2NZXWfzq
+5qCWsUEv8i9am2V2ZdfJc6yyx3UGH8ZMgsBf4LNoW0GU1n9nsCOGof2D2W2U110
9zVBifXD9pRZMtfJeY6g4Yq95JyveKhRBsO7UvYuWnrMZBuLQtEco8R2014jJqhY
yESt0EqVscwOGzJdqzWIOMUE/ToJ3ugl4cjShzlcgxICskoHs0V8CzC+YTYcH4f+
BYoOi3+esdFnJlQHsNPcP8Yr4418hBgLGAZ35JnxEsLj/BUQ3quTIwUdqgeZdiak
JJG4vPws5/TgGPL0nQgWBU/uG8YLTmXTSU2ikyJnr0CQ6rX4+B8CVb7TNAuWj/zy
/RqajEtvLn5Ws+zkxQwEQENSNAV8XIkxfk99la2sw501AwUScnk0JLO0l3Jdek+q
8R/lIPQ0QLapH5hQ+pZL++DS5xyOgisYlcxbavZ3/2NUBMxOXHnR2OWHL50XqPeJ
NGO2L+lkcMmZ4txPDwlIrG+0+l2zp13S5acUYjgGTOVikg5OyOoqFaUcsPOtIaVa
vl5nYkSP6uPQ6pEB2mMONaw2oQIDAQABo1MwUTAdBgNVHQ4EFgQUUoGBsejmUqwG
klDdqoSrSr3LucowHwYDVR0jBBgwFoAUUoGBsejmUqwGklDdqoSrSr3LucowDwYD
VR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAgEABNpwqDevWBuJXYY21Fo5
4NxtJWUZiaaa9vbWRbZ+a2k3f9qul7ZG2Hdjb8aSJfiBm7uJrtWwhb3Mxjqp9XGQ
dGUPsd0DCMLt4BRwMuOpXtLWSuuwb35jrmLrdQZ8FOdGWLhJGn52UaOlppdpyFlH
hH6jIfQE4hnGxgEBQ8lXOycL4IVvBCagBxCE3vX9L2eTjv+PWg5S9J2qhA/RC2dd
CQzLljYqYrMuXD/E4SNh3f4zwP2F+6jn2vg8Ii6Zq58OrW/a0SVflAuT7Pljx6wm
tDh55xEmNGBfQCtsj079BxfFMEE0czKmjus1JHmHhCXdD7sfJlz4vbBx4LFJhfs9
2UjMQzsCBO8N3K+jxEGGum+GLjpfX/vdXgao0TUpfkdnCPr7WO5tw4lSe1RAUqac
yw9hWBCNvW/U9qAPSd8VqrrGntXnf1jzdzsg5ZlzeahiMNQyTj+T0pA+uuj9+yjn
chmWBzCXW7qrzASAbecWNsnp6dIHzLFUVcg2GrwcRnnrH1aFfEkp3aH/xFVD0V1R
Q1TeNWBqQpTEPrqqeDsUMozhfCmRiQEfdQrNvHDFHAMviv5ORW++ok+mh89Brz4R
wtI08nX4tEbrR+eGZ49qykSIVKpRPNfmMiPOm627ZD+mkC/HNabSOFAD40Gwr/gk
ih83Ma39hS7PnFYXJt5retQ=
-----END CERTIFICATE-----

This cert fails to verify with ssl.VERIFY_X509_STRICT:

cert for electrum.emzy.de:50002:s
-----BEGIN CERTIFICATE-----
MIIDnzCCAocCFGw8AU6bWkI4JTKj/TvtS6nuHdiiMA0GCSqGSIb3DQEBCwUAMIGL
MQswCQYDVQQGEwJERTEVMBMGA1UECAwMTmllZGVyc2FjaGVuMRkwFwYDVQQKDBBl
bGVjdHJ1bS5lbXp5LmRlMRIwEAYDVQQLDAllbGVjdHJ1bXgxGTAXBgNVBAMMEGVs
ZWN0cnVtLmVtenkuZGUxGzAZBgkqhkiG9w0BCQEWDGVtenlAZW16eS5kZTAeFw0y
MTEwMTIyMTA2MjdaFw0yNjEwMTEyMTA2MjdaMIGLMQswCQYDVQQGEwJERTEVMBMG
A1UECAwMTmllZGVyc2FjaGVuMRkwFwYDVQQKDBBlbGVjdHJ1bS5lbXp5LmRlMRIw
EAYDVQQLDAllbGVjdHJ1bXgxGTAXBgNVBAMMEGVsZWN0cnVtLmVtenkuZGUxGzAZ
BgkqhkiG9w0BCQEWDGVtenlAZW16eS5kZTCCASIwDQYJKoZIhvcNAQEBBQADggEP
ADCCAQoCggEBALn6g79JySAiT6D/OsDj2DP1yHxbOr5laxhHgWZsvTJjDbm7pYZ8
hPrFrhYZyF/tLrhZSdfCyWBTiINHwGhXTecYeL2oATVz/XQnMX1XzBn7/cQUGX1o
Sqw4wuAwifNp9yvkOEaqSv8kvRiGSHKqxt68RCwWfDR1TyF73ltD52oBnkN9TQ3T
gKvTASoikXJnynpuXWlBKP8TOJ07frTk8ZebB4nHUEgnjx13sDF85pDP1cNazCWS
nk8Jy1+OE7KaRIuO311BXHbgdvgNcR0CIBPcGuAanNmaFLSigWsnnlNNgtkgRYFc
bQFk+mBEnXDz/LvqUe5QAwO0GTVFTSjLzDcCAwEAATANBgkqhkiG9w0BAQsFAAOC
AQEAlraiQDHY/knqRlij+O268yos3rOC+ARKzEDKbONc9EP5fOp+ld4+sd4gQVfW
/SsB+FKx0zCLNG1uCC9NJ0F5Ukbt+dJ8u0B1pKPWs6dId6zAQKk0+yl+DfZvoJ6R
tebT8iI1RVrvxyV488CqbTHOSKZGlJS8SiPkXzu0+Onyr/uNpayAKvwE/NhFbU3h
z8b3TQ9WNGli5Sslx1nMjO0at/gtbUk5DsXvUZnmy3wXmsylnVi7R6+troyuVmP8
CfsTNyoJPvgKQsSNoCRBIPjIx1eTPKQAaFwIu2Ckxo2cykBnTJLsZFKq7yhjjVZO
qiVb2Ts1gAR16RIdK1Ni0T5d5Q==
-----END CERTIFICATE-----

I am not sure what magic incantation openssl needs to tell me what the relevant difference between the two certs is.

@SomberNight SomberNight merged commit 3a2dfb3 into spesmilo:master Oct 20, 2024
16 checks passed
@SomberNight SomberNight added this to the 4.5.7 milestone Oct 20, 2024
@accumulator
Copy link
Member

I am not sure what magic incantation openssl needs to tell me what the relevant difference between the two certs is.

try cat|openssl x509 -text -noout and paste the cert.

the cert for btc.electroncash.dk is a selfsigned (root) CA cert, which apparently passes ssl.VERIFY_X509_STRICT.
Not sure why this is not validated against a trusted set of root CA's and considered more valid than a non-CA selfsigned cert.. looks like a bug?

@SomberNight
Copy link
Member Author

Ok, so I saved the two certs as files a and b.

$ openssl verify -CAfile a a
a: OK

$ openssl verify -CAfile b b
b: OK

$ openssl verify -x509_strict -CAfile a a
a: OK

$ openssl verify -x509_strict -CAfile b b
C = DE, ST = Niedersachen, O = electrum.emzy.de, OU = electrumx, CN = electrum.emzy.de, emailAddress = emzy@emzy.de
error 79 at 0 depth lookup: invalid CA certificate
error b: verification failed

@accumulator
Copy link
Member

accumulator commented Oct 21, 2024

well, with the -CAfile option you mark the CA as trusted, so openssl verify -x509_strict -CAfile a a above will then validate.

b is not a CA cert, so it is not marked as a trusted CA, so with x509_strict there is no path to a trusted root, and it will not validate.

Without -CAfile, openssl SHOULD fall back to openssl's default CA store (which likely is the system's store of trusted root CA's)

@accumulator
Copy link
Member

accumulator commented Oct 21, 2024

Another problem.. from the openssl-verification-options man page:

'CA certificates must explicitly include the keyUsage extension.'

There is no keyUsage record in a above, so it should fail a, even with -CAfile

@SomberNight
Copy link
Member Author

SomberNight commented Oct 21, 2024

well, with the -CAfile option you mark the CA as trusted, so openssl verify -x509_strict -CAfile a a above will of course validate.

interface.py accepts both public CA-signed certs and self-signed certs.
See https://github.com/spesmilo/electrum-docs/blob/6cdb612d8086aae2d93b47e06886913f1971a026/faq.rst#L497-L510
The problem I am trying to diagnose is that as python 3.13 started setting this new flag for openssl by default, many of the servers that use self-signed certs the client can no longer connect to. There is no issue for servers using public CA-signed certs.

I mentioned this command $ openssl verify -CAfile a a because it seems to replicate the behaviour I am seeing for interface.py (so using the python interface to openssl).
If you look at the close-by lines of interface.py:

ca_sslc = ssl.create_default_context(purpose=ssl.Purpose.SERVER_AUTH, cafile=ca_path)

sslc = ssl.create_default_context(purpose=ssl.Purpose.SERVER_AUTH, cafile=self.cert_path)

for the self-signed-cert-using servers, we are basically just setting cafile to a file that contains only the cert that the server sends us as-is. And this behaviour can apparently be replicated using the openssl verify -CAfile a a command. Additionally passing the -x509_strict replicates the new 3.13 behaviour.

@SomberNight
Copy link
Member Author

SomberNight commented Oct 21, 2024

Note:
https://github.com/spesmilo/electrumx/blob/92589103620abb6e4be7faf441c88012e1f8339c/docs/HOWTO.rst?plain=1#&L401-L410
electrumx docs recommend to use

$ openssl genrsa -out server.key 2048
$ openssl req -new -key server.key -out server.csr
$ openssl x509 -req -days 1825 -in server.csr -signkey server.key -out server.crt

This cert will fail -x509_strict.


I've found that the following cert passes -x509_strict:

$ openssl genrsa -out server.key 2048
$ openssl req -x509 -new -key server.key -out server.crt -days 1825

try cat|openssl x509 -text -noout and paste the cert.

Thanks, that was very useful.

I think the difference is due to the presence of the X509v3 extensions of that output.
It does not seem to matter whether it says CA:FALSE or CA:TRUE there, but the field needs to be present.

For example, the cert generated by the following snippet also validates with strict:

$ openssl genrsa -out server.key 2048
$ openssl req -x509 -new -nodes -key server.key -out server.crt    \
  -days 1825                                                       \
  -subj '/CN=random_stuff'                                         \
  -addext 'subjectAltName = DNS:localhost'                         \
  -addext 'authorityKeyIdentifier = keyid,issuer'                  \
  -addext 'basicConstraints = CA:FALSE'                            \
  -addext 'keyUsage = digitalSignature, keyEncipherment'           \
  -addext 'extendedKeyUsage=serverAuth'

@accumulator
Copy link
Member

accumulator commented Oct 21, 2024

well, with the -CAfile option you mark the CA as trusted, so openssl verify -x509_strict -CAfile a a above will of course validate.

interface.py accepts both public CA-signed certs and self-signed certs. See https://github.com/spesmilo/electrum-docs/blob/6cdb612d8086aae2d93b47e06886913f1971a026/faq.rst#L497-L510 The problem I am trying to diagnose is that as python 3.13 started setting this new flag for openssl by default, many of the servers that use self-signed certs the client can no longer connect to. There is no issue for servers using public CA-signed certs.

Yes, this is exactly as expected. My point is that normal self-signed certificates are NOT CA's. btc.electroncash.dk is the weird one here, declaring itself as a CA. This is unusual for self-signed certificates.
Edit: note that your second command openssl req -x509 -new -key server.key -out server.crt -days 1825 creates a CA cert, which is probably also what was used for btc.electroncash.dk

I mentioned this command $ openssl verify -CAfile a a because it seems to replicate the behaviour I am seeing for interface.py (so using the python interface to openssl). If you look at the close-by lines of interface.py:

ca_sslc = ssl.create_default_context(purpose=ssl.Purpose.SERVER_AUTH, cafile=ca_path)

sslc = ssl.create_default_context(purpose=ssl.Purpose.SERVER_AUTH, cafile=self.cert_path)

Yes, adding self-signed NON-CA certs to the cafile param does nothing for these certs when ssl.VERIFY_X509_STRICT is enabled.

for the self-signed-cert-using servers, we are basically just setting cafile to a file that contains only the cert that the server sends us as-is. And this behaviour can apparently be replicated using the openssl verify -CAfile a a command. Additionally passing the -x509_strict replicates the new 3.13 behaviour.

To allow self-signed certificates, we probably should only enforce ssl.VERIFY_X509_STRICT if the cert is not self-signed.

If the cert is self-signed CA we should discard it as invalid or consider it a regular non-CA.
Edit: as the command I mentioned above creates a selfsigned CA cert, we should probably not consider it invalid as it's probably quite common in the wild..

@SomberNight
Copy link
Member Author

Which part of the cat|openssl x509 -text -noout output makes you say one cert is a CA while the other one is not?

The third command from #9258 (comment) creates a cert that has this:

        X509v3 extensions:
            X509v3 Basic Constraints: 
                CA:FALSE

and that cert also passes strict validation.


Anyway, I take it you think we can leave interface.py as-is then (as this PR left it)?
What about the instructions in the electrumx readme, should we change those?

@accumulator
Copy link
Member

accumulator commented Oct 21, 2024

Hmm, interesting.. In my case the generated cert has

            X509v3 Basic Constraints: critical
                CA:TRUE

might be a different distro-supplied /etc/ssl/openssl.cnf?

@accumulator
Copy link
Member

accumulator commented Oct 21, 2024

Anyway, I take it you think we can leave interface.py as-is then (as this PR left it)?

For now I guess it's fine, as it retains the pre-py313 behavior, although I think it's good to eventually use strict checking on non-selfsigned certs.

What about the instructions in the electrumx readme, should we change those?

Your command using -addext seems to work quite deterministically :)

@SomberNight
Copy link
Member Author

Anyway, I take it you think we can leave interface.py as-is then (as this PR left it)?

For now I guess it's fine, as it retains the pre-py313 behavior, although I think it's good to eventually use strict checking on non-selfsigned certs.

Well with this PR we are doing whatever is the default (so strict after 3.13, non-strict before) for public CA-signed certs (line 502 and 512),
and explicit non-strict for self-signed certs (lines 515-519):

# see if we already have cert for this server; or get it for the first time
ca_sslc = ssl.create_default_context(purpose=ssl.Purpose.SERVER_AUTH, cafile=ca_path)
if not self._is_saved_ssl_cert_available():
try:
await self._try_saving_ssl_cert_for_first_time(ca_sslc)
except (OSError, ConnectError, aiorpcx.socks.SOCKSError) as e:
raise ErrorGettingSSLCertFromServer(e) from e
# now we have a file saved in our certificate store
siz = os.stat(self.cert_path).st_size
if siz == 0:
# CA signed cert
sslc = ca_sslc
else:
# pinned self-signed cert
sslc = ssl.create_default_context(purpose=ssl.Purpose.SERVER_AUTH, cafile=self.cert_path)
# note: Flag "ssl.VERIFY_X509_STRICT" is enabled by default in python 3.13+ (disabled in older versions).
# We explicitly disable it as it breaks lots of servers.
sslc.verify_flags &= ~ssl.VERIFY_X509_STRICT
sslc.check_hostname = False

Hmm, interesting.. In my case the generated cert has

            X509v3 Basic Constraints: critical
                CA:TRUE

might be a different distro-supplied /etc/ssl/openssl.cnf?

Right, sorry, I meant the third the command with all the explicit -addexts creates it with CA:FALSE (as expected given that is specified as input), and strict verification passes for that.
With the second command without all the options I get the same as you said. (but yes, good point that distro-supplied defaults could affect it)

@accumulator
Copy link
Member

Anyway, I take it you think we can leave interface.py as-is then (as this PR left it)?

For now I guess it's fine, as it retains the pre-py313 behavior, although I think it's good to eventually use strict checking on non-selfsigned certs.

Well with this PR we are doing whatever is the default (so strict after 3.13, non-strict before) for public CA-signed certs (line 502 and 512), and explicit non-strict for self-signed certs (lines 515-519):

Ah yes, I can't read 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-network 🕸 related to logic in network.py (etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants