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

docs(bindings): add example for kms pkey offload #4980

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Dec 16, 2024

Description of changes:

This example demonstrate the pkey offload feature in combination with KMS APIs.

Call-outs:

I structured this example as a test, because it was the only way to keep my sanity while I was writing it. Furthermore, I think this is an excellent item to add to our CI.

While there might be a slight readability hit by structuring it as a "test", I actually think that keeping everything in the same process is a bit of a readability win. There isn't an CLI parsing, etc. So very happy to make the tradeoff.

Testing:

Verified that the test passes locally.

I also opened #4979 to track this.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@maddeleine maddeleine left a comment

Choose a reason for hiding this comment

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

I really just have nits. I don't know if we can make the example better without fixing our underlying issues.

/// s2n-tls requires that future have 'static bounds, so this function can not
/// operation on `&self`. Instead we clone all of the necessary elements and
/// capture them in the closure.
async fn async_pkey_offload(
Copy link
Contributor

Choose a reason for hiding this comment

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

Err I guess I have no suggestions here, but this is really weird. I see what you mean about the ConnectionFuture not really being idiomatic.

Comment on lines +103 to +107
// > The public key that AWS KMS returns is a DER-encoded X.509 public key,
// > also known as SubjectPublicKeyInfo (SPKI), as defined in RFC 5280.
// > When you use the HTTP API or the AWS CLI, the value is Base64-encoded.
// > Otherwise, it is not Base64-encoded.
// https://docs.aws.amazon.com/kms/latest/developerguide/download-public-key.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I prefer the link first so that I know what I'm reading

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I feel like that's a bit non-standard w.r.t block quotes, e.g. apa.

@jmayclin jmayclin requested a review from lrstewart January 15, 2025 18:29
* make error message parameterized
Comment on lines +150 to +151
//> If this is an OperationType::Sign operation, then this input has
//> already been hashed and is the resultant digest.
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a quote from somewhere, it needs attribution. If it's not a quote, idk what the ">" are about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants