-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add initial PASERK support for PASERK-types local,public,secret #24
Conversation
@not-my-profile You can take a look and check if this suits your needs. |
Thanks for implementing this so quicky! :D I am wondering if you have a reason for introducing the 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 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 |
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. |
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. |
Ok, I'll open a PR :) Edit: opened #25 |
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.
I don't see how a struct helps with that at all. This again imho screams for a trait like |
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.
Yes, I agree with this completely.
I don't know either now and honestly don't remember what my thinking was.
I agree on this. A
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 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 Then 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 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. |
So with my above comment in mind, this PR should be updated to (according to my proposal):
Optionally, take |
You're welcome :) I need a good Paseto Rust library for my application (which I'm also planning on open sourcing).^^
Can you give an example of one key type mapping to multiple PASERK types? |
|
I thought that |
They do need additional parameters, but it's still We could have a |
Ah, yeah you want Rust types for PASERK types so that you can implement traits for them in the future; that makes sense.
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 Btw I think we should introduce the generics (#25) before implementing PASERK types. |
Alright, I can agree to this - seems like a nice approach. And yes, leaving And yes of course, we'll add generics to keys before this. |
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. |
closes #23
Outdated todo
TODO:
paserk
modulepub
Paserk
(local_from
->parse_local
)Paserk::value
field todata
to refer clearer to specToString
impl forPaserk
TryFrom for AsymmetricSecretKey
as the PASERK-key's secret key is different from how pasetors handles themPaserk
should fail validation ifdata
is invalid base64.