NLnetLabs / krill

RPKI Certificate Authority and Publication Server written in Rust
https://nlnetlabs.nl/projects/routing/krill/
Mozilla Public License 2.0
294 stars 41 forks source link

Krill version not set properly #1222

Open timbru opened 3 months ago

timbru commented 3 months ago

Hi!

Apologies, this bug was produced by yours truly....

It looks like the KrillVersion is not set properly inside of the PropertiesManager in start_krill_daemon in src/daemon/http/server.rs. Its .init() function is only called in case there is an actual upgrade.

This can lead to issues with future updates as the upgrade_versions function will not be able to work out what the version was for an existing instance.

One fix would be to add the following after the conditional finalise_data_migration is called:

    if !properties_manager.is_initialized() {
        // This looks like a new instance. Initialise the version so it can be
        // used in future to determine whether an update is needed.
        properties_manager.init(KrillVersion::code_version())?;
    }

But... it would be better to also detect existing situations where the version was not set. If disk is used (and postgresql cannot be used in practice, so this case can be ignored) this can be recognized by the fact that a properties directory exists but it is not initialized. But... this is not entirely trivial because for new installations this PropertyManager instance is created before checking what the current version is, because Krill tries to use it to check if it has been initialized - that act uses a lock and creates the directory. So, probably what would be needed to handle this gracefully is some code in upgrade_versions so that instead of this:

if properties_manager.is_initialized() {
        // The properties manager was introduced in Krill 0.14.0.
        // If it's initialised then it MUST have a Krill Version.
        let current = properties_manager.current_krill_version()?;
        UpgradeVersions::for_current(current)
    } else { ...

Where the .is_initialized creates the directory and lock files, it is checked first whether this directory exists and if:

Then conclude that this must be a 0.14.x installation that never initialized the manager - and therefore never stored the version.

Of course, feel free to fix the bug I introduced in a different way ;) But I hope this helps, and please let me know if this is not clear.