Add initial PASERK support for PASERK-types local,public,secret

[brycx]

closes #23

Outdated todo

TODO: - [X] Make `paserk` module `pub` - [X] Re-think function names for `Paserk` (`local_from` -> `parse_local`) - [X] Add tests (especially test vectors) - [X] Rename `Paserk::value` field to `data` to refer clearer to spec - [X] Add `ToString` impl for `Paserk` - [X] Fix impl of `TryFrom for AsymmetricSecretKey` as the PASERK-key's secret key is different from how pasetors handles them - [X] `Paserk` should fail validation if `data` is invalid base64.

[brycx]

@not-my-profile You can take a look and check if this suits your needs.

[not-my-profile]

Thanks for implementing this so quicky! :D

I am wondering if you have a reason for introducing the Paserk struct? To me it seems like the parse functions could directly return the keys and the serialization functions could directly return a string? I don't quite see the point of an intermediary struct.

I would suggest a trait like:

pub trait WriteAsPaserk {
    fn write_as_paserk(&self, write: &mut dyn std::fmt::Write) -> std::fmt::Result;
}

which could be easily implemented for SymmetricKey and AsymmetricPublicKey (and also potentially avoids an unnecessary string allocation^^) but it wouldn't work for secret because the API currently needs the following check (which I notice is also in PublicToken::sign):

        if secret_key.version != Version::V4 || public_key.version != Version::V4 {
            return Err(Errors::KeyError);
        }

It would be cool if we could somehow avoid that check in general. That is turning the runtime check into a compile time check. Maybe it would be an idea to make the public crypto keys generic over their version? Then we could introduce:

pub struct AsymmetricKeypair<V> {
    pub secret_key: AsymmetricSecretKey<V>,
    pub public_key: AsymmetricPublicKey<V>,
}

seems more rusty i.e. making invalid states unrepresentable (so that if it compiles it most likely works^^).

Then we could also implement WriteAsPaserk for AsymmetricKeypair<V>. What do you think?

[brycx]

The main reason is, that PASERK is a rather large extension to PASETO. If this crate is going to support more PASERK types is the future, I'd like it to be as self-contained as possible, so it's easier to feature-gate (as more deps might be needed etc).

Also, some PASERK types should be allowed in the footer, if more types are indeed added. It'll be easier (I hope) to control excatly which PASERK types are added in the footer at that point.

That said, it might change, since the crate isn't at 1.0.0 yet.

[brycx]

You do raise some valid concerns in the examples/suggestions.

I'll look into your suggestions some more in the following days.

If you want to, feel free to make a draft PR with an API that matches your suggestions. At least for me, it's always easier to look at the actual code.

[not-my-profile]

Ok, I'll open a PR :) Edit: opened #25

[not-my-profile]

If this crate is going to support more PASERK types is the future, I'd like it to be as self-contained as possible, so it's easier to feature-gate (as more deps might be needed etc).

I think it makes sense to have local,public,secret PASERK support as part of the core library, since that doesn't require additional dependencies. I don't see how my trait based approach would pose a problem for implementing the rest of PASERK as an optional module.

Also, some PASERK types should be allowed in the footer, if more types are indeed added. It'll be easier (I hope) to control excatly which PASERK types are added in the footer at that point.

I don't see how a struct helps with that at all. This again imho screams for a trait like FooterSafe that can then simply be implemented for all types that are safe to be put in the footer.

[brycx]

First off, thank you so much for taking the time to provide super valuable feedback and investing time to improve this.

I've been thinking some more about this.

I think it makes sense to have local,public,secret PASERK support as part of the core library, since that doesn't require additional dependencies.

Yes, I agree with this completely.

I don't see how my trait based approach would pose a problem for implementing the rest of PASERK as an optional module.

I don't know either now and honestly don't remember what my thinking was.

This again imho screams for a trait like FooterSafe that can then simply be implemented for all types that are safe to be put in the footer.

I agree on this. A FooterSafe trait for a given PASERK type that is allowed in the footer is the way to go.

---

I don't see how a struct helps with that at all.

I see what you mean. The intention was also, to either differentiate between PASERK type as traits or as structs. What I've been thinking about so far is, it seems to me that implementing each PASERK-type (local,public,secret,etc) as a newtype over a String makes the most sense. The reason is:

