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

Add Azure Signer support #588

Merged
merged 41 commits into from
Jun 1, 2023
Merged

Conversation

malancas
Copy link
Contributor

Fixes: #

Description of the changes being introduced by the pull request:

This pull request adds an Azure signer implementation so we can use Azure's KMS offering Key Vault. This signer currently only supports ECDSA keys. RSA key support can be added later in a follow up pull request.

Please verify and check that the pull request fulfils the following requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

malancas and others added 19 commits May 23, 2023 19:40
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Added packages to requirements and a small bug fix
Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
The CryptographySigner expectes the digest when it performs a signing
operation. It's a nicer API to let the AzureSigner calculate the digest.
Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Make sure to calculated the digest using the correct hash function.
Signed-off-by: Meredith Lancaster <malancas@github.com>
…rmat.

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
@jku
Copy link
Collaborator

jku commented May 26, 2023

Based on a quick glance, looks nice!
Some early comments:

  • tests are failing because the signer modules should be importable even without the optional deps (and cryptography is an optional dep -- you could probably move the import inside the try-except you already have) -- I fixed this one in a later commits
  • tox -e lint runs the same lint tests locally as the CI runs. Please use it, ask if the results don't seem to make sense
  • I think the import_() should not take the fully built "private key URI" as argument -- that forces the caller to understand and be able to build the URI. Instead it should take the logical arguments that azure vault users will understand and the import method should build the URI

Finally a suggestion/comment, feel free to disregard:

malancas and others added 4 commits May 26, 2023 10:51
Signed-off-by: Meredith Lancaster <malancas@github.com>
Specifically
$ black .
$ isort .
These are the trivial looking issues: there are still some left that
require actual decisions (both in pylint and mypy)
To allow the _azure_signer module import when azure dependency modules
are not installed, use stringized annotations (this allows method
definitions to happen without the types existing, while still letting
the static type checkers do their work).
@jku
Copy link
Collaborator

jku commented May 29, 2023

Since you're allowing maintainer pushes (thanks!), I'll take the liberty of adding some annotation and lint fixes:

  • most changes come from running black and isort (like tox -e lint does) but there are some manual lint fixes and some annotation tweaks as well
  • I've only touched things that should not affect intended function
  • the unrelated tests should pass after this
  • there are still linter issues (both pylint and mypy) that require actual changes

Obviously feel free to remove my commits if you we're already working on this and don't need them

It's clear now wheter it's a private uri (azurekms) or an azure uri (https)

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
malancas and others added 3 commits May 30, 2023 06:07
Signed-off-by: Meredith Lancaster <malancas@github.com>
kommendorkapten and others added 2 commits May 30, 2023 14:17
… too

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Update the import method to return a key id that contains the version…
@jku
Copy link
Collaborator

jku commented May 30, 2023

looks good! WRT linter failures:

  • there are some more from pylint and mypy (the tox -e lint run just ends on first error)
  • IIRC most of them are about some azure class not having attributes -- totally fine to skip those checks somehow if that seems appropriate

Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
@jku
Copy link
Collaborator

jku commented May 30, 2023

IIRC most of them are about some azure class not having attributes

scratch that comment: these were seemingly all in the code that you managed to remove in the last commits

Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
@malancas
Copy link
Contributor Author

malancas commented May 30, 2023

@jku All of the linting errors should now be fixed. Thanks for your feedback and help with this PR. I did disable pylint for the too-many-locals rule that was failing on the import_ class method because I find it clearer to use all of the local variables declared in that method. But let me know otherwise.

@kommendorkapten
Copy link
Contributor

We can take this out of "draft" now @malancas right?

@malancas
Copy link
Contributor Author

@kommendorkapten yep, I'll change that now.

@malancas malancas marked this pull request as ready for review May 30, 2023 15:29
Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Great work, @malancas!

securesystemslib/signer/_azure_signer.py Outdated Show resolved Hide resolved
public_key,
)
self.hash_algorithm = self._get_hash_algorithm(public_key)
except UnsupportedKeyType as e:
Copy link
Member

Choose a reason for hiding this comment

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

Should UnsupportedKeyType be public API? Currently it isn't, so a user can't really handle it. Would a built-in exception type also do the trick?

Also, is it really worth to catch, log and re-raise? Doesn't the original error message, which you control, provide enough info? This pattern seems more useful below, where you contextualise the 3rd-party HttpResponseError, although even there I wonder if the stack trace alone might be enough. 🤷

FYI: we have a discussion ticket about Signer exceptions in #468. Curious about your opinions.

Copy link
Collaborator

@jku jku May 31, 2023

Choose a reason for hiding this comment

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

I left a note about this: #588 (comment)

TLDR: I think it's not really public API, it's just part of the larger group of exceptions that we know we may raise that callers are not really meant to handle: GCPSigner handles similar cases by just allowing KeyErrors from dict lookups go through...

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. I was actually trying to challenge the need for a custom exception and the way it is handled internally. But I'm okay with it. The code is still very readable and extra info for the caller is nice.

Copy link
Collaborator

@jku jku left a comment

Choose a reason for hiding this comment

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

I think the code looks good. left some notes but nothing that I think requires changes if you'd like to merge as is.

The one thing I'd like to bring up is testing... I know it's tricky to setup a proper test with the KMS service dependency (I talk a bit more about that in a comment) and we can't require a real time testing setup. But right now we as maintainers are in a situation where we can't see

  • example code -- how is it supposed to be used
  • proof that it actually works anywhere -- logs of a successful signing

Do you have good ideas about this?
Are there public logs of this working?
Should we add testing code (similar to check_kms_signers) even if it is disabled and only works in your fork?

requirements-kms.txt Outdated Show resolved Hide resolved
securesystemslib/signer/_azure_signer.py Outdated Show resolved Hide resolved
securesystemslib/signer/_azure_signer.py Show resolved Hide resolved
securesystemslib/signer/_azure_signer.py Outdated Show resolved Hide resolved
securesystemslib/signer/_azure_signer.py Show resolved Hide resolved
kommendorkapten and others added 2 commits May 31, 2023 14:25
Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
@kommendorkapten
Copy link
Contributor

I'll work on adding azure support in check_kms_signers, I can share credentials with you if you want to replicate the test locally.

@jku
Copy link
Collaborator

jku commented May 31, 2023

I can share credentials with you if you want to replicate the test locally.

Oh no need for that -- would just be good to know that if someone needs to test this, they mostly just need to setup the KMS (and maybe enable the tests or run another tox env or something simple like that).

malancas and others added 3 commits May 31, 2023 07:21
Co-authored-by: Jussi Kukkonen <jku@goto.fi>
Signed-off-by: Meredith Lancaster <malancas@github.com>
…ify signature creation

Signed-off-by: Meredith Lancaster <malancas@github.com>
kommendorkapten and others added 2 commits June 1, 2023 12:44
Added smoke tests for Azure KMS sign.

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Copy link
Collaborator

@jku jku left a comment

Choose a reason for hiding this comment

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

👍 thanks for the test: it works exactly like I expected but it's good to actually see it.

The requirements-kms.txt changes are now not needed (since it is used as requirements for tests.check_kms_signers which doesn't currently include azure). I can remove those changes before merging.

requirements-kms.txt lists the requirements to run
tests.check_kms_signers (which is run on CI): Azure is not currently
part of that test set so the requirements are not needed either.
@malancas
Copy link
Contributor Author

malancas commented Jun 1, 2023

Great, thank you for the reviews!

@jku jku merged commit a4a86db into secure-systems-lab:main Jun 1, 2023
@MVrachev MVrachev mentioned this pull request Aug 30, 2023
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.

4 participants