eworm-de / routeros-scripts

a collection of scripts for MikroTik RouterOS
GNU General Public License v3.0
1.27k stars 285 forks source link

Allow to customize certificate name #46

Closed Kentzo closed 1 year ago

Kentzo commented 1 year ago

I'm using github as a base url to pull updates and thus don't need Let's Encrypt cert. Please consider making certificate name that your script expects customizable.

eworm-de commented 1 year ago

I am kind of confused... This is about check-certificates? You can configure the base url with CertRenewUrl, then name the certificate with CN or ASN and a matching file extension (.p12 or .pem).

What's wrong with this or what would you expect?

Kentzo commented 1 year ago

Please correct me if I'm wrong, but the $CertificateAvailable function that's being called in a few places expects to find a certificate named "R3". Since I do not use Let's Encrypt I do not have the R3 cert installed. I do not want to rename my other certificate (for GitHub) just satisfy this script.

eworm-de commented 1 year ago

Ah, got it... I misunderstood your initial request.

This should import the certificate for github.com:

$CertificateAvailable "DigiCert TLS Hybrid ECC SHA384 2020 CA1"

So you should do the initial installation from my repository, run that command, then switch to your GitHub repository.

Kentzo commented 1 year ago

I understand the code of $ScriptInstallUpdate that it expects to unconditionally find certificates named "R3" and "E1" in /certificate.

You can see this dependency on having a specifically named certificate elsewhere. Sometimes it's just warnings, but at other times it's an error exit.

eworm-de commented 1 year ago

I understand the code of $ScriptInstallUpdate that it expects to unconditionally find certificates named "R3" and "E1" in /certificate.

Yes, that's used by Let's Encrypt. "R3" is the current intermediate certificate, "E1" is the future intermediate certificate for full ecdsa chain. Does not hurt to have these around, even if not used (for example when fetching from a GitHub repository). It matches most installations, as my web server is using this.

Just install the required certificate once and you are done. (Until GitHub changes certificate chain of course... But that is probably years ahead.)

You can see this dependency on having a specifically named certificate elsewhere. Sometimes it's just warnings, but at other times it's an error exit.

Sure, just a warning here, as chances are that another certificate chain is required. Note that the script downloads still fail on invalid or missing chain.

Other code has a hard dependency on certificate, that is if I am sure a specific certificate is used.

Kentzo commented 1 year ago

My current workaround is

:global CertificateAvailable do={ :return true }

in global-config-overlay.

Sure, just a warning here, as chances are that another certificate chain is required. Note that the script downloads still fail on invalid or missing chain.

It's a warning in $ScriptInstallUpdate, but an error in $DownloadPackage, $GetMacVendor and possibly elsewhere.

Is there a good reason for explicitly requiring to have a certificate with a certain name in /certificate? IIRC /tool/fetch doesn't require that as it pulls and verifies the cert on its own.

eworm-de commented 1 year ago

My current workaround is

:global CertificateAvailable do={ :return true }

in global-config-overlay.

I still do not get what this does work around... What's the issue with having these certificates around?

It's a warning in $ScriptInstallUpdate, but an error in $DownloadPackage, $GetMacVendor and possibly elsewhere.

Sure, because I know what certificate these services use.

Is there a good reason for explicitly requiring to have a certificate with a certain name in /certificate? IIRC /tool/fetch doesn't require that as it pulls and verifies the cert on its own.

No, fetch does not do that. It needs certificates in /certificate to verify the server certificates.

Kentzo commented 1 year ago

My argument is that the check is redundant as /tool/fetch will fail on its own if needed certificate is missing.

The workaround removes this dependency.

eworm-de commented 1 year ago

Ok, let's get some facts:

All my scripts use fetch with check-certificate=yes-without-crl. Thus certificates have to be available for server certificates to be verified. The function $CertificateAvailable makes sure the required certificate is available for verification. If you think it works without for you... Good luck.

Kentzo commented 1 year ago

I do not think it's proper to force certificates into user's certificate chains when user knows they won't be needed. There is $ScriptUpdatesBaseUrl, would it harm to have, say, $ScriptUpdatesCertName?

Since the script discloses that and there is a way to override this behavior I'm content.

The function $CertificateAvailable makes sure the required certificate is available for verification.

If i'm not fetching scripts from URLs that use Let's Encrypt certs. Thus "R3" / "E1" are note required: other certificates are. Therefore current application of $CertificateAvailable fails to perform this stated function.

Using /tool/fetch with check-certificate=no (which is the default) does - surprise - not check the certificate

I did not object to this. All my references regarding usage of /tool/fetch in this thread were made with respect to the codebase of this project. Which as you pointed out checks certificate.

With respect to this project since, as you say, check-certificate=yes-without-crl is being used, /tool/fetch/ will, as you say, fail if needed certificate is missing. Thus no additional checks by $CertificateAvailable are strictly needed, except for user's convenience. However, for users who install from elsewhere current implementation that unconditionally wants "R3" and "E1" is an annoyance.

Apologies for any confusion, I hope I clarified my point sufficiently.

eworm-de commented 1 year ago

I do not think it's proper to force certificates into user's certificate chains when user knows they won't be needed.

But they are used. Well, except for a very little fraction of users that do not use my default web server. Still it is not a big deal. This is just some bytes of storage and it does not hurt anybody.

There is $ScriptUpdatesBaseUrl, would it harm to have, say, $ScriptUpdatesCertName?

This is something really different. See script check-certificates (https://git.eworm.de/cgit/routeros-scripts/about/doc/check-certificates.md) for its use case.

If i'm not fetching scripts from URLs that use Let's Encrypt certs. Thus "R3" / "E1" are note required: other certificates are. Therefore current application of $CertificateAvailable fails to perform this stated function.

This should not fail, unless you miss the certificates for your server or clone of the repository. Just import the correct certificate, for github I gave the command above. (After that also importing "R3" & "E1" succeeds.)

With respect to this project since, as you say, check-certificate=yes-without-crl is being used, /tool/fetch/ will, as you say, fail if needed certificate is missing. Thus no additional checks by $CertificateAvailable are strictly needed, except for user's convenience.

Checks and error handling should happen as soon as possible.

It's always descriptive to compare with cars: Let's assume your breaks are malfunctioning. Do you want to know as soon as possible or are your fine to notice after you crashed into a bridge pillar and died?

However, for users who install from elsewhere current implementation that unconditionally wants "R3" and "E1" is an annoyance.

It's not. As said: Get the certificate for your server in place, and everything works without annoyance. You have extra certificates that are not strictly needed, but these do not hurt.

And a last hint: If this really annoys you... Feel free to change it in your own repository. The change should easily rebase on all my changes.