add feature to load an account by private key #94

axos88 · 2025-03-17T20:36:54Z

Addresses #93

djc · 2025-03-17T22:00:32Z

src/lib.rs

+    pub async fn load(
+        private_key_pkcs8_der: &[u8],
+        server_url: &str,
+        http: Option<Box<dyn HttpClient>>,


Not sure the Option type really makes sense for http? If it is Some you don't want to pull in the DefaultClient dependendencies; if it is None you need to. Which one do you want to use?

Ahh, I did not see that was behind a feature gate.

djc · 2025-03-17T22:01:42Z

src/lib.rs

+            None => Client::new(server_url, Box::new(DefaultClient::try_new()?)).await?,
+        };
+
+        let key = Key::from_pkcs8_der(private_key_pkcs8_der)?;


I think we should probably externalize this, instead make Key::from_pkcs8_der() public, and have this function take a Key.

I first did it that way, but wasn't sure you were open to extend the area of public API.

But to be honest, if we are making Key part of the public API, it would make more sense to add the Key as the param of the Account::create as well, so that the user has the ability to create an account with a chosen key instead of forcing to generate one for them.

And in that case for_existing_account would probably just be a convenience wrapper around that.

djc · 2025-03-17T22:01:59Z

src/lib.rs

+            &ignored_account, // This field is ignored as per rfc8555 7.3.1
+            (key, pkcs8),
+            None,             // This field is ignored as per rfc8555 7.3.1


These comments don't quite make sense in their current form.

The first comment is there to explain why a dummy ignored_account is created and why sending that as part of the request is okay. The second one might need to be removed - agreed.

djc · 2025-03-17T22:03:18Z

src/lib.rs

+    ///
+    /// The returned [`AccountCredentials`] can be serialized and stored for later use.
+    /// Use [`Account::from_credentials()`] to restore the account from the credentials.
+    pub async fn load(


Let's call this for_existing_account() and move it up above from_parts()?

cpu · 2025-03-17T22:10:26Z

It would be nice to see test coverage for this as well. Maybe a Pebble integration test (https://github.com/djc/instant-acme/blob/main/tests/pebble.rs) that:

Spins up a Pebble env, and registers a new account with it
Re-creates the account from just the private key, and verifies that works with the new API
Drops the old env
Spins up a second Pebble env
Tries to use the account private key from the first env to recreate the account: this should error

axos88 · 2025-03-18T09:54:33Z

How can I run the pebble test locally?

djc

Please squash all your commits.

djc · 2025-03-18T09:55:03Z

src/lib.rs

+    /// the `DefaultClient::try_new()?` can be used as the http client by default.
+    /// The returned [`AccountCredentials`] can be serialized and stored for later use.
+    /// Use [`Account::from_credentials()`] to restore the account from the credentials.
+    #[cfg(feature = "hyper-rustls")]


Still don't really want to have both the _http() and non-_http() variants. Which one of these do you want to use?

djc · 2025-03-18T09:55:50Z

src/lib.rs

@@ -907,22 +916,22 @@ struct Key {
 }

 impl Key {
-    fn generate() -> Result<(Self, crypto::pkcs8::Document), Error> {
+    pub fn generate() -> Result<(Self, crypto::pkcs8::Document), Error> {


I'm okay with making generate() and from_pkcs8_der() public. I don't think new() and to_pkcs8_der() need to become public.

@cpu thoughts?

@cpu thoughts?

Yeah, in the absence of some additional motivation I agree 👍

djc · 2025-03-18T09:56:49Z

src/types.rs

+/// This is a special value for `NewAccount` that is supplied
+/// when loading an account from a private key.
+/// According to rfc8555 7.3.1 this field needs to be supplied,
+/// but MUST be ignored by the ACME server.
+pub(crate) static LOAD_EXISTING_ACCOUNT: NewAccount = NewAccount {
+    only_return_existing: true,
+    contact: &[],
+    terms_of_service_agreed: true,
+};


Rename this to:

impl NewAccount<'_> { pub(crate) const EXISTING: Self = Self { .. }; }

cpu · 2025-03-18T12:36:53Z

How can I run the pebble test locally?

It's a little more tricky than it should be at the moment because we're waiting on an upstream Pebble release. The basic process is:

Get pebble and pebble-challtestsrv binaries
Put them in the root of your instant-acme checkout
Run cargo test --features=x509-parser --features=time -- --ignored

For step 1, since we're presently waiting on a fresh release of Pebble you'll have to build it from source (which requires Go) for all the tests to pass. You can see how CI does it here: https://github.com/djc/instant-acme/blob/main/.github/workflows/pebble.yml

Alternatively, you could download pre-built releases here: https://github.com/letsencrypt/pebble/releases/tag/v2.7.0 but you'll see that one of the Pebble tests fails (account_deactivate) because it's missing an unreleased Pebble fix.

cpu

Cool, I think this is coming along 👍

cpu · 2025-03-21T14:16:40Z

src/lib.rs

 mod types;
+use crate::types::LOAD_EXISTING_ACCOUNT;


I think this import should be moved into the existing use types::{ ... } group on L40..43

cpu · 2025-03-21T14:17:16Z

src/lib.rs

@@ -389,6 +390,8 @@ impl Deref for ChallengeHandle<'_> {
 /// Create an [`Account`] with [`Account::create()`] or restore it from serialized data
 /// by passing deserialized [`AccountCredentials`] to [`Account::from_credentials()`].
 ///
+/// Alternatively, you can load an account using the private key using [`Account::loadgit ()`].


I think some details shifted here and Account::loadgit() isn't a fn?

Lol, i guess my cli command got there by mistake

cpu · 2025-03-21T14:17:46Z

src/lib.rs

@@ -423,6 +426,48 @@ impl Account {
        })
    }

+    /// Load a new account by private key, with a default or custom HTTP client
+    ///
+    /// https://www.rfc-editor.org/rfc/rfc8555#section-7.3.1


Suggested change

/// https://www.rfc-editor.org/rfc/rfc8555#section-7.3.1

/// <https://www.rfc-editor.org/rfc/rfc8555#section-7.3.1>

cpu · 2025-03-21T14:18:25Z

src/lib.rs

@@ -423,6 +426,48 @@ impl Account {
        })
    }

+    /// Load a new account by private key, with a default or custom HTTP client


I don't think this headline is accurate for this fn: it always uses the default HTTP client.

cpu · 2025-03-21T14:19:11Z

src/lib.rs

+    ///
+    /// https://www.rfc-editor.org/rfc/rfc8555#section-7.3.1
+    ///
+    /// the `DefaultClient::try_new()?` can be used as the http client by default.


similar to my prev comment, for this fn (if both are kept):

Suggested change

/// the `DefaultClient::try_new()?` can be used as the http client by default.

/// the `DefaultClient::try_new()?` will be used as the http client.

cpu · 2025-03-21T14:22:55Z

src/lib.rs

        let rng = crypto::SystemRandom::new();
        let pkcs8 =
            crypto::EcdsaKeyPair::generate_pkcs8(&crypto::ECDSA_P256_SHA256_FIXED_SIGNING, &rng)?;
        Self::new(pkcs8.as_ref(), rng).map(|key| (key, pkcs8))
    }

