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

Passing Ring types and not a T: Read? #8

Open
barzamin opened this issue Oct 10, 2018 · 12 comments
Open

Passing Ring types and not a T: Read? #8

barzamin opened this issue Oct 10, 2018 · 12 comments

Comments

@barzamin
Copy link

What was the rationale behind the http_signatures API taking readers into key files (and, for RSA, parsing the DER inside http_signatures) instead of passing, eg, ring::signature::RSAKeyPair or ring::hmac::SigningKey?

(yes, i know the project has been moved to your gitea, but i can't leave issues there 😄)

@asonix
Copy link
Owner

asonix commented Oct 10, 2018

That's a good point, actually. I was still fairly new to how APIs and stuff should be structured and how things in rust work, but I figured allowing people to supply File or Cursor types to read in keys made sense. Requiring that keys already be in the correct format makes more sense (and avoids issues with async things getting blocked on a filesystem read).

I think I'll go ahead and update this library to do that this weekend

@barzamin
Copy link
Author

oh, thanks for the response! that sounds good.

@asonix
Copy link
Owner

asonix commented Oct 14, 2018

I've updated the crate to now take an untrusted::Input as the key, and there's no longer a GetKey trait. Does that work for you?

@asonix
Copy link
Owner

asonix commented Oct 14, 2018

actually, it might make the MOST sense to do something like

enum Key {
    RSA(RsaKeyPair),
    HMAC(SigningKey),
}

impl From<RsaKeyPair> for Key {
    fn from(key: RsaKeyPair) -> Self {
        Key::RSA(key)
    }
}

impl From<SigningKey> for Key {
    fn from(key: SigningKey) -> Self {
        Key::HMAC(key)
    }
}

fn verify<K: Into<Key>>(..., key: K, ...) {
    ...
}

@asonix
Copy link
Owner

asonix commented Oct 14, 2018

what are your thoughts?

@asonix
Copy link
Owner

asonix commented Oct 14, 2018

I tried my hand at the CreationKey idea, which works a bit different from my example above.

https://git.asonix.dog/asonix/http-signatures/pulls/1

my main issue is that verify requires an untrusted::Input type, so using Ring's key types for the create makes the API feel off.

@barzamin
Copy link
Author

barzamin commented Oct 15, 2018

ahhhhhhhhhhhh, hmmm. i like the Into<Key> idea! but, yeah, that, or CreationKey, do create an unpleasant asymmetry between create/verify :/

is it possible to impl TryFrom for untrusted::Input and use a single Key type?

@asonix
Copy link
Owner

asonix commented Oct 16, 2018

I'd actually prefer not to use TryFrom. My most recent changes work without TryFrom, and actually the base library compiles on stable rust now (everything works except rocket).

As far as Into<Key> it's a little difficult since SigningKey know's it's Sha Size and RSAKeyPair doesn't.

Maybe a greater refactoring is in order, and maybe I just need to split the logic for HMAC from the logic for RSA. Who's even going to be using HMAC HTTP Signatures anyway?

@asonix
Copy link
Owner

asonix commented Oct 16, 2018

I think I prefer the CreationKey type to an IntoKey trait anyway, since either way the coercion to a valid type is defined in application code. I cant just impl IntoKey for Input because HMAC requires knowing a sha size to produce a key

@barzamin
Copy link
Author

ahhhhhhhh, i see

yeah
creationkey is fine by me

@asonix
Copy link
Owner

asonix commented Oct 16, 2018

Here's the release with CreationKey in it: https://git.asonix.dog/asonix/http-signatures/releases

try it out and let me know what could be better

@barzamin
Copy link
Author

thanks! i'm gonna be working on httpsigs in rustodon soon so i'll let you know...

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

No branches or pull requests

2 participants