brycx / pasetors

PASETOrs: PASETO tokens in pure Rust
MIT License
83 stars 8 forks source link

plans for v3.public #40

Closed Eh2406 closed 2 years ago

Eh2406 commented 2 years ago

What is your plans re v3.public will you be supporting it? If so do you have an ETA?

brycx commented 2 years ago

Sorry for the late response.

I don't have any plans regarding v3.public currently. I haven't been needing it myself, nor has anyone requested it. Is this something you'd want to be using? If so, I can look further into it.

Eh2406 commented 2 years ago

It is not a late response. It is your project, you can respond when you have time and want to. Thank you for making the project and responding to my question!

I have a use case for it. The context is the conversation starting at https://github.com/rust-lang/rfcs/pull/3139#issuecomment-920296667 Fundamentally, we (cargo) want to sign some json (so PASETO is a good option) but we want to allow the key to be stored on existing hardware tokens (so Ed25519/v4.public is not going to work).

brycx commented 2 years ago

I have some questions I'm hoping you could clarify upon a bit.

Also, just out of curiosity, do you already know what hardware tokens you'll be using for storage?

Eh2406 commented 2 years ago

Fundamentally I do not have a security background. I will do my best to answer, but they all come with "I don't know until I talked to my security expert".

Is v3.public the only one you need? You won't need v3.local?

Confirmed. The "local" is for an entirely different use case.

PASETO defines deterministic nonces, with P-384, as a SHOULD in the spec, but can fallback to CSPRNG. Do you know if you have any hard requirements on the use of deterministic nonces? I intend to provide an implementation using deterministic nonces, if possible/feasible.

I don't care, but I will have to talk to a security expert.

PASETO requires public keys to use point compression, so that's probably the in/out requirements that'll be used in pasetors public API as well. Is this format suitable for your storage requirements, or do you need uncrompressed public keys as well?

I don't quite know what these words mean, but I don't think I need any particular format for the public key. We can document whatever the code does.

PASETO specifies encoding for keys in v3.public, is this something you'd want/need? It'll probably be added anyway at some point, just want to know if this is something I should prioritize.

That sounds interesting! Do you have the link?

Also, just out of curiosity, do you already know what hardware tokens you'll be using for storage?

No. It is a "bring your own hardware" situation. If we had control over the hardware we would use v4, and give people compatible hardware.

brycx commented 2 years ago

I don't quite know what these words mean, but I don't think I need any particular format for the public key. We can document whatever the code does.

Compressed public key is a X-coordinate for the point on the curve along with a parity bit for the Y-coord. So instead of storing a (x,y) pair, you store parity || x, and compute Y when needed for signing/verification. Hope this helps in clarifying a bit. Sorry if it's too jargoned.

Re encoding, it's the one called PASERK. I'm on mobile atm and can't get the link so this reference will have to suffice until I can. PASERK link in bottom of PASETO spec README.

Eh2406 commented 2 years ago

From the sound of it compressed works just fine for our needs.

PASERK. I have heard of that. I don't know if we will be needing it, I will be asking the relevant security experts later today.

Eh2406 commented 2 years ago

As the conversation has continued will need a small portion of PASERK. Namely public, secret, and the operation ID

brycx commented 2 years ago

I've started the work for compressed points and PASERK encoding.

Eh2406 commented 2 years ago

Thank you!

brycx commented 2 years ago

I have a working implementation of both v3.public and the additional PASERK encoding ID. There is still some polishing work that is needed and two or three things that need investigation, in addition to other things on the issue tracker currently.

A note: I cannot at the time support deterministic nonces, which PASETO recommends. The only suitable P-384 implementation currently available seems to be ring, which doesn't support this. However, I've seen work on this elsewhere, e.g RustCrypto, but the P-384 implementation there is not ready yet. When it is, I will probably switch to that one but I don't know when this will be.

My plan so far is to do a pre-release 0.5.0 within a couple days containing the functionality.

Eh2406 commented 2 years ago

Thank you so much! I look forward to experimenting with your prerelease as soon as it's out.

I will talk to my neighborhood cryptographers about deterministic nonces, before we stabilize this for production.

brycx commented 2 years ago

I've published the 0.5.0 prerelease on crates.io. Please be aware, that there may be some rough edges left, but the majority of the testing regarding v3 is done (specifically, in regards to point compression).

Feel free to let me know if you find something you'd like revised.

Another thing that crossed my mind, since you asked for the ID operation: Are you going to be putting these into the footer of the tokens? If yes, I've had plans to make a typed implementation of a footer, which may be of use to you in that case? In any regard, I don't know when that could be added, since it's not the biggest priority currently.

Eh2406 commented 2 years ago

