repository-service-tuf / repository-service-tuf-worker

Repository Service for TUF: Worker
MIT License
8 stars 15 forks source link

Add FileNameSigner and register in SignerStore #455

Closed lukpueh closed 7 months ago

lukpueh commented 7 months ago

The custom FileNameSigner is similar to the previously registered CryptoSigner provided by securesystemslib, with the following differences:

supersedes #427

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (b70143c) 100.00% compared to head (2d618e0) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #455 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 15 15 Lines 1039 1071 +32 ========================================= + Hits 1039 1071 +32 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kairoaraujo commented 7 months ago

Creating a custom signer is a good approach, as the sslib API makes it very easy to implement. The only thing we can discuss is that it will use a setting outside of RSTUF settings (Dynaconf) as I commented below

I was wondering if isn't better to implement the logic in the SignerStore, in the get.

The user from the RSTUF container will have the parameter RSTUF_ONLINE_KEY_FILE_DIR as optional. If the user sets it and the key is scheme file, it appends as suffix in value.

Advantages

lukpueh commented 7 months ago

I was wondering if isn't better to implement the logic in the SignerStore, in the get.

I strongly recommend to not do that. The main appeal of the Signer API, besides the sign interface, is that signer access/loading is an implementation detail of the individual signer implementation. This interface allowed me to add AWS support with 0 lines of code changes in SignerStore (#237), which is substantially less than a prior wip attempt that implemented the loading logic in the worker (#415).

Case handling different signers in SignerStore weakens the universal Signer.from_priv_key_uri interface of passing specific information about the signer via public key and uri, and providing additional information ambiently (e.g. via env var).

The user from the RSTUF container will have the parameter RSTUF_ONLINE_KEY_FILE_DIR as optional. If the user sets it and the key is scheme file, it appends as suffix in value.

Advantages

  • The CLI user doesn’t need to choose between File and FileName (which can be confused)
  • It become simple and the user in the CLI can give or not the dir suffix

The cli user doesn't need to know about File or FileName at all. IIRC we agreed that the cli only asks the user if they want to provide the online key as file, then transparently adds fn:<keyid> uri to the public key, and informs the user to:

I really don't think that the cli should also provide an option to add a file:/abs/path/to/<keyid>. If we do want to support absolute file path uris, we can easily change FileNameSigner.from_priv_key_uri to threat the file name in the uri as absolute path if RSTUF_ONLINE_KEY_DIR is not set.

But let's not get too caught up with file-based online keys. Given that the Signer API already supports multiple cloud kms services, file-based keys should be an edge case mainly for development.

  • We don’t need to maintain also a custom signer in the CLI and Worker

FileNameSigner is only needed in the worker and it comes at about ~20 lines of self-contained code with a clear interface. IIUC, you suggested above to move that code into the signer store, so I don't think maintaining this here is an issue. That said, @jku thinks that there is value in upstreaming the FileNameSigner (https://github.com/repository-service-tuf/repository-service-tuf/issues/580#issuecomment-1913108310)

  • We keep the RSTUF Worker settings with Dynaconf

This is a legitimate concern and we need to find a solution that also works for other signers with ambient configuration, which require translation from the RSTUF namespace to the domain namespace, e.g. where RSTUF_AWS_ACCESS_KEY_ID becomes AWS_ACCESS_KEY_ID.

I think SignerStore could do that, e.g.:


class SignerStore:
    def __init__(self, settings: Dynaconf):
        ...
        self._env: dict[str, str] = {}
        for k, v in settings:
            if k.startswith("RSTUF_ONLINE_KEY_"):
                self._env[TRANSLATE_ENV(k)] = v

    def get(self, key: Key) -> Signer:
        ...
        with SET_ENV(self._env):
            signer = Signer.from_priv_key_uri(uri, key)
kairoaraujo commented 7 months ago

Thank you for all the detailed information, @lukpueh

kairoaraujo commented 7 months ago
class SignerStore:
    def __init__(self, settings: Dynaconf):
        ...
        self._env: dict[str, str] = {}
        for k, v in settings:
            if k.startswith("RSTUF_ONLINE_KEY_"):
                self._env[TRANSLATE_ENV(k)] = v

    def get(self, key: Key) -> Signer:
        ...
        with SET_ENV(self._env):
            signer = Signer.from_priv_key_uri(uri, key)

The variable in settings will be setttings.ONLINE_KEY_DIR (accessible also by settings.get(ONLINE_KEY_DIR)). Dynaconf removes the prefix RSTUF_ to identify specific settings in the container environment.

Please, can you include the RSTUF_ONLINE_KEY_DIR in the docs/source/guide/Docker_README as well?

lukpueh commented 7 months ago

@kairoaraujo, I added the requested changes. Let me know what you think.