Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DNSSEC Algorithm Refactor #944

Merged
merged 62 commits into from
Jun 25, 2023
Merged

Conversation

jschlyter
Copy link
Contributor

@jschlyter jschlyter commented Jun 17, 2023

Refactor DNSSEC Algorithms by moving all algorithm specific code to dedicated classes in dns.dnssecalgs:

  • AlgorithmPublicKey implements public key operations; signature validation, encoding, to/from DNSKEY
  • AlgorithmPrivateKey implements private key operations; signing and public key conversion.

New algorithms can be registered using register_algorithm_cls. Private algorithms may be registered with a DNS name prefix or a BER-encoded OID (represented as bytes to avoid dependencies on ASN.1 libraries) prefix. Non-private algorithms are registered without prefixes.

No API changes to dns.dnssec.

Based on discussion in #935

@jschlyter jschlyter requested a review from bwelling June 21, 2023 19:23
dns/dnssecalgs/__init__.py Outdated Show resolved Hide resolved
dns/dnssecalgs/__init__.py Outdated Show resolved Hide resolved
@jschlyter jschlyter requested a review from bwelling June 22, 2023 13:13
dns/dnssecalgs/__init__.py Outdated Show resolved Hide resolved
@jschlyter jschlyter requested a review from bwelling June 23, 2023 19:23
@rthalley
Copy link
Owner

This is looking good, and everything is building and testing happily. Here are a few more nits :)

AlgorithmKeyMismatch moved to exception.py but unlike the other things that moved, it is not imported into dnssec.py. We should do that for backwards compatibility, even though we are no longer raising it in dnssec.py.

Ruff, Flake8, and pylint all complain about two long lines:

poetry run ruff dns
dns/dnssecalgs/base.py:53:89: E501 Line too long (94 > 88 characters)
dns/dnssecalgs/base.py:58:89: E501 Line too long (92 > 88 characters)
Found 2 errors.

These long lines don't get formatted by black as they are in docstrings, but just line wrapping them yourself should do.

pylint complains about a bunch of stuff, including unused-import, ungrouped-imports in dnssec.py. This can be fixed with comments to suppress them on those lines.

Other pylint complaints follow...

These seem valid:

dns/dnssecalgs/cryptography.py:13: [W0231(super-init-not-called), CryptographyPublicKey.init] init method from base class 'GenericPublicKey' is not called)
dns/dnssecalgs/cryptography.py:16: [W1116(isinstance-second-argument-not-valid-type), CryptographyPublicKey.init] Second argument of isinstance is not a type)
dns/dnssecalgs/cryptography.py:37: [W0231(super-init-not-called), CryptographyPrivateKey.init] init method from base class 'GenericPrivateKey' is not called)
dns/dnssecalgs/cryptography.py:40: [W1116(isinstance-second-argument-not-valid-type), CryptographyPrivateKey.init] Second argument of isinstance is not a type)

These are confusing, as there's nothing wrong with abc.ABC. I'm thinking it has some sort of magic around "import abc" as if I do that instead of the from style import, it is happy.

dns/dnssecalgs/base.py:1: [E0611(no-name-in-module), ] No name 'ABC' in module 'abc')
dns/dnssecalgs/base.py:1: [E0611(no-name-in-module), ] No name 'abstractmethod' in module 'abc')

These are just nuisances, as you don't need a pass if you have a docstring. Maybe just make sure everything has a docstring and remove the passes?

dns/dnssecalgs/base.py:22: [W0107(unnecessary-pass), GenericPublicKey.verify] Unnecessary pass statement)
dns/dnssecalgs/base.py:48: [W0107(unnecessary-pass), GenericPublicKey.from_dnskey] Unnecessary pass statement)
dns/dnssecalgs/base.py:54: [W0107(unnecessary-pass), GenericPublicKey.from_pem] Unnecessary pass statement)
dns/dnssecalgs/base.py:59: [W0107(unnecessary-pass), GenericPublicKey.to_pem] Unnecessary pass statement)
dns/dnssecalgs/base.py:72: [W0107(unnecessary-pass), GenericPrivateKey.sign] Unnecessary pass statement)
dns/dnssecalgs/base.py:77: [W0107(unnecessary-pass), GenericPrivateKey.public_key] Unnecessary pass statement)
dns/dnssecalgs/base.py:85: [W0107(unnecessary-pass), GenericPrivateKey.from_pem] Unnecessary pass statement)
dns/dnssecalgs/base.py:90: [W0107(unnecessary-pass), GenericPrivateKey.to_pem] Unnecessary pass statement)

The ways I invoke ruff, flake8, and pylint are in the Makefile if you want to run them locally.

@rthalley
Copy link
Owner

I'm also happy to handle the linting if you prefer; it just has to get done before we do a release :)

@jschlyter
Copy link
Contributor Author

@rthalley I've fixed most of the lint errors now, if you can take a look at the rest that would be swell.

Btw, any reason you are not using isort ?

@rthalley
Copy link
Owner

Re isort, the answer is "ignorance" :). I just tried it and it looks pretty good, though it did cause some warnings to resurrect by deleting a comment. I will turn it on and run it, but only AFTER merging your changes :)

@rthalley
Copy link
Owner

Ok, I looked at your current branch and I'm ready to squash and merge it if you are, so let me know. The remaining issues pylint is having are really pylint issues, and I'll silence them after the merge.

@jschlyter
Copy link
Contributor Author

I'm good, let's merge!

@rthalley
Copy link
Owner

Thanks for taking on another big job!

@rthalley rthalley merged commit 7d3cde8 into rthalley:master Jun 25, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants