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

feat: add DAG-JOSE implementation #177

Closed
wants to merge 1 commit into from

Conversation

nathanielc
Copy link

This is an implementation of DAG-JOSE. There are also a few minor changes to existing code.

  • An update to the dag-cbor-derive code in order to allow usage of the macro from within this repo.
  • Add a Bytes type to dag-cbor to make declaring bytes within a struct more straightforward.
  • A dag-jose feature

There are some things left to do before I would consider this PR ready. However I would like feedback on the general design first. You can see its usage in the fixtures.rs test case.

  • Provide complete examples in docs
  • Determine what to do about the fixture tests. We likely want to move that logic to https://github.com/ipld/codec-fixtures
  • Add Ipld conversion to the types to avoid round-tripping through CBOR for JSON encoding.

This is an implementation of DAG-JOSE. There are also a few minor
changes to existing code.

* An update to the dag-cbor-derive code in order to allow usage of the
  macro from within this repo.
* Add a `Bytes` type to dag-cbor to make declaring bytes within a struct
  more straightforward.
* A dag-jose feature

There are some things left to do before I would consider this PR ready.
However I would like feedback on the general design first. You can see
its usage in the fixtures.rs test case.

* Provide complete examples in docs
* Determine what to do about the fixture tests. We likely want to move
  that logic to https://github.com/ipld/codec-fixtures
* Add Ipld conversion to the types to avoid round-tripping through CBOR
  for JSON encoding.
/// Bytes is sequence of byte values.
///
/// Implements Encode and Decode to/from CBOR byte strings
pub type Bytes = Box<[u8]>;
Copy link

Choose a reason for hiding this comment

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

I'm a little confused as to why this is needed. It seems like Vec is covered. Also why Box<[u8]> and not Vec.

Copy link
Author

@nathanielc nathanielc Jan 27, 2023

Choose a reason for hiding this comment

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

It stems from here https://github.com/ipld/libipld/blob/master/dag-cbor/src/decode.rs#L297 and here https://github.com/ipld/libipld/blob/master/dag-cbor/src/decode.rs#L321

Using a Vec<u8> creates an IPLD list of u8 values, where using Box<[u8]> creates an IPLD byte sequence.

We could change the cbor Decode implementations but this was the smallest change I could make to existing code. Additionally we could try and specialize the implementation but that required Rust nightly.

Copy link

Choose a reason for hiding this comment

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

LIttle odd that they didn't handle Vec as a byte string, but is there a reason we need a ByteString here, and an array of u8 isn't sufficient. A large number of libraries work on Vec or bytes::Bytes, so to use this, they'll have to be regularly calling into_boxed_slice() to work with the types specified here.

dag-pb actually uses bytes::Bytes. Oddly, it also returns a Box<[u8]> for the payload here https://github.com/ipld/libipld/blob/master/dag-pb/src/codec.rs#L78 rather than the Vec or a Bytes structure.

let protected = base64_url::decode("eyJhbGciOiJFZERTQSJ9").unwrap();
let signature = base64_url::decode("-_9J5OZcl5lVuRlgI1NJEzc0FqEb6_2yVskUaQPducRQ4oe-N5ynCl57wDm4SPtm1L1bltrphpQeBOeWjVW1BQ").unwrap();
(
payload.into_boxed_slice(),
Copy link

Choose a reason for hiding this comment

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

Seems like this should just be to_vec().

)
}
fn fixture_jws_base64(
payload: &Box<[u8]>,
Copy link

Choose a reason for hiding this comment

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

And this should just take &[u8]

@vmx
Copy link
Member

vmx commented Jan 30, 2023

This is not an in-depth review, but a few general notes.

The way I propose to use DAG-CBOR is via https://crates.io/crates/serde_ipld_dagcbor. It is based on Serde, this way you get access to the Serde ecosystem. Whenever you define Serde (de)serialization for some type, you can transform it into IPLD, that's a huge win. Given that, I hope we can move away from the current dag-cbor crate in this repo and also from dag-cbor-derive. The idea would be that instead of dag-cbor-derive, you would use Serde directly instead. I guess that most of its functionality is already be possible with Serde, the missing pieces could be implemented. The benefit of this approach is, that it would then work for other (future) serde_ipld_dag* codecs automatically and you won't need to re-implement things for a specific codec.

Given all that I propose keeping the DAG-JOSE codec in a separate repo. If changes to dag-cbor-derive are still needed, please open a separate PR for that. In regards to dag-cbor changes, generally I'd like to keep changes to a minimum as I consider it legacy, but I'm also not fully opposed changes if it enable users.

@vmx
Copy link
Member

vmx commented Jan 30, 2023

It would be great to have DAG-JOSE in the codec-fixtures repo. There's also already some work there: ipld/codec-fixtures#11.

@nathanielc
Copy link
Author

Thanks for the comments. I have implemented the changes against a new repo. ceramicnetwork/rust-dag-jose#1

@nathanielc nathanielc closed this Feb 7, 2023
@nathanielc nathanielc deleted the dag-jose branch February 7, 2023 18:26
@nathanielc nathanielc restored the dag-jose branch February 7, 2023 18:26
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.

3 participants