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

Broad strokes of how this package might look #1

Merged
merged 13 commits into from
Oct 25, 2019
Merged

Broad strokes of how this package might look #1

merged 13 commits into from
Oct 25, 2019

Conversation

glasserc
Copy link
Contributor

@glasserc glasserc commented Oct 21, 2019

Here's a code dump of some stuff that does something, although it isn't really complete yet.

The current state of master has a bunch of the boring project infrastructure, so you might check that out too. Some stuff is still missing, like CI and 100% code coverage.

A lot of this code is based on Normandy's signing.py, but restructured a little bit to use cryptography instead of asn1parse and fastecdsa. Unfortunately, we still depend on ecdsa to parse the signature format Autograph gives us. (There may be another way to do this, which I'd love to hear about.)

Still missing:

  • Actually use the cache
  • Verify timestamps on certificates are correct
  • Verify that the certificates chain together
  • Verify subject name
  • May as well rewrite ecdsa utility to avoid ecdsa dependency

/cc @jvehent @mozbhearsum

@glasserc
Copy link
Contributor Author

N.B. This refs mozilla-services/telescope#106 although it doesn't close it.

I decided to just dump everything into the __init__ module because I didn't have any particular opinion about how stuff should be organized. I don't envision the file growing to be too much bigger so maybe that's OK.

The name python_autograph_utils seems like a pretty reasonable, obvious name but I admit it looks a little rough in an import statement. Maybe autograph_utils would make a better Python slug.

Tests are based around a real Autograph signature, taken from my local dev environment using Autograph 2.7.0. The cert is https://raw.githubusercontent.com/mozilla-services/autograph/master/docs/statics/normandy.content-signature.mozilla.org-20210705.dev.chain. Tests are not unit tests against mocks but functional tests. (Unit tests could be useful too but the functional ones seemed essential.)

The cookiecutter template I used automatically set up an entry point for a CLI. I don't do anything with it yet, but it seems like exposing verify-x5u and verify-signature as CLI utilities would be nice!

Copy link
Collaborator

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLI utils could be cool yes!

.isort.cfg Outdated Show resolved Hide resolved
python_autograph_utils/__init__.py Outdated Show resolved Hide resolved
python_autograph_utils/__init__.py Outdated Show resolved Hide resolved
tests/test_python_autograph_utils.py Outdated Show resolved Hide resolved
tests/test_python_autograph_utils.py Outdated Show resolved Hide resolved
python_autograph_utils/__init__.py Outdated Show resolved Hide resolved
Copy link

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a client/wrapper for calling Autograph itself be included in this package, or do you think that's out of scope?

README.rst Show resolved Hide resolved
@glasserc
Copy link
Contributor Author

I should also mention that I decided to use aiohttp because it's the new hotness. This precludes use on Python < 3.4 or so, but I think that's probably OK?

@bhearsum
Copy link

I think excluding Python < 3.4 is fine at this point. However, it does mean that any consumers must be using asyncio (at least to call into here), which makes it more annoying for synchronous programs to use. This will be annoying for Balrog at the moment, but I'm sure we can work around.

I could argue that SignatureVerifier should be pure though, and require you to pass it an x5u instead of an URL.

@glasserc
Copy link
Contributor Author

I think including an Autograph client in this package is a great idea! I filed #2 about it.

I was also thinking that it would be good to define a pure function for verifying a signature if you already have the x509 chain. I'm a little nervous about this because we do additional validity checks on the certificate chain that don't need to be repeated, so long as the certificate chain is the same. I'd like to defer this question until later. I've filed #3 to discuss this further.

Copy link
Collaborator

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great!

I cannot tell too much about ABC, I haven't used them, but it looks like you know what you're doing :)

autograph_utils/__init__.py Outdated Show resolved Hide resolved
autograph_utils/__init__.py Outdated Show resolved Hide resolved
@glasserc
Copy link
Contributor Author

I'm going to merge this and continue work on the remaining issues in another PR. Thanks everyone who looked at it!

@glasserc glasserc merged commit 5595451 into master Oct 25, 2019
@glasserc glasserc deleted the first-cut branch October 25, 2019 14:30
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.

3 participants