unparsed: expose content of the extension

[baloo]

Unparsed is intended to hold the value of the extension. Specification calls for:

    byte             SSH_AGENTC_EXTENSION
    string           extension type
    byte[]           extension request-specific contents

But does not mandates for content to be serialized according to ssh regular serialization format (as in implements ssh_encoding::Decode).

The Unparsed::From implementation allows for content to be formatted in any serialization, but you can't read data back unless it was encoded according to SSH serialization.

This partially reverts #67

[baloo]

I would be happy to add an hazmat feature if needed, or rename it like as_raw or something to highlight this is dangerous, but then I would argue From<Vec<u8>> should be removed too and replaced by:

impl Unparsed {
   pub fn new<T>(v: T) where T: ssh_encoding::Encoding -> Result<Self> {
   }
}

to highlight that.

[wiktor-k]

Okay, I like the "as raw" naming. Maybe to keep the symmetry we'd need "from raw" which takes a vec of unparsed data and doesn't interpret it in any way?

I like this new constructor. Do I read the signature well that it is intended for ssh encod-able data not for raw stuff?

I mean... we need raw support for crazy folks that want to use serde there (just when we got rid of it 😉).

One potential addition, not sure if good, is implementing AsRef<[u8]>. This would allow treating Unparsed as byte buffers (the implementation is the same as what you have here, just... more idiomatic (?)).

[baloo]

AsRef<[u8]> makes accessing the content not-so-explicit imho, which renders the use of a type system a bit moot.

[baloo]

Just to be transparent about the intent:
I have my own protocol over extensions. The messages in the payload are serialized with serde because it makes it easier to work with than having to serialize using ssh's encoding.

Arguably I could just shove everything in a Vec and hand that to Unparsed, but that makes for a double (or triple) serialization, and I find it somehow irritating.

[overhacked]

Summary

PR #73 opened to show proposed alternative.

1. Existing impl Encode for Unparsed just writes the raw bytes to the buffer, per spec
2. AsRef<[u8]> would allow for the serde::Deserialize use case
3. Adding Unparsed::into_bytes(self) -> Vec<u8> would allow for serde::de::DeserializeOwned

Discussion

The Encode implementation for Unparsed does follow the spec, as it just writes the raw bytes to the buffer it is given:

ssh-agent-lib/src/proto/message/unparsed.rs

Lines 32 to 39 in 845c218

	fn encode(&self, writer: &mut impl Writer) -> ssh_encoding::Result<()> {
	// NOTE: Unparsed fields do not embed a length u32,
	// as the inner Vec<u8> encoding is implementation-defined
	// (usually an Extension)
	writer.write(&self.0[..])?;
	Ok(())
	}

Regarding the AsRef<[u8]> vs Unparsed.as_raw() conversation, I lean towards AsRef, because it can be used in existing generic code that has AsRef<[u8]> argument bounds. I don't think it's circumvents the type system the way Deref<[u8]> would, because AsRef coercions are not automatic; a function that takes AsRef<T> has to call T.as_ref() to get the reference. (With Deref, any caller could "accidentally" call a &[u8] function on Unparsed with no explicit conversion.)

Regarding From<Vec<u8>> vs Unparsed.from_raw(), I'd keep the idiomatic impl From because it correctly implies that there is no difference in the data stored in a Vec<u8> and an Unparsed, while from_raw() implies that the internal representation changes.

Arguably I could just shove everything in a Vec and hand that to Unparsed, but that makes for a double (or triple) serialization, and I find it somehow irritating.

I don't think there is any double- or triple-serialization or even allocation happening in this case, because the From<Vec<u8>> takes ownership of the Vec you provide without copying or cloning. When Encode::encode is called, the bytes are in fact copied into the outgoing message buffer, but that occurs with all of the types that implement Encode, not just Unparsed. @baloo, is there something redundant here that I don't see?

If we wanted to provide an owned-not-borrowed deserialization path, I'd propose the inverse of From<Vec<u8>> as Unparsed::into_bytes(self) -> Vec<u8> that consumes the Unparsed, unwrapping the inner bytes.

[baloo]

@baloo, is there something redundant here that I don't see?

No there is not redundancy, I completely missed that part:

The Encode implementation for Unparsed does follow the spec, as it just writes the raw bytes to the buffer it is given:

I can go with #73 beside the small comment I made there.

[baloo]

superseded by #73