Add support for revoking ecdsa keys without --key-path

[atombrella]

See GitHub issue #8569 for details.

Co-Authored-By: commonism [email protected]

In my local setup, it's using josepy 1.6.0, and not 1.7.0 (or 1.8). I think some more tests could be written.

Pull Request Checklist

• If the change being made is to a distributed component, edit the master section of certbot/CHANGELOG.md to include a description of the change being made.
• Add or update any documentation as needed to support the changes in this PR.
• Include your name in AUTHORS.md if you like.

[bmw]

Thanks for the PR @atombrella. Let me know when this is ready for review.

[atombrella]

Thanks for the PR @atombrella. Let me know when this is ready for review.

I'm trying to add more tests. Integration test is there with a template, and probably a few more should be added, e.g., cli test?

[atombrella]

@bmw I believe this is ready for review. I'm not sure about what other tests could be needed, though.

I'm tempted to squash commits, so Co-Authored-By will be recognized by GitHub.

[bmw]

Thanks for the ping @atombrella. This dropped off my radar somehow.

I'm personally satisfied with testing here since the modified code has 100% test coverage and you added an integration test for the problem. If there's something else you'd like to test for but you're not sure the best way to do it, let me know and I can try to help.

I think we should probably structure the fix differently though. acme.client.ClientNetwork takes an alg parameter specifying the algorithm that should be used in signing JWS. The patch from #8569 ignores this attribute and instead determines the algorithm from the key type in this specific method.

I personally think we should instead do something like one of:

1. Fix where acme.client.ClientNetwork is created in Certbot so the right alg parameter is provided in this case.
2. Keeping in mind that acme.client.ClientNetwork is part of our public API, we could maybe deprecate the alg parameter and always determine the algorithm based on the key type.

What do you think? Do you have any other ideas?

If you make this change, I think you can drop commonism's commit from this PR since we'd be taking a different approach and not using that commit at all.

[bmw]

@atombrella, do you have the time and interest to respond to my review comments here or would you like someone else to take over this work?

[atombrella]

@atombrella, do you have the time and interest to respond to my review comments here or would you like someone else to take over this work?

Yes. Sorry, I got caught up with other stuff :)

[atombrella]

Thanks for the ping @atombrella. This dropped off my radar somehow.

I'm personally satisfied with testing here since the modified code has 100% test coverage and you added an integration test for the problem. If there's something else you'd like to test for but you're not sure the best way to do it, let me know and I can try to help.

I think we should probably structure the fix differently though. acme.client.ClientNetwork takes an alg parameter specifying the algorithm that should be used in signing JWS. The patch from #8569 ignores this attribute and instead determines the algorithm from the key type in this specific method.

I personally think we should instead do something like one of:

1. Fix where acme.client.ClientNetwork is created in Certbot so the right alg parameter is provided in this case.
2. Keeping in mind that acme.client.ClientNetwork is part of our public API, we could maybe deprecate the alg parameter and always determine the algorithm based on the key type.

I found this comment: # TODO: Allow for other alg types besides RS256 in acme_from_config_key so would make sense to work around this. I'll try to work around this.

If you make this change, I think you can drop commonism's commit from this PR since we'd be taking a different approach and not using that commit at all.

Yeah, makes sense.

[bmw]

No worries! Thanks for the update and your continued interest here.

[atombrella]

@bmw I think the feedback has been addressed to add the parameter into where acme_from_config_key which also had a TODO about this. This PR has been opened for a while :) The explicit alg parameter is probably a good idea, since I don't really see a way to extract the information of the signing algorithm from the key. Maybe in the future, if josepy could include this. The patch here is quite small.

[bmw]

I didn't quite do a full review here, but I wrote up some suggestions inline.

[bmw]

LGTM!