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

fix(server): add subjectAltName field into self signed certificate #1429

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zouxifeng
Copy link

Current install script only fills CN field with PUBLIC_HOST value. When trying to access api and provide the self signed certificate to verify server certificate, the request will fail with SSL: CERTIFICATE_VERIFY_FAILED error.

To prevent this error, the install script should add "subjectAltName = IP.1:${PUBLIC_HOST}" when generating self signed certificate.

import requests
requests.get('https://${API_PREFIX}/access-keys',verify='shadowbox-selfsigned.crt')

Current install script only fills CN field with PUBLIC_HOST value. When
trying to access api and provide the self signed certificate to verify
server certificate, the request will fail with SSL:
CERTIFICATE_VERIFY_FAILED error.

To prevent this error, the install script should add "subjectAltName =
IP.1:${PUBLIC_HOST}" when generating self signed certificate.

```python
import requests
requests.get('https://${API_PREFIX}/access-keys',verify='shadowbox-selfsigned.crt')
```
@zouxifeng zouxifeng requested a review from a team as a code owner October 23, 2023 08:39
@fortuna
Copy link
Collaborator

fortuna commented Oct 23, 2023

This seems like a library limitation. What is this requests library? Can you provide the documentation? I don't know what that verify parameter means.

The big problem is the fact it's self-signed. We typically validate it with a certificate fingerprint.

@fortuna
Copy link
Collaborator

fortuna commented Oct 23, 2023

Found it: https://requests.readthedocs.io/en/latest/api/

verify takes a CA bundle. I believe it should work as is. You shouldn't need to specify the IP in the SAN of the certificate.

Have you tried using the Python ssl library directly? Does it fail the same way? I think it may be a limitation of requests.

I worry that by putting an IP there the certificate will become a lot more distinguishable and give away it's an Outline server.

@zouxifeng
Copy link
Author

Sorry, I'm not a cryptography expert, so maybe I can't explain it accurately and clearly. From my limited knowledge, a self-signed certificate is not fundamentally different from a CA certificate. A CA Bundle is just a collection of many CA certificates. So passing in a self-signed certificate results in the certificate itself being treated as a CA. Prove its legitimacy by itself.

Because it is a self-signed certificate, there is not a big difference between ignoring certificate verification or verifying the certificate as a CA. However because I want to do more key management through the API, I want to prevent MITM attacks through certificate verification.

Finally, you mentioned that adding the SAN field may cause the server to be easily recognized as an Outline Server, which I haven't considered. It may be possible.

@zouxifeng
Copy link
Author

Append the "-addext" option will add the following section after X509v3 Basic Constraints: critical

X509v3 Subject Alternative Name: 
    IP Address:1.2.3.4

@fortuna fortuna changed the title add subjectAltName field into self signed certificate fix(server): add subjectAltName field into self signed certificate Oct 24, 2023
@fortuna
Copy link
Collaborator

fortuna commented Oct 24, 2023

When in doubt, read the RFC 🤓

https://datatracker.ietf.org/doc/html/rfc6125#section-4.1 says:

Even though many deployed clients still check for the CN-ID
within the certificate subject field, certification authorities
are encouraged to migrate away from issuing certificates that
represent the server's fully qualified DNS domain name in a
CN-ID. Therefore, the certificate SHOULD NOT include a CN-ID
unless the certification authority issues the certificate in
accordance with a specification that reuses this one and that
explicitly encourages continued support for the CN-ID identifier
type in the context of a given application technology.

https://datatracker.ietf.org/doc/html/rfc9110#section-4.3.4-2 also covers some of that.

So CN is deprecated, but still used. SAN is the recommended solution. This PR adds the IP to the SAN.
The Python requests library doesn't seem to support CN anymore, and it requires SAN.

There's one issue though. PUBLIC_HOSTNAME may be set as a hostname, and this change will break.
We need to detect whether it's an IP address, and add accordingly.

Use a regex to test whether input hostname is an IP address. If it's an
IP address, use "IP" prefix. Otherwise, use "DNS".
@zouxifeng
Copy link
Author

This is great. I didn't realize that I should go through the RFC document.

There's one issue though. PUBLIC_HOSTNAME may be set as a hostname, and this change will break.
We need to detect whether it's an IP address, and add accordingly.

If PUBLIC_HOSTNAME is not an IP address, we should put the hostname SAN field as DNS-ID.

-addext "subjectAltName=DNS:${PUBLIC_HOSTNAME}"

Otherwise, use the IP address.

Finally, I went through the Python requests source code even urllib3. The Python SSL module will handle the certificate verification, not requests or urllib3. This makes me think, the underlying libssl deprecates CN and uses SAN field to verify the server certificate. I also have made a quick test to verify that. If a FQDN domain is used, the install script should put it as SAN DNS-ID in the certificate.

I pushed another commit to address this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants