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

Unable to verify a caller based on a public key #395

Closed
paulyoung opened this issue Dec 8, 2022 · 14 comments
Closed

Unable to verify a caller based on a public key #395

paulyoung opened this issue Dec 8, 2022 · 14 comments

Comments

@paulyoung
Copy link
Contributor

I'm struggling to verify a caller based on a public key. I generated a private key with openssl ecparam -name secp256k1 -genkey -noout -out identity.pem and a public key with openssl ec -in secp256k1.pem -pubout -out public.pem

I create an identity with let identity = Secp256k1Identity::from_pem_file(private_key_path)?; which gives me a principal of tvfic-z7h73-2yknr-uyp26-pru5d-bjfbs-3tdnz-mctcw-mkj5d-uecaw-aae

Based on the source code for Secp256k1Identity it looks like I need to provide a der-encoded public key to Principal::self_authenticating:

fn sender(&self) -> Result<Principal, String> {
Ok(Principal::self_authenticating(
self.der_encoded_public_key.as_ref(),
))
}

I'd prefer to support a workflow where users can copy and paste the contents of the public key PEM file, and based on how Secp256kIdentity is encoding the public key I found the following which seems to be exactly what I need:

https://github.com/RustCrypto/elliptic-curves/blob/078363da500b04a031bcefb41bd25363b7ce1df7/p256/tests/pkcs8.rs#L57-L63

This is what I'm doing but the public key fails to parse.

use ic_agent::identity::{Identity as _, Secp256k1Identity};
use p256::pkcs8::EncodePublicKey as _;

fn main() {
    let identity = Secp256k1Identity::from_pem_file(private_key_path)?;

    // This represents the caller as the canister sees it.
    let caller = identity.sender().map_err(|err| anyhow!(err))?;
    trace!("caller: {}", caller);

    // Below represents what I'm trying to do in a canister.

    let public_key = vec![
        "-----BEGIN PUBLIC KEY-----",
        "MFYwEAYHKoZIzj0CAQYFK4EEAAoDQgAERzzyM2WgleyVhRLy9UpXBg8UZkvRNCTY",
        "E7X4b0wxP8XED9VZpqKi0n+CBhh7Wgf1o/qCE64kxfzZSs9yOY95xg==",
        "-----END PUBLIC KEY-----",
        "",
    ]
    .join("");

    // Fails with `Error: crypto error`
    let public_key = public_key.parse::<p256::PublicKey>()?;

    let public_key = public_key.to_public_key_der()?;

    let principal_from_public_key = Principal::self_authenticating(public_key);
    trace!("principal from public key: {}", principal_from_public_key,);

    trace!(
        "caller matches principal from public key: {}",
        caller == principal_from_public_key,
    );
}

Lastly, I was quite confused about Secp256k1Identity vs BasicIdentity. BasicIdentity supports ED25519 so I tried to use that first, but I couldn't figure it out.

openssl on my machine didn't support that algorithm which I thought presented a barrier for people, and even then you'd get a v1 key which doesn't guarantee that the private key matches the public key.

Curious if ED25519 is still the better option.

@paulyoung
Copy link
Contributor Author

These are the keys I'm testing with.

identity.pem.zip
public.pem.zip

@adamspofford-dfinity
Copy link
Contributor

adamspofford-dfinity commented Dec 8, 2022

p256 is the crate for secp256r1 keys, the IC uses secp256k1 keys which are provided by the k256 crate. Also, that vec![].join call produces a pem without newlines, which won't parse.

@adamspofford-dfinity
Copy link
Contributor

BasicIdentity could more descriptively be named Ed25519Identity. You're right that OpenSSL doesn't support it; that's one of the big reasons we support secp256k1 (the other being that Bitcoin uses it). But it is in a general sense better, which is why the IC primarily targets it.

@paulyoung
Copy link
Contributor Author

Also, that vec![].join call produces a pem without newlines, which won't parse.

Thanks, .join("\n") was my intention 🤦‍♂️

@paulyoung
Copy link
Contributor Author

p256 is the crate for secp256r1 keys, the IC uses secp256k1 keys which are provided by the k256 crate.

Thanks again. I was using that at first but changed crates when I found the test I linked to and tried to verify that it worked the same on my system.

@paulyoung
Copy link
Contributor Author

paulyoung commented Dec 8, 2022

But it is in a general sense better, which is why the IC primarily targets it.

By "it" do you mean ed25519Identity or secp256k1?

@paulyoung
Copy link
Contributor Author

You're right that OpenSSL doesn't support it

Just to clarify on this, apparently newer versions of OpenSSL support -algorithm ed25519 but asking people to upgrade might be asking too much.

I tried using ssh-keygen since that supports Ed25519 but apparently couldn't generate a key pair in a format that ring would allow me to convert to an Ed25519KeyPair, even when using -m PEM.

If you know how to make this work, I'd appreciate the insight.

Also dfx rejects v1 keys but that seems to be only because agent-rs doesn't parse them:

Ok(BasicIdentity::from_key_pair(Ed25519KeyPair::from_pkcs8(
pem::parse(&bytes)?.contents.as_slice(),
)?))

I'm not sure if it was a conscious decision to only support v2 keys with from_pkcs8 when from_pkcs8_maybe_unchecked is available.

@paulyoung
Copy link
Contributor Author

For some reason, in my standalone example k256 = "0.11" was fine but in my canister k256 = { version = "0.11", features = ["pem"] } was required or I got this error:

let public_key = public_key.parse::<k256::PublicKey>()
                            ^^^^^ the trait `FromStr` is not implemented for `k256::elliptic_curve::PublicKey<k256::Secp256k1>`

@paulyoung
Copy link
Contributor Author

It also looks like this may not work in a canister as-is since it appears to be trying to use getrandom and suggests enabling the js feature.

Not sure why randomness is even needed for verification so maybe I can turn off a default feature.

Will look into providing something like I did here if not: https://github.com/codebase-labs/ic-auth-tokens/blob/76113cbaa8b788b1989a9d5ef0868cdb12ee46af/crates/ic-auth-tokens/src/lib.rs#L75-L95

@paulyoung
Copy link
Contributor Author

k256 = { version = "0.11", default-features = false, features = ["arithmetic", "pem"] } seems to be sufficient.

@adamspofford-dfinity
Copy link
Contributor

the trait `FromStr` is not implemented

Sounds like you got that backwards - FromStr is only implemented if the pem feature is enabled, and isn't if it's disabled.

If you know how to make this work, I'd appreciate the insight.

The one we use for our tests, I generated in Rust via k256.

By "it" do you mean ed25519Identity or secp256k1?

Ed25519, especially for the speed.

@domwoe
Copy link
Member

domwoe commented Dec 8, 2022

k256 = { version = "0.11", default-features = false, features = ["arithmetic", "pem"] } seems to be sufficient.

yes, I used k256 with default-features = false in canisters before.

@paulyoung
Copy link
Contributor Author

If you know how to make this work, I'd appreciate the insight.

The one we use for our tests, I generated in Rust via k256.

Thanks. I thought that might be it. I was hoping for something that people would likely already have installed.

@paulyoung
Copy link
Contributor Author

Thanks for all the help.

I'll make a PR to update the documentation with what was learned here.

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

3 participants