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

Abstract signing cryptography #21

Merged
merged 3 commits into from Aug 12, 2021
Merged

Conversation

puiterwijk
Copy link
Contributor

This allows implementing signing and verification with more than openssl
PKey(Ref), like a TPM or AWS KMS keys (#5).

It implements the abstracted methods for PKeyRef, and PKey calls out to
PKeyRef.

Signed-off-by: Patrick Uiterwijk patrick@puiterwijk.org

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the Apache License Version 2.0, as specified in the LICENSE file of this repository.

@puiterwijk puiterwijk force-pushed the abstract-crypto branch 3 times, most recently from ec28015 to 9eed7d2 Compare July 6, 2021 19:06
@puiterwijk
Copy link
Contributor Author

The way it is implemented with traits and impls for PKeyRef and PKey, it is still compatible with the old API (it should not be needed to update existing code that is using this).

@petreeftime
Copy link
Contributor

Nice! I actually had something similar in the works, but with dyn Traits instead, so that you could potentially use more than one crypto provider at a time. Seemed useful to have both AWS KMS and OpenSSL, for example; but since this is complete, I am inclined to merge this instead and maybe rework it later on.

@petreeftime
Copy link
Contributor

Actually, this seems to be very similar indeed. Will need to go through a more thorough review, since it's a pretty large change.

@puiterwijk
Copy link
Contributor Author

@petreeftime thanks. Let me know if you want me to rebase this patch.

Also, I submitted implementations for KMS and TPM, but I think those might very well be better positioned as external crates (e.g. aws-nitro-enclaves-cose-tpm), so that it's easier to maintain by other folks if you aren't interested in maintaining the TPM implementation (I'd be happy to maintain that crate separately).
Luckily that is totally possible with this implementation: any external crate can implement this trait.

I mostly submitted those two implementations to show off that this trait implementation makes that absolutely possible.

@puiterwijk
Copy link
Contributor Author

I just rebased this on the current main branch.

@puiterwijk
Copy link
Contributor Author

I fixed the clippy needless borrows in this PR, since this PR would again have merge conflicts if it goes in after fixes for that were to go in.


[features]
default = ["key_openssl_pkey"]
key_openssl_pkey = []

Choose a reason for hiding this comment

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

Can you make openssl an optional dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would make more sense when we add a secondary crypto provider such as ring.

Choose a reason for hiding this comment

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

The traits as proposed in this PR can immediately be implemented by external crates, so I don't see why the OpenSSL dependency should be required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, openssl itself is still needed for encryption (which I did not abstract in this PR), and we still use it for things like computing digests before passing them to the crypto layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have submitted #28, which makes openssl a fully optional dependency (well, it is abstracted away, right now it's the only crypto implementation and you'll need one).

src/crypto/mod.rs Outdated Show resolved Hide resolved
src/sign.rs Show resolved Hide resolved
src/sign.rs Show resolved Hide resolved
src/sign.rs Show resolved Hide resolved
src/sign.rs Show resolved Hide resolved
@petreeftime
Copy link
Contributor

@petreeftime thanks. Let me know if you want me to rebase this patch.

Also, I submitted implementations for KMS and TPM, but I think those might very well be better positioned as external crates (e.g. aws-nitro-enclaves-cose-tpm), so that it's easier to maintain by other folks if you aren't interested in maintaining the TPM implementation (I'd be happy to maintain that crate separately).
Luckily that is totally possible with this implementation: any external crate can implement this trait.

I mostly submitted those two implementations to show off that this trait implementation makes that absolutely possible.

I don't see a reason why the TPM implementation couldn't be maintained here, I don't think it would end up being touched too much after the inital implementation. If this doesn't work out for whatever reason, we can figure out if we can be moved somewhere else instead. I think all we need to do in that case is to have the interface for the PrivateKey and PublicKey as public.

This allows implementing signing and verification with more than openssl
PKey(Ref), like a TPM or AWS KMS keys (awslabs#5).

It implements the abstracted methods for PKeyRef, and PKey calls out to
PKeyRef.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
@puiterwijk
Copy link
Contributor Author

puiterwijk commented Aug 12, 2021

The error is due to bitflags 1.3.0, 1.3.1: that now has a MSRV of 1.45, 1.46, which was released Aug 11, 22:52, Aug 12, 03:34.

Bitflags 1.3.0/1.3.1 requires Rust 1.46 to compile, and that is dragged
in via the openssl package.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
@petreeftime petreeftime merged commit c38c8ec into awslabs:main Aug 12, 2021
@puiterwijk puiterwijk deleted the abstract-crypto branch August 12, 2021 19:19
@runcom runcom mentioned this pull request Oct 5, 2021
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

4 participants