search

hynek / pem

PEM file parsing in Python.
https://pem.readthedocs.io/
MIT License
156 stars 37 forks source link

Fixes #44. Add support for RFC 4716 objects and SSH.com private keys. #46

Closed adiroiban closed 3 years ago

adiroiban commented 4 years ago

Changes

Fixes #44

I have created a new generically named SSHPublicKey class. RFC 4716 is like the public SSH key format.

But maybe it should be renamed RFC4716PublicKey and reserve SSHPublicKey for the generic class for any SSH Public Key, which in the future might include OpenSSH proprietary formats and other SSH specific formats.

But other than RFC4716 I don't know any other PEM like SSH public key formats... And RFC4716 is not really PEM :)

The other "standard" SSH public key format is OpenSSH public key, but it does not look like a PEM object.

Note that with the current implementation, the order from the input stream is not preserved in the resulting PEM object list.

Also added support for SSH.com / Tectia private keys. The PEM armor is always for "encrypted" even if there is no encryption.

Drive by changes

I have updated the automated test to check that the resulting key is the same as the one from the input, and to not check that we just have the right class.

Pull Request Check List

This is just a friendly reminder about the most common mistakes. Please make sure that you tick all boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing left to do.

adiroiban commented 4 years ago

@hynek when you have time, please take a look.

I did my best to get all the tests green.

I have not idea why lint pre-commit hook failed on the GitHub CI.

On my local system it fails, but is due to a strange Ubuntu python installation.

      File "/home/adi/.cache/pre-commit/repovai_r5c8/py_env-python3.8/lib/python3.8/site-packages/pip/_internal/exceptions.py", line 10, in <module>
        from pip._vendor.six import iteritems
    ModuleNotFoundError: No module named 'pip._vendor.six'

Check the log at /home/adi/.cache/pre-commit/pre-commit.log
ERROR: InvocationError for command /home/adi/chevah/pem/.tox/lint/bin/pre-commit run --all-files (exited with code 1)
hynek commented 4 years ago

You should be able to click into the checks and then open the tox run?

In this case both black and isort are failing.

adiroiban commented 4 years ago

Thanks.. I see now black and isort known_third_party ...not sure why they are failing.

I have executed black and isort on this branch before the commit and no file were modified.

adiroiban commented 4 years ago

@hynek can you please help with the command that I need to run before the commit so that black and isort is happy with my change?

Thanks!

I have no idea why pre-commit is not working ... it looks it creates an invalid virtual environment.. I have no idea from where it creates that environment... as that import is available in my dev environment

$ pre-commit
[INFO] Initializing environment for https://github.com/psf/black.
[INFO] Initializing environment for https://github.com/asottile/seed-isort-config.
[INFO] Initializing environment for https://github.com/pre-commit/mirrors-isort.
[INFO] Initializing environment for https://github.com/pre-commit/mirrors-isort:toml.
[INFO] Initializing environment for https://gitlab.com/pycqa/flake8.
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Installing environment for https://github.com/psf/black.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/home/adi/.cache/pre-commit/repokn6d0ckn/py_env-python3.8/bin/python', '/home/adi/.cache/pre-commit/repokn6d0ckn/py_env-python3.8/bin/pip', 'install', '.')
return code: 1
expected return code: 0
stdout: (none)
stderr:
    Traceback (most recent call last):
      File "/home/adi/.cache/pre-commit/repokn6d0ckn/py_env-python3.8/bin/pip", line 5, in <module>
        from pip._internal.cli.main import main
      File "/home/adi/.cache/pre-commit/repokn6d0ckn/py_env-python3.8/lib/python3.8/site-packages/pip/_internal/cli/main.py", line 10, in <module>
        from pip._internal.cli.autocompletion import autocomplete
      File "/home/adi/.cache/pre-commit/repokn6d0ckn/py_env-python3.8/lib/python3.8/site-packages/pip/_internal/cli/autocompletion.py", line 9, in <module>
        from pip._internal.cli.main_parser import create_main_parser
      File "/home/adi/.cache/pre-commit/repokn6d0ckn/py_env-python3.8/lib/python3.8/site-packages/pip/_internal/cli/main_parser.py", line 7, in <module>
        from pip._internal.cli import cmdoptions
      File "/home/adi/.cache/pre-commit/repokn6d0ckn/py_env-python3.8/lib/python3.8/site-packages/pip/_internal/cli/cmdoptions.py", line 24, in <module>
        from pip._internal.exceptions import CommandError
      File "/home/adi/.cache/pre-commit/repokn6d0ckn/py_env-python3.8/lib/python3.8/site-packages/pip/_internal/exceptions.py", line 10, in <module>
        from pip._vendor.six import iteritems
    ModuleNotFoundError: No module named 'pip._vendor.six'