Thank you! I look forward to giving it a try!

I am putting the ID in the footer. But, I am happy to work with an untyped API. Improvements are welcome when they're ready.

brycx commented 2 years ago

I'm leaving it with an untyped API for now then. I'll put up a PR for final review today and expect to have the release of 0.5.0 out in a few days max. Again, let me know if you find something that needs change.

One thing to note is: The PASERK ID operation is currently used with a Id::from(&some_key) and Id::fmt(&mut buf), much like the other types. However, Id does not implement TryFrom<String> like the normal keys do. I could not think of a use-case where this would be needed, but if this needs to be added just let me know. That would be a straight-forward non-breaking change.

Eh2406 commented 2 years ago

I'm sorry I had other work come up and have not had a chance to test your pre release. I very much hope to experiment with it sometime this week.

brycx commented 2 years ago

No problem. I'll hold off the release until you've been able to give it a try then.

Eh2406 commented 2 years ago

So I have tried to use this and I am starting to have some feedback.

https://docs.rs/pasetors/0.5.0-alpha.2/pasetors/index.html#creating-and-verifying-public-tokens is the only docs I saw on how to generate a key-pair, unfortunately it seams to have bit rotted. Like what version of rand dose this work with? Where is generate defined? Where is Keypair defined?

Next I wanted to follow the worked out examines in https://rust-lang.github.io/rfcs/3231-cargo-asymmetric-tokens.html#a-complicated-publish-operation

I successfully round tripped a Paserk PublicKey into a AsymmetricPublicKey<V3> and back to its Paserk string! I was able to get the Id for AsymmetricPublicKey<V3> and have it match the RFC!

I was not able to verify the token:

    let pub_key: AsymmetricPublicKey<V3> = "k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ".to_owned().try_into().unwrap();
    let token = "v3.public.eyJjaGFsbGVuZ2UiOiAiY2hhbGxlbmdlIiwgIm11dGF0aW9uIjogInB1Ymxpc2giLCAibmFtZSI6ICJmb28iLCAidmVycyI6ICIwLjAuMCIsICJja3N1bSI6ICJmN2RiYjZhY2ZlZmYxZDQ5MGZiYTY5M2E0MDI0NTZmNzZiMzQ0ZmVhNzdhNWU3Y2FlNDNiNTk3MGMzMzMyYjhmIiwgInN1YiI6ICJwcml2YXRlLWtleS1zdWJqZWN0IiwgImlhdCI6ICIyMDIyLTAyLTI4VDE4OjMzOjI0KzAwOjAwIn36ifmVYCSBYcjHVjQ_JD6R16dcWPEjHYVFOR7QRx3riOLiH7o-m236uNs2NEu-NzOCDZZbsVXvxhop-aUKRc9D-jphV5KFuC8y6mNLklfg1PpH37QeDsyzJDZy604gZ5c.eyJ1cmwiOiAiaHR0cHM6Ly9yZWdpc3RyeS1jaGFsbGVuZ2Utc3ViamVjdC5jb20vY3JhdGUtaW5kZXgiLCAia2lkIjogImszLnBpZC5RQjNXTkJQLTVqLTBYUVYyTU91dnVPY0xsSjh1ei1wbXF0SVp1czF4M1lUdSJ9";
    PublicToken::verify(&pub_key, &token, None, None).unwrap();
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: TokenValidation'

I think the problem is that the token has a footer but I don't know the value until I verify it. I would expect that footer = Some(&[]) is "fail if it has a footer", and footer = None make shore the sig is correct including the footer in the token whatever it is.

My the way how do I go from a token its parts? Like how to I get back the string I used message and footer when I called sign?

Next how do you get the AsymmetricPublicKey for a AsymmetricSecretKey? Also why does PublicToken.sign take both when one can come from the other?

brycx commented 2 years ago

First of all, thanks so much for taking the time to provide valuable feedback. It really means a lot!

This got a bit lengthy, apologies for that.

Keypair and generate()

Click to expand... > https://docs.rs/pasetors/0.5.0-alpha.2/pasetors/index.html#creating-and-verifying-public-tokens is the only docs I saw on how to generate a key-pair, unfortunately it seams to have bit rotted. Like what version of rand dose this work with? Where is generate defined? Where is Keypair defined? Yes, I agree. This part is indeed very unclear. `pasetors` does not actually provide functionality to generate keypairs. The only hint there is for this, is looking at line 4 of the example, where an import of `Keypair` happens from `use ed25519_dalek::Keypair;`. To answer yours questions here concretely: both `Keypair` and `generate()` are defined in `ed25519_dalek`. It works with the `rand` version that `ed25519_dalek` pulls in. Thinking about it, it seems weird to require users to pull in dependencies which `pasetors` already relies on, in order to generate keypairs. At least if the user isn't on `no_std`. I think adding a `std`-gated `generate()` function to the `AsymmetricKeypair` type would solve this confusion and improve usability. Additionally, I've been thinking about switching out `ed25519_dalek` with [`ed25519-compact`](https://crates.io/crates/ed25519-compact) because `ed25519_dalek` seems to have been stuck on older `rand` versions for quite a while. How does this plan sound to you? Would it solve your issue? **Edit**: Seems there may be an issue with getting a private key from `ring` as it does not allow access to it from its API directly. I'll have to look more into this.

