Event Signing #438
Event Signing #438
Conversation
nateprewitt
force-pushed
the
sign_event
branch
3 times, most recently
from
1bb083a to
fb46168
Compare
nateprewitt
force-pushed
the
sign_event
branch
4 times, most recently
from
15240f3 to
0ae0af4
Compare
| context.properties["identity"] = identity | ||
| context.properties["signer_properties"] = auth_option.signer_properties |
There was a problem hiding this comment.
Nit: These should probably use the typed properties.
These are generic and I think something reasonable to always have after auth resolution, so I think we can leave them here.
There was a problem hiding this comment.
I think we're entirely refactoring this with Jordon's upcoming work. Ideally we're not doing this at all going forward.
codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java
Show resolved
Hide resolved
| return result | ||
|
|
||
| def encode_headers(self, headers: HEADERS_DICT): | ||
| def encode_headers(self, headers: HEADERS_DICT) -> None: |
There was a problem hiding this comment.
I understand why this method builds an internal buffer - but the interface does seem weird. It seems like we could either:
- have this as a class method which would construct an instance, call _encode_headers and then return the result. or
- Have a private class that handles the build/state, _EventHeaderEncoder which encode_headers would instantiate and return the result from.
There was a problem hiding this comment.
Yeah, I'd generally agree with this, the encode workflow here is a bit odd. I think we can look at a refactor as follow up, this PR is just fixing the typing issue from the original.
There was a problem hiding this comment.
Yeah this was intended for streaming out without an intermediary. Then we switched to have an intermediary
packages/aws-event-stream/src/aws_event_stream/_private/serializers.py
Outdated
Show resolved
Hide resolved
| class EventSigner(Protocol): | ||
| """A signer to manage credentials and EventMessages for an Event Stream lifecyle.""" | ||
|
|
||
| def sign_event( |
There was a problem hiding this comment.
I'd like to move this to the regular signer. We can create a central event dataclass or protocol to give it. That should solve the double initialization problem. That can be later though.
There was a problem hiding this comment.
We need to track signing state (previous signature) between events so theres some state management. But we could also add previous signature as an input to sign_event and store it in the publisher.
There was a problem hiding this comment.
Yeah my original plan had been for this context to be in the Publisher rather than the signer. What we have implemented currently makes that a bit harder. I think raising it a level to let the publisher do the orchestration makes more sense long term though.
There was a problem hiding this comment.
We can do that without muddying up the interface too much. Just make an event_signer method on the auth scheme class or whatever that's given the entire transport request and returns the signer to use. A stateless signer could just return itself.
Co-authored-by: Nate Prewitt <nate.prewitt@gmail.com>
3 participants
This PR is a draft implementation of Event signing in
aws-sdk-signersand some temporary scaffolding changes to enable their usage. We have a few spots right now where our implementations don't align nicely. I've added some shims to help test while we discuss a longer term solution.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.