Add captive portal update flag, and re-apply the cert upon reboot

[timrettop]

For your consideration.

Welcome any feedback or recommendations.

[kchristensen]

So what you're doing is along the lines of what I was thinking but there's a couple of things I'm not sure about here:

1. I get that if you're using captive portal you don't want it to be broken for X minutes post reboot until the cert was fixed, but depending on when exactly this gets triggered post reboot you're going to be restarting the controller, possibly interrupting important stuff.

2. I was trying to think of some if/else scenario here, because let's say on reboot your cert was expired, this would restart the controller using an expired cert, then 5 minutes later restart it again with a valid one. Like maybe swap these around so the sleep & renew attempt happen then if ENABLE_CAPTIVE do a restart just in case? The 300 second sleep was totally arbitrary, it may very well be something like 120s would be adequate, I just didn't feel like testing it by rebooting my router a bunch.

[kchristensen]

You could also move the java cert import in udm-le.sh out of deploy_cert and keep it wrapped in the if, that way even if there wasn't a new cert it would do the import?

[timrettop]

Yeah I see your point. The on_boot.d script could apply either the new cert (if renewed) or the existing cert if not renewed that way. That would mean, though, that the cronjob would also re-apply the cert without needing to and restarting unifi-os.

[kchristensen]

How about this:

If you remove the ENABLE_CAPTIVE statement from the on_boot.d script totally, but leave it in the udm-le.sh script but in the main bit of the script not the deploy_cert() bit and wrapped in an if, it COULD import and restart every time it was run.

Now this would mean the cron restarts the controller every night if ENABLE_CAPTIVE was set, but maybe the on_boot.d script exports a variable that udm-le.sh looks for to know if it was invoked at boot time instead of via cron, so it wouldn't end up restarting the cert every night.

[kchristensen]

Alternatively, you could move the logic for the portal stuff in the main script to a function, but bake in some logic that does an md5sum or something on ${UDM_LE_PATH}/lego/certificates/${CERT_NAME}.crt ${UBIOS_CERT_PATH}/unifi-core.crt. If they don't match, you know you probably just rebooted, and you could copy and do the restart. The trick would be not doing a double restart on a new cert deploy.

[timrettop]

Take a look at my last commit, I think I've solved the handling of boot workflow and cronjob workflow.

[timrettop]

Sorry I forgot to remove the unneeded on_boot things and add the new version of the script call, see my latest commit.

[kchristensen]

Minor but your tabbing is off here

[kchristensen]

Maybe I'm trying to think before I've had enough coffee, but I'm having trouble following this:

• When bootrenew gets called on boot, if PODMAN_CMD returns false because there's no new cert isn't it going to just stop and never run the rest of this?
• If you call renew under normal circumstances, isn't it going to run ad_captive and restart the container every night?

[timrettop]

So my logic is the following:

When bootrenew) gets called on boot, the PODMAN_CMD will run and check for the need to update the certificate. Even if the certificate doesn't need to be updated, the return condition (I believe) isn't false and so the deploy_cert function will run, see that there is no new cert, return out and then the add_captive function will run, and finally will restart unifi-os (which may be the only time its technically unneeded). If the cert is updated, all the appropriate steps will occur as well.

I could only test running the boot script with a certificate that didn't require updating, and confirmed that the add_captive and unifi-os restart occurred properly.

When the cronjob calls renew), and the certificate is renewed, a variable called NEW_CERT is set to yes, which will invoke the add_captive and unifi-os restart... otherwise if there is no new certificate that night, nothing is changed.

Does that make sense or does something still seem off?

[kchristensen]

Ok, yeah that makes sense. I had forgotten that I made deploy_cert a no-op unless the find returned true and wasn't relying on the return value of the podman command. I think this looks good and unless UI changes the way the captive portal certs get regenerated is probably the most elegant way to deal given all the limitations. Thanks for sticking with this and seeing it through!