I successfully round tripped a Paserk PublicKey into a AsymmetricPublicKey and back to its Paserk string! I was able to get the Id for AsymmetricPublicKey and have it match the RFC!

Perfect!

Footer None vs. Some(&[])

Click to expand... > I was not able to verify the token: [...] Yeah, this should be the missing footer causing the error. > I would expect that footer = Some(&[]) is "fail if it has a footer", and footer = None make shore the sig is correct including the footer in the token whatever it is. I can see what you mean. I think it might be confusing though, to have `None` represent the path of taking the footer from the token. It seems a bit ambiguous: does `None` mean I expect there to be no footer or does it mean I have no footer to give? The same for `Some(&[])` in that case. We can definitely implement it the way you propose and clearly document the differences. Another approach I have had in mind for some time, is to have a `UnverifiedFooter` type, which can be created from a token and then used to pass a token into `verify()` with an `AsRef<&[u8]>` impl. Would look something like the following: ```rust let pub_key: AsymmetricPublicKey = "k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ".to_owned().try_into().unwrap(); let token = "v3.public.eyJjaGFsbGVuZ2UiOiAiY2hhbGxlbmdlIiwgIm11dGF0aW9uIjogInB1Ymxpc2giLCAibmFtZSI6ICJmb28iLCAidmVycyI6ICIwLjAuMCIsICJja3N1bSI6ICJmN2RiYjZhY2ZlZmYxZDQ5MGZiYTY5M2E0MDI0NTZmNzZiMzQ0ZmVhNzdhNWU3Y2FlNDNiNTk3MGMzMzMyYjhmIiwgInN1YiI6ICJwcml2YXRlLWtleS1zdWJqZWN0IiwgImlhdCI6ICIyMDIyLTAyLTI4VDE4OjMzOjI0KzAwOjAwIn36ifmVYCSBYcjHVjQ_JD6R16dcWPEjHYVFOR7QRx3riOLiH7o-m236uNs2NEu-NzOCDZZbsVXvxhop-aUKRc9D-jphV5KFuC8y6mNLklfg1PpH37QeDsyzJDZy604gZ5c.eyJ1cmwiOiAiaHR0cHM6Ly9yZWdpc3RyeS1jaGFsbGVuZ2Utc3ViamVjdC5jb20vY3JhdGUtaW5kZXgiLCAia2lkIjogImszLnBpZC5RQjNXTkJQLTVqLTBYUVYyTU91dnVPY0xsSjh1ei1wbXF0SVp1czF4M1lUdSJ9"; let unverified_footer = UnverifiedFooter::try_from(token)?; PublicToken::verify(&pub_key, &token, Some(unverified_footer.as_ref()), None).unwrap(); ``` I'm leaning more towards the typed approach, as this seems most clear to me. Do you have any preferences or see any issues with providing an `UnverifiedFooter` type?

Individual token parts after verification

Click to expand... > My the way how do I go from a token its parts? Like how to I get back the string I used message and footer when I called sign? This is an oversight on my part. The API is designed in a way that expects both parties to have knowledge of the footer, treating it more like the `implicit-assert`. This is of course not always the case. The header is constant, so if you need this one, I can make the `const` public. There is missing a way to easily get the message and footer after verification, this seems obvious looking at it now. With the current API, it'd be a `split_at('a')` and some base64 decoding. Not great. Expanding upon the idea of a `UnverifiedFooter` type, what if we have a `UntrustedToken` and `TrustedToken`. The function signatures for `verify()` would become something like: ```rust verify( public_key: &AsymmetricPublicKey, token: &UntrustedToken, footer: Option<&[u8]>, implicit_assert: Option<&[u8]>, ) -> Result {} ``` where the new types would have the follwoing functionality: ```rust let ut_footer = UntrustedToken.get_footer(); let tt_message = TrustedToken.get_message(); let tt_footer = TrustedToken.get_footer(); ``` `UntrustedToken` doesn't need to be able to return `message` before it's been verified, but as you mentioned, you do need to be able to get the footer. All `get_` functions would return base64-decoded data. Maybe it would suffice if the functions `sign()`/`verify()` simply returned a `Token` type that would hold the invidual parts of the token. In a way that users would'nt be able to create a `Token` themself, but only get it from `sign()`/`verify()`. This is not a final design, but what I came up with while writing this. What would you think about this kind of approach? Of course, `verify()` could also just return the decoded message and then stick to `UnverifiedFooter`.