Check the log at /home/adi/.cache/pre-commit/pre-commit.log

$ python
Python 3.8.2 (default, Mar 13 2020, 10:14:16) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from pip._vendor.six import iteritems
>>>
adiroiban commented 4 years ago

I managed to fix pre-commit execution... it was bug https://github.com/pre-commit/pre-commit/issues/1383 and upstream https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=956144

Working with pre-commit after the commit is ugly... I have no idea why isort was auto-updating the pyproject.toml file.

Pypy3 is still failing...but this looks like a pypy compile bug, and not something related to this repo.

I think that this is ready for review.

adiroiban commented 4 years ago

@hynek for pypy3, instead of building from source, maybe is easier to use https://github.com/pyenv/pyenv and get a precompiled version. This is used on Twisted for Travice-CI runs.

Let me know if you need help with that.

hynek commented 4 years ago

What do you mean by "building from source"? We are using GitHub Action's official pypy3 and we don't use Travis CI at all.

The error seems to come from a failed compilation of typed-ast which is required by a new coverage version and doesn't have a pypy3 wheel.

Given how simple pem is, I will just remove pypy from CI completely, please rebase from/merge master.

Also please revert the changes to contributing. :) We use pre-commit for a very good reason, I'm glad you could figure it out.

adiroiban commented 3 years ago

Hi @hynek

Sorry for the late comeback :)

I have rebased the code and it's now ready for review.

Trying to make the builds green.

It would be nice to have a separate job for lint / mypy / docs.

I see that docs are build on Pythton 3.7 and lint and mypy on 3.8.

But now I know that I can run tox -e lint :)

hynek commented 3 years ago

It would be nice to have a separate job for lint / mypy / docs.

Yeah, that the only downside of GitHub Actions, but you can just click them open to get the data. I hope this gets more ergonomic over time.

hynek commented 3 years ago

Thank you Adi, I'll do some polish and release 21.1 later today!

adiroiban commented 3 years ago

@hynek with the current change, PEM will also parse slightly invalid PEM armors.

For example this is the correct armor 

-----BEGIN RSA PRIVATE KEY-----
CONTENTT
-----END RSA PRIVATE KEY-----

but with the current regex, pem library will be happy with something like this. See spaced in BEGIN and lack of spaces in END

---- BEGIN RSA PRIVATE KEY ----
CONTENTT
-----END RSA PRIVATE KEY-----

or even

-----BEGIN RSA PRIVATE KEY ----
CONTENTT
---- END RSA PRIVATE KEY-----

I would say that this is fine , and can be a feature ... but I think it's important to note that PEM is no longer working in the strict mode :)

But we don't have explicit tests for these cases...so this behaviour is kind of undefined.

If we want to support sloppy PEM files, maybe is best to add a test to make it "official"

Otherwise, we can keep the current regex, but add an extra validation step after the regex output

# Marker for PEM armor based on RFC1421. 5 dashes.
_RFC1421 = object()
# Marker for PEM armor based on RFC4716. 4 dashes + 1 space.
_RFC4716 = object()

_PEM_TO_CLASS = {
    b"CERTIFICATE": (Certificate, _RFC1421),
    b"PRIVATE KEY": (PrivateKey, _RFC1421),
    b"SSH2 PUBLIC KEY": (SSHPublicKey, _RFC4716),
    b"SSH2 ENCRYPTED PRIVATE KEY": (SSHCOMPrivateKey, _RFC4716)
}  # type: Dict[bytes, Type[AbstractPEMObject]]

Then after the object is parsed, we can check for PEM armor type, based on the extra information.

What do you think?

hynek commented 3 years ago

I was aware of that fuzziness but I kinda decided that it's kinda OK? Like what's the harm? It's not like pem is a pem file validator… 🤔

adiroiban commented 3 years ago

I was aware of that fuzziness but I kinda decided that it's kinda OK? Like what's the harm? It's not like pem is a pem file validator…

OK. We can leave it as it is... and see if someone will complain.

The idea is to not encourage people to use invalid files.

Someone might write its own PEM writer and observer that all is ok when parsing with pem but then openssl fails to load the PEM files with errors like

unable to load certificate
140092980806976:error:0909006C:PEM routines:get_name:no start line:../crypto/pem/pem_lib.c:745:Expecting: TRUSTED CERTIFICATE

unable to load certificate
140674857485632:error:0908F070:PEM routines:get_header_and_data:short header:../crypto/pem/pem_lib.c:812:

But maybe this can be a feature. If pem can load it, it means that the intention for the content is to be a certificate or private key, but when you later pass it to openssl and you get an error, you know that you have a format error.

Looking forward for using the new release :)

Thanks!