Each PASERK-type is represented differently, even if the same key is used. Thereby, splitting the serialization logic into such types seems the most explicit. Then we can control exactly which keys can be converted to and from different PASERK-types. If we were to have different traits for each PASERK-type, then it wouldn't allow us to support serde easily, because if AsymmetricSecretKey implements serde::Serialize how does that impl know to which PASERK-type the key should be serialized to? (I know, the current PR only has one PASERK-type for each key-type, but this may not always be the case)

Then FooterSafe trait could also be applied selectively to different PASERK-types.

This is also related to:

pub trait WriteAsPaserk {
    fn write_as_paserk(&self, write: &mut dyn std::fmt::Write) -> std::fmt::Result;
}

This is a fine trait, but it's unclear which of the many PASERK-types this functions serializes &self to. Again, currently fine since there's only one, but...

I hope I made a clearer argument this time than before. Let me know if something seems off.

---

Lastly, I agree that the current keys re. versioning are not optimal and could use some improvement.

[brycx]

So with my above comment in mind, this PR should be updated to (according to my proposal):

• Remove the Paserk struct
• Define Local, Public and Secret struct and implement serialization logic there
• Implement From<SymmetricKey> for Local, From<AsymmetricPublicKey> for Publicand From<AsymmetricSecretKey> for Secret

Optionally, take AsymmetricKeyPair into account if added.

[not-my-profile]

First off, thank you so much for taking the time to provide super valuable feedback and investing time to improve this.

You're welcome :) I need a good Paseto Rust library for my application (which I'm also planning on open sourcing).^^

I know, the current PR only has one PASERK-type for each key-type, but this may not always be the case

Can you give an example of one key type mapping to multiple PASERK types?

[brycx]

Can you give an example of one key type mapping to multiple PASERK types?

SymmetricKey represents the local variant of PASETO, so it could be serialized to PASERK types like:

• local
• seal
• local-pw

[not-my-profile]

I thought that seal and local-pw need additional parameters to be converted from/to the keys (i.e. asymmetric wrapping key or password respectively). So I thought that they would need a separate API anyway, but maybe I'm misunderstanding PASERK.

[brycx]

They do need additional parameters, but it's still SymmetricKey that gets serialized. It's because of the need for a different API, I'm thinking of the type-separation, using structs, for different PASERK-types.

We could have a WriteToPaserk trait, which would only do local etc, but then if other PASERK types are added, and the parameters are different, what would then be the approach to make a coherent API?

[not-my-profile]

Ah, yeah you want Rust types for PASERK types so that you can implement traits for them in the future; that makes sense.

it seems to me that implementing each PASERK-type (local,public,secret,etc) as a newtype over a String makes the most sense

I think newtyping String would make for an awkward API (and also require a potentially unnecessary string allocation for serialization). So I'd rather have the new types wrap their components instead of the serialized strings. I'd avoid String newtyping the local,public,secret types. So for example:

use core::convert::TryFrom;

pub struct Error;
pub struct SymmetricKey;

pub trait FormatAsPaserk {
    fn fmt(&self, write: &mut dyn std::fmt::Write) -> std::fmt::Result;
}

pub struct LocalPwKey(String);
impl LocalPwKey {
      pub fn new(key: SymmetricKey, password: String) -> Self { ... }
}
// etc.

impl FormatAsPaserk for SymmetricKey {
    fn fmt(&self, write: &mut dyn std::fmt::Write) -> std::fmt::Result {
        todo!()
    }
}
impl FormatAsPaserk for LocalPwKey {
    fn fmt(&self, write: &mut dyn std::fmt::Write) -> std::fmt::Result {
        todo!()
    }
}
// etc.

impl TryFrom<String> for SymmetricKey {
    type Error = Error;
    fn try_from(value: String) -> Result<Self, Self::Error> {
        todo!()
    }
}

impl TryFrom<String> for LocalPwKey {
    type Error = Error;
    fn try_from(value: String) -> Result<Self, Self::Error> {
        todo!()
    }
}

Note that there is no struct LocalKey(SymmetricKey); since it wouldn't add anything (and make for a more cumbersome API ... even with From conversions).

Btw I think we should introduce the generics (#25) before implementing PASERK types.

[brycx]

Alright, I can agree to this - seems like a nice approach. And yes, leaving LocalKey(SymmetricKey) to be just FormatAsPaserk for SymmetricKey is fine by me. I guess I just hadn't realized how you intended one might expand upon the API you suggested initially.

And yes of course, we'll add generics to keys before this.

[not-my-profile]

I just updated the code. We still want to use String newtypes for the footer-safe PASERK types because you might deal with footer-safe keys for which you don't have the secret.