-    fn from_pkcs8_der(pkcs8_der: &[u8]) -> Result<Self, Error> {
+    /// Create a key from a der encoded ES256


I think we can be more specific here:

Suggested change

/// Create a key from a der encoded ES256

/// Create a `Key` from a DER encoded PKCS8 format ECDSA P-256 private key

cpu · 2025-03-21T14:23:21Z

src/lib.rs

        Self::new(pkcs8_der, crypto::SystemRandom::new())
    }

-    fn new(pkcs8_der: &[u8], rng: crypto::SystemRandom) -> Result<Self, Error> {
+    /// Export a key to a der encoded ES256 key


If this fn stays I think similar feedback as the from_pkcs8_der() specificity applies.

cpu · 2025-03-21T14:23:30Z

src/lib.rs

+        Ok(self.inner.to_pkcs8v1()?)
+    }
+
+    /// Create a key from a der encoded ES256 key and a crypto::SystemRandom


If this fn stays I think similar feedback as the from_pkcs8_der() specificity applies.

cpu · 2025-03-21T14:25:19Z

src/types.rs

+/// This is a special value for `NewAccount` that is supplied
+/// when loading an account from a private key.
+/// According to rfc8555 7.3.1 this field needs to be supplied,
+/// but MUST be ignored by the ACME server.


I think we can describe this a bit clearer:

Suggested change

/// This is a special value for `NewAccount` that is supplied

/// when loading an account from a private key.

/// According to rfc8555 7.3.1 this field needs to be supplied,

/// but MUST be ignored by the ACME server.

/// A `NewAccount` endpoint request payload for fetching an existing account.

///

/// `only_return_existing` is set to true to avoid creating a new account if the

/// existing account is not known to the ACME server for some reason.

cpu · 2025-03-21T14:25:49Z

tests/pebble.rs

+    use serde_json::Value;
+    use instant_acme::Key;


Let's import these top-level

axos88 · 2025-03-21T20:19:19Z

Thanks for the feedback I'll reiterate in a few days when i get some free time

add feature to load an account by private key

cfc3401

djc reviewed Mar 17, 2025

View reviewed changes

axos88 added 3 commits March 18, 2025 09:57

fix lint

b33086b

address some concerns in the pr

72f4b76

add test

0baa454

djc reviewed Mar 18, 2025

View reviewed changes

cpu reviewed Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add feature to load an account by private key #94

add feature to load an account by private key #94

axos88 commented Mar 17, 2025 •

edited

Loading

djc Mar 17, 2025

axos88 Mar 18, 2025

djc Mar 17, 2025

axos88 Mar 18, 2025

axos88 Mar 18, 2025

djc Mar 17, 2025

axos88 Mar 18, 2025

djc Mar 17, 2025

axos88 Mar 18, 2025

cpu commented Mar 17, 2025

axos88 commented Mar 18, 2025

djc left a comment

djc Mar 18, 2025

djc Mar 18, 2025

cpu Mar 21, 2025

djc Mar 18, 2025

cpu commented Mar 18, 2025

cpu left a comment

cpu Mar 21, 2025

cpu Mar 21, 2025

axos88 Mar 21, 2025

cpu Mar 21, 2025

cpu Mar 21, 2025

cpu Mar 21, 2025

cpu Mar 21, 2025

cpu Mar 21, 2025

cpu Mar 21, 2025

cpu Mar 21, 2025

cpu Mar 21, 2025

axos88 commented Mar 21, 2025

	/// https://www.rfc-editor.org/rfc/rfc8555#section-7.3.1
	/// <https://www.rfc-editor.org/rfc/rfc8555#section-7.3.1>

	/// the `DefaultClient::try_new()?` can be used as the http client by default.
	/// the `DefaultClient::try_new()?` will be used as the http client.

	/// Create a key from a der encoded ES256
	/// Create a `Key` from a DER encoded PKCS8 format ECDSA P-256 private key

add feature to load an account by private key #94

Are you sure you want to change the base?

add feature to load an account by private key #94

Conversation

axos88 commented Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Mar 17, 2025

axos88 commented Mar 18, 2025

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Mar 18, 2025

cpu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axos88 commented Mar 21, 2025

axos88 commented Mar 17, 2025 •

edited

Loading