Skip to content

add feature to load an account by private key #94

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 61 additions & 5 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use serde::de::DeserializeOwned;
use tokio::time::sleep;

mod types;
use crate::types::LOAD_EXISTING_ACCOUNT;
Comment on lines 31 to +32
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#[cfg(feature = "time")]
pub use types::RenewalInfo;
pub use types::{
Expand Down Expand Up @@ -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 ()`].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, i guess my cli command got there by mistake

///
/// The [`Account`] type is cheap to clone.
///
/// <https://datatracker.ietf.org/doc/html/rfc8555#section-7.1.2>
Expand Down Expand Up @@ -423,6 +426,48 @@ impl Account {
})
}

/// Load a new account by private key, with a default or custom HTTP client
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// 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")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub async fn for_existing_account(
key: Key,
server_url: &str,
) -> Result<(Account, AccountCredentials), Error> {
let http = Box::new(DefaultClient::try_new()?);

Self::for_existing_account_http(key, server_url, http).await
}

/// 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
///
/// 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.
pub async fn for_existing_account_http(
key: Key,
server_url: &str,
http: Box<dyn HttpClient>,
) -> Result<(Account, AccountCredentials), Error> {
let client = Client::new(server_url, http).await?;
let pkcs8 = key.to_pkcs8_der()?;

Self::create_inner(
&LOAD_EXISTING_ACCOUNT,
(key, pkcs8),
None,
client,
server_url,
)
.await
}

/// Restore an existing account from the given ID, private key, server URL and HTTP client
///
/// The key must be provided in DER-encoded PKCS#8. This is usually how ECDSA keys are
Expand Down Expand Up @@ -454,6 +499,7 @@ impl Account {
) -> Result<(Account, AccountCredentials), Error> {
Self::create_inner(
account,
Key::generate()?,
external_account,
Client::new(server_url, Box::new(DefaultClient::try_new()?)).await?,
server_url,
Expand All @@ -473,6 +519,7 @@ impl Account {
) -> Result<(Account, AccountCredentials), Error> {
Self::create_inner(
account,
Key::generate()?,
external_account,
Client::new(server_url, http).await?,
server_url,
Expand All @@ -482,11 +529,11 @@ impl Account {

async fn create_inner(
account: &NewAccount<'_>,
(key, key_pkcs8): (Key, crypto::pkcs8::Document),
external_account: Option<&ExternalAccountKey>,
client: Client,
server_url: &str,
) -> Result<(Account, AccountCredentials), Error> {
let (key, key_pkcs8) = Key::generate()?;
let payload = NewAccountPayload {
new_account: account,
external_account_binding: external_account
Expand Down Expand Up @@ -861,26 +908,35 @@ impl fmt::Debug for Client {
}
}

struct Key {
/// This struct represents a key used to sign requests to the ACME server
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Key" can be fairly ambiguous in this context :-) Let's explicitly say it's the account private key.

Suggested change
/// This struct represents a key used to sign requests to the ACME server
/// This struct represents an account private key used to sign requests to the ACME server

pub struct Key {
rng: crypto::SystemRandom,
signing_algorithm: SigningAlgorithm,
inner: crypto::EcdsaKeyPair,
thumb: String,
}

impl Key {
fn generate() -> Result<(Self, crypto::pkcs8::Document), Error> {
/// Generate a random key
pub fn generate() -> Result<(Self, crypto::pkcs8::Document), Error> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpu thoughts?

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub fn from_pkcs8_der(pkcs8_der: &[u8]) -> Result<Self, Error> {
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub fn to_pkcs8_der(&self) -> Result<crypto::pkcs8::Document, Error> {
Ok(self.inner.to_pkcs8v1()?)
}

/// Create a key from a der encoded ES256 key and a crypto::SystemRandom
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub fn new(pkcs8_der: &[u8], rng: crypto::SystemRandom) -> Result<Self, Error> {
let inner = crypto::p256_key_pair_from_pkcs8(pkcs8_der, &rng)?;
let thumb = BASE64_URL_SAFE_NO_PAD.encode(Jwk::thumb_sha256(&inner)?);
Ok(Self {
Expand Down
10 changes: 10 additions & 0 deletions src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,16 @@ pub struct NewAccount<'a> {
pub only_return_existing: bool,
}

/// 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.
Comment on lines +510 to +513
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pub(crate) static LOAD_EXISTING_ACCOUNT: NewAccount = NewAccount {
only_return_existing: true,
contact: &[],
terms_of_service_agreed: true,
};
Comment on lines +510 to +518
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this to:

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


#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub(crate) struct DirectoryUrls {
Expand Down
54 changes: 54 additions & 0 deletions tests/pebble.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,60 @@ async fn account_deactivate() -> Result<(), Box<dyn StdError>> {
Ok(())
}

/// Test account loading by private key
#[tokio::test]
#[ignore]
async fn account_from_private_key() -> Result<(), Box<dyn StdError>> {
use serde_json::Value;
use instant_acme::Key;
Comment on lines +293 to +294
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's import these top-level


try_tracing_init();

// Creat an env/initial account
let env = Environment::new(EnvironmentConfig::default()).await?;

let new_account = NewAccount {
contact: &[],
terms_of_service_agreed: true,
only_return_existing: false,
};

let directory_url = format!("https://{}/dir", &env.config.pebble.listen_address);

let (account, credentials) = Account::create(&new_account, &directory_url, None)
.await
.expect("failed to create account");

let json: Value = serde_json::to_value(&credentials).expect("failed to serialize credentials");
let pkey_encoded = json.as_object().expect("a json object").get("key_pkcs8").expect("a field key_pkcs8").as_str().expect("a string encoded value");
let pkey_der = BASE64_URL_SAFE_NO_PAD.decode(pkey_encoded).expect("a base64 encoded value");
let key = Key::from_pkcs8_der(&pkey_der).expect("an ES256 key");

let (account2, credentials2) = Account::for_existing_account(key, &directory_url).await?;

assert_eq!(account.id(), account2.id());
assert_eq!(serde_json::to_string(&credentials).expect("a serializable value"), serde_json::to_string(&credentials2).expect("a serializable value"));

// Creat a new env/initial account
drop(env);
let env = Environment::new(EnvironmentConfig::default()).await?;
let directory_url = format!("https://{}/dir", &env.config.pebble.listen_address);

let key = Key::from_pkcs8_der(&pkey_der).expect("an ES256 key");
let err = Account::for_existing_account(key, &directory_url).await.err().expect("account loading should fail for a non-existing account");

let Error::Api(problem) = err else {
panic!("unexpected error result {:?}", err);
};

assert_eq!(
problem.r#type,
Some("urn:ietf:params:acme:error:accountDoesNotExist".to_string())
);

Ok(())
}

fn try_tracing_init() {
let _ = tracing_subscriber::registry()
.with(fmt::layer())
Expand Down
Loading