Next how do you get the AsymmetricPublicKey for a AsymmetricSecretKey?

There is no way currently. I can provide TryInto<AsymmetricPublicKey<V4>> for AsymmetricSecretKey<V4> in terms of Ed25519 (v4 or v2), but I haven't found a similar funcitonality provided by ring (v3). Are you not able to store both the public and private key used?

Also why does PublicToken.sign take both when one can come from the other?

Because computing the public key from the private key can be a costly operation in terms of resource-constrained devices. So if both parts are already available, one can avoid re-computing a public key for one single private key if used in mulitple sign operations.

Eh2406 commented 2 years ago

This is getting long, should we open up separate issues about each thing?

I think adding a std-gated generate() function to the AsymmetricKeypair<V> type would solve this confusion and improve usability. How does this plan sound to you? Would it solve your issue?

That sounds much more ergonomic! This does not prevent leaving in the code path that uses the dependencies directly, if there are user controls that are better offered by going to the dependencies. But a generate() that uses safe defaults sounds like a really valuable add.

Footer None vs. Some(&[])

If None and Some(&[]) have the same semantic meaning, then you don't need the Option. &[] Is cheap even on no_std environments. If we do give it semantically different meaning, then I see your point about Option not making that clear, we should use some type that has better names. For example FooterFromToken vs FooterMustEqual(foo).

I'm leaning more towards the typed approach, as this seems most clear to me. Do you have any preferences or see any issues with providing an UnverifiedFooter type?

UnverifiedFooter seams fine. Note that the standard specifies:

If f is not empty, implementations MAY verify that the value appended to the token matches some expected string f, provided they do so using a constant-time string compare function.

So we have a full matrix of checked footer vs trusted footer and constant time compare vs fast compare. My use case wants trusted footer + fast compare, it seems a shame if the API constrains me to read the footer in only to checked footer + constant time compare.

Individual token parts after verification

With the current API, it'd be a split_at('a') and some base64 decoding. Not great.

This is not too bad, but unfortunate considering the library already has the code to do this.

Expanding upon the idea of a UnverifiedFooter type, what if we have a UntrustedToken and TrustedToken.

I really like this idea! I think there might be use for a UntrustedToken.get_untrusted_message(); for example if the key identifier is put in the message as opposed to the footer. However, this API should have a long name and lots of documentation discouraging its use.

I can provide TryInto<AsymmetricPublicKey<V4>> for AsymmetricSecretKey<V4>

Given the shape of your API, this was the method I checked the docs to see if you had. It also lets you only provided where the underlying crypto library makes it available.

Are you not able to store both the public and private key used?

The design document assumed this method would be available, and so does not call for the public key being stored. It can be stored in the users config file, but it opens up possibilities of somebody tampering with their config files and having them no longer match. This can definitely be worked around for now, but I would really like to have it fixed before this functionality is stabilized.

Because computing the public key from the private key can be a costly operation in terms of resource-constrained devices.

That makes perfect sense! Hadn't thought of it.

brycx commented 2 years ago

I have created separate issues for the three different topics. I assume the footer discussion will eventually "merge" with UntrustedToken/TrustedToken, but I'd like to get something clarified:

So we have a full matrix of checked footer vs trusted footer and constant time compare vs fast compare. My use case wants trusted footer + fast compare, it seems a shame if the API constrains me to read the footer in only to checked footer + constant time compare.

Not sure I fully understand everything here.

This library requires a constant-time compare of footer values if the user supplies Some() footer value during verify()/decrypt().

My use case wants trusted footer + fast compare, it seems a shame if the API constrains me to read the footer in only to checked footer + constant time compare.

I don't see the difference between checked footer and trusted footer. In pasetors we first compare the footer to some expected string, if user provides this, and then it's also a part of data signed/authenticated as part of the PASETO spec. Concretely, this means that a user must provide the same footer on verification as was used during creation of the token. We have no fast compare in pasetors. You can't provide None during verification if Some() was used during creation.

Eh2406 commented 2 years ago

I think I am paying more attention to performance for my one use case than is quite appropriate. The API you propose can work for me, and further optimization is probably not needed.

brycx commented 2 years ago

I'll close this issue. All discussed points have either been merged into the prerelease branch v0.5.0 (including the typed footer that was shortly mentioned) or have been moved to a separate issue (the case for secret->public).

If anything is still missing, I can re-open it.