Conversation
| with mock.patch('certbot._internal.renewal.hooks.renew_hook'): | ||
| renewal.renew_cert(self.config, None, le_client, lineage) | ||
|
|
||
| assert self.config.rsa_key_size == 2048 |
There was a problem hiding this comment.
This is already the default value. Perhaps it'd make more sense to test with a different key size?
There was a problem hiding this comment.
Just to clarify the purpose of this assertion here. The point is not to verify that the key is actually of 2048 bits (which is the default indeed), but to check that the option for the rsa key size has been set in the configuration, so it will be stored in the renewal params later.
Indeed you will see that I set this value to INVALID_VALUE and expect it to be set to the actual size of the key during the renewal call.
bmw
left a comment
There was a problem hiding this comment.
This LGTM! Thanks for picking this up @adferrand.
| with open(key_path, 'rb') as file_h: | ||
| key = load_pem_private_key(file_h.read(), password=None, backend=default_backend()) | ||
| if isinstance(key, rsa.RSAPrivateKey): | ||
| config.rsa_key_size = key.key_size |
There was a problem hiding this comment.
From #7711 and the ECDSA design doc, in the future I think we should check if values like config.rsa_key_size are set by the user (by checking certbot.cli.set_by_cli) and their value differs from the current key and if so raise an error. I think we should do this for ECDSA keys and for key_type with RSA keys when support for them is added initially here as I think the only reason we shouldn't do this now with rsa_key_size is it is technically a breaking change.
With this in mind, I think it could make sense to check for this case and log a warning for RSA keys now, but I'm also fine if that is done later.
Fixes #7694.
This PR is an alternative of #7798, which strictly focuses on storing the proper key renewal parameters when
--reuse-keyflag is set. The parameters are extracted directly from the key itself, instead of relying on pre-existing input parameters that may be inconsistent.Even if only RSA keys are supported by Certbot as of now, the code is designed to support different kind of keys, with the ECDSA feature soon to be released in mind.