Implements back the port based SSL certificate fetching + real SNI support. #7
Conversation
Implements back the port based SSL certificate fetching. This modernize the Mono code base to allow booth, a in-memory cache and that same file-based cache if the in-memory cache doesn't contain the entry. This makes the previous SetCertificate constructor obsolete, but for good, since we can now set a different certificate per ports.
|
Do you have an actual use case for this? The reason the mono code even has that port-based logic is dated and ancient, I don't see any reason to keep it as functionality here. |
|
I do have an use case if I host various CN names on various ports without having to rely on SAN attributes. |
Do you have an actual use case for this? Sure obviously this would allow you to have separate TLS certs served per port but what use case could that possibly have in 2024 to warrant implementing? I would rather have a more flexible API that also incorporated SNI support or similar via e.g. a callback. Then you can just add the destination socket port in one of the parameters the selection logic and users that really need this can take it from there. |
|
So, the full story is I did this mostly for "completeness" more than anything, I do plan under the hood onto making a SNI api based on the work I did on my server suit (a certificate signing chain). Long story short, my plan is making a check for if the certificate that is set is a Root CA or a chain signed cert, if Root CA enable the SNI signing system. I do have locally a system like this in place, but for some reasons it not work as intended just yet (the reason it's not there yet). |
GitHubProUser67
changed the title
|
The SNI support has finally be added in the repository @PJB3005 ! The way it works is simply by inserting the Root CA Certificate instead of the classic chain signed certificate (via the same constructor), and HTTPListener will sign itself intermediate certificates that are instanced per-session. Simple and powerfull ^^. |
Implements SNI Support. Uses the same constructor as the previous solution, instead you will need to pass-in the CA Certificate.
GitHubProUser67
force-pushed
the
master
branch
from
ab820f8 to
ce3c858
Compare
HttpConnection - fixes non-https path.
…idation for older NET SDKs. HttpListener.Certificates - Implement certificate Authority input validation for older NET SDKs.
GitHubProUser67
force-pushed
the
master
branch
from
ecdc39f to
b7664a6
Compare
Uh, what? Does that actually work? You can't self-sign certificates. And you can't get a root CA cert from anywhere? And isn't signing way too expensive to do for every TLS connection? |
|
I can confirm it works just fine, the root CA will need to be obtained from an outside source (I can make a generator for it too if needed), then this CA cert will be used to sign all the certificates that are generated on the fly. They are all valid for a period of 7 days to avoids making too much of them (this can be extended) and are cached locally during that period. EDIT: if a classic cert, it will naturally fallback to the classic certificate processing like how it worked previously. |
Some web-crawlers tries to send a request with an empty url, we forbid these in order to not throw a debug assertion.
| { | ||
| public class CertificateHelper | ||
| { | ||
| private static readonly string[] tlds = { |
There was a problem hiding this comment.
This is an extremely outdated list of TLDs. You cannot hardcode something like this.
|
|
||
| // set up a certificate creation request. | ||
| CertificateRequest certRequestAny = new CertificateRequest($"CN={certSubject} [{GetRandomInt64(100, 999)}], OU=SpaceStation14 Department," + | ||
| $" O=\"SpaceWizards Corp\", L=New York, S=Northeastern United, C=US", rsa, certHashAlgorithm, RSASignaturePadding.Pkcs1); |
There was a problem hiding this comment.
This organization does not exist and it should not be hardcoded in the library.
| SubjectAlternativeNameBuilder sanBuilder = new SubjectAlternativeNameBuilder(); | ||
|
|
||
| sanBuilder.AddDnsName(certSubject); // Some legacy clients will not recognize the cert serial-number. | ||
| sanBuilder.AddEmailAddress("[email protected]"); |
There was a problem hiding this comment.
This email address does not exist and it should not be hardcoded in the library.
| byte[] certSerialNumber = new byte[16]; | ||
| new Random().NextBytes(certSerialNumber); |
There was a problem hiding this comment.
This rando isn't cryptographically secure.
| private static long GetRandomInt64(long minValue, long maxValue) | ||
| { | ||
| #if NET6_0_OR_GREATER | ||
| return new Random().NextInt64(minValue, maxValue); | ||
| #else | ||
| Random random = new Random(); | ||
| return (long)(((random.Next() << 32) | random.Next()) * (double)(maxValue - minValue) / 0xFFFFFFFFFFFFFFFF) + minValue; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
This random isn't cryptographically secure.
| public static bool IsCertificateAuthority(X509Certificate certificate) | ||
| { | ||
| // Compare the Issuer and Subject properties of the certificate | ||
| return certificate.Issuer == certificate.Subject; | ||
| } |
There was a problem hiding this comment.
This is not a correct way to check this, nor should this library care.
| /// </summary> | ||
| sealed class RsaPkcs1SignatureGenerator : X509SignatureGenerator | ||
| { | ||
| // Workaround for SHA1 and MD5 ban in .NET 4.7.2 and .NET Core. |
There was a problem hiding this comment.
We are not implementing workarounds to security issues like this.
| public static byte[] HexToByteArray(string hex) | ||
| { | ||
| //copypasted from: | ||
| //https://social.msdn.microsoft.com/Forums/en-US/851492fa-9ddb-42d7-8d9a-13d5e12fdc70/convert-from-a-hex-string-to-a-byte-array-in-c?forum=aspgettingstarted | ||
| return Enumerable.Range(0, hex.Length) | ||
| .Where(x => x % 2 == 0) | ||
| .Select(x => Convert.ToByte(hex.Substring(x, 2), 16)) | ||
| .ToArray(); | ||
| } |
There was a problem hiding this comment.
.NET has built-in libraries for this.
|
I am closing this PR. Based on the code I have skimmed over so far it is clear to me that:
If you want to have a way to specify custom certificates dynamically like this, a PR that adds a simple callback so that library consumers can pass their own cert is all that should be necessary. You can implement whatever... this is.. on top of that. |
|
Okay, most of these are valid points I admit. However, if I am being completely honest, I do not understand, nor like, the 2. sentence there, I would like some examples and proof as to where I didn't understand how TLS worked. Some code might not be the best way to put it, but, as far as I know, the certs "works" so does TLS, it is simply skimming over some security, which is a valid point for it not being merged. |
You are self-signing certificates to implement SNI, directly for any domain clients ask for. This is not something you can do in the real world unless you have your own CA certs preinstalled on client machines. Even if you did have that, it would be grossly irresponsible from a security perspective to have your web server self signing certificates right then and there for anything other than dev environment testing. |
Isn't security the whole point of the exercise? |
|
I better understand the point now, the security argument is valid, I did use this system so old systems where SSL is a requirement could work. On my fork I re-did that a bit, to also support chain signed certificate from an other CA (by loading the file directly if it exists) with a fallback to that system if not found. But after all, this is quite use-case specific, so maybe it is best suited for the fork only. |
Implements back the port based SSL certificate fetching.
This modernize the Mono code base to allow booth, a in-memory cache and that same file-based cache if the in-memory cache doesn't contain the entry.
This makes the previous SetCertificate constructor obsolete, but for good, since we can now set a different certificate per ports.
This is not SNI level, but it's still leagues beyond the actual .NET implementation (the MYSTICAL null stubbed return value with the TODO ^^).