Certificate renewal - Githubissues

eyJhb commented 5 years ago

The certificate renewal currently does not work, for NUMEROUS reason.

Firstly, the "configuration" that is applied below, is actually never really applied, so it will try to use HTTP/TLS verification (which will fail).

https://github.com/aau-network-security/haaukins/blob/0544e21ed5b0d6b37ce77bf1b0a3213b25612b35/daemon/daemon.go#L255-L263

A quick solution and maybe permanent is using the certmagic.Default, as it will then actually use the config (remember to not use staging), and actually do cert renewal with DNS.

    certmagic.Default.DNSProvider = provider
    certmagic.Default.Agreed = true
    certmagic.Default.Email = email
    certmagic.Default.CA = certmagic.LetsEncryptStagingCA
    certmagic.Default.Storage = &certmagic.FileStorage{
        Path: pathTLS,
    }

After this, another problem arises, as there are some problem with go-acme/lego Cloudflare plugin with cname, so the records have been changed from cname to a, which only means it points directly to the IP of the server, and does not use the domain anymore to point at the server.

Also... I think it should be a separate service, as just putting everything into one service is bad. Also, ntp-event.dk, should get certs as well, as it is currently only wildcard *.ntp-event.dk that works... And cli should get certs as well..

Hope this helps @mrturkmen06 ! :)

mrtrkmn commented 5 years ago

Hi Johan, Thank you very much for helpful support ! and I totally agree that it should be a separate service which will provide us easy debugging and quick solution. Currently, I grasped some knowledge about how renew process of certification is scheduled on sec02. The latest version is deployed to sec02, (however there is also some minor issues related to versioning builds (apparently) yet it is another topic to discuss ).

jbjjbjjbj commented 5 years ago

Didn't know where else to write this but the error from certmagic.Manage is unhandled in the daemon.Run() function.

https://github.com/aau-network-security/haaukins/blob/master/daemon/daemon.go#L932

Checking this error may help with debugging this issue.

eyJhb commented 5 years ago

@jbjjbjjbj Not this issue, but in the future. There are many errors that are like that, or you get a BS none descriptive error, e.g.

https://github.com/aau-network-security/haaukins/blob/0544e21ed5b0d6b37ce77bf1b0a3213b25612b35/daemon/daemon.go#L960-L963

tpanum commented 5 years ago

I think many of the points mentioned here are correct, but also fairly abstract and imprecise. I believe there is a set of aspects that are fairly important for the issue:

certmagic changed API since it was first introduced into Haaukins, which suggests that now is the time to (finally) adapt Go modules.
Using the (now renamed) certmagic.Default is a bad idea due to:
- It is primarily designed for user friendly cases in which one package only has certmagic as dependency (e.g. main)
- Global variables should be avoided in almost all cases

I am absolutely no expert on the certmagic API, but here are the global variables being used for obtaining gRPC (admin API) and host (e.g. *.ntp-event.dk) certificates: https://github.com/aau-network-security/haaukins/blob/0544e21ed5b0d6b37ce77bf1b0a3213b25612b35/daemon/daemon.go#L930-L941

Despite a local configuration is being used for the gRPC options (admin API) here:

https://github.com/aau-network-security/haaukins/blob/0544e21ed5b0d6b37ce77bf1b0a3213b25612b35/daemon/daemon.go#L912-L918

, which is instantiated here:

https://github.com/aau-network-security/haaukins/blob/0544e21ed5b0d6b37ce77bf1b0a3213b25612b35/daemon/daemon.go#L272-L276

eyJhb commented 5 years ago

The main point nonetheless is, that is makes no sense to have this kind of certificate renewal in Haaukins, as it just increases complexity without any win basically.

As it currently is, testing locally require each test instance to get active certificates from LetsEncrypt.

The reason for using certmagic.Default is also just PoC is mentioned

tpanum commented 5 years ago

Designwise, it can make a lot of sense and should not just be instantly discarded as a bad feature. It provides automation and are helpful for adaptability of the platform, which I believe was set as a requirement for the newly gained funding. Additionally, I believe the intend of the design was to have a solution that allowed both TLS and TCP across every communication channel.

The fact that the implementation does not reflect this, or does so poorly, is more a matter of adding additional craftsmanship.

eyJhb commented 5 years ago

Tons of other services provides this automatically as well. The all-in-one thing with poor error reporting and poor designs for debugging is not ideal, and generally all-in-one bin I would call bad.