Operations contracts #82
Conversation
CDimonaco
added
go
CDimonaco
force-pushed
the
operations-contracts
branch
from
626582d to
73f041f
Compare
CDimonaco
force-pushed
the
operations-contracts
branch
from
73f041f to
21b9463
Compare
There was a problem hiding this comment.
Hey @CDimonaco
Super!
Some few things:
- Let's remove everything not related to the elixir contracts code, to have it slim. Actually, the golang code must be updated as well.
- Adding the expiration time to all the messages, including the unsigned opens a new paradox. For example, right now, wanda evaluation has a timeout of 5 minutes to set the execution as timed out. With the new expiration time, this behaviour changes a lot, as most of the times, the timeout happens because the machine was not actually responding. We would need to set really carefully the expiration time for checks execution to be big enough to cover things we cover now.
|
|
||
| CloudEvents.CloudEvent.encode(cloud_event) | ||
| time_str = Integer.to_string(time) | ||
| signature = :public_key.sign("#{data}#{time_str}#{expire_at}", :sha256, signing_key) |
There was a problem hiding this comment.
question:
do we need to add the expire_at at the signature? I mean, just with the time the replay attack is solved, and it might make creating the signature in golang a bit longer.
Not a big deal though
|
|
||
| defp verify_event_signature(_, _), do: {:error, :invalid_event_signature} | ||
|
|
||
| defp verify_event_validity( |
There was a problem hiding this comment.
nitpick: this should go above verify_event_signature
| end | ||
| end | ||
|
|
||
| defp verify_event_validity(_), do: {:error, :event_expired} |
There was a problem hiding this comment.
nitpick: maybe we should return a different error expire_at_not_received
not a big deal, but just to maybe differenciate between errors.
I'm not even sure of this though
| id = Keyword.get(opts, :id, UUID.uuid4()) | ||
| source = Keyword.get(opts, :source, "trento") | ||
| validity_in_seconds = Keyword.get(opts, :validity_in_seconds, @default_event_validity) |
There was a problem hiding this comment.
nitpick: maybe some other name like valid_for or ttl?
I mean, the in_seconds looks a bit strange. Maybe just documenting would be enough.
I have not seen many of these things in elixir though
|
|
||
| assert {:ok, %Test.Event{id: ^event_id}} = Trento.Contracts.from_event(encoded_cloudevent) | ||
| end | ||
| describe "message with signature" do |
There was a problem hiding this comment.
nitpick: maybe signed message. and the other describe could be unsigned message
| assert {:error, :invalid_envelope} = Trento.Contracts.from_signed_event(event, public_key) | ||
| end | ||
|
|
||
| test "should return error if the could not be decoded", %{ |
There was a problem hiding this comment.
typo: if the cloud event cannot be decoded
| assert {:error, :invalid_envelope} = Trento.Contracts.from_event(event) | ||
| end | ||
|
|
||
| test "should return error if the could not be decoded" do |
| @@ -112,7 +116,40 @@ func EventType(src []byte) (string, error) { | |||
| return string(name), nil | |||
| } | |||
|
|
|||
| func VerifySignature(src []byte, publicKey any) error { | |||
There was a problem hiding this comment.
you didn't change this, right?
i mean, it cannot work as you are not getting the expire_at value
|
@arbulu89 Thanks for the review, I think we can make the expiration time in the unsigned version optional but mandatory in the signed ones, wdyt? |
| source: source | ||
| } | ||
| end | ||
|
|
||
| defp add_signature( | ||
| %CloudEvents.CloudEvent{ | ||
| data: {:proto_data, %Google.Protobuf.Any{value: data}}, | ||
| attributes: | ||
| %{ | ||
| "time" => %CloudEvents.CloudEventAttributeValue{ | ||
| attr: {:ce_timestamp, %{seconds: time}} | ||
| }, | ||
| "expire_at" => %CloudEvents.CloudEventAttributeValue{ | ||
| attr: {:ce_timestamp, %{seconds: expire_at}} | ||
| } | ||
| } = current_attrs | ||
| } = event, | ||
| private_key_content | ||
| ) do | ||
| signing_key = | ||
| private_key_content | ||
| |> :public_key.pem_decode() | ||
| |> Enum.at(0) | ||
| |> :public_key.pem_entry_decode() | ||
|
|
||
| CloudEvents.CloudEvent.encode(cloud_event) | ||
| time_str = Integer.to_string(time) | ||
| signature = :public_key.sign("#{data}#{time_str}#{expire_at}", :sha256, signing_key) |
There was a problem hiding this comment.
I don't think protobuf serde is canonical. See for instance this RFC as an example of canonicalization over JSON. Signing+verification (and many cryptographic operations) over non-canonical structures are not reliably repeatable, if I am understanding this correctly. WDYT?
This PR adds the new protobuf stubs for agent operations.
With this new feature the support for message signing and expiration is also added.