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 3 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
59 changes: 55 additions & 4 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)
}

/// 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 @@ -869,18 +916,22 @@ struct Key {
}

impl Key {
fn generate() -> Result<(Self, crypto::pkcs8::Document), Error> {
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> {
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> {
pub fn to_pkcs8_der(&self) -> Result<crypto::pkcs8::Document, Error> {
Ok(self.inner.to_pkcs8v1()?)
}

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